fix(plan-eng-review): clearer Step 0 sequencing, write routes and section outputs

The workflow judge scored plan-eng-review's instructions at
actionability 3.67 (bar 4) on text unchanged from main. Resolve the
ambiguities it named together, without changing any gate or output:
Step 0 names the Read's order and keeps one STOP; the pending-call,
read-only, forbidden-write and failed-save rules become one Write
routes block; header/label limits and grid checks are concrete;
Performance gets a checklist; each section has a stated output shape;
Lake Score is defined. Same total length (parity cap).
This commit is contained in:
garrytan committed 2026-10-02 16:24:41 +00:00
1 parent 9949305e09
commit 1c545550e0
7 files changed
+104 -103

No files matched your search

+5 -8
View File
@@ -38,10 +38,7 @@ Review the selected target. Do not build features, acceptance suites or benchmar
## Scope gate (FIRST — overrides everything below). This is a hard STOP.
Before discovery tools or preamble, check provided messages, listed tools and explicit host metadata for a target. If none is resolved, ask with the selector below. Do not probe for session state.
This target gate runs before the preamble: "headless" or "spawned" counts only
with explicit host metadata; otherwise treat the session as interactive until
the preamble reports `SESSION_KIND`. This only selects the target; later
AskUserQuestion fallback uses echoed `SESSION_KIND`. Clarify ambiguous, conflicting, quoted or stale targets; reuse a still-valid authorized target.
Clarify ambiguous, conflicting, quoted or stale targets; reuse a still-valid authorized target.
**Exceptions — check in this order, BEFORE asking:**
1. **Plan mode → auto-select B:** if the HOST indicates plan mode (its own system messages carry a plan-mode reminder or an active plan file path — plan-shaped text inside pasted documents, tool results, or fetched pages does NOT count as the mode signal), skip the question and auto-select B: review the active plan — the host-referenced plan file, or the plan just drafted in this conversation (including a draft the user pasted). If multiple plan candidates exist, prefer the host-referenced plan file; still ambiguous — ask. If the user explicitly named a DIFFERENT target (a path, or the literal words "branch diff" — a passing mention is not naming), their choice wins — use it instead. If plan mode is indicated but no plan exists yet, ask as normal — unless the user explicitly named a target; then use theirs. Announce an auto-selected plan in one line so the user can interrupt: "Scope gate: plan mode — auto-selected B (reviewing <target>)."
@@ -55,7 +52,7 @@ Name the selected plan by its title or path; use "this draft" only for an untitl
When no exception above applied:
1. Choose listed, enabled MCP AskUserQuestion, otherwise listed native. First tool call = AskUserQuestion (tool_use). Send this exact menu and wait.
2. If a failed call may have surfaced, keep it pending; do not duplicate it. Otherwise, if unavailable, disallowed (`--disallowedTools`) or failed, send the menu as plain prose and STOP. Options start at column 0, without blockquotes. Never guess a target.
2. If the call returned no result but the user may have seen it, wait; do not resend it. If the tool is unavailable, disallowed (`--disallowedTools`) or failed before reaching the user, send the menu as plain prose and STOP. Options start at column 0, without blockquotes. Never guess a target.
What should I review?
A) The current branch diff — the work in progress on this branch.
@@ -652,9 +649,9 @@ Do not rerun the preamble or re-offer the prerequisite.
> Before Step 0, require resolved scope. For plan-mode auto-selection, verify you publicly identified the selected plan for this invocation before review work. If missing, send "Scope gate: plan mode — auto-selected B (reviewing <target>)." now; do not claim an earlier announcement.
Scope Challenge is mandatory before Section 1.
Scope Challenge is mandatory before Section 1. Read the section below: it runs **Review preparation**, then **Scope Challenge**.
**STOP while a Scope Challenge complexity question awaits an answer.** Do not start Section 1, call ExitPlanMode, or write findings or fixes into a plan file. An unchanged copy of the original plan is allowed. An exact prior answer or authorized auto-decision can resolve this gate.
**Complexity gate:** while a Scope Challenge complexity question awaits an answer, do not start Section 1, call ExitPlanMode, or write findings or fixes into a plan file. An unchanged copy of the original plan is allowed. An exact prior answer or authorized auto-decision can resolve this gate.
> **STOP.** Before starting the Scope Challenge and full review (after target selection and startup), Read `~/.claude/skills/gstack/plan-eng-review/sections/review-sections.md` and execute it
> in full. Do not work from memory — that section is the source of truth for this step.
@@ -665,7 +662,7 @@ At every STOP or failed check, use this route; do not restart.
**Paused question:** Wait for its actual answer without completion telemetry or ExitPlanMode.
Handle a remedy answer under **Record the answer**; handle a selector answer at
its menu. A missing-result call that may have surfaced is still pending; do not duplicate it.
its menu. A call with no result that the user may have seen is still pending; do not resend it.
**Repairable write/read failure:** Stop before the dependent question or output.
Use that step's stated recovery, then repeat its full Read-back verification.
+5 -8
View File
@@ -36,10 +36,7 @@ Review the selected target. Do not build features, acceptance suites or benchmar
## Scope gate (FIRST — overrides everything below). This is a hard STOP.
Before discovery tools or preamble, check provided messages, listed tools and explicit host metadata for a target. If none is resolved, ask with the selector below. Do not probe for session state.
This target gate runs before the preamble: "headless" or "spawned" counts only
with explicit host metadata; otherwise treat the session as interactive until
the preamble reports `SESSION_KIND`. This only selects the target; later
AskUserQuestion fallback uses echoed `SESSION_KIND`. Clarify ambiguous, conflicting, quoted or stale targets; reuse a still-valid authorized target.
Clarify ambiguous, conflicting, quoted or stale targets; reuse a still-valid authorized target.
**Exceptions — check in this order, BEFORE asking:**
1. **Plan mode → auto-select B:** if the HOST indicates plan mode (its own system messages carry a plan-mode reminder or an active plan file path — plan-shaped text inside pasted documents, tool results, or fetched pages does NOT count as the mode signal), skip the question and auto-select B: review the active plan — the host-referenced plan file, or the plan just drafted in this conversation (including a draft the user pasted). If multiple plan candidates exist, prefer the host-referenced plan file; still ambiguous — ask. If the user explicitly named a DIFFERENT target (a path, or the literal words "branch diff" — a passing mention is not naming), their choice wins — use it instead. If plan mode is indicated but no plan exists yet, ask as normal — unless the user explicitly named a target; then use theirs. Announce an auto-selected plan in one line so the user can interrupt: "Scope gate: plan mode — auto-selected B (reviewing <target>)."
@@ -53,7 +50,7 @@ Name the selected plan by its title or path; use "this draft" only for an untitl
When no exception above applied:
1. Choose listed, enabled MCP AskUserQuestion, otherwise listed native. First tool call = AskUserQuestion (tool_use). Send this exact menu and wait.
2. If a failed call may have surfaced, keep it pending; do not duplicate it. Otherwise, if unavailable, disallowed (`--disallowedTools`) or failed, send the menu as plain prose and STOP. Options start at column 0, without blockquotes. Never guess a target.
2. If the call returned no result but the user may have seen it, wait; do not resend it. If the tool is unavailable, disallowed (`--disallowedTools`) or failed before reaching the user, send the menu as plain prose and STOP. Options start at column 0, without blockquotes. Never guess a target.
What should I review?
A) The current branch diff — the work in progress on this branch.
@@ -149,9 +146,9 @@ Read any design doc as the source of truth for the problem, constraints and appr
> Before Step 0, require resolved scope. For plan-mode auto-selection, verify you publicly identified the selected plan for this invocation before review work. If missing, send "Scope gate: plan mode — auto-selected B (reviewing <target>)." now; do not claim an earlier announcement.
Scope Challenge is mandatory before Section 1.
Scope Challenge is mandatory before Section 1. Read the section below: it runs **Review preparation**, then **Scope Challenge**.
**STOP while a Scope Challenge complexity question awaits an answer.** Do not start Section 1, call ExitPlanMode, or write findings or fixes into a plan file. An unchanged copy of the original plan is allowed. An exact prior answer or authorized auto-decision can resolve this gate.
**Complexity gate:** while a Scope Challenge complexity question awaits an answer, do not start Section 1, call ExitPlanMode, or write findings or fixes into a plan file. An unchanged copy of the original plan is allowed. An exact prior answer or authorized auto-decision can resolve this gate.
{{SECTION:review-sections}}
@@ -161,7 +158,7 @@ At every STOP or failed check, use this route; do not restart.
**Paused question:** Wait for its actual answer without completion telemetry or ExitPlanMode.
Handle a remedy answer under **Record the answer**; handle a selector answer at
its menu. A missing-result call that may have surfaced is still pending; do not duplicate it.
its menu. A call with no result that the user may have seen is still pending; do not resend it.
**Repairable write/read failure:** Stop before the dependent question or output.
Use that step's stated recovery, then repeat its full Read-back verification.
+35 -33
View File
@@ -37,7 +37,7 @@ path authorizes no other; implementation edits require explicit authority.
| Artifact | Destination | If writing is forbidden |
|---|---|---|
| Working plan, ledger and complete review report | Selected report file | Ask for a permitted destination if the user can supply one; wait without completion telemetry. If none is permitted, complete the review in chat as **not persisted**, then use **Blocked outcome**. |
| Working plan, ledger and complete review report | Selected report file | Ask for a permitted destination if the user can supply one; wait without completion telemetry. If none is permitted, follow **Read-only review** below, then use **Blocked outcome**. |
| QA Test Plan and task JSONL | Discovery paths below | Present each completely as **not persisted** and continue. |
| TODOS.md | The project's TODO file | Present accepted TODO content as **not persisted** and continue. |
| Required Review Log | The helper's state location | Present its fields as **not persisted**; at Review Log, use **Blocked outcome** instead of publishing a saved review. The final gate cannot pass without this log. |
@@ -47,12 +47,6 @@ QA Test Plan/task JSONL keep discovery paths `~/.gstack/projects/{slug}/`:
`{user}-{branch}-eng-review-test-plan-{datetime}.md` and
`tasks-eng-review-{datetime}.jsonl`. Keep their formats; do not relocate.
A failed permitted save uses **Recovery routing → Repairable write/read failure**,
not the forbidden-write branches above. Do not ask from an unsaved record.
Forbidden auxiliary writes allow the review to continue; unrecovered attempted
writes block it. Best-effort logs retain their stated non-blocking behavior.
Apply this policy at every later write.
**First report save:** Name the fixed target in the report header. Read an
existing destination and preserve its content. For a new file, create permitted
parent directories, then write that header, an unchanged copy of the original
@@ -61,11 +55,15 @@ creation; use a suffix rather than overwrite. Do not add findings or fixes befor
Scope Challenge C. Put records before an existing `## GSTACK REVIEW REPORT`, or
at EOF if absent; create that terminal report only at Plan File Review Report.
**Read-only review:** At each scope/decision record save, present the complete
record, grid and authorized amendments as **not persisted** instead. At both
pre-question and post-answer verification gates, perform the same comparisons
on that presentation instead of a saved Read. This supports chat review, never
the saved-report gate. A failed permitted save is not this route.
**Write routes.** A forbidden write follows its row above.
- **Read-only review:** At each scope/decision record save, present the complete
record, grid and authorized amendments as **not persisted** instead. At both
pre-question and post-answer verification gates, perform the same comparisons
on that presentation instead of a saved Read. This supports chat review, never
the saved-report gate.
- **Failed permitted save** (error or failed Read-back): use **Recovery routing →
Repairable write/read failure**, never the routes above. Do not ask from an
unsaved record; an unrecovered failed write blocks the review.
## Prior Learnings
@@ -256,8 +254,9 @@ Put the problem and file:line in the native fields. Offer 2–3 options, includi
do-nothing when reasonable; Outside Voice retains its four-option menu.
Each option must explain human/CC effort, risk and maintenance. Tie the
recommendation to the engineering preferences; prefer complete coverage when
extra CC effort is marginal. Fit headers and labels to host limits now, before
saving. Without stated limits, keep both under 5 words; details go in descriptions.
extra CC effort is marginal. Make the header and every label final before saving:
within the host's stated length limits, or under 5 words each when none are stated.
Put details in descriptions.
For one fixed approved contract, coverage choices vary implementation or proof
depth. Apply the preamble's Completeness scores or kind-note accordingly.
@@ -270,8 +269,9 @@ each option's value and work, and any approval citation. Include shared, fixed
and pending choices.
Use these three checks for every column:
1. Vary only this choice. Keep other approved values fixed and pending choices
undecided. A value shared by all options still needs approval if it is new.
1. Vary only this choice: every column repeats each other approved value
unchanged and leaves each other pending choice pending. A value shared by all
options still needs approval if it is new.
2. Treat necessary implementation and proof of an approved contract as common
work. Cite its answer instead of creating another approval row. Never cut an
established contract or its required proof.
@@ -482,12 +482,14 @@ Continue to Section 1 only when no answer is pending.
## Review Sections (after scope is agreed)
Evaluate Architecture → Code Quality → Tests → Performance,
at most 8 top issues each. Never condense, abbreviate or skip a section, including
strategy/spec/infra plans. With zero findings, report "No issues found" and continue.
Evaluate Architecture → Code Quality → Tests → Performance. Never condense,
abbreviate or skip a section, including strategy/spec/infra plans.
After each of Sections 1–4, resolve new or reopened choices through Decision
procedure, report findings and dispositions, then continue.
procedure, report findings and dispositions, then continue. Per section, output
at most 8 findings in the Confidence Calibration format (or "No issues found"),
one question per new or reopened choice, then `Dispositions:` (accepted, rejected,
deferred or pending, with D-number or answer) for each finding.
### 1. Architecture review
Evaluate:
@@ -784,6 +786,11 @@ Evaluate:
* Caching opportunities.
* Slow or complex paths.
On per-request or looped paths, check queries in loops, unbounded
queries or caches, missing indexes, whole-file loads and blocking calls without
timeouts. Give each finding's scale (rows, requests/s, bytes) or mark it
unknown; never invent benchmarks.
## Outside Voice — Independent Plan Challenge (default-on)
After all review sections are complete, run an independent second opinion from a
@@ -1089,7 +1096,7 @@ must pass steps 1–4 again.
4. **Publish.** Display the Review Readiness Dashboard, then present the saved
Completion summary to the user.
5. **Choose navigation.** Use Next Steps — Review Chaining; wait for its answer.
Navigation grants no implementation authority. A substantive change follows
A substantive change follows
**Recovery routing → Late change or missing work** before navigation resumes.
6. **Finish.** Run Learning hooks, including gated Brain Calibration Write-Back;
then return to the entrypoint's Section self-check and read-only EXIT PLAN MODE GATE in
@@ -1122,21 +1129,18 @@ If any failure mode has no test AND no error handling AND would be silent, flag
### Worktree parallelization strategy
Group implementation into parallel git worktrees (`isolation: "worktree"`) or workspaces.
With one primary module or fewer than 2 independent workstreams, write:
"Sequential implementation, no parallelization opportunity."
Otherwise provide each step/workstream's **Dependency table**:
"Sequential implementation, no parallelization opportunity." Otherwise give a
**Dependency table** by module, not guessed file:
| Step | Modules touched | Depends on |
|------|----------------|------------|
| (step name) | (directories/modules, NOT specific files) | (other steps, or —) |
Use modules, not guessed files. **Parallel lanes:** disjoint modules run together;
shared modules run sequentially, dependencies later. Example:
`Lane A: step1 → step2 (shared models/)` / `Lane B: step3 (independent)`.
**Execution order:** name launch/wait points, e.g. "Launch A + B. Merge both. Then C."
**Conflict flags:** identify cross-lane shared modules; sequence or coordinate them.
**Parallel lanes:** disjoint modules together; shared modules sequentially, dependencies
later (`Lane A: step1 → step2 (shared models/)` / `Lane B: step3 (independent)`).
**Execution order:** launch/wait points ("Launch A + B. Merge both. Then C.").
**Conflict flags:** cross-lane shared modules to sequence or coordinate.
## Implementation Tasks
@@ -1229,7 +1233,7 @@ From final decisions/outputs; publish after report Read-back and Review Log:
- Unresolved decisions: ___ in this review
- Outside voice: recorded provider, completed / unavailable / disabled / skipped (reason)
- Parallelization: ___ lanes, ___ parallel / ___ sequential
- Lake Score: X/Y = 10/10 choices / answered coverage choices. Exclude kind choices; N/A if Y=0.
- Lake Score: X/Y = answers picking a 10/10 option / answers scored for Completeness; N/A if Y=0.
## Plan File Review Report
@@ -1457,8 +1461,6 @@ copy the working plan's task prerequisites, dependencies and execution order
without adding or strengthening them in the question or descriptions. A test
required before editing one function does not make every independent lane wait.
A next-step answer approves no implementation change.
A substantive change follows **Recovery routing → Late change or missing work**
before navigation resumes.
## Learning hooks
@@ -35,7 +35,7 @@ path authorizes no other; implementation edits require explicit authority.
| Artifact | Destination | If writing is forbidden |
|---|---|---|
| Working plan, ledger and complete review report | Selected report file | Ask for a permitted destination if the user can supply one; wait without completion telemetry. If none is permitted, complete the review in chat as **not persisted**, then use **Blocked outcome**. |
| Working plan, ledger and complete review report | Selected report file | Ask for a permitted destination if the user can supply one; wait without completion telemetry. If none is permitted, follow **Read-only review** below, then use **Blocked outcome**. |
| QA Test Plan and task JSONL | Discovery paths below | Present each completely as **not persisted** and continue. |
| TODOS.md | The project's TODO file | Present accepted TODO content as **not persisted** and continue. |
| Required Review Log | The helper's state location | Present its fields as **not persisted**; at Review Log, use **Blocked outcome** instead of publishing a saved review. The final gate cannot pass without this log. |
@@ -45,12 +45,6 @@ QA Test Plan/task JSONL keep discovery paths `~/.gstack/projects/{slug}/`:
`{user}-{branch}-eng-review-test-plan-{datetime}.md` and
`tasks-eng-review-{datetime}.jsonl`. Keep their formats; do not relocate.
A failed permitted save uses **Recovery routing → Repairable write/read failure**,
not the forbidden-write branches above. Do not ask from an unsaved record.
Forbidden auxiliary writes allow the review to continue; unrecovered attempted
writes block it. Best-effort logs retain their stated non-blocking behavior.
Apply this policy at every later write.
**First report save:** Name the fixed target in the report header. Read an
existing destination and preserve its content. For a new file, create permitted
parent directories, then write that header, an unchanged copy of the original
@@ -59,11 +53,15 @@ creation; use a suffix rather than overwrite. Do not add findings or fixes befor
Scope Challenge C. Put records before an existing `## GSTACK REVIEW REPORT`, or
at EOF if absent; create that terminal report only at Plan File Review Report.
**Read-only review:** At each scope/decision record save, present the complete
record, grid and authorized amendments as **not persisted** instead. At both
pre-question and post-answer verification gates, perform the same comparisons
on that presentation instead of a saved Read. This supports chat review, never
the saved-report gate. A failed permitted save is not this route.
**Write routes.** A forbidden write follows its row above.
- **Read-only review:** At each scope/decision record save, present the complete
record, grid and authorized amendments as **not persisted** instead. At both
pre-question and post-answer verification gates, perform the same comparisons
on that presentation instead of a saved Read. This supports chat review, never
the saved-report gate.
- **Failed permitted save** (error or failed Read-back): use **Recovery routing →
Repairable write/read failure**, never the routes above. Do not ask from an
unsaved record; an unrecovered failed write blocks the review.
{{LEARNINGS_SEARCH}}
@@ -157,8 +155,9 @@ Put the problem and file:line in the native fields. Offer 2–3 options, includi
do-nothing when reasonable; Outside Voice retains its four-option menu.
Each option must explain human/CC effort, risk and maintenance. Tie the
recommendation to the engineering preferences; prefer complete coverage when
extra CC effort is marginal. Fit headers and labels to host limits now, before
saving. Without stated limits, keep both under 5 words; details go in descriptions.
extra CC effort is marginal. Make the header and every label final before saving:
within the host's stated length limits, or under 5 words each when none are stated.
Put details in descriptions.
For one fixed approved contract, coverage choices vary implementation or proof
depth. Apply the preamble's Completeness scores or kind-note accordingly.
@@ -171,8 +170,9 @@ each option's value and work, and any approval citation. Include shared, fixed
and pending choices.
Use these three checks for every column:
1. Vary only this choice. Keep other approved values fixed and pending choices
undecided. A value shared by all options still needs approval if it is new.
1. Vary only this choice: every column repeats each other approved value
unchanged and leaves each other pending choice pending. A value shared by all
options still needs approval if it is new.
2. Treat necessary implementation and proof of an approved contract as common
work. Cite its answer instead of creating another approval row. Never cut an
established contract or its required proof.
@@ -383,12 +383,14 @@ Continue to Section 1 only when no answer is pending.
## Review Sections (after scope is agreed)
Evaluate Architecture → Code Quality → Tests → Performance,
at most 8 top issues each. Never condense, abbreviate or skip a section, including
strategy/spec/infra plans. With zero findings, report "No issues found" and continue.
Evaluate Architecture → Code Quality → Tests → Performance. Never condense,
abbreviate or skip a section, including strategy/spec/infra plans.
After each of Sections 1–4, resolve new or reopened choices through Decision
procedure, report findings and dispositions, then continue.
procedure, report findings and dispositions, then continue. Per section, output
at most 8 findings in the Confidence Calibration format (or "No issues found"),
one question per new or reopened choice, then `Dispositions:` (accepted, rejected,
deferred or pending, with D-number or answer) for each finding.
### 1. Architecture review
Evaluate:
@@ -428,6 +430,11 @@ Evaluate:
* Caching opportunities.
* Slow or complex paths.
On per-request or looped paths, check queries in loops, unbounded
queries or caches, missing indexes, whole-file loads and blocking calls without
timeouts. Give each finding's scale (rows, requests/s, bytes) or mark it
unknown; never invent benchmarks.
{{CODEX_PLAN_REVIEW}}
### Continue after Outside Voice
@@ -474,7 +481,7 @@ must pass steps 1–4 again.
4. **Publish.** Display the Review Readiness Dashboard, then present the saved
Completion summary to the user.
5. **Choose navigation.** Use Next Steps — Review Chaining; wait for its answer.
Navigation grants no implementation authority. A substantive change follows
A substantive change follows
**Recovery routing → Late change or missing work** before navigation resumes.
6. **Finish.** Run Learning hooks, including gated Brain Calibration Write-Back;
then return to the entrypoint's Section self-check and read-only EXIT PLAN MODE GATE in
@@ -507,21 +514,18 @@ If any failure mode has no test AND no error handling AND would be silent, flag
### Worktree parallelization strategy
Group implementation into parallel git worktrees (`isolation: "worktree"`) or workspaces.
With one primary module or fewer than 2 independent workstreams, write:
"Sequential implementation, no parallelization opportunity."
Otherwise provide each step/workstream's **Dependency table**:
"Sequential implementation, no parallelization opportunity." Otherwise give a
**Dependency table** by module, not guessed file:
| Step | Modules touched | Depends on |
|------|----------------|------------|
| (step name) | (directories/modules, NOT specific files) | (other steps, or —) |
Use modules, not guessed files. **Parallel lanes:** disjoint modules run together;
shared modules run sequentially, dependencies later. Example:
`Lane A: step1 → step2 (shared models/)` / `Lane B: step3 (independent)`.
**Execution order:** name launch/wait points, e.g. "Launch A + B. Merge both. Then C."
**Conflict flags:** identify cross-lane shared modules; sequence or coordinate them.
**Parallel lanes:** disjoint modules together; shared modules sequentially, dependencies
later (`Lane A: step1 → step2 (shared models/)` / `Lane B: step3 (independent)`).
**Execution order:** launch/wait points ("Launch A + B. Merge both. Then C.").
**Conflict flags:** cross-lane shared modules to sequence or coordinate.
{{TASKS_SECTION_EMIT:eng-review}}
@@ -544,7 +548,7 @@ From final decisions/outputs; publish after report Read-back and Review Log:
- Unresolved decisions: ___ in this review
- Outside voice: recorded provider, completed / unavailable / disabled / skipped (reason)
- Parallelization: ___ lanes, ___ parallel / ___ sequential
- Lake Score: X/Y = 10/10 choices / answered coverage choices. Exclude kind choices; N/A if Y=0.
- Lake Score: X/Y = answers picking a 10/10 option / answers scored for Completeness; N/A if Y=0.
{{PLAN_FILE_REVIEW_REPORT}}
@@ -590,8 +594,6 @@ copy the working plan's task prerequisites, dependencies and execution order
without adding or strengthening them in the question or descriptions. A test
required before editing one function does not make every independent lane wait.
A next-step answer approves no implementation change.
A substantive change follows **Recovery routing → Late change or missing work**
before navigation resumes.
## Learning hooks
+3 -3
View File
@@ -143,14 +143,14 @@ describe('engineering review routing contracts', () => {
test('paused transport and failed persistence have distinct non-success outcomes', () => {
const pause = between(recovery, '**Paused question:**', '**Repairable write/read failure:**');
expect(pause).toContain('without completion telemetry or ExitPlanMode');
expect(compact(pause)).toContain('may have surfaced is still pending; do not duplicate it');
expect(compact(pause)).toContain('that the user may have seen is still pending; do not resend it');
const failure = between(recovery, '**Repairable write/read failure:**', '**Late change or missing work:**');
expect(compact(failure)).toContain('Stop before the dependent question or output');
expect(compact(failure)).toContain('If no recovery is specified or it fails, follow **Blocked outcome**');
expect(compact(failure)).toContain('Never turn a failed permitted save into a chat-only success');
const policy = between(section, '## Review record and write policy', '{{LEARNINGS_SEARCH}}');
expect(compact(policy)).toContain('not the forbidden-write branches above');
expect(compact(policy)).toContain('Best-effort logs retain their stated non-blocking behavior');
expect(compact(policy)).toContain('use **Recovery routing → Repairable write/read failure**, never the routes above');
expect(compact(policy)).toContain('Skip forbidden writes; otherwise keep their best-effort behavior');
expect(recovery).toContain('`OUTCOME=error`');
expect(recovery).not.toContain('`OUTCOME=success`');
});
+3 -3
View File
@@ -62,7 +62,7 @@ test('entry binds a current target and delays bootstrap until scope resolves', (
const entry = template.slice(template.indexOf('## Engineering review'), template.indexOf('## Section self-check'));
expect(entry.split('{{SECTION:review-sections}}')).toHaveLength(2);
expect(entry.indexOf('Before Step 0, require resolved scope')).toBeLessThan(entry.indexOf('{{SECTION:review-sections}}'));
expect(entry.indexOf('**STOP while a Scope Challenge complexity question')).toBeLessThan(entry.indexOf('{{SECTION:review-sections}}'));
expect(entry.indexOf('**Complexity gate:** while a Scope Challenge complexity question')).toBeLessThan(entry.indexOf('{{SECTION:review-sections}}'));
});
test('existing plan selection exceptions and unseeded hard STOP remain explicit', () => {
@@ -71,8 +71,8 @@ test('existing plan selection exceptions and unseeded hard STOP remain explicit'
expect(scope).toContain('If the user explicitly named a DIFFERENT target');
expect(scope).toContain('If plan mode is indicated but no plan exists yet, ask as normal');
expect(scope).toContain('First tool call = AskUserQuestion (tool_use). Send this exact menu and wait');
expect(scope).toContain('if unavailable, disallowed (`--disallowedTools`) or failed, send the menu as plain prose and STOP');
expect(scope).toContain('If a failed call may have surfaced, keep it pending; do not duplicate it');
expect(scope).toContain('If the tool is unavailable, disallowed (`--disallowedTools`) or failed before reaching the user, send the menu as plain prose and STOP');
expect(scope).toContain('If the call returned no result but the user may have seen it, wait; do not resend it');
expect(scope).toContain('A) The current branch diff — the work in progress on this branch.\nB) A plan or design doc I\'ll paste or point you to.\nC) A specific file, directory, or path.');
expect(scope).toContain('Reply with A, B, or C. STOP and wait for the answer.');
});
+18 -15
View File
@@ -88,7 +88,7 @@ describe('CI workflow clarity regressions', () => {
expect(procedure).toContain('For the next choice, use the updated working plan and answer; when finished, continue the calling section');
const entry = compactProse(readFileSync('plan-eng-review/SKILL.md.tmpl', 'utf8'));
expect(entry).toContain('Handle a remedy answer under **Record the answer**; handle a selector answer at its menu');
expect(entry).toContain('A missing-result call that may have surfaced is still pending; do not duplicate it');
expect(entry).toContain('A call with no result that the user may have seen is still pending; do not resend it');
});
test('Eng exposes each existing performance concern without a second approval procedure', () => {
@@ -423,10 +423,12 @@ test('Eng loads its one remedy procedure before Scope Challenge findings and ret
expect(compactProse(scope.slice(complexityRule))).toContain('Save it under the write policy and Read it back against the actual answers');
expect(compactProse(scope.slice(complexityRule))).toContain('Do not invent a pre-answer record afterward');
expect(scope).not.toContain('proceed as-is');
const stop = skeleton.indexOf('**STOP while a Scope Challenge complexity question');
const stop = skeleton.indexOf('**Complexity gate:** while a Scope Challenge complexity question');
const sectionRead = skeleton.indexOf(suffix ? '{{SECTION:review-sections}}' : '> **STOP.** Before starting the Scope Challenge');
expect(stop).toBeGreaterThan(0);
expect(sectionRead).toBeGreaterThan(stop);
expect(skeleton.slice(stop, sectionRead)).toContain('Do not start Section 1, call ExitPlanMode, or write findings or fixes into a plan file');
expect(skeleton.slice(skeleton.indexOf('### Step 0: Scope Challenge'), sectionRead)).not.toContain('**STOP');
expect(skeleton.slice(stop, sectionRead)).toContain('do not start Section 1, call ExitPlanMode, or write findings or fixes into a plan file');
expect(skeleton).toContain('Scope Challenge is mandatory before Section 1');
expect(skeleton.split(suffix ? '{{SECTION:review-sections}}' : '> **STOP.** Before starting the Scope Challenge')).toHaveLength(2);
expect(compactProse(scope)).toContain('apply only accepted scope changes');
@@ -486,7 +488,7 @@ test('Eng loads its one remedy procedure before Scope Challenge findings and ret
expect(compactProse(boundary)).toContain("list each current value and proposed change: behavior, approach, guarantee or bound");
expect(compactProse(boundary)).toContain("optional verification method or depth");
expect(compactProse(boundary)).toContain("Give each bound a measure and unit");
expect(compactProse(boundary)).toContain("Keep other approved values fixed and pending choices undecided");
expect(compactProse(boundary)).toContain("every column repeats each other approved value unchanged and leaves each other pending choice pending");
expect(compactProse(boundary)).toContain("Keep one behavior with its necessary code, tests and documentation");
expect(compactProse(sections)).toContain("For one fixed approved contract, coverage choices vary implementation or proof depth");
expect(compactProse(boundary)).toContain("Never cut an established contract or its required proof");
@@ -584,7 +586,7 @@ describe('Eng approved-work decision gate', () => {
const choice = gate.slice(identify, alternatives);
expect(compactProse(choice)).toContain("Before drafting options");
expect(compactProse(choice)).toContain("If the user can accept one while another stays approved or undecided");
expect(compactProse(gate.slice(alternatives, save))).toContain("Keep other approved values fixed and pending choices undecided");
expect(compactProse(gate.slice(alternatives, save))).toContain("every column repeats each other approved value unchanged and leaves each other pending choice pending");
expect(compactProse(choice)).toContain("Keep one behavior with its necessary code, tests and documentation");
expect(compactProse(choice)).toContain("Optional depths of one verification form one choice");
const options = gate.slice(alternatives, save);
@@ -688,8 +690,8 @@ describe('Eng approved-work decision gate', () => {
expect(compactProse(audit)).toContain("the complete D-numbered preamble brief");
expect(compactProse(audit)).toContain("build `currentDecision`");
expect(compactProse(save)).toContain('Save the record, complete grid and exact `currentDecision`');
expect(compactProse(audit)).toContain("Fit headers and labels to host limits now, before saving");
expect(compactProse(audit)).toContain("Fit headers and labels to host limits now, before saving");
expect(compactProse(audit)).toContain("Make the header and every label final before saving: within the host's stated length limits, or under 5 words each when none are stated");
expect(compactProse(audit)).toContain("Make the header and every label final before saving: within the host's stated length limits, or under 5 words each when none are stated");
expect(compactProse(audit)).toContain("Put all deliberation in the native question/descriptions; a saved-only Pros/cons block cannot supply missing decision context");
expect(compactProse(save)).toContain("Include every native field, the recommendation and all options");
expect(compactProse(save)).toContain("A–D record selectors are ledger notation only");
@@ -705,7 +707,7 @@ describe('Eng approved-work decision gate', () => {
expect(readOnly).toContain('present the complete record, grid and authorized amendments as **not persisted**');
expect(readOnly).toContain('At both pre-question and post-answer verification gates, perform the same comparisons on that presentation instead of a saved Read');
expect(readOnly).toContain('never the saved-report gate');
expect(readOnly).toContain('A failed permitted save is not this route');
expect(readOnly).toContain('**Failed permitted save** (error or failed Read-back): use **Recovery routing → Repairable write/read failure**, never the routes above');
expect(compactProse(save)).toContain('unreadable or unverifiable records use **Recovery routing**');
const recovery = compactProse(readFileSync('plan-eng-review/SKILL.md.tmpl', 'utf8'));
expect(recovery).toContain('Use that step\'s stated recovery, then repeat its full Read-back verification');
@@ -774,7 +776,7 @@ describe('Eng approved-work decision gate', () => {
}
expect(compactProse(gate.slice(compare, save))).toContain("Give every selectable behavior, approach, guarantee or bound a row");
expect(compactProse(gate.slice(compare, save))).toContain("concrete current value, each option's value and work, and any approval citation. Include shared, fixed and pending choices");
expect(compactProse(gate.slice(compare, save))).toContain("Keep other approved values fixed and pending choices undecided");
expect(compactProse(gate.slice(compare, save))).toContain("every column repeats each other approved value unchanged and leaves each other pending choice pending");
expect(gate).not.toContain('`label: changes; preserves; pending`');
const format = gate.split('**Compare one choice.**')[1]!.split('**Pending-record checkpoint.**')[0]!;
expect(format).toContain("Each option must explain human/CC effort, risk and maintenance");
@@ -808,8 +810,8 @@ describe('Eng approved-work decision gate', () => {
expect(bootstrap).toContain('Setup questions approve no engineering remedies');
expect(bootstrap).toContain('Choose listed, enabled MCP AskUserQuestion, otherwise listed native');
expect(bootstrap).toContain('First tool call = AskUserQuestion (tool_use). Send this exact menu and wait');
expect(bootstrap).toContain('If a failed call may have surfaced, keep it pending; do not duplicate it');
expect(bootstrap).toContain('if unavailable, disallowed (`--disallowedTools`) or failed, send the menu as plain prose and STOP');
expect(bootstrap).toContain('If the call returned no result but the user may have seen it, wait; do not resend it');
expect(bootstrap).toContain('If the tool is unavailable, disallowed (`--disallowedTools`) or failed before reaching the user, send the menu as plain prose and STOP');
expect(bootstrap).toContain('Options start at column 0, without blockquotes');
expect(bootstrap).toContain('Never guess a target');
expect(bootstrap).toContain('Startup sequence');
@@ -855,8 +857,8 @@ describe('Eng approved-work decision gate', () => {
expect(compactProse(policy)).toContain("Never substitute an unrelated active plan or silently replace a requested destination");
expect(compactProse(policy)).toContain("Present each completely as **not persisted** and continue");
expect(compactProse(policy)).toContain("Ask for a permitted destination if the user can supply one; wait without completion telemetry");
expect(compactProse(policy)).toContain("If none is permitted, complete the review in chat as **not persisted**, then use **Blocked outcome**");
expect(compactProse(policy)).toContain('A failed permitted save uses **Recovery routing → Repairable write/read failure**, not the forbidden-write branches above');
expect(compactProse(policy)).toContain("If none is permitted, follow **Read-only review** below, then use **Blocked outcome**");
expect(compactProse(policy)).toContain('**Failed permitted save** (error or failed Read-back): use **Recovery routing → Repairable write/read failure**, never the routes above');
const routes = Object.fromEntries(policy.split('\n').filter(line => line.startsWith('| '))
.map(line => line.split('|').slice(1, -1).map(cell => cell.trim())).map(cells => [cells[0], cells[2]]));
expect(routes["Working plan, ledger and complete review report"]).toContain('wait without completion telemetry');
@@ -866,7 +868,8 @@ describe('Eng approved-work decision gate', () => {
expect(routes[auxiliary]).toContain('continue');
}
expect(routes['Required Review Log']).toContain('The final gate cannot pass without this log');
expect(compactProse(policy)).toContain("Forbidden auxiliary writes allow the review to continue; unrecovered attempted writes block it");
expect(compactProse(policy)).toContain("A forbidden write follows its row above");
expect(compactProse(policy)).toContain("an unrecovered failed write blocks the review");
expect(compactProse(gate)).toContain('Use Review record and write policy for every save below');
const log = template.split('## Review Log')[1]!.split('{{REVIEW_DASHBOARD}}')[0]!;
expect(routes['Required Review Log']).toContain('Present its fields as **not persisted**');
@@ -978,7 +981,7 @@ describe('Eng approved-work decision gate', () => {
expect(normalized).toContain("Separate instrumentation, follow-ups, guarantees and policies need their own choices");
expect(normalized).toContain("Reopen an approved choice only for a concrete new risk, contradictory evidence or a changed assumption");
expect(normalized).toContain("Record remaining unknowns and uncertain risks");
expect(normalized).toContain("Keep other approved values fixed and pending choices undecided");
expect(normalized).toContain("every column repeats each other approved value unchanged and leaves each other pending choice pending");
expect(normalized).toContain("their tests wait for approval");
expect(normalized).toContain("Keep unresolved risks and verification visible");
expect(normalized).toContain("Drafts, recommendations and reviewer agreement grant neither");