Repository navigation
Add interactive review for large changes - #571
Merged
Merged
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Nothing recorded which commits had passed the hooks, and a large change could reach a pull request with nobody having explained it. Both are now checked where a commit or push happens. Each commit carries a Change-Id trailer computed from the previous commit's link and its own tree. The commit-msg hook writes it, post-commit repairs it after an amend, and pre-push and CI recompute the chain from the anchor in scripts/hashchain-base. Clean two-parent GitHub merges need no trailer, and Dependabot commits continue the chain without one. hashchain.py rechain restamps a rewritten series. A topic branch with a commit of 200 added plus deleted lines, or a pull request over 500, is refused at push unless matching review evidence exists for the exact head. Sizes are read under the base's attributes so a branch cannot hide lines by marking a file -diff, and a push with no base to measure against is refused rather than skipped. The chain is a checksum of public inputs. It catches accidental history edits and does not prove that a hook ran. Change-Id: Icd2fb631e1416c2fa5348df8ffe8a92ac14d1dec
A reviewer or an agent could summarize a large change for the contributor, and a pasted summary looked the same as understanding. The review now needs the contributor's own account of one key change, taken before the pull request is opened. A loopback page shows the diff and asks one question grounded in it, with up to two optional ones for other files. The contributor writes an answer, a reviewer returns one reference note, and the contributor records their own assessment. Nothing grades them. Answers, feedback and reflections are bound to the reviewed snapshot, and a change to the code or the answers makes the evidence stale. While they write, the page counts words and offers identifiers from the changed lines. Sending is refused only for an answer that is too short or repeats the question or the code word for word; "I don't know" is always accepted. A managed mode lets an installed Codex CLI answer after the chat turn ends, and while a reviewer holds the answer the page shows that it is working, with the time waited, and stays still for anyone who asked for reduced motion. A diff too large for the reviewer is cut with the file the question is about first, and the files left out are named in the evidence so the reference says what it could not see. The checks are this tool's defaults, not findings about understanding. They make a pasted prompt visible and cannot show that the contributor understands the change. Change-Id: I70af82e07757d48455f8ddaa25b7406e3f2bc3e3
Review evidence lives on the contributor's machine, so a pull request gave CI nothing to compare with the diff, and an agent could skip the hooks with a flag. This adds the pieces that sit around the review. The pull request template ends with an interactive review block that pre-push writes, one per pushed head, to .git/codetrial-review. A pr-description check runs on pull_request_target with a read-only token, checks out the head and the base separately and runs the base's checker, so a pull request cannot change the script that judges it. A large change must say complete, and the block must match this pull request's Head, Base and diff digest, so a change made after the review shows. The triage approval script treats that one workflow as a reviewed exception to its rule against such triggers. scripts/agent-guard.py, registered as a Claude Code PreToolUse hook, refuses a shell command that skips the hooks: --no-verify in any spelling git accepts, core.hooksPath, aliases, commit-tree, update-ref and edits to a hook. It parses the command rather than searching its text, so a flag split across a line continuation, a command inside a group or after a keyword, and a wrapper such as sudo -u root are read, and ordinary git use passes. It remains a speed bump for an agent that follows its tool permissions, not a security boundary, and the file says so. A body line that begins with Change-Id: or Rechained: was stripped by the commit-msg hook, leaving a commit the verifier could not read; it is refused with a message that says to indent or reword. An untracked hashchain-base file is no longer taken as the anchor, and the test runner tries every Python 3.11 or later and names any suite it could not run. docs/development.md says plainly what no local mechanism can enforce and what CI and repository settings must cover, that a changed commit needs a new review session, and why the check trusts the base. A single script runs every test for this workflow. Change-Id: Ic5f05a85b30f9b3f4ff4aafc460ee587fed88a8e
The page printed the raw diff, one colour per line, so a moved function read as a deletion plus an addition and a changed word as a whole changed line. A large change was also cut to its first 200000 bytes for the reviewer, losing whatever came last, often the passage asked about. diffmodel.py now reads the diff once into files, hunks and numbered lines, pairs each removed line with the line it became to mark only the words that differ, and recognises runs of at least three lines that moved or changed only in whitespace. The page draws that reading with line numbers, the function from the hunk header, a neutral sign and a note for moves, side by side or unified, and steps through changes. It checks the reading against the hunk headers and the manifest counts, and shows the raw diff with a warning when they disagree. A large diff is drawn in pieces until every line is there. A function moved to another file under a new name, or with a few lines changed, is matched too. Changed lines are cut into units, a function or a block, and a removed and an added unit are paired when they are the same code with its names changed, or when they keep at least 60 percent of their lines and no other candidate is nearly as close. The note says which names changed and how many lines differ, and every other line stays an edit with its words marked. There is no parser behind this: a unit it cannot segment is matched by line only, and a unit with two plausible sources is left alone. For the reviewer, moves and whitespace-only edits are folded first. Files are then cut by size: the asked file whole, the rest whole, as changed lines, as their opening lines, and last as a count, each named. Change-Id: I90b618206dd333d38899eb2c9a7f1074cabba198
A file renamed and edited made the page look unable to account for the diff, because the manifest is read without rename detection and counts the old file as removed and the new one as added; the page then fell back to the raw diff. Renamed files are no longer compared by count and their old path counts as shown. A changed line beginning with two plus signs, such as ++counter, was taken for a header and dropped. The core file was cut short to keep an allowance for other files, and now has the whole budget first. A diff request that failed left the page believing it had loaded, so Retry never reloaded it. A quote of several lines marked no row as the passage asked about; each of its lines is marked now. Whitespace-only lines were too faint in the light theme, and a malformed explanation file could crash closeout after the review was recorded. The feedback command text said a second reference was refused; it may refine one for the same request, and now says so. Change-Id: If98cc006b1d628abb53e537845b9604e59d41cd4
A merge group runs the workflow as the queue ref has it, so a pull request could delete or skip the job that rechecks its description, and a job its own condition skips counts as passing. The pull-request run is the base branch's, so it now fails when the pull request changes pr-description.yml from where its branch began, adds another workflow that mentions the check, or removes a checker script the base has, unless a maintainer set CODETRIAL_POLICY_APPROVED_HEAD to that exact head. The job lost its condition, and the queue rechecks every open pull request whose head is in the group with the group base's checker. A missing file is told apart from a failed read, so two unreadable copies no longer compare equal. Dependabot was decided in four places by two different facts. The commit author address now lives in hashchain.py alone, and the description check excuses a pull request only when GitHub reports Dependabot as its opener and every commit, merges included, is the bot's. A forged author address no longer excuses a description, though the CI hashchain and commit-log checks still go by the author line, which the docs now say. The commit-log replay fails when it cannot read its input instead of passing, and one git call finds the bot commits for a whole range. Tests pin an approval for another head, the check name in other letters, a group that does not descend from its base, the Dependabot exemption through the command line, and the flags and variables the workflow passes, which a test reads from the workflow's step. Change-Id: I98a0201119a97f616c528a647a160977a2f7e891
Confirming the self-assessment is when the agent has work left to do: fold the grade and any correction into the report, certify the head and say what comes next. Nothing told the agent it had happened. The only wait command woke for a submission, and in managed mode nothing waited at all, so the closeout depended on the contributor typing into chat. wait takes --for confirmation, which returns the confirmed snapshot once the explanation is complete and exits 2 while it is not. A watch is not a task, so it does not mark the agent as waiting on the page. The skill now starts it in the background right after the URL is given, in either reviewer mode, and says what to do on waking. Change-Id: I0c61adf0da7055f68b30ca817e5e9d294f55419e
A heredoc whose quoted body ended in a backslash lost its delimiter, because backslash-newline was deleted from the whole script before the heredoc was parsed, and the commands after it were read as the body. The joining is now done per line, outside quoted bodies. The word time was treated as a keyword, so time -p git commit --no-verify was never looked at; it is a wrapper like the others. The generic --no-verify scan also blocked a commit whose message is --no-verify, so the verbs that read their own options are left to that reader. The guard looked for command substitutions one line at a time inside an unquoted heredoc body, which the shell expands, so one that opened on a line and closed on a later one was never seen. The body is now read whole when the delimiter is reached, and the commands inside each substitution are checked as any other. A quoted body stays data. The pre-push hook left the single-head description block of an earlier push in place when a later push was refused or covered several heads, and reported success when copying it failed. It removes the file first and refuses the push if the copy fails. A malformed explanation whose answers are not an object no longer counts as explained. The confirmation watch in the skill ran in the foreground and turned every error into a clean exit; it now says to background it and leaves with the failing status. A test checks that the watch does not mark the agent as waiting. Change-Id: I5279624f4b92afec75e0748db90f1a4429d1d440
jserv
force-pushed
the
interactive-code-review
branch
from
November 2, 2026 13:42
42d9af9 to
06ab432
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jserv
force-pushed
the
interactive-code-review
branch
from
November 2, 2026 13:48
06ab432 to
0a64349
Compare
The files that decide whether a change is reviewed had no owner, so a pull request could edit its own gate and be approved by anyone. Name the maintainer as owner of the workflows, the scripts that check and chain commits and guard agents, and the review skill. The product code, Rust and JavaScript, and its tests are shared with a second owner. Whole directories are listed instead of single files, so a checker or hook added later is covered without a second edit. The file takes effect once it is on main and the ruleset requires a review from code owners. Change-Id: I6c32688770746f81a1a772d8aaad977ba5a8ae48
jserv
force-pushed
the
interactive-code-review
branch
from
November 2, 2026 14:21
0a64349 to
97c9918
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.pyrefuses an agent's shell command that skips the hooks; the pull request template and aninteractive-reviewcheck (thepr-descriptionworkflow) 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 setsCODETRIAL_POLICY_APPROVED_HEADto that exact head.scripts/hashchain-baseis set to the current main because one earlier commit there has no Change-Id; Dependabot is decided inscripts/hashchain.pyalone: 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.mdsays what no local mechanism can enforce and what CI and repository settings must cover..github/CODEOWNERSnames the maintainers for these files, but required status checks (checkandinteractive-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, whichdocs/development.mdsays.Validation
scripts/test.sh portablepasses on this head (exit 0), and so doesscripts/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. Thepr-descriptionworkflow 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: 97c9918
Base: b6eb8ba
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
scripts/hashchain-base, andhashchain.pyalone decides which commits count as Dependabot's.scripts/agent-guard.pyparses the shell command line, so--no-verifyhidden behind line continuations, groups, wrappers, heredocs or--config-env, plus aliases and hook edits, are refused; a literal--no-verifyused 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.pull_request_targetwith 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.-diff; a push with no base to measure against is refused rather than skipped.CODEOWNERSnames 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
Written for commit 97c9918. Summary will update on new commits.