feat!: adopt the typed graph node union (bomly-sdk v0.9.0, ADR-0041) - #423
Merged
Conversation
bomly-sdk v0.8.0 makes the dependency graph a sealed union of manifest, module, and dependency nodes, with identity minted as a canonical package URL at construction (ADR-0041). This is the CLI's adoption of that model. What changes in the pipeline: - The project's own artifacts are module nodes. Every detector that used to mint a dependency node and then mark it -- FirstParty on coordinates, a forced "workspace:<dir>" ID, DependencySourceWorkspace as a stand-in -- now builds a module node whose ID carries the declaring manifest path. Ownership is the node kind; there is nothing to set afterwards. - Duplicate identities fold. The occurrence machinery is gone: two records that resolved one name@version from different places become one node whose Origins list carries both, which is a stronger dependency-confusion signal than two nodes with byte-identical identity. - Application-typed imports are ordinary dependency nodes, so they diff and match like any other package (ADR-0015). Three helpers absorb what the migration would otherwise have scattered: - internal/nodes reads a node of any kind -- coordinates, display fields, narrowing. The GraphNode interface exposes only what every kind has, so every renderer needed the same type switch; written per caller it disagrees with itself about what a manifest looks like. The SDK is the deeper home (ADR-0040) and v0.8.0 has no coordinates accessor, so this delegates once bomly-dev/bomly-sdk#33 ships. - detectors.EnsureNode is generic in the node type, so inserting a module returns a module. A survivor of another kind is an error rather than a silent nil. - detectors.PropagateScopes replaces three copies of the same BFS scope walk, one per Python lockfile detector. Only the seed differed, so that is the parameter. Tests are migrated in the following commit.
Fixtures can no longer be struct literals with a hand-chosen ID, and graph traversal yields the union rather than dependency nodes, so every test that built or read a node had to change. Two helpers keep that from becoming a type switch per assertion: - internal/testnodes takes the fixture shapes the tests already used and routes them through the real constructors, panicking rather than returning an error -- a fixture whose coordinates cannot mint an identity is a broken test, not a condition under test. - a package-local mustDep narrows a node the case is asserting about, failing instead of panicking when the graph holds another kind. Three suites tested behaviour that no longer exists and were rewritten rather than adjusted, because adjusting them would have left tests that pass while pinning nothing: - The cargo dual-source cases asserted two nodes for one crate resolved from two remotes. They now assert the fold and both origins on the survivor, with the reasoning recorded in detectors.EnsureNode. - The consolidation origin suite was almost entirely about occurrence-ID minting and its order-independence. It now covers what folding must not lose: both resolutions, scopes, locations, the stronger relationship, and the separation between a project module and an external package that names the same coordinates. - detectors.EnsureOccurrence's test became a fold test over the same inputs. Node IDs in assertions are canonical package URLs now; lookups that keyed on the old "name@version" string either use the PURL or find the node by name.
…oad rewrite dropped
…ey set The npm, pnpm, yarn, and bun lockfile parsers each described a package as one struct literal and passed only its coordinates to the constructor, so ResolvedURL, Source, the integrity digests, and the npm metadata stopped reaching the graph -- silently, in four places, for the same reason. detectors.NewDependencyFrom is that copy written once.
Two user-visible losses the union adoption would otherwise have shipped: - A dependency path rendered only its dependency nodes, so every explain path started at the first consumed package and never said which module pulled it in -- which is the question explain answers. Structural nodes render too now, through output.PackageFromGraphNode. - The scan document's manifest listing walked dependency nodes only, so every depends_on chain was headless. Modules are listed alongside packages; manifests are not, being what the listing is about. Also: explain resolves a target by its ecosystem-native name as well as its bare one. Normalization splits a qualified name into org and name, so "bomly explain golang.org/x/text" stopped matching anything.
…url fork go mod tidy demotes github.com/anchore/packageurl-go to an indirect dependency: nothing in this repository imports it any more, which is the removal ADR-0041 slated. purlkit over the official packageurl-go is the parser now.
Contributor
|
Important Review skippedToo many files! This PR contains 211 files, which is 111 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (69)
📒 Files selected for processing (211)
You can disable this status message by setting the 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Bomly Diff SummaryCompared Overview
Dependency ChangesSummary: 0 added, 11 version changed, 1 detail change, 0 removed. Changed Dependencies
Dependency Detail ChangesReview: 0 of 1 detail change needs extra review.
Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
![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: 662568440f
ℹ️ 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".
Five ineffectual assignments and one empty branch, each a leftover from a rewrite rather than a style nit -- and one was a swallowed error: cargo's root construction returned an err the next if-statement shadowed, so a node that failed to build would have been added as nil. The consolidation helper that searched for an uncolliding synthesized root ID is deleted with them: a manifest node's ID is minted from its path by a grammar that cannot collide with a package URL or a module ID.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920b2f536d
ℹ️ 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".
The migration inserted imports at the top of each block, so the SDK and internal packages landed among the standard library. gofmt sorts within a group but never regroups, and nothing in .golangci.yml checks it.
bomly-guy
commented
Sep 2, 2026
bomly-guy
commented
Sep 2, 2026
bomly-guy
commented
Sep 2, 2026
Regenerating locally surfaced three ways the goldens recorded the machine that produced them rather than anything Bomly decided. Each would have made a local regeneration break CI, and each was already latent: - Container scans baked in the runner's architecture. A multi-arch image resolves to the host, so amd64 on CI and arm64 on a laptop differ on every package in the image. Normalized to <arch>, with the alternation listing both spellings a package manager uses for one machine (amd64/x86_64) so a genuinely wrong arch still shows up. - Absolute temp paths leaked through the reachability evidence added in bomly-sdk v0.8.0: module_root carried /var/folders/... on macOS and /tmp on Linux. The random suffix was normalized; the prefix was not. - Strings inside arrays were never normalized at all -- only map values were -- so a package URL in a depends_on list kept the host arch while the same URL as a map value did not. And one that hid real coverage: the golden filter dropped every entry whose ID was not a package URL. That was written when synthetic roots had ad-hoc IDs; every node ID is minted by a grammar now, so the filter was silently removing every module and manifest node -- the project's own code, which is exactly what a workspace or reactor-build case exists to check. The min-version plugin fixture gets its own source. That test builds the example plugin against the oldest SDK release whose binaries must keep loading, and the guarantee is about the wire, not the source API: the node constructors replaced sdk.NewDependency, so one source cannot compile against both v0.1.0 and the pin. Both sources are now compile-checked against the release each targets.
Adding a golden for the exported SBOM found this on the first run: a module component carried module:apps/web/package.json#pkg:npm/web@1.0.0 in the purl field, which both formats define as a Package URL and no consumer can parse. The bom-ref is where the node identity belongs; the purl is the one its coordinates mint. The export had assertions but no golden -- it is the artifact most consumers actually read, and nothing pinned its shape, so a change to which components appear at all went unchecked. Both formats are now exported from one scan and compared, so they cannot drift apart. Documents get their own golden path. normalizeJSON knows the scan/diff/explain response schema and edits it on sight -- it zeroes metadata.duration_ms, and a CycloneDX document has a metadata object too -- so running it over an SBOM invented a field neither side produced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28414831b7
ℹ️ 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".
bomly-sdk v0.9.0 ships what this branch had been carrying locally, so the
stopgaps go and the call sites point at the model.
Deleted, each replaced by its SDK counterpart:
internal/nodes -> sdk.NodeCoordinates, NodeDisplayName,
NodeVersion, AsDependencyNode,
DependencyNodesOf, IsProjectOwned
detectors.EnsureNode -> detectorkit.EnsureNode
detectors.PromoteToModule -> detectorkit.PromoteToModule
detectors.PropagateScopes -> detectorkit.PropagateScopes
detectors.NewDependencyFrom -> sdk.NewDependencyNodeFrom
detectors.NewDependencyOrGeneric -> sdk.NewDependencyNode (the generic
fallback is the constructor's now)
detectors.RefineOrigins -> sdk.MergeOrigins (which drops a
superseded origin itself)
internal/testnodes keeps only what is CLI test ergonomics -- fixture builders
that panic instead of taking a testing.TB, so a table entry stays one
expression. Its label lookups delegate to bomly-sdk/testkit, and its copy of
the matching rules is gone: two answers to "which node is this" is how the
version fold drifted in the first place. Its hand-maintained field list goes
too, DepFrom now being NewDependencyNodeFrom.
The two structural guards move to internal/detectors/guards_test.go and stay
CLI-side, because they police this tree:
TestNodeInsertionGoesThroughTheSharedHelper now names detectorkit.EnsureNode
and exempts nothing, there being no local copy left to reach for.
The Cargo fold decision moves to internal/detectors/cargo/lock_index.go, next
to the fold it explains, rather than being deleted with the helper it was
written above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven findings from review, each verified against the behavior before being believed, and each with the assertion that was missing: Remediation stopped recognizing project roots. executableRootOf narrowed to *DependencyNode, so a module root -- what a normal graph now has -- was rejected before executableRoot ran. inferredPlacement then found no root and every dependency whose detector left Relationship unset degraded to manual review, which is most of a normal scan. executableRoot's own module clause was dead for the same reason and is gone. The provider graph clone kept only dependency nodes, dropping the modules, manifests and root edges, and ignored AddEdge errors. Providers may inspect RemediationHintRequest.Detection, so a disconnected clone changes the plugin contract. It now clones every kind through CopyEdgesInto, edge kinds included. Explain paths published "name@version" while the focused dependency published its canonical node ID, so nothing could join a path entry to a dependency, package, or finding -- and the short form collides across ecosystems. The legacy rewrite is deleted and the function renamed for what it still does. A module component published no group: componentOrg parsed NodeID, which for a module is the structural "module:<path>#<purl>" grammar and no package URL at all. It reads componentPURL now, so group and purl agree. A manifest root decided the document's primary component. The manifest is not a component, so the encoder silently promoted whichever package sorted first to be the subject of the whole document -- a project with two top-level packages described itself as one of them. Structural roots now resolve to the exported nodes beneath them, which lets the synthesized project root form. Module locations were never rebased onto the subproject path, so a recursively discovered module reported "pom.xml" where the repository holds "apps/service/pom.xml", and scan JSON published it. The enrichment INFO log counted every structural node as an excluded package, contradicting the comment above it. Also: make verify now compiles the smoke suite. It is behind a build tag, so `go vet ./...` never saw it, and this branch pushed a smoke file that did not compile. Running smoke needs the network; compiling it costs a second. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rest of the review findings, plus the smoke coverage they exposed. Exported SBOMs published only the first origin. ADR-0041 folds equal-identity records and keeps their disagreement as a list, which is the dependency-confusion signal the fold exists to preserve -- so publishing one described a package that resolved from two registries as though it came from one, backwards for the case that matters most. Component carries the whole list now: CycloneDX emits an external reference per origin, and SPDX, which defines one download location per package, emits the rest as OTHER-category references under a document-defined type. The SPDX category and refType strings are the library's constants rather than transcriptions of them. Module identities were never rebased onto the subproject path. A module's ID carries its declaring manifest path, and a detector writes that relative to its own working directory, so with --recursive two nested projects sharing a package name both minted "module:package.json#pkg:npm/app@1.0.0" and folded into one node holding both projects' edges. Consolidation rebases them where it already rebases manifest paths and locations; the monorepo smoke golden now shows "module:fixtures/webapp/package.json#...". ensureEntryRoot attached only dependency roots, leaving an independent module or manifest root loose -- so the entry still had several roots and every root-based projection downstream got an ambiguous graph, which is the condition that function exists to remove. Non-root module nodes were missing from the top-level parents, so a module another module depends on had its own direct dependencies reported as transitive. The existing test missed it by building its "modules" as application-typed dependency nodes; the new one uses real module nodes and fails without the fix. The pre-push hook accepted a stale stamp on Linux. GNU stat reads -f as "filesystem" and succeeds, so choosing the BSD spelling by "did stdout come back non-empty" filled newest with an inode report, and the comparison then errored inside a condition the hook did not check. It probes the flag now. Smoke: the SBOM export case ran against a mutable branch, so its golden tracked the fixture repository rather than Bomly -- pinned to the tag, like its sibling. The CycloneDX tool version is a separate field the string normalizer could not reach, so the golden carried a literal release number that would fail on the next bump. SARIF had no smoke coverage at all, though it is what GitHub code scanning reads and how Guard annotates a pull request. The new case pins a real rule, result, severity and repo-relative location, driven by a denied package rather than advisory data: the shared mock OSV server answers every query with no vulnerabilities, so a vulnerability-auditor case would have pinned an empty document and caught nothing. Goldens regenerated. The reachability goldens show the remediation fix end-to-end -- manual-review becomes direct-bump and transitive-override across Go, npm and Maven -- and explain paths now publish canonical package URLs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bomly-guy
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75f4889bb4
ℹ️ 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".
…r real PURL Two findings from Codex's review of 75f4889. Four Python parsers each hard-coded "requirements.txt" as the module's declaring manifest. A Pipenv project is declared by Pipfile and a Poetry project by pyproject.toml, and the declaring path is part of a module's identity now -- published in scan JSON, SBOM references and explain paths -- so the literal was both a wrong cross-reference and a fold risk between two projects declared by different files on matching coordinates. The rule gets one home, pythonModuleRoot, which derives the manifest from the package manager already on the coordinates, and TestPythonRootsGoThroughTheSharedConstructor fails if a direct NewModuleNode call reappears in the package. Routing uv through it exposed that its coordinates never stated their package manager -- it had the right literal by hand -- so it states it now and keeps pyproject.toml. pip and the pip-inspect synthetic root keep requirements.txt, so their identities are unchanged. The TUI rendered NodeID under a column labelled "PURL". For a module that is the structural "module:<path>#<purl>" grammar and for a manifest there is no package URL at all, so an interactive scan showed a value no consumer can parse while scan JSON and both SBOM exports had it right. Both TUI paths now use output.PurlFromGraphNode. That projection belongs on the SDK's GraphNode (ADR-0040) and is filed as bomly-dev/bomly-sdk#43. Until it ships there are two copies, and deliberately so: internal/sbom keeps the codec's own rather than importing the CLI's output layer, which would be backwards. Both are commented with the issue and converge on the accessor when it lands. Goldens: only the pipenv and poetry cases change, and neither could be regenerated here -- pipenv, poetry and uv are not installed on this machine, so those five smoke cases skip. Both parsers are pure, so TestPythonParserRootsNameTheDeclaringManifest asserts the new identities directly instead. Smoke runs in the merge queue, not on pull requests, so those two goldens need an Update Smoke Goldens dispatch before merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
bomly-guy
commented
Sep 5, 2026
bomly-guy
commented
Sep 5, 2026
bomly-guy
commented
Sep 5, 2026
…hole tree Three findings from review of 49b50a0. The manager-aware declaring manifest only reached the lockfile parsers. baseDetector.resolveGraph is the pip-inspect path shared by pip, Pipenv, Poetry and uv, and it built the root without saying which manager it spoke for, so every successful pip-inspect graph declared itself from requirements.txt -- and a Pipenv project got its correct Pipfile identity only when it fell back to the pure lock parser. One project, two identities, decided by which strategy happened to succeed. baseDetector carries its Manager now, and filterPythonToolPackages takes it too, because that function looks the root up by ID and would otherwise miss it and silently skip re-parenting orphans. TestPythonRootIdentityAgreesAcrossResolutionStrategies pins the two strategies to one answer. The Python smoke goldens are regenerated, and all five had been stale for the whole migration rather than only the two I claimed: every project root was still a dependency node. They now read module:Pipfile#..., module:pyproject.toml#... for Poetry and uv, and module:requirements.txt#... for pip -- the correct manifests only because of the fix above, which the regeneration is the evidence for. Each inventory loses exactly one package, its own project root, which is what a module node means; no dependency moved. The pre-push hook compared modification times of a hand-written file list covering 541 of 816 tracked files. Editing a shell script, a workflow, an npm wrapper source or a nested testdata fixture left the stamp looking fresh, a deleted file vanished from the list rather than invalidating anything, and a restored mtime read as unchanged -- each reporting a passing verification for work that was never tested, which is the one thing the hook exists to prevent. It compares a digest of HEAD plus the full diff against it now, so git decides what the repository contains rather than a list that has to be kept in step. scripts/verify-snapshot.sh defines it once and `make verify` records it. Exercised against all four previously invisible cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee5b2f91d8
ℹ️ 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".
Both halves of the interactive explain view narrowed to dependency nodes, so the node that answers "which of my modules pulled this in" was dropped -- which is the question explain exists to answer. explainRelationships labelled the union's direct parents but walked only dependency nodes when labelling everything else. A normal graph's root is a module node, and for a transitive target it is not a direct parent, so it fell into that loop and was skipped: no label, no count, and a header reporting "Roots: 0" for a scan that plainly has one. Nested workspace modules were hidden the same way. buildExplainComponentListModel repeated the narrowing, leaving the project's own module out of the component list -- inconsistent with the ordinary component tree beside it, which walks the union and renders whatever kind it finds. Both iterate Graph.Nodes now. The new test drives the transitive case, where the root is an ancestor rather than a parent; restoring either narrowing reproduces "Roots: 0". Interactive rendering only, so no golden moves. Also corrects the verification snapshot added in the previous commit. It folded HEAD into the digest, so `git commit` invalidated a verification that was still perfectly valid -- the content had not changed, only which side of the HEAD boundary it sat on. A gate that fails on every commit is one that teaches people to pass --no-verify, which is worse than no gate. It digests the index plus the worktree diff against it instead: unchanged across a commit, and still changing on any edit or deletion. All three properties exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262043ac06
ℹ️ 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".
Two findings from review of 262043a. Both are ways this migration could return a smaller answer than the input described, without saying so. SBOM ingest dropped a component whose identity could not mint a well-formed package URL, along with every relationship naming it, and returned no error. A document carrying one malformed Maven purl therefore produced a smaller graph than it described, and a scan of it read clean while a genuinely vulnerable dependency was simply absent from the answer. For a tool whose whole output is "what are you shipping and is it vulnerable", quietly returning fewer dependencies than the input listed is the worst available failure. It is an error now, naming the component and the purl so the author can find it -- the same rule ADR-0041 applies at the plugin wire: no lenient path, no pkg:generic coercion for an identity that was asserted and is invalid. Reproduced before fixing; the full smoke suite, SBOM ingest cases included, is unchanged by it. DependenciesFromGraph published a manifest node's ID in depends_on while deliberately omitting manifests from the listing, so a workspace path (module -> child manifest -> child module) left a reference no consumer could resolve. Dropping the ID instead would have severed the workspace, so the hop is stepped through: the parent module depends on the child module, expressed only in IDs the document defines. The walk is bounded against structural cycles. Also corrects the verification snapshot for the second time, and this time against the property rather than a symptom. It read git's bookkeeping -- first HEAD, then the index -- and both invalidated a verification that was still valid, because `git add` and `git commit` move content across those boundaries without changing a byte of it. It digests the worktree tree of everything .gitignore does not exclude, built in a throwaway index, so it changes when a file's bytes change, when one appears, and when one is deleted, and at no other time. All five properties exercised; it costs 0.16s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
bomly-cli/internal/tui/diff.go
Line 917 in 33b9baf
Treat every module as a directness parent
In the merged graph used by the diff TUI, a workspace or reactor module can have an incoming edge and therefore not appear in Roots(). Its immediate package children are never labeled direct here and fall through as transitive, corrupting the relationship summary and relationship-based component filters. Although renderDirectDepsTable now contains fresh module-aware handling for this exact graph shape, this independent classifier remains roots-only; centralize and reuse the module-aware parent rule.
AGENTS.md reference: AGENTS.md:L137-L140
ℹ️ 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".
Three findings from review of 33b9baf, all the same shape: a projection over graph nodes existed once per surface, and the copies had each learned different things. The SBOM export named structural nodes it does not export. A workspace is module -> child manifest -> child module, and the two edges type differently -- the first derives depends-on, the second describes -- so publishing the first left CycloneDX with a dependsOn pointing at no bom-ref, while filtering the second dropped the hop and cut the child module's subtree loose. Reproduced both before fixing. Scan JSON had this same defect fixed one commit earlier, in its own local copy, which is the whole argument. The diff TUI's directness classifier read graph roots only, so a workspace module that another module depends on was never a parent and its immediate packages read transitive -- corrupting the relationship summary and every filter built on it. renderDirectDepsTable had already been taught this; the classifier beside it had not. The raw Relationships view iterated dependency nodes as parents, so it omitted every module-to-package edge: for a project with only direct dependencies it rendered empty while the count beside it reported the edges it was not showing. internal/graphview now owns all three -- the package URL a node publishes, the children a document can name, and which nodes count as top-level parents. It is a leaf, SDK only, so the codec, the renderers and the TUI reach it without depending on each other; that was the obstacle that left two copies of the purl projection behind last time, and this removes it. Every mutation was checked with the tree still compiling, after an earlier attempt "passed" because the mutations broke the build instead of the tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
Author
|
The third finding from that review —
🤖 Addressed by 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
2 participants
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.
Adopts
bomly-dev/bomly-sdkv0.9.0 across the CLI: the dependency graph is a sealed union of manifest, module, and dependency nodes, and a node's identity is a canonical package URL minted at construction (ADR-0041). Also bumps the ninebomly-plugin-*modules to v0.2.0.This is phase 2.1–2.3 of the SDK maturity program. The SDK side is released; this is the consumer.
What changes in the pipeline
The project's own artifacts are module nodes. Every detector that used to mint a dependency node and then mark it —
FirstPartyon coordinates, a forcedworkspace:<dir>ID,DependencySourceWorkspaceas a stand-in — now builds a module node whose ID carries the declaring manifest path. Ownership is the node kind; there is nothing to set afterwards. This covers npm, pnpm, yarn, bun, cargo (both the lock and metadata paths), maven, gradle, and the Python detectors.Duplicate identities fold. The occurrence machinery is gone. Two records that resolved one
name@versionfrom different places become one node whoseOriginslist carries both — a stronger dependency-confusion signal than two nodes with byte-identical identity. The Cargo case (one crate from two git remotes) is the one that made this worth checking rather than assuming; the reasoning is recorded next to the fold, ininternal/detectors/cargo/lock_index.go.Application-typed imports are ordinary dependency nodes, so they diff and match like any other package (ADR-0015).
The stopgaps are gone
An earlier revision of this branch carried three CLI-local helpers with SDK issues filed against them. v0.9.0 shipped all three, so the helpers are deleted and the call sites point at the model:
internal/nodessdk.NodeCoordinates/NodeDisplayName/NodeVersion/AsDependencyNode/DependencyNodesOf/IsProjectOwneddetectors.EnsureNode,PromoteToModule,PropagateScopesdetectorkit.*detectors.NewDependencyFromsdk.NewDependencyNodeFromdetectors.NewDependencyOrGenericsdk.NewDependencyNode(the generic fallback is the constructor's, and records a warning)detectors.RefineOriginssdk.MergeOrigins(which drops a superseded origin itself)internal/testnodeskeeps only what is CLI test ergonomics — fixture builders that panic rather than take atesting.TB, so a table entry stays one expression. Its label lookups delegate tobomly-sdk/testkit, and its copy of the matching rules is gone: two answers to "which node is this" is how they drifted the first time.The two structural guards stay CLI-side, in
internal/detectors/guards_test.go, because they police this tree.TestNodeInsertionGoesThroughTheSharedHelpernow namesdetectorkit.EnsureNodeand exempts nothing — there is no local copy left to reach for.Version case is no longer folded
The SDK used to lowercase every version containing a letter. That corrupted Maven's
1.0-SNAPSHOTinto1.0-snapshot— a different version, Maven versions being case sensitive. v0.9.0 delegates version spelling to packageurl-go, which case-folds exactly one type (huggingface). The Maven, Gradle and reachability goldens show the correction:3.7.0.Final,3.1.1.RELEASE,1.0-SNAPSHOT.It costs an incidental PyPI fold, since PEP 440 does normalize version case and packageurl-go does not implement it. That is bomly-dev/bomly-sdk#39, and
TestPythonVersionCaseIsNotFoldedYetpins the current behaviour so the change is visible rather than silent.Review findings fixed
Fifteen inline findings, each verified against the behaviour before being believed, and each with the assertion that was missing. The ones that mattered most:
executableRootOfnarrowed to*DependencyNode, so a module root — what a normal graph now has — was rejected,inferredPlacementfound no root, and every dependency whose detector leftRelationshipunset degraded to manual review. That is most of a normal scan. The reachability goldens show the fix across three ecosystems:manual-reviewbecomesdirect-bumpandtransitive-override.module:package.json#pkg:npm/app@1.0.0and merged into one node holding both projects' edges. Rebased in consolidation, where the subproject path is already known.name@versionwhile the focused dependency published its canonical node ID.stat -fmeaning "filesystem" and succeeding.Verification
make verify SMOKE=1green: fmt, lint, vet on every build-tag variant,go test ./..., generated-doc drift check, and the full networked smoke suite.GHSA-4mjr-xmp4-gh2g) and a SwiftPM transitive resolving 1.6.0 → 1.7.0.make verifynow compiles the smoke suite. It is behind a build tag, sogo vet ./...never saw it, and this branch pushed a smoke file that did not compile.Smoke coverage
Re-assessed against the feature surface. Two gaps found; one closed here:
mcp/1projection has 49 unit tests, so the gap is the binary-driven path — server startup, tool registration, stdio protocol — which needs a JSON-RPC harness. Left out deliberately rather than rushed in here.Open against the SDK
Not blockers for this PR; filed with reproductions:
PropagateScopesstamps every dependency runtime when the root ID is absent.Phases 2.4–2.8 (document assertions to the codec, strict json/v2 ingest, full scope-set export, helper consolidation, usage attribution) follow as separate PRs.
🤖 Generated with Claude Code