Repository navigation
docs(e2e): fix Phase 3b broken --arg query (P0) and sweep $LASTEXITCODE house rule across E4 + E1 - #1821
Conversation
P0 — Phase 3b had never been able to run. gh issue list --jq --arg t ... at (old) L624 relied on a --arg flag gh does not have; --jq consumed the flag as its expression and turned the rest into stray positionals. gh exited 1 with empty stdout every single run, and 2>\ swallowed the stderr. The pass condition below the query — 'every squad:{x} must be a lowercased roster Name' — is universally quantified and vacuously true over the empty set, so a broken query would have written the passing values into the two `# dispositive` schema fields `squad_devrel_present` / `squad_reviewer_present`, where `true` is the FAIL evidence. This is a third instance of the 'vacuously true over empty set' hazard already fixed at Phase 0b (`\ -eq \`) and Phase 3e (B) (`SetEquals(0,0)`).
Rewrote 3b to interpolate `\` in PowerShell (no `--arg`), added an `\`-empty precheck (with an empty cutoff, `select(.createdAt > """)` matches every issue and drags all 8 pre-existing contaminated issues into the FAIL evidence), added `\0` and zero-row guards printing `3b labels extract: INCONCLUSIVE`, added the 'confirm rows returned' operator instruction Phase 3a already carries, and stated explicitly that INCONCLUSIVE must not be recorded as `false` on the two dispositive fields.
P1 — house rule sweep. Every `gh ... 2>\` code call site in E4 (9 sites: L395, L396, L448, L570, L588, L625, L649, L703, L716) and E1 (2 sites: L143, L147) is now followed by an explicit `\0` assertion. The stderr suppression keeps the terminal clean; the exit-code check keeps a silent failure from being scored as a valid result. Applied to cleared sites too — 'cleared' meant `empty is readable by eye`, not `failure is detectable`. Coordinator's earlier `10 sites` count included L574, which is a prose reference to the pattern (not a call site) — the amendment log calls this out to preserve the enumeration trail.
Do NOT retract or annotate the n=1 executed verdict at L24-60. That PASS scored a different instrument — Condition 2's `labeled`-timeline-events query, which returned 15 events — not the broken 3b query. The exposure is prospective; the fix is prospective.
Do NOT re-run E4 or E1 to verify. Coordinator's authorization is scope-bounded to procedure edits; a fresh run against a refreshed fixture is a separate decision.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7532d43f-3da4-4087-9d0b-5a3c38bbf9e9
|
🟢 Impact Analysis — PR #1821Risk tier: 🟢 LOW 📊 Summary
🎯 Risk Factors
📦 Modules Affectedsquad-state (2 files)
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 | 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 | 1 unresolved Copilot thread(s) — fix and resolve before merging |
| ✅ | CI passing | All checks passing |
Files Changed (2 files, +90 −17)
| File | +/− |
|---|---|
.squad/e2e/E1-merge-continuation-relay.md |
+10 −0 |
.squad/e2e/E4-agent-binding-verification.md |
+80 −17 |
Total: +90 −17
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
In E1, the new $LASTEXITCODE checks still allow exit=0 + empty jq output (no matches / no ClosedEvent) to be silently treated as meaningful output, which can leave the procedure non-dispositive.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the E-series E2E procedure docs to make Phase 3b in E4 executable (fixing an invalid gh invocation that could silently pass) and to standardize the “2>$null must be paired with $LASTEXITCODE” extraction guard across E4 and E1, reducing fail-open/silent-success risk when gh calls fail.
Changes:
- Fix E4 Phase 3b issue-label query so it can actually run (remove invalid
--arg, interpolate$E4_START, add prechecks/INCONCLUSIVE routing). - Sweep
$LASTEXITCODEassertions aftergh … 2>$nullcall sites in E4 and E1 and add operator-facing guidance to avoid scoring from silence. - Add/clarify INCONCLUSIVE messaging to prevent dispositive fields from being recorded as
falsewhen extraction didn’t actually succeed.
File summaries
| File | Description |
|---|---|
| .squad/e2e/E4-agent-binding-verification.md | Repairs Phase 3b label-extraction so it’s runnable and adds exit-code + zero-row guarding to avoid vacuous-pass scoring. |
| .squad/e2e/E1-merge-continuation-relay.md | Adds the $LASTEXITCODE house rule to Rule D extractions to reduce silent failures being mis-scored. |
Review details
Suppressed comments (1)
.squad/e2e/E1-merge-continuation-relay.md:157
- This jq expression can legitimately produce no output with exit=0 when there is no
ClosedEvent(e.g., epic still OPEN), because thecloser:(.timelineItems[]|select(...)|...)subexpression yields no results and the whole object is suppressed. That defeats the goal of making “empty response” unscorable. Usefirst(...) // null(and[]?) so the command always emits{state, closedAt, closer}and then additionally treat empty stdout as INCONCLUSIVE.
gh issue view $EPIC --repo $FIXTURE --json state,closedAt,timelineItems `
--jq '{state,closedAt,closer:(.timelineItems[]|select(.__typename=="ClosedEvent")|.actor.login)}' 2>$null
if ($LASTEXITCODE -ne 0) {
Write-Host "epic-state extract: INCONCLUSIVE — gh issue view exited $LASTEXITCODE. This cell is DISPOSITIVE for Rule D: an empty response is not 'epic OPEN' and not 'epic CLOSED'. Do NOT score the verdict from silence; fix the fetch and re-run before assigning PASS / FAIL / NOT VERIFIED."
}
- Files reviewed: 2/2 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.
| gh run list --repo $FIXTURE --workflow "Squad" --limit 20 ` | ||
| --json databaseId,name,conclusion,updatedAt ` | ||
| --jq '.[] | select(.conclusion=="success" and (.name|test("implement|relay"; "i"))) | | ||
| [(.databaseId|tostring),.name,.updatedAt] | join(" | ")' 2>$null | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Host "relay-timestamp extract: INCONCLUSIVE — gh run list exited $LASTEXITCODE. The line above is silence, not 'no matching run'. Do NOT set `window_started_utc` from this output; fix auth/network and re-run before scoring Rule D." | ||
| } |
Flight — review of #1821 (P0 + P1 bundle)Verdict: APPROVE + squash-merge. The P0 fix is verified by execution, not by reading. One I was asked to test four things rather than read them. All four were executed against the live Test 1 — the fixed 3b query actually runsI extracted the 3b block verbatim from the committed file (doc L636–672) into a script and ran
Mode 2 is the one that matters. The pre-#1821 query returned zero rows on every invocation Mode 1 and mode 2 together are the important pair: the query is now capable of returning rows Test 2 —
|
…ct (#1850) setupConsultMode hides .squad/ by appending to git's info/exclude, but resolved that path with `git rev-parse --git-path info/exclude`, which answers for whichever repository ENCLOSES the directory. The guard that refuses an already-squadified project checked the worktree-local .squad/, so the two disagreed about scope: the check was local, the write global. Two ways that write escapes: - from a linked worktree, info/exclude resolves to the MAIN checkout; - from a directory that is not itself a repo root, it resolves to an ANCESTOR repository. Either way .squad/ is hidden in checkouts the caller never named, and info/exclude is untracked and per-clone, so nothing in the repo can undo it. That is why .squad/e2e/ was invisible despite being absent from .gitignore, and why `git add -f` was needed for #1819-#1821. This test suite was itself a live instance of the second case. The fixture built a fabricated `.git/` directory, which is not a valid repository, so git walked out of the fixture and every run appended the consult block to the developer's own checkout. Byte math matches the forensics in #1826 exactly: 240 B template + 73 B block = the 313 B observed there. CI never surfaced it because CI clones are discarded. The fixture now runs `git init`, and the exclude file stays put. Writing to a worktree-local exclude is not an available alternative: git keeps no per-worktree info/exclude, and a file placed at .git/worktrees/<id>/info/exclude is never read. Verified directly. So refusing is the only safe behaviour, and setupConsultMode now checks containment via a new isExcludeOwnedBy() export and points the caller at the main checkout. Closes #1826 Co-authored-by: brady gaster <bradygaster@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20afe6d2-444e-414e-8a39-e67ab67ca6df
E-scenario runbooks are executed by a human operator, never by CI, so a command that cannot run at all can sit in a runbook indefinitely. That is a documentation bug only until it meets a gate whose pass condition is universally quantified -- such a condition is vacuously true over the empty set, so a command that fails and returns nothing writes the passing answer into the verdict rather than an error. E4 Phase 3b did this: gh was passed a --arg flag it does not have, --jq swallowed it as its expression, gh exited 1 with empty stdout, and 2>$null hid it. The two fields marked # dispositive -- where true is the FAIL evidence -- were handed their false by an infrastructure failure. PR #1821 fixed that instance. This adds the mechanism that catches the next one. The linter reads every fenced command in .squad/e2e/ and fails the build on either property that made the silent pass possible: gh invoked with --arg, or stderr suppressed without a nearby exit-code check. It joins shell line-continuations so a flag is judged against the whole command, and reads only fenced blocks, skipping comments -- so E4's own prose postmortem of this defect does not trip the gate that exists because of it. Verified by mutation: reintroducing the original Phase 3b command fails both rules, while the real runbooks stay green. Measured at introduction, 16 of 17 suppression sites already paired with an exit-code check, so this codifies discipline the runbooks already practice. The half that cannot be mechanized -- assert a gate's input set is non-empty before evaluating it, and never let an infrastructure failure reach a dispositive field -- is recorded as convention in the E2E README. Executes no commands; needs no fixture, network, or Actions minutes. Closes #1822 Co-authored-by: brady gaster <bradygaster@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20afe6d2-444e-414e-8a39-e67ab67ca6df
Branched fresh from
cbf90813(the merge commit of #1820). Bundles the P0 (Phase 3b query was structurally incapable of running) with the P1 sweep of the$LASTEXITCODEhouse rule Coordinator established across both E-series procedures.Do not merge — route to Flight.
Flight''s P2 (Rule D cross-check precision — restate step check as
conclusion == cancelled; derivewindow_ended_utcindependently) and P3 (propagate UNREADABLE to L792/L806) are a separate second PR after this one lands, per Coordinator. Not folded in.P0 — Phase 3b has never been able to run
At
dev:.squad/e2e/E4-agent-binding-verification.mdL622-625:ghhas no--argflag.--jqconsumes--argas its expression and everything after becomes stray positionals.ghexits 1 with empty stdout, and2>$nullswallows the failure message.git log -S''--arg t''places it ina2e5c18c(PR #1791) — dead since the day the file was written.Why this was the worst of the ten sites and why the earlier triage missed it:
squad:{x}must be a lowercased roster Name" is vacuously true over the empty set. Same logical shape asSetEquals(∅,∅)at gate 3e (B) and$null -eq $nullat Phase 0b. Third instance of the same hazard in one file.# dispositive:squad_devrel_present/squad_reviewer_present, wheretrueis the FAIL evidence.Fix (this PR):
--arg t "$E4_START"and interpolated$E4_STARTinto the jq expression in PowerShell instead — no jq flag needed.$E4_START-empty precheck. If$E4_STARTis empty,select(.createdAt > "")matches every issue and drags all 8 pre-existing contaminated issues (Feature: Get squads to use project boards #6-9 from 2026-08-19 and Add / Update documentation on Ralph setup - PAT Classic needed #17-20 from 2026-08-21) into the FAIL evidence. That direction fails closed (noisy false FAIL, survivable) but is still wrong; the precheck prevents it and printsINCONCLUSIVEexplicitly.$LASTEXITCODEcheck + zero-row check → prints3b labels extract: INCONCLUSIVEon either failure.Agentcolumn.falseon the two# dispositivefields.P1 — the
$LASTEXITCODEhouse rule, sweptCoordinator''s rule: any
gh … 2>$nullmust be followed by an explicit$LASTEXITCODEassertion. Null-guards test the symptom; the exit code tests the cause. At Phase 3b the null-guard approach couldn''t help — "zero new labelled issues" and "the query exploded" produce the same output. Only the exit code distinguishes them.Applied to every
gh … 2>$nullcode call site incbf90813:E4 (9 code call sites, 1 prose reference):
gh api … ?ref=dev)$sExit)gh api …)$dExit)gh api … squad.md)$bExit)Agentcolumn from empty file)DevRel/devrel)Coordinator''s earlier tallies used "10 sites" — that count included L574, which is prose ("Prefer
--jqwith2>$null, as above"), not a call site. The amendment log calls this out to preserve the enumeration trail. 9 code sites in E4 with the pattern; all now paired with an exit-code assertion.E1 (2 code call sites):
gh run list --workflow "Squad")window_started_utc, an empty response would silently zero out the observation windowgh issue view --json state,closedAt,timelineItems)The E1 sweep also adds a comment naming the house rule at the top of the extraction block so it survives edits.
Cleared sites got the guard too. "Cleared" in the earlier reviews meant the operator reads output by eye, not silent failure is detectable. A cleared site with
2>$nulland no exit-code check is still a place where a silent gh failure becomes a silent zero-row read that the operator then interprets against a pass condition.What this PR does NOT do
labeled-timeline-events query (returned 15 events), not the broken 3b query. That measurement stands..git/info/exclude(Booster owns.squad/in.git/info/excludesilently hides new squad state files from git #1817)..squad/e2e/doesn''t match the drift regex atsquad-ci.yml:271).Verification
cbf90813(verified withgit rev-parse cbf90813).2>$nullmatches incbf90813:E4withgit show | Select-String(11 total → 9 code call sites + 2 prose references). Enumerated all2>$nullmatches incbf90813:E1(2 code call sites). Enumeration recorded in the amendment log so the count is traceable.git add ./-A/-a. Zero deletions. No CRLF-noise files touched.git diff --cached --stat: 2 files, +90/−17.bfa804ca:Methodology note (per Coordinator)
The two same-shape failures earlier in this workstream (
Measure-Object -Linecount, "exactly one exposure" claim) were both completeness-over-unenumerated-set errors. This PR''s amendment log names the enumeration explicitly by site number, tags L574 as prose rather than treating grep''s raw count as authoritative, and records the count difference from Coordinator''s prior "10 sites" tally as a fact rather than smoothing it over. Enumerate, then claim.Related:
cbf90813) — the base for this branch.plan activatehardcoded roster) — the prospective 3b evidence is what this PR restores the ability to score..squad/in.git/info/excludesilently hides new squad state files from git #1817 (.git/info/excludetrap) — not addressed; Booster owns.