Merge pull request #3750 from jeremymcs/fix/depth-verification-answer-validation
fix(gsd): validate depth verification answer before unlocking write-gate
This commit is contained in:
commit
94f9b311d3
7 changed files with 177 additions and 5 deletions
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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(" "),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import type { ExtensionAPI } from "@gsd/pi-coding-agent";
|
||||
|
||||
export {
|
||||
isDepthConfirmationAnswer,
|
||||
isDepthVerified,
|
||||
isQueuePhaseActive,
|
||||
setQueuePhaseActive,
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue