* test(#2874): add failing-first gate for the executed-plan return Four rows from the matrix's red-first order. E3 pins the one early return, for the opencode family, where a void-shaped hole would otherwise survive unnoticed. E13 sweeps every runtime in the registry - enumerated from the registry rather than hardcoded, so a runtime added later cannot slip past. F2 proves absence of real filesystem contact rather than merely that the happy path ran, which is the difference between a complete seam and a partial one. G1 and G3 are the additive guard and must be green before and after. G3 deliberately leaves the two existing adapter test doubles untouched: if this change required editing them it would not be additive, and the acceptance criterion would be unmet. No production code. All 19 runtimes install without throwing today, so E3 and E13 fail on the undefined comparison alone. Refs #2874 * feat(#2874): return the executed plan and route install IO through a seam installRuntimeArtifacts returned void, so its correctness was observable only by re-reading disk. It now returns what it executed - per kind, per scope - including on the combinedFamilyInstall path, which was the one early return where a void-shaped hole would have survived unnoticed. Failure still throws rather than becoming an ok:false return, so control flow is unchanged for both existing callers. A best-effort cleanup that fails is still swallowed, but is now visible in the returned value rather than silently absent. The fs seam is ambient rather than threaded. Explicit deps through install-profiles and the 3000-line conversion module was impractical; the tradeoff, the synchronous-only re-entrancy assumption, the restore guarantee and the partial-adapter fallback trap are all documented at the seam. findInstallSourceRoot and its sibling stay unrouted by design - they locate the package's own source, not the install destination. readCmdNames keeps a second implementation because the standalone CLI that owns the original cannot require the compiled adapter without a build-order dependency on its own output. A parity test fails if the two ever disagree. Refs #2874 * chore(#2874): gitignore the new build artifact install-fs-adapter.cjs is tsc output from src/install-fs-adapter.cts, not a tracked source file. It was added to eslint's ignore list but not to .gitignore, so it landed as a tracked file - the third time this step of the new-.cts ripple has been missed on this epic. Refs #2874 * fix(#2874): close two seam leaks and correct a false comment A correctness review found the seam still leaked in two places, both subtler than the three already closed. readGsdCommandNames was routed when it should not have been: it reads the package's own commands directory, which a destination-fake is never seeded with, so under a fake adapter it returned an empty or wrong roster instead of failing loudly. It now reads real fs, matching the precedent already documented for findInstallSourceRoot. cleanupStagedSkills ran raw rmSync from a process exit handler, which is real filesystem work deferred past the point where withInstallFs has restored - the one thing the synchronous-only contract exists to exclude. Staging now captures the adapter that created each directory and cleanup replays it, so a real install cleans up exactly as before and a fake-staged path never reaches the real filesystem. Also corrected a comment claiming the migration reads were an unrouted, untested residual gap. They are routed and exercised; a comment understating the seam is as corrosive as one overstating it in a module whose trust rests on being honestly documented. Refs #2874 * test(#2874): migrate the exemplar group and cover the matrix AC3's exemplar migration lands in place: the qwen install group now asserts skills and agents destinations from the returned plan in one deepStrictEqual instead of probing the filesystem for each. Nine facts the old probes established were enumerated first. Two moved to the value assertion; seven were retained deliberately - per-file SKILL.md existence, the VERSION file written outside this function, the manifest content, and the post-uninstall absence checks all sit outside the plan's per-kind contract. A migration that quietly asserts less looks like a win and is a regression, so the enumeration is the guard rather than the line count. Also implements the rest of the matrix: the executed-plan shape, adapter failure modes, the security-boundary rows including a fake that cannot certify an install the real filesystem would refuse, cleanup visibility, and two seeded property tests. Only the two external CI gates are left unticked, because self-certifying them would be a claim rather than a check. Refs #2874 * fix(#2874): restore streaming hashes and derive F2 from the boundary rule The checkpoint found three things reasoning had missed. sha256File had been converted from raw-fd streaming to a single readFileSync on the assumption that GSD artifacts are never large. A test named for exactly that contract already existed and went red. Streaming is restored, now routed through the adapter, which gains openSync, readSync and closeSync. The contract was the specification; the assumption was not. Three existing tests inject faults by monkeypatching real fs. They broke because mkInstallTempDir stopped calling real mkdtempSync, not because of any binding subtlety - the real adapter was already late-bound. It now calls the real function when no fake is injected, so a monkeypatch applied after import is still seen and the additive contract holds. F2 poisoned real fs by method, so a deliberately unrouted package-source read failed a correct design. It now poisons by path: destination IO is forbidden, package-source IO is allowed and positively asserted. The claim was always zero real destination IO, and the test now derives from that rule instead of coincidentally matching it. Refs #2874 * docs(#2874): add the contributor how-to for plan-based test migration The phase gate caught a real gap. The docs plan was Reference plus Explanation only, and every CI check would have passed, because the docs-required lint only verifies that some file under docs/ moved. But this phase exists to demonstrate a pattern for follow-on work, and that work is other contributors migrating probing test groups. The sequence has two live traps - a partial fake silently falls back to real fs, and the seam is ambient and synchronous-only - plus one discipline nobody infers: enumerate the facts before converting, or you assert less and call it a win. The page carries the qwen migration's arithmetic, nine facts enumerated and only two converted, because a reader seeing only the diff would reasonably conclude the pattern is to replace probes wholesale. No locale mirrors: none of the four carries any contributor-only how-to, so a single translated file would manufacture parity rather than provide it. Refs #2874 * chore(#2874): backfill changeset pr number * test(#2874): normalize both sides of the G1 tree comparison G1 failed on Windows only, deterministically on both shards. The defect was in the test helper, not production. _computePathPrefix posix-normalizes the resolved config dir unconditionally, so on Windows the path embedded in every emitted SKILL.md body is forward-slash form. hashDirTree stripped against the raw backslash path from mkdtempSync, so the substring never matched and each install's unique temp suffix stayed baked into every file - all fifteen skill bodies hashed differently for two runs that had written identical bytes. Both sides are now normalized unconditionally rather than gated on path.sep, matching the rule this repo already records: backslash paths arrive on Linux too. Production code is untouched and was verified correct. Normalizing this away on the production side would have hidden a real portability bug if one had existed. Refs #2874 --------- Co-authored-by: sim <sim@local>
252 lines
14 KiB
TypeScript
252 lines
14 KiB
TypeScript
'use strict';
|
|
|
|
/**
|
|
* Install Fs Adapter — #2874 (epic #2866 Phase 5), governed by ADR-58.
|
|
*
|
|
* Narrow, enumerated fs seam for the install/staging call tree rooted at
|
|
* `installRuntimeArtifacts` (install-engine.cts): layout source-root
|
|
* resolution (runtime-artifact-layout.cts), profile staging
|
|
* (install-profiles.cts), content-rewrite passes
|
|
* (runtime-artifact-conversion.cts), the CommonJS module-type marker
|
|
* (commonjs-marker.cts), and the two installer-migrations entry points this
|
|
* call tree reaches — `readInstallManifest` / `classifyArtifact`
|
|
* (installer-migrations.cts). Extends the `deps` bag precedent already
|
|
* established by `createRuntimeArtifactInstallPlan`
|
|
* (runtime-artifact-install-plan.cts:155) — this is NOT a general-purpose
|
|
* `node:fs` wrapper (40-design.md "Rejected" #1): only the operations this
|
|
* call tree actually performs are enumerated below. installer-migrations.cts
|
|
* is ~1200 lines covering migration planning/apply/rollback/locking/journal
|
|
* machinery unrelated to `installRuntimeArtifacts` — only its two reachable
|
|
* entry points (and `sha256File`, their shared hashing helper — still raw-fd
|
|
* streaming via `openSync`/`readSync`/`closeSync`, now routed through this
|
|
* seam instead of calling `node:fs` directly, so large-file hashing never
|
|
* buffers a whole file through the injected adapter either) are routed; the
|
|
* rest of that file is untouched, deliberately, because it is off this call
|
|
* tree.
|
|
*
|
|
* ─────────────────────────────────────────────────────────────────────────
|
|
* DELIVERY MECHANISM — ambient, not threaded (read this before adding a call
|
|
* site that needs a different adapter mid-call)
|
|
* ─────────────────────────────────────────────────────────────────────────
|
|
*
|
|
* A single mutable "current adapter" (`current`, below) is swapped for the
|
|
* duration of one synchronous `installRuntimeArtifacts` call via
|
|
* `withInstallFs`, rather than a `deps` parameter threaded through every
|
|
* function on the call tree. The rejected alternative was threading: it
|
|
* would touch signatures across `install-profiles.cts` (5 staging
|
|
* functions), the 3000+-line `runtime-artifact-conversion.cts` (rewrite
|
|
* passes), `commonjs-marker.cts`, and `installer-migrations.cts` — a dozen+
|
|
* unrelated call sites — for no behavioral gain over an ambient swap, and
|
|
* `createRuntimeArtifactInstallPlan`'s own `deps` bag (the precedent this
|
|
* seam extends) does not reach that deep either. Every fs-touching site on
|
|
* the call tree reads the active adapter via `installFs()` instead of
|
|
* importing `node:fs` directly.
|
|
*
|
|
* SYNCHRONOUS-ONLY / RE-ENTRANCY ASSUMPTION (load-bearing, not incidental):
|
|
* every method on this seam is `*Sync`, and `installRuntimeArtifacts` never
|
|
* awaits mid-call — the whole call tree from the top-level `withInstallFs`
|
|
* wrap down to the last `fs` touch runs on one turn of the event loop with
|
|
* no interleaving. That is what makes a single ambient variable safe instead
|
|
* of a race: nothing else can observe or mutate `current` while it is set.
|
|
* This assumption BREAKS if `installRuntimeArtifacts` (or anything it calls)
|
|
* ever becomes `async`, or if two installs run concurrently in the same
|
|
* process (the second `withInstallFs` call would clobber the first's
|
|
* adapter mid-flight) — neither is true today, but a future change that
|
|
* introduces either must revisit this module before trusting it.
|
|
*
|
|
* DEFERRED CLEANUP ACROSS THE RESTORE (#2874 leak-fix): staged directories
|
|
* outlive one `withInstallFs` call — `install-profiles.cts`'s
|
|
* `cleanupStagedSkills` runs later, from a `process.on('exit'/'SIGINT'/…)`
|
|
* handler, by which time `withInstallFs`'s `finally` has already restored
|
|
* `current` back to whatever was active before (real fs, in the top-level
|
|
* case). A cleanup handler that resolved "which adapter do I use" via
|
|
* `installFs()` at CLEANUP time would therefore always see the real adapter,
|
|
* even for a directory that was staged entirely inside a fake-adapter call —
|
|
* performing real filesystem IO on a path that only ever existed in the
|
|
* fake's in-memory store. `install-profiles.cts` avoids this by capturing
|
|
* the adapter OBJECT `installFs()` returns at STAGING (registration) time,
|
|
* keyed by path, in its own `STAGED_DIR_ADAPTERS` map, and replaying that
|
|
* captured object — not the ambient `current` — at cleanup time. A real
|
|
* install's dirs were staged with the real adapter object, so their cleanup
|
|
* is byte-identical to before this fix.
|
|
*
|
|
* `withInstallFs` ALWAYS restores the previous adapter in a `finally`, even
|
|
* on throw — a failed install (or a test that intentionally throws to prove
|
|
* a guard) must never leak a fake adapter into whatever runs next in the
|
|
* same process (e.g. the next `node:test` in a shared worker).
|
|
*
|
|
* ⚠️ PARTIAL-ADAPTER TRAP: `withInstallFs` merges the injected `partial`
|
|
* OVER the real adapter (`{ ...REAL_ADAPTER, ...partial }`) — any method the
|
|
* partial does not define resolves to REAL `node:fs`, silently. An
|
|
* incomplete fake is not a smaller fake adapter; for the methods it omits,
|
|
* it IS the real filesystem. A test asserting "no real IO happened" against
|
|
* a partial fake must either implement every method the exercised code path
|
|
* touches, or explicitly account for the ones it does not (see
|
|
* `tests/executed-plan.test.cjs`'s F2 test, which poisons real `fs` methods
|
|
* for exactly this reason — a gap here shows up as the poisoned method
|
|
* firing, not as a silent pass).
|
|
*
|
|
* ─────────────────────────────────────────────────────────────────────────
|
|
* SECURITY NOTE (40-design.md rows 6/7, H1-H5)
|
|
* ─────────────────────────────────────────────────────────────────────────
|
|
* This module carries NO policy. `hasExistingSymlinkBetween` and
|
|
* `assertDestWithinConfigHome` keep their REFUSAL DECISIONS outside this
|
|
* seam — only their probe calls (existsSync/lstatSync/realpathSync) are
|
|
* routed through it. Injecting a fake adapter can only change what those
|
|
* probes observe for paths that were never real to begin with; it cannot
|
|
* flip the decision logic itself.
|
|
*
|
|
* ─────────────────────────────────────────────────────────────────────────
|
|
* DELIBERATELY NOT ROUTED (see the call sites themselves for the full
|
|
* reasoning — this is the index)
|
|
* ─────────────────────────────────────────────────────────────────────────
|
|
* `findInstallSourceRoot` / `findAgentsSourceRoot`'s walk-up-from-__dirname
|
|
* step (runtime-artifact-layout.cts) locates THIS PACKAGE'S OWN source tree
|
|
* (`commands/gsd/`, `agents/`) — not the install destination — so it uses
|
|
* `fs.statSync` directly, unrouted. A fake destination adapter's store
|
|
* starts empty and is never seeded with real repo paths; routing this walk
|
|
* through it would make every fake-adapter install throw
|
|
* "could not locate commands/gsd", not gracefully stage nothing. See the
|
|
* comment at each function's Step 2 for the full argument.
|
|
*
|
|
* RULE (40-design.md "Known limits"): this seam makes *destination* IO
|
|
* fake-able; package-source IO (this section) stays real by design — the F2
|
|
* test's poison list should be derived FROM that rule, not the reverse, or a
|
|
* future "complete the poison list" edit that adds `statSync` will break a
|
|
* correct `findInstallSourceRoot` for the wrong reason.
|
|
*/
|
|
|
|
import nodeFs from 'node:fs';
|
|
import type { Dirent } from 'node:fs';
|
|
import os from 'node:os';
|
|
import path from 'node:path';
|
|
import crypto from 'node:crypto';
|
|
|
|
// Precisely typed (unlike install-engine.cts's own house style) so every
|
|
// OTHER file this adapter is imported into — install-profiles.cts,
|
|
// runtime-artifact-layout.cts, retired-artifact-cleanup.cts,
|
|
// command-roster.cts, commonjs-marker.cts, installer-migrations.cts — none
|
|
// of which blanket-disable the `no-unsafe-*` rules — keeps its existing
|
|
// strict typing at each call site instead of degrading to `any` through
|
|
// this seam.
|
|
interface InstallFsAdapter {
|
|
existsSync(p: string): boolean;
|
|
mkdirSync(p: string, opts?: { recursive?: boolean }): string | undefined;
|
|
rmSync(p: string, opts?: { recursive?: boolean; force?: boolean }): void;
|
|
readdirSync(p: string, opts: { withFileTypes: true }): Dirent[];
|
|
readdirSync(p: string): string[];
|
|
readFileSync(p: string, encoding: BufferEncoding): string;
|
|
readFileSync(p: string): Buffer;
|
|
writeFileSync(p: string, data: string | Buffer, opts?: BufferEncoding | { encoding?: BufferEncoding; flag?: string }): void;
|
|
copyFileSync(src: string, dest: string): void;
|
|
cpSync(src: string, dest: string, opts?: { recursive?: boolean }): void;
|
|
lstatSync(p: string): { isFile(): boolean; isDirectory(): boolean; isSymbolicLink(): boolean };
|
|
/** Optional — a partial injected adapter that omits this falls back to the
|
|
* real fs.realpathSync (merged in by `withInstallFs`). The symlink guard
|
|
* already treats a realpathSync FAILURE as "fall back to the lexical
|
|
* form" (see its own doc comment); an absent method degrades the same
|
|
* way — never a hard failure. */
|
|
realpathSync(p: string): string;
|
|
unlinkSync(p: string): void;
|
|
rmdirSync(p: string): void;
|
|
/** Raw-fd streaming trio, added so `installer-migrations.cts`'s
|
|
* `sha256File` can hash a file in fixed-size chunks through this seam
|
|
* instead of buffering the whole file via `readFileSync` — see the
|
|
* module doc's opening paragraph and `tests/installer-migrations.test.cjs`'s
|
|
* "classifies large files without loading the whole file through
|
|
* readFileSync", which pins this contract directly: it monkeypatches real
|
|
* `fs.readFileSync` to throw for the file under test and asserts hashing
|
|
* still succeeds, proving the hash path never calls it. */
|
|
openSync(p: string, flags: string): number;
|
|
readSync(fd: number, buffer: Buffer, offset: number, length: number, position: number | null): number;
|
|
closeSync(fd: number): void;
|
|
}
|
|
|
|
const REAL_ADAPTER: InstallFsAdapter = {
|
|
existsSync: (p) => nodeFs.existsSync(p),
|
|
mkdirSync: (p, opts) => nodeFs.mkdirSync(p, opts),
|
|
rmSync: (p, opts) => nodeFs.rmSync(p, opts),
|
|
readdirSync: ((p: string, opts?: { withFileTypes: true }) =>
|
|
(opts ? nodeFs.readdirSync(p, opts) : nodeFs.readdirSync(p))) as InstallFsAdapter['readdirSync'],
|
|
readFileSync: ((p: string, encoding?: BufferEncoding) =>
|
|
(encoding ? nodeFs.readFileSync(p, encoding) : nodeFs.readFileSync(p))) as InstallFsAdapter['readFileSync'],
|
|
writeFileSync: (p, data, opts) => nodeFs.writeFileSync(p, data, opts),
|
|
copyFileSync: (src, dest) => nodeFs.copyFileSync(src, dest),
|
|
cpSync: (src, dest, opts) => nodeFs.cpSync(src, dest, opts),
|
|
lstatSync: (p) => nodeFs.lstatSync(p),
|
|
realpathSync: (p) => nodeFs.realpathSync(p),
|
|
unlinkSync: (p) => nodeFs.unlinkSync(p),
|
|
rmdirSync: (p) => nodeFs.rmdirSync(p),
|
|
openSync: (p, flags) => nodeFs.openSync(p, flags),
|
|
readSync: (fd, buffer, offset, length, position) => nodeFs.readSync(fd, buffer, offset, length, position),
|
|
closeSync: (fd) => nodeFs.closeSync(fd),
|
|
};
|
|
|
|
let current: InstallFsAdapter = REAL_ADAPTER;
|
|
|
|
/**
|
|
* Returns the fs adapter active for the currently-running install call —
|
|
* the real adapter when no `deps.fs` was injected, or the injected partial
|
|
* adapter merged over the real one (any method it did not override still
|
|
* resolves to real fs — see the module doc's "PARTIAL-ADAPTER TRAP").
|
|
*/
|
|
function installFs(): InstallFsAdapter {
|
|
return current;
|
|
}
|
|
|
|
/**
|
|
* Run `fn` with `partial` merged over the real adapter as the active
|
|
* install-fs adapter, restoring the previous adapter afterward — even on
|
|
* throw (see the module doc's re-entrancy/synchronous-only assumption for
|
|
* why a bare module-level variable is safe here, and why it would not be
|
|
* under async interleaving or concurrent installs). `partial` undefined is
|
|
* a no-op: `fn` runs against whatever adapter was already active (real fs
|
|
* by default) — this is what keeps every existing `deps`-less call site
|
|
* (AC4) byte-identical.
|
|
*/
|
|
function withInstallFs<T>(partial: Partial<InstallFsAdapter> | undefined, fn: () => T): T {
|
|
if (!partial) return fn();
|
|
const previous = current;
|
|
current = { ...REAL_ADAPTER, ...partial };
|
|
try {
|
|
return fn();
|
|
} finally {
|
|
current = previous;
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Create a fresh, uniquely-named temp directory.
|
|
*
|
|
* AC4 requires the no-adapter-injected path to stay byte-identical to the
|
|
* pre-#2874 code, which called real `fs.mkdtempSync` directly — several
|
|
* existing tests (e.g. tests/install-runtime-artifacts.test.cjs's "rmSync is
|
|
* called on the tempDir when readFileSync throws") monkeypatch real
|
|
* `fs.mkdtempSync` to capture the exact directory a call under test creates,
|
|
* and stop working if that real syscall is no longer made. So: when NO fake
|
|
* adapter is active (`current === REAL_ADAPTER`, the exact top-level-install
|
|
* default), this calls `nodeFs.mkdtempSync` directly — the same real call
|
|
* the old code made, still visible to a monkeypatch applied after import
|
|
* because it is a live property lookup on the `node:fs` module object, not a
|
|
* captured reference.
|
|
*
|
|
* When a fake adapter IS active (any `withInstallFs(partial, …)` call, even
|
|
* a partial one — see `current`'s assignment in `withInstallFs`, which
|
|
* always produces a NEW merged object, never `=== REAL_ADAPTER`), this falls
|
|
* back to synthesizing a unique name and creating it via
|
|
* `installFs().mkdirSync`, exactly as before: the injected adapter contract
|
|
* still does not require `mkdtempSync`, so a fake (which only needs to
|
|
* implement `mkdirSync`) can drive the full staging pipeline without ever
|
|
* touching real fs.
|
|
*/
|
|
function mkInstallTempDir(prefix: string): string {
|
|
if (current === REAL_ADAPTER) {
|
|
return nodeFs.mkdtempSync(path.join(os.tmpdir(), prefix));
|
|
}
|
|
const dir = path.join(os.tmpdir(), `${prefix}${crypto.randomBytes(8).toString('hex')}`);
|
|
installFs().mkdirSync(dir, { recursive: true });
|
|
return dir;
|
|
}
|
|
|
|
export = { installFs, withInstallFs, mkInstallTempDir };
|