[#663] Fix CLI build failure (corrupted lockfile)#664
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
project7-interns
left a comment
There was a problem hiding this comment.
T2b APPROVE
Lockfile-only change — correct fix for corrupted transitive dependency resolution. Reviewed:
- Single file changed — only
package-lock.json, no source changes. Clean. - Root cause is sound — tsup's transitive deps missing from lockfile explains the build failure.
- Version bump 0.1.11→0.1.12 in lockfile reflects current
package.jsonstate; no concern. - Dependency version bumps are expected from a clean
npm installpicking up latest semver-compatible versions.
Minor note: T3 correctly flagged the ESM vs CJS discrepancy — acceptance criteria says dist/index.cjs but tsup config targets ESM. Worth updating the issue/docs if this matters for distribution.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
This does make the missing tsup runtime entries appear in the lockfile, but it does so by regenerating the entire monorepo lockfile and pulling in unrelated dependency updates outside the CLI build scope.
Findings
- [medium] The fix is broader than the ticket and introduces unrelated dependency drift across the repo.
- File:
package-lock.json:330 - Suggestion: make the build fix targeted to
packages/cliby declaring the missing tsup runtime deps or pinning/adjusting the tsup dependency, then regenerate the lockfile without refreshing unrelated trees. Right now this PR also upgrades non-CLI packages such aswagmi/@wagmi/connectors(package-lock.json:13483,package-lock.json:13508) and a large set of@aws-sdk/*entries, which creates avoidable regression risk for a CLI build-only ticket.
- File:
Decision
Requesting changes because the implementation is not minimal or focused enough for #663. The build fix itself may be valid, but it should land without a repo-wide lockfile refresh.
npm workspaces failed to resolve 9 of tsup's transitive dependencies (bundle-require, chokidar, consola, etc.) because the lockfile had no entries for them. Adding them as explicit devDependencies in packages/cli ensures they are always installed. Fixes #663 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
067ee31 to
e9a76c6
Compare
project7-interns
left a comment
There was a problem hiding this comment.
T2b APPROVE (re-review after force-push)
Much cleaner — scoped to CLI workspace only. +323/-66 vs the original +1113/-939.
What changed:
packages/cli/package.json: 9 missing tsup transitive deps added as explicit devDependencies ✅package-lock.json: corresponding lockfile entries, no unrelated dependency drift ✅
One note (non-blocking): The lockfile diff removes a stale @plotlink/sdk workspace symlink and its dependency entry from the CLI package. This appears correct — CLI's package.json doesn't list @plotlink/sdk as a dependency, so the old lockfile had a stale reference. The SDK workspace is also now marked extraneous: true. Worth confirming the CLI doesn't import from @plotlink/sdk at runtime, but this looks like lockfile cleanup, not a regression.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The revised PR narrows the fix to the CLI workspace and adds the missing tsup runtime dependencies explicitly in packages/cli/package.json, with corresponding scoped lockfile updates.
Findings
- None blocking.
Decision
Approving because the prior scope-creep concern is resolved and the change now matches #663's intended build/dependency fix.
Summary
package-lock.jsonwhich was missing resolution entries for tsup's transitive dependencies (bundle-require,joycon,tree-kill, etc.)packages/cli/node_modules/but its 17 transitive dependencies had no entries in the lockfile, so npm never installed themrm -rf node_modules package-lock.json && npm install) to regenerate a correct lockfileVerification
Note: acceptance criteria mentions
dist/index.cjsbut the build is configured for ESM format (tsup.config.ts: format: ["esm"]), so the output isdist/index.js.Test plan
cd packages/cli && npm run buildsucceedsnode dist/index.js --helpdisplays all 5 commandspackage-lock.jsonFixes #663
🤖 Generated with Claude Code