Skip to content

deleted-symbols-gate is blind to a re-export dropped from a package __init__.py - #314

Merged
jaylfc merged 1 commit into
masterfrom
exec/tsk-cssuku
Aug 17, 2026
Merged

deleted-symbols-gate is blind to a re-export dropped from a package __init__.py#314
jaylfc merged 1 commit into
masterfrom
exec/tsk-cssuku

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): deleted-symbols-gate is blind to a re-export dropped from a package init.py

Autonomous build of board card tsk-cssuku.

The export-removal clause in find_removed_all_entries only matched
all entries whose def/class lived in the same file. Re-exports
(names imported via from-import, not defined locally) never matched, so
dropping e.g. KnowledgeGraph from taosmd/init.py all while the
import line and the definition survived went unreported.

Add _extract_imports/_get_imports_at_ref to resolve from-import
bindings to candidate definition keys, and extend
find_removed_all_entries to follow those bindings when a same-file
definition is absent. _resolve_export_key lets the existing def and
class search target the real definition file for an accurate added-by.
A genuine deletion that removes the all entry, the import, and the
definition is not flagged by the export half (it is caught by the
def-deletion half).

Adds TestExtractImports, re-export cases to TestFindRemovedAllEntries,
TestResolveExportKey, TestReexportRemovalIntegration, and
TestReexportRemovalControl.

Files:
changelog.d/tsk-cssuku-reexport-gate.md | 2 +
scripts/check_deleted_symbols.py | 142 ++++++++++++-
tests/test_deleted_symbols.py | 360 ++++++++++++++++++++++++++++++++
3 files changed, 495 insertions(+), 9 deletions(-)

The export-removal clause in find_removed_all_entries only matched
__all__ entries whose def/class lived in the same file.  Re-exports
(names imported via from-import, not defined locally) never matched, so
dropping e.g. KnowledgeGraph from taosmd/__init__.py __all__ while the
import line and the definition survived went unreported.

Add _extract_imports/_get_imports_at_ref to resolve from-import
bindings to candidate definition keys, and extend
find_removed_all_entries to follow those bindings when a same-file
definition is absent.  _resolve_export_key lets the existing def and
class search target the real definition file for an accurate added-by.
A genuine deletion that removes the __all__ entry, the import, and the
definition is not flagged by the export half (it is caught by the
def-deletion half).

Adds TestExtractImports, re-export cases to TestFindRemovedAllEntries,
TestResolveExportKey, TestReexportRemovalIntegration, and
TestReexportRemovalControl.
@qodo-code-review

Copy link
Copy Markdown

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

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 7 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: b38339f8-052d-4c7b-a720-105fb754e600

📥 Commits

Reviewing files that changed from the base of the PR and between 0bf8c9f and c150050.

📒 Files selected for processing (3)
  • changelog.d/tsk-cssuku-reexport-gate.md
  • scripts/check_deleted_symbols.py
  • tests/test_deleted_symbols.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

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

@gitar-bot

gitar-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

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

kilo-code-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • scripts/check_deleted_symbols.py - New re-export resolution logic (_resolve_import_parts, _extract_imports, _get_imports_at_ref, _resolve_export_key) and integration into find_removed_all_entries and check_deleted_symbols. Implementation is correct, backward-compatible, and follows existing patterns.
  • tests/test_deleted_symbols.py - Comprehensive unit and integration tests for re-export detection, aliased imports, legitimate deletions, and control cases.
  • changelog.d/tsk-cssuku-reexport-gate.md - Changelog entry.

Reviewed by step-3.7-flash · Input: 106.4K · Output: 35.7K · Cached: 347.5K

@jaylfc

jaylfc commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

MERGE — all four controls reproduced independently, on real repository input rather than fixtures

Reviewed at c150050a on a trial merge with master 8aa62e4 (clean, ort).

Every acceptance item was re-run against the repo itself rather than read out of the test file,
and the RED was proved by running both gates against the same committed tree so the two answers
have to disagree before the new one counts.

RED — the precise case the card filed, and it is the aliased one

taosmd/__init__.py:3 is from .knowledge_graph import TemporalKnowledgeGraph as KnowledgeGraph,
so this is the harder shape: the exported name never appears as a definition anywhere. Dropped
"KnowledgeGraph" from __all__, left the import line intact, committed:

NEW gate (this PR)      exit 1
  DELETED-SYMBOLS FAIL: this PR removes 1 symbol(s) from __all__ while the definition
  still exists at HEAD:
    - taosmd/__init__.py:KnowledgeGraph (added by 93d9ff0 Initial release: taOSmd 0.1.0 ...)

OLD gate (master's), same tree, same command
  deleted-symbols-guard: clean          exit 0

That is the blind spot closing, measured. _resolve_export_key also does its job: the added-by
attribution resolves through the alias to the real definition rather than reporting nothing.

CONTROL 1 — a legitimate deletion still does not trip the export half

Removed the __all__ entry, the import line, and the class TemporalKnowledgeGraph block itself
(lines 103-727 of knowledge_graph.py), committed, re-ran:

export half:      silent — no __all__ section in the output at all,
                  and 0 occurrences of "taosmd/__init__.py:KnowledgeGraph"
definition half:  DELETED-SYMBOLS FAIL: ... deletes 21 symbol(s) ...

Exactly the split the card asked for: the deletion is reported once, by the half that owns it.

CONTROL 2 — no regression on the standing known-bad branch

Replayed both gates against origin/exec/tsk-uyznqh and diffed the export-half findings:

taosmd/config.py:get_human_principal_ids
taosmd/config.py:set_human_principal_ids
taosmd/registry_auth.py:HumanAuthError
taosmd/registry_auth.py:RegistryVerifier.is_human
taosmd/service.py:a2a_thread_messages
taosmd/service.py:a2a_threads
taosmd/service.py:fetch_by_ref

diff old new -> IDENTICAL

All three names #306 was carded for are still there, and nothing was added or lost.

CONTROL 3 and the rest

clean merged tree           deleted-symbols-guard: clean, exit 0
conflict markers            clean
normalise-handle-gate       clean, exit 0
full suite (trial merge)    1461 passed, 12 skipped, exit 0

Scan scope is consistent — all three git archive walks filter on taosmd/, so the import
resolution sees exactly the surface the export and definition scans do.

Notes, none blocking

  1. The PR body does not report a suite count, which was acceptance item 5. It is the one item I
    had to supply myself. The body is otherwise accurate: the file table matches the real diff
    line-for-line, which is not something I can take for granted lately.
  2. The suite is 1461 passed / 12 skipped, up 26 from master's 1435 — the count moves, which is
    what a PR carrying 360 lines of tests should look like.
  3. Your test module cannot go red against the unfixed gate — it imports _extract_imports at
    module level, so against master it produces a collection error rather than a failing assertion.
    That is the weaker form of red flagged in deleted-symbols-gate does not catch a name removed from __all__ while its def survives #306's review, and it is why the evidence above comes
    from running the two gate versions against the same tree instead. The tests themselves are good
    (360 lines, real tmp_path git repos driving the gate end to end, including the alias case and
    a waiver case); this is about what they can prove on their own, not their quality.
  4. _extract_imports walks ast.iter_child_nodes, so it sees top-level from-imports only.
    Measured on master, this is adequate today and I am not asking you to widen it:
    taosmd/__init__.py 38 of 38 top-level, loaders/__init__.py 8 of 8, claims/__init__.py 1 of
    1 — zero nested. That is the same "does not need to handle those shapes yet" bar the card set
    for __all__ assignment forms. Worth one docstring line so the next reader does not assume an
    import inside try: or if TYPE_CHECKING: resolves, because it will not.

One trap for whoever re-runs this comparison

Copying the old gate to /tmp to run it side by side does not work and does not fail honestly
either: REPO_ROOT derives from Path(__file__).parent.parent, so it resolved to / and died in
git archive with a CalledProcessError. Copy the comparison script into the tree under test.
I hit this mid-review and it cost a probe.

Merging. Card tsk-cssuku closed.

Reviewed by @jaylfc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant