fix(gh-aw): stop the validation schema from shipping its own verdict - #1853
Conversation
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
🟡 Impact Analysis — PR #1853Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (2 files)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ✅ | Single commit | 1 commit — clean history |
| ✅ | Not in draft | Ready for review |
| ❌ | Branch up to date | dev is 1 commit(s) ahead — rebase recommended |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ❌ | Copilot threads resolved | 1 unresolved Copilot thread(s) — fix and resolve before merging |
| ✅ | CI passing | All checks passing |
Files Changed (3 files, +130 −8)
| File | +/− |
|---|---|
test/gh-aw-quality.test.ts |
+107 −0 |
workflows/shared/squad-planning-ontology.md |
+16 −7 |
workflows/squad.md |
+7 −1 |
Total: +130 −8
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
There was a problem hiding this comment.
🟡 Changes recommended
The regression tests do not cover the changed overall Result heading, allowing a prefilled verdict to return unnoticed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes prefilled validation verdicts and self-assessed plan validation from gh-aw schemas.
Changes:
- Replaces fixed validation statuses with placeholders.
- Centralizes checks in
squad-plan-validate. - Adds regression tests for schema drift.
File summaries
| File | Summary |
|---|---|
workflows/squad.md |
Removes self-assessed implementation-plan validation. |
workflows/shared/squad-planning-ontology.md |
Makes validation results model-determined. |
test/gh-aw-quality.test.ts |
Adds validation-schema regression coverage. |
Review details
Suppressed comments (3)
test/gh-aw-quality.test.ts:660
- This is only a substring check for
Step 2; it passes if the schema contains that reference and also reintroduces| Agent assignments valid | <status> | ... |(or any other named row). The claimed sole-vocabulary invariant is therefore not mutation-protected. Assert that the extracted table contains only the generic placeholder row, or compare its row names against the Step 2 table.
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/);
test/gh-aw-quality.test.ts:640
- This regression only rejects cells whose text starts with
✅/❌, so a literalPASS/FAIL(or any other non-placeholder status) would satisfy the test and reintroduce the same self-clearing verdict. Assert that the Status cell uses the required angle-bracket placeholder syntax so the gate enforces the stated invariant rather than only the old emoji spelling.
.filter((cells) => cells.slice(1).some((c) => /^(✅|❌)/.test(c)))
workflows/shared/squad-planning-ontology.md:257
- This makes the ontology's validation schema authoritative, but the repository's regression fixture and walkthrough still encode the old contract:
test-fixtures/planning/aspiregregator/validation-output.mdcontainsSizing within bounds (no >XL)and no roster check, whiledocs/demo-agentic-sdlc-walkthrough.mdstill shows nine checks. The fixture README explicitly says to compare outputs against this ontology, so update or clearly mark those examples as legacy; otherwise they continue teaching and preserving the drift this change is intended to remove.
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.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 <placeholder> syntax for ' + | ||
| `every cell the model must determine:\n${offenders.join('\n')}`, | ||
| ).toEqual([]); | ||
| }); |
Refs #1801 — addresses acceptance criteria 1–3. Does not close the issue; the deterministic non-model checker (criteria 4–6) is not included.
The defect
Ontology §3.6 listed five named checks, each pre-filled with a literal
✅:So the model was never asked to determine a verdict. It was handed a table where the verdict was already
PASSand asked to reproduce it — and it did, on a run where every agent binding was invalid:The check passed on precisely the input it exists to reject, and 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 instead of 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 §3.6's final row obeyed while the five above it did not.
What changed
Ontology §3.6 — status cells become
<placeholder>, and the schema defers tosquad-plan-validateStep 2 as the sole check vocabulary rather than restating names. Omission is explicitly called out as not-a-pass.squad.md— drops the self-assessedValidation Pre-checkfrom the implementation plan structure. Surface 1 named that section while specifying zero rows, which is why the model reached for the pre-cleared template. A pass claimed by the skill that authored the plan is not evidence.A contradiction the issue didn't catch. The removed rows carried
Sizing within bounds (no >XL)while Step 2 Check 5 fails a task> L. Two surfaces, two thresholds, silently disagreeing — an XL task failed one and passed the other. Duplication is how they drifted, so the threshold now lives only in Step 2, and a test keeps it from coming back.Verification — mutation, not just green
Five assertions in
test/gh-aw-quality.test.ts. Restoring the pre-fix files turns four of five red, including on the exact row recorded from the E3 run:The fifth ("locates the validation schema") correctly stays green — it exists so the other four cannot pass vacuously over an empty match, which is the failure mode #1822 documents.
test/gh-aw-quality.test.ts: 96 passed, 13 skipped, 0 failedgh aw compile --strict: exit 0, 2 workflows succeeded,squad.lock.yml150,387 BScope note on the rule. It targets the validation schema, not every status literal in the file. §5's Lifecycle Summary legitimately ships a worked snapshot with mixed
✅ Done/⬚ Pendingrows plus an icon legend — bookkeeping recorded from which command ran, not a judgment determined from evidence. A blanket rule would fail it for no benefit. A precise gate that provably catches the real defect beats a general one that misfires.What is deliberately not here
Criteria 4–6 — the deterministic checker comparing the emitted
Agentcolumn against.squad/team.md, failing closed on unparseable input. That is Option B in the issue, it needs a new workflow job reading an emitted artifact, and it is the criterion that retires #1801.This PR removes the proximate cause (the pre-cleared template) and reconciles the three disagreeing surfaces. It does not replace the checker, and it should not be recorded as doing so — the issue is explicit that a prompt-side fix is a complement to deterministic enforcement, never a substitute.
Related
Same class as #1766, #1778, #1784. The E3 artifact that exposed it is cited in #1784.
No changeset — no
packages/*/src/changes.