Repository navigation
Flag code comments that name a mechanism without describing it - #151
Merged
Merged
Conversation
An author who has just worked out how something behaves writes a short phrase that stands for it. The phrase reads as precise to them because they still hold the derivation; to a reader it is a label with nothing behind it. Nothing in the pipeline looked at comment prose in the diff, and the comprehension gate only flagged a coined label when it blocked comprehension outright, which a domain expert guessing correctly never does. The maintainability reviewer now checks comment and docstring lines the diff adds. Its discriminator is a prior-use search against the PR's base: grep the phrase, read the hits, and treat it as the repo's vocabulary when two or more pre-existing uses mean the same thing. A count alone does not work, since "holds the batch" has four hits in posthog and "best effort" has two, and only the first is a coinage; the hits have to be read. The comprehension gate gains an `unresolved` field that makes it restate each label from the body alone, so "I read this" separates from "I inferred this", plus a rule against an invented prefix ahead of the first sentence. Verified against PostHog/posthog#86291, the PR the four real cases came from, checked in as evals/benchmarks/posthog/comment-prose-labels: three runs each, 3 of 3 target phrases flagged and 0 of 6 established-vocabulary traps tripped, against 0 of 3 for the old prompt. The traps include "holds the offset", which shares a verb with the flagged "holds the batch" and must not fire. Claude-Session: https://claude.ai/code/session_01KWdQJyttA3tKpnwzQfa1VQ
The search told the agent that `<base ref>` comes from File Access. It does not: none of the four File Access templates names a base ref, a merge base, or a base branch, and the per-agent prompt carries only the briefing path, the diff path, and that block. The one place a base branch reached a PR-mode agent was the briefing's Branch line, and it rendered blank, because build-agent-briefing.sh read `.pr.head` and `.pr.base` while pr-context.sh emits those fields as `head_ref` and `base_ref`. Neither way out was right. An agent that dropped the undefined ref still ran a successful grep, against a working tree that already held the comments the PR added, so the phrase looked like established vocabulary and the finding was suppressed. An agent that refused to guess reported nothing, since a completed search is a precondition for reporting at all. The documented fallback covered neither, firing only when the checkout is missing or the command errors. The agent now derives the ref from the briefing's Branch line and `git merge-base`. `<source root>` is defined as the directories the diff's own files sit under, the phrase is quoted so a `$` or backtick is searched rather than expanded, and the fallback fires when no base ref can be determined and has to be disclosed in the Investigation Summary. Both readers of the dead fields now use `head_ref` and `base_ref`. The one in review-status-handler.sh also feeds `review-delta.sh --base`, which hard-errors on an empty value, so incremental `--append` re-reviews were falling back to a full pass every time. Three smaller repairs: the rewrite prompt no longer asks for concrete behavior and forbids it in the same sentence, the Overview gate in review-compose.md learned about `unresolved`, and the Investigation Summary bullet names the search log that line 173 makes mandatory. Claude-Session: https://claude.ai/code/session_01KWdQJyttA3tKpnwzQfa1VQ
A frozen diff replaced the review's diff but left the worktree on the PR's current head, and agents verify every line by reading the file there. On an open PR that means they read the corrections the author pushed after the diff was captured. `frozen_diff` was meant to hold the pre-fix code still, per its own comment in run-eval.sh, and it only ever froze half of what the agent sees. `source_sha` in a benchmark's metadata now pins the checkout. pr-worktree.sh reads it from REVIEW_CODE_PR_SHA, requires it to be reachable from the fetched PR ref, and fails rather than falling back to the head, since a silent fallback looks exactly like the bug this fixes. The returned ref is the pinned sha, so `git show "$file_ref:<path>"` reads the pinned content too. comment-prose-labels is rebuilt on that footing. Its diff is now the real master...cb51c55ab3be comparison over the four scored files, replacing a hand-assembled composite whose state existed at no commit: two of its three targets appear nowhere in the PR's history, including the orphaned pre-force-push shas. The answer key keeps the surviving target at fork-flag-evaluations-step.ts:72 and adds the second genuine occurrence at flag-evaluations-service.ts:25, which the scorer previously counted against precision because the load-bearing trap sat within its margin. That trap's generic keywords are gone for the same reason. All six traps and both targets are added lines in the new diff and map through diff-position-mapper.sh. Trap corpus counts in the key now match the base: "best effort" has 2 uses and the hyphenated spelling has 36, load-bearing 9, drained 90, "in flight" 91. The eval has not been re-run, so the counts describe what the benchmark contains, not a measured score. Claude-Session: https://claude.ai/code/session_01KWdQJyttA3tKpnwzQfa1VQ
There was a problem hiding this comment.
🟡 Changes recommended
There is a concrete field-name mismatch between get-review-fields output and build-agent-briefing.sh that will blank out the PR branch line, and the new benchmark’s expected-finding count is inconsistent with the PR’s stated test plan.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens the /review-code skill’s ability to (1) catch “label-only” comment phrases that name a mechanism without describing it and (2) ensure review prose is self-contained by having the comprehension gate explicitly report unresolved mechanism phrases. It also adds benchmark and harness support for frozen diffs by pinning PR worktree checkouts to the original source commit SHA.
Changes:
- Add
unresolvedreporting tocomprehension-gateand propagate it into the rewrite instructions for findings and overview prose. - Add
REVIEW_CODE_PR_SHAsupport topr-worktree.sh(plus tests) and wire it intorun-eval.shvia benchmarkmetadata.json. - Add a new
comment-prose-labelsPostHog benchmark (registry entry + frozen diff + answer key + metadata) and update the maintainability reviewer rubric to search prior usage for flagged phrases.
File summaries
| File | Description |
|---|---|
| tests/unit/test-pr-worktree.bats | Adds coverage for commit-pinned provisioning and unreachable-sha failure behavior. |
| skills/review-code/scripts/review-status-handler.sh | Adjusts the compact session output’s PR branch fields to use *_ref values. |
| skills/review-code/scripts/pr-worktree.sh | Implements REVIEW_CODE_PR_SHA pinning with reachability enforcement and updated output ref. |
| skills/review-code/scripts/build-agent-briefing.sh | Updates displayed branch line in PR-mode briefing (currently mismatched with session field names). |
| skills/review-code/handlers/review.md | Updates comprehension-gate integration to include unresolved and rewrite guidance using it. |
| skills/review-code/handlers/review-compose.md | Applies unresolved + prefix-drop guidance when gating the Overview paragraph. |
| evals/scripts/run-eval.sh | Reads source_sha from benchmark metadata and sets REVIEW_CODE_PR_SHA for frozen-diff runs. |
| evals/benchmarks/registry.json | Registers the new comment-prose-labels benchmark. |
| evals/benchmarks/posthog/comment-prose-labels/metadata.json | Adds benchmark metadata including source_sha and expected/trap counts. |
| evals/benchmarks/posthog/comment-prose-labels/diff.patch | Freezes the upstream PR diff as benchmark input. |
| evals/benchmarks/posthog/comment-prose-labels/answer-key.json | Defines expected findings and trap cases for the benchmark. |
| agents/comprehension-gate.md | Extends gate output schema and rewrite triggers to include unresolved + invented-prefix detection. |
| agents/code-reviewer-maintainability.md | Adds a new maintainability check for label-only mechanism phrases with required prior-use search. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "maintainability" | ||
| ], | ||
| "difficulty": "hard", | ||
| "expected_finding_count": 2, |
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.
An author who has just worked out how something behaves writes a short phrase that stands for it. The phrase reads as precise to them because they still hold the derivation. To a reader it is a label with nothing behind it. Four of these shipped in one session on PostHog/posthog#86291 and the human reviewer caught all four; nothing automated caught any.
Two gaps let them through. No agent read comment prose in the diff at all: the maintainability reviewer checks whether comments exist, whether they are stale, and whether they restate the code, and stops there. And the comprehension gate only flagged a coined label when it blocked comprehension outright, which never happens when a domain expert guesses the label correctly.
The check
The maintainability reviewer now reads comment and docstring lines the diff adds. The discriminator is a prior-use search against the PR's base: grep the phrase, read the hits, and treat it as the repo's vocabulary when two or more pre-existing uses mean the same thing.
A count alone does not work, and the data says so. In posthog,
holds the batchhas four hits at the merge base andbest efforthas two, yet the first is the coinage. Reading the hits is what separates them: every prior use ofholds the batchpairs it with an object (the batch's offset commit, the batch lock, the batch in flight) and the PR left it bare. The search also produces the fix, sinceoffset commitis exactly the object the human's correction restored.Findings are capped at three per review, and there is an explicit do-not-flag list, because a check that rewrites
in flightorload-bearingis worse than no check.The gate
comprehension-gategets anunresolvedfield that makes it restate each label using words from the body alone. That separates "I read this" from "I inferred this", which the old threshold could not do. It also rewrites a body that opens with an invented colon-terminated prefix likeFor scope:, where the reader cannot tell whether it means out of scope, a severity note, or a caveat.Test plan
evals/benchmarks/posthog/comment-prose-labelsis built from PostHog/posthog#86291: 2 expected findings, 6 false-positive traps. The traps includeholds the offset, which shares a verb with the flaggedholds the batchand must not fire.The benchmark has not been scored yet, so those counts describe what the fixture holds rather than a result. Run
bin/setupbeforerun-eval.sh, or the run scores the installed prompt instead of this branch's.An earlier version of this fixture reported 3 of 3 targets and 0 of 6 traps. Those runs happened, but the fixture was not reproducible. Its diff was assembled by hand, and two of its three targets appear at no commit on #86291, including the two orphaned pre-force-push shas. Only
holds the batchexists anywhere, atcb51c55a. The rebuilt benchmark uses the realmaster...cb51c55ab3bediff over the four scored files, keeps that target, and adds the second genuine occurrence inflag-evaluations-service.ts.Freezing the diff was not enough on its own.
run_pr_benchmarksubstituted the frozen patch, but worktree provisioning still checked out the PR's current head, where the author has since rewritten all three comments. Agents verify each line by reading the file, so they read the corrections. A benchmark can now setsource_shato pin the checkout to the commit its diff came from.pr-worktree.shrequires that commit to be reachable from the fetched PR ref. It fails rather than falling back to the head, because a silent fallback looks exactly like the bug it prevents.One comparison from the earlier rounds drove a design decision and still holds, with the same caveat about the fixture. With the literal search command in the dispatch prompt, the check found every target. Removing that help dropped it to one target and two false positives, and the agent had skipped the search entirely. The search is now a reportable precondition because of that. The Investigation Summary has to name each phrase searched and what came back, and a finding without that line is not reportable.
The gate was checked separately. It rewrites a coined label and an invented prefix, passes a control carrying
in flightandbest effort, and passes an ordinary opening clause that is not a label. Over 20 real review bodies, the new rule alone produced no rewrite.bin/fmtandbin/lintare clean.bin/testis 1769 passing with 2 failures intest-setup.bats. Both reproduce on a pristinemain:bin/setupand that test file are byte-identical to it on this branch.