Add interactive review for large changes

Merged
JS
8h ago
+14241-54

Problem and behavior

A large change could reach a pull request with nobody having explained it, and nothing recorded which commits had passed the hooks. This adds a pre-PR workflow: every commit carries a parent-linked Change-Id, a commit of 200 added plus deleted lines or a pull request over 500 cannot be pushed without matching interactive review evidence for the exact head, and the pull request description declares the review so CI can compare it with the diff.

The review is a local page. The contributor explains one key change in their own words, a reviewer returns one reference note, and the contributor records their own assessment; nothing grades them. The checks on the answer are small on purpose: a minimum length, no word-for-word copy of the question or the code, and a hint when it names nothing from the changed lines. The diff is read by content, so moved and renamed functions, changed words and whitespace-only edits are labelled, and a diff too large for the reviewer is reduced by size with every file named instead of being cut off at the end.

Approach

Nine commits. The hooks write and check the Change-Id chain and the review evidence (scripts/hashchain.py, scripts/check-interactive-review.py, scripts/git-pre-push.sh); the workbench is the page and its optional Codex worker; scripts/agent-guard.py refuses an agent's shell command that skips the hooks; the pull request template and an interactive-review check (the pr-description workflow) carry the declaration. That workflow also rechecks each pull request in a merge group, and it cannot be switched off from a pull request: a merge group runs the workflow as the queue ref has it, so the pull-request run, which is the base branch's, refuses a change to the workflow, another workflow that names the check, or the deletion of a checker script, unless a maintainer sets CODETRIAL_POLICY_APPROVED_HEAD to that exact head. scripts/hashchain-base is set to the current main because one earlier commit there has no Change-Id; Dependabot is decided in scripts/hashchain.py alone: a description is excused only when GitHub reports Dependabot as the opener and every commit in the range is its own, and its commits continue the chain without a Change-Id. A push with no base to measure against is refused rather than skipped, since skipping would let any large change through. Sizes are read under the base's attributes, so a branch cannot hide lines by marking a file -diff.

Left out on purpose: there is no parser, so function matching is by indentation and tokens and a language it cannot segment gets line matching only. Nothing here can prove that a person did the review, since the evidence lives on the contributor's machine; docs/development.md says what no local mechanism can enforce and what CI and repository settings must cover. .github/CODEOWNERS names the maintainers for these files, but required status checks (check and interactive-review), code-owner review, the merge queue and an organization required-workflow rule are repository settings and are not part of this change; the hashchain and commit-log checks in CI still go by the commit author line, which docs/development.md says.

Validation

scripts/test.sh portable passes on this head (exit 0), and so does scripts/test-interactive-review.sh, which runs every suite for this workflow including the hook checks and the review-workbench browser tests. Each review round's findings were fixed with a regression test, and the workflow's wiring has a test that fails when a flag or variable it passes is renamed. The rest of the browser lane and the Rust lane run in CI. The pr-description workflow runs the base branch's checker, which has no gate yet, so it skips on this pull request and neither its pull-request run nor its merge-group run has been exercised on GitHub.

Interactive review

This pull request went through its own gate; the block below is what the pre-push check writes for this head.

Interactive review: complete
Head: 97c991833811287acf191c8b923fc894d6dd7e12
Base: b6eb8ba9bfbb14ff8d8a7dee920d29590ee89f25
Diff: sha256:f7c595368633cf11c3fb19d75f5fb40cf5ecf91fdde6213f0aeb927c12ac0ad3
PR text lines: 14295; commits at 200 lines or more: 6

Summary by cubic

Large changes previously could reach a pull request with nothing recorded about which commits had passed the hooks, and no explanation of the change. This adds a pre-PR workflow: each commit carries a parent-linked Change-Id, and a commit of 200 added plus deleted lines or a pull request over 500 cannot be pushed without matching interactive review evidence for the exact head. The pull request description declares that review so CI can compare it with the diff.

Enforcement

  • The commit-msg, post-commit and pre-push hooks write, repair and verify the Change-Id chain; CI recomputes it from scripts/hashchain-base, and hashchain.py alone decides which commits count as Dependabot's.
  • scripts/agent-guard.py parses the shell command line, so --no-verify hidden behind line continuations, groups, wrappers, heredocs or --config-env, plus aliases and hook edits, are refused; a literal --no-verify used as a commit message still passes. Command substitutions inside unquoted heredocs are read whole, so one that spans lines is caught; a quoted heredoc body stays data.
  • The pull request description check runs on pull_request_target with a read-only token and only the base branch's checker, so a pull request cannot change the checker that judges it. The declared Head, Base and diff digest must match, and the merge group rechecks each pull request; a pull request may not remove a checker script or switch the check off.
  • Sizes are read under the base's attributes, so a branch cannot hide lines by marking a file -diff; a push with no base to measure against is refused rather than skipped.
  • The pre-push hook writes one description block per push, clearing the previous one and refusing the push when it cannot, so a stale block never stands in for a later refused or multi-head push. A certified head is required once the contributor confirms, and a malformed explanation file does not count as explained.
  • CODEOWNERS names the maintainers as owners of the gate files and the shared product code, so a pull request cannot edit its own gate and be approved by anyone.

Review workbench

  • A local page shows the diff and asks the contributor to explain one key change; a reviewer returns one reference note, and the contributor records their own assessment, with nothing grading them.
  • The checks are deliberately small: a minimum length, no word-for-word copy of the question or code, and a hint when the answer names nothing from the changed lines.
  • The diff is read by content, so moves, renames and whitespace-only edits are labelled, and a diff too large for the reviewer is reduced by size with every file named.
  • Confirming the self-assessment wakes the agent, which folds the result into the report and certifies the head; the watch waits in the background.