deleted-symbols-gate does not catch a name removed from __all__ while its def survives - #306
Conversation
The deleted-symbols-gate compared def/class definitions between base and
HEAD, but missed names silently dropped from __all__ while their definitions
remained -- the shape a mechanical merge produces when one side of a
conflicted __all__ block is taken wholesale.
Extend the gate to compare __all__ membership via AST walk, not regex, so
continuation lines and column-0 indentation never matter. An entry in base
__all__ absent from head __all__ is a violation only when the corresponding
top-level def/class still exists at HEAD. A genuine deletion removes both
the definition and the __all__ entry, and stays allowed: it is caught by
the existing definition-removal check.
Violation objects gain a kind field ("deleted" or "export-removed") so
the output distinguishes the two shapes. The Removes-Intentionally trailer
waiver covers both kinds uniformly.
|
ⓘ 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: 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 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return symbols | ||
|
|
||
|
|
||
| def _get_all_exports_at_ref(ref: str, repo_root: Path = REPO_ROOT) -> dict[str, str]: |
There was a problem hiding this comment.
WARNING: Duplicated tar extraction logic from _get_symbols_at_ref
This function replicates the entire git archive → tar iteration → .py filter pipeline from _get_symbols_at_ref (lines 138-154). The two functions differ only in the dict variable name and the per-file callback. Extract the shared archive-iteration into a helper (e.g. _iter_python_files_at_ref) to avoid divergent bug fixes when the walk logic changes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| except SyntaxError: | ||
| return exports | ||
|
|
||
| for node in tree.body: |
There was a problem hiding this comment.
SUGGESTION: __all__ inside functions, classes, or conditionals is silently ignored
for node in tree.body: only visits module-level statements. __all__ assigned inside a function, class body, or if block will not be collected. Module-level __all__ is conventional, but documenting this scope limitation (or handling nested assignments) would prevent surprise false negatives.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview| Severity | Count | Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 95K · Output: 28.4K · Cached: 312.6K |
APPROVED. Every acceptance item on
|
… never exercised their own rule (#311) Replaces the all-or-nothing #212 revert with eight per-rule reverts, one per validation rule, so a missing sub-rule can be seen rather than masked by the other seven. That granularity paid: R1 and R5 were false negatives. The test inputs used the string "not a list", which has len 10 and so tripped the max-refs guard, and which iterates into characters and so tripped the item-type guard, before the isinstance check was ever reached. Disabling the is-a-list rule left both tests green. The inputs are now non-iterable (42), and the same perturbation turns both red. Relabels the report TARGETED/UNRELATED, reserving DECORATIVE for tautologies (none found), which removes the contradiction where one test was REAL in one table and DECORATIVE in another. Splits row 13 out: its status assertion is duplicated at the service layer, so only its message assertion is load-bearing. Keeps the test_capability_declarations_do_not_diverge retraction prominent. Reviewer's note on provenance: the PR body's file table and its reported suite count of 1401 belong to a different tree. They credit this PR with scripts/normalise_handle_gate.py, scripts/check_deleted_symbols.py, tests/test_deleted_symbols.py, tests/test_normalise_handle_gate.py and a changelog entry for card tsk-vcda2i, all of which are #306's and #308's already-merged work. This change is two files, 308 insertions, and the suite on the trial merge is 1435 passed / 12 skipped. Card tsk-24ybw5. Review: #311 (comment)
1 participant
CARD TITLE (intent, not commit subject): deleted-symbols-gate does not catch a name removed from all while its def survives
Autonomous build of board card tsk-b5gfwg.
The deleted-symbols-gate compared def/class definitions between base and
HEAD, but missed names silently dropped from all while their definitions
remained -- the shape a mechanical merge produces when one side of a
conflicted all block is taken wholesale.
Extend the gate to compare all membership via AST walk, not regex, so
continuation lines and column-0 indentation never matter. An entry in base
all absent from head all is a violation only when the corresponding
top-level def/class still exists at HEAD. A genuine deletion removes both
the definition and the all entry, and stays allowed: it is caught by
the existing definition-removal check.
Violation objects gain a kind field ("deleted" or "export-removed") so
the output distinguishes the two shapes. The Removes-Intentionally trailer
waiver covers both kinds uniformly.
Files:
changelog.d/tsk-b5gfwg-export-gate.md | 2 +
scripts/check_deleted_symbols.py | 130 +++++++++++++++--
tests/test_deleted_symbols.py | 264 ++++++++++++++++++++++++++++++++++
3 files changed, 386 insertions(+), 10 deletions(-)