Repository navigation
feat(detectors): record which module root each location belongs to (phase 2.8, producer half) - #439
Merged
Merged
Conversation
Scope, relationship and reachability answer questions about a usage, not about a package. Today a node carries a scope union, one merged relationship scalar, and nothing tying either to the module whose resolution observed it -- so "reachable, runtime and direct" can come back true when no single usage satisfies all three: reachable in one module, runtime in another, direct in a third. ADR-0037 fixes that by making the usage unit (module root, declaration site) and storing the module root on the location, because two modules can share one lockfile and the path alone cannot say whose usage a line is. This is the producer half. Native detectors now stamp, on every location they emit, the module root that reached the site, the relationship that root has to the package, and the scopes of that particular usage. The node-level union and scalar are untouched; the per-site record sits beside them. The derivation lives in one place, detectors.Attributed, and every detector returns its result through it. It walks each module node in the result, derives directness from that root's own edges -- deliberately not sdk.RelationshipForPath, which prefers the stored scalar and so cannot tell one root's declaration from another root's transitive path -- and decides what it may honestly say about scopes: a detector that reads per-module declarations passes them in and gets the real per-site scope; without them the node's union is only that root's view when no other root reaches the node, and otherwise no scope is claimed at all. A guard test fails when a detector returns graphs without going through the helper, because the rule is one line and one line is what gets forgotten. A site inside a module's own directory belongs to that module alone, so a Maven reactor sibling does not claim module-a/pom.xml. A site at the top of the scan is shared, so each npm workspace member that reaches a lockfile line gets its own record of it -- which is what makes a package that is a direct development dependency of one member and a transitive runtime dependency of another come back as two distinguishable usages. Two detectors are exempt, with the reason recorded in each file and named by the guard: an ingested SBOM's packages were resolved elsewhere, and a GitHub Actions workflow is not a module. An empty module root is the honest record of a site nobody attributed. Consolidation rebases the module root alongside the paths it already rebases. A detector names its own working directory ".", and two subprojects both claiming "." would make the join key useless the moment a recursive scan merges them. Co-Authored-By: Claude Opus 5 <[email protected]>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (45)
📒 Files selected for processing (43)
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 |
Contributor
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
…here The producer half of this change records per-site attribution, but the output projection dropped it -- so a workspace that reaches one lockfile line from two members rendered the same path twice, byte-identical, with nothing to tell the reader why. That is worse than the summary it replaced, and it is why the two workspace smoke goldens were left red rather than blessed as drift. The projection carries the module root, the per-site scopes and the per-site relationship now, so those records are distinguishable and a conjunctive question -- reachable, runtime and direct -- can be asked of one usage rather than of three different ones summarized onto a package. An attributed site with no path is kept: it still says which module reached the package and how. Carrying a slice costs LocationRef its comparability, the same break the SDK took on PackageLocation for the same reason. Nothing compares one or keys a map by one; the type says so. Goldens and schemas regenerated. Every removed golden line is a real_path re-emitted with a trailing comma because fields now follow it -- checked, not assumed. Co-Authored-By: Claude Opus 5 <[email protected]>
Thirty-six goldens gain module_root, relationship and scopes on their location records. Audited rather than blessed: across every one, the only added keys are those three plus the paths of records that now appear distinctly, and not a single removed line is anything but a real_path or access_path re-emitted with a trailing comma because fields follow it. The baseline workflow golden moved too, and that one was checked on its own: finding ids, dependency refs, package identity and the audit summary are untouched, so baselines still key on package and finding references rather than on locations. Co-Authored-By: Claude Opus 5 <[email protected]>
Collaborator
Author
|
Follow-up pushed: this PR is now self-contained and green. It was opened deliberately red, with two workspace smoke goldens left unregenerated because the output projection dropped the new attribution — so a workspace reaching one lockfile line from two members rendered the same path twice, byte-identical. That was the right call: it is a regression, not drift, and blessing it would have shipped output worse than the summary it replaced. Rather than leave a merge-ordering dependency, I carried the attribution into Those two records are now distinguishable, and a conjunctive question — reachable, runtime and direct — can be asked of one usage instead of three summarized onto a package. Golden movement was wider than the two predicted: 36 goldens carry locations, including the audit, diff, explain and lite suites. Audited rather than accepted — across all 36 the only added keys are
Schemas regenerated ( Mutations: dropping the attribution from the projection fails
🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Producer half of SDK maturity plan row 2.8. Native detectors now record, on every
PackageLocationthey emit, the module root whose resolution produced the site, that site's relationship, and that site's scopes — the join key ADR-0037 ("Usage facts carry their attribution") defines as(module root, declaration site).What attribution now lands
One shared helper,
detectors.Attributed(internal/detectors/attribution.go), is the only place the derivation lives; every detector returns itssdk.DetectionResultthrough it, and a guard test (TestDetectionResultsCarryingGraphsAreAttributed) fails when one does not..for the detector's own working directory,apps/webfor a workspace member,module-afor a Maven reactor project).unknownpreserved when the detector already said the parent could not be recovered. Deliberately notsdk.RelationshipForPath, which prefers the node's stored scalar — the merged value that cannot tell one root's declaration from another root's transitive path. That decline is recorded in the code.ModuleDeclarations); otherwise the node's union, but only when exactly one module root reaches the node. When several do, the union mixes roots and the site claims no scope rather than a scope nobody observed there.module-a/pom.xml); a file at the top of the scan is shared, so each workspace member that reaches a lockfile line gets its own record of it.internal/engine/consolidationrebases the module root next to the paths it already rebases: every detector calls its own working directory., and two subprojects both claiming.would make the key useless the moment a recursive scan merges them. That is the only engine change; the new fields otherwise survive consolidation untouched (verified).The row 2.8 regression case
TestNPMLockfileWorkspaceRecordsOneLocationPerModuleUsage(new fixturenpm-v3-workspaces-cross-scope):shared-toolis a direct development dependency ofapps/weband a transitive runtime dependency ofpackages/lib. It comes back as two records of the same lockfile line —{apps/web, direct, [development]}and{packages/lib, transitive, [runtime]}— while the node's own scope union keeps both, unchanged.TestMavenSharedDependencyGetsOneRecordPerReactorModuleis the reactor form: one artifact, one record per reactor module, each pointing at its own pom, and no invented scope because Maven passes no declarations.Which detectors attribute, and which cannot
Attributing (module root + relationship on every location they emit): cargo, cocoapods, composer, conan, go, gradle, maven, mix, npm, nuget, pnpm, poetry, pip, pipenv, pub, ruby, sbt, swiftpm, uv, yarn.
Not attributing, exemption recorded in the file and named by the guard test:
.github/workflows, a directory that declares no module.Scope precision beyond the module root is currently supplied only by npm; pnpm, yarn, cargo and maven have per-module declarations available and could be upgraded the same way in a follow-up.
Golden movement — read this before merging
make verifypasses with no drift:internal/output.LocationRefprojects onlyreal_path,access_pathandposition, so single-module scan JSON is byte-identical.Two smoke goldens do move, and I did not regenerate them:
scan-npm-workspacesandscan-cargo-workspace. Because the projection drops the attribution, the shared-lockfile records for two workspace members render as duplicate location entries — a diff that shows something other than added attribution, which is a bug rather than drift. The fix belongs to the consumer half of row 2.8 (projectmodule_root/scopes/relationshipintoLocationRef), which is out of this PR's scope.So: this PR should land together with, or after, the consumer-half change that projects the attribution, and those two goldens should be regenerated then. Alone it leaves those two smoke cases red and workspace scan JSON showing duplicated-looking locations.
Related SDK gap found while verifying (recorded in the code, worth an SDK issue):
hasDependencyLocationcompares paths and position only, so merging two separate nodes that each carry one root's record drops the second. Workspace detectors partition with shared node pointers, so the case above survives; two detectors resolving one package from one path would not.Mutations run
Each applied alone, tree kept compiling, then reverted:
TestAttributedRecordsTheModuleRootAndDirectness, npm, maven, gomodTestAttributedGivesEachModuleRootItsOwnRecord, cycle test, npmTestAttributedLeavesAnotherModulesFileAlone, mavenTestRebaseGraphLocations_MovesModuleRootIntoRepositoryCoordinatesAttributeddoes nothingNamelookup onlyQualifiedName()catches it. Redone as M7bTestAttributedGivesEachModuleRootItsOwnRecord, cycle test, npmTestAttributedLeavesScopesEmptyWhenTheUnionMixesModules, mavenAttributed(the realistic form)TestDetectionResultsCarryingGraphsAreAttributedDelegation check
Ran the
delegation-checkskill before implementing. Outcome: no new grammar, vocabulary, or format is introduced. The scope lattice, relationship vocabulary and merge rules are the SDK's and are used through it (sdk.MergeScope,sdk.ScopesOf,sdk.DependencyRelationship*,sdk.AsDependencyNode); path handling ispath/strings. One decline is recorded in the code:sdk.RelationshipForPathis not used for per-site directness because it prefers the stored merged scalar, which is exactly the lossy summary this change exists to replace. No parser added, so no new fuzz target is required.🤖 Generated with Claude Code