From 1c32c7d16ae6a1146cdb7668d33b088ec66ac0aa Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 29 Sep 2026 17:10:33 +0000 Subject: [PATCH] test(ceo-mode-routing): accept the skill-mandated Note form and Recommendation reason as HOLD posture HOLD Defer/Keep briefs must use 'Note: options differ in kind' (preamble), but the answered-HOLD path demanded a Completeness score, rejected a one-line Net with a semicolon, and read posture only from ELI10. The rerun's brief applied HOLD SCOPE in its Recommendation reason. Revert the ineffective 'always'/'handoff chat' wording: two runs still skipped the mode handoff. --- plan-ceo-review/SKILL.md | 4 +- plan-ceo-review/SKILL.md.tmpl | 4 +- test/ceo-mode-option.test.ts | 17 +++++++++ .../ceo-hold-note-briefs-36597762183.json | 37 +++++++++++++++++++ test/helpers/ceo-mode-option.ts | 9 +++-- 5 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 test/fixtures/ceo-hold-note-briefs-36597762183.json diff --git a/plan-ceo-review/SKILL.md b/plan-ceo-review/SKILL.md index 4f46d50de..b0423b53e 100644 --- a/plan-ceo-review/SKILL.md +++ b/plan-ceo-review/SKILL.md @@ -1032,11 +1032,11 @@ Follow the preamble's session rules; `CONDUCTOR_SESSION: true` changes transport wins. When `QUESTION_TUNING: true`, include ``. These modes differ in kind, not coverage; do NOT score completeness. -4. **Mode handoff:** After selection, always send brief chat before tools or further questions: the mode's application and rationale; every governing approved row's ID, answer reference and accepted scope. Keep rows separate. +4. **Mode handoff:** After selection, send brief chat before tools or further questions: the mode's application and rationale; every governing approved row's ID, answer reference and accepted scope. Keep rows separate. - `plan-ceo-review-mode: AUTO_DECIDE`: `Auto-decided review mode → (your preference). Change with /plan-tune. Approved decisions: . .` - Other selections: `Mode: ; approved decisions: . .` -Record mode provenance after the handoff chat: +Record mode provenance after the handoff: - **Explicit user choice:** instruction and mode; no question log because none was asked. - **Successful preference check:** result and recommendation; log `plan-ceo-review-mode`, `auto_decided: true`. - **Actual question answer:** question, answer reference and mode; log `auto_decided: false`, including the question ID only when `QUESTION_TUNING: true`. diff --git a/plan-ceo-review/SKILL.md.tmpl b/plan-ceo-review/SKILL.md.tmpl index 8b47e3195..cd7bb16f1 100644 --- a/plan-ceo-review/SKILL.md.tmpl +++ b/plan-ceo-review/SKILL.md.tmpl @@ -415,11 +415,11 @@ Follow the preamble's session rules; `CONDUCTOR_SESSION: true` changes transport wins. When `QUESTION_TUNING: true`, include ``. These modes differ in kind, not coverage; do NOT score completeness. -4. **Mode handoff:** After selection, always send brief chat before tools or further questions: the mode's application and rationale; every governing approved row's ID, answer reference and accepted scope. Keep rows separate. +4. **Mode handoff:** After selection, send brief chat before tools or further questions: the mode's application and rationale; every governing approved row's ID, answer reference and accepted scope. Keep rows separate. - `plan-ceo-review-mode: AUTO_DECIDE`: `Auto-decided review mode → (your preference). Change with /plan-tune. Approved decisions: . .` - Other selections: `Mode: ; approved decisions: . .` -Record mode provenance after the handoff chat: +Record mode provenance after the handoff: - **Explicit user choice:** instruction and mode; no question log because none was asked. - **Successful preference check:** result and recommendation; log `plan-ceo-review-mode`, `auto_decided: true`. - **Actual question answer:** question, answer reference and mode; log `auto_decided: false`, including the question ID only when `QUESTION_TUNING: true`. diff --git a/test/ceo-mode-option.test.ts b/test/ceo-mode-option.test.ts index 27ffe4aaa..edcc8f8ad 100644 --- a/test/ceo-mode-option.test.ts +++ b/test/ceo-mode-option.test.ts @@ -824,6 +824,23 @@ const mutations:Recordvoid>={ }; for(const [name,mutate] of Object.entries(mutations))test(name,()=>{const x=clone();mutate(x);expect(check(x)).toBe(false)}); test('later quoted withdrawal is not current withdrawal',()=>{const x=clone();x.transcript.assistantMessages.push({sessionId:decision(x).sessionId,timestamp:new Date().toISOString(),text:'Example: "I withdraw this decision."'});expect(check(x)).toBe(true)}); +{ + const briefs = require('./fixtures/ceo-hold-note-briefs-36597762183.json'); + const withBrief = (brief: any, change: (q: any) => void = () => {}) => { + const x = clone(); const c = decision(x); const before = c.questions[0].question; + const q = structuredClone(brief); change(q); c.questions[0] = q; delete c.answers[before]; c.answers[q.question] = q.options[0].label; + x.tools.find((t: any) => t.kind === 'use' && t.toolUseId === c.toolUseId).input.questions = structuredClone(c.questions); + return check(x); + }; + test('census HOLD Defer/Keep brief with the Note form and a one-line Net applies HOLD in its ELI10', () => expect(withBrief(briefs.census)).toBe(true)); + test('rerun HOLD Defer/Keep brief applies HOLD in its Recommendation reason', () => expect(withBrief(briefs.rerun)).toBe(true)); + test.each([ + ['no HOLD rationale', (q: any) => { q.question = q.question.replace('HOLD SCOPE preserves stated scope by default, ', ''); }], + ['a second sentence after Net', (q: any) => { q.question = q.question.replace(/(Net:[^\n]*)$/, '$1 Also add shared views.'); }], + ['a foreign-mode context', (q: any) => { q.question = q.question.replace('HOLD SCOPE review', 'SCOPE EXPANSION review'); }], + ['a missing Note or score', (q: any) => { q.question = q.question.replace(/Note: options differ[^\n]*\n/, ''); }], + ])('rerun brief with %s is not HOLD posture', (_name, change) => expect(withBrief(briefs.rerun, change)).toBe(false)); +} test('new proof path is unavailable without explicit fixture source binding',()=>{const x=clone();expect(hasNativePostAnswerCeoPosture(x.transcript,'HOLD SCOPE',posture,x.selectionStartedAt,x.tools)).toBe(false)}); test('retry source cat requires the actual owned project',()=>{const x=clone(1);x.tools.find((t:any)=>t.kind==='use'&&t.input?.command?.includes('cat PLAN.md')).input.command=x.tools.find((t:any)=>t.kind==='use'&&t.input?.command?.includes('cat PLAN.md')).input.command.replace(x.source.path.replace('/PLAN.md',''),'/foreign');expect(check(x)).toBe(false)}); diff --git a/test/fixtures/ceo-hold-note-briefs-36597762183.json b/test/fixtures/ceo-hold-note-briefs-36597762183.json new file mode 100644 index 000000000..f4ab7a1df --- /dev/null +++ b/test/fixtures/ceo-hold-note-briefs-36597762183.json @@ -0,0 +1,37 @@ +{ + "provenance": { + "census": "36597762183 eval-slices-4 HOLD D2", + "rerun": "local repair rerun HOLD D3", + "note": "Defer/Keep briefs use the preamble's Note form; posture appears in ELI10 (census) or the Recommendation reason (rerun)" + }, + "census": { + "question": "D2 — R2: Keep or defer the saved-view update endpoint?\nProject/branch/task: gstack-plan-count-PhqSAE on main, HOLD SCOPE review of saved project views.\nELI10: The plan lists four endpoints: create, list, update, delete. \"Update\" is what lets a member rename a view or overwrite its filters after tweaking them. The stated goal (save a named view, reopen it later) still works without it: delete the old view and save a new one. HOLD SCOPE asks me to flag anything deferrable, so this is that flag. Deferring saves one endpoint, one UI flow and their tests; keeping it means a member who adjusts a filter can hit \"update\" instead of rebuilding the view from scratch.\nStakes if we pick wrong: Defer and the pilot's reuse metric may drop because stale views get abandoned instead of fixed; keep and we spend a small amount more before adoption is proven.\nRecommendation: B) Keep because the endpoint reuses the create path's validation and authorization almost verbatim (human: ~half a day / CC: ~5 min), and \"my view drifted, let me fix it\" is the exact moment a user decides whether this feature is worth using.\nNote: options differ in kind, not coverage — no completeness score.\nPros / cons:\nA) Defer update to TODOS.md\n ✅ Removes one endpoint, one UI flow and two tests from the first ship; smaller diff to review and roll out\n ✅ Lets the two-week pilot show whether anyone actually edits views before building it\n ❌ A member whose filters drift has to delete and recreate; stale views quietly stop being used and the reuse metric under-reads\nB) Keep update in scope (recommended)\n ✅ Rename and overwrite reuse the create endpoint's validation, ownership check and project scoping, so the marginal cost is small\n ✅ Views stay alive as the project changes (new statuses, new assignees), which is exactly the \"reopen after task changes\" acceptance criterion\n ❌ Slightly more surface to test: concurrent edits from two tabs and rename-to-duplicate-name need explicit handling\nNet: A trades a small first-ship saving for a real risk of the pilot under-measuring; B costs little because it is mostly the create path again.", + "header": "R2 update", + "multiSelect": false, + "options": [ + { + "label": "Defer update to TODOS.md", + "description": "Effort S, risk low. Reuse: none removed. Verification: no update tests. Drops the PATCH endpoint plus rename/overwrite UI to TODOS.md; users delete and re-save. ✅ Smaller first ship. ✅ Pilot decides if editing is wanted. ❌ Stale views get abandoned, reuse metric under-reads." + }, + { + "label": "Keep update in scope (recommended)", + "description": "Effort S, risk low. Reuse: create path's validation, ownership and project scoping. Verification: PATCH request spec (owner, non-owner, other project, missing view) + UI rename/overwrite flow test. ✅ Marginal cost is small. ✅ Views survive project drift. ❌ Must handle two-tab concurrent edits and duplicate names." + } + ] + }, + "rerun": { + "question": "D3 — UPDATE-EP: Defer the update endpoint (rename / overwrite a saved view) to TODOS.md, or keep it in scope?\nProject/branch/task: main, HOLD SCOPE review of PLAN.md (saved project views).\nELI10: The plan lists create, list, update and delete. Update is the one piece the goal does not strictly need: a member can delete a view and save a new one. Keeping it means one more route, action, UI edit control and test group; dropping it means renaming a view is a two-step chore and \"overwrite this view with my current filters\" is impossible until it ships later.\nStakes if we pick wrong: Defer wrongly and pilot users hit a papercut on the first rename; keep wrongly and you spend ~10% more effort on a feature whose reuse you are still measuring.\nRecommendation: B) Keep because update is already in the written plan, HOLD SCOPE preserves stated scope by default, and the extra cost is small (human: ~half a day / CC: ~5 min) while the UX cost of a missing rename shows up in the very pilot you are measuring.\nNote: options differ in kind, not coverage — no completeness score.\nNet: a small effort saving now vs. a visible papercut during the pilot.", + "header": "Update endpoint", + "multiSelect": false, + "options": [ + { + "label": "Defer update endpoint to TODOS.md", + "description": "Ship create/list/delete only; members rename by delete + re-save. Effort S (removes work), risk low, reuse n/a, verification: existing create/list/delete tests. ✅ Fewer routes, actions and UI states to build and test during the pilot. ✅ Smallest possible surface if the pilot shows nobody reuses views. ❌ Renaming or updating a view becomes a two-step chore that pilot users will notice and report." + }, + { + "label": "Keep update endpoint in scope (recommended)", + "description": "Keep rename + overwrite-filters as written, with its own tests. Effort S (human: ~half a day / CC: ~5 min), risk low, reuse: same controller and policy as the other three actions, verification: rename, overwrite, cross-member 403, stale-name 404 tests. ✅ Matches the plan as written; no scope change to explain to the team. ✅ \"Save current filters to this view\" is the natural gesture once someone tweaks a view. ❌ One more action, edit affordance and test group before the pilot can start." + } + ] + } +} \ No newline at end of file diff --git a/test/helpers/ceo-mode-option.ts b/test/helpers/ceo-mode-option.ts index 9cd5fb169..74531e4e3 100644 --- a/test/helpers/ceo-mode-option.ts +++ b/test/helpers/ceo-mode-option.ts @@ -291,7 +291,8 @@ function singleScopeBrief(text: string, descriptions: readonly string[], compari quote => quote.replace(/\?/g, '')) : text; const questions = questionText.replace(/\?[A-Za-z_][\w-]*=/g, '=').match(/\?/g); if ((questions?.length ?? 0) !== (proposalHeading ? 0 : 1) || /```|~~~|^\s*>/m.test(text)) return false; - const comparisonMarker = expansion + // The preamble requires the Note form for different-kind menus (Add/Defer/Skip, Defer/Keep). + const comparisonMarker = expansion || !comparison ? /Completeness:|Note:\s*options differ in kind, not coverage\s*[—–-]\s*no completeness score\./gi : /Completeness:/gi; const markers = [/Project\/branch\/task:/gi, /ELI10:/gi, /Stakes if (?:we pick )?wrong:/gi, @@ -312,7 +313,7 @@ function singleScopeBrief(text: string, descriptions: readonly string[], compari if (!ratings.length || ratings.some(score => Number(score[1]) > 10)) return false; } return complete && (comparison ? /^[^.!?;\n]+ (?:vs|versus) [^.!?;\n]+\.$/.test(net) - : /^[^.!?;\n]+\.$/.test(net.replace(/\bvs\./gi, 'vs'))); + : /^[^.!?\n]+\.$/.test(net.replace(/\bvs\./gi, 'vs'))); } /** Fixture-owned baseline for a completed scope-preservation decision. */ @@ -388,7 +389,9 @@ function hasAnsweredHoldPosture(transcript: PlanCountTranscript, selected: Nativ // standalone prose is published. Metadata and answer echoes do not count. // This recognizes posture language; it does not validate every scope choice. const context = /Project\/branch\/task:([\s\S]*?)(?=ELI10:)/i.exec(q.question)?.[1] ?? ''; - const rationale = /ELI10:([\s\S]*?)(?=Stakes if (?:we pick )?wrong:)/i.exec(q.question)?.[1]?.trim() ?? ''; + // The ELI10 and the Recommendation's reason are both the brief's own rationale. + const rationale = [/ELI10:([\s\S]*?)(?=Stakes if (?:we pick )?wrong:)/i, /Recommendation:[^\n]*?\bbecause\b([^\n]*)/i] + .map(part => part.exec(q.question)?.[1]?.trim() ?? '').join('\n'); const offered = q.options.map(o => o.label.trim()); if (!q.multiSelect && q.options.length >= 2 && q.options.length <= 4 && new Set(offered).size === offered.length && offered.includes(call.answers?.[q.question] ?? '') && /\bHOLD SCOPE\b/i.test(context) &&