Duplicate-definition gate cannot see definitions inside module-level control flow or same-name closures in one parent

[jaylfc]

CARD TITLE (intent, not commit subject): Duplicate-definition gate cannot see definitions inside module-level control flow or same-name closures in one parent

Autonomous build of board card tsk-xhvo4a.

• Definitions inside module-level if/try/for/while/with now share module scope
• Same-name closures inside one parent function are reported
• Same-name closures in different parents stay legal
• try/except ImportError fallback pattern stays silent
• Added tests for all four new behaviours

Files:
.../tsk-xhvo4a-duplicate-definition-gate-scope.md | 2 +
scripts/normalise_handle_gate.py | 178 ++++++++++++++++++---
tests/test_normalise_handle_gate.py | 111 ++++++++++++-
3 files changed, 269 insertions(+), 22 deletions(-)

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 19 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ba0d3574-a589-4821-a6eb-3d0bd31a4738

📥 Commits

Reviewing files that changed from the base of the PR and between 0cc66e6 and eae3cfa.

📒 Files selected for processing (3)

• changelog.d/tsk-xhvo4a-duplicate-definition-gate-scope.md
• scripts/normalise_handle_gate.py
• tests/test_normalise_handle_gate.py

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[kilo-code-bot]

WARNING: _is_try_import_error_fallback does not account for definitions in else or finally blocks

_collect_definitions recurses into node.orelse (line 208) and node.finalbody (line 218) with in_try=True, so those definitions are included in try_body_defs (line 105). The fallback check requires exactly one try-body-side definition, but a legitimate try/except ImportError pattern with a same-named definition in the else block (or finally block) will have two try-body-side definitions and be incorrectly reported as a duplicate.

For example, the following valid import-fallback pattern would be flagged:

try:
    from foo import bar
except ImportError:
    from foo import baz as bar
else:
    def bar():  # same name, different purpose
    ...

_is_try_import_error_fallback should exclude else/finally definitions from the try-body count, or explicitly check that no extra definitions exist in those blocks.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
WARNING	1

Issue Details (click to expand)

WARNING

File	Line	Issue
scripts/normalise_handle_gate.py	107	_is_try_import_error_fallback does not account for else/finally blocks — a name defined in else or finally alongside the try-body and except ImportError definitions will be incorrectly reported as a duplicate

Files Reviewed (3 files)

• scripts/normalise_handle_gate.py - 1 issue
• tests/test_normalise_handle_gate.py
• changelog.d/tsk-xhvo4a-duplicate-definition-gate-scope.md

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 47.3K · Output: 9.9K · Cached: 412.2K}

[jaylfc]

Review: BLOCK

Reviewed at eae3cfa, trial-merged with master 0cc66e6. Both of the card's work items are genuinely delivered and red-proven, and I reproduced that independently before looking for anything else. What blocks is narrower than the work: the widened scan treats two mutually exclusive if/else arms as sequential rebinding, so it now fires on correct code, and it drops a scope master's gate was scanning.

Suite on the trial merge: 1485 passed, 12 skipped (master baseline 1479/12). That is +6 against 8 new tests, and the difference is accounted for: test_nested_functions_are_not_collected is deliberately removed because this PR inverts the behaviour it asserted, and test_try_except_import_error_fallback_is_silent is renamed to ..._with_defs_is_silent with a stronger body. Both removals are correct. Marker grep and check_deleted_symbols.py --base origin/master clean, exit 0, on the merged tree. No merge commits on the branch.

The card's work items are done, and proven RED first

Master's gate and this branch's gate over the same inputs, required to disagree:

                                                   master   #321
module-level def + def inside `if`                  clean    FIRES module:handler@[2,5]
two same-name closures in ONE parent                clean    FIRES module > outer:inner
same-name closures in DIFFERENT parents             clean    clean      (must stay legal)
try/except ImportError fallback                     clean    clean      (must stay silent)
original duplicate _normalise_handle                FIRES    FIRES

Swapping master's gate under this branch's new tests: 6 of the 8 fail. The 2 that pass on both are ..._closures_in_different_parents_are_fine and ..._import_error_fallback_with_defs_is_silent, which are green on both trees by design — they forbid an over-wide fix, the same shape as the third test in #315. That is the right way to build them.

Standing control origin/exec/tsk-v53vta still fires, byte-identical to master's gate: all 3 shadowed tests named at 624/1162, 648/1188, 670/1210. Real tree clean across 183 files, exit 0.

---

BLOCKER 1 — if/else arms are alternatives, not rebinding

scripts/normalise_handle_gate.py:176 folds node.orelse into the same scope as node.body, so the two arms of one if collide with each other:

import sys
if sys.platform == 'win32':
    def p(): ...
else:
    def p(): ...

master   silent
#321     FIRES module 'p' defined on lines [3, 5]

Exactly one of those arms ever executes. Nothing is shadowed and nothing is discarded, which is the whole justification for the rule. This is the canonical platform-split idiom and it is the same class of false positive the card names as having blocked #305 — the card named the try/except ImportError instance, and that one is handled, but the class is not.

if TYPE_CHECKING: stub plus a real definition fires the same way (silent on master, FIRES here).

The distinction the scan needs is one the card's own example already implies:

• module-level def f followed by def f inside an if body — both bind, second wins, fire. This is work item 1 and it works today, keep it.
• def f in an if body vs def f in that same if's orelse — mutually exclusive, stay silent.

So collect each arm against the enclosing module scope, but not against its sibling arms. Same for elif chains and for try/except/else arms of one statement.

BLOCKER 2 — the ImportError fallback is recognised by spelling, not by meaning

:91 and :94 compare the handler name to the literal string ImportError:

try/except ImportError          -> silent   (correct)
try/except ModuleNotFoundError  -> FIRES    (false positive; master: silent)
try/except (ImportError, OSError) -> silent (correct)

ModuleNotFoundError is a subclass of ImportError and is the modern spelling of precisely the fallback the card requires to stay quiet. The same legitimate pattern passes or fails depending on which name the author wrote. Accept the subclass spellings.

BLOCKER 3 — nested classes are no longer scanned at all

:149, the if scope == "module" guard on ClassDef, means a class nested inside a class is never descended into. Master's gate recursed on any ClassDef regardless of depth:

class Outer:
    class Inner:
        def run(self): ...
        def run(self): ...

master   FIRES class Outer.Inner 'run' defined on lines [3, 5]
#321     clean

Lost coverage introduced by this PR, in a PR whose entire purpose is to widen the gate's vision. Live exposure is 0 — I sized it the same way the card sized #316's narrowing, and there are no nested classes anywhere in the 183 files of taosmd/ and tests/. On its own I would have carded this rather than held the build, exactly as I did for #316. It is in the block because the branch is going back for blockers 1 and 2 anyway and this is a one-line guard.

---

Also measured, not blocking

• A duplicate closure where one of the pair sits inside a control-flow block inside a function stays invisible, because the if scope == "module" guards stop the descent one level in:

  def outer(flag):
      if flag:
          def inner(): ...
      def inner(): ...

  clean on master and clean here. Not a regression, and it is the same binding truth as work item 1 one level down. 5 such definitions on the tree, 0 duplicates among them.

• Methods of a class defined inside a function are scanned by neither gate. 15 such classes on the tree.

• The input master's test_try_except_import_error_fallback_is_silent used (import inside try, pass in the handler, def outside) is no longer tested by name. I checked it is still silent under the new gate, so this is coverage bookkeeping rather than a hole — worth keeping both inputs.

One systemic ask for the revision

This is the second consecutive change to this gate that widened one scope and silently narrowed another; #316 dropped control-flow and closure scopes, this one drops nested classes. Both times it surfaced only because the old and new gates were run over the same inputs and required to disagree. Please add that comparison as a test: a small fixture corpus covering every scope the gate claims to handle (module, class, nested class, closure, closure-in-closure, control-flow, try arms, if/else arms) with the expected verdict pinned per case. Then a future change that drops a scope fails a test instead of needing a reviewer to notice.

---

Closing under the close-on-block policy, with a revision card to follow that carries forward everything proven good above — both work items, the eight tests, the two correct removals — so none of it is rebuilt. Branch exec/tsk-xhvo4a is preserved.

[jaylfc]

Revision card is tsk-bpadgg — Revise PR #321: if/else arms are alternatives not rebinding, accept ModuleNotFoundError, restore nested-class scanning. It carries forward everything proven good here (both work items, all eight tests, the two correct removals, the _Def / fallback-pairing machinery) so none of it is rebuilt, and it makes the scope-parity corpus test an acceptance item rather than a suggestion.