From d34f3f00dcd609aeddeeb115ec15e1d4ebb40db5 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" <290147524+clagentic-builder[bot]@users.noreply.github.com> Date: Wed, 15 Jul 2026 13:25:30 -0400 Subject: [PATCH] lr-0d45: fix install:local-test tarball-filename capture bug $(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. --- package.json | 2 +- scripts/install-local-test.js | 97 +++++++++++++++++++++++++ test/install-local-test-lr-0d45.test.js | 90 +++++++++++++++++++++++ 3 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 scripts/install-local-test.js create mode 100644 test/install-local-test-lr-0d45.test.js diff --git a/package.json b/package.json index a54b3d92..e32ed115 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "postinstall": "node scripts/postinstall.js", "prepack": "node scripts/write-build-sha.js", "test": "node --test --test-force-exit test/*.test.js", - "install:local-test": "sh -c 'f=$(npm pack) && npm install -g \"$f\" && rm -f \"$f\"'", + "install:local-test": "node scripts/install-local-test.js", "verify:installed-build": "node scripts/verify-installed-build.js", "semantic-release": "semantic-release" }, diff --git a/scripts/install-local-test.js b/scripts/install-local-test.js new file mode 100644 index 00000000..25beb973 --- /dev/null +++ b/scripts/install-local-test.js @@ -0,0 +1,97 @@ +#!/usr/bin/env node +'use strict'; + +// install-local-test.js — pack the current tree and install it globally (lr-0d45). +// +// Replaces the old `sh -c 'f=$(npm pack) && npm install -g "$f" && rm -f "$f"'` +// one-liner. That form broke because `$(npm pack)` captures npm's multi-line +// human-readable notices AND any output written to stdout by the `prepack` +// lifecycle hook (scripts/write-build-sha.js's own console.log) — not just the +// tarball filename. The resulting `$f` was a garbage multi-line string, so +// `npm install -g "$f"` failed with EINVALIDPACKAGENAME. +// +// `npm pack --json` also mixes prepack-hook stdout into the same stream, so +// this reads npm's output and extracts the tarball filename by parsing the +// trailing JSON array specifically (the only valid JSON substring `npm pack +// --json` ever emits), rather than trusting the whole stdout to be clean. + +const { execFileSync } = require('child_process'); + +// Extracts the packed tarball filename from `npm pack --json`'s raw stdout. +// Exported (not just used internally) so tests can exercise the parsing +// logic against captured/synthetic npm output without shelling out to a +// real `npm pack` — see test/install-local-test-lr-0d45.test.js. +function extractTarballFilename(raw) { + // npm pack --json emits a JSON array `[ { ... } ]` (pretty-printed, + // multi-line, in practice), but the prepack lifecycle hook + // (scripts/write-build-sha.js) writes its own log line to the same stdout + // stream first, and that line itself starts with a literal '[' (its + // "[write-build-sha] wrote ..." prefix) — so a naive raw.indexOf('[') finds + // that '[' instead of the JSON array's. Rather than assume a specific + // whitespace shape around the array's opening bracket, try every '[' that + // starts a line as a candidate array start and parse from there to the end + // of the string, taking the first candidate that parses as a non-empty + // array of objects with a "filename" field. This tolerates both + // pretty-printed and single-line npm output across npm versions. + const lines = raw.split('\n'); + let offset = 0; + const candidates = []; + for (const line of lines) { + if (line.startsWith('[')) { + candidates.push(offset); + } + offset += line.length + 1; + } + + for (const start of candidates) { + let parsed; + try { + parsed = JSON.parse(raw.slice(start)); + } catch { + continue; + } + if (Array.isArray(parsed) && parsed.length > 0 && typeof parsed[0].filename === 'string') { + // npm pack --json's "filename" field uses the @scope/name@version form + // with a literal '/' in place of the tarball's on-disk '-' separator; + // the actual file on disk uses '-'. Normalize to get the real path. + return parsed[0].filename.replace('/', '-'); + } + } + + throw new Error(`could not locate a valid \`npm pack --json\` array in output:\n${raw}`); +} + +function packAndGetFilename() { + const raw = execFileSync('npm', ['pack', '--json'], { encoding: 'utf8' }); + return extractTarballFilename(raw); +} + +function main() { + let tarball; + try { + tarball = packAndGetFilename(); + } catch (err) { + console.error(`[install-local-test] ERROR: ${err.message}`); + process.exit(1); + } + + console.log(`[install-local-test] packed ${tarball}`); + + try { + execFileSync('npm', ['install', '-g', tarball], { stdio: 'inherit' }); + } finally { + try { + execFileSync('rm', ['-f', tarball]); + } catch (cleanupErr) { + console.error(`[install-local-test] WARN: could not remove ${tarball}: ${cleanupErr.message}`); + } + } + + console.log('[install-local-test] ok: installed'); +} + +module.exports = { extractTarballFilename }; + +if (require.main === module) { + main(); +} diff --git a/test/install-local-test-lr-0d45.test.js b/test/install-local-test-lr-0d45.test.js new file mode 100644 index 00000000..e324cf45 --- /dev/null +++ b/test/install-local-test-lr-0d45.test.js @@ -0,0 +1,90 @@ +// Regression test for lr-0d45 — the install:local-test npm script used to be +// `sh -c 'f=$(npm pack) && npm install -g "$f" && rm -f "$f"'`. $(npm pack) +// captures npm's multi-line human-readable notices, so `$f` was a garbage +// multi-line string and `npm install -g "$f"` failed EINVALIDPACKAGENAME. +// This surfaced for real when NAOMI's post-merge hardening (this same task) +// started running install:local-test with on_failure: fail instead of warn. +// +// The fix moved the logic into scripts/install-local-test.js, which shells +// out to `npm pack --json` and parses the tarball filename out of npm's +// output. `npm pack --json`'s own stdout is ALSO not clean — the prepack +// lifecycle hook (scripts/write-build-sha.js) writes a log line ahead of the +// JSON array, and that log line itself starts with a literal '[' (its +// "[write-build-sha] wrote ..." prefix), which broke a naive +// raw.indexOf('[') scan. These tests exercise extractTarballFilename against +// captured/synthetic npm output shapes directly, without shelling out to a +// real npm pack, so a regression in the parsing logic fails here instead of +// only being caught by an interactive install run. The parser itself scans +// every line-leading '[' as a candidate array start and takes the first one +// that parses as a non-empty array of objects with a "filename" field — +// tolerant of both pretty-printed and single-line npm output. + +"use strict"; + +var test = require("node:test"); +var assert = require("node:assert/strict"); + +var { extractTarballFilename } = require("../scripts/install-local-test"); + +test("lr-0d45: extracts filename from clean npm pack --json output", function () { + var raw = JSON.stringify([{ filename: "@clagentic/console-1.7.0-beta.1.tgz" }]); + assert.equal(extractTarballFilename(raw), "@clagentic-console-1.7.0-beta.1.tgz"); +}); + +test("lr-0d45: extracts filename when prepack hook stdout precedes the JSON array", function () { + // Reproduces the real failure mode: write-build-sha.js's own console.log + // line lands on stdout before npm's JSON array, and that line's own "[" + // prefix must not be mistaken for the array's opening bracket. + var raw = + "[write-build-sha] wrote /repo/lib/build-sha.json (sha=abc123)\n" + + JSON.stringify([{ filename: "@clagentic/console-1.7.0-beta.1.tgz" }], null, 2) + + "\n"; + assert.equal(extractTarballFilename(raw), "@clagentic-console-1.7.0-beta.1.tgz"); +}); + +test("lr-0d45: extracts filename when npm warnings precede the JSON array", function () { + var raw = + "npm warn deprecated something@1.0.0: use something-else instead\n" + + JSON.stringify([{ filename: "@clagentic/console-1.7.0-beta.1.tgz" }], null, 2) + + "\n"; + assert.equal(extractTarballFilename(raw), "@clagentic-console-1.7.0-beta.1.tgz"); +}); + +test("lr-0d45: throws (does not silently return garbage) when no JSON array is present", function () { + // This is the shape of the ORIGINAL bug: npm's human-readable notice text + // with no machine-parseable filename anywhere in it. The old `$(npm pack)` + // form would have handed this whole string to `npm install -g` as a single + // corrupt argument; the fix must fail loudly instead. + var raw = + "npm notice \n" + + "npm notice package: @clagentic/console@1.7.0-beta.1\n" + + "npm notice Tarball Contents\n"; + assert.throws(function () { + extractTarballFilename(raw); + }, /could not locate a valid `npm pack --json` array/); +}); + +test("lr-0d45: throws on malformed JSON that merely starts with '['", function () { + var raw = "[not actually json}"; + assert.throws(function () { + extractTarballFilename(raw); + }, /could not locate a valid `npm pack --json` array/); +}); + +test("lr-0d45: throws when the only JSON array present has no filename field", function () { + var raw = JSON.stringify([{ name: "@clagentic/console" }]); + assert.throws(function () { + extractTarballFilename(raw); + }, /could not locate a valid `npm pack --json` array/); +}); + +test("lr-0d45: skips a non-array/no-filename '[' candidate and finds the real array later in the stream", function () { + // Regression guard for the line-scan approach itself: a spurious '['-led + // line earlier in the stream (e.g. from an unrelated hook or warning) must + // not cause a false failure when a valid array follows it. + var raw = + "[irrelevant-tool] some other message\n" + + JSON.stringify([{ filename: "@clagentic/console-1.7.0-beta.1.tgz" }], null, 2) + + "\n"; + assert.equal(extractTarballFilename(raw), "@clagentic-console-1.7.0-beta.1.tgz"); +});