From fb0334eb5c9609c7d2c32fed1f68bf427578d9f8 Mon Sep 17 00:00:00 2001 From: Trevor Bezdek Date: Fri, 16 Oct 2026 23:24:27 -0700 Subject: [PATCH] fix(plan-eng-review): register the structure question id so never-ask can fire The complexity gate's Original/Smaller arrangement question had no registered question_id, so each run invented a per-plan slug (one install logged ten different ids for it). --check matches ids exactly, so a stored never-ask could never fire and the question was asked every time. - Register plan-eng-review-structure as a two-way routing question. - The template names that exact id and says AUTO_DECIDE picks the recommended arrangement; the preamble's check-first rule does the rest. Wording is byte-neutral against the parity budget and the per-host pins. - --check falls back to $GSTACK_HOME/global-question-preferences.json, the file the question-preference hook already reads, when the project file has no entry for the id, so one setting covers every repo and both layers agree. A project entry still wins, the one-way and split-chain guards run first, a malformed or non-object global file is ignored with a NOTE, and --write stays per-project. - Tests for the fallback and the template contract; two wording pins updated. Co-Authored-By: Claude Opus 5.5 --- bin/gstack-question-preference | 18 ++++++- plan-eng-review/sections/review-sections.md | 13 ++--- .../sections/review-sections.md.tmpl | 13 ++--- scripts/question-registry.ts | 8 +++ test/eng-review-routing.test.ts | 8 ++- test/gstack-question-preference.test.ts | 51 +++++++++++++++++++ test/plan-review-cases.test.ts | 2 +- 7 files changed, 97 insertions(+), 16 deletions(-) diff --git a/bin/gstack-question-preference b/bin/gstack-question-preference index 284d57dbd..efadbcad3 100755 --- a/bin/gstack-question-preference +++ b/bin/gstack-question-preference @@ -2,6 +2,9 @@ # gstack-question-preference — read/write/check explicit per-question preferences. # # Preference file: ~/.gstack/projects/{SLUG}/question-preferences.json +# Global fallback: ~/.gstack/global-question-preferences.json, the file the +# question-preference hook reads (--check only; a project entry for the same +# id wins). Hand-edited; --write stays per-project. # Schema: { "": "always-ask" | "never-ask" | "ask-only-for-one-way" } # # Subcommands: @@ -42,6 +45,7 @@ GSTACK_HOME="$_gstack_sr_root" eval "$("$SCRIPT_DIR/gstack-slug" 2>/dev/null || true)" SLUG="${SLUG:-unknown}" PREF_FILE="$GSTACK_HOME/projects/$SLUG/question-preferences.json" +GLOBAL_PREF_FILE="$GSTACK_HOME/global-question-preferences.json" EVENT_FILE="$GSTACK_HOME/projects/$SLUG/question-events.jsonl" mkdir -p "$GSTACK_HOME/projects/$SLUG" @@ -101,13 +105,23 @@ do_check() { fi ensure_file cd "$ROOT_DIR" - PREF_FILE_PATH="$PREF_FILE" QID="$QID" QSUMMARY="$QSUMMARY" bun -e " + PREF_FILE_PATH="$PREF_FILE" GLOBAL_PREF_FILE_PATH="$GLOBAL_PREF_FILE" QID="$QID" QSUMMARY="$QSUMMARY" bun -e " import('./scripts/one-way-doors.ts').then((oneway) => { const fs = require('fs'); const qid = process.env.QID; const summary = process.env.QSUMMARY || undefined; const prefs = JSON.parse(fs.readFileSync(process.env.PREF_FILE_PATH, 'utf-8')); - const pref = prefs[qid]; + let globalPrefs = {}; + try { + if (fs.existsSync(process.env.GLOBAL_PREF_FILE_PATH)) { + const parsed = JSON.parse(fs.readFileSync(process.env.GLOBAL_PREF_FILE_PATH, 'utf-8')); + if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) throw new Error('not an object'); + globalPrefs = parsed; + } + } catch (e) { + console.error('NOTE: global-question-preferences.json is unreadable; ignoring it.'); + } + const pref = qid in prefs ? prefs[qid] : globalPrefs[qid]; // Always check one-way status first — safety overrides preferences. // summary (when piped) lets the keyword net catch ad-hoc destructive diff --git a/plan-eng-review/sections/review-sections.md b/plan-eng-review/sections/review-sections.md index 6c3f5e282..76196859d 100644 --- a/plan-eng-review/sections/review-sections.md +++ b/plan-eng-review/sections/review-sections.md @@ -441,18 +441,19 @@ Initial scope selectors need no grid or **pre-answer** ledger write. Ask and wait before changes. 1. Explain the complexity. Ask each proposed feature cut/deferral separately; - wait before changing scope. With no proposed cuts, keep the feature list and - go directly to the structure question. -2. Always ask the structure question when this gate trips, even with no cuts. - Compare only the file/class arrangement. Use labels `Original arrangement` - and `Smaller arrangement`; put files/classes in each description. Both retain + wait before changing scope. +2. Always resolve the structure question when this gate trips, even with no cuts, + as question_id `plan-eng-review-structure` (no per-plan slug); `AUTO_DECIDE` + picks the recommendation. Compare only the file/class arrangement, labeled + `Original arrangement` and `Smaller arrangement`, with files/classes per + option. Both retain the same approved feature list, contracts and approved security/error/test/performance fixes. Include `Pending remedies not decided here: ` in the question; unapproved fixes stay pending. If no smaller arrangement preserves these commitments, explain that and offer confirmation of the original arrangement or a pause to investigate a smaller one. Wait for the answer. A pause leaves the arrangement undecided: investigate only the agreed question, - then return to this structure selector. Do not continue to C until it is settled. + then return to the selector. Do not continue to C until it is settled. 3. Save the actual feature and structure answers as one scope record: `feature answers: ; structure: ; accepted scope: ; pending remedies: `. diff --git a/plan-eng-review/sections/review-sections.md.tmpl b/plan-eng-review/sections/review-sections.md.tmpl index 737e3db37..63f578797 100644 --- a/plan-eng-review/sections/review-sections.md.tmpl +++ b/plan-eng-review/sections/review-sections.md.tmpl @@ -339,18 +339,19 @@ Initial scope selectors need no grid or **pre-answer** ledger write. Ask and wait before changes. 1. Explain the complexity. Ask each proposed feature cut/deferral separately; - wait before changing scope. With no proposed cuts, keep the feature list and - go directly to the structure question. -2. Always ask the structure question when this gate trips, even with no cuts. - Compare only the file/class arrangement. Use labels `Original arrangement` - and `Smaller arrangement`; put files/classes in each description. Both retain + wait before changing scope. +2. Always resolve the structure question when this gate trips, even with no cuts, + as question_id `plan-eng-review-structure` (no per-plan slug); `AUTO_DECIDE` + picks the recommendation. Compare only the file/class arrangement, labeled + `Original arrangement` and `Smaller arrangement`, with files/classes per + option. Both retain the same approved feature list, contracts and approved security/error/test/performance fixes. Include `Pending remedies not decided here: ` in the question; unapproved fixes stay pending. If no smaller arrangement preserves these commitments, explain that and offer confirmation of the original arrangement or a pause to investigate a smaller one. Wait for the answer. A pause leaves the arrangement undecided: investigate only the agreed question, - then return to this structure selector. Do not continue to C until it is settled. + then return to the selector. Do not continue to C until it is settled. 3. Save the actual feature and structure answers as one scope record: `feature answers: ; structure: ; accepted scope: ; pending remedies: `. diff --git a/scripts/question-registry.ts b/scripts/question-registry.ts index 075bc68b0..583e00797 100644 --- a/scripts/question-registry.ts +++ b/scripts/question-registry.ts @@ -336,6 +336,14 @@ export const QUESTIONS = { signal_key: 'scope-appetite', description: "Plan touches 8+ files — reduce scope or hold?", }, + 'plan-eng-review-structure': { + id: 'plan-eng-review-structure', + skill: 'plan-eng-review', + category: 'routing', + door_type: 'two-way', + options: ['original-arrangement', 'smaller-arrangement'], + description: "Complexity gate — Original arrangement or Smaller arrangement (file/class layout only)?", + }, 'plan-eng-review-test-gap': { id: 'plan-eng-review-test-gap', skill: 'plan-eng-review', diff --git a/test/eng-review-routing.test.ts b/test/eng-review-routing.test.ts index 082959b7c..6c8372d7d 100644 --- a/test/eng-review-routing.test.ts +++ b/test/eng-review-routing.test.ts @@ -76,10 +76,16 @@ describe('engineering review routing contracts', () => { test('high complexity stops before Section 1 and asks cuts and structure separately', () => { expectMentions(compact(complexity), [['stop', 'classes/services', 'section']], 'compact(complexity)'); ordered(compact(complexity), ['1. Explain the complexity', 'Ask each proposed feature cut/deferral separately', - '2. Always ask the structure question', '3. Save the actual feature and structure answers']); + '2. Always resolve the structure question', '3. Save the actual feature and structure answers']); expect(compact(complexity)).toContain('Pending remedies not decided here: '); }); + test('the structure question uses its registered id and honors a stored preference', () => { + ordered(compact(complexity), ['2. Always resolve the structure question', + 'as question_id `plan-eng-review-structure` (no per-plan slug)', '`AUTO_DECIDE` picks the recommendation', + 'Compare only the file/class arrangement']); + }); + test('no safe smaller arrangement does not authorize scope cuts or bypass the pause', () => { ordered(compact(complexity), ['If no smaller arrangement preserves these commitments', 'A pause leaves the arrangement undecided', 'Do not continue to C until it is settled']); diff --git a/test/gstack-question-preference.test.ts b/test/gstack-question-preference.test.ts index f74465eca..9c6437da0 100644 --- a/test/gstack-question-preference.test.ts +++ b/test/gstack-question-preference.test.ts @@ -132,6 +132,57 @@ describe('--check with preferences set', () => { }); }); +// The global file ($GSTACK_HOME/global-question-preferences.json, the same file +// the question-preference hook reads) is a hand-edited fallback for --check +// only: one setting covers every project, a project entry for the same id +// wins, and the one-way / split-chain guards still run. +describe('--check global preference fallback', () => { + function plantGlobal(contents: string) { + fs.writeFileSync(path.join(tmpHome, 'global-question-preferences.json'), contents); + } + + test('global never-ask with no project entry → AUTO_DECIDE', () => { + plantGlobal(JSON.stringify({ 'plan-eng-review-structure': 'never-ask' })); + const r = run('--check', 'plan-eng-review-structure'); + expect(r.status).toBe(0); + expect(r.stdout.trim()).toBe('AUTO_DECIDE'); + }); + + test('project entry wins over the global entry', () => { + plantGlobal(JSON.stringify({ 'plan-eng-review-structure': 'never-ask' })); + run('--write', JSON.stringify({ question_id: 'plan-eng-review-structure', preference: 'always-ask', source: 'plan-tune' })); + const r = run('--check', 'plan-eng-review-structure'); + expect(r.stdout.trim()).toBe('ASK_NORMALLY'); + }); + + test('global never-ask on a one-way id → ASK_NORMALLY with safety note', () => { + plantGlobal(JSON.stringify({ 'ship-test-failure-triage': 'never-ask' })); + const r = run('--check', 'ship-test-failure-triage'); + expect(r.stdout).toContain('ASK_NORMALLY'); + expect(r.stdout).toContain('one-way door overrides'); + }); + + test('global never-ask on a split-chain id → ASK_NORMALLY', () => { + plantGlobal(JSON.stringify({ 'plan-eng-review-split-option-a': 'never-ask' })); + const r = run('--check', 'plan-eng-review-split-option-a'); + expect(r.stdout.split('\n')[0]).toBe('ASK_NORMALLY'); + }); + + test.each(['{not json', 'null', '["never-ask"]'])('unusable global file %p is ignored with a note', (contents) => { + plantGlobal(contents); + const r = run('--check', 'plan-eng-review-structure'); + expect(r.status).toBe(0); + expect(r.stdout.trim()).toBe('ASK_NORMALLY'); + expect(r.stderr).toContain('global-question-preferences.json is unreadable'); + }); + + test('--write stays per-project and never touches the global file', () => { + plantGlobal('{}'); + run('--write', JSON.stringify({ question_id: 'plan-eng-review-structure', preference: 'never-ask', source: 'plan-tune' })); + expect(fs.readFileSync(path.join(tmpHome, 'global-question-preferences.json'), 'utf-8')).toBe('{}'); + }); +}); + // #2024: the keyword net only fires when the question TEXT reaches the // classifier. --summary-stdin pipes it (stdin, not argv — summaries carry // quotes/newlines/shell metacharacters). Without the summary, an unregistered diff --git a/test/plan-review-cases.test.ts b/test/plan-review-cases.test.ts index 738d62188..daccb7ba1 100644 --- a/test/plan-review-cases.test.ts +++ b/test/plan-review-cases.test.ts @@ -319,7 +319,7 @@ test('Eng loads its one remedy procedure before Scope Challenge findings and ret const scope = compactProse(sections.slice(sections.indexOf('## Scope Challenge'), sections.indexOf('## Review Sections'))); expectAll(scope, [/decision-brief format for this complexity gate/i, /ask each proposed feature cut\/deferral separately; wait before changing scope/i, - /unapproved fixes stay pending/i, /always ask the structure question when this gate trips/i, + /unapproved fixes stay pending/i, /always resolve the structure question when this gate trips/i, /offer confirmation of the original arrangement or a pause to investigate a smaller one\. wait for the answer/i, /apply only accepted scope changes/i, /"No issues found" for an empty list/i, /findings and scope answers approve no remedies/i]); const complexityRule = at(scope, /initial scope selectors need no grid or \*\*pre-answer\*\* ledger write/i);