fix(tooling): require the plumbing-skip's sole ctor be reachable as the helper's result - #964
Merged
aallan merged 4 commits intoJul 9, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe diagnostic-fields gate now only exempts a helper’s sole own-scope ChangesPlumbing-skip reachability check
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
=======================================
Coverage 93.29% 93.29%
=======================================
Files 96 96
Lines 31448 31448
Branches 456 456
=======================================
Hits 29339 29339
Misses 2096 2096
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 276-302: The reachability check in _ctor_is_reachable_as_result is
too permissive because it treats any later return or append of the local name as
proof that the ctor result is reachable, even if that name was rebound first.
Update the logic in _ctor_is_reachable_as_result to track whether the assigned
local (for example, the Name captured from ast.Assign) is reassigned before the
final return/append, and treat such rebinding as breaking reachability. Use the
existing _walk_own_scope and _is_append_call flow, but ensure the ctor binding
remains the same value when the name is returned or appended.
In `@tests/test_diagnostic_fields.py`:
- Around line 640-708: Add an explicit negative fixture in
TestPlumbingSkipRequiresReachability for the rebinding-before-return case so
`_ctor_is_reachable_as_result` is exercised on a ctor that is overwritten before
escape. Use the existing `Diagnostic`/`check_source` style fixtures to model `d
= Diagnostic(...); d = other(); return d`, then assert
`spec_ref_violations_in_source` and
`error_code_registration_violations_in_source` report violations instead of
skipping. Keep the new case alongside `DISPATCHED_NOT_RETURNED` and `APPENDED`
so the reachability gate is pinned by this test class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94df453e-bd38-46c1-b567-f53d8d535736
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
chethanuk
force-pushed
the
fix/issue-956-plumbing-skip-reachability
branch
2 times, most recently
from
July 9, 2026 01:44
b54f2be to
0f08954
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 271-273: The reachability logic in `_is_append_call` is too broad
because it treats any `append(...)` as plumbing, which can let `Diagnostic(...)`
escape field checks through unrelated containers. Tighten the sink detection in
`scripts/check_diagnostic_fields.py` so only the real diagnostic sink(s) are
considered, and do not count arbitrary intermediate list appends as
reachability. Update the related traversal logic around `_is_append_call` to use
the specific sink symbols already present in the script, and add a regression
test covering a non-sink case such as `tmp.append(d)`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c53acc4-c304-4f54-a39c-05ea5b380d29
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
6 tasks
chethanuk
force-pushed
the
fix/issue-956-plumbing-skip-reachability
branch
from
July 9, 2026 03:43
0f08954 to
f1267f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/check_diagnostic_fields.py (1)
271-273: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not treat arbitrary
.append(...)calls as plumbing reachability.This still counts
tmp.append(d)as a helper output even whentmpis just an intermediate container, so a bogus sole ctor can remain exempt without being returned or appended to the real diagnostic sink. Restrict this predicate to the actual diagnostic sink shape(s), and keep a regression for a non-sink append.#!/bin/bash set -euo pipefail # Inspect current helper append sinks without executing repository code. rg -n -C 4 'def _(?:error|warning)\b|\.append\(' vera scripts tests🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_diagnostic_fields.py` around lines 271 - 273, The _is_append_call helper is too broad because it treats any ast.Call with an append attribute as plumbing reachability, so narrow it to only the real diagnostic sink patterns used by the diagnostic-field logic in scripts/check_diagnostic_fields.py. Update the predicate and any callers that rely on it so only genuine sink-shaped appends are counted, not intermediate containers like tmp.append(...). Add or adjust a regression test that exercises a non-sink append and verifies it is not treated as helper output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 287-310: The rebinding counter in _rebound_names currently misses
annotated assignments, so an existing ctor name can still be treated as
unchanged after an AnnAssign overwrite. Update _rebound_names to handle
ast.AnnAssign alongside Assign, AugAssign, and the other binding forms, and add
a test fixture that overwrites a ctor with an annotated assignment so the
reachable-as-result check covers this case.
In `@tests/test_diagnostic_fields.py`:
- Around line 709-753: Add a new regression test in the diagnostic field
coverage around spec_ref_violations_in_source to cover annotated rebinding with
ast.AnnAssign, since the current rebinding checks only exercise ast.Assign and
for-loop targets. Create a fixture in the same style as REASSIGNED_BEFORE_RETURN
and REBOUND_VIA_FOR_LOOP where d starts as a Diagnostic, is then rebound via an
annotated assignment like d: object = self.template, and is returned; assert the
result is still inspected and not skipped. Keep the new test alongside
test_reassigned_local_is_not_reachable_as_result and
test_for_loop_rebound_local_is_not_reachable_as_result so the rebinding logic in
mod.spec_ref_violations_in_source is validated for this additional syntax form.
---
Duplicate comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 271-273: The _is_append_call helper is too broad because it treats
any ast.Call with an append attribute as plumbing reachability, so narrow it to
only the real diagnostic sink patterns used by the diagnostic-field logic in
scripts/check_diagnostic_fields.py. Update the predicate and any callers that
rely on it so only genuine sink-shaped appends are counted, not intermediate
containers like tmp.append(...). Add or adjust a regression test that exercises
a non-sink append and verifies it is not treated as helper output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4ee9dc89-45bd-4dba-9978-54b701c9e777
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…he helper's result _plumbing_ctors already required a genuine helper method (aallan#827's fix) whose sole own-scope Diagnostic() construction is unambiguous, but never confirmed that construction was actually the helper's output. A helper that builds its one ctor and hands it to something other than return/append — e.g. self.dispatch(d) instead of self.errors.append(d) — still had it elected as plumbing and skipped by all three passes, so a bogus spec_ref or unregistered error_code on it would ship silently. _ctor_is_reachable_as_result now requires the sole ctor be return-ed, appended, or bound to a local that is later return-ed/appended, gated after the existing single-ctor check so aallan#827's ambiguity handling is untouched. On vera/ the exempt set is unchanged: all five real helpers return or append their sole ctor directly, so the gap was latent, closed defensively. The name-based return/append match in the second half of the check has no real data-flow: a local rebound after the ctor-binding assign would still match on the name alone and be wrongly treated as reachable. Fixed conservatively via _rebound_names, which counts every name-binding form in own scope — plain and augmented assignment, for/with targets, walrus, and except-as names — not just plain Assign. A name rebound more than once by any of these forms is unreliable, so the ctor is inspected rather than exempted. Separately, _is_append_call treated any .append(...) call as evidence of reachability, including an append to an unrelated throwaway local the helper never reads again. Every real diagnostic-list sink in vera/ has the self.<attr>.append(...) shape, so the check is now scoped to that. Fixes aallan#956 Co-Authored-By: Claude <noreply@anthropic.invalid>
chethanuk
force-pushed
the
fix/issue-956-plumbing-skip-reachability
branch
from
July 9, 2026 05:25
f1267f4 to
078b1d8
Compare
…(PR aallan#964 review) A value-carrying annotated assignment (d: object = self.template) rebinds the local without being an ast.Assign, so a bogus ctor followed by an AnnAssign rebind was still elected as plumbing and skipped by all three passes (probe-confirmed EXEMPT). Dually, d: Diagnostic = Diagnostic(...) was never recognised as the initial binding, permanently inspecting genuine plumbing. Both arms fixed: _rebound_names counts value-carrying AnnAssign targets (a bare annotation does not assign and is not counted), and the binding recognition accepts an AnnAssign whose value is the ctor. Six new fixtures: the two AnnAssign directions (RED-confirmed pre-fix, each arm mutation-killed), the bare-annotation distinction, and one pin each for the with/walrus/except rebind arms so a dropped arm ships RED (PR aallan#965 review ask, pinned here where the machinery lives). Exempt set on vera/ unchanged (the same five plumbing ctors). TESTING.md/ROADMAP counts follow (6,827 -> 6,833).
…s, match captures, params, nonlocal (PR aallan#964 panel review) The enumeration approach was fail-OPEN for every binding form it did not list: starred unpacks (a, *d = ...), import ... as d, match-case captures, a parameter shadowed by the ctor's name, a nonlocal declaration rebinding the helper's local from a nested function, and a return that precedes the binding (order-insensitive name match). Each shape kept a swapped-out bogus ctor exempt (six RED fixtures confirmed pre-fix). _rebound_names now counts bindings generically: any Store/Del-context Name in own scope (subsuming the old enumeration), the string-carrying forms (import-as, except-as, match captures), the helper's parameters, and nonlocal declarations across nested scopes; a bare annotation (no value) is still not a rebind. The return/append name-match gains an order guard (at-or-after the binding line). Params/nonlocal/order/ Store arms each mutation-killed; exempt set on vera/ unchanged. TESTING.md/ROADMAP counts follow (6,833 -> 6,839).
# Conflicts: # CHANGELOG.md # ROADMAP.md # TESTING.md
Owner
|
Merged — thanks @chethanuk, and this one deserves the long comment: the reachability predicate is a genuinely good idea, your revision loop was fast (the rebinding conservatism and the narrowed What the maintainer loop added on top, and the design reasoning:
The exempt set on |
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
2 participants
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.
Fixes #956
Summary
Test plan
Summary by CodeRabbit
Diagnostic(...)calls are exempt only when the single own-scope constructor is provably reachable as the helper’s result (e.g., returned/appended), not merely dispatched.for-loop targets) to avoid incorrect skipping.for-rebound scenarios.