Repository navigation
Conversation
Record the design and implementation plan for issue #1105. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Add a Documentation proposal workflow that runs GitHub Copilot CLI on each code merge to main, proposes edits to docs/guide or docs/index.md, and opens one pull request per merge on docs/auto/<sha12>. The publish job runs the docs gates first, skips empty diffs, and never reopens a closed proposal. It also posts a review comment on every hunk that cites the merged source and offers a suggestion reverting the hunk. Closes #1105. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Changes are needed to keep generated content away from publication credentials, preserve queued merges, and recover missing reviews after partial publication failures.
One inline comment includes a one-click suggestion; the other two describe coordinated fixes to apply manually.
Blocking
- 🔧 [P1] Validate generated documentation without publication credentials — see inline at scripts/docs-proposal/publish.sh:46.
- 🔧 [P2] Preserve pending merge runs — see inline at .github/workflows/docs-proposal.yml:29 (suggestion).
- 🔧 [P2] Resume review publication after a partial failure — see inline at scripts/docs-proposal/publish.sh:61–64.
Validation
The 16 hunk tests, existing shell integration tests, and shellcheck pass locally. A harmless allowed Markdown SSR probe passed docs lint, format, and build while reading a dummy process environment variable. A temporary failure/retry fixture reproduced missing review recovery. The proposed concurrency YAML parses; queue: max is documented by GitHub. A real Copilot run has not been exercised.
CI Status
All 21 reported checks passed when queried during this review. A later refresh encountered a reviewer-side network failure.
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- Analyze (javascript-typescript): PASS
- format-typescript: PASS (required)
- docs-proposal scripts: PASS
- Analyze (rust): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo fmt: PASS (required)
- CLAUDE.md symlink guard: PASS
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- Analyze (javascript-typescript): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (actions): PASS
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed f70ce72181e839b0bd0d2c9a93bfb56a66694c21 against cb945254521ed6dab365072f7caec10c524e323b. All 14 changed files and the proposal-to-publication path were inspected. Two additional P2 findings are posted inline. Existing feedback was checked and is not duplicated.
Validation
node --test scripts/docs-proposal/hunks.test.mjs: all 16 passed.scripts/docs-proposal/test.sh: passed.bash -n scripts/docs-proposal/*.sh: passed.git diff --check cb945254521ed6dab365072f7caec10c524e323b...f70ce72181e839b0bd0d2c9a93bfb56a66694c21: passed.- Docs lint, format, and build passed in temporary copies.
- A scratch check exercised 300 real Git diffs; 691 revert suggestions restored the original text.
- All 21 reported CI checks passed. Shellcheck passed in CI and was unavailable locally.
Real Copilot authentication, organization prerequisites, live workflow publication, and GitHub-side suggestion application remain untested. Repository files were not modified and no review work was delegated.
Split the docs gates into a validate job with a read-only token and no persisted credentials, since VitePress executes Vue in generated Markdown. The publish job now re-checks the path allowlist on a fresh checkout without building. Every job runs the scripts from the workflow's revision against a separate checkout of the merge commit, so historical merges can be retried. Queue every pending run instead of cancelling all but one, and have Copilot inspect the full pushed range so merge commits and multi-commit pushes are not missed. A retry with an unchanged tree now reuses the pushed commit and posts the review if an earlier run failed before posting it. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Second pass at ae8d9c5. The earlier findings from both reviews (credential split, queued merges, review recovery, merge-commit diffing, tooling checkout) are addressed in f22e366a, and I re-checked them. One new blocking issue: the retry path force-pushes over maintainer commits on the proposal branch, which the design itself asks maintainers to make. There's also a hardening gap in the Copilot tool allowlist.
Blocking
- 🔧 [P1] Retries force-push over maintainer commits. See inline at
scripts/docs-proposal/publish.sh:57-62(suggestion). Reproduced.
Should fix
- 🤔 [P2]
shell(git grep:*)allows running any command via--open-files-in-pager. See inline atscripts/docs-proposal/propose.sh:39(suggestion). Reproduced. - 🤔 [P2] Drop the npm cache in the Copilot job. See inline at
.github/workflows/docs-proposal.yml:72-73(suggestion). - 🤔 [P2] Rationale goes into the PR body without
@-mention stripping. See inline atscripts/docs-proposal/lib.sh:66-68.
Nits / notes
- ⛏ No
timeout-minuteson the Copilot job. See inline at.github/workflows/docs-proposal.yml:44(suggestion). - 📝 Merging this PR triggers the workflow itself (
scripts/README.mdmatchesscripts/**). The first run will fail unless the org Copilot policy and the "Allow GitHub Actions to create pull requests" setting are already on. - 📝 Self-trigger loop: none. Proposal PRs touch only
docs/guide/**/docs/index.mdand are opened withGITHUB_TOKEN. - 📝 The
AGENTS.md"CI Gates" list doesn't mention the newdocs-proposal scriptsjob. - 📝 The PR body says actionlint is clean except
copilot-requests, but it also flagsconcurrency.queue. Both are actionlint false positives. - ⛏
scripts/README.mdsays "runs both proposal scripts", but there are three (propose,validate,publish). - ⛏
prompt.md/evidence-prompt.mdhardcode.docs-proposal/, whilepropose.shaccepts any work dir. - 🌱 Cost: nearly every merge touches
crates/**, so each runs Copilot twice. Consider skipping dependency-bump and bot merges.
Validation
scripts/docs-proposal/test.sh: passnode --test scripts/docs-proposal/hunks.test.mjs: 16/16 passshellcheck scripts/docs-proposal/*.sh(0.11.0): cleanactionlint1.7.12: only the two false positives above- Force-push repro against a bare repo: maintainer commit lost on rerun; kept with the suggested fix
- CI: all 21 checks pass
A real Copilot run and a live publication remain untested.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed ae8d9c5a9ff581cfe462745bb6970b5a5277cdbe against 80483011e30a446ac741b4423e36f8d142c5d2ac. All 15 changed files and the proposal-to-publication flow were inspected. One additional P2 finding is posted inline. Existing branch-overwrite and hardening findings are not repeated.
Safety proof
- Publication rechecks the path allowlist and does not invoke the docs gates. Executed evidence: the script integration tests rejected forbidden changes before publication and successful publication invoked no npm commands. Proven for the exercised script paths.
- Generated revert suggestions restore the original changed text. Executed evidence: all 16 hunk tests passed, and a scratch check applied 506 generated revert suggestions across 300 real Git diffs, restoring the original lines in every case. Proven for the tested text cases.
Validation and review context
node --test scripts/docs-proposal/hunks.test.mjs: 16/16 passed.scripts/docs-proposal/test.sh: passed.for script in scripts/docs-proposal/*.sh; do bash -n "$script" || exit; done: passed.git diff --check 80483011e30a446ac741b4423e36f8d142c5d2ac...ae8d9c5a9ff581cfe462745bb6970b5a5277cdbe: passed.- Scratch Node and Python checks: revert round trips passed; the manual-retry defect was reproduced using the real proposal, validation, and publication scripts with Copilot, npm, and GitHub calls stubbed.
- CI: all 21 reported checks passed, with no failures or skipped checks. Shellcheck was unavailable locally but passed in CI.
- Existing reviews, inline comments, replies, issue comments, and thread resolution states were inspected.
- Real Copilot authentication, live workflow isolation/publication, and GitHub-side suggestion application remain untested.
- Revision remained unchanged; worktree stayed clean. No files modified or review work delegated.
A retry for the same merge regenerated the proposal and force-pushed it, discarding commits maintainers added on top, including applied revert suggestions. An empty rerun also closed the pull request and deleted that branch. publish.sh now leaves a branch alone when its head is not the generated proposal commit, and pushes with a lease instead of a bare force. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
git grep --open-files-in-pager (or -O) runs an arbitrary command, so allowing shell(git grep:*) let content in the merged change steer Copilot into executing commands in the propose job. Copilot's built-in file reads cover what git grep was used for. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Copilot can write files in the propose job that propose.sh then executes, and validate builds generated Markdown that VitePress runs as Vue. Either job could therefore save a poisoned main-scoped setup-node cache entry that later runs restore. Neither job restores or saves a dependency cache now, and the design spells out that the job, not the tool allowlist, is the trust boundary. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Copilot CLI has no limit of its own, so a hung run inherited the 360-minute default and, with the concurrency queue, delayed every later merge's proposal. The propose, validate, and publish jobs now time out after 30, 15, and 10 minutes. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Evidence already had its @ mentions broken because it is model output, but up to 20 KB of Copilot's rationale went into the pull request body verbatim, so a rationale naming a team would ping it on every proposal. hunks.mjs gains a sanitize subcommand that inserts a zero width space after each @, which the PR body helper now pipes the rationale through. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
A manual dispatch had no base, so retrying a multi-commit push such as a rebase merge narrowed the inspected range to the final commit. The retry could then miss documented behavior, or produce an empty patch that closed and deleted a valid proposal. The dispatch gains an optional base input. propose.sh records the resolved base in the artifact and warns when it discards a supplied base. publish.sh records the base in the pull request body and fails, naming the original base, when an open proposal was generated from a different range. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
AGENTS.md now lists the docs-proposal scripts job among the CI gates. scripts/README.md names all three scripts the workflow runs, and warns that the run triggered by merging the workflow fails unless the Copilot policy and pull request setting are already enabled. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
propose.sh accepts any work directory, but both prompts told Copilot to write its outputs under .docs-proposal/. The prompts now use a <work-dir> placeholder that propose.sh replaces with the work directory relative to the repository root. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…tomation Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed b3e3b22066d8a72b6ef767e5beab272c0f016d10 against 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1, from feat/docs-proposal-automation into main. Inspected all 16 changed files and traced generation, artifacts, validation, publication, reviews, and retries. One P2 finding is posted inline.
Safety proof
Publication rejects forbidden paths without running the docs build. Executed evidence: scripts/docs-proposal/test.sh passed; forbidden changes were rejected through lib.sh:48 before publication, and successful publication invoked no npm commands. Proven for the exercised script paths.
Validation and review context
node --test scripts/docs-proposal/hunks.test.mjs: 17/17 passed.scripts/docs-proposal/test.sh: passed.for script in scripts/docs-proposal/*.sh; do bash -n "$script" || exit; done: passed.shellcheck scripts/docs-proposal/*.sh, version 0.11.0: passed.git diff --check 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1...b3e3b22066d8a72b6ef767e5beab272c0f016d10: passed.- In a scratch archive:
npm ci --no-audit --no-fund && npm run lint && npm run format && npm run build: passed. Changed Markdown outsidedocs/also passed Prettier. - Scratch revert verification: 644 suggestions across 299 real Git diffs restored the original text. An initial fixture failure came from local nonstandard diff prefixes; isolated Git configuration passed.
- Actionlint 1.7.12 reported unsupported but documented
queueandcopilot-requestskeys, plus unquoted-output ShellCheck warnings informat.yml. No behavioral finding arose from those diagnostics. - CI: all 23 reported checks passed at the reviewed head.
- Existing feedback: inspected review bodies, comments, replies, and all 11 resolved threads. The inline finding adds concurrent-push and fetch-failure evidence beyond the earlier branch-overwrite feedback.
- Residual risk: real Copilot authentication, live workflow isolation/publication, and GitHub-side suggestion application remain untested.
- Repository unchanged; no review work delegated.
An empty rerun deleted the proposal branch via gh pr close --delete-branch, so a maintainer push after the head inspection was lost, and a failed fetch hid existing maintainer commits. Look up the branch with git ls-remote --exit-code and fail unless it is verifiably absent, delete it with --force-with-lease on the inspected head, and close the pull request only after the deletion succeeds. Add regressions for a concurrent maintainer push, fetch failures on the empty and push paths, and the lease-guarded deletion. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The design holds up well: Copilot runs in a job whose token can't publish, the allowed docs paths are re-checked downstream, and the revert suggestions are built from the diff rather than from model output. One blocking issue remains: the proposal-PR lookup also matches PRs from forks. Four non-blocking follow-ups cover rationale rendering, cache writes, and two edge cases.
2 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them. The others describe the fix in prose because the change spans several files or several places in a file.
Blocking
🔧 wrench
- Proposal lookup matches fork PRs with the same branch name: see inline at
scripts/docs-proposal/publish.sh:29
Non-blocking
🤔 thinking
- Rationale still renders closing keywords,
#Nreferences and links: see inline atscripts/docs-proposal/lib.sh:72 - The propose job can still write a cache that
deploy-docsrestores: see inline atscripts/docs-proposal/propose.sh:67
⛏ nitpick
- The revert suggestion for a new page leaves an empty file: see inline at
scripts/docs-proposal/hunks.mjs:227 - A non-SHA
baseinput is only rejected at publish time: see inline atscripts/docs-proposal/propose.sh:29
👍 praise
- The revert suggestion is built from the diff, not from model output, and its fence grows to fit any backtick run.
--force-with-leaseeverywhere, including the empty-lease "must not exist" case and the delete path.- An unchanged tree reuses the earlier commit, so a retry resumes a review that was never posted.
- The allowed-paths check fails closed:
--no-renamesis set, and quoted paths (non-ASCII, newline, traversal) never match the regex.
Cross-cutting / body-level findings
- 📝 actionlint errors are false positives: actionlint 1.7.12 rejects
concurrency.queue: maxand thecopilot-requests: writepermission, but both are valid GitHub syntax. No change needed. - 📝 Test count:
hunks.test.mjshas 17 tests (all pass locally); the PR description says 16.
Verified locally at 0bc4bf7a: node --test scripts/docs-proposal/hunks.test.mjs (17/17), bash scripts/docs-proposal/test.sh (pass), shellcheck scripts/docs-proposal/*.sh (clean).
CI Status
All 23 checks pass, including docs-proposal scripts, Analyze (actions), CodeQL, cargo fmt (required), cargo test (required), format-typescript (required) and format-docs (required).
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 0bc4bf7a13eca8e68275055996a547fb171b74d0 against 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1, from feat/docs-proposal-automation into main. Inspected all 16 changed files and traced generation, artifacts, validation, publication, retries, and review suggestions.
No new, non-duplicate actionable findings. This approval records the result of this pass; it does not establish that existing unresolved feedback has been addressed.
Safety proof
- Publication rejects forbidden paths without running the docs build. Executed evidence:
scripts/docs-proposal/test.shpassed, exercising rejection throughlib.sh:45–49and verifying successful publication invokes no npm commands. Proven for the exercised script paths. - Added maintainer commits survive retries and raced deletion. Executed evidence: the script tests exercise
publish.sh:60–71against a temporary bare repository. Maintainer commits remain intact, concurrent deletion fails under the lease, and fetch failures prevent publication or closure. Proven for the exercised cases.
Validation and review context
node --test scripts/docs-proposal/hunks.test.mjs: 17/17 passed.scripts/docs-proposal/test.sh: passed./home/pav/.local/share/mise/installs/shellcheck/0.11.0/shellcheck-v0.11.0/shellcheck scripts/docs-proposal/*.sh: passed.for script in scripts/docs-proposal/*.sh; do bash -n "$script" || exit; done: passed.git diff --check 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1...0bc4bf7a13eca8e68275055996a547fb171b74d0: passed.- In a temporary archive,
npm ci --no-audit --no-fund && npm run lint && npm run format && npm run build: passed. Changed Markdown outsidedocs/also passed Prettier. - Inline Python/Node round-trip check: 686 suggestions across 300 real Git text diffs restored the original lines.
- Pinned
@github/copilot@1.0.90 --help: confirmed the invoked flags. - Actionlint 1.7.7 reported unsupported
queueandcopilot-requestskeys, both confirmed in current GitHub documentation, plus unquoted-output ShellCheck warnings. No behavioral finding arose from these diagnostics. - CI: all 23 reported checks passed. The format workflow's head SHA matches the reviewed revision.
- Existing feedback: inspected reviews, inline comments, replies, issue comments, and thread resolution states. Five threads remain unresolved, covering fork-PR selection, rationale rendering, cache writes, new-page reverts, and base-input normalization. Existing findings are not duplicated.
- Residual risk: real Copilot authentication, live workflow isolation/publication, cache isolation, and GitHub-side suggestion application remain unverified. Rust and browser suites were not rerun locally.
- Revision unchanged; worktree clean. No repository files modified or review work delegated.
aram356
left a comment
There was a problem hiding this comment.
Summary
Reviewed 0bc4bf7a1 against main (merge base 7a0ecb4cb). One blocking gap: the path allowlist checks paths but not file modes, so a symlink under docs/guide/ passes both checks and VitePress publishes its target. The rest are smaller fixes to the empty-rerun close path, test coverage, local portability of the new CI gate, and docs that trail the code.
7 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them. The other 2 describe the fix in prose because it changestest.shin more than one place or together withpublish.sh.
Blocking
🔧 wrench
- The path allowlist admits symlinks, so a proposal can publish pages the site excludes: see inline at
scripts/docs-proposal/lib.sh:51
Non-blocking
🤔 thinking
- An empty rerun probably closes the proposal without its explanation: see inline at
scripts/docs-proposal/publish.sh:72-73 - CI never runs the evidence half of
propose.sh: see inline atscripts/docs-proposal/test.sh:92-95 - A failed evidence run discards a finished proposal: see inline at
scripts/docs-proposal/propose.sh:84 - The committed plan still prescribes the designs the reviews removed: see inline at
docs/superpowers/plans/2026-10-01-docs-proposal-automation.md:3 - Proposal PRs can't merge until someone starts CI by hand: see inline at
scripts/README.md:36-38
♻️ refactor
render_promptwrites".docs-proposal"/rationale.mdunder bash 3.2: see inline atscripts/docs-proposal/propose.sh:38-42- The mention assertion depends on the caller's locale: see inline at
scripts/docs-proposal/test.sh:50
⛏ nitpick
- The spec's trigger bullet predates the
baseinput andvalidate.sh: see inline atdocs/superpowers/specs/2026-10-01-docs-proposal-automation-design.md:50-52
Cross-cutting / body-level findings
- 🌱 Check that cited sources exist. Model citations are the weakest input to the per-hunk review.
publish.shcould rungit cat-file -e "$sha:<path>"on each evidencesource, and check the line number against the file's length, then mark misses as "cited source not found" instead of passing them through. Not for this PR. - 📝 The PR description trails the code. It says "New two-job workflow" (there are three jobs now:
propose,validate,publish), leavesvalidate.shout of the Changes table, and says 16 of 16 hunk tests pass (there are 17). - 📝 Not repeated here: prk-Jr's five open threads at this head (fork-PR lookup, rationale rendering, cache writes, new-file revert,
basenormalization) still apply.
Validation
node --test scripts/docs-proposal/hunks.test.mjs: 17/17 pass.shellcheck scripts/docs-proposal/*.sh: clean.scripts/docs-proposal/test.sh: passes on Linux bash 5.2 withLANG=C.UTF-8(Dockernode:24, matching the runners); 1 failure withoutLANG; 3 failures under macOS/bin/bash3.2.57.- Symlink probe:
docs_proposal_applyaccepts a120000entry.npm ci, lint, format, and build run on the docs tree at this head show the symlinkedsuperpowers/**spec published atdist/guide/zz-probe.html. - Every suggestion was applied alone to a clean checkout of this head and verified with
shellcheck, the hunk tests, andtest.shon all three shells (or the Prettier gate for Markdown), with no drift between the applied bytes and the verified tree. All of them applied together, plus the two prose fixes, pass on all three shells. - Each proposed test fails without its fix: the symlink case fails both assertions without the
lib.shchange, the evidence test fails three assertions against a brokenhunks.mjs listcall, and the new close assertion fails without thepublish.shchange. concurrency.queue: maxis documented GitHub syntax (up to 100 pending runs), so actionlint's error there is a false positive.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
…onale Ignore fork pull requests that reuse the proposal branch name, reject non-regular files such as symlinks, and render the rationale as an inert text block. Comment before closing an emptied proposal, keep a proposal when the evidence run fails, resolve a manual base to a full SHA, and fill prompts portably across bash versions. Ask for deletion instead of an empty suggestion on new pages, drop the npm cache from the Pages deploy, mark the original plan superseded, and cover the evidence half, symlinks, and fork pull requests in the script tests. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…tomation Signed-off-by: dhruv8sh <dhruv8sh@proton.me> # Conflicts: # AGENTS.md
prk-Jr
left a comment
There was a problem hiding this comment.
Approving. The three-job trust split holds up: Copilot only gets a read-only token, the VitePress build runs without write credentials, and publish re-checks the path allowlist and the regular-file rule on a fresh checkout without executing proposal content. Retry handling (lease pushes, base-mismatch guard, maintainer-commit detection, never reopening closed proposals, ignoring fork PRs) looks correct, and deploy-docs.yml was the only privileged workflow with a cache, so dropping it closes that path.
One medium item I'd like fixed before merge (evidence reason rendered as live Markdown), plus a few low-severity notes inline.
Nit, description only: "The publish job runs the docs gates itself" should say the validate job runs them.
| const source = cited?.source | ||
| ? `\`${cited.source}\`` | ||
| : "none cited — verify this hunk against the merged code."; | ||
| const reason = cited?.reason ? ` — ${cited.reason}` : ""; |
There was a problem hiding this comment.
Medium: reason is posted as live Markdown. sanitize() strips backticks and newlines and breaks @ mentions, but links, images, raw HTML and #N references survive. With evidence whose reason is See #1 and [click](https://evil.example.com)  <img src=x>, the review comment body comes out as:
**Source:** `x.rs:1` — See #1 and [click](https://evil.example.com)  <img src=x>
#N references also add backlinks to the referenced issues. The rationale is already rendered inert, so this is the one model-text path that isn't. Suggest wrapping reason in inline code like source (backticks are already stripped), or escaping Markdown/HTML, plus a test case.
| --allow-tool='shell(git show:*)' \ | ||
| --allow-tool='shell(git diff:*)' \ |
There was a problem hiding this comment.
Low: shell(git diff:*) and shell(git log:*) also allow --output=<path>, which writes an arbitrary file, including under ../tools. Combined with --allow-tool=write, Copilot could also add diff.external/textconv config to .git/config, so it can effectively run code in this job. The design already treats propose as untrusted and the artifact is re-checked downstream, so nothing breaks, but the comment above ("writes are allowed so Copilot can edit pages") understates it. Worth stating that propose should be assumed to allow arbitrary code execution.
| cd "$root" | ||
| docs_proposal_require_sha "$sha" | ||
| branch="$(docs_proposal_branch "$sha")" | ||
| base="$(cat "$work_dir/base")" |
There was a problem hiding this comment.
Low: base comes from the artifact, and Copilot can write anywhere in .docs-proposal/, including overwriting this file after propose.sh writes it. The regex check only guarantees it's a 40-character SHA, so a crafted value could fake the range the retry guard compares. Low impact; optionally also check git merge-base --is-ancestor "$base" "$sha" here.
| run: echo "node-version=$(grep '^nodejs ' .tool-versions | awk '{print $2}')" >> $GITHUB_OUTPUT | ||
|
|
||
| # No dependency cache: the docs-proposal workflow runs generated content | ||
| # on the main ref, where it could save a poisoned entry under this key. |
There was a problem hiding this comment.
Low: Untrusted code in propose/validate runs on the main ref, so it can still poison other main-scoped caches (rust-cache and npm in test.yml/format.yml, restored by every PR). Those have read-only tokens and no secrets, so the impact is CI integrity only today. A future privileged workflow that adds a cache would be exposed, though. Consider a note in docs-proposal.yml or the design doc saying privileged workflows must not restore caches.
| node-version: ${{ steps.node-version.outputs.node-version }} | ||
|
|
||
| - name: Install Copilot CLI | ||
| run: npm install -g @github/copilot@1.0.90 |
There was a problem hiding this comment.
Low: Only the top-level package is pinned; npm install -g resolves its transitive dependencies freshly on every run, in a job holding a token. Optional: install from a committed lockfile (e.g. a tiny package.json + package-lock.json with npm ci).
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed a40b31933 against main (merge base 695ab669a). All nine findings from my previous review are fixed in a7ad11de, and I re-ran their probes at this head. Two small follow-ups remain.
Both inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch to apply both at once).
Blocking
🔧 wrench
- A page Copilot creates never reaches the sidebar: see inline at
scripts/docs-proposal/prompt.md:36-37 - Without
jq, the fork tests fail with assertion noise instead of naming the missing tool: see inline atscripts/docs-proposal/test.sh:9
Cross-cutting / body-level findings
- 📝 Previous findings, re-verified at this head: the round-1 symlink patch is now rejected as "not regular files"; breaking the
hunks.mjs listcall inpropose.shnow fails 6 assertions instead of passing;test.shpasses under macOS bash 3.2 and under bash 5.2 with and withoutLANG; the plan carries the superseded banner; the empty-rerun path comments before it closes. - 📝 Not repeated here: prk-Jr's open items at this head (evidence
reasonrendered as live Markdown,git diff --output,baseread from the artifact, othermain-scoped caches, transitive dependencies ofnpm install -g, and the description's "publish job runs the docs gates").
Validation
node --test scripts/docs-proposal/hunks.test.mjs: 19/19 pass.shellcheck scripts/docs-proposal/*.sh: clean.scripts/docs-proposal/test.shat this head: passes on bash 5.2 withjq(withLANG=C.UTF-8and withoutLANG) and on macOS/bin/bash3.2.57; fails 4 assertions wherejqis missing.- Both suggestions were applied alone and together to a clean checkout of this head and pass
test.shon all three shells; the prompt change also passes the root Markdown Prettier gate, and thetest.shchange passesshellcheck. No drift between the applied bytes and the verified tree. - The merges from
mainresolve cleanly: AGENTS.md numbers the new gate 10 aftermain's gate 9, and theformat.ymljob is intact.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| - Do not edit files outside `docs/guide/`, `docs/index.md`, and | ||
| `<work-dir>/`. Do not commit, push, or create branches. |
There was a problem hiding this comment.
🔧 wrench: A page Copilot creates never reaches the sidebar
The sidebar in docs/.vitepress/config.mts lists every guide page by hand (48 /guide/ links), and that file is outside the allowlist, so a proposal can't add a new page to it. A new page still builds and shows up in local search, but it has no sidebar entry or previous/next links, and nothing in the proposal says so. a7ad11d gave new files their own review comment, so they're an expected case. Telling Copilot about the limit keeps the rationale honest about the follow-up:
| - Do not edit files outside `docs/guide/`, `docs/index.md`, and | |
| `<work-dir>/`. Do not commit, push, or create branches. | |
| - Do not edit files outside `docs/guide/`, `docs/index.md`, and | |
| `<work-dir>/`. Do not commit, push, or create branches. | |
| - Prefer extending an existing page. The sidebar is in | |
| `docs/.vitepress/config.mts`, which you cannot edit, so when a new page is | |
| unavoidable, say in the rationale that it needs a sidebar entry. |
If you'd rather not depend on the model following that, the new-file comment in hunks.mjs (line 214) could say it instead, with the matching expectation in hunks.test.mjs.
|
|
||
| here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| # shellcheck source=scripts/docs-proposal/lib.sh | ||
| source "$here/lib.sh" |
There was a problem hiding this comment.
🔧 wrench: Without jq, the fork tests fail with assertion noise instead of naming the missing tool
The gh stub now pipes STUB_PR_JSON through jq. In the node:24 image, which has no jq, the stub exits 127, publish.sh stops at the existing= lookup, and the run reports 4 assertion failures ("should skip a newer fork PR with the proposal branch name", "should update the same-repository proposal", "should ignore a fork PR when no proposal exists", "should open the proposal despite a fork PR") that read like regressions. CI's runners ship jq and scripts/README.md lists it, but AGENTS.md gate 10 doesn't, so local runs are where this shows up. Checking up front turns it into one clear line:
| source "$here/lib.sh" | |
| source "$here/lib.sh" | |
| # The gh stub runs the real --jq filters through jq. | |
| if ! command -v jq > /dev/null; then | |
| printf 'test.sh needs jq on PATH for its gh stub.\n' >&2 | |
| exit 1 | |
| fi |
With this applied, shellcheck is clean, the suite passes wherever jq is installed, and without jq it prints that line and exits 1.
Brings #1225 at c3a33e6. Integration decisions: - .gitignore, AGENTS.md and scripts/README.md keep both sides: #1254's lint-domains gate stays CI gate 10 and #1225's documentation proposal scripts become gate 11; the scripts table keeps #1257's check-ci-node-pin.py row beside #1225's docs-proposal rows. - The lint-domains job from #1254 now uses #1259's setup-rust action and pins Node before cargo, so #1257's check-ci-node-pin gate passes. The two PRs merged cleanly but the combination failed that gate.
Brings #1226 at 5d423a2. Integration decisions: - AGENTS.md lists #1226's wasm clock lint as CI gate 12, after #1254's lint-domains (10) and #1225's documentation proposal scripts (11). - The integration fixture builder keeps #1226's table-customizing app_config_envelope and gains #1107's trace flag as a parameter; integration_app_config_envelope_with_trace and #1226's auction fixture both build on it. - The Cloudflare environment keeps #1226's spawn_with_config_json and shared wrangler launcher, and threads #1107's readiness probe through it, so spawn_with_readiness and the default spawn path still use their own readiness checks. - scripts/lint-wasm-clock.sh passes across the RC's Cloudflare build.
Summary
mainthat change documented behavior currently rely on the author remembering to updatedocs/. A newDocumentation proposalworkflow runs GitHub Copilot CLI on each such merge and opens one reviewable PR per merge (docs/auto/<sha12>), never committing tomain.validatejob applies the patch, enforces adocs/guide/**/docs/index.mdallowlist (regular files only, so no symlinks), and runs the docs lint, format, and build gates. Only then does a thirdpublishjob, which never builds or executes proposal content, re-check the allowlist and publish. Empty diffs open no PR, a retry updates the same branch and PR, and a closed proposal is never reopened.suggestionthat reverts the hunk in one click (a hunk that creates a new page says to delete the file instead). The revert is built from the diff itself, never from model output, so it is always exact.Changes
.github/workflows/docs-proposal.ymlproposehascontents: readandcopilot-requests: write.validatehascontents: read.publishhascontents: writeandpull-requests: write. Runs one merge at a time, and a manual run with a fullsharetries a merge..github/workflows/format.ymldocs-proposal-scriptsjob runningshellcheck, the Node tests, and the script tests on every PR.github/workflows/deploy-docs.ymlAGENTS.mdscripts/docs-proposal/hunks.mjs@mentions are stripped. The rationale renders as an inert fencedtextblock, so links, HTML,#Nreferences and closing keywords stay inactive.scripts/docs-proposal/hunks.test.mjsnode:testcoverage for parsing, ids, every revert-anchor case, new-file hunks, evidence sanitizing, the inert rationale block, and the review payloadscripts/docs-proposal/propose.shscripts/docs-proposal/validate.shscripts/docs-proposal/publish.shscripts/docs-proposal/lib.shscripts/docs-proposal/prompt.md,evidence-prompt.mdexample.comvaluesscripts/docs-proposal/test.shghandnpm. Covers empty diff, stale-proposal close, disallowed path, symlinks, fork PRs, first publish, unchanged retry, updated proposal, closed proposal, the evidence run and its failure, a base revision expression, and an&in the work dir.scripts/README.md.gitignore.docs-proposal/work dirdocs/superpowers/specs/2026-10-01-docs-proposal-automation-design.md,docs/superpowers/plans/2026-10-01-docs-proposal-automation.mdCloses
Closes #1105
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run. All 1185 pass on the pinned Node 24 behavior. Locally on Node 26 they needNODE_OPTIONS=--no-experimental-webstorage, because Node's built-inlocalStorageshadows jsdom's. That failure is unrelated to this PR.cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1(no Rust changes)fastly compute serve(not applicable)node --test scripts/docs-proposal/hunks.test.mjs: 19 of 19 pass.scripts/docs-proposal/test.shpasses, including withLANG/LC_ALLunset. A mutation check confirmed it fails when either the unchanged-retry guard or the path allowlist is disabled.shellcheck0.11.0 is clean on all four scripts.actionlint1.7.12 is clean except two false positives:copilot-requestsandconcurrency.queue. Its latest release (2026-03) predates thecopilot-requestspermission, which GitHub's Copilot CLI Actions docs document, and it doesn't know theconcurrency.queuekey.@github/copilot@1.0.90 --help.propose.shwas dry-run in a temporary clone with stubbedcopilotandnpm.cd docs && npm run buildpasses.Not yet exercised: a real Copilot run, which cannot happen locally. After merge, run the workflow manually with a recent merge SHA to validate end to end.
Before the first run, a maintainer must enable:
Proposal PRs are opened with
GITHUB_TOKEN, so they don't trigger other workflows. The publish job runs the docs gates itself.Checklist
unwrap()in production code — useexpect("should ...")(no Rust changes)logmacros (notprintln!) (no Rust changes)