From 400db94e02b3067efbb01c0000ed3dcfcf7f3ab9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 29 Aug 2026 13:51:42 -0400 Subject: [PATCH] fix(#3894): quick path honors workflow.research_before_questions; key resolves from global defaults (#4047) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3894): research_before_questions must resolve globally and order quick.md (failing first) * fix(#3894): quick path honors workflow.research_before_questions; key resolves from global defaults Two layers, one key. The quick workflow ran its discussion phase before its research phase unconditionally — neither quick.md nor its steps ever read workflow.research_before_questions, though the key is documented, schema-registered, /gsd-settings-writable, and honored by /gsd-discuss-phase and /gsd-new-project. A gray-area answer given without research is then written to -CONTEXT.md as a locked decision downstream agents are told not to revisit — an evidence-free choice made unfalsifiable (the reporter's #3714 misresolution). - quick.md Step 4 now carries the same research-before-questions check the two honoring paths make: when enabled, research-phase executes before discussion-phase; false/unset keeps the written order. Both sections stay section-manifest gated. - src/config-loader.cts forwarded workflow.post_planning_gaps from ~/.gsd/defaults.json but silently dropped this key — same file, same nesting, one resolved and one didn't. Now forwarded with the same flat + nested-alias fallback shape, added to the resolution-keys lockstep canary and the #3532 shadowed-warning set (nested alias reporting generalized over both keys). Emitted-Drift-Ack-Growth: quick.md — #3894: +Step 4 ordering rule (the research-before-questions check the discuss-phase and new-project paths already make); a real behavioral gate, not incidental bloat. * fix(#3894): review fold-ins — gate the CONTEXT.md reference, colon slash-forms - quick/steps/research-phase.md directed the researcher subagent to read -CONTEXT.md under DISCUSS_MODE with no existence hedge — but under the new ordering (research BEFORE discussion) the file cannot exist yet when the researcher is dispatched. The reference now says read-only-if-present with the #3894 reason; the alignment purpose still applies on the default ordering. - quick.md's new rule used the hyphen slash forms (/gsd-discuss-phase, /gsd-new-project); source artifacts under gsd-core/workflows must author the colon form the install-time converters key on — the same file already uses /gsd:new-project and /gsd:quick elsewhere. * docs(#3894): planning-config row names the flat CONFIG_DEFAULTS alias config-field-docs requires every CONFIG_DEFAULTS key to appear in the doc; the row documented the canonical namespaced form only. Adds the same alias sentence post_planning_gaps's row carries, plus the #3894 quick-path note. * chore(#3894): changeset fragment (pr number backfilled after PR creation) * chore(#3894): backfill changeset PR number (4047) --------- Co-authored-by: sim --- .changeset/sturdy-dogs-hop.md | 5 ++ gsd-core/references/planning-config.md | 2 +- gsd-core/workflows/quick.md | 2 + .../workflows/quick/steps/research-phase.md | 2 +- src/config-loader.cts | 22 ++++-- tests/config-loader.test.cjs | 74 ++++++++++++++++++- tests/quick-research.test.cjs | 33 +++++++++ 7 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 .changeset/sturdy-dogs-hop.md diff --git a/.changeset/sturdy-dogs-hop.md b/.changeset/sturdy-dogs-hop.md new file mode 100644 index 000000000..125000e69 --- /dev/null +++ b/.changeset/sturdy-dogs-hop.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4047 +--- +workflow.research_before_questions now works on /gsd:quick (research runs before discussion questions when enabled — a gray-area answer without research becomes a locked decision in the quick task context) and resolves from ~/.gsd/defaults.json like its sibling workflow.post_planning_gaps, which the global-defaults merge previously forwarded while silently dropping this key (#3894) diff --git a/gsd-core/references/planning-config.md b/gsd-core/references/planning-config.md index 2e83ec22d..cfc3cac21 100644 --- a/gsd-core/references/planning-config.md +++ b/gsd-core/references/planning-config.md @@ -267,7 +267,7 @@ Set via `workflow.*` namespace in config.json (e.g., `"workflow": { "research": | `workflow.ui_phase` | boolean | `true` | `true`, `false` | Generate UI-SPEC.md for frontend phases | | `workflow.ui_safety_gate` | boolean | `true` | `true`, `false` | Require safety gate approval for UI changes | | `workflow.text_mode` | boolean | `false` | `true`, `false` | Use plain-text numbered lists instead of AskUserQuestion menus | -| `workflow.research_before_questions` | boolean | `false` | `true`, `false` | Run research before interactive questions in discuss phase | +| `workflow.research_before_questions` | boolean | `false` | `true`, `false` | Run research before interactive questions in discuss phase (also honored on the `/gsd:quick` path, #3894). _Alias:_ `research_before_questions` is the flat-key form used in `CONFIG_DEFAULTS`; `workflow.research_before_questions` is the canonical namespaced form. | | `workflow.discuss_mode` | string | `"discuss"` | `"discuss"`, `"assumptions"` | Default mode for discuss-phase: `"discuss"` runs interactive questioning; `"assumptions"` analyzes codebase and surfaces assumptions instead | | `workflow.skip_discuss` | boolean | `false` | `true`, `false` | Skip discuss phase entirely | | `workflow.use_worktrees` | boolean | `true` | `true`, `false` | Run executor agents in isolated git worktrees | diff --git a/gsd-core/workflows/quick.md b/gsd-core/workflows/quick.md index 8514092f1..225b05383 100644 --- a/gsd-core/workflows/quick.md +++ b/gsd-core/workflows/quick.md @@ -270,6 +270,8 @@ Store `$QUICK_DIR` for use in orchestration. --- +**Step 4 ordering (#3894):** Check whether `workflow.research_before_questions` is enabled in `.planning/config.json` (or the config from init context) — the same check `/gsd:discuss-phase` and `/gsd:new-project` already make. When **enabled**, execute the research-phase section BELOW BEFORE the discussion-phase section: a gray-area answer given without research is written to `-CONTEXT.md` as a locked decision downstream agents are told not to revisit, so the evidence must come first. When **false or unset**, keep the written order (discussion, then research) — behavior unchanged. + If `section_manifest` is `null` or `"discussion-phase"` is in its `included` list: read and execute `gsd-core/workflows/quick/steps/discussion-phase.md`. Otherwise skip — do not read the file. diff --git a/gsd-core/workflows/quick/steps/research-phase.md b/gsd-core/workflows/quick/steps/research-phase.md index ecbfb652d..51948b224 100644 --- a/gsd-core/workflows/quick/steps/research-phase.md +++ b/gsd-core/workflows/quick/steps/research-phase.md @@ -32,7 +32,7 @@ Agent( - ${STATE_PATH} (Project state — what's already built) - ${PROJECT_PATH} (Project context) - ./CLAUDE.md or ./.claude/CLAUDE.md (if exists — project-specific guidelines) -${DISCUSS_MODE ? '- ' + QUICK_DIR + '/' + quick_id + '-CONTEXT.md (User decisions — research should align with these)' : ''} +${DISCUSS_MODE ? '- ' + QUICK_DIR + '/' + quick_id + '-CONTEXT.md (User decisions — research should align with these. #3894: when workflow.research_before_questions is enabled research runs BEFORE discussion, so this file will not exist yet — read it only if present)' : ''} ${AGENT_SKILLS_PLANNER} diff --git a/src/config-loader.cts b/src/config-loader.cts index 4e3af331d..4d191e07e 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -133,6 +133,7 @@ const CONFIG_DEFAULTS = { security_asvs_level: _getNestedConfigDefault('workflow', 'security_asvs_level'), security_block_on: _getNestedConfigDefault('workflow', 'security_block_on'), post_planning_gaps: _getNestedConfigDefault('workflow', 'post_planning_gaps'), + research_before_questions: _getNestedConfigDefault('workflow', 'research_before_questions'), // #3894 smart_zone_tokens: _getNestedConfigDefault('workflow', 'smart_zone_tokens'), inline_plan_threshold: _getNestedConfigDefault('workflow', 'inline_plan_threshold'), // #3801 max_prompt_tokens: _getNestedConfigDefault('review', 'max_prompt_tokens'), @@ -345,7 +346,7 @@ function _resetRuntimeWarningCacheForTests(): void { // drift in either direction. const GLOBAL_DEFAULTS_RESOLUTION_KEYS = [ 'model_profile', 'commit_docs', 'research', 'plan_checker', 'verifier', - 'nyquist_validation', 'post_planning_gaps', 'parallelization', 'text_mode', + 'nyquist_validation', 'post_planning_gaps', 'research_before_questions', 'parallelization', 'text_mode', 'resolve_model_ids', 'context_window', 'subagent_timeout', 'model_overrides', 'models', 'granularity', 'granularities', 'planning', 'dynamic_routing', 'effort', 'fast_mode', 'agent_skills', 'response_language', 'runtime', @@ -365,11 +366,14 @@ function _warnShadowedGlobalDefaults(globalDefaults: Record, gl // `?? globalDefaults['workflow']?.['post_planning_gaps']` fallback in // _globalBaseCfg) — a global file using only the nested form is equally // shadowed, so it reports under its dotted name. - if (!shadowed.includes('post_planning_gaps')) { - const wf = globalDefaults['workflow']; - if (wf && typeof wf === 'object' && !Array.isArray(wf) && - Object.prototype.hasOwnProperty.call(wf, 'post_planning_gaps')) { - shadowed.push('workflow.post_planning_gaps'); + // #3894: research_before_questions gets the same nested-alias reporting. + const nestedAliasKeys = ['post_planning_gaps', 'research_before_questions']; + const wf = globalDefaults['workflow']; + if (wf && typeof wf === 'object' && !Array.isArray(wf)) { + for (const k of nestedAliasKeys) { + if (!shadowed.includes(k) && Object.prototype.hasOwnProperty.call(wf, k)) { + shadowed.push(`workflow.${k}`); + } } } if (shadowed.length === 0) return; @@ -1020,6 +1024,12 @@ function loadConfigResolved(cwd: string, options: Record = {}): post_planning_gaps: (globalDefaults['post_planning_gaps']) ?? (globalDefaults['workflow'] as Record | undefined)?.['post_planning_gaps'] ?? defaults.post_planning_gaps, + // #3894: same nested-alias shape as post_planning_gaps above — the key + // was silently dropped from global defaults, so it was unavailable at + // user scope AND inert at project scope on the /gsd-quick path. + research_before_questions: (globalDefaults['research_before_questions']) + ?? (globalDefaults['workflow'] as Record | undefined)?.['research_before_questions'] + ?? defaults.research_before_questions, parallelization: (globalDefaults['parallelization']) ?? defaults.parallelization, text_mode: (globalDefaults['text_mode']) ?? defaults.text_mode, resolve_model_ids: (globalDefaults['resolve_model_ids']) ?? defaults.resolve_model_ids, diff --git a/tests/config-loader.test.cjs b/tests/config-loader.test.cjs index 72a5fdb22..4614223c0 100644 --- a/tests/config-loader.test.cjs +++ b/tests/config-loader.test.cjs @@ -1299,7 +1299,7 @@ describe('#2997: phase_id_convention is not silently dropped on a clean read', ( // would be false for the channel users control via effort sync. const GLOBAL_KEYS_SHADOWED_UNDER_PROJECT = [ 'model_profile', 'commit_docs', 'research', 'plan_checker', 'verifier', - 'nyquist_validation', 'post_planning_gaps', 'parallelization', 'text_mode', + 'nyquist_validation', 'post_planning_gaps', 'research_before_questions', 'parallelization', 'text_mode', 'resolve_model_ids', 'context_window', 'subagent_timeout', 'model_overrides', 'models', 'granularity', 'granularities', 'planning', 'dynamic_routing', 'fast_mode', 'agent_skills', 'response_language', 'runtime', @@ -1601,3 +1601,75 @@ describe('regressions — #3760 loader never expands or persists a non-object se assert.equal(config.branching_strategy, 'none', 'the hoisted value still resolves'); }); }); + +// ─── #3894: workflow.research_before_questions resolves from global defaults ── + +describe('#3894 research_before_questions global-defaults forwarding', () => { + test('nested workflow.research_before_questions in ~/.gsd/defaults.json resolves', () => { + const homeTmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3894-home-')); + const origGsdHome = process.env['GSD_HOME']; + try { + const gsdDir = path.join(homeTmp, '.gsd'); + fs.mkdirSync(gsdDir, { recursive: true }); + // The reporter's exact shape: same nesting as the forwarded + // workflow.post_planning_gaps, one resolves and one did not. + fs.writeFileSync(path.join(gsdDir, 'defaults.json'), JSON.stringify({ + workflow: { post_planning_gaps: true, research_before_questions: true }, + }), 'utf-8'); + process.env['GSD_HOME'] = homeTmp; + const noPlanning = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3894-noplanning-')); + try { + const result = loadConfigResolved(noPlanning); + assert.equal(result.config.post_planning_gaps, true, 'control: the already-forwarded key resolves'); + assert.equal( + result.config.research_before_questions, true, + '#3894: same file, same nesting — the quick-path key must resolve too, not just post_planning_gaps' + ); + } finally { + cleanup(noPlanning); + } + } finally { + if (origGsdHome === undefined) delete process.env['GSD_HOME']; + else process.env['GSD_HOME'] = origGsdHome; + cleanup(homeTmp); + } + }); + + test('flat top-level research_before_questions in ~/.gsd/defaults.json also resolves (Branch D alias parity)', () => { + const homeTmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3894b-home-')); + const origGsdHome = process.env['GSD_HOME']; + try { + const gsdDir = path.join(homeTmp, '.gsd'); + fs.mkdirSync(gsdDir, { recursive: true }); + fs.writeFileSync(path.join(gsdDir, 'defaults.json'), JSON.stringify({ research_before_questions: true }), 'utf-8'); + process.env['GSD_HOME'] = homeTmp; + const noPlanning = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3894b-noplanning-')); + try { + const result = loadConfigResolved(noPlanning); + assert.equal(result.config.research_before_questions, true); + } finally { + cleanup(noPlanning); + } + } finally { + if (origGsdHome === undefined) delete process.env['GSD_HOME']; + else process.env['GSD_HOME'] = origGsdHome; + cleanup(homeTmp); + } + }); + + test('unset resolves to the documented default (false), never undefined', () => { + const noPlanning = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3894c-')); + const homeTmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3894c-home-')); + const origGsdHome = process.env['GSD_HOME']; + try { + process.env['GSD_HOME'] = homeTmp; // no .gsd/defaults.json — builtin defaults + const result = loadConfigResolved(noPlanning); + assert.equal(result.config.research_before_questions, false, 'CANONICAL_CONFIG_DEFAULTS.workflow.research_before_questions is false'); + } finally { + if (origGsdHome === undefined) delete process.env['GSD_HOME']; + else process.env['GSD_HOME'] = origGsdHome; + cleanup(homeTmp); + cleanup(noPlanning); + } + }); +}); diff --git a/tests/quick-research.test.cjs b/tests/quick-research.test.cjs index 29f57c7a3..c6987cdd4 100644 --- a/tests/quick-research.test.cjs +++ b/tests/quick-research.test.cjs @@ -324,3 +324,36 @@ describe('quick workflow: banner variants for flag combinations', () => { ); }); }); + +// ─── #3894: quick.md honors workflow.research_before_questions ─────────────── + +describe('#3894 quick.md research-before-questions ordering', () => { + const QUICK = fs.readFileSync(path.join(WORKFLOWS_DIR, 'quick.md'), 'utf-8'); + + test('quick.md reads workflow.research_before_questions', () => { + assert.ok( + QUICK.includes('research_before_questions'), + '#3894: the quick path must read the documented key — before this fix neither quick.md nor its steps referenced it' + ); + }); + + test('when enabled, research-phase is ordered BEFORE discussion-phase', () => { + const orderingIdx = QUICK.indexOf('research_before_questions'); + assert.ok(orderingIdx !== -1, 'key referenced'); + const ruleWindow = QUICK.slice(orderingIdx, orderingIdx + 900); + assert.ok( + /research[- ]phase[^]{0,200}(before|prior to|first)[^]{0,120}discussion/i.test(ruleWindow) + || /when[^]{0,60}(true|enabled)[^]{0,300}research/i.test(ruleWindow), + 'the ordering rule must state research runs before discussion when the key is true' + ); + assert.ok( + ruleWindow.includes('false') || ruleWindow.includes('unset') || ruleWindow.toLowerCase().includes('unchanged'), + 'and must keep the written order when false/unset' + ); + }); + + test('both step sections remain section-manifest gated (no structural regression)', () => { + assert.ok(/gsd:section id="discussion-phase"/.test(QUICK), 'discussion-phase section marker intact'); + assert.ok(/gsd:section id="research-phase"/.test(QUICK), 'research-phase section marker intact'); + }); +});