From 3994ddfdd7c66e37866659f0b0b775c781fd5254 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 8 Sep 2026 18:55:37 +0000 Subject: [PATCH] fix(design-detect): audit directories scan as dumps; scans print probe lines on stderr; refused base always exits 1; PATH loses project entries Adversarial review (Claude subagent): - A DIRECTORY target under designs/ (the audit dir, which the prose hands the agent as REPORT_DIR) classified as an artifact, so the engine walked its dom/ subtree WITH inline ignores honored. Any directory under designs/ is now scanned as dumps. - A scan whose probe no longer finds an engine wrote its sentinel lines to stdout and exited 0, so `scan > "$_DJ"` captured "IMPECCABLE_NOT_AVAILABLE" as the scan result and the rendered bash read a clean scan. Probe lines go to stderr on every path; stdout is the JSON document or nothing. - A refused --changed base exited 0/2 when explicit targets were also given; it folds into the exit code (1 over 2 over 0). A trailing --changed no longer defaults to main. - A hand-edited `design_detector: Off` re-enabled the detector; the value is compared case-insensitively. - The engine inherited PATH entries inside the project (a direnv .envrc adding node_modules/.bin); those are filtered like every other project path. - DOM_DUMP_MISSING names the case where the dump script wrote nothing. Co-Authored-By: Claude Fable 5.1 --- bin/gstack-design-detect.ts | 35 ++++++++++++------ lib/design-detect-contract.ts | 3 +- test/gstack-design-detect.test.ts | 60 ++++++++++++++++++++++++++----- 3 files changed, 79 insertions(+), 19 deletions(-) diff --git a/bin/gstack-design-detect.ts b/bin/gstack-design-detect.ts index d2c6052aa..583343ad6 100755 --- a/bin/gstack-design-detect.ts +++ b/bin/gstack-design-detect.ts @@ -131,7 +131,7 @@ function configDesignDetector(): 'auto' | 'off' { // flat YAML: drop a trailing comment and surrounding quotes value = m[1].replace(/\s+#.*$/, '').trim().replace(/^["'](.*)["']$/, '$1'); } - return value === 'off' ? 'off' : 'auto'; + return value.toLowerCase() === 'off' ? 'off' : 'auto'; // `Off` by hand must not silently re-enable a third-party binary } catch { return 'auto'; } @@ -524,7 +524,10 @@ function targetClass(real: string, p: Probe): TargetClass | null { if (!projects || !isInside(real, projects)) return null; const rel = path.relative(projects, real).split(path.sep); if (rel.length < 3 || rel[1] !== 'designs') return null; - return rel[3] === 'dom' ? 'dom-dump' : 'artifact'; + if (rel[3] === 'dom') return 'dom-dump'; + // A directory under designs/ (an audit dir, the designs root) may hold dom/ subtrees the engine will walk: treat it as dumps. + try { if (fs.statSync(real).isDirectory()) return 'dom-dump'; } catch { /* vanished: the engine reports it */ } + return 'artifact'; } function allowedTarget(real: string, p: Probe): boolean { @@ -586,20 +589,30 @@ const ENGINE_ENV_KEYS = new Set([ 'SYSTEMROOT', 'USERPROFILE', 'APPDATA', 'LOCALAPPDATA', 'PATHEXT', 'COMSPEC', 'HOMEDRIVE', 'HOMEPATH', 'PROGRAMDATA', ]); -/** The engine sees PATH/HOME/TMPDIR/locale and its own IMPECCABLE_* knobs, never the agent's tokens. */ -function engineEnv(): Record { +/** + * The engine sees PATH/HOME/TMPDIR/locale and its own IMPECCABLE_* knobs, never the + * agent's tokens. PATH loses entries inside the project (a direnv `.envrc` adding + * `$PWD/node_modules/.bin` must not let the repository supply helpers by name). + */ +function engineEnv(p: Probe): Record { const out: Record = {}; for (const [k, v] of Object.entries(ENV)) { if (v === undefined) continue; const key = WIN ? k.toUpperCase() : k; - if (ENGINE_ENV_KEYS.has(key) || key.startsWith('LC_') || key.startsWith('IMPECCABLE_')) out[k] = v; + if (key === 'PATH') { + out[k] = v.split(path.delimiter).filter(e => { + if (!e || !path.isAbsolute(e)) return false; + const real = realpathOrNull(e); + return real !== null && !underProject(real, p.repoRoot, p.cwd); + }).join(path.delimiter); + } else if (ENGINE_ENV_KEYS.has(key) || key.startsWith('LC_') || key.startsWith('IMPECCABLE_')) out[k] = v; } return out; } -function runEngine(engine: string, batch: string[], cwd: string, timeoutMs: number, extra: string[] = []): EngineRun { +function runEngine(p: Probe, engine: string, batch: string[], cwd: string, timeoutMs: number, extra: string[] = []): EngineRun { const r = Bun.spawnSync([engine, 'detect', '--json', ...extra, ...batch], { - cwd, stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', env: engineEnv(), + cwd, stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', env: engineEnv(p), timeout: timeoutMs, killSignal: 'SIGKILL', maxBuffer: DETECT_LIMITS.stdoutBytes + 1024, }); const out = r.stdout ?? new Uint8Array(); @@ -642,12 +655,13 @@ function normalize(raw: unknown): NormalizedFinding { function scan(args: ScanArgs): number { const p = probe(args.host); + // Every probe line goes to stderr: stdout is the JSON document or nothing, so the + // rendered `scan > "$_DJ"` never captures a sentinel as if it were a scan result. + for (const line of probeLines(p)) process.stderr.write(line + '\n'); if (!p.engine) { - process.stdout.write(probeLines(p).join('\n') + '\n'); analytics({ verb: 'scan', sentinel: sentinelName(p), exit: 0 }); return 0; } - for (const line of probeLines(p)) process.stderr.write(line + '\n'); const { targets, refusedBase } = resolveTargets(args, p); if (!targets.length) { @@ -679,7 +693,7 @@ function scan(args: ScanArgs): number { exit = 1; break; } - const run = runEngine(p.engine, batch, p.repoRoot, timeoutMs, extra); + const run = runEngine(p, p.engine, batch, p.repoRoot, timeoutMs, extra); for (const line of run.stderr.split('\n')) { if (!line.trim()) continue; diagnosticsTotal++; @@ -701,6 +715,7 @@ function scan(args: ScanArgs): number { else if (run.exit !== 0 && run.exit !== 2 && exit !== 1) exit = 1; } + if (refusedBase) exit = 1; // a refused base is a failed target even when explicit targets scanned if (args.format === 'raw') { process.stdout.write(rawChunks.length === 1 ? rawChunks[0] : JSON.stringify(rawFindings, null, 2) + '\n'); } else { diff --git a/lib/design-detect-contract.ts b/lib/design-detect-contract.ts index d4b93c522..0fd49bf30 100644 --- a/lib/design-detect-contract.ts +++ b/lib/design-detect-contract.ts @@ -46,6 +46,7 @@ export const SENTINEL = { DETECT_JSON: 'DETECT_JSON', /** printed by rendered bash after a DOM dump is persisted */ DOM_DUMP_OK: 'DOM_DUMP_OK', + DOM_DUMP_MISSING: 'DOM_DUMP_MISSING', DOM_DUMP_REDACTION_BLOCKED: 'DOM_DUMP_REDACTION_BLOCKED', DOM_DUMP_TOO_LARGE: 'DOM_DUMP_TOO_LARGE', DESIGN_MD_FORMAT: 'DESIGN_MD_FORMAT', @@ -75,7 +76,7 @@ export const SELF_DESCRIBING_SENTINELS: readonly string[] = [ SENTINEL.ENGINE_UNTESTED, SENTINEL.DETECT_EXIT, SENTINEL.DETECT_REFUSED, SENTINEL.DETECT_NO_TARGETS, SENTINEL.DETECT_TIMEOUT, SENTINEL.DETECT_PARSE_ERROR, SENTINEL.DETECT_OUTPUT_TOO_LARGE, SENTINEL.DESIGN_MD_TOKEN_REF_INVALID, SENTINEL.DESIGN_MD_WRITTEN, SENTINEL.DESIGN_MD_BACKUP, SENTINEL.DESIGN_MD_EDIT_REFUSED, - SENTINEL.PROBE_STEP, SENTINEL.ENGINE_STDERR, + SENTINEL.PROBE_STEP, SENTINEL.ENGINE_STDERR, SENTINEL.DOM_DUMP_MISSING, ]; /** Engine versions the committed fixtures were captured from. */ diff --git a/test/gstack-design-detect.test.ts b/test/gstack-design-detect.test.ts index b76a3495c..1a3751105 100644 --- a/test/gstack-design-detect.test.ts +++ b/test/gstack-design-detect.test.ts @@ -223,7 +223,8 @@ describe('probe', () => { expect(r.out).toContain('never runs a repository-local launcher'); expect(r.out).not.toContain(`run \``); const s = run(['scan', 'src/styles.css']); - expect(lines(s.out)[0]).toBe(`${SENTINEL.NOT_CACHED}: repository-local install`); + expect(lines(s.err)[0]).toBe(`${SENTINEL.NOT_CACHED}: repository-local install`); // a scan's probe lines go to stderr; stdout stays JSON-or-nothing + expect(s.out).toBe(''); expect(fs.existsSync(marker)).toBe(false); } finally { fs.rmSync(path.join(REPO, '.claude'), { recursive: true, force: true }); @@ -317,7 +318,7 @@ describe('scan', () => { test('not READY → prints the probe lines, exit 0, engine never needed', () => { const r = run(['scan', 'src/styles.css']); expect(r.code).toBe(0); - expect(lines(r.out)[0]).toBe(SENTINEL.NOT_AVAILABLE); + expect(lines(r.err)[0]).toBe(SENTINEL.NOT_AVAILABLE); }); test.skipIf(!POSIX)('URL and out-of-root targets are refused and the engine is never spawned', () => { @@ -567,7 +568,7 @@ describe('coverage: probe edges', () => { fs.rmSync(log, { force: true }); try { const r = run(['scan', 'src/styles.css'], { env: { IMPECCABLE_BIN: FAKE, IMPECCABLE_FAKE_LOG: log } }); - expect(lines(r.out)[0]).toBe(SENTINEL.DISABLED); + expect(lines(r.err)[0]).toBe(SENTINEL.DISABLED); expect(r.code).toBe(0); expect(fs.existsSync(log)).toBe(false); } finally { @@ -658,14 +659,16 @@ describe('coverage: scan edges', () => { expect(r2.code).toBe(2); }); - test.skipIf(!POSIX)('argument parsing: unknown flags warn, -- ends flags, bad --format falls back to gstack, trailing --changed defaults to main', () => { + test.skipIf(!POSIX)('argument parsing: unknown flags warn, -- ends flags, bad --format falls back to gstack, a trailing --changed is refused', () => { const r = run(['scan', '--bogus', '--format', 'nope', '--', 'src/styles.css'], { env: { IMPECCABLE_BIN: FAKE } }); expect(r.err).toContain('ignoring unknown flag --bogus'); expect(JSON.parse(r.out).schemaVersion).toBe(1); const log = path.join(SANDBOX, 'argv-trailing.log'); fs.rmSync(log, { force: true }); const r2 = run(['scan', 'src/styles.css', '--changed'], { env: { IMPECCABLE_BIN: FAKE, IMPECCABLE_FAKE_LOG: log } }); - expect(r2.code).toBe(2); // base "main" exists in the fixture repo; the explicit target scans + expect(r2.err).toContain(`${SENTINEL.DETECT_REFUSED}: (empty) (not a ref name)`); // never silently defaults to main + expect(JSON.parse(r2.out).targets).toBe(1); // the explicit target still scans + expect(r2.code).toBe(1); }); test.skipIf(!POSIX)('the rendered persist block refuses a dump with a HIGH redaction finding (DOM_DUMP_REDACTION_BLOCKED) and keeps a clean one', () => { @@ -757,12 +760,16 @@ describe('coverage: scan security edges', () => { const out = path.join(SANDBOX, 'env-seen.txt'); fs.writeFileSync(envDump, `#!/bin/sh\nenv > ${JSON.stringify(out)}\necho "[]"\n`); fs.chmodSync(envDump, 0o755); - const r = run(['scan', 'src/styles.css'], { env: { IMPECCABLE_BIN: envDump, ANTHROPIC_API_KEY: 'sk-ant-secret', GITHUB_TOKEN: 'ghp_secret', IMPECCABLE_HOME } }); + const repoBin = path.join(REPO, 'node_modules', '.bin'); + fs.mkdirSync(repoBin, { recursive: true }); + const r = run(['scan', 'src/styles.css'], { env: { IMPECCABLE_BIN: envDump, ANTHROPIC_API_KEY: 'sk-ant-secret', GITHUB_TOKEN: 'ghp_secret', IMPECCABLE_HOME, PATH: `${repoBin}${path.delimiter}${process.env.PATH}` } }); expect(r.code).toBe(0); const seen = fs.readFileSync(out, 'utf-8'); expect(seen).not.toContain('sk-ant-secret'); expect(seen).not.toContain('ghp_secret'); expect(seen).toContain('PATH='); + expect(seen).not.toContain(repoBin); // a project-local PATH entry (direnv, node_modules/.bin) never reaches the engine + fs.rmSync(path.join(REPO, 'node_modules'), { recursive: true, force: true }); expect(seen).toContain('IMPECCABLE_HOME='); }); @@ -834,8 +841,8 @@ describe('engine identity: named impeccable, realpath outside the project', () = fs.writeFileSync(path.join(REPO, 'detect'), `echo ran > ${JSON.stringify(marker)}\n`); try { const r = run(['scan', 'src/styles.css'], { env: { IMPECCABLE_BIN: '/bin/sh' } }); - expect(r.out).toContain(`${SENTINEL.ENV_IGNORED}: IMPECCABLE_BIN is not named impeccable`); - expect(lines(r.out)[0]).toBe(SENTINEL.NOT_AVAILABLE); + expect(r.err).toContain(`${SENTINEL.ENV_IGNORED}: IMPECCABLE_BIN is not named impeccable`); + expect(lines(r.err)[0]).toBe(SENTINEL.NOT_AVAILABLE); expect(fs.existsSync(marker)).toBe(false); } finally { fs.rmSync(path.join(REPO, 'detect'), { force: true }); @@ -999,3 +1006,40 @@ describe('scan: option-like bases and page-controlled inline ignores', () => { } }); }); + +describe('adversarial round: audit directories, config case, refused base with explicit targets', () => { + test.skipIf(!POSIX)('an audit DIRECTORY under designs/ scans as dumps (--no-inline-ignores), because the engine would walk its dom/ subtree', () => { + const audit = path.join(GSTACK_HOME, 'projects', 'x', 'designs', 'design-audit-20260908-dir'); + fs.mkdirSync(path.join(audit, 'dom', 'run1'), { recursive: true }); + fs.writeFileSync(path.join(audit, 'dom', 'run1', 'home.dom.html'), ''); + const log = path.join(SANDBOX, 'argv-audit-dir.log'); + fs.rmSync(log, { force: true }); + try { + const r = run(['scan', '--format', 'gstack', audit], { env: { IMPECCABLE_BIN: FAKE, IMPECCABLE_FAKE_LOG: log } }); + expect(r.code).toBe(2); + const argv = JSON.parse(fs.readFileSync(log, 'utf-8').trim().split('\n')[0]).argv as string[]; + expect(argv).toContain('--no-inline-ignores'); + } finally { + fs.rmSync(audit, { recursive: true, force: true }); + } + }); + + test.skipIf(!POSIX)('design_detector: Off (hand-edited casing) still disables; a scan with no engine prints its sentinels on stderr and nothing on stdout', () => { + fs.writeFileSync(path.join(GSTACK_HOME, 'config.yaml'), 'design_detector: Off\n'); + try { + const r = run(['scan', 'src/styles.css'], { env: { IMPECCABLE_BIN: FAKE } }); + expect(lines(r.err)[0]).toBe(SENTINEL.DISABLED); + expect(r.out).toBe(''); + expect(r.code).toBe(0); + } finally { + fs.rmSync(path.join(GSTACK_HOME, 'config.yaml'), { force: true }); + } + }); + + test.skipIf(!POSIX)('a refused --changed base makes the scan exit 1 even when explicit targets scanned', () => { + const r = run(['scan', '--format', 'gstack', '--changed', 'no-such-ref-xyz', 'src/styles.css'], { env: { IMPECCABLE_BIN: FAKE } }); + expect(r.err).toContain(`${SENTINEL.DETECT_REFUSED}: no-such-ref-xyz`); + expect(JSON.parse(r.out).targets).toBe(1); + expect(r.code).toBe(1); + }); +});