Skip to content

Code review · D5 · Dev previews end to end

Range: 98d74c2..1215f4f on autopilot/D5 (PR #13). Test command: python3 -m unittest discover -s tools/previews -p 'test_*.py' (12 tests, OK). Reviewers ran as independent headless sessions (claude -p --agent …, read-only tools).

Pass Round 1 (HEAD 086a84b) Round 2 (HEAD 311d13e)
code-reviewer changes, b/M/m = 0/1/4 approve, 0/0/2
security-reviewer changes, 0/1/3 approve, 0/0/2
code-simplifier (HEAD 1215f4f) — no change, tests pass

CR-01 · major · The "second push keeps the first URL" check had no evidence

  • Where: the step's verification (.github/workflows/previews.yml)
  • Evidence: the round-1 report only listed checks after the first push (950e1de).
  • Problem: the step prompt asks for that check explicitly.
  • Proposal: curl both hash URLs and the alias after the second push.
  • Found by: code-reviewer
  • Status: applied. After run 40523119556, 4367baf3.spectio.pages.dev still showed build 950e1de and 8c909d6d.spectio.pages.dev showed build 086a84b, both 200. The reviewer re-ran the curl in round 2.

CR-02 · minor · simplify: one-off secret-name diagnostic

  • Where: previews.yml, "Check the Cloudflare secrets"
  • Evidence: it answered its question once (run 40522381183: only the misspelt names are set).
  • Problem: a permanent log line for a one-time question.
  • Proposal: drop it and record the answer in the spec.
  • Found by: code-reviewer
  • Status: applied (311d13e; spec §7.1).

CR-03 · minor · The pagination fallback without result_info was untested

  • Where: tools/previews/versions_page.py fetch()
  • Evidence: both fetch tests sent total_pages.
  • Problem: the fallback branch never ran in a test.
  • Proposal: a test with no result_info.
  • Found by: code-reviewer
  • Status: applied (test_without_result_info_a_short_page_stops).

CR-04 · minor · Codespaces static check not reported

  • Where: .devcontainer/
  • Evidence: the round-1 report only mentioned the Node bump.
  • Problem: the step asks for the static check.
  • Proposal: report it with file references.
  • Found by: code-reviewer
  • Status: applied (spec §7.2, Progress page).

CR-05 · minor · Spec §7.1 did not describe the wave path

  • Where: dev-environment-and-testing.md §7.1
  • Evidence: the rule only lived in a YAML comment.
  • Problem: the rule had no owning spec text.
  • Proposal: write it in §7.1, with the 2026-10-27 checks.
  • Found by: code-reviewer
  • Status: applied (§7.1 "Wave approval" and "Checked end to end on 2026-10-27").

CR-06 · minor · The alias test's bash flags

  • Where: tools/previews/test_alias.py
  • Evidence: the reviewer assumed GitHub runs bash -eo pipefail.
  • Problem: the test's flags could differ from CI's.
  • Proposal: match CI's flags.
  • Found by: code-reviewer
  • Status: rejected, with a comment added (1215f4f). The run logs show shell: /usr/bin/bash -e {0}, because the step sets no shell:, so the test already matches CI.

CR-07 · minor · The job if has no unit test

  • Where: previews.yml, job if
  • Evidence: a GitHub expression cannot run locally.
  • Problem: the sender and base conditions have no regression test.
  • Proposal: a comment, plus a live negative check.
  • Found by: code-reviewer
  • Status: applied. A comment was added, and the owner's label on PR #13 (base autopilot/integration) skipped the job (run 40524120754).

SEC-1 · major · wave-approved accepted any labeller, any base, and manual runs could take any alias

  • Where: previews.yml, job if, alias step
  • Evidence: any same-repo PR with a W<n> title could repoint wave-w<n>, and workflow_dispatch could overwrite main or pr-*.
  • Problem: "validated wave" is an integrity claim that the code did not enforce.
  • Proposal: require the owner as sender and main as base; refuse reserved aliases on manual runs; consider an environment with a required reviewer.
  • Found by: security-reviewer
  • Status: applied (311d13e, tested in test_alias.py). The required-reviewer environment is an option for Thomas [to be validated: plan support]. The residual risk is SEC-5.

SEC-2 · minor · PR-head code runs with the Cloudflare token; wrangler's dependencies are not locked

  • Where: previews.yml, the versions and deploy steps
  • Evidence: a same-repo PR controls the scripts that hold the token.
  • Problem: a token holder could be modified by a PR.
  • Proposal: a base checkout for the script, and a lockfile for wrangler.
  • Found by: security-reviewer
  • Status: accepted as residual risk. With pull_request, the workflow file itself comes from the PR, so a base checkout adds nothing; the fork guard is the boundary. The lockfile is left to Renovate (spec §7.1).

SEC-3 · minor · The PR comment was matched by its prefix only

  • Where: previews.yml, the comment step
  • Evidence: anyone could post a comment starting with the marker.
  • Problem: the bot would then rewrite another user's comment.
  • Proposal: also match the bot's login.
  • Found by: security-reviewer
  • Status: applied (311d13e).

SEC-4 · minor · Action SHA pins unverified by the reviewer

  • Where: previews.yml, the uses: lines
  • Evidence: the reviewer's sandbox could not call the API.
  • Problem: a pin could name the wrong commit.
  • Proposal: check each pin against its tag.
  • Found by: security-reviewer
  • Status: applied. git ls-remote --tags on 2026-10-27 gave checkout v4.4.0 11d5960…, setup-uv v6.8.0 d0cc045… and github-script v7.1.0 f28e40c….

SEC-5 · minor · The wave gate rests on a procedural rule

  • Where: previews.yml and §7.1
  • Evidence: the autopilot acts with Thomas's credentials, so the webhook sender is the same for its labels. On PR #13 the issues events API showed performed_via_github_app: claude.
  • Problem: the code cannot tell Thomas's label from an agent's.
  • Proposal: write the rule in §7.1; optionally reject labels applied through a GitHub App.
  • Found by: security-reviewer
  • Status: applied in §7.1 ("Only Thomas adds this label; agents never do"). The app check is not built: a label added through the session proxy showed no app attribution, so the check would not be reliable.

SEC-6 · minor · The SEC-2 residuals needed a written record

  • Where: spec §7.1
  • Evidence: the token scope and the missing wrangler lockfile were not written down.
  • Problem: the residual risks were not recorded.
  • Proposal: write both into §7.1.
  • Found by: security-reviewer
  • Status: applied (§7.1, "How").