From 52f4ea17cc39daf61cdf3467b3909daae120fd45 Mon Sep 17 00:00:00 2001 From: sim Date: Fri, 14 Aug 2026 21:53:48 -0400 Subject: [PATCH] feat(#2873): project installed surfaces into a shadow report New read-only leaf src/install-shadow-report.cts turns Phase 3's resolveInstalledSurfaces output into a typed shadow-report IR plus a line-array renderer, with declaredRuntime sanitized at the render seam (ANSI, C0/C1, newlines, bidi overrides; idempotent, no second truncation over the reader's 64-char cap). Also closes the symlink asymmetry the resolver carried: the local scope resolves against process.cwd(), and this phase is what makes that path reachable from an arbitrary cloned repository, so the manifest read is now lstat-guarded rather than following. Matches the getAgentsDir precedent and degrades to the same installed:false shape the EACCES path already returned. Refs #2873 --- .gitignore | 1 + src/install-shadow-report.cts | 298 +++++++++++++++++++++++++++++ src/installed-surface-resolver.cts | 81 ++++++-- 3 files changed, 368 insertions(+), 12 deletions(-) create mode 100644 src/install-shadow-report.cts diff --git a/.gitignore b/.gitignore index c35e537e0..c70d11541 100644 --- a/.gitignore +++ b/.gitignore @@ -218,6 +218,7 @@ build/ /gsd-core/bin/lib/runtime-artifact-layout.cjs /gsd-core/bin/lib/install-scope.cjs /gsd-core/bin/lib/installed-surface-resolver.cjs +/gsd-core/bin/lib/install-shadow-report.cjs /gsd-core/bin/lib/runtime-config-adapter-registry.cjs /gsd-core/bin/lib/runtime-hooks-surface.cjs /gsd-core/bin/lib/command-routing-hub.cjs diff --git a/src/install-shadow-report.cts b/src/install-shadow-report.cts new file mode 100644 index 000000000..9782f7557 --- /dev/null +++ b/src/install-shadow-report.cts @@ -0,0 +1,298 @@ +/** + * install-shadow-report.cts — Cross-Scope Shadow Report Module (#2873, epic + * #2866 Phase 4a — governed by + * `.gsd/phase/feat-2873-cross-scope-shadowing/40-design.md`). + * + * A read-only PROJECTION over `resolveInstalledSurfaces` + * (`installed-surface-resolver.cts`, #2872 Phase 3). That module answers + * "what is installed"; it is documented there as read-only, and rendering + * plus sanitization are a different concern with a different consumer set + * (installer + `/gsd-health`) — the design doc's "Rejected" #5 is why this is + * a separate leaf module rather than a second export bolted onto the + * resolver. + * + * ── What "shadowed" means here ────────────────────────────────────────────── + * A trigger is shadowed when `resolveTriggerSurface` (via the resolver) + * recorded a non-null `shadowedBy` for it: two scopes both installed a + * trigger-bearing artifact under the SAME trigger name, and only one wins. + * For claude (`skills`@global vs `commands`@local) the KINDS differ, so the + * loser's entire spec tree becomes unreachable through the trigger — the bug + * #2218 diagnosed. For the 12 both-scopes-`skills` runtimes the kinds are the + * SAME on both sides, so the loser is merely overridden, not vanished + * (design row #5 / "Not-corruption"). `kindsDiffer` on `ShadowReport` is what + * lets `renderShadowReport` word the two cases correctly. + * + * ── Report, don't correct (mirrors the resolver's own law) ───────────────── + * `mismatches` surfaces a declared runtime/scope that disagrees with the + * probed one (Postel's Law, design doc: liberal in what is accepted, but the + * mismatch is never silently absorbed). This module never substitutes a + * declared value for a probed one; it only reports the disagreement the + * resolver already computed. + * + * ── Sanitize at the render seam ───────────────────────────────────────────── + * `declaredRuntime` is attacker-influenceable (it comes from a manifest that + * may live inside a merely-cloned repository) and length-bounded but + * deliberately NOT charset-gated by the reader (`declaredRuntimeMatchesProbe` + * needs the raw value there). THIS module is what renders it to an operator, + * so this module owns the guard — `sanitizeForRender` strips ANSI escapes, + * C0/C1 controls, and Unicode bidi overrides/isolates, then collapses + * whitespace. It never truncates: `readInstallManifest` already caps at 64 + * chars, and a second truncation here would double-truncate. + * + * Trigger names, by contrast, are already `SAFE_STEM`-gated upstream + * (`installed-surface-resolver.cts`'s `deriveStemsForKindEntry`) before they + * ever reach a `TriggerSurface` — this module does not re-gate them. + * + * ── Pure with respect to caller-visible state ─────────────────────────────── + * `buildShadowReport` builds a fresh `ShadowReport` (fresh arrays, fresh + * objects) on every call, exactly as the resolver documents for itself + * (`installed-surface-resolver.cts`'s "Pure with respect to caller-visible + * state" paragraph) — no shared or cached state between calls. + */ + +import { type InstallScope } from './install-scope.cjs'; +import { + resolveInstalledSurfaces, + type ResolveInstalledSurfacesOptions, + type InstalledRuntimeSurface, +} from './installed-surface-resolver.cjs'; + +// ── Reason enum ───────────────────────────────────────────────────────── + +export const SHADOW_REASON = Object.freeze({ + NOT_SHADOWED: 'not_shadowed', + SCOPE_SHADOWED: 'scope_shadowed', + RESOLVER_UNAVAILABLE: 'resolver_unavailable', +}); + +// ── Public types ──────────────────────────────────────────────────────── + +export interface ShadowedTrigger { + trigger: string; + winnerKind: string; + winnerScope: InstallScope; + shadowedKind: string; + shadowedScope: InstallScope; +} + +export interface DeclarationMismatch { + scope: InstallScope; + declaredRuntime: string | null; + declaredRuntimeMatchesProbe: boolean | null; + declaredScope: InstallScope | null; + declaredScopeMatchesProbe: boolean | null; +} + +export interface ShadowReport { + runtime: string; + reason: string; + shadowed: boolean; + winner: { kind: string; scope: InstallScope } | null; + shadowedSide: { kind: string; scope: InstallScope } | null; + kindsDiffer: boolean; + triggers: ShadowedTrigger[]; + mismatches: DeclarationMismatch[]; +} + +// ── Sanitization ──────────────────────────────────────────────────────── + +/** CSI (`\x1b[...final`) and OSC (`\x1b]...BEL-or-ST`) sequences. An + * unterminated/malformed sequence is left for the C0-control strip below to + * remove the bare `\x1b` byte — liberal, never a throw. */ +const ANSI_RE = /\x1b(?:\[[0-?]*[ -/]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\))/g; + +/** C0 controls (`\x00`-`\x1f`, including any `\x1b` the ANSI strip above did + * not consume) and DEL/C1 (`\x7f`-`\x9f`). */ +const CONTROL_RE = /[\x00-\x1f\x7f-\x9f]/g; + +/** Unicode bidi embedding/override controls (U+202A-U+202E) and bidi + * isolates (U+2066-U+2069) — the RTL-spoofing class the design doc's row + * #13 names. */ +const BIDI_RE = /[‪-‮⁦-⁩]/g; + +/** + * Sanitize a `declaredRuntime` (or any similarly attacker-influenceable + * string) for terminal/console rendering. `null` passes through as `null`; + * `''` passes through as `''`. Strips ANSI escapes, C0/C1 controls, and + * Unicode bidi overrides/isolates (replacing each stripped run with + * nothing — never a space), then collapses any remaining whitespace run + * (including adjacent spaces left behind by a removed newline) to a single + * space and trims. + * + * Idempotent by construction: once ANSI/control/bidi bytes are gone and + * whitespace is collapsed to single internal spaces with no leading/ + * trailing space, a second pass finds nothing left to strip or collapse. + * Never truncates — `readInstallManifest` already caps at 64 chars. + */ +export function sanitizeForRender(value: string | null): string | null { + if (value === null) return null; + const stripped = value + .replace(ANSI_RE, '') + .replace(CONTROL_RE, '') + .replace(BIDI_RE, ''); + return stripped.replace(/\s+/g, ' ').trim(); +} + +// ── Report builder ────────────────────────────────────────────────────── + +/** + * Build a shadow report for one runtime. `opts` is forwarded VERBATIM to + * `resolveInstalledSurfaces` — this function adds no option of its own. + * Production call shape: `buildShadowReport('claude', { home, cwd })`. + * + * A `resolveInstalledSurfaces` `TypeError` (unknown runtime, or + * `configHome.kind === 'none'`, e.g. vscode — design row #7) degrades to + * `reason: RESOLVER_UNAVAILABLE` rather than propagating: an install-time or + * `/gsd-health` caller must never crash because a runtime has no installable + * config directory. Any other error type is rethrown — mirrors the + * resolver's own `TypeError` narrowing (`resolveInstalledSurfaces`'s sweep + * catch, and `buildScopeRecord`'s stem-derivation catch) so the two cannot + * drift apart. + */ +export function buildShadowReport(runtime: string, opts: ResolveInstalledSurfacesOptions = {}): ShadowReport { + let surfaces: InstalledRuntimeSurface[]; + try { + surfaces = resolveInstalledSurfaces(runtime, opts); + } catch (error) { + if (!(error instanceof TypeError)) throw error; + return { + runtime, + reason: SHADOW_REASON.RESOLVER_UNAVAILABLE, + shadowed: false, + winner: null, + shadowedSide: null, + kindsDiffer: false, + triggers: [], + mismatches: [], + }; + } + + // resolveInstalledSurfaces(runtime, opts) with an explicit string `runtime` + // always returns exactly one element (see its own doc comment). + const surface = surfaces[0]; + + const shadowedSurfaces = surface.triggers.filter((t) => t.shadowedBy !== null); + const triggers: ShadowedTrigger[] = shadowedSurfaces + .map((t) => ({ + trigger: t.trigger, + // `shadowedBy` is non-null by construction of the filter above. + winnerKind: t.shadowedBy!.kind, + winnerScope: t.shadowedBy!.scope, + shadowedKind: t.kind, + shadowedScope: t.scope, + })) + .sort((a, b) => (a.trigger < b.trigger ? -1 : a.trigger > b.trigger ? 1 : 0)); + + const mismatches: DeclarationMismatch[] = []; + for (const record of surface.scopes) { + if (record.declaredRuntimeMatchesProbe === false || record.declaredScopeMatchesProbe === false) { + mismatches.push({ + scope: record.scope, + // Postel's Law (design doc): sanitized here because this is the + // render seam — never silently absorbed, always surfaced. + declaredRuntime: sanitizeForRender(record.declaredRuntime), + declaredRuntimeMatchesProbe: record.declaredRuntimeMatchesProbe, + declaredScope: record.declaredScope, + declaredScopeMatchesProbe: record.declaredScopeMatchesProbe, + }); + } + } + + if (triggers.length === 0) { + return { + runtime, + reason: SHADOW_REASON.NOT_SHADOWED, + shadowed: false, + winner: null, + shadowedSide: null, + kindsDiffer: false, + triggers: [], + mismatches, + }; + } + + // Winner/shadowedSide are the (kind,scope) pair of the FIRST shadowed + // trigger (post-sort, for the same determinism reason the array itself is + // sorted). They are asserted-by-construction uniform across the whole set + // for every runtime this module has seen (every trigger shadowed by the + // SAME scope, with the SAME two kinds, on one machine) — but if a future + // registry shape ever produced a non-uniform set, this still returns the + // first pair rather than throwing; every distinct (kind,scope) pair is + // already visible per-entry in `triggers` itself, so nothing is lost. + const first = triggers[0]; + const winner = { kind: first.winnerKind, scope: first.winnerScope }; + const shadowedSide = { kind: first.shadowedKind, scope: first.shadowedScope }; + + return { + runtime, + reason: SHADOW_REASON.SCOPE_SHADOWED, + shadowed: true, + winner, + shadowedSide, + kindsDiffer: winner.kind !== shadowedSide.kind, + triggers, + mismatches, + }; +} + +// ── Renderer ──────────────────────────────────────────────────────────── + +/** + * Render a `ShadowReport` to plain lines — no ANSI, no color, no leading + * indent. The caller (installer console output, `/gsd-health` text mode) + * owns terminal formatting; this keeps the module free of terminal concerns + * and testable without a spawned process. Structured (`--json`) health + * output (design row #17) consumes the typed `ShadowReport` directly and + * never calls this function. + * + * `reason !== SCOPE_SHADOWED` renders nothing — there is nothing to report + * (design rows #1, #2, #6, #7, #8, #11). + */ +export function renderShadowReport(report: ShadowReport, opts: { sampleLimit?: number } = {}): string[] { + if (report.reason !== SHADOW_REASON.SCOPE_SHADOWED || report.winner === null || report.shadowedSide === null) { + return []; + } + + const sampleLimit = opts.sampleLimit ?? 5; + const count = report.triggers.length; + const plural = count === 1 ? '' : 's'; + const { winner, shadowedSide, kindsDiffer } = report; + + const lines: string[] = []; + lines.push( + kindsDiffer + ? `${count} trigger${plural} shadowed: the ${shadowedSide.scope} ${shadowedSide.kind} surface is unreachable through ${count === 1 ? 'that trigger' : 'those triggers'} — ${winner.scope} ${winner.kind} wins instead.` + : `${count} trigger${plural} shadowed: the ${shadowedSide.scope} ${shadowedSide.kind} ${count === 1 ? 'entry is' : 'entries are'} overridden by ${winner.scope} ${winner.kind}.`, + ); + + // Trigger names in `report.triggers` are already SAFE_STEM-gated upstream + // (installed-surface-resolver.cts's deriveStemsForKindEntry) — no re-gating + // needed here. + const sample = report.triggers.slice(0, sampleLimit); + for (const t of sample) { + lines.push(` - ${t.trigger}: ${t.shadowedScope}/${t.shadowedKind} shadowed by ${t.winnerScope}/${t.winnerKind}`); + } + const remaining = count - sample.length; + if (remaining > 0) { + lines.push(` ...and ${remaining} more`); + } + + for (const m of report.mismatches) { + // Re-sanitized defensively: `buildShadowReport` already sanitizes + // `declaredRuntime` before it reaches a `ShadowReport`, and + // `sanitizeForRender` is idempotent, so this is a no-op in the normal + // path and a real guard against a hand-built `ShadowReport` (e.g. a + // renderer-only test) that skipped it. + const declaredRuntime = sanitizeForRender(m.declaredRuntime); + const parts: string[] = []; + if (m.declaredRuntimeMatchesProbe === false) { + parts.push(`declared runtime "${declaredRuntime}" does not match this runtime`); + } + if (m.declaredScopeMatchesProbe === false) { + parts.push(`declared scope "${m.declaredScope}" does not match the probed ${m.scope} scope`); + } + lines.push(`Note: ${m.scope} scope manifest mismatch — ${parts.join('; ')}.`); + } + + return lines; +} diff --git a/src/installed-surface-resolver.cts b/src/installed-surface-resolver.cts index a1b2c4c10..b804a72c7 100644 --- a/src/installed-surface-resolver.cts +++ b/src/installed-surface-resolver.cts @@ -53,6 +53,8 @@ * than re-deriving either rule as a fourth independent copy. */ +import fs from 'node:fs'; +import path from 'node:path'; import { resolveScope, SCOPE_ORDER, type InstallScope } from './install-scope.cjs'; import { posixNormalize } from './shell-command-projection.cjs'; @@ -68,7 +70,7 @@ const { // eslint-disable-next-line @typescript-eslint/no-require-imports import installerMigrationsMod = require('./installer-migrations.cjs'); -const { readInstallManifest } = installerMigrationsMod; +const { readInstallManifest, MANIFEST_NAME } = installerMigrationsMod; // In .cts (CommonJS output) files, `require` is available as a global. const _require: NodeRequire = require; @@ -130,6 +132,12 @@ export interface ResolveInstalledSurfacesOptions { cwd?: string; env?: Record; existsSync?: (p: string) => boolean; + /** Injected for tests, matching `existsSync` above; defaults to + * `node:fs`'s `lstatSync`. Used to refuse a manifest read when the + * scope's config dir or manifest file is a symlink — see the + * `buildScopeRecord` comment for why this is a deliberate hardening, + * not an oversight. */ + lstatSync?: (p: string) => { isSymbolicLink(): boolean }; registry?: unknown; /** Injected for tests; defaults to installer-migrations' readInstallManifest. */ readManifest?: (configDir: string) => { @@ -252,6 +260,45 @@ function deriveStemsFromManifest( return [...stems].sort(); } +/** + * True when `p` is a symlink. An `lstatSync` throw (ENOENT — nothing at this + * path) is NOT evidence of a symlink; it is treated as "not a symlink" here + * and left for `readManifest` to classify (it already owns the absent-file + * case, per C14 above). + */ +function isSymlinkPath(p: string, lstatSync: (p: string) => { isSymbolicLink(): boolean }): boolean { + try { + return lstatSync(p).isSymbolicLink(); + } catch { + return false; + } +} + +/** + * The shared "not installed" degraded shape (C14's EACCES path, and this + * phase's new symlink-guard path). A FACTORY, not a module-level constant + * object: a `const` object spread at each return site would still share the + * same `stems` ARRAY reference across every call (`...` shallow-copies the + * object but not the array a property points at), which would violate this + * module's own "builds fresh arrays/objects on every call" contract (C15) — + * a caller mutating one degraded record's `stems` must never be visible on + * another's. + */ +function notInstalledScopeRecordFields(): Pick< + InstalledScopeRecord, + 'installed' | 'manifestVersion' | 'declaredRuntime' | 'declaredScope' | 'declaredScopeMatchesProbe' | 'declaredRuntimeMatchesProbe' | 'stems' +> { + return { + installed: false, + manifestVersion: null, + declaredRuntime: null, + declaredScope: null, + declaredScopeMatchesProbe: null, + declaredRuntimeMatchesProbe: null, + stems: [], + }; +} + /** * Build one scope's record. `resolvedConfigHome` has already been probed * successfully by the time this is called (a `resolveScope` `TypeError` is @@ -278,6 +325,26 @@ function buildScopeRecord( resolvedConfigHome: string, opts: ResolveInstalledSurfacesOptions, ): InstalledScopeRecord { + // Hardening requirement 2 (#2873 design doc, "Hardening requirements + // claimed from #2873's comment"): `readInstallManifest` -> `readJsonIfPresent` + // uses `existsSync` + `readFileSync` and therefore FOLLOWS symlinks, and + // this module resolves `local` against `process.cwd()` — a directory that, + // as of this phase, becomes reachable from an arbitrary cloned repository + // (this is the same phase that makes the local scope's manifest a first + // read target, not merely a write target). The in-tree precedent is + // `getAgentsDir` (`agent-install-check.cts`), which probes with + // `fs.lstatSync(...).isDirectory()`/`.isFile()` and deliberately does not + // follow. #2872 left this resolver's read un-guarded only because the path + // had zero callers at the time; refusing to follow a symlinked config dir + // or manifest here closes that asymmetry rather than carrying it forward. + // Degrading to `installed: false` reuses the SAME shape the EACCES catch + // below already returns — no new failure shape is introduced. + const lstatSync = opts.lstatSync ?? fs.lstatSync; + const manifestPath = path.join(resolvedConfigHome, MANIFEST_NAME); + if (isSymlinkPath(resolvedConfigHome, lstatSync) || isSymlinkPath(manifestPath, lstatSync)) { + return { scope: scopeId, configHome: resolvedConfigHome, ...notInstalledScopeRecordFields() }; + } + let manifest: { manifestVersion: number | null; runtime: string | null; @@ -293,17 +360,7 @@ function buildScopeRecord( // read failure at all (EACCES, ENOENT-after-race, a corrupt filesystem) // legitimately means "cannot tell whether installed" (design row C14), so // there is no error TYPE here that should instead propagate. - return { - scope: scopeId, - configHome: resolvedConfigHome, - installed: false, - manifestVersion: null, - declaredRuntime: null, - declaredScope: null, - declaredScopeMatchesProbe: null, - declaredRuntimeMatchesProbe: null, - stems: [], - }; + return { scope: scopeId, configHome: resolvedConfigHome, ...notInstalledScopeRecordFields() }; } const installed = manifest.manifestVersion !== null; // C9: presence, never the new fields