From bb9cf2ba2ca22dfc37dcb655be575c007c877226 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 26 Jul 2026 23:57:41 +0300 Subject: [PATCH 1/6] fix(smoke): validate remote mktemp -d before using it as an rm -rf target scripts/smoke-machine-sync-release.mjs set remoteDir from an unchecked remote `mktemp -d` and later ran `ssh rm -rf ${shellQuote(remoteDir)}`. `mktemp -d` prints its error on stderr and nothing on stdout, so a failure left remoteDir empty. That is inert today only because shellQuote renders '' as '' -- one appended `/*` at the cleanup site turns it into the 2026-07-24 incident, where an empty command substitution made `rm -rf "$(cmd)"/*` run as `rm -rf /*` and destroyed a machine's checkouts. The check belongs at the assignment, not at the delete, and it must not lean on quoting. New scripts/lib/remote-temp-dir.mjs exports assertRemoteTempDir, which accepts only what the template could actually have produced: `mktemp` replaces the trailing X run in place, so the result must be the template's fixed prefix plus a single non-empty path component. Checking only the template's parent directory is not enough -- it would accept /tmp/somebody-elses-dir and delete it. The bound is derived from the template rather than hardcoded, so it stays correct if the template moves. Applied at the assignment and re-asserted immediately before the `rm -rf`. Packaging: smoke-machine-sync-release.mjs is a published file, so the new module is registered in all three deliberate allowlists (package.json files, scripts/validate-public-package.mjs, tests/package-release.test.ts). Without that the published script would fail with ERR_MODULE_NOT_FOUND for every installed copy. Also adds a test asserting that every relative import in a published script is itself published. The existing allowlists only caught the opposite mistake (shipping something unreviewed). This one immediately found pre-existing breakage: scripts/apply-cloud-migrations.mjs:23 imports ../src/storage.ts, but src/ has never been in `files`, so the published script already throws ERR_MODULE_NOT_FOUND. Verified against a packed tarball. That path carries a single exemption named in the test and tracked as todos task 1eb481d5; removing the exemption is that task's acceptance criterion. It is not fixed here because choosing between repointing at dist, shipping src, or unpublishing the script is a separate decision. Evidence: bun test tests/machine-smoke-script.test.ts tests/package-release.test.ts -> 14 pass, 0 fail node scripts/validate-public-package.mjs -> passed; public scripts included: 7 npm pack + extract + `bun scripts/smoke-machine-sync-release.mjs --help` -> resolves and runs from the tarball full suite (HASNA_KNOWLEDGE_API_* unset) -> 227 pass, 2 skip, 1 fail; that one failure reproduces on the untouched baseline via git stash, so it is pre-existing and unrelated. gitleaks staged scan -> no leaks found Negative controls, proving the guards fail on bad input rather than only passing on good: assertRemoteTempDir rejects '', whitespace-only, multi-line output, relative paths, /, /tmp, /usr, /home/hasna, a sibling dir sharing the temp parent, a path escaping via .., and a malformed template. Removing the new module from the allowlist makes the packaging test fail with the exact missing path. No `rm` is executed by any test. --- package.json | 1 + scripts/lib/remote-temp-dir.mjs | 56 +++++++++++++++++++++ scripts/smoke-machine-sync-release.mjs | 12 ++++- scripts/validate-public-package.mjs | 1 + tests/machine-smoke-script.test.ts | 67 ++++++++++++++++++++++++++ tests/package-release.test.ts | 32 +++++++++++- 6 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 scripts/lib/remote-temp-dir.mjs diff --git a/package.json b/package.json index 1378ba8..97b4025 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "bin", "dist", "scripts/apply-cloud-migrations.mjs", + "scripts/lib/remote-temp-dir.mjs", "scripts/smoke-machine-sync-release.mjs", "scripts/smoke-machines-adapter.mjs", "scripts/smoke-open-files-installed-boundary.mjs", diff --git a/scripts/lib/remote-temp-dir.mjs b/scripts/lib/remote-temp-dir.mjs new file mode 100644 index 0000000..4ab18e8 --- /dev/null +++ b/scripts/lib/remote-temp-dir.mjs @@ -0,0 +1,56 @@ +/** + * Validate a path that will later be handed to `rm -rf` on a remote host. + * + * `mktemp -d ` returns the created directory on stdout and prints nothing + * there on failure. Consuming that output unchecked means an `rm -rf` whose target is + * whatever the remote shell happened to produce. `shellQuote('')` renders as `''` and is + * inert today, but the value is one appended `/*` away from the 2026-07-24 incident in which + * an empty command substitution turned `rm -rf "$(cmd)"/*` into `rm -rf /*` and destroyed a + * machine's checkouts. So the check belongs at the assignment, not at the delete, and it + * does not lean on quoting. + * + * Accepts only what this exact template could have produced: `mktemp` replaces the trailing + * run of `X` in place, so the result is the template's fixed prefix followed by a single + * non-empty path component. Checking against the template's parent directory alone is not + * enough - it would accept any unrelated directory that happens to sit beside the temp dir. + * The bound is derived from the template, so it stays correct if the template moves. + * + * @param {string} remote Remote host, used in the error message only. + * @param {unknown} dir Trimmed stdout of the remote `mktemp -d`. + * @param {string} template Absolute template passed to `mktemp -d`, ending in `XXX...`. + * @returns {string} The validated directory. + * @throws {Error} If the value is anything other than a directory that template could create. + */ +export function assertRemoteTempDir(remote, dir, template) { + const fail = (why) => { + throw new Error( + `Refusing to use remote temp dir from ${remote}: ${why}. ` + + `Template ${template}, got ${JSON.stringify(dir)}. ` + + 'This value is used as an `rm -rf` target on the remote host.' + ); + }; + + if (typeof template !== 'string' || !template.startsWith('/')) { + throw new Error(`Remote temp dir template must be an absolute path, got ${JSON.stringify(template)}.`); + } + // POSIX mktemp requires at least three trailing X in the final component. + const match = template.match(/^(.*[^X])(X{3,})$/); + if (!match) { + throw new Error(`Remote temp dir template must end in at least three X, got ${JSON.stringify(template)}.`); + } + const [, prefix] = match; + if (prefix.includes('/') === false) { + throw new Error(`Remote temp dir template must have a directory component, got ${JSON.stringify(template)}.`); + } + + if (typeof dir !== 'string' || dir === '') fail('mktemp -d produced no path'); + if (/[\r\n]/.test(dir)) fail('path spans multiple lines'); + if (!dir.startsWith('/')) fail('path is not absolute'); + if (!dir.startsWith(prefix)) fail(`path does not start with the template prefix ${prefix}`); + + const suffix = dir.slice(prefix.length); + if (suffix === '') fail('path is the bare template prefix, so mktemp created nothing'); + if (suffix.includes('/')) fail('path descends below the directory the template would create'); + + return dir; +} diff --git a/scripts/smoke-machine-sync-release.mjs b/scripts/smoke-machine-sync-release.mjs index 0dda0b2..f190432 100644 --- a/scripts/smoke-machine-sync-release.mjs +++ b/scripts/smoke-machine-sync-release.mjs @@ -5,6 +5,7 @@ import { chmodSync, cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rm import { tmpdir } from 'node:os'; import { dirname, join, relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { assertRemoteTempDir } from './lib/remote-temp-dir.mjs'; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const packageJson = JSON.parse(readFileSync(join(repoRoot, 'package.json'), 'utf8')); @@ -122,6 +123,11 @@ function runRemote(remote, command, options = {}) { return runChecked('ssh', [remote, command], options); } +function createRemoteTempDir(remote, template) { + const dir = runRemote(remote, `mktemp -d ${shellQuote(template)}`).trim(); + return assertRemoteTempDir(remote, dir, template); +} + function parseJsonOutput(label, raw) { try { return JSON.parse(raw); @@ -411,7 +417,8 @@ function syncMachineArgs(options, remoteDir, runOptions = {}) { function runSyncSmoke(options, runOptions = {}) { const localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`)); - const remoteDir = runRemote(options.remote, `mktemp -d ${shellQuote(`/tmp/knowledge-linux-node-a-${options.knowledgeVersion}-XXXXXX`)}`).trim(); + const remoteTemplate = `/tmp/knowledge-linux-node-a-${options.knowledgeVersion}-XXXXXX`; + const remoteDir = createRemoteTempDir(options.remote, remoteTemplate); const localCommandOptions = runOptions.localCommandOptions ?? {}; const learnCommandOptions = runOptions.learnCommandOptions ?? {}; try { @@ -576,6 +583,9 @@ function runSyncSmoke(options, runOptions = {}) { } finally { if (!options.keepTemp) { rmSync(localDir, { recursive: true, force: true }); + // Re-assert immediately before the delete: the value must still be the temp dir this + // run created, whatever happened in between. + assertRemoteTempDir(options.remote, remoteDir, remoteTemplate); run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]); } } diff --git a/scripts/validate-public-package.mjs b/scripts/validate-public-package.mjs index 0d2e1e5..bd46e86 100644 --- a/scripts/validate-public-package.mjs +++ b/scripts/validate-public-package.mjs @@ -20,6 +20,7 @@ const publicDocs = [ const publicScripts = [ 'scripts/apply-cloud-migrations.mjs', + 'scripts/lib/remote-temp-dir.mjs', 'scripts/smoke-machine-sync-release.mjs', 'scripts/smoke-machines-adapter.mjs', 'scripts/smoke-open-files-installed-boundary.mjs', diff --git a/tests/machine-smoke-script.test.ts b/tests/machine-smoke-script.test.ts index 4d721eb..57c9881 100644 --- a/tests/machine-smoke-script.test.ts +++ b/tests/machine-smoke-script.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import { join, resolve } from 'node:path'; +import { assertRemoteTempDir } from '../scripts/lib/remote-temp-dir.mjs'; const repoRoot = resolve(import.meta.dir, '..'); const script = join(repoRoot, 'scripts', 'smoke-machine-sync-release.mjs'); @@ -102,3 +103,69 @@ describe('installed open-files boundary smoke script', () => { expect(output.checks).toContain('scan source and peer knowledge SQLite/artifacts for raw sentinel and base64'); }); }); + +/** + * `runSyncSmoke` sends `rm -rf ` over ssh, where remoteDir came from a remote + * `mktemp -d`. Consuming that output unchecked is the same class of defect as the 2026-07-24 + * incident, where an empty `$(bun pm cache)` turned `rm -rf "$(cmd)"/*` into `rm -rf /*`. + * These cases prove the guard rejects bad input rather than merely accepting good input. + * No `rm` runs here: only the validator is exercised. + */ +describe('remote temp dir guard', () => { + const remote = 'linux-node-a'; + const template = '/tmp/knowledge-linux-node-a-0.0.0-test-XXXXXX'; + + test('accepts what a successful mktemp -d actually returns', () => { + expect(assertRemoteTempDir(remote, '/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9', template)) + .toBe('/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9'); + }); + + test('rejects the empty output a failing mktemp -d leaves behind', () => { + // This is the live defect: `mktemp -d` prints its error on stderr and nothing on stdout, + // so the unchecked assignment yielded '' and the cleanup ran `rm -rf ''`. + expect(() => assertRemoteTempDir(remote, '', template)).toThrow(/produced no path/); + expect(() => assertRemoteTempDir(remote, ' '.trim(), template)).toThrow(/produced no path/); + }); + + test('rejects paths that are not a temp dir this template could have created', () => { + for (const bad of [ + '/', + '/tmp', + '/usr', + '/home/hasna', + '/tmp/somebody-elses-dir', + 'knowledge-linux-node-a-0.0.0-test-Ab3De9', + './relative', + '/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9/../../etc', + ]) { + expect(() => assertRemoteTempDir(remote, bad, template)).toThrow(); + } + }); + + test('rejects multi-line output, so only the first line is never silently used', () => { + expect(() => assertRemoteTempDir( + remote, + '/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9\nmktemp: failed', + template, + )).toThrow(/multiple lines/); + }); + + test('the failure names the path and says why it matters', () => { + expect(() => assertRemoteTempDir(remote, '', template)) + .toThrow(/used as an `rm -rf` target on the remote host/); + }); + + test('rejects a sibling directory that merely shares the temp parent', () => { + // Checking only the parent directory would accept this, and `rm -rf` would then delete + // somebody else's directory. + expect(() => assertRemoteTempDir(remote, '/tmp/somebody-elses-dir', template)) + .toThrow(/does not start with the template prefix/); + }); + + test('rejects a malformed template rather than deriving a bogus bound from it', () => { + expect(() => assertRemoteTempDir(remote, '/tmp/x', 'relative-XXXXXX')) + .toThrow(/must be an absolute path/); + expect(() => assertRemoteTempDir(remote, '/tmp/x', '/tmp/no-placeholder')) + .toThrow(/must end in at least three X/); + }); +}); diff --git a/tests/package-release.test.ts b/tests/package-release.test.ts index 3b0df20..5be1cd5 100644 --- a/tests/package-release.test.ts +++ b/tests/package-release.test.ts @@ -6,7 +6,7 @@ import { describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; -import { dirname, join } from 'node:path'; +import { dirname, join, posix } from 'node:path'; import { fileURLToPath } from 'node:url'; const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); @@ -27,6 +27,9 @@ const publicDocs = [ const publicScripts = [ 'scripts/apply-cloud-migrations.mjs', + // Imported by scripts/smoke-machine-sync-release.mjs, so it must ship with it or the + // published script fails to resolve at runtime. + 'scripts/lib/remote-temp-dir.mjs', 'scripts/smoke-machine-sync-release.mjs', 'scripts/smoke-machines-adapter.mjs', 'scripts/smoke-open-files-installed-boundary.mjs', @@ -61,6 +64,33 @@ describe('public package release safety', () => { } }); + /** + * A published script that imports a file left out of `files` resolves fine in the repo and + * throws ERR_MODULE_NOT_FOUND for everyone who installs the package. The allowlists above + * only catch the opposite mistake (shipping something unreviewed), so this closes the gap. + */ + test('every relative import in a published script is itself published', () => { + const importPattern = /(?:from\s*|\bimport\s*\(\s*)['"](\.[^'"]+)['"]/g; + + // Pre-existing breakage, tracked as todos task 1eb481d5: this script imports + // ../src/storage.ts but src/ has never been in `files`, so the published copy already + // fails with ERR_MODULE_NOT_FOUND. Exempted by name rather than by weakening the rule, + // and removing this entry is that task's acceptance criterion. + const knownUnresolvedImports = new Set([ + 'scripts/apply-cloud-migrations.mjs -> ../src/storage.ts', + ]); + + for (const script of publicScripts) { + const source = readFileSync(join(repoRoot, script), 'utf8'); + for (const [, specifier] of source.matchAll(importPattern)) { + if (knownUnresolvedImports.has(`${script} -> ${specifier}`)) continue; + const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); + expect(publicScripts, `${script} imports ${specifier}, which is not in the published files list`) + .toContain(resolved); + } + } + }); + test('npm pack dry-run includes only public docs', () => { const result = spawnSync('node', ['scripts/validate-public-package.mjs', '--json'], { cwd: repoRoot, From 0902c4bf75a00feff7cc3b86faa626bfae61723f Mon Sep 17 00:00:00 2001 From: andreihasna Date: Mon, 27 Jul 2026 00:28:28 +0300 Subject: [PATCH 2/6] fix(smoke): close review findings in the remote temp dir guard Adversarial review found the guard silently degraded into the parent-directory-only check this module exists to avoid, and that the defect narrative described an unreachable code path. Both fixed. F1 (high): when a template's final component is nothing but X, the derived prefix is a bare directory and every sibling under it validated. `/tmp/XXXXXX` accepted `/tmp/somebody-elses-dir`, `/XXXXXX` accepted `/etc` and `/home`, and `/home/hasna/XXXXXX` accepted `/home/hasna/.ssh` -- all reproduced. `/tmp/XXXXXX` is an ordinary mktemp template. The guard that should have caught this, `prefix.includes('/') === false`, could never fire, because '/tmp/'.includes('/') is true; mutation testing confirmed that line was both ineffective and untested. Templates without a fixed prefix in their final component are now refused. F2 (medium): the stated defect was not reachable. runRemote goes through runChecked, which throws on a non-zero remote exit, so a FAILING `mktemp -d` (exit 1, empty stdout) aborts the run and never assigns "". Verified against GNU coreutils 9.4. The reachable hole is the remote exiting ZERO with stdout that is not the path -- an ssh Banner, a chatty login profile, or a ForceCommand wrapper. Verified: `echo banner; mktemp -d ...` exits 0 and yields a two-line value the guard rejects. The claim appeared in four places (commit, PR body, module docstring, test comment); the docstring and test comment are corrected here and the PR body separately. An incident-remediation artifact that misstates the defect it fixes corrupts the incident record, which is worse than the bug. F3 (medium): the bound is derived from a template built from a caller-supplied version string, and templates containing `..` passed validation with a meaningless bound. Templates must now be normalized. F4 (medium): the re-assertion before the `rm -rf` sits in a `finally`, where a throw REPLACES any in-flight exception from the try block and skips the cleanup -- masking a real smoke failure and guaranteeing a remote leak. It is now contained: it reports and skips the delete, and can never mask. Its actual purpose (catching a future refactor that makes the value reassignable) is stated plainly. F5 (medium): the new "published imports must be published" test only scanned relative specifiers, so it missed that scripts/apply-cloud-migrations.mjs has a SECOND unresolvable import -- `@hasna/contracts/auth`, a devDependency only. Task 1eb481d5 would have closed on a false green with the published script still throwing. The test now checks bare specifiers against runtime dependencies too, both are exempted by name, and both must be removed for that task to be done. F6 (low-medium): the import regex missed side-effect imports (`import './x.mjs'`) and fired inside comments. Comments and template literals are now stripped, and side-effect imports are matched. Runtime builtins (node:, bun:) are excluded -- found by the new check flagging bun:sqlite. F7/F8 (low): the GNU suffix form is documented as deliberately refused; the local temp dir can no longer leak on the guard's throw path, because the remote dir is now created first. Evidence: 18 pass, 0 fail on the touched tests (was 14). The three mutations that SURVIVED the reviewer's mutation testing (parent-only bound, bare prefix accepted, absolute check dropped) are now all CAUGHT, as is dropping the new normalization check. Package validator passes, packed tarball still resolves and runs. Full suite 231 pass / 1 fail, that failure being the pre-existing dated time bomb tracked as d1b93867. No rm executed by any test. --- scripts/lib/remote-temp-dir.mjs | 49 ++++++++++++++++----- scripts/smoke-machine-sync-release.mjs | 25 ++++++++--- tests/machine-smoke-script.test.ts | 52 +++++++++++++++++++++-- tests/package-release.test.ts | 59 ++++++++++++++++++++------ 4 files changed, 152 insertions(+), 33 deletions(-) diff --git a/scripts/lib/remote-temp-dir.mjs b/scripts/lib/remote-temp-dir.mjs index 4ab18e8..7eaf1a0 100644 --- a/scripts/lib/remote-temp-dir.mjs +++ b/scripts/lib/remote-temp-dir.mjs @@ -1,19 +1,30 @@ +import { posix } from 'node:path'; + /** * Validate a path that will later be handed to `rm -rf` on a remote host. * - * `mktemp -d ` returns the created directory on stdout and prints nothing - * there on failure. Consuming that output unchecked means an `rm -rf` whose target is - * whatever the remote shell happened to produce. `shellQuote('')` renders as `''` and is - * inert today, but the value is one appended `/*` away from the 2026-07-24 incident in which - * an empty command substitution turned `rm -rf "$(cmd)"/*` into `rm -rf /*` and destroyed a - * machine's checkouts. So the check belongs at the assignment, not at the delete, and it - * does not lean on quoting. + * What this actually defends against - stated precisely, because an earlier version of this + * comment described an unreachable code path: + * + * `runRemote` goes through `runChecked`, which throws on a non-zero remote exit. A *failing* + * `mktemp -d` exits 1, so it aborts the run and never yields an empty string. The reachable + * hole is the remote exiting **zero** with stdout that is not the path: an ssh `Banner`, a + * chatty login profile, or a `ForceCommand` wrapper prepends lines, and a wrapper can mask + * the real exit status entirely. Whatever lands in that variable is then interpolated into + * `--peer-workspace` and into `ssh rm -rf `. + * + * That is the same class as the 2026-07-24 incident - a delete target taken on faith from a + * command's output - which is why the check belongs at the assignment rather than relying on + * `shellQuote` downstream to neutralise a value that should never have been accepted. * * Accepts only what this exact template could have produced: `mktemp` replaces the trailing * run of `X` in place, so the result is the template's fixed prefix followed by a single * non-empty path component. Checking against the template's parent directory alone is not - * enough - it would accept any unrelated directory that happens to sit beside the temp dir. - * The bound is derived from the template, so it stays correct if the template moves. + * enough - it would accept any unrelated directory sitting beside the temp dir - so a + * template whose final component is nothing but `X` (`/tmp/XXXXXX`) is refused outright: + * its prefix is a bare directory, which would silently degrade this into exactly that + * parent-only check. The bound is derived from the template, so it stays correct if the + * template moves. * * @param {string} remote Remote host, used in the error message only. * @param {unknown} dir Trimmed stdout of the remote `mktemp -d`. @@ -33,18 +44,34 @@ export function assertRemoteTempDir(remote, dir, template) { if (typeof template !== 'string' || !template.startsWith('/')) { throw new Error(`Remote temp dir template must be an absolute path, got ${JSON.stringify(template)}.`); } + // A template containing `.` or `..` makes the derived bound meaningless, and the template + // is built from a caller-supplied version string. Require it to be already normalized + // rather than normalizing silently, so a surprising template is loud instead of accepted. + if (posix.normalize(template) !== template) { + throw new Error( + `Remote temp dir template must be normalized (no "." or ".." segments), got ${JSON.stringify(template)}.` + ); + } // POSIX mktemp requires at least three trailing X in the final component. const match = template.match(/^(.*[^X])(X{3,})$/); if (!match) { throw new Error(`Remote temp dir template must end in at least three X, got ${JSON.stringify(template)}.`); } const [, prefix] = match; - if (prefix.includes('/') === false) { - throw new Error(`Remote temp dir template must have a directory component, got ${JSON.stringify(template)}.`); + // Without a fixed part in the final component, `prefix` is a bare directory and every + // sibling under it would validate - the parent-only check this function exists to avoid. + if (prefix.endsWith('/')) { + throw new Error( + 'Remote temp dir template must have a fixed prefix in its final component ' + + `(e.g. /tmp/knowledge-XXXXXX, not /tmp/XXXXXX), got ${JSON.stringify(template)}.` + ); } if (typeof dir !== 'string' || dir === '') fail('mktemp -d produced no path'); if (/[\r\n]/.test(dir)) fail('path spans multiple lines'); + // Redundant while the template is absolute and normalized (prefix then starts with `/`), + // kept as a cheap invariant so a future change to the template rules cannot quietly admit + // a relative delete target. if (!dir.startsWith('/')) fail('path is not absolute'); if (!dir.startsWith(prefix)) fail(`path does not start with the template prefix ${prefix}`); diff --git a/scripts/smoke-machine-sync-release.mjs b/scripts/smoke-machine-sync-release.mjs index f190432..f5dab10 100644 --- a/scripts/smoke-machine-sync-release.mjs +++ b/scripts/smoke-machine-sync-release.mjs @@ -416,9 +416,11 @@ function syncMachineArgs(options, remoteDir, runOptions = {}) { } function runSyncSmoke(options, runOptions = {}) { - const localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`)); + // The remote dir is created first because it is the step that can reject: doing it before + // mkdtempSync means a refused temp dir cannot leak a local one. const remoteTemplate = `/tmp/knowledge-linux-node-a-${options.knowledgeVersion}-XXXXXX`; const remoteDir = createRemoteTempDir(options.remote, remoteTemplate); + const localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`)); const localCommandOptions = runOptions.localCommandOptions ?? {}; const learnCommandOptions = runOptions.learnCommandOptions ?? {}; try { @@ -583,10 +585,23 @@ function runSyncSmoke(options, runOptions = {}) { } finally { if (!options.keepTemp) { rmSync(localDir, { recursive: true, force: true }); - // Re-assert immediately before the delete: the value must still be the temp dir this - // run created, whatever happened in between. - assertRemoteTempDir(options.remote, remoteDir, remoteTemplate); - run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]); + // Belt-and-braces re-check before the delete. `remoteDir`/`remoteTemplate` are const and + // the validator is pure, so this cannot currently fail - it exists so a future refactor + // that makes the value reassignable is caught here rather than at the `rm -rf`. + // A throw inside `finally` would REPLACE any in-flight exception from the try block and + // skip the cleanup below, so it is contained: report and skip the delete, never mask. + let remoteDirStillValid = true; + try { + assertRemoteTempDir(options.remote, remoteDir, remoteTemplate); + } catch (error) { + remoteDirStillValid = false; + process.stderr.write( + `[smoke] refusing remote cleanup, temp dir no longer validates: ${error instanceof Error ? error.message : String(error)}\n` + ); + } + if (remoteDirStillValid) { + run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]); + } } } } diff --git a/tests/machine-smoke-script.test.ts b/tests/machine-smoke-script.test.ts index 57c9881..4d3563d 100644 --- a/tests/machine-smoke-script.test.ts +++ b/tests/machine-smoke-script.test.ts @@ -106,8 +106,17 @@ describe('installed open-files boundary smoke script', () => { /** * `runSyncSmoke` sends `rm -rf ` over ssh, where remoteDir came from a remote - * `mktemp -d`. Consuming that output unchecked is the same class of defect as the 2026-07-24 - * incident, where an empty `$(bun pm cache)` turned `rm -rf "$(cmd)"/*` into `rm -rf /*`. + * `mktemp -d`. Taking a delete target on faith from a command's output is the same class of + * defect as the 2026-07-24 incident, where an empty `$(bun pm cache)` turned + * `rm -rf "$(cmd)"/*` into `rm -rf /*`. + * + * Precisely which case is live: `runRemote` goes through `runChecked`, which throws on a + * non-zero remote exit, so a *failing* `mktemp -d` (exit 1) aborts the run and never assigns + * an empty string. The reachable hole is the remote exiting **zero** with stdout that is not + * the path - an ssh `Banner`, a chatty login profile, or a `ForceCommand` wrapper prepending + * lines or masking the status. Verified: `echo banner; mktemp -d ...` exits 0 and yields + * `"Welcome to linux-node-a\n/tmp/knowledge-banner-XXXXXX"`, which the guard rejects. + * * These cases prove the guard rejects bad input rather than merely accepting good input. * No `rm` runs here: only the validator is exercised. */ @@ -121,8 +130,9 @@ describe('remote temp dir guard', () => { }); test('rejects the empty output a failing mktemp -d leaves behind', () => { - // This is the live defect: `mktemp -d` prints its error on stderr and nothing on stdout, - // so the unchecked assignment yielded '' and the cleanup ran `rm -rf ''`. + // Defence in depth rather than the live defect: runChecked aborts on a non-zero remote + // exit, so an empty value cannot reach here via a failing mktemp. It can via a wrapper + // that exits 0 without printing a path. expect(() => assertRemoteTempDir(remote, '', template)).toThrow(/produced no path/); expect(() => assertRemoteTempDir(remote, ' '.trim(), template)).toThrow(/produced no path/); }); @@ -168,4 +178,38 @@ describe('remote temp dir guard', () => { expect(() => assertRemoteTempDir(remote, '/tmp/x', '/tmp/no-placeholder')) .toThrow(/must end in at least three X/); }); + + /** + * Regression for the review finding that this guard silently degraded into the + * parent-directory-only check it exists to avoid. With a template like `/tmp/XXXXXX` the + * derived prefix is a bare directory, so every sibling under it validated: + * `/tmp/XXXXXX` accepted `/tmp/somebody-elses-dir`, and `/XXXXXX` accepted `/etc`. + */ + test('refuses a template whose final component is only X, which would accept any sibling', () => { + for (const badTemplate of ['/tmp/XXXXXX', '/XXXXXX', '/home/hasna/XXXXXXXX']) { + expect(() => assertRemoteTempDir(remote, '/tmp/anything', badTemplate)) + .toThrow(/must have a fixed prefix in its final component/); + } + }); + + test('refuses a template containing . or .., which makes the derived bound meaningless', () => { + // The template is built from a caller-supplied version string. + expect(() => assertRemoteTempDir( + remote, + '/tmp/knowledge-x/../../../home/hasna-AbCdEf', + '/tmp/knowledge-x/../../../home/hasna-XXXXXX', + )).toThrow(/must be normalized/); + expect(() => assertRemoteTempDir(remote, '/tmp/k-AbCdEf', '/tmp/./k-XXXXXX')) + .toThrow(/must be normalized/); + }); + + test('rejects the bare template prefix, which means mktemp created nothing', () => { + expect(() => assertRemoteTempDir(remote, '/tmp/knowledge-linux-node-a-0.0.0-test-', template)) + .toThrow(/mktemp created nothing/); + }); + + test('rejects a relative path even though the template bound implies absoluteness', () => { + expect(() => assertRemoteTempDir(remote, 'tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9', template)) + .toThrow(/not absolute/); + }); }); diff --git a/tests/package-release.test.ts b/tests/package-release.test.ts index 5be1cd5..8f21980 100644 --- a/tests/package-release.test.ts +++ b/tests/package-release.test.ts @@ -65,28 +65,61 @@ describe('public package release safety', () => { }); /** - * A published script that imports a file left out of `files` resolves fine in the repo and - * throws ERR_MODULE_NOT_FOUND for everyone who installs the package. The allowlists above + * A published script that imports something the package does not ship resolves fine in the + * repo and throws ERR_MODULE_NOT_FOUND for everyone who installs it. The allowlists above * only catch the opposite mistake (shipping something unreviewed), so this closes the gap. + * + * Both directions are checked, because a published script can fail to resolve two ways: + * a relative import of a file outside `files`, or a bare import of a package that is not a + * runtime dependency. Checking only relative imports would let this test go green while the + * packed script is still broken. */ - test('every relative import in a published script is itself published', () => { - const importPattern = /(?:from\s*|\bimport\s*\(\s*)['"](\.[^'"]+)['"]/g; + test('every import in a published script resolves from the published package', () => { + // Strip comments and template literals first: a commented-out import must not fail the + // test, and a dynamic import with an interpolated path cannot be resolved statically. + const strip = (source: string) => source + .replace(/\/\*[\s\S]*?\*\//g, ' ') + .replace(/(^|[^:])\/\/[^\n]*/g, '$1 ') + .replace(/`(?:[^`\\]|\\.)*`/g, '``'); - // Pre-existing breakage, tracked as todos task 1eb481d5: this script imports - // ../src/storage.ts but src/ has never been in `files`, so the published copy already - // fails with ERR_MODULE_NOT_FOUND. Exempted by name rather than by weakening the rule, - // and removing this entry is that task's acceptance criterion. + // `from '...'`, bare `import '...'` (side-effect), and `import('...')`. + const specifierPattern = + /(?:\bfrom\s*|\bimport\s*\(\s*|\bimport\s+(?=['"]))['"]([^'"]+)['"]/g; + + const runtimeDeps = new Set(Object.keys( + (packageJson as unknown as { dependencies?: Record }).dependencies ?? {}, + )); + + // Pre-existing breakage, tracked as todos task 1eb481d5. This script has TWO unresolvable + // imports, not one: `../src/storage.ts` (src/ is not in `files`) and `@hasna/contracts/auth` + // (a devDependency only). Both are exempted by name so the rule is not weakened, and + // BOTH must be removed for that task to be genuinely done -- deleting only the first + // would leave the published script still throwing. const knownUnresolvedImports = new Set([ 'scripts/apply-cloud-migrations.mjs -> ../src/storage.ts', + 'scripts/apply-cloud-migrations.mjs -> @hasna/contracts/auth', ]); for (const script of publicScripts) { - const source = readFileSync(join(repoRoot, script), 'utf8'); - for (const [, specifier] of source.matchAll(importPattern)) { + const source = strip(readFileSync(join(repoRoot, script), 'utf8')); + for (const [, specifier] of source.matchAll(specifierPattern)) { if (knownUnresolvedImports.has(`${script} -> ${specifier}`)) continue; - const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); - expect(publicScripts, `${script} imports ${specifier}, which is not in the published files list`) - .toContain(resolved); + // Runtime builtins ship with the runtime, not with the package. + if (specifier.startsWith('node:') || specifier.startsWith('bun:')) continue; + + if (specifier.startsWith('.')) { + const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); + expect(publicScripts, `${script} imports ${specifier}, which is not in the published files list`) + .toContain(resolved); + continue; + } + + // Bare specifier: must be a runtime dependency, not a devDependency. + const pkg = specifier.startsWith('@') + ? specifier.split('/').slice(0, 2).join('/') + : specifier.split('/')[0]; + expect(runtimeDeps, `${script} imports ${specifier}, which is not a runtime dependency`) + .toContain(pkg); } } }); From a15850f34e441e8efd38a8c60edff8fe6fc68c22 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Mon, 27 Jul 2026 00:54:53 +0300 Subject: [PATCH 3/6] fix(smoke): close re-review findings, including a new one in the guard-rail test itself Re-review returned APPROVE-WITH-FIXES and found that round 2 introduced a defect in the same class it was closing. N1 (new, mine): the import test's comment stripper was regex-based, so `/\*[\s\S]*?\*/` treated the `/*` inside an ordinary glob string like '**/*.ts' as a comment opener and deleted everything up to the next `*/`. One innocuous glob line before a broken import made the check go green -- precisely the passes-while-broken failure that test exists to catch. Reproduced, then replaced with a character scanner that tracks string and template literals, so a `/*` or `//` inside a string is no longer a comment. Verified by planting `const g = '**/*.ts'` + a broken import: now fails with the exact missing specifier. N9: the bare-specifier check false-positived on legitimate forms. Builtins imported without the `node:` prefix, the package's own name (valid via `exports`), and `#`-prefixed `imports` subpaths are now allowed. The self-reference case was live, not hypothetical: scripts/smoke-machines-adapter.mjs already imports '@hasna/knowledge' and was passing only by accident of the old template-literal strip. N3a: the docstring claimed the guard accepts "only what this exact template could have produced", which was false -- any suffix of any length and charset was accepted. mktemp replaces the X run with exactly that many [A-Za-z0-9] characters, so that is now enforced and the claim is true. Rejects one-char, over-long, `$(id)`, `..` and space-containing suffixes. N3b: a comment presented a paraphrase as a verbatim transcript -- `echo banner` cannot print "Welcome to linux-node-a", and mktemp never prints the unexpanded XXXXXX. Replaced with the actual measured output. This is the same defect F2 was raised for, in text rewritten to fix F2. N3c: "interpolated into --peer-workspace" was loose; that site passes a separate argv element. The real shell interpolations are the remote `cd && ...` and `rm -rf `. Corrected. N4/N5/N6: the finally block is now fully contained. Round 2 wrapped only the assertion -- the statement that cannot currently fail -- while leaving rmSync and the ssh spawn able to throw, replace the in-flight exception from the try block, and skip the remaining cleanup. All three are individually contained now, and a non-zero remote cleanup exit is reported instead of discarded. Both temp dirs are created inside the try with null guards in finally, so whichever one exists is cleaned up; round 2's reordering had only moved the leak from local to remote. N7/N8: added the missing fixtures -- the banner-first shape the docstring calls the live case, the `X{3,}` POSIX boundary, and suffix shape. Bare `.toThrow()` assertions now pin a message pattern. Evidence: mutation retest of all 10 checks -- every one CAUGHT, including M8 (`X{3,}` -> `X{1,}`) which survived the reviewer's run, and M5 which my own charset check had subsumed until a message-ordering test pinned it. Touched-file tests 22 pass / 0 fail (was 18). Validator passes, packed tarball resolves and runs. Full suite 235 pass / 2 skip / 1 fail, that failure being the pre-existing dated time bomb (task d1b93867). CI note: PR #33 is red with TWO failures, not one. Both reproduce on main's own CI for the same base commit, and `git diff origin/main...HEAD -- src/ tests/cli.test.ts` is empty, so neither is caused by this PR. The macOS-only redaction failure is recorded on d1b93867 alongside the time bomb. --- scripts/lib/remote-temp-dir.mjs | 14 +++-- scripts/smoke-machine-sync-release.mjs | 71 ++++++++++++++++++-------- tests/machine-smoke-script.test.ts | 50 ++++++++++++++++-- tests/package-release.test.ts | 62 ++++++++++++++++++---- 4 files changed, 162 insertions(+), 35 deletions(-) diff --git a/scripts/lib/remote-temp-dir.mjs b/scripts/lib/remote-temp-dir.mjs index 7eaf1a0..5c638ec 100644 --- a/scripts/lib/remote-temp-dir.mjs +++ b/scripts/lib/remote-temp-dir.mjs @@ -10,8 +10,9 @@ import { posix } from 'node:path'; * `mktemp -d` exits 1, so it aborts the run and never yields an empty string. The reachable * hole is the remote exiting **zero** with stdout that is not the path: an ssh `Banner`, a * chatty login profile, or a `ForceCommand` wrapper prepends lines, and a wrapper can mask - * the real exit status entirely. Whatever lands in that variable is then interpolated into - * `--peer-workspace` and into `ssh rm -rf `. + * the real exit status entirely. Whatever lands in that variable is then passed as a + * `--peer-workspace` argv element and, more importantly, interpolated into the remote shell + * strings `cd && knowledge ...` and `rm -rf `. * * That is the same class as the 2026-07-24 incident - a delete target taken on faith from a * command's output - which is why the check belongs at the assignment rather than relying on @@ -57,7 +58,7 @@ export function assertRemoteTempDir(remote, dir, template) { if (!match) { throw new Error(`Remote temp dir template must end in at least three X, got ${JSON.stringify(template)}.`); } - const [, prefix] = match; + const [, prefix, xRun] = match; // Without a fixed part in the final component, `prefix` is a bare directory and every // sibling under it would validate - the parent-only check this function exists to avoid. if (prefix.endsWith('/')) { @@ -78,6 +79,13 @@ export function assertRemoteTempDir(remote, dir, template) { const suffix = dir.slice(prefix.length); if (suffix === '') fail('path is the bare template prefix, so mktemp created nothing'); if (suffix.includes('/')) fail('path descends below the directory the template would create'); + // mktemp replaces the X run with exactly that many characters from [A-Za-z0-9]. Requiring + // the same shape is what makes "only what this template could have produced" literally + // true, rather than "anything under the prefix without a slash in it". + if (suffix.length !== xRun.length) { + fail(`path suffix is ${suffix.length} characters, but the template's X run is ${xRun.length}`); + } + if (!/^[A-Za-z0-9]+$/.test(suffix)) fail('path suffix contains characters mktemp never generates'); return dir; } diff --git a/scripts/smoke-machine-sync-release.mjs b/scripts/smoke-machine-sync-release.mjs index f5dab10..9312f5b 100644 --- a/scripts/smoke-machine-sync-release.mjs +++ b/scripts/smoke-machine-sync-release.mjs @@ -416,14 +416,18 @@ function syncMachineArgs(options, remoteDir, runOptions = {}) { } function runSyncSmoke(options, runOptions = {}) { - // The remote dir is created first because it is the step that can reject: doing it before - // mkdtempSync means a refused temp dir cannot leak a local one. + // Both temp dirs are created INSIDE the try, with the finally null-guarding each one, so + // whichever creation succeeds is always cleaned up. Creating either one outside only moves + // the leak around: remote-first leaks a remote dir when mkdtempSync fails, local-first + // leaks a local dir when the remote dir is refused. const remoteTemplate = `/tmp/knowledge-linux-node-a-${options.knowledgeVersion}-XXXXXX`; - const remoteDir = createRemoteTempDir(options.remote, remoteTemplate); - const localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`)); + let remoteDir = null; + let localDir = null; const localCommandOptions = runOptions.localCommandOptions ?? {}; const learnCommandOptions = runOptions.learnCommandOptions ?? {}; try { + remoteDir = createRemoteTempDir(options.remote, remoteTemplate); + localDir = mkdtempSync(join(tmpdir(), `knowledge-linux-node-b-${options.knowledgeVersion}-`)); knowledgeJson(localDir, ['db', 'init', '--scope', 'project', '--json'], localCommandOptions); remoteKnowledgeJson(options.remote, remoteDir, ['db', 'init', '--scope', 'project', '--json']); @@ -584,23 +588,50 @@ function runSyncSmoke(options, runOptions = {}) { return summary; } finally { if (!options.keepTemp) { - rmSync(localDir, { recursive: true, force: true }); - // Belt-and-braces re-check before the delete. `remoteDir`/`remoteTemplate` are const and - // the validator is pure, so this cannot currently fail - it exists so a future refactor - // that makes the value reassignable is caught here rather than at the `rm -rf`. - // A throw inside `finally` would REPLACE any in-flight exception from the try block and - // skip the cleanup below, so it is contained: report and skip the delete, never mask. - let remoteDirStillValid = true; - try { - assertRemoteTempDir(options.remote, remoteDir, remoteTemplate); - } catch (error) { - remoteDirStillValid = false; - process.stderr.write( - `[smoke] refusing remote cleanup, temp dir no longer validates: ${error instanceof Error ? error.message : String(error)}\n` - ); + // EVERY statement in this finally is individually contained. A throw here would replace + // the in-flight exception from the try block - turning a real smoke failure into a + // confusing cleanup error - and would skip the remaining cleanup. Containing only the + // one statement that "cannot currently fail" while leaving the two that can is not + // containment. + const report = (what, error) => process.stderr.write( + `[smoke] ${what}: ${error instanceof Error ? error.message : String(error)}\n` + ); + + if (localDir !== null) { + try { + rmSync(localDir, { recursive: true, force: true }); + } catch (error) { + report(`could not remove local temp dir ${localDir}`, error); + } } - if (remoteDirStillValid) { - run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]); + + if (remoteDir !== null) { + // Re-check before the delete. remoteDir is assigned once from a validated value, so + // this cannot currently fail; it exists so a future refactor that makes the value + // reassignable is caught here rather than at the `rm -rf`. + let remoteDirStillValid = true; + try { + assertRemoteTempDir(options.remote, remoteDir, remoteTemplate); + } catch (error) { + remoteDirStillValid = false; + report('refusing remote cleanup, temp dir no longer validates', error); + } + + if (remoteDirStillValid) { + try { + // A failed remote delete leaves the temp dir behind; `run` swallows the exit + // status, so it is surfaced here rather than discarded. + const cleanup = run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]); + if (cleanup.status !== 0) { + report( + `remote cleanup of ${remoteDir} on ${options.remote} exited ${cleanup.status}; it may still exist`, + cleanup.stderr.trim() || 'no stderr' + ); + } + } catch (error) { + report(`remote cleanup of ${remoteDir} could not be spawned`, error); + } + } } } } diff --git a/tests/machine-smoke-script.test.ts b/tests/machine-smoke-script.test.ts index 4d3563d..72a90e8 100644 --- a/tests/machine-smoke-script.test.ts +++ b/tests/machine-smoke-script.test.ts @@ -114,8 +114,13 @@ describe('installed open-files boundary smoke script', () => { * non-zero remote exit, so a *failing* `mktemp -d` (exit 1) aborts the run and never assigns * an empty string. The reachable hole is the remote exiting **zero** with stdout that is not * the path - an ssh `Banner`, a chatty login profile, or a `ForceCommand` wrapper prepending - * lines or masking the status. Verified: `echo banner; mktemp -d ...` exits 0 and yields - * `"Welcome to linux-node-a\n/tmp/knowledge-banner-XXXXXX"`, which the guard rejects. + * lines or masking the status. Measured verbatim: + * + * $ echo "Welcome to linux-node-a"; mktemp -d /tmp/knowledge-banner-XXXXXX + * Welcome to linux-node-a + * /tmp/knowledge-banner-jjuN3k + * + * exit 0, so `runChecked` passes it through; the guard rejects it as multi-line. * * These cases prove the guard rejects bad input rather than merely accepting good input. * No `rm` runs here: only the validator is exercised. @@ -148,7 +153,8 @@ describe('remote temp dir guard', () => { './relative', '/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9/../../etc', ]) { - expect(() => assertRemoteTempDir(remote, bad, template)).toThrow(); + expect(() => assertRemoteTempDir(remote, bad, template)) + .toThrow(/Refusing to use remote temp dir/); } }); @@ -208,6 +214,44 @@ describe('remote temp dir guard', () => { .toThrow(/mktemp created nothing/); }); + test('rejects the banner-first shape the docstring calls the live case', () => { + // Documented as the reachable hole but previously untested: only path-then-junk was + // covered, never junk-then-path. + expect(() => assertRemoteTempDir( + remote, + 'Welcome to linux-node-a\n/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9', + template, + )).toThrow(/multiple lines/); + }); + + test('rejects a suffix mktemp could not have generated', () => { + const base = '/tmp/knowledge-linux-node-a-0.0.0-test-'; + // mktemp replaces the X run with exactly that many [A-Za-z0-9] characters. + expect(() => assertRemoteTempDir(remote, `${base}Z`, template)).toThrow(/X run is 6/); + expect(() => assertRemoteTempDir(remote, `${base}ZZZZZZZZZZ`, template)).toThrow(/X run is 6/); + expect(() => assertRemoteTempDir(remote, `${base}A B123`, template)) + .toThrow(/characters mktemp never generates/); + expect(() => assertRemoteTempDir(remote, `${base}$(id)`, template)).toThrow(); + expect(assertRemoteTempDir(remote, `${base}Ab3De9`, template)).toBe(`${base}Ab3De9`); + }); + + test('a suffix containing a separator reports descent, not a charset complaint', () => { + // The charset rule below would also reject this, but the descent message is the useful + // one. Asserting the message pins the ordering so the clearer diagnostic cannot be lost. + expect(() => assertRemoteTempDir( + remote, + '/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9/nested', + template, + )).toThrow(/descends below the directory the template would create/); + }); + + test('requires at least three X, the POSIX minimum the template rule cites', () => { + // Pins the documented boundary: without this, X{3,} could be relaxed to X{1,} unnoticed. + expect(() => assertRemoteTempDir(remote, '/tmp/k-A', '/tmp/k-X')).toThrow(/at least three X/); + expect(() => assertRemoteTempDir(remote, '/tmp/k-AB', '/tmp/k-XX')).toThrow(/at least three X/); + expect(assertRemoteTempDir(remote, '/tmp/k-ABC', '/tmp/k-XXX')).toBe('/tmp/k-ABC'); + }); + test('rejects a relative path even though the template bound implies absoluteness', () => { expect(() => assertRemoteTempDir(remote, 'tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9', template)) .toThrow(/not absolute/); diff --git a/tests/package-release.test.ts b/tests/package-release.test.ts index 8f21980..b10fcd8 100644 --- a/tests/package-release.test.ts +++ b/tests/package-release.test.ts @@ -7,6 +7,7 @@ import { describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import { dirname, join, posix } from 'node:path'; +import { builtinModules } from 'node:module'; import { fileURLToPath } from 'node:url'; const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); @@ -75,12 +76,46 @@ describe('public package release safety', () => { * packed script is still broken. */ test('every import in a published script resolves from the published package', () => { - // Strip comments and template literals first: a commented-out import must not fail the - // test, and a dynamic import with an interpolated path cannot be resolved statically. - const strip = (source: string) => source - .replace(/\/\*[\s\S]*?\*\//g, ' ') - .replace(/(^|[^:])\/\/[^\n]*/g, '$1 ') - .replace(/`(?:[^`\\]|\\.)*`/g, '``'); + // Comments must be removed with a scanner, not a regex. `/\*[\s\S]*?\*/` treats the `/*` + // inside a string literal such as '**/*.ts' as a comment opener and deletes everything up + // to the next `*/`, silently swallowing any import in between -- a guard-rail test that + // goes green while the published script is broken, which is the exact failure this test + // exists to catch. + const stripComments = (source: string): string => { + let out = ""; + let i = 0; + while (i < source.length) { + const ch = source[i]; + const next = source[i + 1]; + + if (ch === "/" && next === "/") { + while (i < source.length && source[i] !== "\n") i += 1; + continue; + } + if (ch === "/" && next === "*") { + i += 2; + while (i < source.length && !(source[i] === "*" && source[i + 1] === "/")) i += 1; + i += 2; + out += " "; + continue; + } + if (ch === "'" || ch === '"' || ch === "`") { + const quote = ch; + out += ch; + i += 1; + while (i < source.length) { + if (source[i] === "\\") { out += source.slice(i, i + 2); i += 2; continue; } + out += source[i]; + if (source[i] === quote) { i += 1; break; } + i += 1; + } + continue; + } + out += ch; + i += 1; + } + return out; + }; // `from '...'`, bare `import '...'` (side-effect), and `import('...')`. const specifierPattern = @@ -89,6 +124,16 @@ describe('public package release safety', () => { const runtimeDeps = new Set(Object.keys( (packageJson as unknown as { dependencies?: Record }).dependencies ?? {}, )); + // A published script may also import Node/Bun builtins without the `node:` prefix, its own + // package (valid via the `exports` map), and `#`-prefixed `imports` subpaths. + const ownName = (packageJson as unknown as { name?: string }).name ?? ""; + const isAlwaysResolvable = (specifier: string): boolean => + specifier.startsWith("node:") + || specifier.startsWith("bun:") + || specifier.startsWith("#") + || builtinModules.includes(specifier) + || specifier === ownName + || specifier.startsWith(`${ownName}/`); // Pre-existing breakage, tracked as todos task 1eb481d5. This script has TWO unresolvable // imports, not one: `../src/storage.ts` (src/ is not in `files`) and `@hasna/contracts/auth` @@ -101,11 +146,10 @@ describe('public package release safety', () => { ]); for (const script of publicScripts) { - const source = strip(readFileSync(join(repoRoot, script), 'utf8')); + const source = stripComments(readFileSync(join(repoRoot, script), 'utf8')); for (const [, specifier] of source.matchAll(specifierPattern)) { if (knownUnresolvedImports.has(`${script} -> ${specifier}`)) continue; - // Runtime builtins ship with the runtime, not with the package. - if (specifier.startsWith('node:') || specifier.startsWith('bun:')) continue; + if (isAlwaysResolvable(specifier)) continue; if (specifier.startsWith('.')) { const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); From 17f328fcabe75eeac9746772b7e035502847efa7 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Mon, 27 Jul 2026 01:20:14 +0300 Subject: [PATCH 4/6] fix(tests): use a real parser for published imports, and pin the check to itself Round-3 review REJECTED a15850f: my fix for N1 reintroduced N1, and nothing in the suite pinned either round-3 change. THE REAL DEFECT WAS STRUCTURAL, not the parsing bug. Three versions of this check shipped in a row, each replaceable with something that reports nothing while the whole suite stayed green, because the tests meant to pin them exercised a private copy of the logic instead of the logic itself. Round 3's mutation run proved it: stripComments could be replaced with `s => ""`, or reverted to the exact regex N1 condemned, and every test passed. Moving the implementation never fixed the operating mode, which was silent-green. Fixed by extracting `unresolvableImportsIn(script, source)` and having BOTH the enforcement test and the pinning tests call it. Mutation retest, all now CAUGHT and previously all SURVIVED: isAlwaysResolvable always true; extractor reports nothing; exemptions widened to everything; relative check disabled; bare-dep check disabled; `bun:` blessed as a class again. Parsing: replaced the hand-written scanner with Bun.Transpiler().scan(). Two hand-rolled strippers had the same defect - a `/*` inside an ordinary string opened a phantom comment and deleted real imports. The second only moved the trigger: a regex literal containing a quote (`/['"]/`) shifted it into string state and everything after it vanished, which the reviewer demonstrated on the real published smoke script with two innocuous added lines. Extracting imports is a parsing problem; anything short of a parser is a guess that fails silently. The parser also removed the reason for three allowances, so all three are deleted rather than justified: - `ownName`: existed only to mask a phantom the regex stripper invented from a template literal that GENERATES a child script. A real parser does not report it. The allowance also waved through `@hasna/knowledge/` for subpaths absent from `exports`, which fail with ERR_PACKAGE_PATH_NOT_EXPORTED. - unprefixed builtins: `builtinModules` under `bun test` is BUN's list and contains `ws`, `undici` and `bun` - real npm names, NOT Node builtins. Verified: all three are absent from Node 22's list and throw ERR_MODULE_NOT_FOUND from the tarball. - `#`-prefixed: package.json has no `imports` map, so every one is guaranteed to fail. It could only ever hide breakage. `bun:` is no longer blessed as a class either. Verified from the packed tarball: `node scripts/smoke-open-files-installed-boundary.mjs` throws ERR_UNSUPPORTED_ESM_URL_SCHEME. Exempted by name and filed as task 104f993d. F6/F7 - containment was correct in runSyncSmoke but violated one frame up. Its two callers' finally blocks used a raw rmSync, discarding the exception runSyncSmoke takes care to preserve, in exactly the --no-machines-sync paths the release smoke runs. And `report` itself was unguarded: process.stderr.write is synchronous on a pipe and throws EPIPE when the consumer exits, masking the in-flight error and skipping the remote cleanup. Both are now shared, individually-contained helpers (reportCleanupProblem, removeTempDirSafely) used at every finally; zero raw rmSync remains in any finally. F8 - the two validator checks that survived round 3 are now pinned: a lone \r (the multi-line check could be narrowed to \n unnoticed) and the suffix charset, with 6-character fixtures so only the charset rule can reject them - the earlier fixtures were all the wrong length, leaving the shell-metacharacter backstop pinned by nothing. Evidence: touched-file tests 24 pass / 0 fail (was 22). All 6 import-check mutations CAUGHT (all 6 previously survived); both validator mutations CAUGHT. Validator passes; packed tarball resolves and runs UNDER NODE, not just Bun. Full suite 239 pass / 2 skip / 1 fail, that failure being the pre-existing dated time bomb (task d1b93867). gitleaks clean. --- scripts/smoke-machine-sync-release.mjs | 61 ++++--- tests/machine-smoke-script.test.ts | 22 +++ tests/package-release.test.ts | 231 +++++++++++++++---------- 3 files changed, 199 insertions(+), 115 deletions(-) diff --git a/scripts/smoke-machine-sync-release.mjs b/scripts/smoke-machine-sync-release.mjs index 9312f5b..6e909e4 100644 --- a/scripts/smoke-machine-sync-release.mjs +++ b/scripts/smoke-machine-sync-release.mjs @@ -123,6 +123,32 @@ function runRemote(remote, command, options = {}) { return runChecked('ssh', [remote, command], options); } +/** + * Report a cleanup problem without ever throwing. + * + * Used only from `finally` blocks, where a throw REPLACES the in-flight exception from the + * try block and skips the remaining cleanup. `process.stderr.write` is synchronous on a pipe, + * so it throws EPIPE when the consumer has exited - which is why the reporter itself has to + * be contained, not just the statements it reports on. + */ +function reportCleanupProblem(what, error) { + try { + process.stderr.write(`[smoke] ${what}: ${error instanceof Error ? error.message : String(error)}\n`); + } catch { + // Nothing can be done about a failed diagnostic, and it must not mask the real error. + } +} + +/** Remove a directory from a `finally` block without ever throwing. */ +function removeTempDirSafely(dir, label) { + if (dir === null || dir === undefined) return; + try { + rmSync(dir, { recursive: true, force: true }); + } catch (error) { + reportCleanupProblem(`could not remove ${label} ${dir}`, error); + } +} + function createRemoteTempDir(remote, template) { const dir = runRemote(remote, `mktemp -d ${shellQuote(template)}`).trim(); return assertRemoteTempDir(remote, dir, template); @@ -588,22 +614,11 @@ function runSyncSmoke(options, runOptions = {}) { return summary; } finally { if (!options.keepTemp) { - // EVERY statement in this finally is individually contained. A throw here would replace - // the in-flight exception from the try block - turning a real smoke failure into a - // confusing cleanup error - and would skip the remaining cleanup. Containing only the - // one statement that "cannot currently fail" while leaving the two that can is not - // containment. - const report = (what, error) => process.stderr.write( - `[smoke] ${what}: ${error instanceof Error ? error.message : String(error)}\n` - ); - - if (localDir !== null) { - try { - rmSync(localDir, { recursive: true, force: true }); - } catch (error) { - report(`could not remove local temp dir ${localDir}`, error); - } - } + // EVERY statement in this finally is individually contained, including `report` itself. + // A throw here would replace the in-flight exception from the try block - turning a real + // smoke failure into a confusing cleanup error - and would skip the remaining cleanup. + + removeTempDirSafely(localDir, 'local temp dir'); if (remoteDir !== null) { // Re-check before the delete. remoteDir is assigned once from a validated value, so @@ -614,7 +629,7 @@ function runSyncSmoke(options, runOptions = {}) { assertRemoteTempDir(options.remote, remoteDir, remoteTemplate); } catch (error) { remoteDirStillValid = false; - report('refusing remote cleanup, temp dir no longer validates', error); + reportCleanupProblem('refusing remote cleanup, temp dir no longer validates', error); } if (remoteDirStillValid) { @@ -623,13 +638,13 @@ function runSyncSmoke(options, runOptions = {}) { // status, so it is surfaced here rather than discarded. const cleanup = run('ssh', [options.remote, `rm -rf ${shellQuote(remoteDir)}`]); if (cleanup.status !== 0) { - report( + reportCleanupProblem( `remote cleanup of ${remoteDir} on ${options.remote} exited ${cleanup.status}; it may still exist`, cleanup.stderr.trim() || 'no stderr' ); } } catch (error) { - report(`remote cleanup of ${remoteDir} could not be spawned`, error); + reportCleanupProblem(`remote cleanup of ${remoteDir} could not be spawned`, error); } } } @@ -645,7 +660,7 @@ function runNoMachinesSyncSmoke(options, dirs) { try { probe = knowledgeJson(probeDir, ['machines', 'topology', '--no-tailscale', '--json'], { env: runner.env }); } finally { - if (!options.keepTemp) rmSync(probeDir, { recursive: true, force: true }); + if (!options.keepTemp) removeTempDirSafely(probeDir, 'probe dir'); } if (probe.adapter?.implementation !== 'disabled' || probe.adapter?.available !== false) { throw new Error(`no-machines probe did not disable adapter: ${JSON.stringify(probe.adapter)}`); @@ -675,7 +690,7 @@ function runNoMachinesSyncSmoke(options, dirs) { sync, }; } finally { - if (!options.keepTemp) rmSync(runner.app_dir, { recursive: true, force: true }); + if (!options.keepTemp) removeTempDirSafely(runner.app_dir, 'runner app dir'); } } @@ -687,7 +702,7 @@ function runNoMachinesRegistrySyncSmoke(options, dirs) { try { probe = knowledgeJson(probeDir, ['machines', 'topology', '--no-tailscale', '--json'], { env: runner.env }); } finally { - if (!options.keepTemp) rmSync(probeDir, { recursive: true, force: true }); + if (!options.keepTemp) removeTempDirSafely(probeDir, 'probe dir'); } if (probe.adapter?.implementation !== 'disabled' || probe.adapter?.available !== false) { throw new Error(`no-machines registry probe did not disable adapter: ${JSON.stringify(probe.adapter)}`); @@ -730,7 +745,7 @@ function runNoMachinesRegistrySyncSmoke(options, dirs) { sync, }; } finally { - if (!options.keepTemp) rmSync(runner.app_dir, { recursive: true, force: true }); + if (!options.keepTemp) removeTempDirSafely(runner.app_dir, 'runner app dir'); } } diff --git a/tests/machine-smoke-script.test.ts b/tests/machine-smoke-script.test.ts index 72a90e8..03ef035 100644 --- a/tests/machine-smoke-script.test.ts +++ b/tests/machine-smoke-script.test.ts @@ -245,6 +245,28 @@ describe('remote temp dir guard', () => { )).toThrow(/descends below the directory the template would create/); }); + test('rejects a lone carriage return, not just a newline', () => { + // The multi-line check is /[\r\n]/; a fixture using only \n would let it be narrowed to + // /\n/ unnoticed. + expect(() => assertRemoteTempDir( + remote, + '/tmp/knowledge-linux-node-a-0.0.0-test-Ab3De9\rjunk', + template, + )).toThrow(/multiple lines/); + }); + + test('rejects shell metacharacters and separators in the suffix by charset, not by length', () => { + // Each of these is exactly 6 characters, so only the charset rule can reject them. The + // earlier fixtures were all the wrong length, leaving the charset check - the + // shell-metacharacter backstop - pinned by nothing. + const base = '/tmp/knowledge-linux-node-a-0.0.0-test-'; + for (const suffix of ['Ab3De.', 'Ab3De-', 'Ab3De_', 'Ab3De$', 'Ab3De;', 'Ab3De*', 'Ab3De ']) { + expect(() => assertRemoteTempDir(remote, `${base}${suffix}`, template)) + .toThrow(/characters mktemp never generates/); + } + expect(assertRemoteTempDir(remote, `${base}Ab3De9`, template)).toBe(`${base}Ab3De9`); + }); + test('requires at least three X, the POSIX minimum the template rule cites', () => { // Pins the documented boundary: without this, X{3,} could be relaxed to X{1,} unnoticed. expect(() => assertRemoteTempDir(remote, '/tmp/k-A', '/tmp/k-X')).toThrow(/at least three X/); diff --git a/tests/package-release.test.ts b/tests/package-release.test.ts index b10fcd8..5ee20f3 100644 --- a/tests/package-release.test.ts +++ b/tests/package-release.test.ts @@ -7,7 +7,6 @@ import { describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import { dirname, join, posix } from 'node:path'; -import { builtinModules } from 'node:module'; import { fileURLToPath } from 'node:url'; const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); @@ -44,6 +43,18 @@ const forbiddenPackagePaths = [ ].sort(); describe('public package release safety', () => { + // Pre-existing breakage, each verified against a packed tarball and tracked in todos. + // Every entry must be removed by its task; removing one and not the others leaves the + // published script still throwing, so they are listed individually. + const knownUnresolvedImports = new Set([ + // task 1eb481d5 - src/ is not in `files` + 'scripts/apply-cloud-migrations.mjs -> ../src/storage.ts', + // task 1eb481d5 - @hasna/contracts is a devDependency only + 'scripts/apply-cloud-migrations.mjs -> @hasna/contracts/auth', + // task 104f993d - bun:sqlite throws ERR_UNSUPPORTED_ESM_URL_SCHEME under node + 'scripts/smoke-open-files-installed-boundary.mjs -> bun:sqlite', + ]); + test('package files list public docs explicitly', () => { expect(packageJson.files).not.toContain('docs'); expect(packageJson.files).not.toContain('docs/*'); @@ -66,104 +77,140 @@ describe('public package release safety', () => { }); /** - * A published script that imports something the package does not ship resolves fine in the - * repo and throws ERR_MODULE_NOT_FOUND for everyone who installs it. The allowlists above - * only catch the opposite mistake (shipping something unreviewed), so this closes the gap. + * Imports of a published script that would NOT resolve for someone who installed the + * package. Returns the offending specifiers, so both the enforcement test and the tests + * that pin it call THIS function - if it stops reporting, both fail. * - * Both directions are checked, because a published script can fail to resolve two ways: - * a relative import of a file outside `files`, or a bare import of a package that is not a - * runtime dependency. Checking only relative imports would let this test go green while the - * packed script is still broken. + * That structure is the point. Three previous versions of this check could each be replaced + * with something that reports nothing while the whole suite stayed green, because the tests + * that were supposed to pin them exercised a private copy of the logic instead of the logic + * itself. Silent-green was the defect; moving the implementation never fixed it. + * + * Imports are extracted with the real transpiler, NOT by stripping comments and matching a + * regex. Two hand-written strippers were tried and both had the same defect: a `/*` inside + * an ordinary string opened a phantom comment and silently deleted real imports. The second + * only moved the trigger - a regex literal containing a quote (`/['"]/`) shifted it into + * string state and everything after it vanished. Extracting imports is a parsing problem; + * anything short of a parser is a guess that fails silently. */ - test('every import in a published script resolves from the published package', () => { - // Comments must be removed with a scanner, not a regex. `/\*[\s\S]*?\*/` treats the `/*` - // inside a string literal such as '**/*.ts' as a comment opener and deletes everything up - // to the next `*/`, silently swallowing any import in between -- a guard-rail test that - // goes green while the published script is broken, which is the exact failure this test - // exists to catch. - const stripComments = (source: string): string => { - let out = ""; - let i = 0; - while (i < source.length) { - const ch = source[i]; - const next = source[i + 1]; - - if (ch === "/" && next === "/") { - while (i < source.length && source[i] !== "\n") i += 1; - continue; - } - if (ch === "/" && next === "*") { - i += 2; - while (i < source.length && !(source[i] === "*" && source[i + 1] === "/")) i += 1; - i += 2; - out += " "; - continue; - } - if (ch === "'" || ch === '"' || ch === "`") { - const quote = ch; - out += ch; - i += 1; - while (i < source.length) { - if (source[i] === "\\") { out += source.slice(i, i + 2); i += 2; continue; } - out += source[i]; - if (source[i] === quote) { i += 1; break; } - i += 1; - } - continue; - } - out += ch; - i += 1; - } - return out; - }; - - // `from '...'`, bare `import '...'` (side-effect), and `import('...')`. - const specifierPattern = - /(?:\bfrom\s*|\bimport\s*\(\s*|\bimport\s+(?=['"]))['"]([^'"]+)['"]/g; - + function unresolvableImportsIn(script: string, source: string): string[] { + const transpiler = new Bun.Transpiler({ loader: 'ts' }); const runtimeDeps = new Set(Object.keys( (packageJson as unknown as { dependencies?: Record }).dependencies ?? {}, )); - // A published script may also import Node/Bun builtins without the `node:` prefix, its own - // package (valid via the `exports` map), and `#`-prefixed `imports` subpaths. - const ownName = (packageJson as unknown as { name?: string }).name ?? ""; - const isAlwaysResolvable = (specifier: string): boolean => - specifier.startsWith("node:") - || specifier.startsWith("bun:") - || specifier.startsWith("#") - || builtinModules.includes(specifier) - || specifier === ownName - || specifier.startsWith(`${ownName}/`); - - // Pre-existing breakage, tracked as todos task 1eb481d5. This script has TWO unresolvable - // imports, not one: `../src/storage.ts` (src/ is not in `files`) and `@hasna/contracts/auth` - // (a devDependency only). Both are exempted by name so the rule is not weakened, and - // BOTH must be removed for that task to be genuinely done -- deleting only the first - // would leave the published script still throwing. - const knownUnresolvedImports = new Set([ - 'scripts/apply-cloud-migrations.mjs -> ../src/storage.ts', - 'scripts/apply-cloud-migrations.mjs -> @hasna/contracts/auth', - ]); + /** + * Only `node:`-prefixed builtins are unconditionally resolvable. + * + * Deliberately NOT allowed, each verified against this repo rather than assumed: + * - unprefixed builtins. `builtinModules` under `bun test` is BUN's list, which contains + * `ws`, `undici` and `bun` - real npm package names that are NOT Node builtins, so a + * published script importing `ws` would pass here and throw for every node installer. + * - the package's own name. No published script imports it; that allowance existed only + * to mask a phantom the old regex stripper invented from a template literal, which a + * real parser does not report at all. + * - `#`-prefixed subpaths. package.json has no `imports` map, so every one is guaranteed + * to fail with ERR_PACKAGE_IMPORT_NOT_DEFINED. + * - `bun:` builtins. They resolve under Bun and throw ERR_UNSUPPORTED_ESM_URL_SCHEME + * under node, so they are exempted by name below, never blessed as a class. + */ + const isAlwaysResolvable = (specifier: string): boolean => specifier.startsWith('node:'); + + const unresolvable: string[] = []; + for (const { path: specifier } of transpiler.scan(source).imports) { + if (knownUnresolvedImports.has(`${script} -> ${specifier}`)) continue; + if (isAlwaysResolvable(specifier)) continue; + + if (specifier.startsWith('.')) { + const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); + if (!publicScripts.includes(resolved)) unresolvable.push(specifier); + continue; + } + + const pkg = specifier.startsWith('@') + ? specifier.split('/').slice(0, 2).join('/') + : specifier.split('/')[0]; + if (!runtimeDeps.has(pkg)) unresolvable.push(specifier); + } + return unresolvable; + } + + /** + * A published script that imports something the package does not ship resolves fine in the + * repo and throws ERR_MODULE_NOT_FOUND for everyone who installs it. The allowlists above + * only catch the opposite mistake (shipping something unreviewed), so this closes the gap. + */ + test('every import in a published script resolves from the published package', () => { for (const script of publicScripts) { - const source = stripComments(readFileSync(join(repoRoot, script), 'utf8')); - for (const [, specifier] of source.matchAll(specifierPattern)) { - if (knownUnresolvedImports.has(`${script} -> ${specifier}`)) continue; - if (isAlwaysResolvable(specifier)) continue; - - if (specifier.startsWith('.')) { - const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); - expect(publicScripts, `${script} imports ${specifier}, which is not in the published files list`) - .toContain(resolved); - continue; - } - - // Bare specifier: must be a runtime dependency, not a devDependency. - const pkg = specifier.startsWith('@') - ? specifier.split('/').slice(0, 2).join('/') - : specifier.split('/')[0]; - expect(runtimeDeps, `${script} imports ${specifier}, which is not a runtime dependency`) - .toContain(pkg); + const source = readFileSync(join(repoRoot, script), 'utf8'); + expect(unresolvableImportsIn(script, source), `unresolvable imports in ${script}`).toEqual([]); + } + }); + + /** + * Pins the function above against the mutation that kept slipping through: every previous + * version could be replaced with something that reports nothing while the suite stayed + * green. Each case below drives the REAL function, so disabling the extractor, widening the + * exemptions, or making everything "always resolvable" fails here. + */ + test('the published-import check reports real breakage, in every shape', () => { + const check = (source: string) => unresolvableImportsIn('scripts/probe.mjs', source); + + // The two shapes that defeated both hand-written strippers. + expect(check(`const g = '**/*.ts';\nimport bad from 'ghost-a';`)).toContain('ghost-a'); + expect(check(`const q = /['"]/g;\nconst g = '**/*.json';\nconst m = import('ghost-b');`)).toContain('ghost-b'); + expect(check(`const re = /[a\\/*]/;\nimport bad from 'ghost-c';`)).toContain('ghost-c'); + // Every import form the check claims to cover. + expect(check(`import './not-shipped.mjs';`)).toContain('./not-shipped.mjs'); + expect(check(`export { a } from './not-shipped2.mjs';`)).toContain('./not-shipped2.mjs'); + expect(check(`export * from './not-shipped3.mjs';`)).toContain('./not-shipped3.mjs'); + expect(check(`const m = await import('ghost-d');`)).toContain('ghost-d'); + // The allowances are exactly as narrow as documented. + expect(check(`import x from 'ws';`)).toContain('ws'); + expect(check(`import x from 'undici';`)).toContain('undici'); + expect(check(`import x from '#internal/x.js';`)).toContain('#internal/x.js'); + expect(check(`import x from '@hasna/knowledge/not-in-exports';`)).toContain('@hasna/knowledge/not-in-exports'); + expect(check(`import { Database } from 'bun:sqlite';`)).toContain('bun:sqlite'); + + // ...and legitimate code is not reported, or the check false-positives and gets widened. + expect(check(`import { readFileSync } from 'node:fs';`)).toEqual([]); + expect(check(`import { z } from 'zod';`)).toEqual([]); + expect(check(`// import dead from 'ghost-e';`)).toEqual([]); + expect(check(`/* import dead from 'ghost-f'; */`)).toEqual([]); + expect(check(`const s = "import x from 'ghost-g'";`)).toEqual([]); + // A child script emitted as a template literal is data, not an import of this file. + expect(check("const child = `import { k } from '@hasna/knowledge';`;")).toEqual([]); + }); + + test('the exemptions stay narrow, and each names real breakage', () => { + const transpiler = new Bun.Transpiler({ loader: 'ts' }); + const importsOf = (script: string) => + transpiler.scan(readFileSync(join(repoRoot, script), 'utf8')).imports.map((i) => i.path); + + // Each exempted import must still exist; a stale exemption invites the next one to be + // added without evidence. + expect(importsOf('scripts/apply-cloud-migrations.mjs')).toContain('../src/storage.ts'); + expect(importsOf('scripts/apply-cloud-migrations.mjs')).toContain('@hasna/contracts/auth'); + expect(importsOf('scripts/smoke-open-files-installed-boundary.mjs')).toContain('bun:sqlite'); + + // ...and each is genuinely unresolvable, not merely unfamiliar. + expect(packageJson.files).not.toContain('src'); + const deps = (packageJson as unknown as { dependencies?: Record }).dependencies ?? {}; + expect(Object.keys(deps)).not.toContain('@hasna/contracts'); + + // Removing an exemption must make the check report it again. + for (const [script, specifier] of [ + ['scripts/apply-cloud-migrations.mjs', '../src/storage.ts'], + ['scripts/apply-cloud-migrations.mjs', '@hasna/contracts/auth'], + ['scripts/smoke-open-files-installed-boundary.mjs', 'bun:sqlite'], + ] as const) { + const key = `${script} -> ${specifier}`; + knownUnresolvedImports.delete(key); + try { + expect(unresolvableImportsIn(script, readFileSync(join(repoRoot, script), 'utf8'))) + .toContain(specifier); + } finally { + knownUnresolvedImports.add(key); } } }); From 9f888df07c2c797d781e6bcece9d6465765a7548 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Mon, 27 Jul 2026 01:49:05 +0300 Subject: [PATCH 5/6] fix(smoke): stop executing on import, and close round-5 review findings The most important change came out of a review accident, not a finding. Importing scripts/smoke-machine-sync-release.mjs executed it: `main()` ran at module scope, so merely loading the PUBLISHED script performed `bun install -g @hasna/knowledge` on the local machine and attempted `ssh bun install -g` - before any argument was inspected. It fired for real during review of this PR. Any tool that imports the file for linting or dependency analysis - including the packaging check this PR adds - triggers it. `main()` now runs only when invoked directly, compared against `process.argv[1]` rather than `import.meta.main`, which Node did not support until v24 and which would have silently turned the script into a no-op there. Verified: still works under both bun and node when invoked; importing is inert in 9ms with no install and no ssh. Incident recorded with blast radius (local install only; the remote host has no DNS or ssh-config entry, so nothing on the fleet was touched). F-1: the exemption list was duplicated - the test meant to pin it iterated a hardcoded literal array rather than the Set - so ADDING an exemption was checked by nothing. A genuinely broken import plus a matching entry left the whole file green. Same silent-green class as round 3, one level down. The loop now derives from the Set, and every entry must name a published script. Verified: that exact combined mutation is now CAUGHT. F-2: deleting the allowances over-corrected and created two false positives, both proven against an extracted tarball under node rather than argued: import { sep } from 'path' RESOLVES (unprefixed Node builtin) @hasna/knowledge/storage RESOLVES (declared in exports) Both are restored, narrowly: unprefixed builtins are matched against a hardcoded NODE list (not `builtinModules`, which under `bun test` returns BUN's list including ws/undici/bun), and self-reference only for subpaths the `exports` map declares. Still refused, also verified: @hasna/knowledge/ (ERR_PACKAGE_PATH_NOT_EXPORTED), `#`-prefixed (ERR_PACKAGE_IMPORT_NOT_DEFINED), ws/undici (ERR_MODULE_NOT_FOUND). The previous comment claimed each deletion was "verified against this repo"; the self-reference one was assumed, and was wrong. F-3: the relative branch checked a hand-maintained 7-script list instead of what the package ships, so `../package.json` and `../dist/index.js` - both published, both verified resolvable - were reported as breakage. Now checked against `files`, with package.json treated as always shipped since npm includes it regardless. F-4: documented the extractor's real scope. It reports import/import()/export-from, not require(), import.meta.resolve() or new URL(). None are used today; saying so beats letting the test name imply more than it covers. F-5: the EPIPE rationale was factually wrong. Measured on Linux, writing to a stderr pipe whose reader has exited raises an asynchronous 'error' event that kills the process - not a synchronous throw - so the try/catch cannot intercept it. The comment now says what the guard does and does not cover. Evidence: touched-file tests 26 pass / 0 fail. Mutation battery all CAUGHT, including X1 (broken import + matching exemption) which SURVIVED round 5, plus the bogus-extra- exemption, check-returns-nothing, always-resolvable, bare-dep-disabled and builtins-widened mutations. Validator passes; full suite 239 pass / 2 skip / 1 fail, that failure being the pre-existing dated time bomb (task d1b93867). Corrected evidence claim: the touched-file count is 26, not the 24 stated last round. --- scripts/smoke-machine-sync-release.mjs | 29 +++++++-- tests/package-release.test.ts | 81 +++++++++++++++++++++++--- 2 files changed, 97 insertions(+), 13 deletions(-) diff --git a/scripts/smoke-machine-sync-release.mjs b/scripts/smoke-machine-sync-release.mjs index 6e909e4..4a7645c 100644 --- a/scripts/smoke-machine-sync-release.mjs +++ b/scripts/smoke-machine-sync-release.mjs @@ -127,9 +127,14 @@ function runRemote(remote, command, options = {}) { * Report a cleanup problem without ever throwing. * * Used only from `finally` blocks, where a throw REPLACES the in-flight exception from the - * try block and skips the remaining cleanup. `process.stderr.write` is synchronous on a pipe, - * so it throws EPIPE when the consumer has exited - which is why the reporter itself has to - * be contained, not just the statements it reports on. + * try block and skips the remaining cleanup, so the reporter itself is contained rather than + * only the statements it reports on. + * + * The try/catch covers a synchronous throw (a non-string argument, a closed fd). It does NOT + * cover EPIPE: measured on Linux, writing to a stderr pipe whose reader has exited raises an + * asynchronous 'error' event that terminates the process, and no catch here can intercept + * that. An earlier version of this comment claimed otherwise; the residual risk of the + * process dying mid-cleanup is real and is not addressed by this function. */ function reportCleanupProblem(what, error) { try { @@ -864,4 +869,20 @@ function main() { outputSummary(summary, options); } -main(); +/** + * Run only when invoked directly, never on import. + * + * `main()` at module scope means merely importing this file performs `bun install -g` locally + * AND `ssh bun install -g` on a fleet host, before any argument is inspected. That is + * not hypothetical: it fired during review of this very PR, when the file was imported to test + * import resolution, and it installed a package on the reviewer's machine and attempted a + * remote install. A published script must not mutate a machine as a side effect of being + * loaded. Compared against argv rather than `import.meta.main`, which Node did not support + * until v24 and which would silently turn this script into a no-op there. + */ +const invokedDirectly = process.argv[1] !== undefined + && resolve(process.argv[1]) === fileURLToPath(import.meta.url); + +if (invokedDirectly) { + main(); +} diff --git a/tests/package-release.test.ts b/tests/package-release.test.ts index 5ee20f3..377cf97 100644 --- a/tests/package-release.test.ts +++ b/tests/package-release.test.ts @@ -43,6 +43,29 @@ const forbiddenPackagePaths = [ ].sort(); describe('public package release safety', () => { + // Node's builtin list, hardcoded on purpose. `builtinModules` under `bun test` returns BUN's + // list, which includes `ws`, `undici` and `bun` - real npm package names that are NOT Node + // builtins, so using it would bless imports that throw for every node installer. + const NODE_BUILTINS = new Set([ + 'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'console', 'constants', + 'crypto', 'dgram', 'diagnostics_channel', 'dns', 'domain', 'events', 'fs', 'http', 'http2', + 'https', 'inspector', 'module', 'net', 'os', 'path', 'perf_hooks', 'process', 'punycode', + 'querystring', 'readline', 'repl', 'stream', 'string_decoder', 'timers', 'tls', 'trace_events', + 'tty', 'url', 'util', 'v8', 'vm', 'wasi', 'worker_threads', 'zlib', + ]); + + /** Does the published package ship this repo-relative path? */ + const isShippedPath = (repoRelativePath: string): boolean => { + // npm always includes package.json regardless of `files`, and a version string read from + // it is an ordinary thing for a published script to do. + if (repoRelativePath === 'package.json') return true; + const shipped = (packageJson as unknown as { files?: string[] }).files ?? []; + return shipped.some((entry) => { + const normalized = entry.replace(/\/+$/, ''); + return repoRelativePath === normalized || repoRelativePath.startsWith(`${normalized}/`); + }); + }; + // Pre-existing breakage, each verified against a packed tarball and tracked in todos. // Every entry must be removed by its task; removing one and not the others leaves the // published script still throwing, so they are listed individually. @@ -86,6 +109,12 @@ describe('public package release safety', () => { * that were supposed to pin them exercised a private copy of the logic instead of the logic * itself. Silent-green was the defect; moving the implementation never fixed it. * + * Scope limit, stated rather than implied by the name: the transpiler reports `import`, + * `import()` and `export … from`. It does NOT report `require()`, `import.meta.resolve()`, + * or `new URL('./x', import.meta.url)`. No published script uses those today, and the last + * is the plausible one to appear, since it is how you reference a sibling that must be in + * `files`. A file the transpiler cannot parse throws, so that path is loud, not silent. + * * Imports are extracted with the real transpiler, NOT by stripping comments and matching a * regex. Two hand-written strippers were tried and both had the same defect: a `/*` inside * an ordinary string opened a phantom comment and silently deleted real imports. The second @@ -114,7 +143,26 @@ describe('public package release safety', () => { * - `bun:` builtins. They resolve under Bun and throw ERR_UNSUPPORTED_ESM_URL_SCHEME * under node, so they are exempted by name below, never blessed as a class. */ - const isAlwaysResolvable = (specifier: string): boolean => specifier.startsWith('node:'); + // `node:`-prefixed builtins, unprefixed NODE builtins, and self-reference through the + // package's own `exports` map all resolve from a real install - each verified against an + // extracted tarball under node, not assumed. Deleting these outright was a false positive: + // `import { sep } from 'path'` and `@hasna/knowledge/storage` both resolve. + // + // Still NOT allowed, also verified: `@hasna/knowledge/` + // (ERR_PACKAGE_PATH_NOT_EXPORTED), `#`-prefixed (ERR_PACKAGE_IMPORT_NOT_DEFINED, no + // `imports` map), and `ws`/`undici`/`bun` (ERR_MODULE_NOT_FOUND - they are in BUN's + // builtinModules, not Node's, which is why NODE_BUILTINS is hardcoded). + const exportsMap = (packageJson as unknown as { exports?: Record }).exports ?? {}; + const ownName = (packageJson as unknown as { name?: string }).name ?? ''; + const exportedSubpaths = new Set( + Object.keys(exportsMap).map((key) => (key === '.' ? ownName : `${ownName}/${key.replace(/^\.\//, '')}`)), + ); + + const isAlwaysResolvable = (specifier: string): boolean => { + if (specifier.startsWith('node:')) return true; + if (NODE_BUILTINS.has(specifier)) return true; + return ownName !== '' && exportedSubpaths.has(specifier); + }; const unresolvable: string[] = []; for (const { path: specifier } of transpiler.scan(source).imports) { @@ -123,7 +171,10 @@ describe('public package release safety', () => { if (specifier.startsWith('.')) { const resolved = posix.normalize(posix.join(posix.dirname(script), specifier)); - if (!publicScripts.includes(resolved)) unresolvable.push(specifier); + // Checked against everything the package SHIPS, not just the scripts allowlist: + // `../package.json` and `../dist/index.js` are published and resolve from a real + // install, and both were reported as breakage by the scripts-only check. + if (!isShippedPath(resolved)) unresolvable.push(specifier); continue; } @@ -173,8 +224,18 @@ describe('public package release safety', () => { expect(check(`import { Database } from 'bun:sqlite';`)).toContain('bun:sqlite'); // ...and legitimate code is not reported, or the check false-positives and gets widened. + // Every entry below was verified to RESOLVE from an extracted tarball under node. expect(check(`import { readFileSync } from 'node:fs';`)).toEqual([]); expect(check(`import { z } from 'zod';`)).toEqual([]); + // Unprefixed Node builtins resolve; nothing in this repo forces the node: prefix. + expect(check(`import { sep } from 'path';`)).toEqual([]); + expect(check(`import { readFileSync as r } from 'fs';`)).toEqual([]); + expect(check(`import { spawnSync } from 'child_process';`)).toEqual([]); + // Self-reference resolves for subpaths the exports map declares - and only those. + expect(check(`import x from '@hasna/knowledge';`)).toEqual([]); + expect(check(`import x from '@hasna/knowledge/storage';`)).toEqual([]); + // Shipped non-script files are published and resolve. + expect(check(`import pkg from '../package.json' with { type: 'json' };`)).toEqual([]); expect(check(`// import dead from 'ghost-e';`)).toEqual([]); expect(check(`/* import dead from 'ghost-f'; */`)).toEqual([]); expect(check(`const s = "import x from 'ghost-g'";`)).toEqual([]); @@ -198,13 +259,15 @@ describe('public package release safety', () => { const deps = (packageJson as unknown as { dependencies?: Record }).dependencies ?? {}; expect(Object.keys(deps)).not.toContain('@hasna/contracts'); - // Removing an exemption must make the check report it again. - for (const [script, specifier] of [ - ['scripts/apply-cloud-migrations.mjs', '../src/storage.ts'], - ['scripts/apply-cloud-migrations.mjs', '@hasna/contracts/auth'], - ['scripts/smoke-open-files-installed-boundary.mjs', 'bun:sqlite'], - ] as const) { - const key = `${script} -> ${specifier}`; + // Derived from the Set, never restated. A hardcoded copy meant ADDING an exemption was + // pinned by nothing: a genuinely broken import plus a matching entry left the file green. + expect(knownUnresolvedImports.size).toBeGreaterThan(0); + for (const key of [...knownUnresolvedImports]) { + const separator = key.indexOf(' -> '); + const script = key.slice(0, separator); + const specifier = key.slice(separator + 4); + // Every exemption must name a script that is actually published... + expect(publicScripts, `exemption names an unpublished script: ${key}`).toContain(script); knownUnresolvedImports.delete(key); try { expect(unresolvableImportsIn(script, readFileSync(join(repoRoot, script), 'utf8'))) From 17cd2a6d3484ea1e8fd77ab45ebc5bdd52083131 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Mon, 27 Jul 2026 04:51:01 +0300 Subject: [PATCH 6/6] fix(smoke): resolve symlinks in the direct-invocation guard, and pin both fixes with tests Closes the three round-6 review findings, all of the same class: the two fixes this PR exists to make were not observable by any test, and the guard added for one of them introduced a third silent failure. 1. The direct-invocation guard compared `resolve(process.argv[1])` with `fileURLToPath(import.meta.url)`. Node resolves the main module through symlinks before deriving `import.meta.url`; `resolve()` is purely lexical. So through any symlinked path - a pnpm-style `node_modules/@hasna/knowledge -> /package`, or macOS `/tmp -> /private/tmp`, which is the PR's own documented `npm pack && tar xzf` evidence procedure - `main()` never ran, the script printed nothing and exited 0. A release gate checking only the exit status would have recorded a green smoke that did nothing. Compare realpaths instead, falling back to the lexical comparison only when a path cannot be resolved at all. Verified against the packed tarball: through a pnpm-style symlink under node, `--dry-run --json` now produces output and `--totally-bogus` raises "Unknown argument"; before, both exited 0 silently. 2. Nothing pinned that `assertRemoteTempDir` was called before the remote `rm -rf`; deleting both calls left the whole suite green. `createRemoteTempDir` and `removeRemoteTempDir` now live in the side-effect-free `scripts/lib` module and take their ssh work as a parameter, so tests drive the functions that decide whether the validator runs, including the negative that matters - no delete is attempted when the value does not validate. They are kept out of the smoke script so this test file never has to import it. 3. Nothing pinned "does not execute on import"; reverting to a bare `main();` left the suite green. A new test imports the script in a child process whose PATH puts recorders in front of bash/bun/ssh, with a control proving the recorders are reachable, and asserts nothing was installed and nothing was sent to the remote. Both argv shapes are covered (`node -e`, and an ordinary importer module). Also corrects the guard's comment, which claimed Node had no `import.meta.main` until v24; measured, v22.22.3 defines it as a boolean. Mutation-verified - each fix reverted individually, each caught by exactly one test: lexical compare restored -> symlink test fails; `main()` unconditional -> import test fails; create-side assert deleted -> createRemoteTempDir test fails; delete-side re-check deleted -> "never attempts a delete" test fails; helpers bypassed in the script -> wiring test fails. Tests: 249 pass, 2 skip, 1 fail (bun test), up from 239 pass, 2 skip, 1 fail. The one failure is tests/cli.test.ts:2680, reproduced identically at base main 2023a26 (66 pass / 1 fail) and unrelated to this PR. --- scripts/lib/remote-temp-dir.mjs | 65 +++++ scripts/smoke-machine-sync-release.mjs | 97 +++++--- tests/machine-smoke-script.test.ts | 315 ++++++++++++++++++++++++- 3 files changed, 435 insertions(+), 42 deletions(-) diff --git a/scripts/lib/remote-temp-dir.mjs b/scripts/lib/remote-temp-dir.mjs index 5c638ec..10c4939 100644 --- a/scripts/lib/remote-temp-dir.mjs +++ b/scripts/lib/remote-temp-dir.mjs @@ -89,3 +89,68 @@ export function assertRemoteTempDir(remote, dir, template) { return dir; } + +/** + * Create a remote temp dir, returning only a path this exact template could have produced. + * + * `makeTempDir` is injected rather than imported so this module never spawns a process - the + * published script stays the only thing that talks to ssh - and, more to the point, so a test + * can drive THIS function, the one that decides whether the validator runs at all. Exercising + * `assertRemoteTempDir` on its own proves the validator works but pins nothing about it being + * wired in: deleting the call below used to leave the entire suite green. + * + * @param {string} remote Remote host. + * @param {string} template Absolute template passed to `mktemp -d`, ending in `XXX...`. + * @param {(remote: string, template: string) => string} makeTempDir Runs `mktemp -d