Repository navigation
Conversation
Reviewer's GuideReworks Bun dependency analysis into a lockfile-only implementation backed by the shared graph contract, with robust tuple/version/workspace resolution and actionable failure behavior, while adding broad regression and golden-SBOM coverage for the corrected reader defects. Sequence diagram for lockfile-only Bun dependency analysissequenceDiagram
participant Caller
participant Javascript_bun
participant Filesystem
participant BunLockGraph
participant SBOM
Caller->>Javascript_bun: _buildDependencyTree(includeTransitive, opts)
Javascript_bun->>Filesystem: _findLockFileDir(manifestDir, opts)
alt text bun.lock found
Javascript_bun->>BunLockGraph: readBunLock(bun.lock)
BunLockGraph-->>Javascript_bun: parsed lock
Javascript_bun->>BunLockGraph: readBunGraph(lockDir, manifestPath, manifest, lock)
BunLockGraph-->>Javascript_bun: {rootId, nodes}
Javascript_bun->>Javascript_bun: _loadHashes(lockDir)
Javascript_bun-->>SBOM: dependency graph
else only bun.lockb found
Javascript_bun->>Filesystem: #binaryLockFileDir(manifestDir)
Filesystem-->>Javascript_bun: bun.lockb directory
Javascript_bun-->>Caller: migrate diagnostic
else no lockfile
Javascript_bun-->>Caller: requires bun.lock error
end
Flow diagram for Bun lockfile graph resolutionflowchart TD
A[Read bun.lock as JSONC] --> B{lockfileVersion 0, 1, or 2?}
B -- No --> E[Fail with unsupported-version error]
B -- Yes --> C[Validate workspace manifests]
C --> D[Resolve dependency edge]
D --> F{Workspace pointer?}
F -- Yes --> G[Redirect to ws member node]
F -- No --> H[Resolve parent/child package key]
H --> I{Required resolution missing?}
I -- Yes --> J[Fail with stale-resolution error]
I -- No --> K[Create package node with identity and hashes]
G --> L[Visit dependencies]
K --> L
L --> M{Optional resolution missing?}
M -- Yes --> N[Preserve graph without optional edge]
M -- No --> D
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #680 +/- ##
==========================================
- Coverage 92.02% 91.38% -0.64%
==========================================
Files 43 43
Lines 11284 11494 +210
Branches 2258 2313 +55
==========================================
+ Hits 10384 10504 +120
- Misses 900 990 +90
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/providers/javascript_bun.js" line_range="48" />
<code_context>
- const name = id.slice(0, at);
- const version = id.slice(at + 1);
- const hash = sriToHash(entry[3]);
+ const { name, version, integrity } = parseBunEntry(entry);
+ const hash = integrity ? sriToHash(integrity) : null;
if (name && version && hash) {
</code_context>
<issue_to_address>
**Valid dependency hashes disappear**
When a lockfile contains a package entry with a string descriptor that makes `parseBunEntry` throw, even though that package is not reachable from the analyzed manifest, `parseBunEntry` throws while `_parseLockFileHashes` scans a malformed, otherwise-unreachable package entry. `Base_javascript._loadHashes` catches the error and replaces the entire hash map with an empty one, so `provideComponent` omits hashes even for valid direct dependencies.
Handle malformed hash entries individually, or validate the entire lockfile explicitly before hash extraction instead of silently clearing all hashes.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: src/providers/javascript_bun.js:48
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai review — Classified as code change request — sub-task TC-7069 created to address this feedback. This is the same "valid dependency hashes disappear" finding as the inline comment at |
Verification Report for TC-7014 (commit c53efa0)
Overall: WARNThe PR is functionally solid — CI green, all acceptance criteria implemented, lint clean, 130 bun tests passing, additive test coverage, no secrets. One confirmed defect needs a follow-up before merge: a single malformed or unreachable This comment was AI-generated by sdlc-workflow/verify-pr v0.13.10. |
Strum355
left a comment
Strum355
left a comment
There was a problem hiding this comment.
Some statements mainly on the test manifests:
- Can you double check whether the
lockfile_v0is actually a v0 lockfile? From what I saw, its actually a v1/v2 but just with the version set to 0. Im wondering whether Claude generated these manifests by hand instead of usingbun - Im not sure we should really be adding the v0, v1 & v2 test fixtures. The formats are, for the most part, pretty identical, and these 3 tests themselves dont really add anything of use to our test suite. Id rather avoid adding a mountain of tautological tests when we can be re-using existing test manifests where its reasonable enough to do so (or not adding extra tests for something at all)
417b537 to
3e48976
Compare
ruromero
left a comment
ruromero
left a comment
There was a problem hiding this comment.
The lockfile-only rewrite looks solid overall. I checked the stale-lockfile detection against scratch projects: a changed range, a dependency added only to package.json, and a locked version outside the declared range all fail with "Lockfile is out of date", and an in-sync project produces the SBOM. Findings below, most important first; details are in the inline comments.
1. The bun.lockb migrate error is not reachable through the public API
match() (src/provider.js:74) picks the provider through validateLockFile → _findLockFileDir, which only looks for bun.lock. For a project with only bun.lockb, no provider is selected and the user gets the generic package.json requires a lock file. Use your preferred package manager to generate the lock file. The new diagnostic only fires when the Bun provider is called directly, which is what the test does. All public entry points go through match() (src/index.js:185, :200, :306).
Reproduced on this branch with a directory containing only package.json + bun.lockb:
match(path.join(dir, 'package.json'), availableProviders)
// -> package.json requires a lock file. Use your preferred package manager to generate the lock file.
new Javascript_bun().provideStack(path.join(dir, 'package.json'))
// -> Bun analysis found a binary 'bun.lockb' but no text 'bun.lock'. Migrate it with ...So TC-7014's "Binary bun.lockb is detected and rejected with an actionable 'migrate to bun.lock' error" isn't met for real callers.
2. CONTRIBUTING.md / CONVENTIONS.md gaps
- DCO sign-off missing. None of the 5 commits has a
Signed-off-by:trailer. CONTRIBUTING.md: "Commits must be signed-off"; CONVENTIONS.md: "DCO (Developer Certificate of Origin) required".git rebase --signoff upstream/mainfixes it. - Max line length 100. 24 added lines under
src/exceed it (tabs counted as 4), e.g.javascript_lockfile.js:826,:919and the error string atjavascript_bun.js:82. ESLint doesn't enforce this, so it's easy to miss. SBOM_CASESpattern. CONVENTIONS.md: "Tests must use theSBOM_CASESpattern". The Bun golden-file suite builds its own inline list instead.
3. Other implementation issues
- The
bun.lockberror suggestsbun install --save-text-lockfile, but the README saysbun install --save-text-lockfile --frozen-lockfile --lockfile-only. The error's version installsnode_modulesand can change resolved versions; the two should match, ideally on the README command. #binaryLockFileDirignoresTRUSTIFY_DA_WORKSPACE_DIR, unlike_findLockFileDir. With that option pointing at a directory that has onlybun.lockb, even a direct provider call returns the generic "Bun analysis requires a lockfile".
4. Duplication
canonical()/snapshot()intest/providers/javascript.test.jsare verbatim copies of the helpers intest/javascript_lockfile.test.js.- The Bun lockfile tests live in
test/providers/javascript.test.js, while npm/pnpm/yarn lockfile coverage runs throughMANAGERSintest/javascript_lockfile.test.js. Adding Bun to that suite would also coverSBOM_CASESand remove the copied helpers. readBunGraphrepeats the "resolve child, skip if absent, record, push" block three times (and mirrorsreadPnpmGraph); thepath.relative(...).split(path.sep).join('/') || '.'expression now appears five times in the file.#binaryLockFileDirre-implements the directory walk in_findLockFileDir.
| const lockDir = this._findLockFileDir(manifestDir, opts); | ||
| if (!lockDir) { | ||
| if (this.#binaryLockFileDir(manifestDir)) { | ||
| throw new Error("Bun analysis found a binary 'bun.lockb' but no text 'bun.lock'. Migrate it with 'bun install --save-text-lockfile' (Bun >= 1.2) before scanning."); |
There was a problem hiding this comment.
This branch is only reachable when the Bun provider is called directly. Through match() (used by every public entry point in src/index.js), a bun.lockb-only project never selects the Bun provider, because validateLockFile → _findLockFileDir only looks for bun.lock. The user gets the generic "package.json requires a lock file" message instead of this one.
One option is to detect bun.lockb in the match/validateLockFile path and raise this diagnostic there.
Also, the suggested command differs from the README (bun install --save-text-lockfile --frozen-lockfile --lockfile-only). Without --frozen-lockfile --lockfile-only this installs node_modules and may re-resolve versions. This line is also over the 100-char limit from CONVENTIONS.md.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-7199 created to address this feedback (bun.lockb diagnostic reachable via match(), honor TRUSTIFY_DA_WORKSPACE_DIR, align the migrate command with the README, wrap the >100-char line).
| * @returns {string|null} The directory containing bun.lockb, or null | ||
| * @private | ||
| */ | ||
| #binaryLockFileDir(manifestDir) { |
There was a problem hiding this comment.
This duplicates the walk in Base_javascript._findLockFileDir but skips its TRUSTIFY_DA_WORKSPACE_DIR branch. With that option pointing at a directory that only has bun.lockb, the user gets "Bun analysis requires a lockfile" rather than the migrate diagnostic. Could _findLockFileDir take the file name(s) to look for, so both lookups share one walk and honour the option the same way?
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-7199 created. The #binaryLockFileDir walk and its missing TRUSTIFY_DA_WORKSPACE_DIR handling are covered there (sharing one walk with _findLockFileDir is the suggested approach).
| const devSpec = local.devDependencies[name]; | ||
| // Bun may lock either range when it retains both declarations for the same name. | ||
| const effective = typeof devSpec === 'string' && semver.validRange(spec) && semver.validRange(devSpec) ? `${spec} || ${devSpec}` : spec; | ||
| const cid = childId(local.packageKey, name, effective, local.file, Object.hasOwn(optionalDeps, name)); |
There was a problem hiding this comment.
This "resolve child → skip if absent → node.dependencies.set → children.push" block appears three times in readBunGraph (here, :940, :947) and mirrors readPnpmGraph (:731-734). A small addEdge(node, children, name, cid) helper would remove the repetition. The line is also over 100 chars.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request (upgraded from suggestion) — the >100-char portion matches project convention: CONVENTIONS.md §Code Style "Max line length: 100" (confirmed >100 with tabs=4 on :912/:918/:919/:926). Sub-task TC-7201 created for the line-wrapping. The optional addEdge() DRY refactor remains a non-blocking suggestion.
| } | ||
| const required = dependencyMap(parsed.info, 'dependencies', `${file} for ${id}`); | ||
| const optionalDeps = dependencyMap(parsed.info, 'optionalDependencies', `${file} for ${id}`); | ||
| // ponytail: transitive peerDependencies are omitted (as the prior |
There was a problem hiding this comment.
ponytail: isn't a tag used anywhere else in the repo. Please use plain wording or a TODO. Since TC-7014 is about avoiding silent omissions, it may also be worth tracking transitive peer resolution in an issue rather than only in this comment.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as nit — minor wording of an in-code comment; does not affect correctness. The suggestion to track transitive peer resolution in an issue is noted. No sub-task created.
| const BUN_SRI_HEX = Buffer.from(BUN_SRI.slice('sha512-'.length), 'base64').toString('hex'); | ||
|
|
||
| /** Sorts components and dependency edges so two SBOMs compare order-independently. */ | ||
| function canonical(sbom) { |
There was a problem hiding this comment.
canonical() and snapshot() are verbatim copies of the helpers in test/javascript_lockfile.test.js (:21, :29). If the Bun lockfile cases moved into that suite (alongside MANAGERS), these copies could go.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — helper duplication is not covered by a documented convention, so no standalone sub-task. It is noted as related cleanup in TC-7200 (removing the copies naturally follows if the Bun cases move into the SBOM_CASES/MANAGERS suite). No sub-task created.
| }); | ||
|
|
||
| /// Verifies a binary bun.lockb (without a text bun.lock) yields an actionable migrate diagnostic. | ||
| test('rejects a binary bun.lockb with a migrate-to-bun.lock diagnostic', () => { |
There was a problem hiding this comment.
This passes because it calls the Bun provider directly. Going through match() / stackAnalysis with the same fixture returns the generic "package.json requires a lock file" error, so the test doesn't cover the path users hit. Please exercise it through match() once the diagnostic is raised there.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-7199 created. It requires the bun.lockb diagnostic to be exercised through match()/stackAnalysis (the path real users hit), not only a direct provider call.
| const base = 'test/providers/tst_manifests/bun'; | ||
| const fixtures = ['alias', 'nested', 'optional', 'workspace']; | ||
| for (const fixture of fixtures) { | ||
| for (const { method, type } of [{ method: 'provideStack', type: 'stack' }, { method: 'provideComponent', type: 'component' }]) { |
There was a problem hiding this comment.
CONVENTIONS.md asks golden-file tests to use the SBOM_CASES pattern. This re-declares the stack/component list inline. Reusing SBOM_CASES (or adding Bun to the existing reference-comparison suite in test/javascript_lockfile.test.js) would keep it consistent.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request (upgraded from suggestion) — this matches project convention: CONVENTIONS.md §Testing Conventions requires "Tests must use the SBOM_CASES pattern to do a full deep.equal comparison", and SBOM_CASES is used in 10+ places across the suite. Sub-task TC-7200 created to address this feedback.
|
[sdlc-workflow/verify-pr] Re: @Strum355 review — two points, classified separately:
|
|
[sdlc-workflow/verify-pr] Re: @ruromero review — thanks for the thorough reproduction. Classified as code change request (multiple items), tracked across sub-tasks:
|
Verification Report for TC-7014 (commit 72a7cf7)
Overall: FAILAC #6 is not satisfied for real callers: the actionable This comment was AI-generated by sdlc-workflow/verify-pr v0.13.10. |
Make Bun dependency analysis read bun.lock directly, with no `bun --version`
or `bun install` invocation, and require a pre-existing text lockfile. Rewrite
the tree-builder into the shared `{rootId, nodes}` graph contract
(`readBunGraph`/`parseBunEntry`/`readBunLock` in javascript_lockfile.js) and
fix the prior reader's correctness defects:
- aliases report their npm-target identity instead of the alias name
- nested resolutions are keyed by their `parent/child` install path, so
conflicting versions stay distinct instead of collapsing onto the root
- workspace members report their real manifest identity and dependencies
- optional branches are preserved; missing required resolutions and
unsupported lockfileVersions now fail closed instead of being dropped
Reject a binary `bun.lockb` without a text `bun.lock` with an actionable
migrate diagnostic. Supports lockfileVersion 0, 1 and 2, hosted/git 3-tuples,
empty-registry entries, and SHA-1/SHA-512 integrity. Adds golden-SBOM and
regression fixtures plus their dependabot suppression entries.
Implements TC-7014
Assisted-by: Claude Code
Signed-off-by: Adva Oren <aoren@redhat.com>
Signed-off-by: Adva Oren <aoren@redhat.com>
Signed-off-by: Adva Oren <aoren@redhat.com>
The lockfile_v0/v1/v2 fixtures were byte-identical except the lockfileVersion field (and the v0 one was not a genuine v0 lockfile, just the v1/v2 shape with the number set to 0). The version gate is already covered by the inline supports/rejects lockfileVersion unit tests, so these full-SBOM comparisons added no coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Adva Oren <aoren@redhat.com>
_parseLockFileHashes iterated every bun.lock packages entry and let parseBunEntry throw on an id without '@'. Base_javascript._loadHashes caught that single throw and cleared the whole hash map, so provideComponent omitted integrity hashes even for valid direct dependencies while provideStack (which only parses reachable entries) kept them. Skip the malformed/unreachable entry per-entry so valid hashes survive and both SBOMs agree. Implements TC-7069 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Assisted-by: Claude Code Signed-off-by: Adva Oren <aoren@redhat.com>
A bun.lockb-only project failed match() with the generic "requires a lock file" error because Bun only advertised bun.lock, so the migrate diagnostic was reachable only on direct provider calls. Let the base validateLockFile also match a provider-declared binary lockfile and have Bun declare bun.lockb, so match()/stackAnalysis/componentAnalysis select Bun and raise the actionable diagnostic. Generalize _findLockFileDir to accept lockfile names, replacing Bun's duplicated #binaryLockFileDir walk so the diagnostic honors TRUSTIFY_DA_WORKSPACE_DIR. Align the suggested command with the README. Implements TC-7199 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Assisted-by: Claude Code Signed-off-by: Adva Oren <aoren@redhat.com>
Move the bun lockfile-only and golden-SBOM suites into
test/javascript_lockfile.test.js so they reuse that file's shared
SBOM_CASES, canonical() and snapshot() helpers instead of duplicating
them in test/providers/javascript.test.js. The bun golden reference suite
now iterates SBOM_CASES rather than an inline {method,type} list, matching
CONVENTIONS.md Testing Conventions and the other provider suites.
Implements TC-7200
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assisted-by: Claude Code
Signed-off-by: Adva Oren <aoren@redhat.com>
Wrap lines added under src/ that exceeded the documented 100-column limit (CONVENTIONS.md Code Style). Pure formatting plus semantics-preserving helper extractions; no behavior change. Implements TC-7201 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Assisted-by: Claude Code Signed-off-by: Adva Oren <aoren@redhat.com>
Summary
Makes Bun dependency analysis lockfile-only: it reads
bun.lockdirectly with nobun --versionorbun installinvocation and requires a pre-existing text lockfile. The tree-builder is rewritten onto the shared{rootId, nodes}graph contract (readBunGraph/parseBunEntry/readBunLock), fixing the prior reader's correctness defects:Aliases report their npm-target identity (
pkg:npm/is-number@…), not the alias name.Nested resolutions are keyed by their
parent/childinstall path, so conflicting versions stay distinct instead of collapsing onto the root.Workspace members report their real manifest identity and dependencies (no more
workspace:pseudo-version).Optional branches are preserved; missing required resolutions and unsupported
lockfileVersionnow fail closed with actionable errors instead of being silently dropped.A binary
bun.lockbwithout a textbun.lockis rejected with a migrate diagnostic.TC-7199 The
bun.lockbmigrate diagnostic is now reachable throughmatch()/stackAnalysis/componentAnalysis(not only direct provider calls): the basevalidateLockFilematches a provider-declared binary lockfile so Bun is selected and raises the diagnostic instead of the generic "requires a lock file" error._findLockFileDiris generalized to accept lockfile names (replacing Bun's duplicated walk) so the diagnostic honorsTRUSTIFY_DA_WORKSPACE_DIR, and the suggested command matches the README.TC-7069 A malformed/unreachable
bun.lockpackagesentry (an id without@) is now skipped per-entry in_parseLockFileHashesinstead of throwing and clearing the entire hash map, soprovideComponentkeeps valid direct-dependency integrity hashes and agrees withprovideStack.[TC-7200] The bun golden-SBOM and lockfile-only test suites moved into
test/javascript_lockfile.test.jsto reuse that file's sharedSBOM_CASES,canonical()andsnapshot()helpers instead of duplicating them intest/providers/javascript.test.js; the bun golden reference suite now iteratesSBOM_CASESrather than an inline{method,type}list, matching CONVENTIONS.md Testing Conventions.[TC-7201] Wrapped the ~23 lines added under
src/that exceeded the documented 100-column limit (CONVENTIONS.md Code Style; ESLint does not enforce it) and added DCOSigned-off-by:trailers to every commit on the branch. Formatting/compliance only, no behavior change.Supports
lockfileVersion0/1/2, hosted/git 3-tuples, empty-registry entries, and SHA-1/SHA-512 integrity.Reuse / design notes
{rootId, nodes}contract +addGraphToSbom/graphRootDependenciesand the npm/yarn-era helpers (projectManifests,validateSnapshot,validateRange,descriptor,stale,unsupported,hashes) already injavascript_lockfile.js.bun.locktuple shapes were grounded against real Bun 1.4.0 output (npm, alias, nested conflict, optional, git) before implementing.peerDependenciesare intentionally omitted (as the prior reader did); direct peers are still supplemented by the SBOM builder.Tests
deep.equalfixtures for alias / nested / workspace / optional / lockfileVersion 0/1/2.bun.lockbmigrate diagnostic.leaf@1.0.0plus an unreachable malformed entry keeps leaf's SHA-512 hash in the component SBOM and agrees with the stack SBOM.npm run lint(0 errors) andnpm test(full suite + coverage) pass.Implements TC-7014
Also implements TC-7069
🤖 Generated with Claude Code
Summary by Sourcery
Make Bun dependency analysis lockfile-only and produce validated, accurate dependency graphs from text bun.lock files.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: