Skip to content

fix(npm): honor codewhaleBinaryVersion in installer (#3769)#3774

Open
Hmbown wants to merge 1 commit into
mainfrom
codex/0866-3769-npm-binary-version
Open

fix(npm): honor codewhaleBinaryVersion in installer (#3769)#3774
Hmbown wants to merge 1 commit into
mainfrom
codex/0866-3769-npm-binary-version

Conversation

@Hmbown

@Hmbown Hmbown commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #3769. npm/codewhale/scripts/install.js was the only wrapper path that skipped pkg.codewhaleBinaryVersion, so install-time asset resolution could disagree with run.js and verify-release-assets.js.

Changes

  • resolvePackageVersion() precedence is now DEEPSEEK_TUI_VERSIONDEEPSEEK_VERSIONpkg.codewhaleBinaryVersionpkg.deepseekBinaryVersionpkg.version, matching the sibling scripts.
  • Function is now (pkgObj = pkg, env = process.env) for injectable testing and exported via _internal.
  • Adds a precedence test.

Validation

  • node --test npm/codewhale/test/install.test.js → 10 passed (incl. new precedence test).
  • Full wrapper suite (node --test test/*.test.js) → 32 passed.

Acceptance criteria

  • install.js honors pkg.codewhaleBinaryVersion before legacy + pkg.version
  • Focused precedence test added
  • Legacy env var behavior unchanged
  • run.js / install.js / verify-release-assets.js consistent
  • npm wrapper tests pass

`install.js` resolved the binary version from legacy env vars and
`deepseekBinaryVersion`, skipping `pkg.codewhaleBinaryVersion`, while
`run.js` and `verify-release-assets.js` already honor it. That let
install-time asset resolution disagree with runtime and release verification
on packaging-only npm releases.

- `resolvePackageVersion` now resolves `pkg.codewhaleBinaryVersion` before
  `deepseekBinaryVersion` and `pkg.version`, matching the sibling scripts.
- Legacy `DEEPSEEK_TUI_VERSION` / `DEEPSEEK_VERSION` precedence is unchanged.
- Parameterized `(pkgObj, env)` for testability and exported it via `_internal`.
- Adds a precedence test covering codewhaleBinaryVersion > deepseekBinaryVersion
  > pkg.version, with env vars still taking top precedence.

Fixes #3769
@Hmbown Hmbown added this to the v0.8.66 milestone Jun 29, 2026
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 1m 9s —— View job


Code Review

  • Read changed files
  • Compare against sibling scripts (run.js, verify-release-assets.js)
  • Analyze test coverage
  • Post review

Summary: Clean, targeted fix. The logic is correct and the test coverage is solid. A few minor observations below — none are blockers.


Correctness ✓

install.js:86–93 — The precedence chain now matches verify-release-assets.js:12–18 exactly:

env.DEEPSEEK_TUI_VERSION || env.DEEPSEEK_VERSION || pkgObj.codewhaleBinaryVersion || pkgObj.deepseekBinaryVersion || pkgObj.version

run() at line 1120 calls resolvePackageVersion() with no arguments, which correctly falls through to the pkg/process.env defaults. No callers need updating.


Minor observations

run.js:10–12printVersionFallback is still env-var-blind

const binVersion =
  pkg.codewhaleBinaryVersion || pkg.deepseekBinaryVersion || pkg.version;

This function doesn't consult DEEPSEEK_TUI_VERSION / DEEPSEEK_VERSION. It's a display-only fallback shown when the binary fails to launch, so a slight inconsistency here is probably harmless in practice. Worth noting in case someone sets DEEPSEEK_TUI_VERSION and sees a different version in --version output vs. install messages. Not introduced by this PR, but it's the one remaining inconsistency now that the install path is aligned.

Block comment at install.js:81–85

Five lines is on the verbose side given the codebase's comment style. The test already serves as executable documentation for the precedence contract. Not a blocking concern.

Test coverage

All five precedence levels are covered. One combination not explicitly tested: DEEPSEEK_TUI_VERSION winning over a present DEEPSEEK_VERSION simultaneously. This is trivially implied by JS || semantics, so it's not a gap worth blocking on.


Verdict

Approve. The fix is correct, the injectable-args pattern makes the function properly testable, and the precedence test documents the full contract. The run.js:printVersionFallback inconsistency pre-exists this PR and is a separate follow-up if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v0.8.66: Honor codewhaleBinaryVersion in npm installer

1 participant