From a4797346aba44a8f5d6ebdd2ff68ed3604a0e6c6 Mon Sep 17 00:00:00 2001 From: brady gaster Date: Mon, 24 Aug 2026 11:20:38 -0700 Subject: [PATCH] fix(gh-aw): stop the validation schema from shipping its own verdict The planning ontology's Validation Result schema listed five named checks, each pre-filled with a literal check mark. The model was never asked to determine a verdict -- it was handed a table whose verdict was already PASS and asked to reproduce it. It did, on a run where every agent binding was invalid: | Agent assignments valid | (pass) (lead, lead, devrel) | The check passed on precisely the input it exists to reject, and it has reported green across a population of runs in which correct binding had a zero success rate. This is #1784's mechanism aimed at the verdict rather than the value: a concrete literal in a prompt gets copied verbatim, and here the salient literal was the pass mark. The ontology's own convention already said how to avoid it -- angle brackets mean "you fill this in" -- which the schema's final row obeyed while the five above it did not. Three surfaces disagreed on what the checks even were. The implementation plan named a `Validation Pre-check` section with zero rows specified, so the model reached for the pre-cleared template; and squad-plan-validate Step 2 numbers ten checks whose names never matched the five. The ambiguity resolved toward the pre-filled template, which is the outcome you would predict. Changes: - Ontology 3.6 status cells become placeholders, and the schema now defers to Step 2 as the sole check vocabulary rather than restating names. Omission is called out as not-a-pass. - Drops the self-assessed Validation Pre-check from the implementation plan structure. A pass claimed by the skill that authored the plan is not evidence. - Removes a duplicated threshold that had silently drifted: the schema said sizing fails above XL while Step 2 Check 5 fails above L. Five assertions in test/gh-aw-quality.test.ts enforce all of it. Verified by mutation: against the pre-fix files four of the five go red, including on the exact row recorded from the E3 run. They target the validation schema rather than every status literal, because the Lifecycle Summary legitimately ships a mixed worked snapshot with a legend -- a precise gate that catches the real defect beats a general one that misfires. `gh aw compile --strict` succeeds; no test asserts merely that prompt text is present. Addresses acceptance criteria 1-3 of #1801. The deterministic non-model checker (criteria 4-6) is not included, so this does not close the issue. Refs #1801 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20afe6d2-444e-414e-8a39-e67ab67ca6df --- test/gh-aw-quality.test.ts | 107 ++++++++++++++++++++ workflows/shared/squad-planning-ontology.md | 23 +++-- workflows/squad.md | 8 +- 3 files changed, 130 insertions(+), 8 deletions(-) diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index a435efffa..0b23c2fca 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -573,6 +573,113 @@ describe('gh-aw: planning state machine', () => { }); }); +// --------------------------------------------------------------------------- +// Test: Validation schema must not ship its own verdict (#1801) +// --------------------------------------------------------------------------- + +/** + * The planning ontology's Validation Result schema (§3.6) once listed five named + * checks, each pre-filled with a literal `✅`. The model was therefore not asked + * to determine a verdict — it was handed a table whose verdict was already PASS + * and asked to reproduce it. It did, including on a run where every agent binding + * was invalid: `| Agent assignments valid | ✅ (lead, lead, devrel) |`. The check + * passed on precisely the input it exists to reject. + * + * That is #1784's mechanism aimed at the verdict instead of the value — a concrete + * literal in the prompt gets copied verbatim, and here the salient literal was the + * pass mark. The ontology's own convention already said how to avoid it: angle + * brackets mean "you fill this in", which §3.6's final row obeyed while the five + * above it did not. + * + * Scope note: this deliberately targets the *validation* schema rather than every + * status literal in the file. §5's Lifecycle Summary legitimately ships a worked + * snapshot with mixed `✅ Done` / `⬚ Pending` rows plus an icon legend — that is + * bookkeeping recorded from which command ran, not a judgment determined from + * evidence, and a blanket rule would fail it for no benefit. A precise gate that + * provably catches the real defect beats a general one that misfires. + */ +describe('gh-aw: validation schema ships no pre-filled verdict (#1801)', () => { + const ontology = readText(join(SHARED_DIR, 'squad-planning-ontology.md')); + + /** The fenced `## Plan Validation` template from ontology §3.6. */ + function validationSchema(): string { + const match = ontology.match(/```markdown\n(## Plan Validation[\s\S]*?)```/); + if (!match) { + throw new Error( + 'Could not locate the fenced `## Plan Validation` schema in ' + + 'squad-planning-ontology.md. If §3.6 was renamed, update this test — ' + + 'do not delete it.', + ); + } + return match[1]; + } + + /** Rows of a markdown table, minus header and separator. */ + function dataRows(block: string): string[][] { + return block + .split('\n') + .map((l) => l.trim()) + .filter((l) => l.startsWith('|') && !/^\|[\s\-:|]+\|$/.test(l)) + .map((l) => + l + .replace(/^\||\|$/g, '') + .split('|') + .map((c) => c.trim()), + ) + .filter((cells) => cells[0] !== 'Check'); + } + + it('locates the validation schema', () => { + // Guards the assertions below against silently evaluating an empty set — + // the same vacuous-pass failure this suite exists to prevent. + expect(dataRows(validationSchema()).length).toBeGreaterThan(0); + }); + + it('supplies no literal verdict in a cell the model must determine', () => { + const offenders = dataRows(validationSchema()) + .filter((cells) => cells.slice(1).some((c) => /^(✅|❌)/.test(c))) + .map((cells) => `| ${cells.join(' | ')} |`); + + expect( + offenders, + 'A named check paired with a literal verdict hands the model its answer, ' + + 'and it will be copied verbatim (#1784). Use syntax for ' + + `every cell the model must determine:\n${offenders.join('\n')}`, + ).toEqual([]); + }); + + it('defers to one check vocabulary instead of naming its own', () => { + // Three surfaces once disagreed on what the checks were: this schema listed + // five names, `squad-plan-validate` Step 2 numbered ten different ones, and + // the implementation plan specified none at all. The ambiguity resolved + // toward the pre-filled template. One vocabulary, named in one place. + expect( + validationSchema(), + 'The Validation Result schema must point at squad-plan-validate Step 2 ' + + 'as the sole check vocabulary rather than restating check names.', + ).toMatch(/Step 2/); + }); + + it('does not restate a pass threshold that Step 2 owns', () => { + // The removed rows carried `Sizing within bounds (no >XL)` while Step 2 + // Check 5 fails a task `> L`. Two surfaces, two thresholds, one silent + // contradiction — duplication is how they drifted apart. + expect(validationSchema()).not.toMatch(/>\s*XL/); + }); + + it('keeps the self-assessed pre-check out of the implementation plan', () => { + // Surface 1: the implementation plan named a `Validation Pre-check` section + // with zero rows specified, so the model reached for the pre-cleared + // template in §3.6. A pass claimed by the skill that authored the plan is + // not evidence. + expect( + readText(SQUAD_WORKFLOW), + 'squad.md must not reintroduce a self-assessed Validation Pre-check; ' + + 'validation is /squad plan validate\'s artifact.', + ).not.toMatch(/→\s*Validation Pre-check/); + }); +}); + // --------------------------------------------------------------------------- // Test: Frontmatter Schema // --------------------------------------------------------------------------- diff --git a/workflows/shared/squad-planning-ontology.md b/workflows/shared/squad-planning-ontology.md index be64d509b..1b10edb15 100644 --- a/workflows/shared/squad-planning-ontology.md +++ b/workflows/shared/squad-planning-ontology.md @@ -247,22 +247,31 @@ The issue body IS the intent. No special format required, but structured intents ```markdown ## Plan Validation -### Result: ✅ PASS | ❌ FAIL +### Result: <✅ PASS | ❌ FAIL> ### Checks + +One row per check in `squad-plan-validate` Step 2 — that numbered table is the +sole check vocabulary. Do not invent check names, do not restate their pass +thresholds here, and do not omit a row: a check absent from this table has not +been run, and an omitted row is not a pass. + | Check | Status | Detail | |-------|--------|--------| -| All tasks map to an epic | ✅ | — | -| No circular dependencies | ✅ | — | -| Sizing within bounds (no >XL) | ✅ | — | -| Agent assignments valid | ✅ | — | -| Scope boundary respected | ✅ | — | -| | ❌ | | +| | <✅ or ❌> | | ### Diagnostics (if FAIL) - ``` +> **Status cells are determined, never shipped.** An earlier revision of this +> schema listed five check names each pre-filled `✅`, so the model was handed a +> table in which the verdict was already `PASS` and asked to reproduce it. It did +> — including on a run whose agent bindings were entirely invalid (#1801). A +> concrete literal in a prompt gets copied verbatim (#1784); when that literal is +> a verdict, the check clears itself. Every cell the model must determine uses +> `` syntax, and `test/gh-aw-quality.test.ts` enforces it. + ### 3.7 Acceptance Records **Scope Acceptance:** diff --git a/workflows/squad.md b/workflows/squad.md index dd9596962..75693e1b7 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1120,10 +1120,16 @@ Check: sizes ≤ L, no cycles, traceability, coverage, agent validity (every `Ag `add-comment` with `data: {"squad_artifact":"implementation","schema_version":"1","origin_issue":{issue_number},"phases":[]}`. -Structure: `## 🔧 Squad Implementation Plan` → Program ref → Phase tables (Title|Size|Depends On|Agent|Epic) → Details per task (Scope, Acceptance criteria, Dependencies, Rollout, Traces to) → Dependency Graph → Sizing Summary table → Validation Pre-check → Next: `/squad plan validate`. +Structure: `## 🔧 Squad Implementation Plan` → Program ref → Phase tables (Title|Size|Depends On|Agent|Epic) → Details per task (Scope, Acceptance criteria, Dependencies, Rollout, Traces to) → Dependency Graph → Sizing Summary table → Next: `/squad plan validate`. Re-check every `Agent` against the Step 2 binding rule before posting. +Do **not** emit a self-assessed validation section here. Validation is +`/squad plan validate`'s artifact and uses its check vocabulary; a pass claimed +by the skill that authored the plan is not evidence, and an earlier unspecified +`Validation Pre-check` section is what let a plan certify its own invalid agent +bindings (#1801). + ##### Step 5: Update Lifecycle Set Implementation Plan = `✅ Done`, state = Implementation planned, next = `/squad plan validate`.