Skip to content

fix(dev-lead): verify an additive fix a pass made in a file the finding names (#2269) - #2303

Merged
don-petry merged 5 commits into
mainfrom
fix/issue-2269-additive-fixed-disposition
Oct 11, 2026
Merged

don-petry merged 5 commits into
mainfrom
fix/issue-2269-additive-fixed-disposition

Conversation

@don-petry

@don-petry don-petry commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A fixed comment disposition can now verify a fix that only adds lines.

Problem

Closes #2269. The #2004 rule accepts a fixed disposition only when the cited diff removes a token the finding names. A finding such as "this test never asserts X" is fixed by adding the assertion, so nothing is removed, the verdict is content-no-token-removed, and the comment stays open until a human clears it. On PR #2205, commit 3e5025bd added one line for a CodeAnt finding and could never have verified.

The maintainer chose option 1 from the issue on 2026-10-11: align with the rule a review-thread claim already passes (acv_claim_in_pass), limited to commits the pass itself made. This PR is narrower than that option as written in one respect: it does not accept a diff that adds a named token. An additive fix that repeats the finding's own token (for example an assertion that greps for the named string) therefore still does not verify. Widening that is a separate decision.

Risk

Medium. This widens what the approval gate accepts as a verified fixed disposition.

  • The new path needs all of: a commit this pass produced, on the pushed head, off the base branch, dated after the finding, changing a file the finding names. An unreadable input fails closed, as before.
  • "A file the finding names" is a whole-path match. A finding that names only foo.sh does not match scripts/foo.sh, and a link's URL is not read as a file.
  • It does not check that the added lines address the finding, only where and when the commit was made: a comment-only line in the named file verifies. That is the same strength as the review-thread rule, and weaker than a removed token. A test pins it by name.

Known limits:

  • "Produced by this pass" means "not reachable from the pre-pass head". When the push guard rebases onto a concurrent commit, that foreign commit also fits. The model would have to cite it; the review-thread rule has the same property.
  • A finding that names no file gets no new path. It still needs a removed token, or no token at all.
  • The optional "within a few lines of a named path:line" check from the issue is not implemented.
  • The full-script test of the real PR feat: implement issue #1787 — calibrate solution-architect escalation so conforming changes are not escalated #2205 comment appends a new test to the named test file. The real fix added a line inside an existing test; that exact edit is tested only against the pure verdict, with the real diff.

Test plan

  • 18 new bats tests: 11 in tests/dev-lead/unit/test_comment_disposition_verify.bats (one runs the real PR feat: implement issue #1787 — calibrate solution-architect escalation so conforming changes are not escalated #2205 CodeAnt comment and the real diff of 3e5025bd, saved under tests/fixtures/comment-disposition/), 7 in tests/dev-lead/unit/test_fix_reviews.bats through the full script (one runs the real comment against its test file, through the test-tamper guard).
  • Counterfactual, run on the first 12: against main's scripts, prompts and doc, 9 fail. The 3 that pass either way are the guards that an earlier-pass commit, or one outside the named files, still does not verify. The 6 added in review were not re-run against main.
  • test_comment_disposition_verify.bats: 71/71. The #2269 tests in test_fix_reviews.bats: 7/7. Of the #2004 tests beside them, 2 fail locally; those 2 failures (#2004 AC4(c) and (e)) fail the same way on main because they call GNU sed -i, which this Mac lacks. CI is the authority.
  • shellcheck --severity=warning -x on both scripts, scripts/check-duplicate-decls.sh, markdownlint-cli2 on the doc: clean.

Rollback

Revert this PR. A comment already minimized under the new rule stays minimized.

Monitoring

In a fix-bot-comment or fix-reviews run log: `fixed` disposition verified against <sha> (this-pass-touches-named-file). Inert until the dev-lead channel tag moves onto it (next autocut to dev-lead/v139-next, then ring promotion).

Interaction contract

  • N/A: no agentic role added or changed.

🤖 Generated with Claude Code

don-petry and others added 2 commits October 11, 2026 12:28
…ng names (#2269)

A `fixed` comment disposition verified only when the cited diff removed a token
the finding names (#2004). A fix that only adds lines, such as a missing
assertion or guard, removes nothing, so it could never verify and its comment
stayed open (PR #2205, commit 3e5025b).

Maintainer ruling on #2269: option 1. A commit that removes no named token now
verifies when this pass produced it and its diff changes a file the finding
names by its full repo path (`this-pass-touches-named-file`). It must still be
on the pushed head, off the base branch, non-empty and dated after the finding.
A commit from an earlier pass, or one outside the named files, keeps the
removed-token rule. This is the rule a review-thread claim already passes.

- cdv_finding_paths, cdv_touches_named_file: pure helpers.
- cdv_fixed_verdict takes an eighth argument, <touches_named>.
- rdc_fixed_verdict passes the commit's own files against the finding's paths.
- Both prompts and docs/pr-review-agent/maintainer-comment-gate.md state it.
- Tests: 8 pure (one on the real PR #2205 comment and the diff of 3e5025b,
  saved under tests/fixtures/comment-disposition/) and 4 through the full
  script. 9 of the 12 fail on main's sources; the other 3 are the guards that
  an unrelated or earlier commit still does not verify.

Closes #2269

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@don-petry don-petry added the dev-lead:hands-off Exclude from dev-lead agent automation label Oct 11, 2026
@don-petry
don-petry requested a review from a team as a code owner October 11, 2026 18:14
@don-petry don-petry added the dev-lead:hands-off Exclude from dev-lead agent automation label Oct 11, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 89c547de-3747-416a-aa39-c4410c9ce6ec

📥 Commits

Reviewing files that changed from the base of the PR and between 109ad26 and 97bf758.


📒 Files selected for processing (9)
  • docs/pr-review-agent/maintainer-comment-gate.md
  • prompts/dev-lead/fix-bot-comment.md
  • prompts/dev-lead/fix-reviews.md
  • scripts/dev-lead-fix-reviews.sh
  • scripts/lib/comment-disposition-verify.sh
  • tests/dev-lead/unit/test_comment_disposition_verify.bats
  • tests/dev-lead/unit/test_fix_reviews.bats
  • tests/fixtures/comment-disposition/pr2205-codeant-nitpick.md
  • tests/fixtures/comment-disposition/pr2205-fix-3e5025bd.diff

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@don-petry

Copy link
Copy Markdown
Collaborator Author

dev-lead is withholding action on this item.

It is labeled dev-lead:hands-off (deliberately kept off-limits to dev-lead (e.g. changes to the dev-lead automation itself)), so dev-lead will not pick it up while that label is present. This notice is posted once so the withhold is visible rather than looking like a stalled run.

To re-enable automated pickup: remove the dev-lead:hands-off label.

@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T19:16:12Z.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements support for verifying additive fixes (fixes that only add lines without removing tokens) under rule #2269. It introduces helper functions to extract file paths from finding bodies and check if the current pass's commit touches those files, updating relevant documentation, prompts, and unit tests. The review feedback suggests optimizing the cdv_touches_named_file function by using pure Bash associative arrays instead of spawning grep inside a loop, which improves performance.

@don-petry

Copy link
Copy Markdown
Collaborator Author

QA Lead — test-risk advisory

Risk tier: MEDIUM. This PR widens what the approval gate accepts as a verified fixed disposition. A false verify hides a bot finding, so the PR can merge with that finding unaddressed. The new path still fails closed on unreadable input and keeps every earlier check, and those are well covered.

What I'd shore up (highest leverage first):

  • Missing negative test for content-adds-token. The new path accepts both content-no-token-removed and content-adds-token. Take a finding like "remove eval from scripts/x.sh". A commit from this pass that adds another eval to that file now verifies as fixed. Add a test that pins the intended answer here. If the answer should be "no", limit the additive path to content-no-token-removed. The real PR feat: implement issue #1787 — calibrate solution-architect escalation so conforming changes are not escalated #2205 case only needs that one.
  • The motivating case is never run through the full script. The real fix was to a test file. AC1 uses a script because the stub can't serve the test-tamper guard's PR lookup. So nothing exercises how the new verdict interacts with that guard, which is the one path this change exists for. Extend the stub rather than route around it.
  • A comment-only commit counts as a fix. AC1's "fix" appends a single # comment line, and it verifies. This weakness is documented and accepted. Still, pin it with a test named for that behaviour so tightening it later is a deliberate change.
  • Edge cases in path extraction: renamed files (old path vs. new path in own_files), a file the commit deletes, path:L12-L14 forms, and backticked paths outside links. Each needs a table-driven case for cdv_finding_paths and cdv_touches_named_file.

Escalate? No. The gate loosening is the trade-off the maintainer chose, and it fails closed. The content-adds-token boundary needs a decision and a test before merge.

Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture
(bmad-tea v1.19.0). Opt out on this item with the qa-lead:hands-off label.

@codeant-ai

codeant-ai Bot commented Oct 11, 2026

Copy link
Copy Markdown

CodeAnt Nitpicks

1 code suggestion

1. The named-file rule also accepts commits that add the finding's token, so an introducing change from this pass can still mark the original finding fixed.

Logic error · docs/pr-review-agent/maintainer-comment-gate.md:116-119

@codeant-ai

codeant-ai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

CodeAnt PR Risk: Medium Risk

  • The PR needs attention before merging because a same-pass edit to a named file can mark a finding fixed without showing that the edit addresses it.
  • A test explicitly accepts a comment-only edit, so this gate does not verify that an additive change resolves the reported issue.
  • The reported token-addition bypass is blocked: a diff that adds a named token still fails verification.

Assessed commit: 97bf758f8770

don-petry and others added 3 commits October 11, 2026 13:28
…ditive fix (#2269)

Review (CodeAnt, QA Lead): the new path accepted `content-adds-token` as well
as `content-no-token-removed`. For a finding like "remove `eval` from x.sh", a
commit from this pass that adds another `eval` to that file would have
verified. The path is now limited to a diff that neither removes nor adds a
token the finding names; `content-adds-token` fails whatever pass made the
commit, as before this branch.

- Prompts and the gate doc say so.
- Tests: the boundary at the pure level and through the full script; a table
  of the forms reviewer bots write a location in; renamed and deleted files;
  and one that pins the accepted limit that a comment-only line in the named
  file verifies.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#2269)

The comment names a test file, so the pass also goes through the test-tamper
guard. The test serves the guard's base-branch lookup and gives the repo an
origin to fetch main from, then checks the disposition verifies and the comment
is minimized.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T19:32:25Z.

@don-petry
don-petry merged commit 9d35d5f into main Oct 11, 2026
84 of 95 checks passed
@don-petry
don-petry deleted the fix/issue-2269-additive-fixed-disposition branch October 11, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev-lead:hands-off Exclude from dev-lead agent automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A fixed disposition can never verify an additive fix when the finding names a token: the #2004 content rule only accepts a removed token

2 participants