From dd08f501e53fdafd05fd2bd36dd4b7261b0ee9c6 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 7 Apr 2026 19:26:54 -0500 Subject: [PATCH] fix(gsd): validate depth verification answer before unlocking write-gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool_result handler called markDepthVerified() whenever ask_user_questions returned any response with a depth_verification question ID — without checking what the user actually selected. Selecting "Not quite", "None of the above", or garbage input all unlocked the gate. - Extract isDepthConfirmationAnswer() into write-gate.ts with structural validation: cross-references selected answer against the question's defined options, only accepting an exact match of the first option (confirmation by convention). Rejects free-form "Other" text and decouples from any specific label substring. - Harden block message with explicit anti-bypass language - Add anti-bypass instructions to all three discuss prompts - Add 8 new tests covering: structural validation, free-form bypass rejection, label-drift resilience, fallback behavior, edge cases Closes #3749 --- .../gsd/bootstrap/register-hooks.ts | 9 +- .../extensions/gsd/bootstrap/write-gate.ts | 37 ++++- src/resources/extensions/gsd/index.ts | 1 + .../extensions/gsd/prompts/discuss.md | 2 + .../gsd/prompts/guided-discuss-milestone.md | 2 + src/resources/extensions/gsd/prompts/queue.md | 2 + .../extensions/gsd/tests/write-gate.test.ts | 129 +++++++++++++++++- 7 files changed, 177 insertions(+), 5 deletions(-) diff --git a/src/resources/extensions/gsd/bootstrap/register-hooks.ts b/src/resources/extensions/gsd/bootstrap/register-hooks.ts index 7d82b956f..f511fbadf 100644 --- a/src/resources/extensions/gsd/bootstrap/register-hooks.ts +++ b/src/resources/extensions/gsd/bootstrap/register-hooks.ts @@ -6,7 +6,7 @@ import { isToolCallEventType } from "@gsd/pi-coding-agent"; import { buildMilestoneFileName, resolveMilestonePath, resolveSliceFile, resolveSlicePath } from "../paths.js"; import { buildBeforeAgentStartResult } from "./system-context.js"; import { handleAgentEnd } from "./agent-end-recovery.js"; -import { clearDiscussionFlowState, isDepthVerified, isQueuePhaseActive, markDepthVerified, resetWriteGateState, shouldBlockContextWrite, shouldBlockQueueExecution } from "./write-gate.js"; +import { clearDiscussionFlowState, isDepthVerified, isDepthConfirmationAnswer, isQueuePhaseActive, markDepthVerified, resetWriteGateState, shouldBlockContextWrite, shouldBlockQueueExecution } from "./write-gate.js"; import { isBlockedStateFile, isBashWriteToStateFile, BLOCKED_WRITE_ERROR } from "../write-intercept.js"; import { cleanupQuickBranch } from "../quick.js"; import { getDiscussionMilestoneId } from "../guided-flow.js"; @@ -249,7 +249,12 @@ export function registerHooks(pi: ExtensionAPI): void { const questions: any[] = (event.input as any)?.questions ?? []; for (const question of questions) { if (typeof question.id === "string" && question.id.includes("depth_verification")) { - markDepthVerified(); + // Only unlock the gate if the user selected the first option (confirmation). + // Cross-references against the question's defined options to reject free-form "Other" text. + const answer = details.response?.answers?.[question.id]; + if (isDepthConfirmationAnswer(answer?.selected, question.options)) { + markDepthVerified(); + } break; } } diff --git a/src/resources/extensions/gsd/bootstrap/write-gate.ts b/src/resources/extensions/gsd/bootstrap/write-gate.ts index c73d7f828..3b8041bb1 100644 --- a/src/resources/extensions/gsd/bootstrap/write-gate.ts +++ b/src/resources/extensions/gsd/bootstrap/write-gate.ts @@ -54,6 +54,35 @@ export function markDepthVerified(): void { depthVerificationDone = true; } +/** + * Check whether a depth_verification answer confirms the discussion is complete. + * Uses structural validation: the selected answer must exactly match the first + * option label from the question definition (the confirmation option by convention). + * This rejects free-form "Other" text, decline options, and garbage input without + * coupling to any specific label substring. + * + * @param selected The answer's selected value from details.response.answers[id].selected + * @param options The question's options array from event.input.questions[n].options + */ +export function isDepthConfirmationAnswer( + selected: unknown, + options?: Array<{ label?: string }>, +): boolean { + const value = Array.isArray(selected) ? selected[0] : selected; + if (typeof value !== "string" || !value) return false; + + // If options are available, structurally validate: selected must exactly match + // the first option (confirmation) label. Rejects free-form "Other" and decline options. + if (Array.isArray(options) && options.length > 0) { + const confirmLabel = options[0]?.label; + return typeof confirmLabel === "string" && value === confirmLabel; + } + + // Fallback when options aren't available (e.g., older call sites): + // accept only if it contains "(Recommended)" — the prompt convention suffix. + return value.includes("(Recommended)"); +} + export function shouldBlockContextWrite( toolName: string, inputPath: string, @@ -71,7 +100,13 @@ export function shouldBlockContextWrite( return { block: true, - reason: `Blocked: Cannot write to milestone CONTEXT.md during discussion phase without depth verification. Call ask_user_questions with question id "depth_verification" first to confirm discussion depth before writing context.`, + reason: [ + `HARD BLOCK: Cannot write to milestone CONTEXT.md without depth verification.`, + `This is a mechanical gate — you MUST NOT proceed, retry, or rationalize past this block.`, + `Required action: call ask_user_questions with question id containing "depth_verification".`, + `The user MUST select the "(Recommended)" confirmation option to unlock this gate.`, + `If the user declines, cancels, or the tool fails, you must re-ask — not bypass.`, + ].join(" "), }; } diff --git a/src/resources/extensions/gsd/index.ts b/src/resources/extensions/gsd/index.ts index 836aae28b..45f2bf8a7 100644 --- a/src/resources/extensions/gsd/index.ts +++ b/src/resources/extensions/gsd/index.ts @@ -1,6 +1,7 @@ import type { ExtensionAPI } from "@gsd/pi-coding-agent"; export { + isDepthConfirmationAnswer, isDepthVerified, isQueuePhaseActive, setQueuePhaseActive, diff --git a/src/resources/extensions/gsd/prompts/discuss.md b/src/resources/extensions/gsd/prompts/discuss.md index 334abcdc0..0e2cd4e15 100644 --- a/src/resources/extensions/gsd/prompts/discuss.md +++ b/src/resources/extensions/gsd/prompts/discuss.md @@ -114,6 +114,8 @@ If they clarify, absorb the correction and re-verify. The depth verification is the required write-gate. Do **not** add another meta "ready to proceed?" checkpoint immediately after it unless there is still material ambiguity. +**CRITICAL — Non-bypassable gate:** The system mechanically blocks CONTEXT.md writes until the user selects the "(Recommended)" option. If the user declines, cancels, or the tool fails, you MUST re-ask — never rationalize past the block ("tool not responding, I'll proceed" is forbidden). The gate exists to protect the user's work; treat a block as an instruction, not an obstacle to work around. + ## Wrap-up Gate Once the depth checklist is fully satisfied, move directly into requirements and roadmap preview. Do not insert a separate "are you ready to continue?" gate unless the user explicitly wants to keep brainstorming or you still see material ambiguity. diff --git a/src/resources/extensions/gsd/prompts/guided-discuss-milestone.md b/src/resources/extensions/gsd/prompts/guided-discuss-milestone.md index 0d651e30c..a2f992631 100644 --- a/src/resources/extensions/gsd/prompts/guided-discuss-milestone.md +++ b/src/resources/extensions/gsd/prompts/guided-discuss-milestone.md @@ -100,6 +100,8 @@ If they clarify, absorb the correction and re-verify. The depth verification is the only required confirmation gate. Do not add a second "ready to proceed?" gate after it. +**CRITICAL — Non-bypassable gate:** The system mechanically blocks CONTEXT.md writes until the user selects the "(Recommended)" option. If the user declines, cancels, or the tool fails, you MUST re-ask — never rationalize past the block ("tool not responding, I'll proceed" is forbidden). The gate exists to protect the user's work; treat a block as an instruction, not an obstacle to work around. + --- ## Output diff --git a/src/resources/extensions/gsd/prompts/queue.md b/src/resources/extensions/gsd/prompts/queue.md index 34620bd4e..5bbdd7b2a 100644 --- a/src/resources/extensions/gsd/prompts/queue.md +++ b/src/resources/extensions/gsd/prompts/queue.md @@ -103,6 +103,8 @@ The user confirms or corrects before you write. One depth verification per miles **If you skip this step, the system will block the CONTEXT.md write and return an error telling you to complete verification first.** +**CRITICAL — Non-bypassable gate:** The system mechanically blocks CONTEXT.md writes until the user selects the "(Recommended)" option. If the user declines, cancels, or the tool fails, you MUST re-ask — never rationalize past the block ("tool not responding, I'll proceed" is forbidden). The gate exists to protect the user's work; treat a block as an instruction, not an obstacle to work around. + ## Output Phase Once the user is satisfied, in a single pass for **each** new milestone: diff --git a/src/resources/extensions/gsd/tests/write-gate.test.ts b/src/resources/extensions/gsd/tests/write-gate.test.ts index 8ca4ee7b5..35c610a64 100644 --- a/src/resources/extensions/gsd/tests/write-gate.test.ts +++ b/src/resources/extensions/gsd/tests/write-gate.test.ts @@ -12,6 +12,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { + isDepthConfirmationAnswer, shouldBlockContextWrite, isDepthVerified, isQueuePhaseActive, @@ -117,9 +118,9 @@ test('write-gate: regex does not match slice context files (S01-CONTEXT.md)', () assert.strictEqual(result.block, false, 'S01-CONTEXT.md should not be blocked'); }); -// ─── Scenario 7: Error message contains actionable instruction ── +// ─── Scenario 7: Error message contains actionable instruction and anti-bypass language ── -test('write-gate: blocked reason contains depth_verification keyword', () => { +test('write-gate: blocked reason contains depth_verification keyword and anti-bypass language', () => { const result = shouldBlockContextWrite( 'write', '.gsd/milestones/M999/M999-CONTEXT.md', @@ -129,6 +130,8 @@ test('write-gate: blocked reason contains depth_verification keyword', () => { assert.strictEqual(result.block, true); assert.ok(result.reason!.includes('depth_verification'), 'reason should mention depth_verification question id'); assert.ok(result.reason!.includes('ask_user_questions'), 'reason should mention ask_user_questions tool'); + assert.ok(result.reason!.includes('MUST NOT'), 'reason should include anti-bypass language'); + assert.ok(result.reason!.includes('(Recommended)'), 'reason should specify the required confirmation option'); }); // ─── Scenario 8: Queue mode blocks CONTEXT.md write without depth verification ── @@ -191,3 +194,125 @@ test('write-gate: markDepthVerified unblocks queue-mode writes when milestoneId clearDiscussionFlowState(); }); + +// ─── Standard options fixture used across depth confirmation tests ── + +const STANDARD_OPTIONS = [ + { label: 'Yes, you got it (Recommended)' }, + { label: 'Not quite — let me clarify' }, +]; + +// ─── Scenario 11: accepts first option (confirmation) with structural validation ── + +test('write-gate: isDepthConfirmationAnswer accepts first option with options present', () => { + assert.strictEqual( + isDepthConfirmationAnswer('Yes, you got it (Recommended)', STANDARD_OPTIONS), + true, + 'should accept exact match of first option label', + ); +}); + +// ─── Scenario 12: rejects second option (decline) ── + +test('write-gate: isDepthConfirmationAnswer rejects decline option', () => { + assert.strictEqual( + isDepthConfirmationAnswer('Not quite — let me clarify', STANDARD_OPTIONS), + false, + 'should reject the clarification option', + ); +}); + +// ─── Scenario 13: rejects "None of the above" ── + +test('write-gate: isDepthConfirmationAnswer rejects None of the above', () => { + assert.strictEqual( + isDepthConfirmationAnswer('None of the above', STANDARD_OPTIONS), + false, + 'should reject None of the above', + ); +}); + +// ─── Scenario 14: rejects garbage/empty input ── + +test('write-gate: isDepthConfirmationAnswer rejects garbage and edge cases', () => { + assert.strictEqual(isDepthConfirmationAnswer('discord', STANDARD_OPTIONS), false, 'garbage string'); + assert.strictEqual(isDepthConfirmationAnswer('', STANDARD_OPTIONS), false, 'empty string'); + assert.strictEqual(isDepthConfirmationAnswer(undefined, STANDARD_OPTIONS), false, 'undefined'); + assert.strictEqual(isDepthConfirmationAnswer(null, STANDARD_OPTIONS), false, 'null'); + assert.strictEqual(isDepthConfirmationAnswer(42, STANDARD_OPTIONS), false, 'number'); +}); + +// ─── Scenario 15: handles array-wrapped selection ── + +test('write-gate: isDepthConfirmationAnswer handles array-wrapped selected value', () => { + assert.strictEqual( + isDepthConfirmationAnswer(['Yes, you got it (Recommended)'], STANDARD_OPTIONS), + true, + 'should accept array-wrapped confirmation', + ); + assert.strictEqual( + isDepthConfirmationAnswer(['Not quite — let me clarify'], STANDARD_OPTIONS), + false, + 'should reject array-wrapped decline', + ); + assert.strictEqual( + isDepthConfirmationAnswer([], STANDARD_OPTIONS), + false, + 'should reject empty array', + ); +}); + +// ─── Scenario 16: rejects free-form "Other" text that contains "(Recommended)" ── + +test('write-gate: isDepthConfirmationAnswer rejects free-form text containing Recommended', () => { + assert.strictEqual( + isDepthConfirmationAnswer('I think this is fine (Recommended)', STANDARD_OPTIONS), + false, + 'free-form text with (Recommended) substring must not unlock gate', + ); + assert.strictEqual( + isDepthConfirmationAnswer('(Recommended)', STANDARD_OPTIONS), + false, + 'bare (Recommended) string must not unlock gate', + ); +}); + +// ─── Scenario 17: works with changed label text (decoupled from specific copy) ── + +test('write-gate: isDepthConfirmationAnswer works with different label text', () => { + const customOptions = [ + { label: 'Looks good, proceed' }, + { label: 'Needs more discussion' }, + ]; + assert.strictEqual( + isDepthConfirmationAnswer('Looks good, proceed', customOptions), + true, + 'should accept first option regardless of label text', + ); + assert.strictEqual( + isDepthConfirmationAnswer('Needs more discussion', customOptions), + false, + 'should reject second option', + ); + // Old label should NOT work with new options + assert.strictEqual( + isDepthConfirmationAnswer('Yes, you got it (Recommended)', customOptions), + false, + 'old label text should not match new options', + ); +}); + +// ─── Scenario 18: fallback when options not available ── + +test('write-gate: isDepthConfirmationAnswer falls back to (Recommended) match without options', () => { + assert.strictEqual( + isDepthConfirmationAnswer('Yes, you got it (Recommended)'), + true, + 'should accept via fallback when no options provided', + ); + assert.strictEqual( + isDepthConfirmationAnswer('Not quite — let me clarify'), + false, + 'should reject non-Recommended via fallback', + ); +});