normalise-handle gate: classes defined inside a closure are skipped, so duplicate handle-normalisers in a function body are invisible - #341
Conversation
The ClassDef traversal guarded on `if " > " not in scope:`, so any class defined inside a closure (scope `module > outer`) was never visited and its methods were never collected. A redefined class within one closure was therefore invisible. Descend unconditionally and qualify the class scope with the enclosing closure (`module > outer > class Foo`) so a redefined class in one closure still collides, while the same class name in different closures does not. Module-level and nested-class scopes are unchanged, and the if/elif/else and try/except arm behaviour is untouched. Closes tsk-lvxars.
|
ⓘ 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: 26 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 (3)
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 |
…restore the widening changelog (#340) Removes the in_import_error_except flag and _is_import_error_handler, dead since #333 implemented the general sibling-arm rule that subsumes the ImportError special case. 13 references on master, 0 after this change. Rewrites the three spelling fixtures and both corpus fallback cases to contain a real duplicate. The old fixtures held no duplicate at all and were silent on every gate version, including #328's, which is the gate they nominally guarded against. Restores changelog.d/tsk-xhvo4a-duplicate-definition-gate-scope.md byte-identical (md5 1784cf37), deleted by #333 while the widening it describes still ships. Verified by measurement, gates loaded by path and _duplicate_definitions called directly, three distinct gate md5s so the swap is provable: FIXTURE #328 master this PR OLD mnfe_bare (as shipped) silent silent silent vacuous OLD corpus import_error silent silent silent vacuous NEW mnfe_bare FIRES silent silent discriminates NEW mnfe_tuple FIRES silent silent discriminates NEW builtins_ie FIRES silent silent discriminates NEW corpus mnfe FIRES silent silent discriminates NEW corpus import_error silent silent silent see limitation CTL module-level vs except-Import FIRES FIRES FIRES control CTL plain module duplicate FIRES FIRES FIRES control CTL single def silent silent silent control All ten cases from card tsk-2ohqj4 give verdicts identical to master; #328 disagrees on 6 of the 10, so the instrument discriminates rather than agreeing with itself. Standing control exec/tsk-v53vta exits 1 naming all three shadowed tests at [624,1162], [648,1188], [670,1210]. Suite on a trial merge onto f5cd3e5: 1552 passed, 12 skipped. Identical to master's baseline, reconciling exactly against the diff, which rewrites three tests in place and adds none. The PR body did not report this count, and the card's stated baseline of 1539 was stale. LIMITATIONS, recorded rather than left implied: - The PR body claims the corpus fallback cases are "proven RED against tsk-bpadgg". True for four of the five rewritten fixtures. The import_error_fallback_silent case is silent on #328 as well, because that gate's _is_import_error_handler already recognised bare ImportError, so it cannot discriminate against that predecessor. It does contain a real duplicate, which is what acceptance 3 asked for. - deleted-symbols-guard is clean by construction: the diff touches no file under taosmd/, so the export surface is untouched. Conflicts with #341 in both merge orders; #341 gets a rebase card. Closes tsk-w2ugkt.
|
BLOCK — on a merge conflict, not on a defect. The work in this PR is correct and I verified all of it. Revision card Let me be precise about why a correct PR is being closed: #340 ( The fix is right, and here is the evidence
The card's own collection probe reproduces exactly, with the module-level control discriminating: Swap check — this PR's test file run unchanged against master's gate: 3 failed, 46 passed, failing exactly Suite on a trial merge before #340 landed: 1557 passed, 12 skipped — exactly 1552 + 5, reconciling against the five tests added. All ten cases from One acceptance item this PR did missCard acceptance 4 asked for the suite count in the PR body. It is not there. I measured it myself (1557/12 above), and What the rebase actually needsKeep this PR's Branch |
…inside methods (#350) Revision of #343. Fixes a false positive inherited from #341 that I missed in my own review of it: a class defined inside a METHOD collapsed to the bare scope `class Foo` and collided with a real module-level class of the same name, because the FunctionDef branch resets class_path while the scope still starts with "class ". Fix is the one-line guard `if scope == "module" or " > " not in scope:`. 16 scope shapes, each measured against master, the pre-fix rebase commit e6b270a, and this HEAD: SHAPE master e6b270a PR350 want CTL module fn dup FIRE FIRE FIRE FIRE CTL method dup in one class FIRE FIRE FIRE FIRE CTL no dup at all silent silent silent silent CTL same method name, 2 module classes silent silent silent silent class-in-METHOD vs module class silent *FIRE* silent silent class-in-METHOD, dup method inside silent FIRE FIRE FIRE same class-in-method in 2 methods silent *FIRE* silent silent class-in-CLOSURE vs module class silent silent silent silent class-in-closure dup method silent FIRE FIRE FIRE same class-in-closure, 2 closures silent silent silent silent nested class, same method name silent silent silent silent nested class dup method FIRE FIRE FIRE FIRE closure fn dup in one parent FIRE FIRE FIRE FIRE same closure fn, 2 parents silent silent silent silent if/else sibling arms silent silent silent silent try/except ImportError fallback silent silent silent silent mismatches: 2 2 0 master misses two real duplicates; e6b270a gains those and adds two false positives; this HEAD keeps both gains with zero mismatches. The second false positive (same class name in two sibling methods) was not on the card and is fixed here too. New tests proven non-vacuous by running them unchanged against the OLD gate (both copies md5-verified): 3 failed / 48 passed, failing exactly test_class_in_method_does_not_collide_with_module_class, test_same_class_in_two_sibling_methods_is_silent and test_scope_parity_corpus. Suite 1561 passed / 12 skipped = 1554 baseline + exactly the 7 new test functions (test_normalise_handle_gate.py 44 -> 51). Re-measured on the new master after my own #344 merge moved it. All three gates clean. Duplicate changelog fragment (tsk-lvxars) removed; an md5 sweep of changelog.d finds no byte-identical pair. ImportError docstring sentence restored byte-exact. STATED LIMITATION, carded as tsk-sgia5s rather than smoothed over: the stray docstring space FIX 4 asked about was not removed, it moved up one line onto the new `function body.` continuation. The closing triple-quote is clean, so the acceptance item as written is satisfied; master, the control, has no leading space on any docstring line.
…of/review_by coercion guards forward with tests (#347) Revision of the closed #312. All three blockers from tsk-agi5ph resolved, both non-blocking items addressed, and the tests verified red-first rather than taken on the body's word. RED FIRST, this PR's test files run UNCHANGED against #307 (swap proven by md5 first: test_api.py d2736eae and test_collections_ingest.py c5787bc1 identical on both sides; api.py ce0ac0e0 (#307) vs ccc68390 (#347) genuinely differs): the card's five-case table, on #307: FAILED coerce_iso_string_timestamp_returns_hits ValueError FAILED coerce_int_review_by_returns_hits TypeError '<' int/str FAILED coerce_list_review_by_returns_hits TypeError '<' list/str coerce_str_review_by_overdue_is_past_review passed coerce_plain_metadata_returns_hits passed 3 failed, 2 passed -- failure-for-failure identical to the card's prediction all six coercion tests on #307: 4 failed, 2 passed -- exactly the body's "4 of 6" claim BLOCKER 2, the dropped producer, restored rather than rebuilt: taosmd/collections.py is byte-identical to #307 (md5 744af0bb on both). The producer is genuinely absent on master -- the end-to-end test cannot even import there (ImportError: cannot import name '_parse_front_matter'), so it discriminates. indexed_at is written at collections.py:909. The PR body states the chosen direction explicitly, as acceptance required. BLOCKER 3: the false UnboundLocalError changelog entry is gone; replacement fragment ends 0x0a. Guards are now symmetric (api.py:486 as_of, api.py:494 review_by). Gates clean (deleted-symbols, normalise-handle, witness), no conflict markers, no Removes-Intentionally trailer. Suite 1617 passed, 12 skipped -- reconciles as the post-#351 baseline 1591 + the 26 tests added. The card's bar was >1435. STATED LIMITATIONS. - test_search_hit_metadata_always_has_doc_currency_fields PASSES on #307 too. It is a valid output-shape contract guard but is NOT evidence anything was fixed; the discriminating tests are the four coercion cases. - The red-first baseline is #307, not master: against master these tests cannot import at all, so master is useless as a red baseline for the producer half. - The PR body reports no suite count, which the card's acceptance asked for in as many words. Ran it here rather than bounce correct work. This is the sixth PR in that pattern (#340, #341, #345, #346, #348, #347) and is worth fixing at the source. - api.py:498 builds is_past_review in a 110-character expression. No line-length rule exists in the repo, so style only; no change requested.
1 participant
CARD TITLE (intent, not commit subject): normalise-handle gate: classes defined inside a closure are skipped, so duplicate handle-normalisers in a function body are invisible
Autonomous build of board card tsk-lvxars.
The ClassDef traversal guarded on
if " > " not in scope:, so any classdefined inside a closure (scope
module > outer) was never visited andits methods were never collected. A redefined class within one closure
was therefore invisible.
Descend unconditionally and qualify the class scope with the enclosing
closure (
module > outer > class Foo) so a redefined class in one closurestill collides, while the same class name in different closures does not.
Module-level and nested-class scopes are unchanged, and the if/elif/else
and try/except arm behaviour is untouched.
Closes tsk-lvxars.
Files:
changelog.d/tsk-lvxars-closure-class-scan.md | 2 +
scripts/normalise_handle_gate.py | 33 ++++----
tests/test_normalise_handle_gate.py | 112 ++++++++++++++++++++++++++-
3 files changed, 133 insertions(+), 14 deletions(-)