From 63d812ed97a460e1b57a346e3d7f3a0f24b7c1f3 Mon Sep 17 00:00:00 2001 From: montjeffrey Date: Fri, 2 Oct 2026 10:29:36 -0400 Subject: [PATCH] fix(gateguard): make the hidden-intent runner safe to scale Four defects found by running the repository's own checks and one more trial against it. ESLint rejected an unused `armSettings` import in intent-eval.js; the caller supplies `armSettingsByName` already. A timeout was lost whenever a later turn came back clean: `a || b ? c : false` evaluates the condition first, so `timedOut` was recomputed from the new turn instead of being kept. Isolating each trial's config directory, to stop persisted sessions piling up under the projects folder, puts the credentials out of reach. Every turn then returns "Not logged in", and the harness graded that as a coverage-hole - an auth failure recorded as a finding. Keep the real config directory and delete only the session folders this run's work root created, and adopt lib.js's own guards: a trial that is unauthenticated, or whose gated arm edited files with no metrics line and no denial, stops the run instead of being recorded. Record denials per turn as well as in total. The hook marks a file checked once per session and --resume keeps one session id for the conversation, so a zero after a non-zero is the latch rather than the gate staying quiet, and silence-hole has to be read with that column in view. Verified: a real trial authenticates, produces a graded silence-hole, and leaves the projects directory at the same count it started with. --- docker/gateguard-effectiveness/intent-eval.js | 8 +- docker/gateguard-effectiveness/run-intent.js | 36 +++++++++ docker/gateguard-effectiveness/session.js | 25 ++++++- docs/gateguard/question-effectiveness.md | 29 ++++++++ tests/docker/gateguard-intent.test.js | 74 +++++++++++++++++++ 5 files changed, 169 insertions(+), 3 deletions(-) diff --git a/docker/gateguard-effectiveness/intent-eval.js b/docker/gateguard-effectiveness/intent-eval.js index b20546a5e..ef3f99b7c 100644 --- a/docker/gateguard-effectiveness/intent-eval.js +++ b/docker/gateguard-effectiveness/intent-eval.js @@ -12,7 +12,7 @@ const fs = require('fs'); const path = require('path'); const { prepareWorkspace, commitWorkspace, grade, readMetrics } = require('./lib'); -const { ARMS, TRIAL_FILE, armSettings, trialPatch } = require('./arms'); +const { ARMS, TRIAL_FILE, trialPatch } = require('./arms'); const { runConversation } = require('./session'); const { classify } = require('./coverage'); @@ -107,7 +107,13 @@ function runIntentTrial(trial, { workRoot, armSettingsByName, executable, model, disclosed: conversation.disclosed, disclosedDecisive: conversation.disclosedDecisive, judgeFailed: conversation.judgeFailed, + providerError: conversation.providerError, + providerMessage: conversation.providerMessage, + // Same guard as lib.js: a gated arm whose agent edited files with no metrics + // line and no denial means the hook never ran, so the trial proves nothing. + hookObserved: !ARMS[trial.arm].gate || conversation.editCalls === 0 || metrics.length > 0 || conversation.gateDenials > 0, gateDenials: conversation.gateDenials, + denialsPerTurn: conversation.denialsPerTurn, questionsAsked: [...new Set(denials.flatMap(event => event.questions || []))].sort(), editCalls: conversation.editCalls, tools: conversation.tools, diff --git a/docker/gateguard-effectiveness/run-intent.js b/docker/gateguard-effectiveness/run-intent.js index c6e3202a3..8b1533f56 100644 --- a/docker/gateguard-effectiveness/run-intent.js +++ b/docker/gateguard-effectiveness/run-intent.js @@ -91,6 +91,32 @@ function readResults(file) { return fs.readFileSync(file, 'utf8').split('\n').filter(Boolean).map(line => JSON.parse(line)); } +// Resuming a session needs persistence, so Claude writes a transcript folder per +// trial workspace under its projects directory. A per-trial CLAUDE_CONFIG_DIR +// would isolate those but also hides the credentials, so they are removed here +// instead. Only folders this run created, named after its own temp work root, +// are touched. +function sessionsDir() { + const home = process.env.CLAUDE_CONFIG_DIR || path.join(os.homedir(), '.claude'); + return path.join(home, 'projects'); +} + +function listSessions() { + try { + return new Set(fs.readdirSync(sessionsDir())); + } catch (_) { + return new Set(); + } +} + +function removeTrialSessions(before) { + const dir = sessionsDir(); + for (const entry of listSessions()) { + if (before.has(entry) || !entry.includes('gateguard-intent')) continue; + fs.rmSync(path.join(dir, entry), { recursive: true, force: true }); + } +} + function writeReport(options, rows, meta) { const header = `Model: ${meta.model}; judge: ${meta.judgeModel}; hook commit: ${meta.sha}; reps: ${meta.reps}; user turns: ${meta.userTurns}.\n\n`; const body = rows.length ? renderHoles(rows) : 'No trials recorded yet.\n'; @@ -181,6 +207,7 @@ function main() { fs.writeFileSync(metaFile, `${JSON.stringify(meta, null, 2)}\n`); let judgeFailures = 0; for (const [index, trial] of trials.entries()) { + const sessionsBefore = listSessions(); const transcript = []; const execute = (file, args, spawnOptions) => { const result = spawnSync(file, args, spawnOptions); @@ -203,6 +230,15 @@ function main() { transcript.join('\n') ); fs.rmSync(path.join(workRoot, trial.key.replace(/\//g, '__')), { recursive: true, force: true }); + removeTrialSessions(sessionsBefore); + // An unauthenticated or hookless run must not be recorded: graded as data it + // looks like a finding (the first such trial was filed as a coverage hole). + if (row.providerError && /authenticat|log ?in|oauth/i.test(row.providerMessage || '')) { + throw new Error(`${trial.key}: Claude is not signed in (${row.providerMessage}); sign in with the claude CLI and rerun with the same --out`); + } + if (!row.hookObserved) { + throw new Error(`${trial.key}: the gate did not run although the agent edited files; check the hook setup before spending more sessions`); + } fs.appendFileSync(resultsFile, `${JSON.stringify(row)}\n`); rows.push(row); writeReport(options, rows, meta); diff --git a/docker/gateguard-effectiveness/session.js b/docker/gateguard-effectiveness/session.js index 7982600c3..fa3761510 100644 --- a/docker/gateguard-effectiveness/session.js +++ b/docker/gateguard-effectiveness/session.js @@ -77,19 +77,26 @@ function runConversation({ cwd, stateDir, settingsPath, prompt, intent }, option execute = spawnSync, judge = userSim.answer, judgeModel = userSim.JUDGE_MODEL } = options; + // The config directory is deliberately NOT isolated. Resuming needs session + // persistence, and the obvious fix - a per-trial CLAUDE_CONFIG_DIR - puts the + // credentials out of reach: every turn then returns "Not logged in". The + // persisted transcripts are cleaned up by the runner instead. const env = childEnv(stateDir); const usage = emptyUsage(); const exchange = []; const disclosed = new Set(); + const denialsPerTurn = []; let gateDenials = 0; let editCalls = 0; - let tools = {}; + const tools = {}; let judgeFailed = false; let asked = false; let resumeId = null; let input = `${prompt}\n\n${COLLAB_NOTE}\n`; let lastStatus = null; let timedOut = false; + let providerError = false; + let providerMessage = null; for (let turn = 0; turn <= userTurns; turn++) { const args = sessionArgs({ model, settingsPath, maxTurns }, resumeId); @@ -99,9 +106,18 @@ function runConversation({ cwd, stateDir, settingsPath, prompt, intent }, option timeout: timeoutMs, killSignal: 'SIGKILL', maxBuffer: 64 * 1024 * 1024 }); lastStatus = run.status; - timedOut = timedOut || run.error ? /ETIMEDOUT|SIGKILL/.test(String(run.error && run.error.code)) : false; + // A timeout on any turn stands: `a || b ? c : false` cleared it on the next clean turn. + timedOut = timedOut || /ETIMEDOUT|SIGKILL/.test(String(run.error && run.error.code)); const stream = parseStream(run.stdout); + // Same test as lib.js: a missing or is_error result is a provider failure, + // not a trial. Recorded per turn so a failure on any turn invalidates the + // conversation rather than being graded as a finding. + if (!stream.result || stream.result.is_error === true) { + providerError = true; + providerMessage = String((stream.result && stream.result.result) || finalText(stream) || '').slice(0, 200) || null; + } addUsage(usage, stream.result); + denialsPerTurn.push(stream.gateDenials); gateDenials += stream.gateDenials; editCalls += stream.editCalls; for (const [name, count] of Object.entries(stream.tools)) tools[name] = (tools[name] || 0) + count; @@ -127,11 +143,16 @@ function runConversation({ cwd, stateDir, settingsPath, prompt, intent }, option disclosed: [...disclosed], disclosedDecisive: decisive.length > 0 && decisive.every(id => disclosed.has(id)), gateDenials, + // Per turn, because the gate marks a file checked once per session: a zero + // after a non-zero is the latch, not the gate choosing to stay quiet. + denialsPerTurn, editCalls, tools, judgeFailed, exitStatus: lastStatus, timedOut, + providerError, + providerMessage, ...usage }; } diff --git a/docs/gateguard/question-effectiveness.md b/docs/gateguard/question-effectiveness.md index e7f6c9582..73beb315f 100644 --- a/docs/gateguard/question-effectiveness.md +++ b/docs/gateguard/question-effectiveness.md @@ -295,3 +295,32 @@ per user turn. independence of whoever writes the next ones. - Telling every arm that the user is reachable raises asking across the board, which is the right comparison but not the shipped default. + +### Scaling a conversation + +Three properties matter once a trial is more than one turn, and only the first +is fully solved. + +- **Persisted sessions are cleaned up.** Resuming needs session persistence, so + Claude writes a transcript folder per trial workspace under its projects + directory. Isolating that with a per-trial `CLAUDE_CONFIG_DIR` does not work: + it puts the credentials out of reach and every turn returns "Not logged in", + which the harness then grades as a `coverage-hole`. The runner therefore keeps + the real config directory and deletes only the folders its own work root + created. A trial that is unauthenticated, or whose gated arm edited files with + no sign of the hook, stops the run rather than being recorded. +- **The gate fires once per file per session, so its influence decays.** The + hook keeps a `checked` list in session state, and `--resume` keeps one session + id for the whole conversation. After the first touch of a file the gate passes + it, so in a long conversation almost all of the gate's effect lands in the + first turn. `denialsPerTurn` records the denials of each turn separately, so a + zero following a non-zero reads as the latch rather than as the gate choosing + to stay quiet. `silence-hole` should be read with that column in view, and the + hook's `MAX_CHECKED_ENTRIES` pruning can let a file be gated again in a very + long session. +- **Cost grows faster than turn count.** Each turn is a fresh `claude --print + --resume`, which replays the conversation so far, so tokens per turn rise as + the exchange lengthens. `--user-turns 3` means up to four agent invocations + plus one judge call per answer: a 6-scenario, 3-arm, 3-repetition run is 54 + trials and up to 216 billed invocations, not 54. Raise `--user-turns` only + with a reason, and read the per-arm cost before scaling repetitions. diff --git a/tests/docker/gateguard-intent.test.js b/tests/docker/gateguard-intent.test.js index 28f360604..69cb49d52 100644 --- a/tests/docker/gateguard-intent.test.js +++ b/tests/docker/gateguard-intent.test.js @@ -271,6 +271,80 @@ test('a question is answered and the reply is fed back as the next turn', () => assert.strictEqual(result.gateDenials, 2); }); +test('a timeout on an earlier turn is not cleared by a later clean turn', () => { + let call = 0; + const execute = () => { + call++; + if (call === 1) return { stdout: '', status: null, error: { code: 'ETIMEDOUT' } }; + return streamFor('done'); + }; + let asked = 0; + const judge = () => { + asked++; + return asked === 1 + ? { isQuestion: true, reply: 'carry on', disclosed: [], judgeFailed: false } + : { isQuestion: false, reply: INTENT.stonewall, disclosed: [], judgeFailed: false }; + }; + const result = session.runConversation( + { cwd: '.', stateDir: '.', settingsPath: 's', prompt: 'p', intent: INTENT }, + { model: 'm', maxTurns: 5, timeoutMs: 1000, userTurns: 2, execute, judge } + ); + assert.strictEqual(result.timedOut, true); +}); + +test('the config directory is never isolated, because that hides the credentials', () => { + // A per-trial CLAUDE_CONFIG_DIR looks like the right way to stop persisted + // sessions piling up, but it makes every turn return "Not logged in", and the + // trial is then graded as a coverage hole. Guard against reintroducing it. + let seen = 'unset'; + const execute = (file, args, options) => { + seen = options.env.CLAUDE_CONFIG_DIR; + return streamFor('done'); + }; + const judge = () => ({ isQuestion: false, reply: INTENT.stonewall, disclosed: [], judgeFailed: false }); + session.runConversation( + { cwd: '.', stateDir: '.', settingsPath: 's', prompt: 'p', intent: INTENT }, + { model: 'm', maxTurns: 5, timeoutMs: 1000, userTurns: 1, execute, judge } + ); + assert.strictEqual(seen, process.env.CLAUDE_CONFIG_DIR); +}); + +test('a missing or errored result is a provider error, not a trial', () => { + const noResult = () => ({ stdout: JSON.stringify({ type: 'assistant', message: { content: [{ type: 'text', text: 'Not logged in - Please run /login' }] } }), status: 1 }); + const judge = () => ({ isQuestion: false, reply: INTENT.stonewall, disclosed: [], judgeFailed: false }); + const result = session.runConversation( + { cwd: '.', stateDir: '.', settingsPath: 's', prompt: 'p', intent: INTENT }, + { model: 'm', maxTurns: 5, timeoutMs: 1000, userTurns: 1, execute: noResult, judge } + ); + assert.strictEqual(result.providerError, true); + assert.match(result.providerMessage, /Not logged in/); + // The runner turns this into a thrown error rather than a graded row. + assert.match(result.providerMessage, /log ?in/i); +}); + +test('denials are recorded per turn, so the once-per-session latch is visible', () => { + let call = 0; + const execute = () => { + call++; + // The gate fires on the first touch and is silent afterwards because the + // file is already marked checked, not because it chose to stay quiet. + return call === 1 ? streamFor('which window?', { denials: 2 }) : streamFor('done', { denials: 0 }); + }; + let asked = 0; + const judge = () => { + asked++; + return asked === 1 + ? { isQuestion: true, reply: '400 days, mark them', disclosed: ['window', 'mode'], judgeFailed: false } + : { isQuestion: false, reply: INTENT.stonewall, disclosed: [], judgeFailed: false }; + }; + const result = session.runConversation( + { cwd: '.', stateDir: '.', settingsPath: 's', prompt: 'p', intent: INTENT }, + { model: 'm', maxTurns: 5, timeoutMs: 1000, userTurns: 2, execute, judge } + ); + assert.deepStrictEqual(result.denialsPerTurn, [2, 0]); + assert.strictEqual(result.gateDenials, 2); +}); + test('disclosedDecisive needs every decisive fact, not just one', () => { const execute = () => streamFor('how long is the window?'); const judge = () => ({ isQuestion: true, reply: '400 days', disclosed: ['window'], judgeFailed: false });