Skip to content

Propose documentation updates after code merges to main - #1225

Open
dhruv8sh wants to merge 18 commits into
mainfrom
feat/docs-proposal-automation
Open

dhruv8sh wants to merge 18 commits into
mainfrom
feat/docs-proposal-automation

Conversation

@dhruv8sh

@dhruv8sh dhruv8sh commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Code merges to main that change documented behavior currently rely on the author remembering to update docs/. A new Documentation proposal workflow runs GitHub Copilot CLI on each such merge and opens one reviewable PR per merge (docs/auto/<sha12>), never committing to main.
  • Copilot runs in a job whose token cannot push or open PRs. A second read-only validate job applies the patch, enforces a docs/guide/** / docs/index.md allowlist (regular files only, so no symlinks), and runs the docs lint, format, and build gates. Only then does a third publish job, 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.
  • Reviewers get one review comment per hunk. It cites the merged source Copilot used and carries a suggestion that 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

File Change
.github/workflows/docs-proposal.yml New three-job workflow. propose has contents: read and copilot-requests: write. validate has contents: read. publish has contents: write and pull-requests: write. Runs one merge at a time, and a manual run with a full sha retries a merge.
.github/workflows/format.yml New docs-proposal-scripts job running shellcheck, the Node tests, and the script tests on every PR
.github/workflows/deploy-docs.yml Drop the npm cache, so a cache saved by a job that ran generated content can't be restored in the privileged Pages deploy
AGENTS.md CI gate for the documentation proposal scripts
scripts/docs-proposal/hunks.mjs Dependency-free diff parser. Lists hunks with stable ids for Copilot and builds the per-hunk revert review. Model text is sanitized: backticks, newlines, and @ mentions are stripped. The rationale renders as an inert fenced text block, so links, HTML, #N references and closing keywords stay inactive.
scripts/docs-proposal/hunks.test.mjs node:test coverage for parsing, ids, every revert-anchor case, new-file hunks, evidence sanitizing, the inert rationale block, and the review payload
scripts/docs-proposal/propose.sh Resolves the base to a full SHA, renders the prompt (portable to bash 3.2), runs Copilot, normalizes edits with Prettier, writes the patch, then runs Copilot again for per-hunk evidence. A failed evidence run keeps the proposal with empty evidence.
scripts/docs-proposal/validate.sh Applies the patch, enforces the allowlist, and runs the docs gates without write credentials
scripts/docs-proposal/publish.sh Applies the patch, re-checks the allowlist, pushes the branch, creates or edits the PR (ignoring fork PRs that reuse the branch name), and posts the review. An emptied rerun comments on the PR before closing it.
scripts/docs-proposal/lib.sh Shared SHA validation, branch naming, allowlist, and PR body helpers
scripts/docs-proposal/prompt.md, evidence-prompt.md Copilot instructions: shipped behavior only, smallest accurate edit, fictional example.com values
scripts/docs-proposal/test.sh Bash tests against a temporary bare repo with stubbed gh and npm. 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 Script rows plus org and repo prerequisites
.gitignore Ignore the .docs-proposal/ work dir
docs/superpowers/specs/2026-10-01-docs-proposal-automation-design.md, docs/superpowers/plans/2026-10-01-docs-proposal-automation.md Design decisions for the issue's open questions, and the implementation plan

Closes

Closes #1105

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run. All 1185 pass on the pinned Node 24 behavior. Locally on Node 26 they need NODE_OPTIONS=--no-experimental-webstorage, because Node's built-in localStorage shadows jsdom's. That failure is unrelated to this PR.
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1 (no Rust changes)
  • Manual testing via fastly compute serve (not applicable)
  • Other:
    • node --test scripts/docs-proposal/hunks.test.mjs: 19 of 19 pass.
    • scripts/docs-proposal/test.sh passes, including with LANG/LC_ALL unset. A mutation check confirmed it fails when either the unchanged-retry guard or the path allowlist is disabled.
    • shellcheck 0.11.0 is clean on all four scripts.
    • actionlint 1.7.12 is clean except two false positives: copilot-requests and concurrency.queue. Its latest release (2026-03) predates the copilot-requests permission, which GitHub's Copilot CLI Actions docs document, and it doesn't know the concurrency.queue key.
    • Every Copilot flag was checked against @github/copilot@1.0.90 --help.
    • propose.sh was dry-run in a temporary clone with stubbed copilot and npm.
    • cd docs && npm run build passes.

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:

  • the organization Copilot policy "Allow use of Copilot CLI billed to the organization";
  • the repository setting "Allow GitHub Actions to create and approve pull requests".

Proposal PRs are opened with GITHUB_TOKEN, so they don't trigger other workflows. The publish job runs the docs gates itself.

Checklist

  • Changes follow AGENTS.md conventions
  • No unwrap() in production code — use expect("should ...") (no Rust changes)
  • Uses log macros (not println!) (no Rust changes)
  • New code has tests
  • No secrets or credentials committed

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 prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread scripts/docs-proposal/publish.sh Outdated
Comment thread .github/workflows/docs-proposal.yml
Comment thread scripts/docs-proposal/publish.sh Outdated

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread scripts/docs-proposal/prompt.md Outdated
Comment thread .github/workflows/docs-proposal.yml
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 prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at scripts/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 at scripts/docs-proposal/lib.sh:66-68.

Nits / notes

  • ⛏ No timeout-minutes on the Copilot job. See inline at .github/workflows/docs-proposal.yml:44 (suggestion).
  • 📝 Merging this PR triggers the workflow itself (scripts/README.md matches scripts/**). 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.md and are opened with GITHUB_TOKEN.
  • 📝 The AGENTS.md "CI Gates" list doesn't mention the new docs-proposal scripts job.
  • 📝 The PR body says actionlint is clean except copilot-requests, but it also flags concurrency.queue. Both are actionlint false positives.
  • ⛏ scripts/README.md says "runs both proposal scripts", but there are three (propose, validate, publish).
  • ⛏ prompt.md / evidence-prompt.md hardcode .docs-proposal/, while propose.sh accepts 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: pass
  • node --test scripts/docs-proposal/hunks.test.mjs: 16/16 pass
  • shellcheck scripts/docs-proposal/*.sh (0.11.0): clean
  • actionlint 1.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.

Comment thread scripts/docs-proposal/publish.sh
Comment thread scripts/docs-proposal/propose.sh Outdated
Comment thread .github/workflows/docs-proposal.yml Outdated
Comment thread scripts/docs-proposal/lib.sh Outdated
Comment thread .github/workflows/docs-proposal.yml

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread scripts/docs-proposal/propose.sh
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 ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 outside docs/ 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 queue and copilot-requests keys, plus unquoted-output ShellCheck warnings in format.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.

Comment thread scripts/docs-proposal/publish.sh Outdated
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 prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, #N references and links: see inline at scripts/docs-proposal/lib.sh:72
  • The propose job can still write a cache that deploy-docs restores: see inline at scripts/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 base input is only rejected at publish time: see inline at scripts/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-lease everywhere, 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-renames is 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: max and the copilot-requests: write permission, but both are valid GitHub syntax. No change needed.
  • 📝 Test count: hunks.test.mjs has 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).

Comment thread scripts/docs-proposal/publish.sh Outdated
Comment thread scripts/docs-proposal/lib.sh Outdated
Comment thread scripts/docs-proposal/propose.sh
Comment thread scripts/docs-proposal/hunks.mjs
Comment thread scripts/docs-proposal/propose.sh

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.sh passed, exercising rejection through lib.sh:45–49 and 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–71 against 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 outside docs/ 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 queue and copilot-requests keys, 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 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 changes test.sh in more than one place or together with publish.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 at scripts/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_prompt writes ".docs-proposal"/rationale.md under bash 3.2: see inline at scripts/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 base input and validate.sh: see inline at docs/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.sh could run git cat-file -e "$sha:<path>" on each evidence source, 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), leaves validate.sh out 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, base normalization) 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 with LANG=C.UTF-8 (Docker node:24, matching the runners); 1 failure without LANG; 3 failures under macOS /bin/bash 3.2.57.
  • Symlink probe: docs_proposal_apply accepts a 120000 entry. npm ci, lint, format, and build run on the docs tree at this head show the symlinked superpowers/** spec published at dist/guide/zz-probe.html.
  • Every suggestion was applied alone to a clean checkout of this head and verified with shellcheck, the hunk tests, and test.sh on 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.sh change, the evidence test fails three assertions against a broken hunks.mjs list call, and the new close assertion fails without the publish.sh change.
  • concurrency.queue: max is 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

Comment thread scripts/docs-proposal/lib.sh
Comment thread scripts/docs-proposal/publish.sh Outdated
Comment thread scripts/docs-proposal/test.sh
Comment thread scripts/docs-proposal/propose.sh Outdated
Comment thread scripts/docs-proposal/propose.sh
Comment thread scripts/docs-proposal/test.sh Outdated
Comment thread docs/superpowers/plans/2026-10-01-docs-proposal-automation.md
Comment thread scripts/README.md Outdated
Comment thread docs/superpowers/specs/2026-10-01-docs-proposal-automation-design.md Outdated
…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
@dhruv8sh
dhruv8sh requested review from aram356 and prk-Jr October 10, 2026 06:31

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}` : "";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ![](https://track.example.com/p.png) <img src=x>, the review comment body comes out as:

**Source:** `x.rs:1` — See #1 and [click](https://evil.example.com) ![](https://track.example.com/p.png) <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.

Comment on lines +58 to +59
--allow-tool='shell(git show:*)' \
--allow-tool='shell(git diff:*)' \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at scripts/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 list call in propose.sh now fails 6 assertions instead of passing; test.sh passes under macOS bash 3.2 and under bash 5.2 with and without LANG; 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 reason rendered as live Markdown, git diff --output, base read from the artifact, other main-scoped caches, transitive dependencies of npm 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.sh at this head: passes on bash 5.2 with jq (with LANG=C.UTF-8 and without LANG) and on macOS /bin/bash 3.2.57; fails 4 assertions where jq is missing.
  • Both suggestions were applied alone and together to a clean checkout of this head and pass test.sh on all three shells; the prompt change also passes the root Markdown Prettier gate, and the test.sh change passes shellcheck. No drift between the applied bytes and the verified tree.
  • The merges from main resolve cleanly: AGENTS.md numbers the new gate 10 after main's gate 9, and the format.yml job 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

Comment on lines +36 to +37
- Do not edit files outside `docs/guide/`, `docs/index.md`, and
`<work-dir>/`. Do not commit, push, or create branches.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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:

Suggested change
- 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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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:

Suggested change
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.

@aram356 aram356 added this to the 202610 milestone Oct 12, 2026
aram356 added a commit that referenced this pull request Oct 12, 2026
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.
aram356 added a commit that referenced this pull request Oct 12, 2026
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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatically open PR for documentation changes when code merged to main

4 participants