From 8d9632e2d0032e2d1627c244a87efbe75b086bfd Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Sun, 12 Apr 2026 16:23:24 +0100 Subject: [PATCH 1/6] fix: Resolve short-name collision that silently drops duplicate symbol names --- .../_internal/changelog/write_changelog_md.py | 4 +- pkg_ext/_internal/models/code_state.py | 13 +- pkg_ext/_internal/models_test.py | 125 ++++++++++++++++++ pkg_ext/_internal/pkg_state.py | 36 +++-- pkg_ext/_internal/reference_handling/added.py | 5 +- .../_internal/reference_handling/removed.py | 9 +- 6 files changed, 158 insertions(+), 34 deletions(-) diff --git a/pkg_ext/_internal/changelog/write_changelog_md.py b/pkg_ext/_internal/changelog/write_changelog_md.py index 7816f7c..33a9f40 100644 --- a/pkg_ext/_internal/changelog/write_changelog_md.py +++ b/pkg_ext/_internal/changelog/write_changelog_md.py @@ -102,8 +102,8 @@ def as_changelog_line(action: ChangelogAction, remote_url: str, ctx: pkg_ctx) -> return "" case FixAction(message=msg, changelog_message=cl_msg, short_sha=sha): return f"{cl_msg or msg} {_commit_url(remote_url, sha)}" - case MakePublicAction(name=name): - return f"New {ctx.code_state.ref_symbol(name).type} `{name}`" + case MakePublicAction(name=name, full_path=full_path): + return f"New {ctx.code_state.ref_symbol(full_path or name).type} `{name}`" case DeleteAction(name=name, group=group): return f"Removed `{group}.{name}`" case RenameAction(name=name, old_name=old_name, group=group): diff --git a/pkg_ext/_internal/models/code_state.py b/pkg_ext/_internal/models/code_state.py index 680666b..f802606 100644 --- a/pkg_ext/_internal/models/code_state.py +++ b/pkg_ext/_internal/models/code_state.py @@ -41,14 +41,17 @@ def validate_and_prepare(self): self.files.sort() return self - def ref_symbol(self, name: str) -> RefSymbol: - if ref_state := self.named_refs.get(name): - return ref_state.symbol - raise RefSymbolNotInCodeError(name) + def ref_symbol(self, name_or_local_id: str) -> RefSymbol: + if ref := self.import_id_refs.get(name_or_local_id): + return ref + for ref in self.import_id_refs.values(): + if ref.local_id == name_or_local_id or ref.name == name_or_local_id: + return ref + raise RefSymbolNotInCodeError(name_or_local_id) @property def named_refs(self) -> dict[str, RefStateWithSymbol]: - return {ref.name: RefStateWithSymbol(name=ref.name, symbol=ref) for ref in self.import_id_refs.values()} + return {ref.local_id: RefStateWithSymbol(name=ref.name, symbol=ref) for ref in self.import_id_refs.values()} def lookup(self, ref: RefSymbol) -> Any: full_reference_id = ref.full_id(self.pkg_import_name) diff --git a/pkg_ext/_internal/models_test.py b/pkg_ext/_internal/models_test.py index 1b929e5..34fa50a 100644 --- a/pkg_ext/_internal/models_test.py +++ b/pkg_ext/_internal/models_test.py @@ -1,4 +1,5 @@ from datetime import UTC, datetime +from pathlib import Path from typing import Callable import pytest @@ -6,16 +7,21 @@ from pkg_ext._internal.changelog.actions import ( GroupModuleAction, + KeepPrivateAction, MakePublicAction, changelog_filepath, dump_changelog_actions, ) from pkg_ext._internal.changelog.parser import parse_changelog +from pkg_ext._internal.errors import RefSymbolNotInCodeError from pkg_ext._internal.models import ( + PkgCodeState, PublicGroups, RefSymbol, SymbolType, ) +from pkg_ext._internal.models.py_files import PkgSrcFile +from pkg_ext._internal.pkg_state import PkgExtState from pkg_ext._internal.settings import PkgSettings @@ -151,3 +157,122 @@ def test_reconcile_logs_when_ambiguous(_public_groups, caplog): assert _public_groups.reconcile_moved_refs(refs) == 0 assert "_internal.old.Parser" in group.owned_refs # kept stale assert "Cannot resolve" in caplog.text + + +def _code_state(*refs: RefSymbol) -> PkgCodeState: + import_id_refs = {f"pkg.{r.local_id}": r for r in refs} + files = [ + PkgSrcFile( + path=Path(f"/tmp/{r.rel_path}.py"), + relative_path=r.rel_path, + pkg_import_name="pkg", + ) + for r in refs + ] + seen_paths: set[str] = set() + unique_files = [] + for f in files: + if f.relative_path not in seen_paths: + seen_paths.add(f.relative_path) + unique_files.append(f) + return PkgCodeState(pkg_import_name="pkg", import_id_refs=import_id_refs, files=unique_files) + + +def _tool_state(tmp_path: Path) -> PkgExtState: + changelog_dir = tmp_path / ".changelog" + changelog_dir.mkdir() + pkg_path = tmp_path / "pkg" + pkg_path.mkdir() + return PkgExtState(repo_root=tmp_path, changelog_dir=changelog_dir, pkg_path=pkg_path) + + +def test_named_refs_preserves_duplicate_short_names(): + """Two functions named init_cmd in different modules both appear in named_refs.""" + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + + named = code.named_refs + assert len(named) == 2 + local_ids = set(named.keys()) + assert "_internal.core.cmd_init.init_cmd" in local_ids + assert "_internal.config.cmd_config.init_cmd" in local_ids + + +def test_has_decision_distinguishes_same_name(tmp_path): + """has_decision for one init_cmd must not block the other.""" + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="init_cmd", group="core", full_path="_internal.core.cmd_init.init_cmd", author="test") + ) + assert state.has_decision("_internal.core.cmd_init.init_cmd") + assert not state.has_decision("_internal.config.cmd_config.init_cmd") + + +def test_added_refs_includes_both_same_name(tmp_path): + """Both init_cmd refs appear as added when neither has a decision.""" + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + state = _tool_state(tmp_path) + + added = state.added_refs(code.named_refs) + assert len(added) == 2 + + +def test_added_refs_filters_only_decided(tmp_path): + """After deciding one init_cmd, the other still appears as added.""" + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="init_cmd", group="core", full_path="_internal.core.cmd_init.init_cmd", author="test") + ) + + added = state.added_refs(code.named_refs) + assert len(added) == 1 + assert "_internal.config.cmd_config.init_cmd" in added + + +def test_keep_private_does_not_shadow_other_same_name(tmp_path): + """keep_private for one ref must not block the other from appearing as added.""" + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + state = _tool_state(tmp_path) + state.update_state(KeepPrivateAction(name="init_cmd", full_path="_internal.core.cmd_init.init_cmd", author="test")) + + added = state.added_refs(code.named_refs) + assert len(added) == 1 + assert "_internal.config.cmd_config.init_cmd" in added + + +def test_ref_symbol_lookup_by_local_id(): + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + + found = code.ref_symbol("_internal.core.cmd_init.init_cmd") + assert found.rel_path == "_internal/core/cmd_init" + + found2 = code.ref_symbol("_internal.config.cmd_config.init_cmd") + assert found2.rel_path == "_internal/config/cmd_config" + + # import_id key lookup (first branch in ref_symbol) + found3 = code.ref_symbol(f"pkg.{core_ref.local_id}") + assert found3.rel_path == "_internal/core/cmd_init" + + with pytest.raises(RefSymbolNotInCodeError): + code.ref_symbol("nonexistent_func") + + +def test_exposed_refs_uses_short_name_key(tmp_path): + ref_a = _ref("my_func", "_internal/mod_a") + code = _code_state(ref_a) + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="my_func", group="grp", full_path="_internal.mod_a.my_func", author="test") + ) + exposed = state.exposed_refs("grp", code.named_refs) + assert "my_func" in exposed diff --git a/pkg_ext/_internal/pkg_state.py b/pkg_ext/_internal/pkg_state.py index 64dc6dd..1eade51 100644 --- a/pkg_ext/_internal/pkg_state.py +++ b/pkg_ext/_internal/pkg_state.py @@ -39,6 +39,10 @@ class PkgExtState(Entity): default_factory=PublicGroups, description="Use with caution, inferred by changelog_dir entries.", ) + decided_local_ids: set[str] = Field( + default_factory=set, + description="local_ids that have a make_public or keep_private decision", + ) ignored_shas: set[str] = Field( default_factory=set, description="Fix commits not included in the changelog", @@ -105,12 +109,16 @@ def _handle_make_public(self, name: str, group: str, full_path: str) -> None: grp = self.groups.get_or_create_group(group) grp.owned_refs.add(full_path) grp.owned_modules.add(ref_id_module(full_path)) + self.decided_local_ids.add(full_path) def _handle_keep_private(self, name: str, full_path: str) -> None: - if state := self.refs.get(name): + key = full_path or name + if state := self.refs.get(key): state.type = RefStateType.HIDDEN else: - self.refs[name] = RefState(name=name, type=RefStateType.HIDDEN) + self.refs[key] = RefState(name=name, type=RefStateType.HIDDEN) + if full_path: + self.decided_local_ids.add(full_path) for grp in self.groups.groups: grp.owned_refs.discard(full_path) @@ -177,35 +185,23 @@ def is_group_ga(self, group: str) -> bool: def get_deprecation_replacement(self, key: str) -> str: return self.deprecation_replacements.get(key, "") - def _refs_by_short_name(self) -> dict[str, list[RefState]]: - """Group refs by short name for lookups when group is unknown.""" - from collections import defaultdict - - result: dict[str, list[RefState]] = defaultdict(list) - for state in self.refs.values(): - result[state.name].append(state) - return result - - def has_decision(self, ref_name: str) -> bool: - """Check if any decision (expose/hide) has been made for this short name.""" - return any(state.type != RefStateType.UNSET for state in self._refs_by_short_name().get(ref_name, [])) + def has_decision(self, local_id: str) -> bool: + return local_id in self.decided_local_ids def removed_refs(self, code: PkgCodeState) -> list[tuple[str, RefState]]: - """Returns list of (group, RefState) for removed refs.""" - named_refs = code.named_refs + active_names = {ref.name for ref in code.import_id_refs.values()} result: list[tuple[str, RefState]] = [] for key, state in self.refs.items(): if state.type not in {RefStateType.EXPOSED, RefStateType.DEPRECATED}: continue - if state.name in named_refs: + if state.name in active_names: continue - # Extract group from qualified_name key group = key.rsplit(".", 1)[0] if "." in key else "" result.append((group, state)) return result def added_refs(self, active_refs: dict[str, RefStateWithSymbol]) -> dict[str, RefStateWithSymbol]: - return {ref_name: ref_symbol for ref_name, ref_symbol in active_refs.items() if not self.has_decision(ref_name)} + return {local_id: ref for local_id, ref in active_refs.items() if not self.has_decision(local_id)} def add_changelog_actions(self, actions: list[ChangelogAction]) -> None: assert actions, "must add at least one action" @@ -219,7 +215,7 @@ def is_exposed(self, group: str, ref_name: str) -> bool: return False def exposed_refs(self, group: str, active_refs: dict[str, RefStateWithSymbol]) -> dict[str, RefSymbol]: - return {name: state.symbol for name, state in active_refs.items() if self.is_exposed(group, name)} + return {state.name: state.symbol for state in active_refs.values() if self.is_exposed(group, state.name)} def is_pkg_relative(self, rel_path: str) -> bool: pkg_rel_path = self.pkg_path.relative_to(self.repo_root) diff --git a/pkg_ext/_internal/reference_handling/added.py b/pkg_ext/_internal/reference_handling/added.py index 67d8e62..597e372 100644 --- a/pkg_ext/_internal/reference_handling/added.py +++ b/pkg_ext/_internal/reference_handling/added.py @@ -71,7 +71,7 @@ def _expose_function_args( for func_ref, arg_refs in args_exposed.items(): arg_refs_all.extend(arg_refs) for ref in arg_refs: - if tool_state.has_decision(ref.name): + if tool_state.has_decision(ref.local_id): continue _expose_ref( ctx, @@ -211,7 +211,8 @@ def handle_added_refs(ctx: pkg_ctx) -> None: ) all_refs_decided = relevant_refs + extra_refs_decided for ref in all_refs_decided: - added_refs.pop(ref.name, None) + local_id = ref.local_id if isinstance(ref, RefSymbol) else ref.symbol.local_id + added_refs.pop(local_id, None) task.update(advance=len(all_refs_decided)) if added_refs: remaining_str = "\n".join(str(ref) for ref in added_refs.values()) diff --git a/pkg_ext/_internal/reference_handling/removed.py b/pkg_ext/_internal/reference_handling/removed.py index a12150e..803b960 100644 --- a/pkg_ext/_internal/reference_handling/removed.py +++ b/pkg_ext/_internal/reference_handling/removed.py @@ -23,18 +23,17 @@ def process_reference_renames( ) -> set[str]: """Process renames. Returns set of old names that were renamed.""" renamed_names: set[str] = set() - used_active: set[str] = set() + used_local_ids: set[str] = set() for group, ref in renames: - rename_choices = [state for name, state in active_refs.items() if name not in used_active] + rename_choices = [state for lid, state in active_refs.items() if lid not in used_local_ids] new_ref = select_ref( f"Select new name for the reference {ref.name} with type: {ref.type}", rename_choices, ) - new_name = new_ref.name - used_active.add(new_name) + used_local_ids.add(new_ref.symbol.local_id) if confirm_create_alias(ref, new_ref): raise NotImplementedError("Alias creation is not implemented yet") - ctx.add_changelog_action(RenameAction(name=new_name, group=group, old_name=ref.name)) + ctx.add_changelog_action(RenameAction(name=new_ref.name, group=group, old_name=ref.name)) renamed_names.add(ref.name) task.update(advance=1) return renamed_names From 010a2ecea76c0bd4f6664516f6211a5dc61db068 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Sun, 12 Apr 2026 16:24:01 +0100 Subject: [PATCH 2/6] refactor: Resolve short-name collision that silently drops duplicate symbol names --- pkg_ext/_internal/models_test.py | 22 ++++++++++++++++++++++ pkg_ext/_internal/pkg_state.py | 10 +++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/pkg_ext/_internal/models_test.py b/pkg_ext/_internal/models_test.py index 34fa50a..8603b3b 100644 --- a/pkg_ext/_internal/models_test.py +++ b/pkg_ext/_internal/models_test.py @@ -276,3 +276,25 @@ def test_exposed_refs_uses_short_name_key(tmp_path): ) exposed = state.exposed_refs("grp", code.named_refs) assert "my_func" in exposed + + +def test_removed_refs_detects_specific_deletion(tmp_path): + """When core.init_cmd is deleted but config.init_cmd remains, only core is removed.""" + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="init_cmd", group="core", full_path="_internal.core.cmd_init.init_cmd", author="test") + ) + state.update_state( + MakePublicAction( + name="init_cmd", group="config", full_path="_internal.config.cmd_config.init_cmd", author="test" + ) + ) + # Only config.init_cmd remains in code + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(config_ref) + + removed = state.removed_refs(code) + assert len(removed) == 1 + group, ref_state = removed[0] + assert group == "core" + assert ref_state.name == "init_cmd" diff --git a/pkg_ext/_internal/pkg_state.py b/pkg_ext/_internal/pkg_state.py index 1eade51..ac805a6 100644 --- a/pkg_ext/_internal/pkg_state.py +++ b/pkg_ext/_internal/pkg_state.py @@ -189,14 +189,18 @@ def has_decision(self, local_id: str) -> bool: return local_id in self.decided_local_ids def removed_refs(self, code: PkgCodeState) -> list[tuple[str, RefState]]: - active_names = {ref.name for ref in code.import_id_refs.values()} + active_local_ids = {ref.local_id for ref in code.import_id_refs.values()} result: list[tuple[str, RefState]] = [] for key, state in self.refs.items(): if state.type not in {RefStateType.EXPOSED, RefStateType.DEPRECATED}: continue - if state.name in active_names: - continue group = key.rsplit(".", 1)[0] if "." in key else "" + if grp := self.groups.name_to_group.get(group): + owned_for_name = {r for r in grp.owned_refs if r.endswith(f".{state.name}")} + if owned_for_name & active_local_ids: + continue + elif state.name in {ref.name for ref in code.import_id_refs.values()}: + continue result.append((group, state)) return result From 5636abf602fdc393b44746d39f494e2e865cb4ab Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Sun, 12 Apr 2026 16:54:56 +0100 Subject: [PATCH 3/6] fix: Update decided_local_ids for moved symbols during reconciliation --- pkg_ext/_internal/cli/workflows.py | 4 +- pkg_ext/_internal/models/code_state.py | 10 ++- pkg_ext/_internal/models_test.py | 102 ++++++++++++++++++++++++- pkg_ext/_internal/pkg_state.py | 20 ++++- 4 files changed, 130 insertions(+), 6 deletions(-) diff --git a/pkg_ext/_internal/cli/workflows.py b/pkg_ext/_internal/cli/workflows.py index e92daf6..7a2a0b0 100644 --- a/pkg_ext/_internal/cli/workflows.py +++ b/pkg_ext/_internal/cli/workflows.py @@ -104,7 +104,7 @@ def create_ctx(api_input: GenerateApiInput) -> pkg_ctx: with exit_stack: code_state = parse_pkg_code_state(settings) tool_state, extra_actions = parse_changelog(settings, code_state) - tool_state.groups.reconcile_moved_refs(code_state.import_id_refs) + tool_state.reconcile_with_code(code_state.import_id_refs) git_changes_input = GitChangesInput( repo_path=settings.repo_root, @@ -223,7 +223,7 @@ def clean_old_entries(settings: PkgSettings): def create_stability_ctx(settings: PkgSettings) -> pkg_ctx: code_state = parse_pkg_code_state(settings) tool_state, extra_actions = parse_changelog(settings, code_state) - tool_state.groups.reconcile_moved_refs(code_state.import_id_refs) + tool_state.reconcile_with_code(code_state.import_id_refs) return pkg_ctx( settings=settings, diff --git a/pkg_ext/_internal/models/code_state.py b/pkg_ext/_internal/models/code_state.py index f802606..35ff08a 100644 --- a/pkg_ext/_internal/models/code_state.py +++ b/pkg_ext/_internal/models/code_state.py @@ -44,9 +44,17 @@ def validate_and_prepare(self): def ref_symbol(self, name_or_local_id: str) -> RefSymbol: if ref := self.import_id_refs.get(name_or_local_id): return ref + matches: list[RefSymbol] = [] for ref in self.import_id_refs.values(): - if ref.local_id == name_or_local_id or ref.name == name_or_local_id: + if ref.local_id == name_or_local_id: return ref + if ref.name == name_or_local_id: + matches.append(ref) + if len(matches) == 1: + return matches[0] + if matches: + local_ids = [m.local_id for m in matches] + raise RefSymbolNotInCodeError(f"{name_or_local_id} is ambiguous, matches: {local_ids}") raise RefSymbolNotInCodeError(name_or_local_id) @property diff --git a/pkg_ext/_internal/models_test.py b/pkg_ext/_internal/models_test.py index 8603b3b..d763579 100644 --- a/pkg_ext/_internal/models_test.py +++ b/pkg_ext/_internal/models_test.py @@ -21,6 +21,7 @@ SymbolType, ) from pkg_ext._internal.models.py_files import PkgSrcFile +from pkg_ext._internal.models.ref_state import RefState, RefStateType from pkg_ext._internal.pkg_state import PkgExtState from pkg_ext._internal.settings import PkgSettings @@ -267,7 +268,7 @@ def test_ref_symbol_lookup_by_local_id(): code.ref_symbol("nonexistent_func") -def test_exposed_refs_uses_short_name_key(tmp_path): +def test_exposed_refs_keys_by_local_id(tmp_path): ref_a = _ref("my_func", "_internal/mod_a") code = _code_state(ref_a) state = _tool_state(tmp_path) @@ -275,7 +276,104 @@ def test_exposed_refs_uses_short_name_key(tmp_path): MakePublicAction(name="my_func", group="grp", full_path="_internal.mod_a.my_func", author="test") ) exposed = state.exposed_refs("grp", code.named_refs) - assert "my_func" in exposed + assert "_internal.mod_a.my_func" in exposed + + +def test_exposed_refs_preserves_both_same_name(tmp_path): + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="init_cmd", group="grp", full_path="_internal.core.cmd_init.init_cmd", author="test") + ) + state.update_state( + MakePublicAction(name="init_cmd", group="grp", full_path="_internal.config.cmd_config.init_cmd", author="test") + ) + exposed = state.exposed_refs("grp", code.named_refs) + assert len(exposed) == 2 + assert "_internal.core.cmd_init.init_cmd" in exposed + assert "_internal.config.cmd_config.init_cmd" in exposed + + +def test_ref_symbol_raises_on_ambiguous_short_name(): + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + + with pytest.raises(RefSymbolNotInCodeError, match="ambiguous"): + code.ref_symbol("init_cmd") + + +def test_ref_symbol_unique_short_name_resolves(): + ref_a = _ref("my_func", "_internal/mod_a") + code = _code_state(ref_a) + found = code.ref_symbol("my_func") + assert found.rel_path == "_internal/mod_a" + + +def test_code_ref_resolves_via_owned_refs(tmp_path): + core_ref = _ref("init_cmd", "_internal/core/cmd_init") + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(core_ref, config_ref) + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="init_cmd", group="core", full_path="_internal.core.cmd_init.init_cmd", author="test") + ) + state.update_state( + MakePublicAction( + name="init_cmd", group="config", full_path="_internal.config.cmd_config.init_cmd", author="test" + ) + ) + result = state.code_ref(code, "core", "init_cmd") + assert result is not None + assert result.rel_path == "_internal/core/cmd_init" + + result2 = state.code_ref(code, "config", "init_cmd") + assert result2 is not None + assert result2.rel_path == "_internal/config/cmd_config" + + +def test_removed_refs_fallback_without_group(tmp_path): + """Legacy ref without group info: short-name fallback keeps it when any same-name ref exists.""" + state = _tool_state(tmp_path) + state.refs["init_cmd"] = RefState(name="init_cmd", type=RefStateType.EXPOSED) + config_ref = _ref("init_cmd", "_internal/config/cmd_config") + code = _code_state(config_ref) + + removed = state.removed_refs(code) + assert len(removed) == 0 + + +def test_reconcile_with_code_covers_moved_symbol(tmp_path): + """After a symbol moves, reconcile_with_code prevents re-flagging as new.""" + state = _tool_state(tmp_path) + state.update_state( + MakePublicAction(name="check_cmd", group="core", full_path="_internal.core.cmd_check.check_cmd", author="test") + ) + assert state.has_decision("_internal.core.cmd_check.check_cmd") + assert not state.has_decision("_internal.check.cmd_check.check_cmd") + + new_ref = _ref("check_cmd", "_internal/check/cmd_check") + code = _code_state(new_ref) + state.reconcile_with_code(code.import_id_refs) + assert state.has_decision("_internal.check.cmd_check.check_cmd") + + added = state.added_refs(code.named_refs) + assert len(added) == 0 + + +def test_reconcile_with_code_no_move_leaves_decisions_unchanged(tmp_path): + """When nothing moves, no new decisions are added from owned_refs.""" + state = _tool_state(tmp_path) + grp = state.groups.get_or_create_group("core") + grp.owned_refs.add("_internal.core.cmd_init.init_cmd") + + ref = _ref("init_cmd", "_internal/core/cmd_init") + code = _code_state(ref) + state.reconcile_with_code(code.import_id_refs) + + assert not state.has_decision("_internal.core.cmd_init.init_cmd") def test_removed_refs_detects_specific_deletion(tmp_path): diff --git a/pkg_ext/_internal/pkg_state.py b/pkg_ext/_internal/pkg_state.py index ac805a6..7dd82d7 100644 --- a/pkg_ext/_internal/pkg_state.py +++ b/pkg_ext/_internal/pkg_state.py @@ -72,6 +72,11 @@ def code_ref(self, code_state: PkgCodeState, group: str, name: str) -> RefSymbol key = qualified_name(group, name) if state := self.refs.get(key): if state.exist_in_code: + if grp := self.groups.name_to_group.get(group): + for owned_ref in grp.owned_refs: + if owned_ref.endswith(f".{name}"): + with suppress(RefSymbolNotInCodeError): + return code_state.ref_symbol(owned_ref) with suppress(RefSymbolNotInCodeError): return code_state.ref_symbol(name) return None @@ -185,6 +190,15 @@ def is_group_ga(self, group: str) -> bool: def get_deprecation_replacement(self, key: str) -> str: return self.deprecation_replacements.get(key, "") + def reconcile_with_code(self, import_id_refs: dict[str, RefSymbol]) -> int: + """Run reconcile_moved_refs and update decided_local_ids for moved symbols.""" + old_owned = {ref for group in self.groups.groups for ref in group.owned_refs} + count = self.groups.reconcile_moved_refs(import_id_refs) + if count: + new_owned = {ref for group in self.groups.groups for ref in group.owned_refs} + self.decided_local_ids.update(new_owned - old_owned) + return count + def has_decision(self, local_id: str) -> bool: return local_id in self.decided_local_ids @@ -200,6 +214,8 @@ def removed_refs(self, code: PkgCodeState) -> list[tuple[str, RefState]]: if owned_for_name & active_local_ids: continue elif state.name in {ref.name for ref in code.import_id_refs.values()}: + # Fallback for legacy refs without group info: any symbol with the + # same short name still in code prevents marking as removed. continue result.append((group, state)) return result @@ -219,7 +235,9 @@ def is_exposed(self, group: str, ref_name: str) -> bool: return False def exposed_refs(self, group: str, active_refs: dict[str, RefStateWithSymbol]) -> dict[str, RefSymbol]: - return {state.name: state.symbol for state in active_refs.values() if self.is_exposed(group, state.name)} + return { + state.symbol.local_id: state.symbol for state in active_refs.values() if self.is_exposed(group, state.name) + } def is_pkg_relative(self, rel_path: str) -> bool: pkg_rel_path = self.pkg_path.relative_to(self.repo_root) From dddfde49f8f350b602268db9aa1f0a0c08b33fdf Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Sun, 12 Apr 2026 16:57:40 +0100 Subject: [PATCH 4/6] fix: Reconcile owned_modules alongside owned_refs when symbols move --- pkg_ext/_internal/models/groups.py | 10 ++++++++-- pkg_ext/_internal/models_test.py | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg_ext/_internal/models/groups.py b/pkg_ext/_internal/models/groups.py index ee8dfe9..9a1b4ea 100644 --- a/pkg_ext/_internal/models/groups.py +++ b/pkg_ext/_internal/models/groups.py @@ -12,7 +12,7 @@ from pkg_ext._internal.errors import InvalidGroupSelectionError, NoPublicGroupMatch from .py_symbols import RefSymbol -from .types import PyIdentifier, SymbolRefId, ref_id_name +from .types import PyIdentifier, SymbolRefId, ref_id_module, ref_id_name if TYPE_CHECKING: # why type checking? Can we not use the root import? from pkg_ext._internal.config import ProjectConfig @@ -146,7 +146,7 @@ def merge_config(self, config: ProjectConfig) -> None: group.docstring = group_cfg.docstring def reconcile_moved_refs(self, import_id_refs: dict[str, RefSymbol]) -> int: - """Auto-fix refs that have moved to different modules.""" + """Auto-fix refs and modules that have moved to different paths.""" name_to_candidates: dict[str, list[RefSymbol]] = {} for ref in import_id_refs.values(): name_to_candidates.setdefault(ref.name, []).append(ref) @@ -176,11 +176,17 @@ def reconcile_moved_refs(self, import_id_refs: dict[str, RefSymbol]) -> int: else: updated_refs.add(ref_id) # unresolved - keep stale ref group.owned_refs = updated_refs + self._reconcile_owned_modules(group, updated_refs) if total_updated: self.write() return total_updated + @staticmethod + def _reconcile_owned_modules(group: PublicGroup, updated_refs: set[SymbolRefId]) -> None: + """Rebuild owned_modules to match the modules referenced by updated_refs.""" + group.owned_modules = {ref_id_module(ref) for ref in updated_refs} + def _resolve_ambiguous_ref( self, group: PublicGroup, stale_ref_id: SymbolRefId, candidates: list[RefSymbol] ) -> SymbolRefId | None: diff --git a/pkg_ext/_internal/models_test.py b/pkg_ext/_internal/models_test.py index d763579..128ffbd 100644 --- a/pkg_ext/_internal/models_test.py +++ b/pkg_ext/_internal/models_test.py @@ -106,6 +106,8 @@ def test_reconcile_moved_refs_updates_moved_symbol(_public_groups, caplog): assert _public_groups.reconcile_moved_refs(refs) == 1 assert "_internal.new_module.MyClass" in group.owned_refs assert "_internal.old_module.MyClass" not in group.owned_refs + assert "_internal.new_module" in group.owned_modules + assert "_internal.old_module" not in group.owned_modules assert "Symbol moved:" in caplog.text From d3488cf8c1255940c1412afa0d4eed3d4505c3f7 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Sun, 12 Apr 2026 17:11:33 +0100 Subject: [PATCH 5/6] chore: add changelog entries --- .changelog/029.yaml | 28 ++++++++++++++++++++++++++++ .groups.yaml | 1 - 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 .changelog/029.yaml diff --git a/.changelog/029.yaml b/.changelog/029.yaml new file mode 100644 index 0000000..b341126 --- /dev/null +++ b/.changelog/029.yaml @@ -0,0 +1,28 @@ +name: main +ts: 2026-04-12 16:10:19.714611+00:00 +type: keep_private +full_path: _internal.cli.main +--- +name: workflows +ts: 2026-04-12 16:10:30.816480+00:00 +type: fix +author: Espen Albert +changelog_message: 'fix: Resolve short-name collision that silently drops duplicate symbol names' +message: 'fix: Resolve short-name collision that silently drops duplicate symbol names' +short_sha: 8d9632 +--- +name: workflows +ts: 2026-04-12 16:10:51.908835+00:00 +type: fix +author: Espen Albert +changelog_message: 'fix: Update decided_local_ids for moved symbols during reconciliation' +message: 'fix: Update decided_local_ids for moved symbols during reconciliation' +short_sha: 5636ab +--- +name: workflows +ts: 2026-04-12 16:11:02.339052+00:00 +type: fix +author: Espen Albert +changelog_message: 'fix: Reconcile owned_modules alongside owned_refs when symbols move' +message: 'fix: Reconcile owned_modules alongside owned_refs when symbols move' +short_sha: dddfde diff --git a/.groups.yaml b/.groups.yaml index 4c3645d..86b55a7 100644 --- a/.groups.yaml +++ b/.groups.yaml @@ -47,7 +47,6 @@ groups: - _internal.cli.gen_cmds owned_refs: - _internal.cli.gen_cmds.gen_docs - - _internal.cli.gen_cmds.gen_tests - name: stability owned_modules: - _internal.cli.stability_cmds From d93196a453862f4186f2274284253cebac0d2a83 Mon Sep 17 00:00:00 2001 From: Espen Albert Date: Sun, 12 Apr 2026 17:29:12 +0100 Subject: [PATCH 6/6] refactor(reconcile): Prevent spurious re-detection of moved symbols when groups.yaml already correct --- pkg_ext/_internal/models/groups.py | 12 ++++++-- pkg_ext/_internal/models_test.py | 48 +++++++++++++++++++++++++----- pkg_ext/_internal/pkg_state.py | 19 ++++++++---- 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/pkg_ext/_internal/models/groups.py b/pkg_ext/_internal/models/groups.py index 9a1b4ea..ff0edc7 100644 --- a/pkg_ext/_internal/models/groups.py +++ b/pkg_ext/_internal/models/groups.py @@ -145,13 +145,17 @@ def merge_config(self, config: ProjectConfig) -> None: group.docs_exclude = group_cfg.docs_exclude.copy() group.docstring = group_cfg.docstring - def reconcile_moved_refs(self, import_id_refs: dict[str, RefSymbol]) -> int: - """Auto-fix refs and modules that have moved to different paths.""" + def reconcile_moved_refs(self, import_id_refs: dict[str, RefSymbol]) -> tuple[int, set[SymbolRefId]]: + """Auto-fix refs and modules that have moved to different paths. + + Returns (count_of_updates, set_of_moved_to_ref_ids). + """ name_to_candidates: dict[str, list[RefSymbol]] = {} for ref in import_id_refs.values(): name_to_candidates.setdefault(ref.name, []).append(ref) total_updated = 0 + moved_to: set[SymbolRefId] = set() for group in self.groups: updated_refs: set[SymbolRefId] = set() for ref_id in list(group.owned_refs): @@ -165,12 +169,14 @@ def reconcile_moved_refs(self, import_id_refs: dict[str, RefSymbol]) -> int: if current_id != ref_id: logger.warning(f"Symbol moved: {ref_id} -> {current_id} (group: {group.name})") updated_refs.add(current_id) + moved_to.add(current_id) total_updated += 1 else: updated_refs.add(ref_id) elif resolved := self._resolve_ambiguous_ref(group, ref_id, candidates): if resolved != ref_id: logger.warning(f"Symbol moved: {ref_id} -> {resolved} (group: {group.name})") + moved_to.add(resolved) total_updated += 1 updated_refs.add(resolved) else: @@ -180,7 +186,7 @@ def reconcile_moved_refs(self, import_id_refs: dict[str, RefSymbol]) -> int: if total_updated: self.write() - return total_updated + return total_updated, moved_to @staticmethod def _reconcile_owned_modules(group: PublicGroup, updated_refs: set[SymbolRefId]) -> None: diff --git a/pkg_ext/_internal/models_test.py b/pkg_ext/_internal/models_test.py index 128ffbd..6f762f0 100644 --- a/pkg_ext/_internal/models_test.py +++ b/pkg_ext/_internal/models_test.py @@ -93,7 +93,9 @@ def test_reconcile_moved_refs_no_changes(_public_groups): _public_groups.get_or_create_group("my_group").owned_refs.add("_internal.models.MyClass") refs = _refs_dict(_ref("MyClass", "_internal/models")) - assert _public_groups.reconcile_moved_refs(refs) == 0 + count, moved_to = _public_groups.reconcile_moved_refs(refs) + assert count == 0 + assert not moved_to assert "_internal.models.MyClass" in _public_groups.name_to_group["my_group"].owned_refs @@ -103,7 +105,9 @@ def test_reconcile_moved_refs_updates_moved_symbol(_public_groups, caplog): group.owned_modules.add("_internal.old_module") refs = _refs_dict(_ref("MyClass", "_internal/new_module")) - assert _public_groups.reconcile_moved_refs(refs) == 1 + count, moved_to = _public_groups.reconcile_moved_refs(refs) + assert count == 1 + assert "_internal.new_module.MyClass" in moved_to assert "_internal.new_module.MyClass" in group.owned_refs assert "_internal.old_module.MyClass" not in group.owned_refs assert "_internal.new_module" in group.owned_modules @@ -115,7 +119,9 @@ def test_reconcile_moved_refs_keeps_deleted_symbol(_public_groups): group = _public_groups.get_or_create_group("my_group") group.owned_refs.add("_internal.models.DeletedClass") - assert _public_groups.reconcile_moved_refs({}) == 0 + count, moved_to = _public_groups.reconcile_moved_refs({}) + assert count == 0 + assert not moved_to assert "_internal.models.DeletedClass" in group.owned_refs @@ -126,7 +132,9 @@ def test_reconcile_moved_refs_multiple_groups(_public_groups, caplog): group2.owned_refs.add("_internal.old.Func2") refs = _refs_dict(_ref("Func1", "_internal/new"), _ref("Func2", "_internal/new")) - assert _public_groups.reconcile_moved_refs(refs) == 2 + count, moved_to = _public_groups.reconcile_moved_refs(refs) + assert count == 2 + assert moved_to == {"_internal.new.Func1", "_internal.new.Func2"} assert "_internal.new.Func1" in group1.owned_refs assert "_internal.new.Func2" in group2.owned_refs @@ -137,7 +145,9 @@ def test_reconcile_disambiguates_via_owned_modules(_public_groups): group.owned_modules.add("_internal.models") refs = _refs_dict(_ref("Parser", "_internal/models"), _ref("Parser", "_internal/utils")) - assert _public_groups.reconcile_moved_refs(refs) == 1 + count, moved_to = _public_groups.reconcile_moved_refs(refs) + assert count == 1 + assert "_internal.models.Parser" in moved_to assert "_internal.models.Parser" in group.owned_refs @@ -148,7 +158,9 @@ def test_reconcile_disambiguates_via_other_groups(_public_groups): group2.owned_refs.add("_internal.utils.Parser") # already owned by group2 refs = _refs_dict(_ref("Parser", "_internal/models"), _ref("Parser", "_internal/utils")) - assert _public_groups.reconcile_moved_refs(refs) == 1 + count, moved_to = _public_groups.reconcile_moved_refs(refs) + assert count == 1 + assert "_internal.models.Parser" in moved_to assert "_internal.models.Parser" in group1.owned_refs @@ -157,7 +169,9 @@ def test_reconcile_logs_when_ambiguous(_public_groups, caplog): group.owned_refs.add("_internal.old.Parser") refs = _refs_dict(_ref("Parser", "_internal/a"), _ref("Parser", "_internal/b")) - assert _public_groups.reconcile_moved_refs(refs) == 0 + count, moved_to = _public_groups.reconcile_moved_refs(refs) + assert count == 0 + assert not moved_to assert "_internal.old.Parser" in group.owned_refs # kept stale assert "Cannot resolve" in caplog.text @@ -365,6 +379,26 @@ def test_reconcile_with_code_covers_moved_symbol(tmp_path): assert len(added) == 0 +def test_reconcile_with_code_groups_yaml_already_has_correct_ref(tmp_path): + """When .groups.yaml already has the correct path, the moved-to ref still enters decided_local_ids.""" + state = _tool_state(tmp_path) + # Changelog replays with stale full_path + state.update_state( + MakePublicAction(name="check_cmd", group="core", full_path="_internal.core.cmd_check.check_cmd", author="test") + ) + # Simulate .groups.yaml already containing the correct ref (e.g., manually fixed or from a prior run) + grp = state.groups.name_to_group["core"] + grp.owned_refs.discard("_internal.core.cmd_check.check_cmd") + grp.owned_refs.add("_internal.check.cmd_check.check_cmd") + + new_ref = _ref("check_cmd", "_internal/check/cmd_check") + code = _code_state(new_ref) + state.reconcile_with_code(code.import_id_refs) + + assert state.has_decision("_internal.check.cmd_check.check_cmd") + assert len(state.added_refs(code.named_refs)) == 0 + + def test_reconcile_with_code_no_move_leaves_decisions_unchanged(tmp_path): """When nothing moves, no new decisions are added from owned_refs.""" state = _tool_state(tmp_path) diff --git a/pkg_ext/_internal/pkg_state.py b/pkg_ext/_internal/pkg_state.py index 7dd82d7..fddf266 100644 --- a/pkg_ext/_internal/pkg_state.py +++ b/pkg_ext/_internal/pkg_state.py @@ -24,7 +24,7 @@ from pkg_ext._internal.models.groups import PublicGroups from pkg_ext._internal.models.py_symbols import RefSymbol from pkg_ext._internal.models.ref_state import RefState, RefStateType, RefStateWithSymbol -from pkg_ext._internal.models.types import qualified_name, ref_id_module +from pkg_ext._internal.models.types import qualified_name, ref_id_module, ref_id_name class PkgExtState(Entity): @@ -192,11 +192,18 @@ def get_deprecation_replacement(self, key: str) -> str: def reconcile_with_code(self, import_id_refs: dict[str, RefSymbol]) -> int: """Run reconcile_moved_refs and update decided_local_ids for moved symbols.""" - old_owned = {ref for group in self.groups.groups for ref in group.owned_refs} - count = self.groups.reconcile_moved_refs(import_id_refs) - if count: - new_owned = {ref for group in self.groups.groups for ref in group.owned_refs} - self.decided_local_ids.update(new_owned - old_owned) + count, moved_to = self.groups.reconcile_moved_refs(import_id_refs) + if moved_to: + self.decided_local_ids.update(moved_to) + all_owned = {ref for group in self.groups.groups for ref in group.owned_refs} + stale_ids = self.decided_local_ids - all_owned + if stale_ids: + owned_by_name: dict[str, set[str]] = {} + for ref_id in all_owned: + owned_by_name.setdefault(ref_id_name(ref_id), set()).add(ref_id) + for stale_id in stale_ids: + if current := owned_by_name.get(ref_id_name(stale_id)): + self.decided_local_ids.update(current) return count def has_decision(self, local_id: str) -> bool: