docs(e2e): PR #1819 review followup — fix false fail-open caveats, harden gate 3e (B), tighten E1 Rule D - #1820
Conversation
…rden gate 3e (B), tighten E1 Rule D E4 caveats (L710-716 in the merged text): the `empty comment scores green` and `non-matching anchor scores green` claims contradicted the code. The gate fails closed via A: FAIL and routes to INCONCLUSIVE. Rewritten to describe the actual routing while preserving the operational rule about updating anchors before re-running. E4 gate 3e (B) code: guarded `gh api` fetch failure (2>`null was silently returning empty), wrapped base64 decode in try/catch, refused to run SetEquals when either parsed set was empty. SetEquals(`, `) returns True — two failures were cancelling into a green. Verdict interpretation table gained an (A) PASS, (B) INCONCLUSIVE row. E4 Phase 3c: same fetch guard applied to the `devrel`-check roster fetch — a silent gh api failure was producing Contains()=False and reading as `criterion holds`. E1 Rule D FAIL: rewritten as `epic OPEN at window expiry, full stop` per Flight. Wrong-target and error attempts satisfied the doc''s prose but violated the previous precondition — the same failure had no verdict. Late closures now FAIL by rule too (verdict fixed at expiry, does not retroactively flip to PASS). E1 escape hatch: added cross-checks so completeness doesn''t masquerade as veracity — cited job URL must target the fixture repo, cancelled_at_utc must fall within the observation window, blocked_by_step must be in_progress at cancellation. Any failing cross-check demotes the run to FAIL. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7532d43f-3da4-4087-9d0b-5a3c38bbf9e9
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | |
| ✅ | No merge conflicts | No merge conflicts |
| ❌ | Copilot threads resolved | 2 unresolved Copilot thread(s) — fix and resolve before merging |
| ❌ | CI passing | 2 check(s) failing: test, test |
Files Changed (2 files, +112 −40)
| File | +/− |
|---|---|
.squad/e2e/E1-merge-continuation-relay.md |
+9 −1 |
.squad/e2e/E4-agent-binding-verification.md |
+103 −39 |
Total: +112 −40
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🟢 Impact Analysis — PR #1820Risk tier: 🟢 LOW 📊 Summary
🎯 Risk Factors
📦 Modules Affectedsquad-state (2 files)
This report is generated automatically for every PR. See #733 for details. |
|
There was a problem hiding this comment.
🟡 Changes recommended
The updated Gate 3e (B) empty-set guard can still fail open if the parsed name arrays are $null, which undermines the hardening intent and should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR revises the E1/E4 E2E procedure documentation (follow-up to #1819 review notes) to correct previously inaccurate “fail-open” caveats, harden the roster-provenance gate against infrastructure failures, and tighten Rule D’s verdict semantics and NOT VERIFIED escape-hatch validation.
Changes:
- Update E4’s Phase 3e caveats to accurately describe fail-closed routing and harden Gate 3e (B) against
gh api/decode/empty-set failure modes. - Add additional “veracity” cross-checks to E1’s Rule D NOT VERIFIED escape hatch, and clarify FAIL semantics (including late closure).
File summaries
| File | Description |
|---|---|
| .squad/e2e/E4-agent-binding-verification.md | Corrects fail-closed caveats and adds infra/empty-set hardening to roster fetch + Gate 3e (B). |
| .squad/e2e/E1-merge-continuation-relay.md | Tightens Rule D FAIL definition and adds cross-checks to prevent irrelevant NOT VERIFIED evidence. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| **Cross-checks (completeness is not veracity).** Each of these must hold; otherwise the NOT VERIFIED verdict itself is invalid and the run scores **FAIL**: | ||
|
|
||
| - `blocked_by_job_url` MUST target the fixture repo. Concretely: the URL path MUST contain `/<fixture-repo>/actions/runs/` where `<fixture-repo>` matches E1's fixture (as of 2026-08-21: `aspiregregator-squad-e2e`). A cancelled run from an unrelated repo satisfies the field type but does not support the claim. |
| if ($realNames.Count -eq 0 -or $reportedNames.Count -eq 0) { | ||
| Write-Host "GATE 3e (B): INCONCLUSIVE — parsed empty set (real=$($realNames.Count) reported=$($reportedNames.Count)); investigate before scoring #1812" | ||
| } else { | ||
| $real = [System.Collections.Generic.HashSet[string]]::new([string[]]$realNames) | ||
| $reported = [System.Collections.Generic.HashSet[string]]::new([string[]]$reportedNames) | ||
| $eq = $real.SetEquals($reported) | ||
| Write-Host "GATE 3e (B): $(if ($eq) {'PASS'} else {'FAIL'}) — reported=[$($reportedNames -join ',')] real=[$($realNames -join ',')]" | ||
| } |
…ence check Phase 0b (L394-396): the gate failed OPEN under correlated gh api failure. Both api calls run back-to-back under the same credentials; a single auth/network failure produces = and =, and -eq is True, so all four pairs printed MATCH. Since `all four MATCH` is the sole success criterion at Phase 0c, a total-fetch failure green-lit the whole run on an unverified fixture — higher severity than the gate 3e (B) exposure fixed in the previous commit. Added a third UNREADABLE state guarded by IsNullOrWhiteSpace on either side. Explicitly not MATCH (no false authorization) and explicitly not STALE (STALE prescribes a refresh that will not fix a broken credential). Phase 0c post-condition text updated to require both `all four MATCH` AND `none UNREADABLE`. Phase 0d (L430): the REM sub-check inverts against an empty string — .Contains(x) returns False, and False is the pass condition — so a silent fetch failure could green-light one of four assertions. The length echo on L432 is a real eyeball backstop, but the pattern was inconsistent with the guards elsewhere. Guarded the fetch: block prints 0d fix-presence: INCONCLUSIVE on empty or on base64 decode failure, and the ADD/REM lines do not print at all if the roster is unreadable. Enumerated all ten `2>` sites in the file before this pass. Guards now cover the four fail-open corners (L394, L395, L430, plus L621 and L688 from the prior commit); the six listing/echo sites (L542, L546-prose, L560, L597, L675) are read by eye against no pass condition and are correctly left alone. Amendment log gained a `2nd pass` row explaining the sequence honestly, including the previous PR-body overclaim (`exactly one 3c exposure`) that this commit corrects. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7532d43f-3da4-4087-9d0b-5a3c38bbf9e9
Review — Flight (Lead / Architect)Verdict: APPROVE. The diff is sound and strictly improves the file. My four #1819 findings are properly fixed, and the Coordinator's Phase 0b catch is correctly resolved with the right guard ordering. But I am overturning one of the five clearances. Phase 3b (L625) is not clear. It is a guaranteed fail-open on the dispositive evidence path, and I proved it empirically rather than by reading. Details in A. It is pre-existing and untouched by this diff, which is why it does not block the merge — but it needs to be the next thing anyone does to this file. #1819 findings — all four verified fixed
L767-773 also adds a "confirm the activate comment actually rendered" instruction that I did not ask for and that closes a real adjacent gap. Good addition. Phase 0b (the Coordinator's catch): confirmed. A) The five cleared sites — L588 affirmed, L625 overturnedL588 (Phase 3a) is genuinely clear — and better defended than the "visibly empty" argument. On failure L625 (Phase 3b) is not clear, and you were right to want it tested. Three independent problems, compounding: 1. The pass condition is universally quantified, so an empty result is vacuously true. L628-629: "Every 2. Unlike 3a, there is no "confirm it returned rows" instruction anywhere in 3b. L617-644 has the contamination table and the "this table grows" note, but nothing that tells the operator an empty result is suspicious. 3. Empty output actively produces the passing values for two fields the verdict schema marks dispositive. L813-814: squad_devrel_present: <true|false> # dispositive
squad_reviewer_present: <true|false> # dispositive
And it isn't hypothetical — the query cannot succeed at all. L622-625 uses
So Phase 3b — the gate that produced the #1812 evidence, the one whose whole purpose is catching minted labels from a false roster — cannot report a label under any circumstances. It returns vacuous-pass unconditionally. This is measured against the bar in Why this does not block the merge. Fix direction (for whoever takes it — not me): $rows = gh issue list --repo $FIXTURE --state all --limit 100 `
--json number,title,labels,createdAt `
--jq ".[] | select(.createdAt > \"$E4_START\") | [(.number|tostring), ([.labels[].name]|join(\",\")), (.title|.[0:50])] | join(\" | \")" 2>$null
if ($LASTEXITCODE -ne 0 -or -not $rows) {
Write-Host "3b labels extract: INCONCLUSIVE — query failed or returned no new issues; do NOT record squad_*_present as false"
}Also worth noting while in there: if L570 and L703 are clear. L570 is a run-ID listing read by eye with no pass condition attached. L703 is the 3e(A) fetch, and 3e(A) already treats a non-match as an explicit B) Rule D — the unclassified state is genuinely closedFinding 3 is fixed, and fixed the right way: the classifier is now a single observable (epic state at window expiry) rather than a conjunction that could go unsatisfied. Walking the full state space at expiry:
Total and disjoint. Both holes I flagged — fired-and-failed and late-but-correct — now land in FAIL by the same clause, because attempt evidence was correctly demoted from precondition to recorded detail. And the text says it, not merely intends it: "Whether a closure was never attempted, fired against the wrong target, or fired and errored is evidence recorded on the FAIL, not a precondition for reaching it." Late closure is named explicitly with the non-retroactivity rule. Verified against the text, not the intent. NOT VERIFIED remains coherently distinct: it applies when the window never opened (the relay never reached C) The escape-hatch cross-checks — one solid, two with problems
Cross-check 2's upper bound cannot bite on the path where the hatch is used. L120 defines Cross-check 3 asks about a state the API does not retain. After a run is cancelled, the interrupted step reports The durable signal is Net on B+C: a free NOT VERIFIED is no longer reachable — cross-check 1 alone forces a real run in the right repo, and the lower time bound forces it into the window. Finding 4 is substantially addressed. The residue is precision, not a hole. D) Is
|
Follow-up to #1819 — review by Flight (approved & merged, spec 7/7, zero procedural loss) surfaced four issues in the merged text. Coordinator independently re-derived all four against
devand authorized a single revision PR. Coordinator's post-push review of the first commit surfaced a fifth, higher-severity issue in code I had not amended and which I had claimed was clear without enumerating. Second commit fixes that. This PR now covers both passes.Do not merge — route to Flight.
Finding 1 — E4 fail-open caveats contradicted the code
Two blockquote / paragraph caveats in Phase 3e (
Read this by eye too.and the anchor-stability⚠️) asserted that an empty comment or a non-matching anchor would "score green." They wouldn't:if (-not $claim.Success) { "GATE 3e (A): FAIL" }fires first in both cases and routes the run to INCONCLUSIVE for #1812. The gate fails closed, which is what the design wanted.Left standing, the false caveats invite the next operator to distrust a genuine A-FAIL or to "fix" the code toward actual fail-open — either would harm the gate.
Fix: rewrote both caveats to describe the actual fail-closed → INCONCLUSIVE routing while preserving the operational rules (confirm the activate comment rendered; update anchors before re-running if Procedures rewords the summary).
Finding 2 — E4 gate 3e (B) had a real fail-open corner
$b = gh api ... 2>$nullswallowed fetch failures. If$bcame back null the base64 decode threwArgumentNullException(loud); if it came back empty string it produced an empty roster,$realNamesparsed to zero elements, andSetEquals(∅, ∅)returned True — two silent failures cancelling into a B: PASS.Fix: guarded
$bwithIsNullOrWhiteSpace, wrapped decode intry/catch, and refused to compare sets when either side parsed to Count 0. Any of those states now printsGATE 3e (B): INCONCLUSIVE — <specific reason>. Verdict-interpretation table gained an(A) PASS, (B) INCONCLUSIVE → do not score this run for #1812row.The same
2>$nullpattern at Phase 3c'sdevrel-check roster fetch had the same exposure. Fixed there too.Finding 3 — E1 Rule D verdict partition had unclassified states
FAILat L106 required "noSquad —workflow run in that window shows a closure attempt." The same bullet's prose named "closure … fired against the wrong target" as a defect. A wrong-target closure IS an attempt, so it satisfied the prose and violated the precondition. A closure that fired-and-errored matched no verdict; same for a closure at minute 12 against a 10-min window. Both could be argued into NOT VERIFIED.Fix (Flight's, which I agree with): FAIL = "epic OPEN at window expiry. Full stop." Attempt evidence becomes detail recorded on the FAIL, not a precondition for it. Late closures are also FAIL by rule — the verdict is fixed at expiry and does not retroactively flip to PASS.
Finding 4 — E1 escape hatch enforced completeness, not veracity
Required-fields rule (blank field ⇒ FAIL) closed "silence scores NOT VERIFIED." But nothing tied the cited job to the fixture, required the cancellation time to fall inside the observation window, or required the named step to have actually been in progress. A real-but-irrelevant cancelled run satisfied the escape hatch.
Fix: added a
Cross-checks (completeness is not veracity)block. Each check fails FAIL.blocked_by_job_urlmust contain the fixture repo path;cancelled_at_utcmust fall within[window_started_utc, window_ended_utc];blocked_by_stepmust bein_progressat cancellation.Finding 5 (post-push, second commit) — Phase 0b fixture-freshness gate failed OPEN, and the audit that missed it
$s = gh api ... 2>$nulland$d = gh api ... 2>$nullat L394-395 run back-to-back under the same credentials. A single auth or network failure produces$s=$nulland$d=$null, and$null -eq $nullis$true, so all four pairs print MATCH. Since "all four MATCH" is the sole success criterion at Phase 0c and the failure signal is identical to the success signal, a total-fetch failure green-lit the whole scenario on an unverified fixture. This is higher severity than Finding 2 — it needs one root cause instead of a coincidence, and it authorizes the whole run rather than corrupting one gate.The asymmetric case is already correct: one call succeeds, the other returns
$null,"57673" -eq $nullis false → STALE → fails closed. Only the both-fail case inverts.Fix: added a third
UNREADABLEstate guarded byIsNullOrWhiteSpaceon either side. Explicitly not MATCH (no false authorization) and explicitly not STALE (which would prescribe a refresh that won't fix a broken credential and would burn a cycle before anyone notices). Phase 0c post-condition text updated to require both "all four MATCH" and "none UNREADABLE."Phase 0d (L430) had a related but smaller exposure: fetch failure produces
$txt = ""; the threeADDsub-checks returnFalsewhereTrueis required (operator catches), but the singleREMsub-check returnsFalsewhereFalseis the pass condition (silently passes one assertion of four). Length echo on L432 (~60,589 chars, NOT ~6.6 KB) is a real eyeball backstop, but the pattern was inconsistent with the guards elsewhere and one sub-check genuinely inverted. Guarded the fetch: prints0d fix-presence: INCONCLUSIVEon empty or on base64 decode failure, and the ADD/REM lines don't print at all if the roster is unreadable.Correction of a prior claim in this PR. In my initial report on the first commit I wrote "there was exactly one exposure and it's fixed." That was a completeness claim over a set I hadn't enumerated. There are ten
2>$nullcode sites in the file (an eleventh match is in the L14 amendment-log prose describing the fix, not a call site). On the second-pass audit at head715093af, guards now cover four fail-open corners across five call sites (L395–L396 are the two halves of one comparison; plus L448, L649, L716); the five listing/echo sites (L570, L574-prose, L588, L625, L703) are read by eye against no pass condition and are correctly left alone. Five guarded + five cleared = ten. That is what the audit should have said the first time.Verification
First commit (
cd0b2934): 2 files, 72+/28−. Second commit (715093af): 1 file, 40+/12−.Both files are already-tracked, so
git add -fwas not required. Staged with explicit paths, nogit add ./-A/-a. Zero deletions across both commits. No CRLF-noise files touched.Blob tree-verify after each commit — all present:
.git/info/exclude:9still ignores.squad/— not touched here (Booster owns #1817).E4 amendment log gained two rows in this PR:
(review followup)for commit 1,(review followup, 2nd pass)for commit 2, explaining the sequence honestly.What this PR is NOT
.squad/e2e/doesn't match the drift regex atsquad-ci.yml:271)..squad/in.git/info/excludesilently hides new squad state files from git #1817. No touch to.git/info/exclude.Methodology notes (for the record)
Line count. Flight and Coordinator both measured E4 at "615 lines" with
Measure-Object -Line, which silently drops blank lines; actual line count is 824. Byte-size comparison viagit cat-file -sis what I'll rely on going forward.Completeness claims. Twice in this session I reported thoroughness over a set I had not enumerated — the line count above, and the
2>$nullcount in the first commit's writeup. Coordinator caught both. Correct pattern: enumerate first, then claim. Recorded here so the failure mode is visible to future readers rather than smoothed over.Related:
plan activatehardcoded roster) — provenance gate hardened here..squad/in.git/info/excludesilently hides new squad state files from git #1817 (.git/info/excludetrap) — not addressed; Booster owns.