normalise-handle gate is keyed on one hardcoded name, so duplicate defs of anything else (including shadowed tests) are invisible - #316
Conversation
The duplicate-definition gate was keyed on one hardcoded name (_normalise_handle), so duplicate definitions of anything else were invisible, including shadowed test functions. Generalise it to report any top-level function or method name defined more than once in the same module, and extend the scan to cover tests/. @typing.overload stubs and @Property / @<name>.setter / @<name>.getter / @<name>.deleter accessor pairs remain excluded as legitimate same-name definitions, reusing the decorator-skip introduced by #308. Proven red against origin/exec/tsk-v53vta, where three shadowed test_a2a_banner_mode_* functions in tests/test_http_server.py are defined twice, and green on current master.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| count += 1 | ||
| return count | ||
| defs: dict[tuple[str, str], list[int]] = defaultdict(list) | ||
| for scope, name, lineno in _collect_definitions(tree.body): |
There was a problem hiding this comment.
WARNING: _collect_definitions(tree.body) misses functions inside module-level control flow
_collect_definitions only iterates over tree.body, so functions defined inside module-level if, try, with, for, while, or match blocks are invisible to the gate. The previous implementation used ast.walk(tree) and would catch duplicate definitions anywhere in the module. This narrowing means shadowed tests or redefs inside conditional blocks at module scope will be silently missed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| monkeypatch.setattr(nhg, "REPO_ROOT", repo) | ||
| rc = main([]) | ||
| assert rc == 0 | ||
| main([]) |
There was a problem hiding this comment.
SUGGESTION: test_clean_output_when_passing does not assert the return code
Unlike the other integration tests, this test calls main([]) without checking the return value. A regression that returns non-zero while still omitting the word "fail" from stdout would not be caught. Assert rc == 0 to match the other integration tests.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 87.5K · Output: 20.3K · Cached: 438.7K |
Review: MERGEVerified independently rather than read. Every claim below was measured on this branch at The headline claim reproduces on real repo inputNot a fixture. The gate from this branch, copied into the preserved known-bad branch All three names and all six line numbers match the card exactly, and the old gate is blind on the identical tree. That is the discriminating shape: same perturbation, both versions, forced disagreement. The controls that had to hold, and didEleven synthetic repo roots, each built twice, with each gate version copied into the tree it scans (
The first row is the one I most wanted to see. Generalising the alphabet did not cost the name the gate was built for, and there is a test pinning it ( On the real merged tree, across 183 files of Suite, read against what the diff touched1473 passed, 12 skipped on a trial merge with One thing that looks alarming and is not: Conflict markers: none. Deleted-symbols gate from inside the merged tree: clean. The tests are realI checked for the failure mode that has cost this repo three PRs today, tests that recompute the production logic inside the test body and assert against their own arithmetic. These do not. They write real files, call the real functions, and assert on real output, and the integration tests monkeypatch One scope change that is not a blocker but is going on the recordThe implementation swaps Kilo flagged the first of these at line 109 and it is correct. I had measured it before reading the comment, which is why it is stated here as confirmed rather than repeated. I sized it rather than guessing: the real tree has 5 definitions inside module-level control flow and 190 nested directly inside another function, and under a maximal-scope checker zero of them are currently duplicated. So this is a forward gap, not a live regression, and on today's tree this gate is strictly better than the one it replaces. The nested-function exclusion is documented at the definition with a reason; the control-flow one is not, and the stated reason ("same-name closures in different parents are legal") does not cover two closures in the same parent. Both go on a follow-up card rather than against this build, because the card asked for top-level functions and methods and that is what was delivered. Kilo's other note, that Merging. |
#319) A2A auth-mode banner: tests now assert on real serve() output The banner production change is carried forward byte-identical from #310 (git diff against exec/tsk-v53vta over taosmd/ is empty); that diff was already reviewed and verified correct. What changed is the tests. The previous six tests re-implemented serve()'s mode/where logic inside the test body and asserted against their own copy, so all six passed against unfixed master and measured nothing. Three test functions were also defined twice, silently discarding three of nine. The replacements run serve() in a real subprocess, read its actual stdout, and assert against literal expected strings. Each subprocess emits a PROVENANCE marker that the test checks against http_server.__file__, so a stale installed copy cannot produce a false pass. Verified by reverting only taosmd/http_server.py to master on the merged tree, leaving the new tests in place, and confirming the banner string was absent (grep count 0): 6 failed against unfixed production, 6 passed against the fixed tree. The duplicate-definition gate merged in #316 reports clean on this tree, independently confirming the shadowed duplicates are gone. Suite on a trial merge with fc44ec6: 1479 passed, 12 skipped (baseline 1473, plus exactly the 6 new tests). The PR body reports 1441, correct against its own base 0bf8c9f and stale only because master moved twice. Card: tsk-6xrv7d
…class scanning, and pin every scope with a parity corpus (#333) Fourth pass over this gate and the first where every blocker moved. All ten cases from card tsk-2ohqj4's table now give the required verdict, re-measured by importing each gate version by path and calling _duplicate_definitions directly: CASE master #328 #333 WANTS CTRL+ modlevel def then def in if-body silent FIRES FIRES FIRES CTRL+ plain module duplicate FIRES FIRES FIRES FIRES CTRL- closures in DIFFERENT parents silent silent silent silent CTRL- ImportError fallback silent silent silent silent B1 platform if/else arms silent FIRES silent silent B1 elif chain arms silent FIRES silent silent B1 try/except/else sibling arms silent FIRES silent silent B2 ModuleNotFoundError fallback silent FIRES silent silent B2 ModuleNotFoundError in a tuple silent FIRES silent silent B3 nested-class duplicate methods FIRES silent FIRES FIRES The master and #328 columns reproduce the card's recorded table exactly, which is what shows the instrument is sound rather than agreeing with itself. Sibling arms are tracked by (statement id, arm); a definition with no arm tracker always collides. So module-level def still collides with def in an if body, while if and else arms do not collide with each other, which is the binding truth the rule encodes. Nested classes are scanned at any depth with the correct class Outer.Inner scope string. The tests discriminate: this PR's test file scores 44 passed against the shipped gate and 6 failed / 38 passed against #328's gate. test_scope_parity_ corpus is among the failures. It asserts the full verdict tuple (scope, name, exact lines) from literals rather than recomputed logic, so a scope that silently stops firing fails loudly. That is what neither #316 nor #321 had and is the systemic ask of the card. Standing control exec/tsk-v53vta still exits 1 naming all three shadowed tests at [624,1162] [648,1188] [670,1210]. Real tree clean, exit 0. rev-list --merges is 0. The gate blob changed from 0cbab67 to 9ffd023, which is the check #328 failed. Suite 1539 passed, 12 skipped = the 1522 baseline plus exactly the 17 net new tests. Three defects found and carded as tsk-w2ugkt rather than blocked, none a behaviour regression: 1. in_import_error_except is dead code. _is_import_error_handler sets a flag threaded through eleven sites that _duplicate_definitions never reads; the dedup branches only on arm_tracker. Patching it to return False changes no verdict on any constructible input and the full 44-test file still passes. It went dead because this PR's general sibling-arm rule subsumes the special case #321 added. 2. The three exception-spelling tests have zero power: each fixture holds one def and no duplicate at all, so the result is [] on every gate ever written. Same for the corresponding corpus case. 3. changelog.d/tsk-xhvo4a-duplicate-definition-gate-scope.md was deleted while the widening it describes still ships, so that change lands with no changelog line. Carded rather than blocked because the behaviour finding 2 nominally protects is genuinely pinned by the sibling-arm tests, which do fail against #328's gate. A test that passes with its feature deleted is normally blocking here; it is not blocking this time only because the feature it names turned out to be dead, so the test documents a mechanism rather than guarding one. Supersedes #328 and #321. Card: tsk-2ohqj4. Follow-up: tsk-w2ugkt.
1 participant
CARD TITLE (intent, not commit subject): normalise-handle gate is keyed on one hardcoded name, so duplicate defs of anything else (including shadowed tests) are invisible
Autonomous build of board card tsk-35ydf3.
The duplicate-definition gate was keyed on one hardcoded name
(_normalise_handle), so duplicate definitions of anything else were
invisible, including shadowed test functions. Generalise it to report any
top-level function or method name defined more than once in the same
module, and extend the scan to cover tests/.
@typing.overload stubs and @Property / @.setter / @.getter /
@.deleter accessor pairs remain excluded as legitimate same-name
definitions, reusing the decorator-skip introduced by #308.
Proven red against origin/exec/tsk-v53vta, where three shadowed
test_a2a_banner_mode_* functions in tests/test_http_server.py are
defined twice, and green on current master.
Files:
.github/workflows/normalise-handle-gate.yml | 12 +-
.../tsk-35ydf3-generalise-duplicate-gate.md | 2 +
scripts/normalise_handle_gate.py | 120 +++++++--
tests/test_normalise_handle_gate.py | 291 +++++++++++++++------
4 files changed, 317 insertions(+), 108 deletions(-)