Revise PR #343: the rebase is correct, but the closure-class scoping rule false-positives on a class defined inside a METHOD (inherited from #341, missed in my review of it) and the changelog entry ships twice - #350
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 > factory > 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. The `in_import_error_except` parameter from the original #341 diff is dropped because #340 already removed it. Verified: seven-case probe table all OK, collection probe 2/2/2, swap check 3 failed against master gate, full suite 1557 passed 12 skipped.
…de methods Revise PR #343: the rebase itself was correct, but the new class-scope guard false-positives on a class defined inside a method. The guard checked scope.startswith('class '), which matches a method closure scope ('class Outer > make') as well as a bare class scope ('class Foo'). Replace it with 'scope == module or > > not in scope', which distinguishes the two: a pure class nest never contains ' > ', while a method closure always does. Also: - Delete the duplicate changelog fragment tsk-lvxars-closure-class-scan.md (byte-identical copy of tsk-ja3pr4, never landed on master). - Restore the ImportError/ModuleNotFoundError docstring sentence to master's mechanism-accurate wording about the sibling-arm rule. - Remove the stray leading space before the docstring terminator. - Add tests for the two missed cases (class-in-method vs module-level class, same class name in two sibling methods) to the scope-parity corpus and as standalone assertions.
|
ⓘ 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: 32 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}
| same sibling-arm rule, since at most one arm ever binds. | ||
| Nested classes are scanned at any depth. | ||
| Nested classes are scanned at any depth, including those defined inside a | ||
| function body. |
There was a problem hiding this comment.
SUGGESTION: Stray leading space in docstring continuation line
The line function body. has an unintended leading space. The PR description explicitly calls for removing this space ("Remove the stray leading space before the docstring terminator"), but the diff shows it being added instead. This breaks the visual consistency of the docstring where every other continuation line starts at column 0.
| function body. | |
| function body. |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 99.4K · Output: 31.1K · Cached: 429.2K |
MERGE. The scoping fix is correct across every shape I could enumerate, and it also closes a second false positive the card never named.This is the revision of #343, which I blocked while calling the rebase itself correct. That block was on a defect I had missed in my own review of #341 — a class defined inside a METHOD getting the bare scope I enumerated the scope shapes myself rather than taking the test list as the shape listThat was the exact hole that let the defect through two reviews. 16 shapes, each run against master, the pre-fix rebase commit master misses two real duplicates (what #341 set out to fix). The fix is the one-line guard Two corrections to my own probe, recorded because both would have passed silentlyMy first harness used The second harness put the method body at the same indent as The new tests are non-vacuous — proven by running them against the OLD gateNew test file taken unchanged from this branch, dropped onto That is cases A and B plus the corpus, failing for exactly the right reason, and the The remaining acceptance items1561 = the 1554 baseline after my #344 merge + exactly the 7 new test functions ( Carded, not blocked:
|
…ationships honestly (#348) Revision of #342, which was blocked because the fabrication had migrated from numbers to a RELATIONSHIP. That block is resolved: every number and every enumeration here was re-measured independently at review time. CLAIM MEASURED VERDICT oracle byte size 15388478 stat -c %s -> 15388478 ok oracle sha256 821a2034d219ab45... sha256sum -> 821a2034... ok oracle questions 500 len(json) -> 500 ok evidence-only haystacks, all 500 500 / 500 exact, 0 differing ok control: distinct question_ids 500 (file is not degenerate) s_full.json "does not exist on this machine" find -> 0 hits, whole box ok control: oracle find -> 10 hits, 3 repos, all byte-identical clean_longmemeval.py "does not exist" find -> 0 hits ok longmemeval_granularity.py uses s_cleaned DATA_PATH line 24 ok oracle loader list (was 2 of 3) grep -> exactly those 3 ok 97.0% / REPRODUCE-longmemeval.md sentence restored ok The new 9-runner enumeration replacing the `longmemeval_*` catch-all reconciles in both directions: 9 of 9 real, 0 invented. The 3 found-but-unclaimed are the README itself, REPRODUCE-longmemeval.md (a doc, cited separately) and run_on_fedora.sh (a shell wrapper), none of them runners. Marking longmemeval_s_cleaned.json as NOT YET PINNED rather than guessing its size and count is the right call and the opposite of what this chain was blocked for twice. Suite 1561 passed / 12 skipped = exactly the post-#350 baseline, which is what a docs-only change must produce. All three gates clean; no conflict markers. The automated REVIEW WARNING on this PR is a FALSE POSITIVE, now 4 for 4 (#331, #342, #345, #348). tsk-7cl7rk asks for measurements, never tests, and this is a README plus a changelog fragment. Do not re-triage it on a docs card. CARDED, not blocked: - tsk-r44fqf: acceptance item 3 asked the PR body to state whether the longmemeval_s_full.json pin was verified and to file a card if not; neither was done. The pin (277383467 bytes, d6f21ea9...) is not known to be wrong, it is UNVERIFIABLE on this box, and it sits in the one file whose numbers have been wrong three times running, formatted identically to the numbers that are checked. Card filed by the reviewer rather than bouncing a correct docs fix. - tsk-sgia5s: the README lost its trailing newline (0x0a on master, 0x2a here). Control: 38 of 40 sampled .md files on master end 0x0a, and the 2 that do not are changelog fragments already covered by tsk-v2pr2b. Note for the record, not actioned here: there is still no gate on this file (git grep -ln 'longmemeval|data/README' -- scripts .github -> 0 hits), because tsk-yssnei's doc-gate died with #334. That remains tsk-r4j272 territory, and it is why all three fabrications in this chain had to be caught by hand.
1 participant
CARD TITLE (intent, not commit subject): Revise PR #343: the rebase is correct, but the closure-class scoping rule false-positives on a class defined inside a METHOD (inherited from #341, missed in my review of it) and the changelog entry ships twice
Autonomous build of board card tsk-odrncg.
Revise PR #343: the rebase itself was correct, but the new
class-scope guard false-positives on a class defined inside a method.
The guard checked scope.startswith('class '), which matches a method
closure scope ('class Outer > make') as well as a bare class scope
('class Foo'). Replace it with 'scope == module or > > not in scope',
which distinguishes the two: a pure class nest never contains ' > ',
while a method closure always does.
Also:
(byte-identical copy of tsk-ja3pr4, never landed on master).
master's mechanism-accurate wording about the sibling-arm rule.
class, same class name in two sibling methods) to the scope-parity
corpus and as standalone assertions.
Files:
changelog.d/tsk-ja3pr4-closure-class-scan.md | 2 +
changelog.d/tsk-odrncg-gate-method-class-scope.md | 2 +
scripts/normalise_handle_gate.py | 31 +++--
tests/test_normalise_handle_gate.py | 162 ++++++++++++++++++++++
4 files changed, 185 insertions(+), 12 deletions(-)