Skip to content

Code review · D4 (CI and Claude in GitHub)

review-code on autopilot/D4, base 98d74c2, 2026-10-27. Reviewers: code-reviewer and security-reviewer, run independently with claude -p --agent, in parallel; three rounds (two fix rounds, then a final review whose two minors were fixed directly). Format: README.md.

Round Head code-reviewer security-reviewer
1 7c0b994 changes (0 blockers, 1 major, 4 minors) changes (0 blockers, 3 majors, 6 minors)
2 30b6ba3 approve (0 blockers, 0 majors, 3 minors) changes (0 blockers, 1 major, 2 minors)
3 29f1c9e approve (0 blockers, 0 majors, 1 minor) approve (0 blockers, 0 majors, 1 minor)

Both final-round minors were fixed in 21fd4b1. Report lines of the final round:

CODE-REVIEW  verdict=approve  blockers=0  majors=0  minors=1  simplify=0
SECURITY-REVIEW  verdict=approve  blockers=0  majors=0  minors=1

code-reviewer

CR-1 · major · The §11 workflow conventions were applied only to the new workflows

  • Where: docs.yml (docs-gate, pr-body), release-guard.yml, pipelines-synthetic.yml (scenarios), pipelines-real.yml (matrix).
  • Evidence: §11 "Workflow conventions" asks for concurrency and timeout-minutes everywhere; these jobs had neither.
  • Problem: ci.yml now calls the synthetic scenarios on every push, and a hung job would hold ci-ok for GitHub's 6-hour default.
  • Proposal: a timeout on every job; concurrency on docs.yml and release-guard.yml.
  • Found by: code-reviewer
  • Status: applied (783fe08)

CR-2 · minor · osv-scanner fails on vulnerabilities that have no fix

  • Where: tools/ci/security.sh, deps.
  • Evidence: §11 said "a known vulnerability that has a fixed version"; osv-scanner exits 1 on any vulnerability (osv-scanner docs, output and return codes, v2.7.0).
  • Problem: the code was stricter than the spec.
  • Proposal: record the stricter rule, or filter the JSON output.
  • Found by: code-reviewer
  • Status: applied (§11 "Stricter than first written", 30b6ba3)

CR-3 · minor · The semgrep severity claim had no source

  • Where: tools/ci/security.sh, sast.
  • Evidence: "ERROR is the highest severity" was stated without a source.
  • Problem: if HIGH or CRITICAL existed, --severity ERROR would drop them.
  • Proposal: cite the CLI reference or semgrep scan --help.
  • Found by: code-reviewer
  • Status: applied: semgrep scan --help of 1.182.1 accepts INFO, WARNING and ERROR only (783fe08)

CR-4 · minor · The "base checkout" assumption of the Claude review was unverified

  • Where: claude-review.yml, checkout and prompt.
  • Evidence: the action's docs/security.md describes restoring configuration from the base when the head is checked out.
  • Problem: if the action switched to the head, the hooks named in .claude/settings.json would run PR code with the Claude secret.
  • Proposal: log git rev-parse HEAD on the first real run.
  • Found by: code-reviewer
  • Status: open: needs the first real run, which needs a Claude secret (Progress page, open question 4)

CR-5 · minor · The pipelines-synthetic.yml row did not name autopilot/integration

  • Where: §11 table.
  • Evidence: ci.yml runs the synthetic scenarios on pushes to autopilot/integration too.
  • Problem: the spec row and the code disagreed.
  • Proposal: name the branch in the row.
  • Found by: code-reviewer
  • Status: applied (30b6ba3)

CR-6 · minor · The deny list missed the attached form of -F

  • Where: claude-review.yml, --disallowedTools.
  • Evidence: Bash(* -F *) needs a space; gh also accepts -F/path.
  • Problem: gh pr comment N -F/proc/<pid>/environ would pass.
  • Proposal: Bash(* -F*), and deny Bash(*/proc/*).
  • Found by: code-reviewer
  • Status: applied (29f1c9e)

CR-7 · minor · docs.yml cancelled runs on pushes to main

  • Where: docs.yml, concurrency.
  • Evidence: cancel-in-progress: true for every event, unlike ci.yml.
  • Problem: two quick merges leave the first main commit with a cancelled docs-gate.
  • Proposal: cancel on pull requests only.
  • Found by: code-reviewer
  • Status: applied (29f1c9e)

CR-8 · minor · The §11 row omitted the new exit-128 condition

  • Where: §11 security table, dependency audit row.
  • Evidence: the script lets exit 128 pass only while no lockfile is tracked.
  • Problem: the spec row was out of date.
  • Proposal: add the condition.
  • Found by: code-reviewer
  • Status: applied (29f1c9e)

CR-9 · minor · Bash(*environ*) also blocked ordinary reviewer commands

  • Where: claude-review.yml, --disallowedTools.
  • Evidence: "environment" contains "environ"; docs/specs/dev-environment-and-testing.md and three design files match.
  • Problem: the reviewer could not git show the CI spec, nor post a summary that mentions an environment.
  • Proposal: Bash(*/environ*).
  • Found by: code-reviewer
  • Status: applied (21fd4b1)

security-reviewer

SEC-D4-01 · major · CODEOWNERS and ci-ok were not enforced by the protection payload

  • Where: docs/playbooks/autopilot.md §8.
  • Evidence: the payload listed the matrix legs and had no ci-ok and no require_code_owner_reviews.
  • Problem: a PR could edit tools/ci/security.sh or an ignore file and still pass; docs-only PRs would wait for ever on the legs.
  • Proposal: require ci-ok, docs-gate, pr-body and code-owner reviews; decide enforce_admins and the autopilot's identity.
  • Found by: security-reviewer
  • Status: applied (30b6ba3); enforce_admins and the identity are open for Thomas (Progress page, open question 2)

SEC-D4-02 · major · Inline suppressions bypassed the code-owned ignore files

  • Where: tools/ci/security.sh, gitleaks and semgrep.
  • Evidence: gitleaks honours gitleaks:allow by default (README v8.30.1, --ignore-gitleaks-allow); semgrep honours nosemgrep by default (--enable-nosem, "Enabled by default").
  • Problem: a secret marked gitleaks:allow would pass ci-ok, and no code owner would see it.
  • Proposal: --ignore-gitleaks-allow and --disable-nosem, with a planted test.
  • Found by: security-reviewer
  • Status: applied (783fe08); planted tests in the Progress page

SEC-D4-03 · major · The release environment protects the App key only with a main-only policy

  • Where: release-please.yml, environment: release.
  • Evidence: environment branch policies are settings, not YAML; nothing documented them.
  • Problem: any same-repository branch could run a job in release, mint a contents: write token and push v* tags.
  • Proposal: document the one-time setup in §12 and the PR body.
  • Found by: security-reviewer
  • Status: applied (§12, 30b6ba3)

SEC-D4-04 · minor · A prompt injection could post the Claude credential as a PR comment

  • Where: claude-review.yml.
  • Evidence: the action scrubs subprocess environments only for non-write users unless the workflow sets CLAUDE_CODE_SUBPROCESS_ENV_SCRUB (action.yml, docs/security.md).
  • Problem: Read and gh pr comment --body-file could carry /proc/self/environ out.
  • Proposal: set the scrub; deny --body-file, --output, --no-index; post with --body; set include_comments_by_actor before going public.
  • Found by: security-reviewer
  • Status: applied (783fe08); include_comments_by_actor open until the repository goes public

SEC-D4-05 · minor · Incomplete supply-chain pins, and no Python audit

  • Where: docs.yml pip install, previews.yml wrangler@4, semgrep registry rulesets, osv-scanner exit 128.
  • Problem: floating dependencies next to tokens; exit 128 would hide a lockfile osv-scanner cannot read.
  • Proposal: pin them; make the 128 pass conditional.
  • Found by: security-reviewer
  • Status: partly applied: exit 128 passes only while no lockfile is tracked (783fe08). The pins are open (§11 "Not pinned"; previews.yml belongs to D5)

SEC-D4-06 · minor · The scanner cache was keyed by name and not re-verified

  • Where: tools/ci/security.sh, fetch.
  • Problem: a version bump kept the old binary; a cached binary was trusted unchecked.
  • Proposal: key by version, re-check the checksum.
  • Found by: security-reviewer
  • Status: applied: keyed by SHA-256, re-verified on every run (783fe08)

SEC-D4-07 · minor · CODEOWNERS missed paths that steer agents

  • Where: .github/CODEOWNERS.
  • Proposal: add tools/autopilot/, docs/playbooks/triage-policy.md, release-please-config.json, .release-please-manifest.json.
  • Found by: security-reviewer
  • Status: applied (783fe08)

SEC-D4-08 · minor · §11 conventions unmet in the edited workflows

  • Where: docs.yml, release-guard.yml, pipelines-*, previews.yml.
  • Problem: no timeouts or concurrency; permissions at workflow level rather than per job.
  • Proposal: add them.
  • Found by: security-reviewer (with CR-1)
  • Status: partly applied: timeouts and concurrency (783fe08). Workflow-level read-only permissions stay as they are, open

SEC-D4-09 · minor · The fixture redaction is narrow, and the leak stays in history

  • Where: tools/fixtures/megatests.py; .gitleaksignore.
  • Proposal: run gitleaks dir on curated output; rewrite history before going public, with a GitHub Support purge of PR refs.
  • Found by: security-reviewer
  • Status: open (inbox #5)

SEC-D4-10 · major · A .semgrepignore in any subdirectory switched SAST off without code-owner review

  • Where: .github/CODEOWNERS (/.semgrepignore, root only); tools/ci/security.sh, sast.
  • Evidence: semgrep 1.182.1 reads "all '.semgrepignore' files found in the project tree" (semgrep scan --help).
  • Problem: src/foo/.semgrepignore with * would hide that tree from SAST, unseen by any code owner.
  • Proposal: **/.semgrepignore in CODEOWNERS; fail SAST on a nested ignore file.
  • Found by: security-reviewer
  • Status: applied (29f1c9e); a planted tools/ci/.semgrepignore makes security.sh sast exit 1

SEC-D4-11 · minor · The deny rules reduce the credential risk but do not remove it

  • Where: claude-review.yml, comment and --disallowedTools; §11 "Trust".
  • Evidence: git diff /dev/null /proc/<pid>/environ works without --no-index; bubblewrap is installed by the action only for non-write users (action.yml).
  • Problem: the comment claimed more than the rules deliver.
  • Proposal: reword to "reduce"; install bubblewrap; deny -F with an attached value.
  • Found by: security-reviewer
  • Status: applied (29f1c9e): bubblewrap step, Bash(*/proc/*) and Bash(*environ*) denied, wording in the workflow and §11

SEC-D4-12 · minor · The admin-bypass question was not in the inbox

  • Where: docs/playbooks/autopilot.md §8, "Open point".
  • Problem: with "enforce_admins": false, an agent acting as Thomas could merge past code-owner review.
  • Proposal: an inbox entry on the autopilot's identity and enforce_admins.
  • Found by: security-reviewer
  • Status: open: Progress page open question 2, routed to the inbox by the next triage step

SEC-D4-13 · minor · The isolation step's comment overstated it

  • Where: claude-review.yml, "Install subprocess isolation".
  • Evidence: the step has continue-on-error: true; upstream claims PID isolation only "on Linux runners with bubblewrap available".
  • Problem: "cannot read the parent's environment" brought back the absolute wording SEC-D4-11 removed.
  • Proposal: say "best-effort"; print bwrap --version.
  • Found by: security-reviewer
  • Status: applied (21fd4b1)