From bcc97074b79cbfcf4c0d966fff5d9ae8ea670ea3 Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Fri, 28 Aug 2026 06:31:05 +0600 Subject: [PATCH 1/3] test(studio): pin runStudio's spawn error listener against an unattended process kill `spawn()` reports EACCES, EPERM and a Gatekeeper refusal by emitting 'error' on a later tick, not by throwing, so runStudio's try/catch never sees it and an unlistened event is rethrown as an uncaught exception on a CLI process that has already detached the child and returned to the shell. The listener has always been there; nothing asserted it, because fakeSpawn returns { on: () => undefined }. Reuses auto-launch's real-EventEmitter fake so the ordering claim is about the one moment the command stops being able to attach anything, not about what a later look happens to find. --- tests/unit/cli/studio.test.ts | 75 +++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/unit/cli/studio.test.ts b/tests/unit/cli/studio.test.ts index 39e43d45..c02a48f2 100644 --- a/tests/unit/cli/studio.test.ts +++ b/tests/unit/cli/studio.test.ts @@ -45,6 +45,7 @@ import { MarkStore } from '../../../src/studio/mark/store.js'; import { ProfileStore } from '../../../src/studio/profile-store.js'; import { scopeStorageStateToOrigin } from '../../../src/studio/login-capture.js'; import { readFileSync, mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { EventEmitter } from 'node:events'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { SUBSTRATE_RECORD } from '../../../src/studio/substrate-acquire.js'; @@ -1720,6 +1721,80 @@ describe('runStudio — the acquired substrate is the only launch target', () => }); }); +/** + * THE ASYNCHRONOUS SPAWN FAILURE, which is a different failure from the try/catch already pinned + * above. + * + * `spawn()` does NOT throw for EACCES, EPERM or a Gatekeeper refusal — it returns a child and + * reports the failure by emitting `'error'` on a later tick, so `runStudio`'s try/catch, which can + * only ever see a synchronous throw, is not in that path at all. An `'error'` event with no + * listener is rethrown by `EventEmitter` as an uncaught exception, and the child is `unref`'d and + * detached by then, so the exception lands on a CLI process that has already returned to the + * human's shell. + * + * `runStudio` has attached that listener since it was written; nothing asserted it. `fakeSpawn` + * returns `{ on: () => undefined }`, so deleting the listener leaves all six arms above green + * while the process-killing shape reopens. `defaultLaunch` carries this exact pin in + * tests/unit/studio/auto-launch.test.ts, and the asymmetry was the gap. + * + * THE FAKE IS A REAL `EventEmitter` ON PURPOSE. The throw it produces with no listener attached is + * Node's own behaviour rather than a simulation of it, which is what lets `rethrown` stand in for + * "the CLI process would have died here" without actually killing the runner. + */ +describe('runStudio — a spawn that fails asynchronously must not kill the process', () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), 'wig-runstudio-err-')); + }); + + afterEach(() => { + rmSync(dataDir, { recursive: true, force: true }); + }); + + /** A stand-in child that records the state of its own listeners at `unref()` time. */ + function fakeChild(order: string[]): EventEmitter & { unref(): void } { + const child = new EventEmitter() as EventEmitter & { unref(): void }; + child.unref = () => order.push(`unref:errorListeners=${child.listenerCount('error')}`); + return child; + } + + it('attaches the error listener BEFORE unref, so there is no window', () => { + plantRecord(dataDir); + const order: string[] = []; + const child = fakeChild(order); + + runStudio([], { dataDir, spawnFn: () => child, log: () => undefined }); + + // Not merely "a listener exists by the time the test looks" — it existed at the one moment the + // command hands the child away, detaches it and stops being able to attach anything. + expect(order).toEqual(['unref:errorListeners=1']); + }); + + it('reports the failure on the human line instead of rethrowing it as an uncaught exception', () => { + plantRecord(dataDir); + const lines: string[] = []; + const child = fakeChild([]); + + runStudio([], { dataDir, spawnFn: () => child, log: (m) => lines.push(m) }); + + // Emitted only now, and that ordering IS the defect being pinned: the failure arrives after + // `runStudio` has returned, so nothing on the synchronous path is still holding a catch for it. + let rethrown: unknown = null; + try { + child.emit('error', new Error('spawn EACCES')); + } catch (e) { + rethrown = e; + } + expect(rethrown).toBeNull(); + // …and it is not swallowed either: a human typed the command, so the reason the window never + // appeared has to reach them rather than vanishing with the detached child. + expect(lines.join('\n')).toMatch(/spawn EACCES/); + // Capability language holds on the failure line too — it is user-facing text. + expect(lines.join('\n')).not.toMatch(/electron|playwright|chromium/i); + }); +}); + describe('runStudio — the deleted workspace leaves no residue in the source', () => { it('src/cli/studio.ts references no apps/ workspace and no -w flag', () => { // The split left ZERO workspace surface: there is no `workspaces` key and no `apps/` on this From 47eb97c9105956408127fcce81aa9eae10e1e959 Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Fri, 28 Aug 2026 06:31:42 +0600 Subject: [PATCH 2/3] test(studio): pin '.' and '..' as versions, the two that feed rmSync its own root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing arm plants '../escape', so a predicate narrowed to !/[\\/]/ stays green — and that narrowing reads as a simplification. It is not: destDir is join(root, version) and the next statement is a recursive rmSync, so '.' deletes every installed version plus the record and '..' deletes the data dir, cache DB, keys and profiles. Neither carries a separator. The fixture half-uninstalls first because a valid record short-circuits to already_present before destDir is ever computed; record-present-executable-gone is the reachable state, and is the one this file already names elsewhere. Asserts what survived rather than the outcome word: under the narrowed predicate '..' still ends in 'failed' — after wiping the data dir, then failing to write the record into the substrate/ directory it just deleted. --- tests/unit/studio/substrate-acquire.test.ts | 66 +++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/unit/studio/substrate-acquire.test.ts b/tests/unit/studio/substrate-acquire.test.ts index 655b2fee..d3f2679c 100644 --- a/tests/unit/studio/substrate-acquire.test.ts +++ b/tests/unit/studio/substrate-acquire.test.ts @@ -13,6 +13,7 @@ import { substratePresent, substrateRoot, SUBSTRATE_PATH_ENV, + SUBSTRATE_RECORD, type SubstrateSource, } from '../../../src/studio/substrate-acquire.js'; @@ -283,6 +284,71 @@ describe('the version a record is filed under must be one directory name', () => expect(r.outcome).toBe('acquired'); expect(readSubstrateRecord(dataDir)?.version).toBe('1.2.3'); }); + + /** + * THE TWO VERSIONS THAT CARRY NO SEPARATOR AND STILL NAME SOMEWHERE ELSE. + * + * The arm above plants `../escape`, so it stays green against a predicate narrowed to + * `!/[\\/]/` — and that narrowing looks like a simplification, because "one directory name" and + * "contains no separator" read as the same rule. They are not. `destDir` is `join(root, version)` + * and the NEXT statement is `rmSync(destDir, { recursive: true, force: true })`, so: + * + * `.` → destDir IS the substrate root → deletes every installed version and the record + * `..` → destDir IS the data dir → deletes the cache DB, the keys and the profiles + * + * Neither spelling contains a separator, so neither is caught by the narrowed form, and both + * reach a recursive delete before anything is verified. + * + * WHY THE FIXTURE HALF-UNINSTALLS FIRST. With a valid record on disk `acquireSubstrate` returns + * `already_present` before it ever computes `destDir`, so a fully-healthy machine cannot reach + * the delete at all. The state that CAN is the one this file already names elsewhere — the + * record is still there and the executable it points at is gone — which is what an interrupted + * uninstall, a partial upgrade, or a first run against a populated root all look like. + * + * The assertions are about what survived, not about the outcome word, because `..` under the + * narrowed predicate still ends in `failed`: it destroys the data dir, then fails writing the + * record into the `substrate/` directory it just deleted. An outcome-only arm would go green on + * the very shape that wiped the machine. + */ + it.each(['.', '..'])('refuses version %j rather than making it the directory it deletes', async (version) => { + await acquireSubstrate({ dataDir, source: localPathSource(sourceDir) }); + const root = substrateRoot(dataDir); + const installed = join(root, '1.2.3'); + const exec = join(installed, 'bin', 'run'); + expect(readSubstrateRecord(dataDir)?.version).toBe('1.2.3'); + + // A data-dir neighbour that only `..` reaches — the cache DB stands in for everything the + // substrate root's PARENT holds and that acquisition has no business touching. + const neighbour = join(dataDir, 'cache.db'); + writeFileSync(neighbour, 'not-a-substrate'); + + // Half-uninstalled: the record survives, its executable does not, so the record reads as + // absent and the acquisition proceeds past the `already_present` gate. + rmSync(exec); + expect(readSubstrateRecord(dataDir)).toBeNull(); + + const evil: SubstrateSource = { + id: 'evil', + manifest: { version, executable: 'bin/run' }, + async install(destDir: string) { + mkdirSync(join(destDir, 'bin'), { recursive: true }); + writeFileSync(join(destDir, 'bin', 'run'), '#!/bin/sh\n'); + }, + }; + const r = await acquireSubstrate({ dataDir, source: evil }); + + expect(r.outcome).toBe('failed'); + // Nothing was deleted: the earlier install's directory, the record filed beside it, and the + // rest of the data dir are all still where they were. + expect(existsSync(installed)).toBe(true); + expect(existsSync(join(root, SUBSTRATE_RECORD))).toBe(true); + expect(existsSync(neighbour)).toBe(true); + expect(readFileSync(neighbour, 'utf-8')).toBe('not-a-substrate'); + // And the earlier record reads back the moment its executable returns — only possible because + // neither the record nor the directory it names was removed. + writeFileSync(exec, '#!/bin/sh\n'); + expect(readSubstrateRecord(dataDir)?.version).toBe('1.2.3'); + }); }); /** From 035b3770dddef2b594e964e60a36b6763dbd6bf2 Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Fri, 28 Aug 2026 06:31:53 +0600 Subject: [PATCH 3/3] test(studio): pin that findEscapingLink terminates on a contained link cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Termination rests on one fact: readdirSync(withFileTypes) stats without following, so a link to a directory reports isSymbolicLink() and is judged rather than descended. The framework fixture has contained directory links but none pointing at an ancestor, so the walk finished for reasons unrelated to the rule. 'self -> .' is a legal, contained tree that must install. Descending into contained directory links — which reads as making the walk more thorough — turns it into self/self/self/... The caller is acquireSubstrate on the warmup path, unattended and with no timeout of its own. Carries a per-test timeout so a lost rule reports as a failing test rather than a runner that stopped making progress. --- tests/unit/studio/substrate-acquire.test.ts | 53 +++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/unit/studio/substrate-acquire.test.ts b/tests/unit/studio/substrate-acquire.test.ts index d3f2679c..9a832fca 100644 --- a/tests/unit/studio/substrate-acquire.test.ts +++ b/tests/unit/studio/substrate-acquire.test.ts @@ -558,6 +558,59 @@ describe('the install refuses a tree whose links leave it', () => { }); }); +/** + * THE WALK TERMINATES, AND ONLY ONE LINE MAKES THAT TRUE. + * + * `findEscapingLink` walks the installed tree with `readdirSync(withFileTypes)`, which stats + * WITHOUT following, so a link to a directory reports `isSymbolicLink()` and never + * `isDirectory()` — it is judged as a link and is not pushed onto the queue. That single fact is + * the whole termination argument, and the arms above do not test it: the framework fixture has + * contained directory links, but none of them points at an ancestor, so the walk finishes for + * reasons that have nothing to do with the rule. + * + * `self -> .` is a LEGAL tree. It is contained, so it must install; a bundle can legitimately + * carry one (`Versions/Current -> .` shapes appear in the wild). Following directory links — + * swapping `entry.isDirectory()` for a `statSync(child).isDirectory()`, which reads as making the + * walk "more thorough" — turns it into `self/self/self/…` forever. The caller is `acquireSubstrate` + * on the warmup path, unattended and with no timeout of its own, so the failure is a warmup that + * never returns rather than a component that fails to install. + * + * Windows is skipped for the same reason as the arms above: creating a symlink there needs + * elevation. The per-test timeout is deliberate — if the rule is ever lost, this arm must report + * as a failing test rather than as a runner that stopped making progress. + */ +describe('the walk terminates on a link cycle that is contained', () => { + it.skipIf(process.platform === 'win32')( + 'installs a tree whose directory link points at its own parent', + async () => { + const src = mkdtempSync(join(tmpdir(), 'wigolo-substrate-cycle-')); + try { + mkdirSync(join(src, 'bin'), { recursive: true }); + writeFileSync(join(src, 'bin', 'run'), '#!/bin/sh\n'); + // The cycle: a directory link back to the directory that holds it. + symlinkSync('.', join(src, 'self')); + writeFileSync(join(src, 'substrate.json'), JSON.stringify({ version: '3.3.7', executable: 'bin/run' })); + + // CONTROL: the cycle is real — the link resolves to the directory it sits in, so a walk + // that descended into it would be re-reading its own parent. + expect(realpathSync(join(src, 'self'))).toBe(realpathSync(src)); + + const r = await acquireSubstrate({ dataDir, source: localPathSource(src) }); + + // Contained is contained: this is an ACCEPT, not a refusal that happens to terminate. + expect(r.outcome).toBe('acquired'); + expect(readSubstrateRecord(dataDir)?.version).toBe('3.3.7'); + // The link survived the copy as a link, so the installed tree still carries the cycle the + // walk had to survive — not a resolved directory that quietly removed it. + expect(readlinkSync(join(substrateRoot(dataDir), '3.3.7', 'self'))).toBe('.'); + } finally { + rmSync(src, { recursive: true, force: true }); + } + }, + 10_000, + ); +}); + describe('containment is answered by the filesystem, not by string comparison', () => { let outside: string;