feat(extension-api): add path-attribution seam for which-module claims - #1072
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR adds Axis-D path attribution as a sibling extension capability. It discovers and merges module ownership claims, resolves paths through longest-prefix matching, replaces the hardcoded map, and exposes attribution provenance in ChangesPath attribution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
PR Reviewer Guide 🔍(Review updated until commit 9f17caf)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
resolve_path_attribution, the seam import is only guarded forImportError; if_load_path_attribution_seamitself raises other exceptions (e.g. fromresolve_bundle_pathor discovery code), they will bubble and breakwhich-module, so consider broadening the exception handling there to maintain the "null-on-absent, never raise" contract. - The path-attribution seam functions returned by
_load_path_attribution_seamare re-imported on everyresolve_path_attributioncall even though the merged claims are memoized; caching the(discover_path_attributors, merge_path_claims, lookup_claim)triple at module level would avoid repeated cross-skill imports without changing behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolve_path_attribution`, the seam import is only guarded for `ImportError`; if `_load_path_attribution_seam` itself raises other exceptions (e.g. from `resolve_bundle_path` or discovery code), they will bubble and break `which-module`, so consider broadening the exception handling there to maintain the "null-on-absent, never raise" contract.
- The path-attribution seam functions returned by `_load_path_attribution_seam` are re-imported on every `resolve_path_attribution` call even though the merged claims are memoized; caching the `(discover_path_attributors, merge_path_claims, lookup_claim)` triple at module level would avoid repeated cross-skill imports without changing behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Triage dispositionsIn reply to comment_id:
|
|
/review |
9f17caf to
3d360db
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
marketplace/bundles/plan-marshall/skills/manage-architecture/scripts/_architecture_core.py (1)
821-824: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not catch bundle-resolver failures inside
_load_path_attribution_seam.
resolve_bundles_root()andresolve_bundle_path()can raiseRuntimeErrorbefore the extension-api imports; theexcept ImportError:around_load_path_attribution_seam()does not catch those failures. Updateresolve_path_attribution()’s contract/guard to match_load_route_matcher()and avoid returning(None, [])for resolver misconfiguration.marketplace/bundles/plan-marshall/skills/extension-api/standards/ext-point-path-attribution.md (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe Axis-C purity wording was copied to a hook that takes no arguments.
claim_paths()declares no parameters, so "a pure function of its arguments" does not describe it.derive_edges(derived_by_name, enriched_by_name)does take arguments, which is where the phrase came from.
marketplace/bundles/plan-marshall/skills/extension-api/standards/ext-point-path-attribution.md#L60-L60: state that the attributor is stateless and declares claims from static knowledge, with no subprocess and no filesystem access.marketplace/bundles/plan-marshall/skills/script-shared/scripts/extension/extension_base.py#L1475-L1500: apply the same wording to theclaim_pathsdocstring at line 1478.marketplace/bundles/plan-marshall/skills/extension-api/scripts/extension_discovery.py (1)
573-622: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared sibling-ABC collector.
Lines 573-622 duplicate
discover_derivation_resolvers(lines 478-527) almost exactly. Only the ABC, the log noun, and the local names differ. Two copies of the identity validation and the duplicate-id rule must now stay in step by hand, and a fifth axis would add a third copy.Extract one private helper parameterized by the ABC and the label, then make both public collectors thin wrappers. The helper also lets both axes share one candidate scan instead of calling
discover_all_extensions()anddiscover_build_extensions()twice per process.♻️ Suggested shape
def _discover_sibling_axis_implementors(abc_cls, label: str) -> list[dict[str, Any]]: """Collect every registered implementor of a sibling axis ABC, sorted by id.""" accessor = 'derivation_resolver_id' if label == 'derivation resolver' else 'path_attributor_id' candidates: list[tuple[str, Any]] = [] for ext in discover_all_extensions(): module = ext.get('module') if module is not None: candidates.append((ext.get('bundle', 'unknown'), module)) for ext in discover_build_extensions(): module = ext.get('module') if module is not None: candidates.append((ext.get('skill', 'unknown'), module)) # ... one copy of the isinstance filter, the guarded id call, the # non-empty-string check, the uniqueness check, and the sort. def discover_path_attributors() -> list[dict[str, Any]]: from extension_base import PathAttributionBase return _discover_sibling_axis_implementors(PathAttributionBase, 'path attributor')Pass the accessor explicitly rather than deriving it from the label.
marketplace/bundles/plan-marshall/skills/extension-api/scripts/_path_attribution_merge.py (1)
44-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winA dot-relative or traversing prefix survives normalization and can never match.
_ROOT_ISH_PREFIXEScatches only the exact strings'','.', and'..'. A claim on'./doc'normalizes to'./doc', and a claim on'../sibling'normalizes to'../sibling'. Both pass the bounded-subtree filter, enter the claim list, and raise the attributor'sclaim_count. Neither can ever match a repo-relative path, so the report shows a confident non-zero claim count for a claim that never fires. That is the same vacuity the merge notes exist to remove.Drop a prefix whose first segment is
.or..and append amerge:note, as the other filters do.♻️ Suggested normalization change
def _normalize_prefix(raw: str) -> str: prefix = raw.strip().replace('\\', '/').rstrip('/') - return '' if prefix in _ROOT_ISH_PREFIXES else prefix + if prefix in _ROOT_ISH_PREFIXES: + return '' + # A leading '.' or '..' segment either restates the root or escapes it, so + # the claim can never contain a repo-relative path. + if prefix.split('/', 1)[0] in ('.', '..'): + return '' + return prefixtest/plan-marshall/extension-api/test_path_attribution_merge.py (1)
487-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a lookup case for a differently spelled candidate path.
The lookup tests all pass an already-normalized
path. No test states whatlookup_claimmust do when the candidate carries a trailing slash, a./head, or backslashes, which is the asymmetry raised onmarketplace/bundles/plan-marshall/skills/extension-api/scripts/_path_attribution_merge.pylines 323-331. Add the case together with that fix so the intended answer is pinned.💚 Suggested test
def test_lookup_claim_normalizes_the_candidate_path(): # Arrange — the claim side is normalized by the merge, so the lookup side # must apply the same rule or the two disagree on spelling. claims = [{'prefix': '.plan', 'module': 'plan-marshall', 'producers': ['alpha']}] # Act / Assert assert _merge.lookup_claim('.plan/local/', claims) == 'plan-marshall' assert _merge.lookup_claim('.plan\\local', claims) == 'plan-marshall'
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 515de2aa-cf59-44f6-9446-de3cfac0fd4d
⛔ Files ignored due to path filters (1)
doc/resources/diagrams/extension-topology.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
doc/concepts/README.adocdoc/concepts/code-intelligence.adocdoc/concepts/extension-architecture.adocmarketplace/bundles/plan-marshall/skills/extension-api/SKILL.mdmarketplace/bundles/plan-marshall/skills/extension-api/scripts/_path_attribution_merge.pymarketplace/bundles/plan-marshall/skills/extension-api/scripts/extension_discovery.pymarketplace/bundles/plan-marshall/skills/extension-api/standards/ext-point-path-attribution.mdmarketplace/bundles/plan-marshall/skills/extension-api/standards/extension-contract.mdmarketplace/bundles/plan-marshall/skills/manage-architecture/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-architecture/scripts/_architecture_core.pymarketplace/bundles/plan-marshall/skills/manage-architecture/scripts/_cmd_client_handlers.pymarketplace/bundles/plan-marshall/skills/manage-architecture/standards/client-api.mdmarketplace/bundles/plan-marshall/skills/plan-marshall-plugin/extension.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/extension/extension_base.pytest/plan-marshall/extension-api/test_path_attribution_discovery.pytest/plan-marshall/extension-api/test_path_attribution_merge.pytest/plan-marshall/manage-architecture/test_cmd_client.pytest/plan-marshall/manage-architecture/test_files_inventory.pytest/plan-marshall/manage-architecture/test_which_module_plan_claim.pytest/plan-marshall/script-shared/test_extension_base_path_attribution.py
Triage dispositionsIn reply to comment_id:
|
PR #1072 review follow-up. The claim side normalized path spelling while the lookup side compared raw, so a covered path handed to which-module in an equivalent-but-different spelling answered a confident module: null — the exact vacuity this seam exists to remove. Probed before the fix: './.plan/local' and '.plan\local' both resolved to null; '.plan/' and '.plan/local/' already resolved (the trailing-slash half of the review comment was refuted, and is now pinned as a regression test). Both sides now consume one shared spelling normalizer (whitespace, backslash folding, leading './' runs, trailing '/'). A dot-relative claim prefix is NORMALIZED to its bare form rather than dropped — its ownership intent is unambiguous, and dropping it would discard a stated declaration. Only a '..'-rooted prefix is dropped, because no repo-relative path can fall inside it, and that drop is reported through the existing merge: note channel rather than being silent. Also de-duplicates the sibling-axis collector this PR introduced: discover_path_attributors was a hand-copied twin of discover_derivation_resolvers, including two copies of the identity validation and the duplicate-id rule. Both are now thin wrappers over one private helper parameterized by the ABC, the accessor name, and the log labels. The accessor is passed explicitly rather than derived from the log wording, so rewording a WARNING cannot change which method is called. Public signatures, return shapes, and per-skip WARNING wording are unchanged, so the Axis-C tests stay green. Documentation corrections: the Axis-C "pure function of its arguments" phrasing was copied onto claim_paths(), which takes no arguments — reworded to statelessness at both sites and in the extension_base docstring. Adds a Raises: note recording that resolve_path_attribution deliberately lets a bundle-resolver RuntimeError propagate rather than degrading it to (None, []), so the narrow ImportError guard is not widened by a future reader. Co-Authored-By: Claude <noreply@anthropic.com>
…, and the claim merge
…r the attribution seam
…closed on which-module
… and residue distinction
…ation Document Axis-D across the three concept pages and the topology diagram: - code-intelligence.adoc gains a Tier-0 companion section covering why path attribution is an extension point, how the keyed-mapping merge differs from the Axis-C edge union (a value exists to disagree about, so a conflict is expressible), and the attributor_count residue table. - extension-architecture.adoc moves every stale cardinality to the current counts (eleven -> twelve domain-facing points, 16 -> 17 contracts, three -> four axes), gains an Axis-D bullet, extends the disjointness rationale to the fourth axis, and rewords the multiple-inheritance exclusivity claim to name both derivation-resolver and path-attribution. - README.adoc updates the extension-architecture summary line. - extension-topology.svg grows from nine to twelve cards in a filled 6x2 grid (domain-verb, derivation-resolver, path-attribution), with the viewBox, core panel, connector arrows, footer caption, title and desc re-spaced to match. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…broken-implementor try A truthy non-iterable claims value escaped merge_path_claims as an uncaught TypeError, aborting path-attribution resolution and blanking the whole ownership map behind which-module rung-3. Materialize the claims collection inside the existing guard so a non-iterable, and a bare string that would iterate into characters rather than pairs, degrade to the same error report a raising claim_paths() already produces. A falsy non-iterable still means claimed nothing. Widens the module docstring and ext-point-path-attribution.md to match, and adds three tests pinning both broken shapes and the falsy carve-out. Co-Authored-By: Claude <noreply@anthropic.com>
PR #1072 review follow-up. The claim side normalized path spelling while the lookup side compared raw, so a covered path handed to which-module in an equivalent-but-different spelling answered a confident module: null — the exact vacuity this seam exists to remove. Probed before the fix: './.plan/local' and '.plan\local' both resolved to null; '.plan/' and '.plan/local/' already resolved (the trailing-slash half of the review comment was refuted, and is now pinned as a regression test). Both sides now consume one shared spelling normalizer (whitespace, backslash folding, leading './' runs, trailing '/'). A dot-relative claim prefix is NORMALIZED to its bare form rather than dropped — its ownership intent is unambiguous, and dropping it would discard a stated declaration. Only a '..'-rooted prefix is dropped, because no repo-relative path can fall inside it, and that drop is reported through the existing merge: note channel rather than being silent. Also de-duplicates the sibling-axis collector this PR introduced: discover_path_attributors was a hand-copied twin of discover_derivation_resolvers, including two copies of the identity validation and the duplicate-id rule. Both are now thin wrappers over one private helper parameterized by the ABC, the accessor name, and the log labels. The accessor is passed explicitly rather than derived from the log wording, so rewording a WARNING cannot change which method is called. Public signatures, return shapes, and per-skip WARNING wording are unchanged, so the Axis-C tests stay green. Documentation corrections: the Axis-C "pure function of its arguments" phrasing was copied onto claim_paths(), which takes no arguments — reworded to statelessness at both sites and in the extension_base docstring. Adds a Raises: note recording that resolve_path_attribution deliberately lets a bundle-resolver RuntimeError propagate rather than degrading it to (None, []), so the narrow ImportError guard is not widened by a future reader. Co-Authored-By: Claude <noreply@anthropic.com>
19deafe to
ec9f8eb
Compare
1 participant
Summary
Replaces the hardcoded
_PROJECT_LOCAL_PREFIX_MAPsingle-entry tuple inmanage-architecture's_architecture_core.pywith a bundle-contributed path-attribution extension point, declared as a fourth sibling axis (PathAttributionBase, Axis-D) and modelled face-for-face on the shippedext-point-derivation-resolvercontract — declaration, discovery, dispatch, null-on-absent.Core keeps the merge (longest-prefix wins), the provenance (which bundle claimed the match), and the resolution order inside
cmd_which_module's ladder; bundles own only their individual path claims. A path claimed by two bundles is an ambiguous key — the merge emits no attribution and reports the collision rather than breaking the tie by iteration order.The seam ships with two claims: the re-homed
.claude/skills → plan-marshallentry (owner unchanged) and core's own.plan/** → plan-marshallclaim, which closes themodule: nullanswer that every.plan/script path previously received.Changes
New extension point (contract + discovery + merge)
marketplace/bundles/plan-marshall/skills/extension-api/standards/ext-point-path-attribution.md— new Axis-D contract, sibling toext-point-derivation-resolver.md.marketplace/bundles/plan-marshall/skills/script-shared/scripts/extension/extension_base.py— declares thePathAttributionBaseABC.marketplace/bundles/plan-marshall/skills/extension-api/scripts/extension_discovery.py— span-both-collectors discovery for the new axis.marketplace/bundles/plan-marshall/skills/extension-api/scripts/_path_attribution_merge.py— core-owned claim merge: longest-prefix wins, provenance retained, ambiguous keys suppressed.marketplace/bundles/plan-marshall/skills/extension-api/SKILL.md,marketplace/bundles/plan-marshall/skills/extension-api/standards/extension-contract.md— register the new point in the contract index.Retire the prefix map, re-home and add the claims
marketplace/bundles/plan-marshall/skills/manage-architecture/scripts/_architecture_core.py—_PROJECT_LOCAL_PREFIX_MAPremoved; attribution now flows through the seam.marketplace/bundles/plan-marshall/skills/plan-marshall-plugin/extension.py— declares both claims (.claude/skillsre-homed,.plan/**new).Fail-closed residue reporting
marketplace/bundles/plan-marshall/skills/manage-architecture/scripts/_cmd_client_handlers.py—which-moduledistinguishesattributor_count: 0fromattributor_count: Nwith no claim, mirroring theresolver_countdistinction at Tier 1.Documentation
marketplace/bundles/plan-marshall/skills/manage-architecture/SKILL.md,marketplace/bundles/plan-marshall/skills/manage-architecture/standards/client-api.md— updatedwhich-moduleresolution-order contract and response shape.doc/concepts/code-intelligence.adoc,doc/concepts/extension-architecture.adoc,doc/concepts/README.adoc,doc/resources/diagrams/extension-topology.svg— updated attribution model, axis counts, and extension-topology diagram.Tests
test/plan-marshall/script-shared/test_extension_base_path_attribution.py— ABC contract.test/plan-marshall/extension-api/test_path_attribution_discovery.py,test/plan-marshall/extension-api/test_path_attribution_merge.py— discovery and merge (including ambiguous-key suppression).test/plan-marshall/manage-architecture/test_which_module_plan_claim.py—.plan/**resolves toplan-marshall.test/plan-marshall/manage-architecture/test_files_inventory.py,test/plan-marshall/manage-architecture/test_cmd_client.py— updated for the retired map and the residue distinction.Test Plan
plugin-doctorclean across the 4 gated skillsCompatibility
compatibility=breaking— clean-slate replacement of the prefix map, no deprecation shim..claude/skills/**resolution is unchanged;.plan/**newly resolves toplan-marshallinstead ofnull.Generated by plan-finalize skill
Summary by CodeRabbit
New Features
.claude/skillsand.planpath ownership.Documentation
Bug Fixes