Skip to content

fix(lr-0d45): robust tarball-filename capture in install:local-test#346

Merged
clagentic-merger[bot] merged 1 commit into
mainfrom
fix/lr-0d45-install-local-test-tarball-parse
Jul 15, 2026
Merged

fix(lr-0d45): robust tarball-filename capture in install:local-test#346
clagentic-merger[bot] merged 1 commit into
mainfrom
fix/lr-0d45-install-local-test-tarball-parse

Conversation

@clagentic-builder

Copy link
Copy Markdown
Contributor

Fixes the install:local-test bug that PR #345's on_failure:fail hardening (lr-0d45) caught in production: the old install:local-test script used sh -c with npm pack captured into a shell variable, which captured npm's multi-line human-readable notices instead of just the tarball filename, so npm install -g received a garbage multi-line argument and failed EINVALIDPACKAGENAME.

What changed

  • Replaced the sh -c one-liner in package.json's install:local-test script with scripts/install-local-test.js.
  • The new script shells out to npm pack --json and extracts the tarball filename by scanning every line-leading '[' as a candidate JSON-array start, taking the first candidate that parses as a non-empty array of objects with a filename field. This is more robust than assuming --silent/--json alone yields clean stdout: the prepack lifecycle hook (scripts/write-build-sha.js) writes its own log line to the same stream, and that line itself starts with a literal '[', which broke a naive bracket-position search during verification.
  • Added test/install-local-test-lr-0d45.test.js: 7 unit tests exercising extractTarballFilename against captured/synthetic npm output shapes, without shelling out to a real npm pack.

Verification (acceptance criteria for this task)

Ran the full post-merge chain locally against this branch (based on main after PR #345 merged):

npm run install:local-test succeeded: packed clagentic-console-1.7.0-beta.1.tgz, installed 123 packages, ok: installed.

npm run verify:installed-build passed: ok: installed build matches merged HEAD (a8f21624c0507c6a600fe3e58d1b64f534f68a9e).

verify:installed-build compares the installed build's baked-in SHA (written by the prepack hook at pack time) against git rev-parse HEAD in the working tree. Confirmed this is a real, non-vacuous comparison (not a hardcoded pass) by reading scripts/verify-installed-build.js in full and independently checking that git rev-parse HEAD and the installed /usr/lib/node_modules/@clagentic/console/lib/build-sha.json both resolve to the same SHA, matching this branch's actual HEAD at pack time.

Full test suite: npm test -- 984 pass / 4 fail out of 988. The 4 failures (lr-1f7e context-window-warn-toast tests) are pre-existing and unrelated: reproduced identically via git stash on this branch's HEAD before this commit (same 4 test names, same TAP line numbers, both before and after this fix). This change adds 7 new tests, all passing, and does not change the pass/fail count of any pre-existing test.

Scope note

Daemon restart stays out of scope per task instructions (operator owns restart). This PR only fixes the install-script bug -- no .crew/naomi.yaml or .clagentic/loadout/config.yaml changes (those already landed in PR #345).

Task: lr-0d45

$(npm pack) captured npm's multi-line human notices, not just the
tarball filename, causing npm install -g to fail EINVALIDPACKAGENAME.
This ran during PR #345's post_merge_steps and failed loud, exactly as
the on_failure:fail hardening intended, but the install must actually
succeed.

Replaces the sh -c one-liner with scripts/install-local-test.js, which
shells out to npm pack --json and locates the tarball filename by
scanning line-leading '[' candidates for the first one that parses as
a non-empty array with a "filename" field. This tolerates prepack-hook
stdout (write-build-sha.js's own "[write-build-sha] wrote ..." line,
which itself starts with '[' and previously broke a naive bracket
search) landing ahead of npm's JSON on the same stream.

Verified end-to-end: npm run install:local-test succeeds, and npm run
verify:installed-build passes, confirming the installed build-sha.json
matches the working tree's actual HEAD SHA.
@clagentic-reviewer

Copy link
Copy Markdown

PEACHES — clean (0 findings)

Summary

The install:local-test refactor robustly handles multi-line npm pack output by moving from shell capture to explicit JSON parsing. The parser correctly tolerates the prepack hook's own stdout line (which starts with '[') by scanning all line-leading '[' candidates and validating JSON structure on each.

Checked:

  • amos.code-craft.1 (minimal change): refactor only, no scope creep
  • amos.code-craft.4 (regression tests): 7 test cases cover failure mode, edge cases (missing array, malformed JSON, no filename field, spurious bracket before valid array)
  • amos.code-craft.6 (no bare except): bare catch in JSON.parse loop is intentional (candidate rejection, not suppression)
  • amos.code-craft.10 (no hardcoded secrets/hosts/paths): no hardcoded paths
  • clagentic-console.brand-product-name: no user-facing strings in bin/lib
  • clagentic-console.no-hardcoded-paths: none present
  • clagentic-console.no-new-sdk-direct-require: no SDK imports

Parser robustness:

  • Prepack hook '[write-build-sha]' line correctly skipped via candidate scanning (JSON.parse fails, loop continues)
  • Cleanup in finally block ensures rm -f runs even on npm install failure
  • Cleanup errors logged as warnings, not hard failures (appropriate for utility)
  • execFileSync with argv array (not shell concatenation) — safe subprocess invocation
{"reviewer": "peaches", "review_status": "clean", "head_sha": "d34f3f00dcd609aeddeeb115ec15e1d4ebb40db5", "pr_number": 346}

@clagentic-security

Copy link
Copy Markdown

BOBBIE security audit of PR #346 (lr-0d45) at head d34f3f0.

Scope: base a868099..head d34f3f0, 3 files (+188/-1): package.json, scripts/install-local-test.js (new), test/install-local-test-lr-0d45.test.js (new).

Focus: scripts/install-local-test.js replaces the old sh -c one-liner in package.json with a Node script. Reviewed for command injection, unsafe npm-output parsing, and path traversal in the tarball filename passed to npm install -g.

Findings: none.

  • scripts/install-local-test.js:65,81,84 - all three subprocess calls (npm pack --json, npm install -g , rm -f ) use execFileSync with argv arrays, no shell:true, no string concatenation into a shell command. Removes the shell-metacharacter surface the prior sh -c form had.
  • scripts/install-local-test.js:24-61 - tarball filename is extracted from npm pack --json's own stdout (npm-generated) via JSON.parse of a located array, then used only as an argv element to execFileSync, never concatenated into a shell string or used as a path in a file read/write API. Does not fit the bobbie.sast.5 path-traversal threat model.
  • package.json:16 - script line change removes the shell invocation entirely; no new shell surface introduced.
  • No dependency changes in this PR (package-lock.json untouched); osv-scanner has nothing new to evaluate.

Scanners: gitleaks (0 findings), semgrep p/command-injection + p/security-audit against scripts/install-local-test.js (0 findings), osv-scanner not applicable (no lockfile change in diff).

review.status: clean

{"reviewer": "bobbie", "review_status": "clean", "head_sha": "d34f3f00dcd609aeddeeb115ec15e1d4ebb40db5", "pr_number": 346}

@clagentic-merger clagentic-merger Bot merged commit 31f2dd8 into main Jul 15, 2026
1 check passed
@clagentic-merger

Copy link
Copy Markdown
Contributor

Merged via clagentic-loadout v0.1.0

Field Value
Gated HEAD SHA d34f3f00dcd609aeddeeb115ec15e1d4ebb40db5
Merged SHA d34f3f00dcd609aeddeeb115ec15e1d4ebb40db5
Reviews clagentic-reviewer[bot], clagentic-security[bot]
CI status no-runner-by-design (0 commit-status entries at HEAD)

@clagentic-merger clagentic-merger Bot deleted the fix/lr-0d45-install-local-test-tarball-parse branch July 15, 2026 17:32
@clagentic-release-bot

Copy link
Copy Markdown
Contributor

This issue has been resolved in version 1.7.1-beta.1.

To update, run:

npx @clagentic/console@1.7.1-beta.1

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants