add syncing docs workflow #237
Conversation
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This PR adds automated documentation syncing using Claude Code - great concept, but several critical issues need fixing before merge.
Critical Issues:
-
Repository name mismatch - The workflow references
cloudflare/agentsinstead ofcloudflare/sandbox-sdkin multiple places (lines 117, 136-137 of sync-docs.yml). This will create incorrect commit messages and broken PR cross-references. -
Unexplained token change - Switching from
SANDBOX_GITHUB_TOKENtoGITHUB_TOKENin release.yml needs explanation and testing. If this is unrelated to docs sync, it should be a separate PR. If the default token lacks permissions, releases will fail.
Important Issues:
-
No path filtering - This runs on every PR, wasting Actions minutes and API credits. Add path-based triggers like other workflows (e.g., pkg-pr-new.yml) or at minimum exclude
.github/workflows/**. -
No error handling - If Claude fails or token expires, there's no cleanup or notification. Add failure handling and consider branch collision scenarios.
Recommendations:
- Test manually with workflow_dispatch before enabling on all PRs
- Add error handling and cleanup steps
- Consider expanding allowed-tools to include Glob/Grep for better file discovery
The workflow structure is solid and security looks good (proper GitHub App token usage, appropriate permissions). Fix the critical issues and this will be a valuable automation.
Verdict: Needs fixes before merge.
| @@ -0,0 +1,237 @@ | |||
| name: Documentation Sync with Claude Code | |||
|
|
|||
| on: | |||
There was a problem hiding this comment.
Missing path filter: This will run on every PR, including test-only changes, workflow updates, etc. This wastes Actions minutes and Anthropic API credits.
Recommend adding path filtering like pkg-pr-new.yml does:
on:
pull_request:
types: [opened, synchronize, edited]
paths:
- 'docs/**'
- 'packages/sandbox/src/**'
- 'examples/**'
- '!**/*.test.ts'
- '!**/tests/**'Or at minimum, exclude workflow changes:
paths:
- '**'
- '!.github/workflows/**'| cd cloudflare-docs | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git checkout -b sync-docs-pr-${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
Branch collision risk: If a previous run failed, this branch may already exist and git checkout -b will fail.
Recommend making this idempotent:
# Delete local/remote branch if exists
git branch -D sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
git push origin --delete sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
# Create fresh branch
git checkout -b sync-docs-pr-${{ github.event.pull_request.number }}| prompt: ${{ steps.create-prompt.outputs.prompt }} | ||
| claude_args: '--allowed-tools "Read,Edit,Write,Bash"' | ||
| env: | ||
| GH_TOKEN: ${{ steps.generate-token.outputs.token }} |
There was a problem hiding this comment.
No error handling: Add steps to handle failure:
- Cleanup step:
- name: Cleanup on failure
if: failure()
run: |
cd cloudflare-docs
git checkout main
git branch -D sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
git push origin --delete sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true- Notification comment:
- name: Comment on failure
if: failure()
uses: actions/github-script@v7
with:
script: |
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: '⚠️ Documentation sync failed. Check the [workflow logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})'
});| publish: npx tsx .github/changeset-publish.ts | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.SANDBOX_GITHUB_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Unexplained token change: Why switch from SANDBOX_GITHUB_TOKEN to default GITHUB_TOKEN?
Questions:
- Does default token have sufficient permissions for changesets action?
- Was
SANDBOX_GITHUB_TOKENa GitHub App token with broader permissions? - Why bundle this with docs sync instead of separate PR?
If the default token lacks permissions (e.g., for creating "Version Packages" PR), releases will fail. This should be tested separately.
There was a problem hiding this comment.
Claude Code Review
The hardcoded repository references are now fixed, but critical issues remain that will cause production failures.
CRITICAL - Branch collision (line 50): When this PR is updated (synchronize event), git checkout -b fails because the branch already exists. Need idempotent branch creation:
git fetch origin
git branch -D sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
git push origin --delete sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
git checkout -b sync-docs-pr-${{ github.event.pull_request.number }}CRITICAL - Missing path filter (lines 3-5): This runs on EVERY PR including test-only changes and workflow updates. Wastes Claude API calls and CI resources. Add:
on:
pull_request:
types: [opened, synchronize, edited]
paths:
- 'docs/**'
- 'packages/*/README.md'
- 'examples/**/*.md'
- 'packages/*/src/**/*.ts'IMPORTANT - No cleanup on failure: Failed runs leave orphaned branches in cloudflare-docs. Add cleanup step with if: always().
Release.yml token change (line 212): Switching from SANDBOX_GITHUB_TOKEN to default GITHUB_TOKEN may break releases if the custom token had elevated permissions. This change appears unrelated to docs sync - needs explanation or revert.
Verdict: DO NOT MERGE - Issues #1 and #2 will cause immediate workflow failures.
| cd cloudflare-docs | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git checkout -b sync-docs-pr-${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
Branch creation fails on PR updates because branch already exists from first run. Make idempotent:
git fetch origin
git branch -D sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
git push origin --delete sync-docs-pr-${{ github.event.pull_request.number }} 2>/dev/null || true
git checkout -b sync-docs-pr-${{ github.event.pull_request.number }}|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, edited] |
There was a problem hiding this comment.
Missing path filter means this runs on every PR (tests, workflows, etc). Add paths filter to only trigger on documentation-relevant changes.
| anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} | ||
| github_token: ${{ steps.generate-token.outputs.token }} | ||
| prompt: ${{ steps.create-prompt.outputs.prompt }} | ||
| claude_args: '--allowed-tools "Read,Edit,Write,Bash"' |
There was a problem hiding this comment.
No cleanup step. Failed runs leave garbage branches in cloudflare-docs repo. Add cleanup with if: always() condition.
| Copy and run this EXACT script (replace PLACEHOLDER values with actual values from Context): | ||
| ```bash | ||
| # Set variables from Context section above |
There was a problem hiding this comment.
Placeholder replacement is fragile. If Claude fails to replace PLACEHOLDER values, commands silently fail with cryptic errors. Consider passing values via environment variables instead.
| publish: npx tsx .github/changeset-publish.ts | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.SANDBOX_GITHUB_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Why change tokens? If SANDBOX_GITHUB_TOKEN had elevated permissions (e.g., to trigger workflows), default GITHUB_TOKEN will break releases. This change seems unrelated to docs sync.
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-237-abbeb6cVersion: You can use this Docker image with the preview package from this PR. |
1 participant
No description provided.