* 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.
This commit is contained in:
Tom Boucher
5
.changeset/codex-discuss-fallback.md
Normal file
5
.changeset/codex-discuss-fallback.md
Normal file
@@ -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.
|
||||
@@ -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:
|
||||
|
||||
178
tests/bug-3018-codex-discuss-fallback.test.cjs
Normal file
178
tests/bug-3018-codex-discuss-fallback.test.cjs
Normal file
@@ -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 </codex_skill_adapter> 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}`);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user