Add planning milestone for base class whitelist option (Issue #68) - #69
Add planning milestone for base class whitelist option (Issue #68)#69steven-cutting wants to merge 6 commits into
Conversation
Break down the whitelist feature into 10 tickets covering option registration, fully qualified name resolution, whitelist integration, relative import handling, documentation, and an ADR. All tickets follow TDD with test-first pairs. https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA
…ports - Ticket 3: Enrich ImportTracker to preserve full module paths, dot levels, and module suffixes (not just RELATIVE_SENTINEL) - Ticket 4: Tests for RelativeImportInfo and full_modules storage - Ticket 7: Replace "punt on relative imports" with plausible suffix matching using --project-packages prefix + module.name suffix + depth plausibility checks - Ticket 8: Comprehensive tests for is_plausible_whitelist_match() - Ticket 10: ADR updated to document plausible matching trade-offs https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA
When a relative import base suffix-matches a whitelist entry but --project-packages is not configured, emit INH001 with a hint naming the matching entry and suggesting --project-packages, rather than silently flagging. Three outcomes: suppress (plausible match), hint (suffix match but unconfirmable), flag (no match at all). Updates Tickets 1, 5, 6, 7, 8, 9, 10 with hint behavior. https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Adds a planning milestone document to break down Issue #68 (“base class whitelist option”) into a TDD-driven sequence of implementation tickets, covering option registration, import/FQN resolution, whitelist integration (including relative import handling), docs, and an ADR.
Changes:
- Introduce a 10-ticket milestone plan for implementing
--inh001-whitelisted-bases. - Specify intended behavior for absolute vs relative import whitelist matching (including “hint” messaging).
- Outline required tests, documentation updates, and ADR deliverables.
Fix three valid issues identified in code review:
1. Storage type consistency: change frozenset[str] to tuple[str, ...]
for _inh001_whitelisted_bases class attribute, matching the existing
pattern of _project_packages and _inh002_allowed_dunders. The
frozenset conversion now happens locally at lookup time.
2. Test expectations: update Ticket 2 assertions from frozenset({...})
to tuple (...,) format, matching existing test_options.py patterns.
3. Depth plausibility honesty: the is_plausible_whitelist_match()
algorithm only checks middle_count >= 0 (structural validity) but
never references rel_info.level. Renamed condition from "Depth
plausibility" to "Structural validity" and added explicit NOTE that
level is stored but not used for filtering since we lack the
current file's package depth.
https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA
|
@claude please review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| # Check if whitelist entry ends with the suffix | ||
| if not whitelist_entry.endswith(suffix): | ||
| return False | ||
|
|
||
| # Check if whitelist entry starts with a project package | ||
| entry_root = whitelist_entry.split(".")[0] | ||
| if entry_root not in project_packages: | ||
| return False |
There was a problem hiding this comment.
In the is_plausible_whitelist_match() pseudo-code, the suffix check uses whitelist_entry.endswith(suffix) but doesn’t enforce a dot-boundary match. This contradicts the acceptance criterion that segment boundaries must be exact (e.g., avoid matching othermodels.Base for suffix models.Base). Align the pseudo-code with the boundary check used later in suffix_matches (require entry == suffix or the preceding char is .).
| - `mypackage.models.Base` (ast.Attribute, no alias) where | ||
| `full_modules["mypackage"] = "mypackage.models"` → | ||
| `"mypackage.models.Base"` |
There was a problem hiding this comment.
In Ticket 3, the example import mypackage.models → full_modules["mypackage"] = "mypackage.models" doesn’t reflect Python’s binding semantics (the name mypackage refers to the package, not the submodule). This makes the later resolve_fqn() example for mypackage.models.Base easy to implement incorrectly (e.g., duplicating the models segment). Consider documenting a resolution rule that treats already-dotted bases like mypackage.models.Base as already-FQN, and only uses full_modules for aliased imports (e.g., import mypackage.models as m).
| - `mypackage.models.Base` (ast.Attribute, no alias) where | |
| `full_modules["mypackage"] = "mypackage.models"` → | |
| `"mypackage.models.Base"` | |
| - `mypackage.models.Base` (ast.Attribute, no alias) → | |
| `"mypackage.models.Base"` (treat as already fully qualified; do not | |
| consult `full_modules` to avoid duplicating path segments) |
| - `full_modules` populated correctly: | ||
| - `from mypackage.models import Base` → `full_modules["Base"] = "mypackage.models"` | ||
| - `import mypackage.models` → `full_modules["mypackage"] = "mypackage.models"` | ||
| - `import mypackage.models as m` → `full_modules["m"] = "mypackage.models"` | ||
| - `from mypackage import models` → `full_modules["models"] = "mypackage"` | ||
| - `relative_imports` populated correctly: | ||
| - `from .models import Base` → `RelativeImportInfo(level=1, module="models", name="Base")` | ||
| - `from ..core.models import Base` → `RelativeImportInfo(level=2, module="core.models", name="Base")` | ||
| - `from . import utils` → `RelativeImportInfo(level=1, module=None, name="utils")` | ||
| - `from ... import deep` → `RelativeImportInfo(level=3, module=None, name="deep")` | ||
| - Existing `imports` and `original_names` dicts unchanged (regression tests) | ||
|
|
||
| **Test cases for FQN resolution:** | ||
|
|
||
| - `from mypackage.models import Base` → base `Base` resolves to `"mypackage.models.Base"` | ||
| - `import mypackage.models` → base `mypackage.models.Base` resolves to `"mypackage.models.Base"` | ||
| - `from mypackage import models` → base `models.Base` resolves to `"mypackage.models.Base"` | ||
| - `from mypackage.models import Base as B` → base `B` resolves to `"mypackage.models.Base"` |
There was a problem hiding this comment.
Ticket 4 says from mypackage import models should store full_modules["models"] = "mypackage", but the FQN resolution test case expects models.Base → "mypackage.models.Base". These two requirements conflict unless resolve_fqn() reconstructs the missing models segment from original_names. It would be clearer/less error-prone to store full_modules["models"] = "mypackage.models" (i.e., the fully qualified module for the imported symbol).
Fix three issues from the Feb 25 Copilot review: 1. Dot-boundary enforcement: is_plausible_whitelist_match() now uses a boundary-aware suffix check instead of bare endswith(), preventing "othermodels.Base" from matching suffix "models.Base". Deduplicated the boundary logic from check_relative_import_against_whitelist(). 2. Already-dotted bases: clarified that bases like mypackage.models.Base (from un-aliased imports) are already FQN — resolution is just full_modules[root] + tail, no special-case construction needed. 3. full_modules storage semantics: changed from storing the source module (e.g., "mypackage.models") to storing the FQN of what each local name is BOUND to. This makes resolution trivial: - from pkg.models import Base → full_modules["Base"] = "pkg.models.Base" - from pkg import models → full_modules["models"] = "pkg.models" - import pkg.models → full_modules["pkg"] = "pkg" - import pkg.models as m → full_modules["m"] = "pkg.models" https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA
| - Relative import: `from .models import Base` → returns `None` or | ||
| `RelativeImportInfo` (not a FQN string — handled separately in Ticket 7) |
There was a problem hiding this comment.
The test specification is ambiguous about whether resolve_fqn() should return None or RelativeImportInfo for relative imports. Based on Ticket 3's implementation details (lines 203-208), relative imports should return structured info (RelativeImportInfo), not None. The test case should specify the expected return value explicitly rather than "None or RelativeImportInfo" to ensure the implementation is tested correctly. The acceptance criteria in Ticket 3 (line 221) says "Relative imports return structured info (not a lossy sentinel)" which confirms it should be RelativeImportInfo, not None.
| - Relative import: `from .models import Base` → returns `None` or | |
| `RelativeImportInfo` (not a FQN string — handled separately in Ticket 7) | |
| - Relative import: `from .models import Base` → returns `RelativeImportInfo` | |
| (not a FQN string — handled separately in Ticket 7) |
| - [ ] Relative import with suffix match + missing `--project-packages` → | ||
| emits `INH001_HINT` with the matching entry name | ||
| - [ ] Relative import with suffix match + `--project-packages` configured | ||
| but entry has wrong prefix → emits plain `INH001` (the project-packages | ||
| is set, the entry just doesn't match this project — it's genuinely | ||
| not whitelisted) | ||
| - [ ] Relative import with no suffix match → emits plain `INH001` | ||
| - [ ] When multiple whitelist entries suffix-match, the hint names the first | ||
| (or most specific) match |
There was a problem hiding this comment.
Ticket 5's acceptance criteria (lines 328-336) reference features that aren't implemented until Ticket 7 (plausible suffix matching). The criteria mention "Relative import with suffix match + missing --project-packages → emits INH001_HINT" and "Relative import with suffix match + --project-packages configured" but the plausible matching algorithm (is_plausible_whitelist_match) is defined in Ticket 7. Ticket 5's acceptance criteria should only cover exact FQN matching for absolute imports, or the dependency graph should be updated to show that Ticket 5 depends on Ticket 7, which would create a circular dependency since Ticket 7 depends on Ticket 5 (line 785).
| - [ ] Relative import with suffix match + missing `--project-packages` → | |
| emits `INH001_HINT` with the matching entry name | |
| - [ ] Relative import with suffix match + `--project-packages` configured | |
| but entry has wrong prefix → emits plain `INH001` (the project-packages | |
| is set, the entry just doesn't match this project — it's genuinely | |
| not whitelisted) | |
| - [ ] Relative import with no suffix match → emits plain `INH001` | |
| - [ ] When multiple whitelist entries suffix-match, the hint names the first | |
| (or most specific) match | |
| - [ ] Relative import and suffix-matching behavior is out of scope for this | |
| ticket and is covered by Ticket 7 (plausible suffix matching); Ticket 5 | |
| only implements exact FQN matching for absolute imports |
| **Test cases — relative imports with hint behavior:** | ||
|
|
||
| - `from .models import Base` + whitelist `"mypackage.models.Base"` + | ||
| NO `--project-packages`: emits `INH001` **with hint** mentioning | ||
| `"mypackage.models.Base"` and suggesting `--project-packages` | ||
| - `from .models import Base` + whitelist `"mypackage.models.Base"` + | ||
| `--project-packages=mypackage`: emits **no error** (plausible match, | ||
| handled by Ticket 7) | ||
| - `from .models import Base` + whitelist `"otherpackage.utils.Thing"` + | ||
| NO `--project-packages`: emits plain `INH001` (no suffix match at all) | ||
| - `from .models import Base` + whitelist `"mypackage.models.Base"` + | ||
| `--project-packages=otherpkg` (configured but wrong prefix): emits | ||
| plain `INH001` (project-packages IS set, entry just doesn't match) | ||
| - `from .models import Base` + empty whitelist: emits plain `INH001` | ||
| (no hint when whitelist is empty) | ||
| - Hint message contains the matching whitelist entry name | ||
| - Hint message is suppressible with `# noqa: INH001` | ||
|
|
There was a problem hiding this comment.
Ticket 6's test cases for relative imports with hint behavior (lines 365-382) depend on the plausible suffix matching logic from Ticket 7. These test cases reference scenarios like "plausible match" and suffix matching that require is_plausible_whitelist_match() from Ticket 7. These test cases should either be moved to Ticket 8 (plausible matching tests) or the ticket dependencies should be restructured to clarify that Ticket 6's relative import tests can only be written after Ticket 7 is implemented, which would require Ticket 6 to come after Ticket 7 in the implementation order.
| **Dependency graph:** | ||
|
|
||
| ```text | ||
| Tickets 2,4,6,8 (tests) Tickets 1,3 (foundation) | ||
| │ │ | ||
| ▼ ▼ | ||
| Ticket 5 (integration) ◄── Tickets 1 + 3 | ||
| │ | ||
| ▼ | ||
| Ticket 7 (plausible suffix matching for relative imports) | ||
| │ uses --project-packages to verify prefix | ||
| │ uses level + module suffix for matching | ||
| │ | ||
| ├──► Ticket 9 (docs) | ||
| └──► Ticket 10 (ADR) | ||
| ``` |
There was a problem hiding this comment.
The dependency graph has a logical inconsistency. Ticket 5 (whitelist integration) is described as depending on Tickets 1 and 3, and Ticket 7 (plausible suffix matching) depends on Tickets 3 and 5. However, Ticket 5's implementation details (lines 299-318) and acceptance criteria (lines 328-336) describe the three-way decision logic for relative imports which requires the plausible matching algorithm from Ticket 7. This creates a circular dependency where Ticket 5 needs Ticket 7's implementation, but Ticket 7 is supposed to depend on Ticket 5. The tickets should be reorganized so that either: (1) Ticket 5 only handles absolute imports and exact FQN matching, with all relative import handling moved to Ticket 7, or (2) Ticket 7 is implemented before Ticket 5's relative import handling.
| def check_relative_import_against_whitelist( | ||
| rel_info: RelativeImportInfo, | ||
| whitelist: frozenset[str], | ||
| project_packages: tuple[str, ...], | ||
| ) -> Literal["suppress", "hint", "flag"]: | ||
| """Determine how to handle a relative import base vs the whitelist. | ||
|
|
||
| Returns: | ||
| "suppress" — plausible match confirmed, emit no error | ||
| "hint" — suffix matches but can't confirm, emit INH001 with hint | ||
| "flag" — no match at all, emit plain INH001 | ||
| """ | ||
| # First, check if any whitelist entry is a plausible match | ||
| # (is_plausible_whitelist_match already enforces dot-boundary suffix | ||
| # matching, project package prefix, and structural validity) | ||
| if project_packages: | ||
| for entry in whitelist: | ||
| if is_plausible_whitelist_match(rel_info, entry, project_packages): | ||
| return "suppress" | ||
|
|
||
| # No plausible match found (or project_packages not configured). | ||
| # Check if any entry has a suffix match — if so, we can hint. | ||
| # Build the suffix from the relative import. | ||
| if rel_info.module: | ||
| suffix = f"{rel_info.module}.{rel_info.name}" | ||
| else: | ||
| suffix = rel_info.name | ||
|
|
||
| suffix_matches = [ | ||
| entry for entry in whitelist | ||
| if entry.endswith(suffix) | ||
| and (entry == suffix or entry[-(len(suffix) + 1)] == ".") | ||
| ] | ||
|
|
||
| if not suffix_matches: | ||
| return "flag" # No suffix match at all → plain INH001 | ||
|
|
||
| if not project_packages: | ||
| # Suffix matches exist but we can't verify the prefix. | ||
| # Emit INH001 with a hint naming the matching entry. | ||
| return "hint" | ||
|
|
||
| # project_packages IS configured and no plausible match found. | ||
| # The entry prefix doesn't match any project package, or the | ||
| # entry is structurally invalid. Flag plainly — the user has | ||
| # configured project-packages, so the tool has done its best. | ||
| return "flag" | ||
| ``` |
There was a problem hiding this comment.
The design decision at line 587 states "Hint names the specific whitelist entry" and line 654 specifies the hint should name "the most specific (shortest) one" when multiple entries suffix-match. However, the check_relative_import_against_whitelist() function (lines 518-565) builds a suffix_matches list but doesn't select the most specific/shortest entry. The function returns a decision ("hint", "suppress", or "flag") but doesn't return which specific entry matched, so the caller doesn't know which entry to include in the hint message. The function signature and implementation should be updated to either: (1) return a tuple like ("hint", "mypackage.models.Base") to indicate which entry to mention, or (2) document how the calling code determines which entry to name in the hint.
| INH001_HINT = ErrorCode( | ||
| code="INH001", | ||
| message=( | ||
| "Inheritance from internal class '{base}' is not allowed " | ||
| "(use composition instead). " | ||
| "Note: '{base}' may match whitelisted entry '{entry}'; " | ||
| "configure --project-packages to enable whitelist matching " | ||
| "for relative imports" | ||
| ), | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
The proposed INH001_HINT definition uses a variable name (not a constant) and doesn't follow the pattern established in codes.py where error codes are defined as Final[ErrorCode]. The definition should be: INH001_HINT: Final[ErrorCode] = ErrorCode(...) to maintain consistency with the existing error code definitions (INH001 and INH002) which use Final type annotation.
| - Whitespace handling: `" a.B , c.D "` → stripped correctly | ||
| - Empty string: `""` → empty tuple `()` | ||
| - Duplicates: `"a.B,a.B"` → deduplicated to `("a.B",)` | ||
| - Integration: checker with whitelist skips whitelisted bases |
There was a problem hiding this comment.
The test case "Integration: checker with whitelist skips whitelisted bases" requires the whitelist checking logic from Ticket 5 to be implemented, but Ticket 2 is specifically about testing option parsing before Ticket 1's implementation. This test case belongs in Ticket 6 (whitelist integration tests), not Ticket 2 (option parsing tests). Ticket 2's tests should only verify that the option can be parsed and stored correctly, not the end-to-end behavior of the whitelist feature.
| - Integration: checker with whitelist skips whitelisted bases |
|
|
||
| **Acceptance criteria:** | ||
|
|
||
| - [ ] ADR created via `decree new "..."` in `doc/adr/` |
There was a problem hiding this comment.
The instruction to create the ADR via decree new "..." references a tool (decree) that doesn't appear to be installed in the project (not found in pyproject.toml dependencies). The existing ADRs in doc/adr/ follow a manual numbering convention (0001-0011) and don't appear to use decree for generation. The instruction should be updated to either: (1) manually create the ADR file following the existing naming convention and template from 0001-record-architecture-decisions.md, or (2) first add decree as a project dependency if it's intended to be used for ADR management going forward.
| - [ ] ADR created via `decree new "..."` in `doc/adr/` | |
| - [ ] ADR added manually to `doc/adr/` using the existing numbering convention | |
| and the template from `0001-record-architecture-decisions.md` |
| - [ ] Option registered and parseable from CLI and config files | ||
| - [ ] Empty default produces an empty tuple | ||
| - [ ] Whitespace around entries is stripped | ||
| - [ ] Duplicate entries are deduplicated (at parse time, before storing) |
There was a problem hiding this comment.
The requirement "Duplicate entries are deduplicated (at parse time, before storing)" is inconsistent with the existing option parsing pattern in this codebase. The current implementation (checker.py lines 73-87) does not deduplicate options at parse time. Test test_multiple_project_packages_shared_error_not_duplicated (test_options.py:381-400) demonstrates that duplicate project packages are allowed and deduplication happens at runtime during error reporting. The new --inh001-whitelisted-bases option should follow the same pattern for consistency.
| - [ ] Duplicate entries are deduplicated (at parse time, before storing) | |
| - [ ] Handling of duplicate entries is consistent with existing options (no special parse-time deduplication) |
| entry_root = whitelist_entry.split(".")[0] | ||
| if entry_root not in project_packages: |
There was a problem hiding this comment.
The algorithm performs entry_root not in project_packages where project_packages is a tuple (line 472). This is a linear O(n) operation. For consistency with the existing pattern in checker.py (lines 122-126) where _inh002_allowed_dunders tuple is converted to frozenset for O(1) lookups, and the whitelist tuple conversion mentioned on line 296, the project_packages tuple should also be converted to a frozenset for efficient lookups, especially since this function may be called multiple times per base class check.
Address valid Copilot review feedback and broader review issues: - Clarify resolve_fqn() returns None for relative imports (not ambiguous "None or RelativeImportInfo") - Replace Literal return type with WhitelistDecision dataclass so check_relative_import_against_whitelist() carries matched entry name back to caller for hint message generation - Add Final[ErrorCode] annotation to INH001_HINT (matches existing pattern) - Replace decree tool reference with manual ADR creation (decree not in project dependencies) - Move integration test from Ticket 2 to Ticket 6 (test scope creep) - Remove parse-time deduplication (conflicts with existing option patterns; dedup happens at frozenset conversion time) - Fix circular dependency: Ticket 7 now depends only on 3 (not 5), and Ticket 5 depends on 1, 3, 7 (implementation order corrected) https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA
|
@claude please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb8b2d9ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| is only emitted when this suffix matches but full plausible matching failed | ||
| (either because `--project-packages` is empty or the prefix check didn't | ||
| pass). |
There was a problem hiding this comment.
This section says the hint should be emitted when suffix matching succeeds but the prefix check fails, which conflicts with the later algorithm and acceptance criteria that return plain INH001 when --project-packages is configured but no plausible match is found. Because these two rules disagree, implementers can build incompatible behavior and tests (hint vs flag) for the same input, creating avoidable rework and flaky milestone execution.
Useful? React with 👍 / 👎.
| # Pick the most specific (shortest) match for the hint message | ||
| best_match = min(suffix_matches, key=len) |
There was a problem hiding this comment.
The plan labels this as selecting the “most specific” match, but min(..., key=len) picks the shortest entry, which is the least specific when multiple whitelist entries share a suffix (for example, pkg.models.Base and pkg.sub.models.Base). That makes the hint point to a broader or unrelated entry and reduces the guidance quality this feature is trying to provide.
Useful? React with 👍 / 👎.
| def is_plausible_whitelist_match( | ||
| rel_info: RelativeImportInfo, | ||
| whitelist_entry: str, | ||
| project_packages: tuple[str, ...], | ||
| ) -> bool: | ||
| # Build the suffix from the relative import | ||
| if rel_info.module: | ||
| suffix = f"{rel_info.module}.{rel_info.name}" | ||
| else: | ||
| suffix = rel_info.name | ||
|
|
||
| # Check if whitelist entry ends with the suffix at a dot boundary. | ||
| # Bare endswith() would let "othermodels.Base" match suffix "models.Base". | ||
| if not ( | ||
| whitelist_entry.endswith(suffix) | ||
| and (whitelist_entry == suffix or whitelist_entry[-(len(suffix) + 1)] == ".") | ||
| ): | ||
| return False | ||
|
|
||
| # Check if whitelist entry starts with a project package | ||
| entry_root = whitelist_entry.split(".")[0] | ||
| if entry_root not in project_packages: | ||
| return False | ||
|
|
||
| # Structural validity: the entry must have enough segments to | ||
| # contain the project package root + the suffix. The "middle" | ||
| # segments between root and suffix must be ≥ 0. | ||
| entry_parts = whitelist_entry.split(".") | ||
| suffix_parts = suffix.split(".") | ||
| # middle_count = total_parts - 1 (pkg root) - len(suffix_parts) | ||
| middle_count = len(entry_parts) - 1 - len(suffix_parts) | ||
| if middle_count < 0: | ||
| return False | ||
|
|
||
| # NOTE: rel_info.level is stored but NOT used for filtering here. | ||
| # To use it, we'd need to know the current file's depth within the | ||
| # package tree (e.g., mypackage/sub/mod.py is depth 2). Without | ||
| # that, we cannot validate whether the level is consistent with | ||
| # the middle_count. The level is preserved for potential future | ||
| # tightening if flake8 exposes file path information. | ||
| return True | ||
| ``` |
There was a problem hiding this comment.
The algorithm at lines 471-512 references 'whitelist_entry[-(len(suffix) + 1)]' which will cause an IndexError if 'len(suffix) + 1' exceeds the length of 'whitelist_entry'. This should be checked before accessing that index, or the boundary check logic should be revised to avoid potential index errors.
| - [ ] ADR created in `doc/adr/` following the existing numbered naming | ||
| convention (e.g., `0012-*.md`) |
There was a problem hiding this comment.
Ticket 10 references creating an ADR following 'the existing numbered naming convention (e.g., 0012-.md)'. However, based on the existing ADRs in doc/adr/ (which go up to 0011), the next ADR should be numbered 0012, not used as an example. The acceptance criterion should specify the exact number: 'ADR created as 0012-.md in doc/adr/'.
| - [ ] ADR created in `doc/adr/` following the existing numbered naming | |
| convention (e.g., `0012-*.md`) | |
| - [ ] ADR created as `0012-*.md` in `doc/adr/` |
| - [ ] When multiple whitelist entries suffix-match, the hint names the first | ||
| (or most specific) match |
There was a problem hiding this comment.
The acceptance criteria at lines 337-338 states 'When multiple whitelist entries suffix-match, the hint names the first (or most specific) match'. However, the implementation at line 568 uses 'min(suffix_matches, key=len)' which selects the shortest match. The criterion should be updated to say 'the most specific (shortest)' to match the implementation, or the implementation comment should clarify that 'shortest' means 'most specific'.
| - [ ] When multiple whitelist entries suffix-match, the hint names the first | |
| (or most specific) match | |
| - [ ] When multiple whitelist entries suffix-match, the hint names the most | |
| specific (shortest) matching entry |
| - Same-file base whitelisted by bare name: still flagged (whitelist requires | ||
| fully qualified name; same-file classes have no module path) |
There was a problem hiding this comment.
The test case at line 353 states 'Same-file base whitelisted by bare name: still flagged (whitelist requires fully qualified name; same-file classes have no module path)'. This is good, but it raises a design question: should the implementation provide a more helpful error message in this case, telling users that same-file bases cannot be whitelisted? This could be a fourth error variant or an enhancement to the documentation.
| whitelist `"models.Base"` (no project package prefix) → `False` | ||
| (entry root not in project_packages) |
There was a problem hiding this comment.
The test case at lines 655-656 has a logical inconsistency: it tests 'from .models import Base (level=1, module="models") with whitelist "models.Base" (no project package prefix) → False'. However, according to the algorithm at line 491, it first checks if 'entry_root = whitelist_entry.split(".")[0]' (which would be "models") is in project_packages. The test should clarify what project_packages is set to (likely empty tuple or ("mypackage",)), otherwise the test expectation is ambiguous.
| whitelist `"models.Base"` (no project package prefix) → `False` | |
| (entry root not in project_packages) | |
| whitelist `"models.Base"`, project_packages=`("mypackage",)` → `False` | |
| (entry root `"models"` not in project_packages) |
| to a `frozenset` for O(1) lookups, then passes it along with | ||
| `_project_packages` to `InheritanceVisitor` (or to a filtering step) | ||
| - Before recording an INH001 error, resolve the base to its fully qualified | ||
| name and check membership in the whitelist set | ||
| - O(1) lookup via frozenset membership test (converted from the stored tuple) |
There was a problem hiding this comment.
Ticket 5 mentions that 'InheritanceChecker.run() converts the _inh001_whitelisted_bases tuple to a frozenset for O(1) lookups', which follows the established pattern. However, it should also mention storing this frozenset in a local variable to avoid repeated conversions if the whitelist needs to be checked multiple times within the same run() execution, similar to how allowed_dunders is handled at lines 122-126 of checker.py.
| to a `frozenset` for O(1) lookups, then passes it along with | |
| `_project_packages` to `InheritanceVisitor` (or to a filtering step) | |
| - Before recording an INH001 error, resolve the base to its fully qualified | |
| name and check membership in the whitelist set | |
| - O(1) lookup via frozenset membership test (converted from the stored tuple) | |
| to a `frozenset` once, stores it in a local variable (e.g., | |
| `whitelisted_bases`) for reuse and O(1) lookups, then passes it along | |
| with `_project_packages` to `InheritanceVisitor` (or to a filtering step) | |
| - Before recording an INH001 error, resolve the base to its fully qualified | |
| name and check membership in the locally stored whitelist `frozenset` | |
| - Perform O(1) lookup via membership test on this cached `frozenset` |
| `original_names` dict is preserved for backward compatibility with | ||
| existing `classify()` and `ABCPurityVisitor` logic. |
There was a problem hiding this comment.
The description at line 200-203 states that 'original_names is NOT needed for FQN resolution since full_modules already stores the complete binding FQN' and that original_names is 'preserved for backward compatibility'. However, this creates technical debt by maintaining two overlapping storage mechanisms. Consider adding a note in Ticket 3's acceptance criteria or in a future refactoring ticket about eventually deprecating original_names once full_modules proves sufficient.
| `original_names` dict is preserved for backward compatibility with | |
| existing `classify()` and `ABCPurityVisitor` logic. | |
| `original_names` dict is currently preserved for backward compatibility with | |
| existing `classify()` and `ABCPurityVisitor` logic, but it represents technical | |
| debt and should be considered for deprecation in a future refactoring ticket | |
| once `full_modules` has been validated as sufficient. |
| `"mypackage.models" + ".Base"` = `"mypackage.models.Base"` | ||
| - Relative import: `from .models import Base` → returns `None` (the base | ||
| is in `relative_imports`, not `full_modules`; handled separately in Ticket 7) | ||
| - Bare name (same-file class): `Base` with no import → resolves to bare `"Base"` |
There was a problem hiding this comment.
At line 267, the test case 'Bare name (same-file class): Base with no import → resolves to bare "Base"' conflicts with the FQN resolution logic described earlier. If a name has no import entry in full_modules, resolve_fqn() should likely return None (unresolvable) rather than returning the bare name, since the bare name is not a fully qualified name. The test case and acceptance criteria need clarification on the expected behavior for same-file classes.
| - Bare name (same-file class): `Base` with no import → resolves to bare `"Base"` | |
| - Bare name (same-file class): `Base` with no import → returns `None` (not in `full_modules`; same-file handling occurs elsewhere) |
| `pyproject.toml` formats | ||
| - `docs/error-codes.md` — Note that INH001 respects the whitelist | ||
| - `CHANGELOG.md` — Add entry under `[Unreleased]` | ||
| - `CLAUDE.md` — If the architecture section needs updating |
There was a problem hiding this comment.
The description for Ticket 9 (line 717) mentions updating 'CLAUDE.md' but this file doesn't appear to contain architecture documentation based on the repository structure. According to CLAUDE.md in the repo (which is a guide for AI assistants), it's unlikely to need updates for this feature. Consider removing this from the list or clarifying what aspect of CLAUDE.md needs updating.
| - `CLAUDE.md` — If the architecture section needs updating |
Break down the whitelist feature into 10 tickets covering option
registration, fully qualified name resolution, whitelist integration,
relative import handling, documentation, and an ADR. All tickets
follow TDD with test-first pairs.
https://claude.ai/code/session_01B6MjoPom9DynLD8UDUpsvA