From 6df9b4429727594a1f9776bd0f25aa5003e2e64c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 2 May 2026 11:45:36 -0400 Subject: [PATCH] fix(#3018): codex adapter must stop and ask, not silently default decisions (#3027) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3018): codex adapter must stop and ask, not silently default decisions @jon-hendry: running `\$gsd-discuss-phase 81` in Codex Default mode proceeded toward writing CONTEXT.md / DISCUSSION-LOG.md / checkpoint artifacts without surfacing the discussion questions to the user. The generated Codex skill adapter explicitly told it to do that: Execute mode fallback: - When `request_user_input` is rejected (Execute mode), present a plain-text numbered list and pick a reasonable default. That instruction is wrong for any workflow whose contract is to discuss with the user (most prominently `$gsd-discuss-phase`). The fallback now requires the agent to: 1. STOP. Present the questions as a plain-text numbered list, then wait for the user's reply. 2. Only proceed without a user answer when one of these is true: (a) invocation included --auto / --all, (b) user explicitly approved a default for this question, or (c) workflow's documented contract permits autonomous defaults. 3. Do NOT write CONTEXT.md, DISCUSSION-LOG.md, PLAN.md, or checkpoint files until the user has answered or one of (a)-(c) above applies. Tests: - bug-3018-codex-discuss-fallback.test.cjs (5 tests, structural-IR): parses the generated header into sections via regex, asserts on the Execute-mode-fallback section's content (must contain stop/ wait + plain-text directives, must NOT contain "pick a reasonable default", must name a permission path, must forbid artifact writing). No raw text snapshot — the assertions describe the behavioral invariant, so prose can be reworded without test churn. - codex-config.test.cjs:128 still passes — section still mentions "Execute mode" as required. Verification: - 5/5 pass on new regression test - 116/116 pass on bug-3018 + codex-config combined - 6763/6763 pass on full suite - lint-no-source-grep clean Closes #3018 * test(#3018): parse fallback into typed semantic-flag record (CR) CodeRabbit nitpick on PR #3027: the regression tests grepped the generated header prose with regex, which is brittle and tests wording rather than semantics. Per CONTRIBUTING.md "no-source-grep" standard. Refactored to a structural-IR shape: - New `parseExecuteModeFallback(section)` walks the section text once and returns a typed record: { ok, sectionLength, instructsStop, // STOP/HALT/WAIT directive presentsPlainTextQuestions, // plain-text / numbered list namesPermissionPath, // --auto / --all / explicit approval forbidsWritingArtifactsBeforeAnswer, // write-ban + named artifact class silentlyPicksDefaults, // anti-pattern guard (must be false) } - Each positive invariant gets its own test asserting on the parsed boolean, so a failure points at the exact invariant that broke. - A final test does a single assert.deepStrictEqual against the full expected contract — gives a structured diff when any flag flips. - The artifact-write ban now requires BOTH a "do not write" intent AND a named artifact class (was a single broad regex), so generic "do not write" prose elsewhere in the section can't satisfy it. Verification: 8/8 pass; lint-no-source-grep clean. --- .changeset/codex-discuss-fallback.md | 5 + bin/install.js | 7 +- .../bug-3018-codex-discuss-fallback.test.cjs | 178 ++++++++++++++++++ 3 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 .changeset/codex-discuss-fallback.md create mode 100644 tests/bug-3018-codex-discuss-fallback.test.cjs diff --git a/.changeset/codex-discuss-fallback.md b/.changeset/codex-discuss-fallback.md new file mode 100644 index 000000000..f341bc3e5 --- /dev/null +++ b/.changeset/codex-discuss-fallback.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: TBD +--- +**Codex skill adapter no longer instructs the agent to silently default discuss-phase decisions.** When `request_user_input` was rejected (Default mode), the generated adapter said "pick a reasonable default" — so `$gsd-discuss-phase` proceeded toward writing CONTEXT.md / DISCUSSION-LOG.md / checkpoints without ever asking the user. Adapter prose now requires the agent to STOP, present plain-text questions, and wait, with explicit named exceptions (`--auto`/`--all`/explicit user approval). See #3018. diff --git a/bin/install.js b/bin/install.js index a9c6504ea..0e9e98d96 100755 --- a/bin/install.js +++ b/bin/install.js @@ -2177,7 +2177,12 @@ Multi-select workaround: - Codex has no \`multiSelect\`. Use sequential single-selects, or present a numbered freeform list asking the user to enter comma-separated numbers. Execute mode fallback: -- When \`request_user_input\` is rejected (Execute mode), present a plain-text numbered list and pick a reasonable default. +- When \`request_user_input\` is rejected or unavailable, you MUST stop and present the questions as a plain-text numbered list, then wait for the user's reply. Do NOT pick a default and continue (#3018). +- You may only proceed without a user answer when one of these is true: + (a) the invocation included an explicit non-interactive flag (\`--auto\` or \`--all\`), + (b) the user has explicitly approved a specific default for this question, or + (c) the workflow's documented contract says defaults are safe (e.g. autonomous lifecycle paths). +- Do NOT write workflow artifacts (CONTEXT.md, DISCUSSION-LOG.md, PLAN.md, checkpoint files) until the user has answered the plain-text questions or one of (a)-(c) above applies. Surfacing the questions and waiting is the correct response — silently defaulting and writing artifacts is the #3018 failure mode. ## C. Task() → spawn_agent Mapping GSD workflows use \`Task(...)\` (Claude Code syntax). Translate to Codex collaboration tools: diff --git a/tests/bug-3018-codex-discuss-fallback.test.cjs b/tests/bug-3018-codex-discuss-fallback.test.cjs new file mode 100644 index 000000000..9957d38c4 --- /dev/null +++ b/tests/bug-3018-codex-discuss-fallback.test.cjs @@ -0,0 +1,178 @@ +/** + * Regression test for bug #3018. + * + * @jon-hendry: running `$gsd-discuss-phase 81` in Codex Default mode (where + * `request_user_input` is rejected) caused the agent to pick "reasonable + * defaults" and proceed straight into writing CONTEXT.md / DISCUSSION-LOG.md + * checkpoints — without ever surfacing the questions to the user. The + * generated Codex skill adapter explicitly told it to do that: + * + * "When `request_user_input` is rejected (Execute mode), present a + * plain-text numbered list and pick a reasonable default." + * + * Discuss-mode is the wrong place for that fallback. The contract should be: + * stop, render the questions as plain text, wait for the user's answer. + * Defaults may only be picked when the user has authorized non-interactive + * mode (--auto / --all) or has explicitly approved them. + * + * Test design (#3027 CR follow-up): instead of grepping the prose with + * regex, parse the fallback section into a typed semantic-flag record and + * assert on those booleans. This adheres to CONTRIBUTING.md "no-source-grep" + * — the test names a behavioral invariant, the parser walks the prose + * once and exposes the invariants as named flags, and the prose can be + * reworded freely as long as the flags stay true. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); + +const INSTALL = require(path.join(__dirname, '..', 'bin', 'install.js')); +const { getCodexSkillAdapterHeader } = INSTALL; + +/** + * Extract the "Execute mode fallback" section text from the adapter header. + * Returns null if the section is missing. Section runs from the + * "Execute mode fallback:" label up to the next heading or tag. + */ +function extractExecuteModeFallback(header) { + const m = header.match(/Execute mode fallback:\s*\n([\s\S]*?)(?=\n##\s|\n<\/codex_skill_adapter>)/); + return m ? m[1].trim() : null; +} + +/** + * Parse the Execute-mode-fallback section into a typed semantic-flag + * record. Each flag answers a single behavioral question that the #3018 + * fix is contractually required to encode in the prose. Tests assert on + * the booleans, not the wording — so the prose can evolve without test + * churn as long as the semantics stay correct. + * + * The flags are derived from a single pass over the section text: each + * one looks for any of a small set of synonym phrases that a correct + * implementation would use. The negative anti-pattern flag + * (`silentlyPicksDefaults`) is the regression guard — the prose under + * #3018 told the agent to "pick a reasonable default" autonomously, + * which is exactly what this fix removes. + */ +function parseExecuteModeFallback(section) { + if (!section || typeof section !== 'string') { + return { + ok: false, + sectionLength: 0, + instructsStop: false, + presentsPlainTextQuestions: false, + namesPermissionPath: false, + forbidsWritingArtifactsBeforeAnswer: false, + silentlyPicksDefaults: false, + }; + } + const lower = section.toLowerCase(); + // (a) STOP/WAIT directive — the agent must halt instead of proceeding. + const instructsStop = /\b(stop|halt|wait)\b/.test(lower); + // (b) Plain-text fallback presentation — the agent must surface the + // questions in some inspectable form (numbered list / plain text). + const presentsPlainTextQuestions = /plain.?text|numbered list/.test(lower); + // (c) Permission path that DOES allow defaults — must name at least + // one (--auto / --all / explicit user approval / autonomous workflow). + const namesPermissionPath = + /--auto|--all/.test(section) || + /explicit(ly)? (approv|authoriz|consent)/i.test(section) || + /user (has )?approv|user (has )?authoriz|user (has )?consent/i.test(section) || + /autonomous (lifecycle|workflow|paths?)/i.test(section); + // (d) Artifact-write ban — the agent must not produce workflow files + // (CONTEXT.md, DISCUSSION-LOG.md, PLAN.md, checkpoints) before the + // user answers or one of the permission-path conditions applies. + // Require BOTH a "do not write" intent AND a named artifact class so + // generic "do not write" prose elsewhere can't satisfy the flag. + const forbidsWriteIntent = /do not write|don'?t write|must not write|shall not write/i.test(section); + const namesArtifactClass = /artifact|checkpoint|context\.md|discussion.?log|plan\.md/i.test(section); + const forbidsWritingArtifactsBeforeAnswer = forbidsWriteIntent && namesArtifactClass; + // Anti-pattern guard — the prose that caused #3018. This MUST be false. + const silentlyPicksDefaults = /pick (a |the )?(reasonable|sensible|sane) default/i.test(section); + return { + ok: true, + sectionLength: section.length, + instructsStop, + presentsPlainTextQuestions, + namesPermissionPath, + forbidsWritingArtifactsBeforeAnswer, + silentlyPicksDefaults, + }; +} + +describe('bug #3018: codex skill adapter encodes the discuss-mode fallback contract', () => { + test('exports the adapter generator', () => { + assert.equal(typeof getCodexSkillAdapterHeader, 'function'); + }); + + test('Execute mode fallback section exists and has content', () => { + const header = getCodexSkillAdapterHeader('gsd-discuss-phase'); + const section = extractExecuteModeFallback(header); + const parsed = parseExecuteModeFallback(section); + assert.equal(parsed.ok, true, `section must parse, got header:\n${header}`); + assert.ok(parsed.sectionLength > 0, 'section must be non-empty'); + }); + + test('fallback instructs STOP/WAIT (not silent continuation)', () => { + const section = extractExecuteModeFallback(getCodexSkillAdapterHeader('gsd-discuss-phase')); + const parsed = parseExecuteModeFallback(section); + assert.equal(parsed.instructsStop, true, + `must instruct stop/halt/wait — section was:\n${section}`); + }); + + test('fallback prescribes plain-text question presentation', () => { + const section = extractExecuteModeFallback(getCodexSkillAdapterHeader('gsd-discuss-phase')); + const parsed = parseExecuteModeFallback(section); + assert.equal(parsed.presentsPlainTextQuestions, true, + `must mention plain-text / numbered-list presentation — section was:\n${section}`); + }); + + test('fallback names a permission path under which defaults ARE allowed (--auto / --all / explicit approval / autonomous)', () => { + const section = extractExecuteModeFallback(getCodexSkillAdapterHeader('gsd-discuss-phase')); + const parsed = parseExecuteModeFallback(section); + assert.equal(parsed.namesPermissionPath, true, + `must name at least one permission path — section was:\n${section}`); + }); + + test('fallback forbids writing workflow artifacts before user answers', () => { + const section = extractExecuteModeFallback(getCodexSkillAdapterHeader('gsd-discuss-phase')); + const parsed = parseExecuteModeFallback(section); + assert.equal(parsed.forbidsWritingArtifactsBeforeAnswer, true, + `must encode write-ban + named artifact class — section was:\n${section}`); + }); + + test('fallback does NOT contain the #3018 anti-pattern ("pick a reasonable default")', () => { + const section = extractExecuteModeFallback(getCodexSkillAdapterHeader('gsd-discuss-phase')); + const parsed = parseExecuteModeFallback(section); + assert.equal(parsed.silentlyPicksDefaults, false, + `regression — fallback must NOT instruct the agent to pick defaults autonomously, section was:\n${section}`); + }); + + test('all four positive flags + the negative anti-pattern flag — typed-record snapshot', () => { + // Single assertion that the whole semantic record matches the contract. + // If any flag flips, the test fails with a structured diff naming the + // exact invariant that broke. + const section = extractExecuteModeFallback(getCodexSkillAdapterHeader('gsd-discuss-phase')); + const parsed = parseExecuteModeFallback(section); + const semanticContract = { + ok: parsed.ok, + instructsStop: parsed.instructsStop, + presentsPlainTextQuestions: parsed.presentsPlainTextQuestions, + namesPermissionPath: parsed.namesPermissionPath, + forbidsWritingArtifactsBeforeAnswer: parsed.forbidsWritingArtifactsBeforeAnswer, + silentlyPicksDefaults: parsed.silentlyPicksDefaults, + }; + assert.deepStrictEqual(semanticContract, { + ok: true, + instructsStop: true, + presentsPlainTextQuestions: true, + namesPermissionPath: true, + forbidsWritingArtifactsBeforeAnswer: true, + silentlyPicksDefaults: false, + }, `discuss-mode fallback contract violated — section was:\n${section}`); + }); +});