Skip to content

Route the SDLC control plane on read facts, grade verification three-valued, and gate merge on it (Refs #3065) - #3123

Open
valorengels wants to merge 19 commits into
mainfrom
session/sdlc-3065
Open

valorengels wants to merge 19 commits into
mainfrom
session/sdlc-3065

Conversation

@valorengels

@valorengels valorengels commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Refs #3065

Implements docs/plans/sdlc-control-plane-asserted-facts.md (revision r2, verdict READY TO BUILD WITH CONCERNS, accepted). The through-line: the SDLC control plane repeatedly decided from a subset of the state it could actually read, and then could not show what it decided from.

What changes

The router routes on facts it read, and shows them (agent/sdlc_router.py, tools/sdlc_next_skill.py)

  • reconcile_dispatch re-runs the guard list against the skill the dispatch table selected, so a guard veto constrains the decision that is about to ship rather than only a proposal a caller happened to supply. Bounded to one pass on the selection and one on its redirect; a non-converging veto returns Blocked(RECONCILE_DEADLOCK) carrying both verdicts instead of iterating into G4's oscillation cap several turns later.
  • G3's redirect ladder gains its /do-docs arm (G3_REDIRECT_REASON_DOCS_PENDING), and arm 1 now requires a recorded APPROVED verdict rather than the REVIEW marker alone.
  • Blocked(NO_RULE) and dispatches carry decision_inputs — the stage_states and meta the router decided from, plus an unrecorded-previous-dispatch signal. Both are compare=False: evidence is not identity.
  • resolve_branch_truth answers "which pushed branch holds this lane's work?" three-valued (found / absent / indeterminate) for both router callers, through resolve_pr_head_sha and never a bare gh read. G8 dispatches only on absent and steps aside on indeterminate.
  • A recorded lane slug proven wrong by a unique branch-truth match is now repairable at the fail-closed decision point, with the evidence recorded (tools/lane_identity.py). Ordinary reads still return the recorded slug.

The verification runner can say "I could not evaluate this" (agent/verification_parser.py, scripts/validate_build.py)

  • passed: bool is replaced by a three-valued CheckOutcome (PASS / FAIL / UNEVALUATED) — removal, not addition. UNEVALUATED covers timeouts, runner exceptions, unrecognized expectation forms, and command cells with no backticked span, each carrying a reason, and never renders as [FAIL].
  • The expectation grammar gains exit N, prints \N`, >= N, > N, == N, and empty output`. Newer forms are anchored; the three pre-existing forms stay prefix-matched so the trailing-gloss idiom in live plans keeps working.
  • Check tables are classified by column contract rather than by any of the first three headers being Command; a non-check table emits a SkippedTable diagnostic instead of having its second column executed as a shell command.
  • validate_build.py and the parser converge on one timeout bound and one disposition, with timeout and malformed rows added to the parity fixture so a divergence can actually be caught.

This closes the substance of #2791, #3022, and #2901, all three of which were closed as consolidated with no fix and all of which reproduced on main.

The merge predicate can see a gate the plan declared (tools/merge_predicate.py)

  • The runner records its graded aggregate to _verification_outcomes in the ledger's existing stage_states JSON; the predicate reads it. Recorded state only — the gate never re-executes plan-authored commands.
  • A FAIL or UNEVALUATED row refuses the merge and names the row. This makes the Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 / ba092a06d owner ruling machine-readable; it previously lived only in plan prose, which is how PR Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 merged past it.
  • The aggregate is stamped with the PR head SHA it was graded against and trusted only while fresh. Mismatch, absence, or an unresolvable head all refuse — deliberately stricter than the REVIEW-verdict path's timestamp fallback, because there are no legacy records to stay compatible with.
  • The build-vs-ship split lives on the consumer, not on a row annotation: the build gate may let UNEVALUATED pause and proceed, the merge predicate may not. No GATE: marker, no new frontmatter key, no fourth plan grammar.

session-ensure stops destroying the lease it was asked to grant (tools/sdlc_session_ensure.py)

Readback is by primary key rather than re-querying and taking [0], candidate provenance is tracked explicitly instead of being inferred from an overwritten reuse_run_id, and all three release sites are gated on provenance == minted. release_issue_lock is unchanged — it was correct; the bug was what it was handed.

Round 2 patch — the three blockers

Round 2 returned CHANGES REQUESTED on three blockers, all in the arming of the gate rather than its design. All three are fixed on 76e8d3055.

1. The writer's argv parser accepted a flag as a value (scripts/validate_build.py). The documented invocation interpolates shell variables, so an empty one collapsed the argument list and the naive reading took the following flag. _parse_argv now rejects a value that is itself a flag, takes the plan path from the first non-flag positional, and reports an unparseable --issue/--pr on its own line rather than raising through the report. All three reproduced modes are pinned by tests, and re-verified live on this head:

Reported mode Was Now
Empty $ISSUE_NUMBER--issue --pr 77 ValueError after the summary, exit code rewritten ARGS: ignoring --issue, RECORD: skipped, exit unchanged
Empty $TARGET_REPO--repo --issue 4242 ledger row written under repo --issue, reported success ARGS: ignoring --repo, RECORD: skipped, no write
--record-outcomes plan.md (flag first) exit 0 having run zero checks usage message, exit 1

The variables in docs/sdlc/do-pr-review.md:72 are quoted.

2. read_verification_outcomes failed open (agent/verification_parser.py). It was the one read in the merge path that fetched the blocking evidence, and the predicate could not tell an error from an absence — so a store blip converted a recorded FAIL into an unenforced pass, inverted against every neighbouring group in the same module. Genuine absence still returns None; anything that prevents an answer raises VerificationOutcomesUnavailableError, and _check_verification_outcomes failed.appends on that branch while keeping absence as a note.

3. Group (e) was a merge refusal no dispatch rule could see (agent/sdlc_router.py). Row 8g is its routing-side twin, ordered immediately before row 10 so it preempts only the merge dispatch: a blocking or unfresh aggregate routes to /do-pr-review, which re-runs § 4.5 and re-records, instead of dead-ending on a merge the predicate is certain to refuse until G4 blocks the lane. Its dispositions mirror the predicate's exactly, and test_router_and_predicate_agree_on_the_blocking_set asserts that agreement across the whole CheckOutcome enum so the two sides cannot drift.

Review items also fixed (in blast radius)

  • TD 1 — the REVIEW-stage invocation is now pinned by assertion (TestRecordingIsWiredIntoTheReviewStage), not only by tests that patch the writer. Deleting the flag from the addendum now fails a test.
  • TD 2docs/sdlc/merge-troubleshooting.md gains a Verification Outcomes Hold the PR section naming both new refusals, the FAIL vs UNEVALUATED distinction, and the --record-outcomes re-run.
  • TD 3DEFAULT_TIMEOUT_S gets a lever: VERIFICATION_TIMEOUT_S and a --timeout flag wired into the parameter the function already accepted. This is what resolved the reviewer's one UNEVALUATED row; it was contention, as they diagnosed.
  • TD 4 — the plan gains ### Risk 8 naming the open edge honestly: the gate fires only on a recorded aggregate, absence is not enforced, and the § 4.5 trigger is skill prose. The reviewer's accepted rationale for not failing closed on absence is recorded there rather than left implicit.
  • TD 9 — the trailing-gloss rule is now symmetric across >, >=, and ==: the output-prefixed spellings are prefix-matched (the live authoring idiom), the bare spellings stay anchored. This unblocks three real rows in other lanes' plans (durability-m1-fence-canary.md:1012, watch-skill-video-scoping-controls.md:606-607) that a blocking gate would otherwise have held. Pinned verbatim as regression cases.
  • TD 10 — the false 4-column docstring at test_verification_parser.py:561 is corrected to describe what the test actually pins.
  • Nits 1, 2, 3, 4, 8, 9None-checked unpack at the check_column_indices seam; dead docs_status != conjunct removed; RECONCILE_DEADLOCK documented; G4's Blocked now carries decision_inputs (the one blocked verdict Cluster D names as its motivation, previously the only one with no evidence — swept to four more Blocked sites while there); parenthesized !r binding; and the four tests that leaked real issue locks into the shared Redis now release them in a try/finally fixture via release_issue_lock.

Deferred, with reasons

These are all carried, pre-existing on main, and structural rather than mechanical — each wants its own lane rather than an edit inside a merge-gate PR:

Item Why deferred
TD 5 — session_runner/runner.py:1415 runs up to five network round trips on the event loop Real, and correctly traced. It is an async-architecture fix in a file this PR does not otherwise touch; the payload it buys (next_skill) never changes decision.allow.
TD 6 — repair_lane_slug reachable from a read-intent path Placement, not correctness, as the reviewer says. Threading allow_repair through _verify_stage_artifacts(_live) changes a signature two callers share.
TD 7 — patch_claimed dropped the pr_identifiable conjunct Narrows #2757 for a merged-and-branch-deleted lane whose PR lookup failed. Needs the three-valued indeterminate treatment, which is a sdlc_next_skill change, not a gate change.
TD 8 — lease heartbeat gated on reuse_run_id rather than _CandidateProvenance Pre-existing on main. This PR introduces the primitive that expresses it; using it is a behavior change to lease acquisition and deserves its own test round.
Nits 5, 6, 7 — a doc sentence on branch_exists, _CandidateProvenance as a StrEnum, object-typed parameters Cosmetic and outside the gate; batching them into a follow-up keeps this diff readable.

Happy to file these as a single follow-up issue if the reviewer prefers them tracked rather than listed.

Round 3 patch — reachability and blast radius

Round 3 confirmed two of round 2's three blockers fixed, and returned three more: one was round 2's blocker 3 incompletely fixed, and two were the consequences of the gate arming for real. All three are fixed on a9d411beb.

1. Row 8g was structurally inert. The rule was correct in isolation but never fired. Both router callers take stage_states from query_enriched()["stages"], which filters to ALL_STAGES and then threads exactly two underscore keys back in — _verification_outcomes was not one of them. So the rule returned False for every production lane while 626 tests passed over it, because every row-8g test hand-built its payload with the key already injected. The one-line thread-through is at tools/sdlc_stage_query.py, and the important half is the seam test: it drives row 8g from a real query_enriched payload. I verified it is load-bearing by deleting the key from the tuple and watching the test fail with KeyError: '_verification_outcomes', then restoring it.

This is the same class of gap TD 1 closed on the writer side, and the fix has the same shape.

2. A plan with no verification table recorded a 0-row UNEVALUATED and was refused forever. 4 of the 20 live plans are in that shape. The governing principle: absence of a contract is not a failed contract. A plan that declares no check table has declared no gate, so the runner now declines to record and says so — which is what the plan's own ### Risk 8 claimed the design already did. The state is distinguishable from a 0-row aggregate and from an unreadable one. The exit code is untouched.

RECORD: skipped -- plan declares no verification table, so there is no gate to record

3. Two in-flight PRs would have been refused on legitimate authoring. promise-gate-recorded-obligations.md (PR #3093) and unblock-dependency-bumps-coupled-set-gate.md (PR #3089) carried four cells the widened grammar still did not cover: output `0` , output is 0, and exit 0, JSON "compatible": true. The gloss rule is now stated once as a rule — a gloss set off by a delimiter (a comma, or a paren/backtick span) is ignored; a bare trailing word stays ambiguous and UNEVALUATED — which grades exit 0, JSON ... while leaving exit 0 maybe correctly unevaluated. no output is accepted as an exact synonym of empty output. Both plans now grade 0 ungradeable.

The regrade, re-run after extending the grammar

Every distinct Expected cell in docs/plans/ (50 cells across 20 plans), graded under origin/main's grammar and this branch's, across 8 (exit, output) samples — 400 gradings per side:

differences: 58
PASS<->FAIL verdict flips: 18
  ...on cells the OLD grammar actually recognized:  0
  ...on cells it never accepted (its FAIL was a lie): 18

The raw flip count is misleading until you separate the two, so this run does it mechanically. origin/main's evaluate_expectation returns a bare False on fallthrough, so its FAIL on an unrecognized form was never a verdict — it was the #2901/#3022 defect this PR exists to fix, a gate saying "your code is wrong" when it meant "my grader is wrong". A cell the old grammar genuinely recognized must return True for at least one sample; zero such cells changed. Every one of the 58 differences is on a cell the old grammar never accepted.

Five cells remain ungradeable, all in two plans with no open PR (agent_wiki.md, watch-skill-video-scoping-controls.md): ok, pass, watcher line 67 skips, contains `[^a-z0-9` , and single scalar == 0 (...). The first three are genuinely unusable expectation cells rather than a grammar gap — UNEVALUATED reporting that the grader could not answer is the correct outcome, and guessing at them is exactly what this module must not do. Noted in #3125 as an authoring pass owed before either lane ships.

Review items also fixed

  • TD 1docs/sdlc/do-merge.md gains a Verification Outcomes section pointing at the recovery in merge-troubleshooting.md, so the MERGE-stage agent can reach the remedy without a lucky grep.
  • TD 2 — an empty "$PLAN_PATH" no longer produces a raw IsADirectoryError (Path("") is Path("."), which .exists() accepts); the guard is now .is_file().
  • TD 3 — a non-positive --timeout or VERIFICATION_TIMEOUT_S is rejected at both entry points. --timeout 0 previously turned all 29 rows into UNEVALUATED, and since the recorder writes regardless of exit code, a stray env value would have persisted an aggregate that held the lane.
  • TD 4 — a failed anchor attempt is now stamped distinctly from "never anchored". Fixing blocker 1 downgrades this further: an unanchored record now self-heals by routing back to re-review instead of dead-ending at G4.
  • TD 5 — the troubleshooting remedy names the step it was missing: plan files live on main and never travel in the PR, but /do-pr-review grades "$PLAN_PATH" in the lane worktree, so an Expected-cell fix must reach the branch before re-running or the row refuses again and reads as though the fix failed.
  • Nits 1-6 — the unreachable except ... : raise is removed; _parse_argv rejects single-dash values (--issue -5 no longer yields a negative issue number), reports repeated flags and dropped positionals instead of swallowing them, and honors --help anywhere in argv.
  • Nit 2 (the drift test) — this one deserved more than the nit it was filed as. The old test re-spelled {FAIL, UNEVALUATED} locally and compared the router to itself, so a drift in the predicate could not have failed it. The blocking set is now defined once as BLOCKING_OUTCOMES in agent/verification_parser.py — the only place both sides can reach, since the router may not import tools/ — and both consumers read it.
  • Nit 3 — the row-8g docstring no longer overclaims. It now says the mirroring is on the overall outcome, names the malformed-aggregate shapes the predicate refuses and 8g does not inspect, and explains why those should reach the predicate's refusal rather than be re-routed by a rule that cannot explain them.

Carried deferrals now tracked

Filed as #3125, as the reviewer asked: TD 5-8 (event-loop network calls in the session runner, repair_lane_slug placement, the pr_identifiable conjunct, the lease-heartbeat provenance gate) and carried nits 5-7, plus the five ungradeable cells above. All are pre-existing on main and structural enough to want their own lane.

Verification

scripts/validate_build.py against the plan, on a9d411beb: 29 PASS, 0 FAIL, 0 UNEVALUATED, 0 SKIP, exit 0.

Narrow-scoped tests across every touched suite (test_sdlc_router, test_sdlc_stage_query, test_verification_parser, test_validate_build, test_merge_predicate, sdlc_session_ensure/, test_sdlc_next_skill): 677 passed, 0 failed. ruff check . and ruff format --check . clean across all 1445 files.

Merge order

This PR must merge after #3089 and #3093. It arms a gate that grades their plans, and although both now grade clean, landing the gate first would put two in-flight lanes behind a mechanism they have never been reviewed against. Both were still OPEN at the time of writing.

Superseded: round-2 verification numbers

scripts/validate_build.py against the plan, on 76e8d3055: 29 PASS, 0 FAIL, 0 UNEVALUATED, 0 SKIP, exit 0.

Round 2 reproduced 28/0/1 — the one UNEVALUATED was Verdict unit tests pass timing out at the 120s bound under concurrent load, which the reviewer correctly diagnosed as contention (43s standalone) and filed as TD 3. Re-running with the --timeout lever this patch adds returns the row to PASS. The reviewer was right to record the previous body's "29 PASS" as unverified at that head; the number is reproduced here, and this is a live demonstration of the lever rather than a raised constant.

Tests, narrow-scoped to the diff: test_sdlc_router.py, test_verification_parser.py, test_validate_build.py, test_merge_predicate.py, tests/unit/sdlc_session_ensure/ = 455 passed, 0 failed. Ruff check and format clean on every touched file.

Two things the reviewer should rule on

  1. This is one PR where the plan recommended two. Open Question 1 recommended splitting the router half (clusters A/D) from the verification/merge half (B/C) as two sequential PRs, on Risk 6 grounds — the router half is higher-risk and deserves undiluted attention. A predecessor build had already landed both halves in interleaved commits before this session resumed, so splitting would now mean rewriting history rather than sequencing work. Naming it rather than quietly bundling; if the reviewer wants the split, say so and it can be done as two stacked branches.
  2. docs/features/README.md carries a two-row sort fix that is not this lane's work. Two Telegram rows were already out of alphabetical order on main, and the sort validator blocks any edit to that file until corrected. The swap is mechanical and content-free.

Refs #3065, not Closes — this lane closes the batch residual, not all 51 consolidated members (#2491's pipeline-graph duplication is the largest survivor). Ratified by the PM 2026-09-03.

)

Replaces CheckResult.passed with CheckOutcome, extends the expectation
grammar, classifies check tables by column contract, extracts the first
backticked span as the command, converges validate_build onto one bound
and one timeout disposition, and persists the graded aggregate.
#3065)

Replaces test_unknown_expectation_returns_false (which pinned the bug),
adds the extended grammar, command-cell extraction, run_checks timeout
and exception paths, the #3022 header shape, and the persisted aggregate.
…Refs #3065)

The parity fixture gains the two shapes on which the runners actually
disagreed and which it could not previously express: a timeout (FAIL at
120s in the canonical runner vs a non-blocking SKIP at 30s in
validate_build) and an expectation neither grammar reads. validate_build
now takes its bound and its timeout disposition from the canonical
runner, and UNEVALUATED blocks the exit code where SKIP did not.
A sweep of docs/plans/ found eleven live rows writing a trailing gloss on
exit code N / output > N. Anchoring those would have turned working rows
into blocking UNEVALUATED for a change nobody asked for, so the
pre-existing forms keep prefix semantics and only the forms this lane
adds are anchored. Verified against main: the only grading change across
active plans is 'exit 0', which previously graded false.
…efs #3065)

Covers task 1 of docs/plans/sdlc-control-plane-asserted-facts.md (Cluster E):

- duplicate-row readback stability: two real AgentSession rows sharing one
  session_id, six consecutive binds onto a specific row, all must succeed.
  Deterministically red on the pre-fix filter(session_id=...)[0] readback.
- adopted candidates survive all three release sites (save failure, raising
  readback, readback mismatch) and both adopt shapes (live-lock reuse and
  supervised-signal self-recognition); the live lease stays owned.
- minted candidates are still released at the save and readback sites, so
  provenance gating does not become never-release.
- structural: the bind path calls query.get and never query.filter.

Also stubs query.get on the multi-lineage integration readback mock, whose
four concurrent-contention cases modelled the old session_id-index readback.
Task 8: group (e) reads the _verification_outcomes aggregate, checks it
fresh against the PR head fail-closed, and refuses on any FAIL/UNEVALUATED
row by name. Reconciles a concurrent duplicate implementation of the same
task in this worktree.
…3065)

Task 8. Group (e) reads the recorded `_verification_outcomes` aggregate --
never re-executing a plan-authored command (PM ruling, 2026-09-03) -- and:

- refuses on any FAIL or UNEVALUATED row, naming the row and its reason.
  This is the #3080 / ba092a0 owner ruling made machine-readable; it lived
  only in plan prose and PR #3080 merged straight past it.
- checks the aggregate's freshness against the PR's current head first, via
  head_sha_of_record vs resolve_pr_head_sha, fail-closed on all three
  dispositions: match grades, mismatch refuses with
  VERIFICATION_OUTCOMES_STALE_REASON without reading the cached PASS, and a
  missing/unparseable head_sha or unresolvable PR head refuses. Deliberately
  stricter than group (c)'s recorded_at fallback, which exists only for
  records predating #2769.
- keeps three branches distinguishable: no plan document and no recorded
  aggregate are REPORTED; a present aggregate that cannot be shown fresh is
  ENFORCED.

The plan document is resolved through `tracking:` frontmatter
(lane_identity.find_plan_path, scoped to the predicate's explicit repo_root),
never by filename -- the tests' plan file is deliberately named something
other than the lane slug.

VERIFICATION_OUTCOMES_STALE_REASON is defined in tools/merge_predicate.py,
where the predicate consumes it; the unread copy in agent/verification_parser.py
is removed so there is one definition, not two.
…arm, guard reconciliation

Tasks 2, 3 and 4 of docs/plans/sdlc-control-plane-asserted-facts.md.

Task 2 — decisions carry their evidence (Cluster D). Blocked(NO_RULE) now
carries `decision_inputs`: the stage_states and meta it decided on, plus an
`unrecorded_dispatch` signal, surfaced through sdlc-tool next-skill's JSON.
The batch reported a NO_RULE on a state row 5 has owned since c1e9919 and
the report could be neither confirmed nor refuted, because the payload carried
nothing. detect_unrecorded_dispatch names all three "no confirming record"
shapes — no record at all, a record naming another skill, and a router slot
never confirmed by a stage entry — so a skipped `dispatch record` surfaces now
instead of four turns later as a G4 oscillation block blamed on the wrong
cause. next-skill still persists nothing; this is a read.

Task 3 — G3's redirect ladder is complete. Adds the /do-docs arm
(G3_REDIRECT_REASON_DOCS_PENDING) for REVIEW complete + APPROVED + DOCS
pending, which previously fell to the else and was sent back to a
/do-pr-review it had already passed. Arm 1 now requires a recorded APPROVED
verdict rather than the REVIEW marker alone, matching the #1932 gap-(c) gate
rows 9 and 10 already apply — a completed REVIEW marker with no verdict is an
unearned marker, not evidence of approval, and must not fast-path to /do-merge.

Task 4 — guards are reconciled against the selected dispatch (the keystone).
Guards ran at :2246-2248, the table at :2250-2255, and nothing re-validated in
between, so G3 constrained a suggestion but never a decision. reconcile_dispatch
re-runs the guard list with the table's selection as the proposed skill.
Bounded by construction: exactly one pass on the selection, at most one on the
resulting redirect, then a Blocked(RECONCILE_DEADLOCK) carrying both the
selected row and the vetoing guard. Never a third pass. A guard naming the
skill already chosen is agreement, not a veto, and does not trigger a block.

Row 2b's predicate is NOT edited — #1639 made it marker-agnostic deliberately;
it is constrained from outside. On the #2771/#2334 shipped-lane shape the
router now answers /do-docs where main answers /do-plan-critique.

The guards are not pure and reconciliation runs them twice. This is a stated
invariant, written into reconcile_dispatch's docstring:
guard_g5_artifact_hash_cache mutates record["artifact_hash"] in place and logs
a WARNING on legacy-hash migration, and double invocation is idempotent ONLY
because stage_states is passed by reference. A defensive copy would re-run the
migration. Tests assert the WARNING fires exactly once per decide call and that
the guards receive the caller's own objects by identity.

A raising guard during reconciliation is deliberately not caught, preserving
the existing asymmetry (rule predicates are try/except-wrapped at :2260-2263,
guards at 1083-1086 are not). Swallowing a bug into a NO_RULE would misreport
it as a routing hole.

Evidence fields are compare=False on both Dispatch and Blocked: evidence is not
identity, and without it attaching evidence would silently redefine equality for
every caller comparing against an expected Dispatch(...). That keeps
tests/unit/test_sdlc_router.py purely additive — every pre-existing assertion,
including the bfa4a6f / d9cf29d / 3c689f2 regression floor, is unchanged.

Demonstrated red against main (#2658), all four executed:
  - Blocked has no decision_inputs field at all
  - shipped lane routes /do-plan-critique instead of /do-docs
  - docs-pending falls to the else, producing a redundant /do-pr-review
  - REVIEW+DOCS completed with no verdict fast-paths to /do-merge

Verified: 561 passed, 0 failed across tests/unit/test_sdlc_router.py,
test_sdlc_verdict.py, test_sdlc_router_oscillation.py,
test_sdlc_router_reconciliation.py, test_sdlc_next_skill.py,
sdlc_router_decision/ and test_sdlc_dispatch.py. ruff check and ruff format
clean on both touched source files.

Refs #3065
…#3065)

resolve_branch_truth answers found / absent / indeterminate from the PR head
SHA (via tools/pr_head_resolver.resolve_pr_head_sha, never a bare gh read)
matched against git ls-remote --heads origin. It replaces _check_branch_pushed,
whose two-valued answer gave a wrong-but-present recorded slug, a genuinely
unpushed branch, and an unreachable remote the same fail-closed /do-patch.

G8 may fail closed on absent only; indeterminate is reported and deferred.
An infra error in _verify_stage_artifacts now reports itself as indeterminate
instead of being indistinguishable from a clean verification. The PATCH check's
old 'no recorded PR number -> no-op' proxy is replaced by a MERGE-completed
skip, since a lane with a PR whose head matches nothing is now indeterminate.

agent/session_runner/runner.py stops deciding from an empty context: both
decide_next_dispatch callers now assemble it through build_decision_context.
…Refs #3065)

repair_lane_slug corrects a recorded slug that branch truth contradicts. It
fires only where a fail-closed decision is about to be taken on the recorded
name, and only on a UNIQUE git ls-remote --heads origin match against the PR
head SHA resolved through tools/pr_head_resolver.resolve_pr_head_sha. Zero and
two-or-more matches leave the record alone, matching _adopt_from_pr's ambiguity
discipline; one matcher (_match_pr_head_to_lane_branches) now serves both so
that discipline cannot drift between them.

_record_slug_if_empty is deliberately not reused: its no-overwrite behavior IS
the defect. The repair re-reads the recorded value under the slug lock
immediately before writing, so a concurrent repair converges to a no-op rather
than a second write (Race 2), and a record that moved to a third value is left
untouched. Every correction files its justification on the ledger.

Rung 1 is unchanged: ordinary reads still return the recorded slug. The module
docstring's 'a wrong adoption could never be corrected' claim is replaced.
#3065)

Branch truth resolving to a branch other than the one the recorded slug names
IS the proof repair_lane_slug is gated on, and the G8 PATCH check is where that
proof is produced. Wire the two together so a slug proven wrong is corrected
then, rather than left to confuse every future tick and every other consumer of
the recorded identity. Best-effort and non-fatal; repair_lane_slug
re-adjudicates uniqueness itself, so a stale read here cannot force a bad
write.
Both poles: a FOUND branch that disagrees with the recorded slug calls
repair_lane_slug with the issue number and resolved repo; a FOUND branch that
agrees must not call it at all.
… (Refs #3065)

New feature doc for reconciliation: why a guard must see the selected dispatch
and not only a caller's proposal, the at-most-two-pass bound, the by-reference
invariant that keeps the impure G5 guard idempotent, and how to read a Blocked
carrying two verdicts.

machine-readable-dod gains the expanded expectation grammar, the anchored-vs-
prefix-matched split, and the UNEVALUATED outcome with its build-gate/merge-gate
consumer split. The pipe-escape rule is now reachable from the expectation list
instead of only from the anti-criteria section.

PLAN_TEMPLATE carries the grammar, three-valued grading, and the escape rule
inline, with a sample anti-criterion that actually demonstrates BRE alternation
(doubled backslash) — proven two-pole through the parser rather than asserted.

CLAUDE.md is deliberately unedited: reconciliation changes no caller-visible
router contract, and that file is regex-parsed into worker prompts.

The features README sort fix on the two Telegram rows is pre-existing drift on
main, not from this lane; the sort validator blocks any edit to the file until
it is corrected.
…efs #3065)

Principle 9 already carried the head-SHA rule for one gate. The merge predicate
now reads a second recorded fact under the same rule: a plan's Verification rows
grade three-valued, a FAIL or UNEVALUATED row refuses the merge, and the recorded
aggregate is trusted only while its stamped head SHA matches the PR head.

The build-vs-ship asymmetry is stated where callers will look for it: UNEVALUATED
may let a build proceed and may never let a lane ship. That ruling previously
lived only in one plan's prose, which is how PR #3080 merged past it.

Confined to principle 9; `## Work Completion Criteria` is untouched because it is
regex-parsed into worker system prompts and asserted byte-for-byte.
@valorengels

Copy link
Copy Markdown
Collaborator Author

Review: Changes Requested

Multi-judge consensus (code-quality, risk), rule any-blocker-wins. Both judges ran independently against the PR head in a clean worktree; the code-quality judge raised two blockers, the risk judge raised none. Consensus verdict: CHANGES REQUESTED.

This is strong work. The router half is more carefully reasoned than most things that land here: the reconciliation step is bounded by construction, its by-reference invariant is documented at the one place a later contributor would break it, and the risk judge's independent enumeration of every proposed_skill reader confirms Risk 1 and Risk 2 are mitigated in the code rather than merely argued about in the plan. The tri-state grading is a genuine removal, not an addition. The tests are real — test_sdlc_router_reconciliation.py guards its own fixtures so it cannot silently stop testing, and the readback tests use two real duplicate AgentSession rows rather than mocking the duplicate away.

The two blockers are both instances of the shape this lane exists to eliminate.

Blocker 1 is a gate structurally incapable of firing. record_verification_outcomes has zero production callers — a repo-wide grep finds it only in two test modules and one docstring. Neither runner this PR rewrote records: not scripts/validate_build.py::check_verification_table, and not the one-liner in docs/sdlc/do-build.md:187. So read_verification_outcomes always returns None, _check_verification_outcomes always takes the aggregate is None branch, and group (e) is "reported, not enforced" for every lane that will ever reach it. The #3080 ruling is not machine-readable; it is a reader with no writer. Plan task 8 said "Add the write to task 7's outputs: the runner gains a persistence point alongside its existing result rendering. If the builder finds the runner has no natural write point for this, stop and escalate rather than forcing one — that is a real design question, and the PM asked to be brought it directly." Neither the write nor the escalation happened, and the PR body states the write as done.

The risk judge found the second half of this, which changes what the fix has to be: group (e) is not only inert, it is not yet safely enable-able. record_verification_outcomes skips the head_sha stamp when pr_number is falsy, and the runner is invoked at BUILD time, typically before a PR exists. Wiring the writer into /do-build alone would produce unanchored aggregates that the reader then refuses with a reason no lane can clear — and the same applies to every lane that takes one more commit after grading, which is nearly all of them, since /do-docs and /do-patch run after TEST. The missing piece is a re-grade at REVIEW/DOCS time with the PR number in hand.

Blocker 2 is a live regression, measured on both sides. The new positional column contract turns docs/plans/overclaim-guard-greps-whole-worktree.md (tracking #2807, header | # | Check | Command | Expected |) from 30 executable checks into 0 checks and 2 malformed rows — including the | Rows | Run from | Why | prose table that main correctly classified as skipped. I reproduced both sides: main parses it 30/0/1, this branch parses it 0/2/0. The plan's own anchoring rule refuses to break live plan rows for "a change nobody asked for"; this applies the opposite standard one function over.

I ran the plan's ## Verification table myself rather than crediting the PR body: 29 PASS, 0 FAIL, 0 UNEVALUATED, exit 0, reproduced. The narrow-scope test counts in the PR body (837 passed) were not reproduced and are recorded as unverified, not as satisfied.

Ruling on the two questions you raised. (1) One PR where the plan recommended two: accepted. A predecessor build interleaved the commits before this session resumed, so splitting means rewriting history rather than sequencing work, and you named it instead of bundling it quietly. (2) The docs/features/README.md two-row sort fix: accepted, mechanical and content-free.

Rubric

  • 1. Plan vs. implementation match — fail — Task 8's persistence point was neither implemented nor escalated; the Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 success criterion is not met in any production path.
  • 2. New code quality — pass — High. Docstrings carry the reasoning, named constants replace inlined prose, the by-reference invariant is documented where it would be broken. Nits only.
  • 3. Test coverage — pass — Broad and real, both poles on freshness, failure paths exercised. One coverage gap (T1) is the guard that should have caught blocker 2; recorded as tech debt.
  • 4. Regression risk to existing callers — fail — A live plan's Verification gate goes from 30 running checks to unrunnable (blocker 2). Separately, the PATCH-claim no-op change narrows the Post-merge, G8 dispatches /do-build on completed work: artifact verifier resolves PR None once the PR is closed #2757 protection (tech debt).
  • 5. Data integrity — pass — _verification_outcomes is a new key in an already-flexible JSON blob, mirroring _verdicts; PM ruled no migration required. _write_slug_repair re-reads under the slug lock and compares to the expected value before writing.
  • 6. Security — pass — No new eval/exec, no request-derived subprocess input, no secrets. subprocess(shell=True) is pre-existing and scoped to plan-authored commands, which the merge predicate explicitly never executes.
  • 7. Documentation accuracy — fail — CLAUDE.md principle 9 says "UNEVALUATED may let a build proceed" while scripts/validate_build.py:401, docs/sdlc/do-build.md:189, and docs/features/machine-readable-dod.md:110 all say it blocks the build.
  • 8. PR body accuracy — fail — "The runner records its graded aggregate to _verification_outcomes … the predicate reads it" — the reader exists, the writer is never called.
  • 9. Disclosed deferrals — pass — Both disclosures are explicit ask-the-reviewer items with sound rationale; ruled on above.
  • 10. Follow-up claims verified — n/a — The PR body makes no "filed as follow-up #N" claim.

Pre-Verdict Checklist

  • 1. All plan acceptance/success criteria validated against diff — FAIL — The Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 machine-readable-gate criterion and the "runner records its aggregate" criterion are unmet in production.
  • 2. No-Gos from plan — none violated — PASS — ANTI rows confirm .claude/hooks/validators/ untouched, pyproject.toml/uv.lock untouched, no raw Redis on Popoto keys, no Claude co-authorship trailer. Refs #3065 used, not Closes.
  • 3. New except Exception blocks — each has logger/raise/swallow-ok — PASS — Every new broad catch logs with type and message; the fail-open direction is stated in the docstring at each site.
  • 4. New integration tests — exercise serialization boundary (not in-memory only) — PASS — test_merge_predicate.py's gate_lane writes through a real PipelineLedger and reads back through the real predicate.
  • 5. Plan internal consistency — spike findings match task steps — PASS — Spikes 1-5 each map to a task; the named deviation on critique blocker 1 is recorded in the plan.
  • 6. No hardcoded secrets or debug artifacts — PASS
  • 7. New public APIs — docstrings present — PASS — reconcile_dispatch, resolve_branch_truth, repair_lane_slug, record_verification_outcomes, aggregate_outcomes all documented, several unusually well.
  • 8. Breaking changes — migration path documented — PASS — passed: boolCheckOutcome is a hard removal; the scoped reader sweep is an ANTI row and docs/sdlc/do-build.md updates the caller one-liner.
  • 9. Tests added for new behavior — PASS
  • 10. Tests cover the failure path (not just happy path) — PASS — Timeout, runner exception, unparseable expectation, empty cell, no-backtick cell, stale aggregate, ambiguous branch match, unreadable remote all have cases.
  • 11. UI changes (if any) — screenshot captured — N/A — No HTML/CSS/JS/TSX/Vue/template files in the diff; the visual-proof gate is a no-op.
  • 12. Docs updated for user-facing changes — FAIL — Cascade is otherwise complete, but CLAUDE.md contradicts the three artifacts it should agree with (see rubric 7).

Blockers

  • agent/verification_parser.py:849def record_verification_outcomes(...)The writer has no production caller, so merge-predicate group (e) can never fire. A repo-wide grep finds the symbol only in tests/unit/test_verification_parser.py, tests/unit/test_merge_predicate.py, and a docstring at tools/merge_predicate.py:746. Neither runner rewritten by this PR calls it: scripts/validate_build.py::check_verification_table does not, and the one-liner at docs/sdlc/do-build.md:187 does not. read_verification_outcomes therefore always returns None and _check_verification_outcomes always exits at the aggregate is None note. Plan task 8 required the write be added to task 7's outputs, or that the builder stop and escalate; neither occurred, and the PR body asserts the write as delivered. Fix: add a real persistence point — preferably a python -m agent.verification_parser --plan … --issue N --pr N entrypoint the skill docs invoke, rather than a copy-pasted shell one-liner. Wire it so grading happens (or re-happens) at REVIEW/DOCS time with the PR number in hand, because a BUILD-time-only writer produces head_sha-less aggregates the reader refuses with a reason no lane can clear. Alternatively, make the writer return False rather than persist an unanchored aggregate, so that record can never exist. If wiring the writer is genuinely out of this PR's appetite, then task 8's escalation clause is the honest path: say so, file the writer as a tracked follow-up, and correct the PR body's claim.

  • agent/verification_parser.py:250header_cells[1].strip().lower() == "command" and header_cells[2].strip().lower() == "expected"The new positional contract breaks a live plan's verification gate. docs/plans/overclaim-guard-greps-whole-worktree.md (tracking Overclaim guard greps the whole worktree, so a stale pre-fix .pyc in tools/__pycache__ fails a clean source tree (and the PATH-resolved grep makes it environment-dependent) #2807) carries | # | Check | Command | Expected |. Measured both parsers against that file: origin/main yields 30 checks / 0 malformed / 1 skipped; this branch yields 0 checks / 2 malformed / 0 skipped — the section falls to the "zero executable checks" branch and even the | Rows | Run from | Why | prose table, correctly skipped on main, becomes a malformed row. Thirty real checks stop running. This is the same standard the anchoring rule in evaluate_expectation explicitly refuses to apply to live rows ("anchoring would turn eleven working rows into blocking UNEVALUATED for a change nobody asked for"). Fix: locate the Command/Expected pair instead of pinning it to indices 1 and 2 — find i >= 1 where header_cells[i] == "command" and header_cells[i+1] == "expected", then read the row as (cells[i-1], cells[i], cells[i+1]). That still rejects both false positives the docstring names: | Command | Observed stdout | Observed exit | (no Expected follows Command) and | # | Criterion | Check |. Add the real 4-column header shape to the test at tests/unit/test_verification_parser.py:561 (see tech debt below).

Tech Debt

  • tests/unit/test_verification_parser.py:561"| Check | Command | Expected | Notes |\n|--|--|--|--|\n" — The test named test_column_count_comes_from_the_header documents itself as protecting "one plan in docs/plans/ [that] carries a 4-column Verification table", but that plan puts the extra column first (| # | Check | Command | Expected |), not last. No plan in the repo uses Check | Command | Expected | Notes. The test passes while the plan it names fails — this is the guard that should have caught blocker 2, reading as coverage without being coverage. Fix: use the real header shape, and consider a sweep over docs/plans/*.md asserting every ## Verification section still yields at least one check.

  • agent/session_runner/runner.py:1415context = build_decision_context(issue_number, stage_states, meta) — The completion guard's nudge path now performs up to five uncached network round trips synchronously on the asyncio event loop: _ls_remote_heads() (10s timeout), _fetch_pr_state (gh), resolve_pr_head_sha, a second _ls_remote_heads() + head resolve inside repair_lane_slug, and the WS3d head fetch at tools/sdlc_next_skill.py:766. tools/pr_head_resolver.py is deliberately cache-immune, so none are memoized. What it buys is next_skill, consumed only by _reroute_message for nudge prose — it never changes decision.allow. The method's own docstring two lines below boasts of "avoiding an unnecessary gh call on every other completion". Fix: either restrict the nudge's context to the cheap non-network facts, or await asyncio.to_thread(...) it under a short deadline and fall back to next_skill=None, which _reroute_message already handles.

  • tools/sdlc_next_skill.py:581repair_lane_slug(issue_number, target_repo=repo) — A read-intent path acquired a persistent write. Reached from runner.py:1415, this can rewrite the lane's recorded slug — the value naming the worktree, the branch, and the task list — from a caller whose result is discarded. The write's mechanics are well defended (slug lock, re-read-and-compare under the lock, unique-match-only gate, evidence filed, non-fatal call site), so this is placement, not correctness. The plan calls the slug's ownership change "the single most consequential change in the plan"; it should not be reachable from nudge-text assembly. Fix: thread an explicit allow_repair: bool = False through _verify_stage_artifacts(_live), set True only from the CLI decide() path.

  • tools/sdlc_next_skill.py:511patch_claimed = patch_marked and bool(lane_branch) and not merge_recorded — Dropping the pr_identifiable conjunct narrows the Post-merge, G8 dispatches /do-build on completed work: artifact verifier resolves PR None once the PR is closed #2757 protection. The justifying comment holds only when meta["pr_number"] reflects the world. The missed shape: PATCH completed, slug recorded, MERGE marker not completed, pr_number unresolved — a merged-and-branch-deleted lane whose PR lookup failed. resolve_branch_truth(lane_branch, pr_number=None) then takes the no-PR path and returns ABSENT, setting stage_artifacts_verified: False and force-dispatching /do-patch on shipped work. Reachability is narrow (needs a gh failure while ls-remote succeeds), which is why this is tech debt. Fix: require a positive read before absent — treat "no recorded pr_number and the PR lookup errored" as indeterminate, or widen the skip to MERGE in SETTLED_STATUSES plus meta.get("pr_state") == "MERGED".

  • CLAUDE.md, principle 9 — "UNEVALUATED may let a build proceed and may never let a lane ship" — Contradicted by three artifacts in this same PR: scripts/validate_build.py:401 (return 1 if (fail_count or unevaluated_count) else 0), docs/sdlc/do-build.md:189 ("it blocks exactly like FAIL"), and docs/features/machine-readable-dod.md:110. The consumer split is real as a design permission, but no build consumer currently exercises it. Relatedly, tests/unit/test_merge_predicate.py:929 test_build_vs_ship_split_lives_on_the_consumer asserts only that the record carries no severity field and never exercises a build-side consumer, so its name overclaims. Fix: amend the CLAUDE.md sentence to match the three implementations, and rename the test to what it asserts.

  • tools/sdlc_session_ensure.py:759if not reuse_run_id: — The lease heartbeat is still gated on reuse_run_id rather than on the provenance primitive this PR introduces. When reuse_run_id is truthy but _validated_reuse_candidate returns None, the call falls back to minted_candidate — a genuinely fresh mint whose heartbeat nobody launched, contradicting the comment directly above it. Pre-existing (identical on main), but this PR builds the primitive that expresses it correctly. Fix: if provenance == _CandidateProvenance.MINTED:.

  • agent/verification_parser.py:598re.match(r"(?:output\s*)?==\s*(\d+)\s*$", expected)output == N (gloss) is anchored out while output > N (gloss) is deliberately kept prefix-matched, though both are the same authoring idiom. Three live rows use it (docs/plans/watch-skill-video-scoping-controls.md:606,607, docs/plans/durability-m1-fence-canary.md:1012) and will grade UNEVALUATED under a gate that blocks. Not a regression — main had no == branch at all — but those rows now block instead of silently failing. The docstring at tests/unit/test_verification_parser.py:294 also cites output == 2 (the two read sites) as appearing "verbatim in a live plan"; that string is not in any live plan. Fix: allow a trailing parenthetical/em-dash gloss uniformly on the numeric comparators, or correct the citation and accept the three rows explicitly.

Nits

  • agent/sdlc_router.py:686elif review_status == STATUS_COMPLETED and review_approved and docs_status != STATUS_COMPLETED: — the docs_status != STATUS_COMPLETED conjunct can never be false here; the preceding if consumed the equal case.
  • tools/sdlc_next_skill.py:733context["branch_exists"] narrowed from "local or remote-tracking substring match" to "on origin". Removing the stale-ref false positive is the point; the unstated half is that a local-only, never-pushed lane branch now reports False and can fall to Blocked(NO_RULE) where it previously got /do-build. Low reachability and now diagnosable via decision_inputs — worth one sentence in docs/features/sdlc-router-decision-reconciliation.md.
  • agent/sdlc_router.py:2490 — the RECONCILE_DEADLOCK branch is unreachable with the production guard list (G3 is the sole proposed_skill consumer, and none of its redirect targets are plan-family, so the second pass always converges). The only test that reaches it replaces GUARDS wholesale. That is the safe direction, but the branch has no production coverage and will rot. Add a docstring line saying it exists for the guard added next, so nobody deletes it as dead or assumes it is battle-tested.
  • agent/sdlc_router.py:2586guard_result = evaluate_guards(...)/return guard_result returns before the unrecorded_dispatch attachment and carries no decision_inputs. G4's Blocked is exactly the one Cluster D names as its motivation ("G4 caps the lane for 'oscillating' — an accurate observation attributed to the wrong cause"), and it is the one that carries no evidence. Attaching the payload once at decide_next_dispatch's return boundary would cover every path.
  • tools/sdlc_session_ensure.py:106_CandidateProvenance is a bare class of string constants while this same PR introduces CheckOutcome(StrEnum) for the same job.
  • tools/sdlc_next_skill.py:249pr_number: object = None, heads: object = _UNSET defeat type checking on parameters used as int and dict.
  • tools/merge_predicate.py:874,895{row.get('name') or '<unnamed>'!r} binds !r to the whole or expression (correct, but reads as if it applies to the literal). Prefer {(row.get('name') or '<unnamed>')!r}.
  • tests/unit/sdlc_session_ensure/test_sdlc_session_ensure_readback_provenance.py:154,185,211,240 — four tests acquire real issue locks and never release them, while TestReadbackByPrimaryKey in the same file does.

Miscellaneous

  • None

Acknowledged Deferrals (verified)

  • "This is one PR where the plan recommended two." — Explicit ask-the-reviewer disclosure, no tracking issue claimed or required. Rationale verified against the plan (Open Question 1, Risk 6): a predecessor build landed both halves in interleaved commits before this session resumed, so splitting means rewriting history rather than sequencing work. Ruled: accepted as one PR.
  • "docs/features/README.md carries a two-row sort fix that is not this lane's work." — Explicit out-of-scope disclosure, no tracking required. Verified in the diff: 5 lines, mechanical, content-free, unblocking the sort validator. Ruled: accepted.
  • Refs #3065, not Closes — verified against the plan's merge-lane note and the Success Criteria header; SDLC control plane routes on asserted facts, not read facts (consolidates 51 self-filed pipeline issues) #3065 remains OPEN and the umbrella's disposition belongs to its owner.

Verification Results

Reproduced independently on the PR head, not credited from the PR body:

$ .venv/bin/python scripts/validate_build.py docs/plans/sdlc-control-plane-asserted-facts.md
Result: 29 PASS, 0 FAIL, 0 UNEVALUATED, 0 SKIP   (exit 0)

All 29 rows pass, including the six documentation-cascade file assertions, the six named pytest suites, the four ANTI scope rows, and both ruff rows. The PR body's narrow-scope test counts (526 + 130 + 181 = 837 passed) were not reproduced in this environment and are recorded as unverified rather than as satisfied.

Note on Risk 7, which the plan raises and this review inherits: the grep rows in this table are graded by the very runner this PR modifies. The code-quality judge independently checked every ordering pair in evaluate_expectation and confirmed the new branches are strictly additive over main's six — every form main recognized, this branch recognizes identically — so the table's greens are not an artifact of the change. The one live-plan breakage is the table-header contract (blocker 2), not the grammar.

Screenshots

Not applicable — no UI files in the diff.

… (Refs #3065)

Both blockers from PR #3123's review, and both were instances of the defect
class this lane exists to remove: a gate structurally incapable of firing.

record_verification_outcomes had zero production callers. Neither runner called
it, so the merge predicate's verification check always took its "no aggregate"
branch and was reported-not-enforced for every lane. scripts/validate_build.py
now carries the write behind --record-outcomes/--repo/--issue/--pr.

REVIEW is the recording stage and BUILD deliberately is not. The record is only
worth reading when stamped with the PR head SHA it was graded against, and BUILD
grades before the lane has a PR; recording there would have written an
unanchored aggregate that the predicate refuses, blocking every lane with a
reason no lane could clear. docs/sdlc/do-pr-review.md invokes the script with
the flag, docs/sdlc/do-build.md without it.

To record what a run graded without running every command twice, this runner now
delegates execution to run_checks and derives its report shape from the results,
rather than carrying a parallel execution loop. That is the convergence the plan
asked for: the two runners can no longer drift on what a check did, only on how
it is printed.

The check-table column contract searched only indices 1 and 2, which rejected
the leading-index shape (| # | Check | Command | Expected |) that live plans
already use — docs/plans/overclaim-guard-greps-whole-worktree.md went from 30
executable checks to 0 checks and 2 malformed rows. The contract now locates an
Expected column immediately after a Command column with at least one column
ahead of it, which still rejects both false positives the docstring names, and
takes the check's name from the column before Command so an index column does
not become the name.

CLAUDE.md's new sentence claimed UNEVALUATED "may let a build proceed", which
contradicted the three artifacts in this branch that say it blocks. Restated as
what is actually true: each consumer owns its disposition, and the build gate
blocks today.
@valorengels

Copy link
Copy Markdown
Collaborator Author

Review: Changes Requested

Round 2, on head 2ed0f61 (round 1 judged 2f09345). Multi-judge consensus (code-quality, risk), rule any-blocker-wins. Both judges ran independently in a provisioned worktree at the PR head; code-quality raised 1 blocker, risk raised 2. Consensus: CHANGES REQUESTED, 3 blockers.

Both round-1 blockers are fixed, and I verified them rather than crediting the commit message.

  • Column contract. I swept all 20 docs/plans/*.md through both parsers — origin/main's and this branch's — and compared (checks, malformed, skipped) per file: 20 files, 0 differences. overclaim-guard-greps-whole-worktree.md is back to 30 checks / 0 malformed / 1 skipped. Both documented false positives stay rejected (check_column_indices(['Command','Observed stdout','Observed exit'])None; ['#','Criterion','Check']None), and the leading-index shape now yields (2, 3) with the name taken from the column ahead of Command. The fix is better than the one round 1 suggested — it is the shape, not an offset.
  • Writer. record_verification_outcomes now has a real caller: scripts/validate_build.py --record-outcomes, invoked from docs/sdlc/do-pr-review.md:72. The stage choice is right and the reasoning in machine-readable-dod.md is the argument I would have made: REVIEW is the first stage with a head to stamp against, so BUILD must not record. The code-quality judge independently checked every seam of the check_verification_tablerun_checks delegation (unstripped output into evaluate_expectation, identical UNEVALUATED reason strings, ordering, cwd) and found no behavioral drift. That convergence is a genuine removal.

The three new blockers are all in the arming of the gate, not in its design.

The staleness worry I brought into this review was wrong, and I want that on the record. I predicted that recording at REVIEW, then letting /do-docs commit on the feature branch, would strand every lane behind VERIFICATION_OUTCOMES_STALE_REASON with a remedy no stage doc names. The risk judge refuted the conclusion while confirming every individual link: agent/sdlc_router.py:2369 row 8f (_rule_review_verdict_head_stale) is ordered before row 9 precisely so a stale approval re-reviews before docs/merge, and the DOCS commit stales the REVIEW verdict and the aggregate together. So the normal path self-heals through one extra REVIEW round — pre-existing #2062 behavior, not new damage. What survives is narrower and is filed below as tech debt: the two can diverge, and when they do nothing routes out.

I ran the plan's ## Verification table myself: 28 PASS, 0 FAIL, 1 UNEVALUATED, exit 1. The one UNEVALUATED is Verdict unit tests pass, timed out at the 120s bound under concurrent load; I timed that same suite at 43s standalone (134 passed), so it is contention, not code. It is also a live demonstration of tech debt 3 below. The PR body's "29 PASS, 0 UNEVALUATED" and its 837-test narrow-scope count were not reproduced and are recorded as unverified, not as satisfied.

One process note for whoever patches this: a uv sync without --extra dev leaves a worktree with no .venv/bin/pytest, scripts/pytest-clean.sh falls through to the PATH pytest, and the branch's tests silently exercise the primary checkout's code. That produced three phantom FAILs on my first pass. The #3033 guard checks for .venv, not for pytest inside it.

Rubric

  • 1. Plan vs. implementation match — fail — Task 8's persistence point now exists, but the writer's entry point mis-parses its own documented invocation (blocker 1), so the gate is armed with a footgun rather than armed.
  • 2. New code quality — fail — _opt is a hand-rolled argv parser that accepts the next flag as a value and crashes after the report. Everything else in the commit is high quality.
  • 3. Test coverage — pass — The new TestLeadingIndexColumnIsACheckTable pins the exact shape round 1 broke, including that the name comes from the column before Command, and grades the table end to end. 149 passed across the two parser/runner suites.
  • 4. Regression risk to existing callers — fail — The round-1 live-plan regression is gone (parity sweep above). New: a merge-refusal condition the router cannot see (blocker 3).
  • 5. Data integrity — pass — _verification_outcomes remains a new key in an already-flexible JSON blob; no migration required.
  • 6. Security — pass — No new eval/exec, no request-derived subprocess input, no secrets. The merge predicate still never executes a plan-authored command.
  • 7. Documentation accuracy — fail — CLAUDE.md principle 9 is corrected and now matches the three artifacts. But do-docs.md, do-merge.md, and merge-troubleshooting.md carry zero mentions of verification outcomes, so neither new refusal has a documented recovery at the stage that hits it.
  • 8. PR body accuracy — fail — "29 PASS, 0 FAIL, 0 UNEVALUATED" does not reproduce on this head (28/0/1). The writer claim is now true; this one is stale.
  • 9. Disclosed deferrals — pass — Both round-1 disclosures were ruled on and accepted; nothing new was disclosed.
  • 10. Follow-up claims verified — n/a — The PR body makes no "filed as follow-up #N" claim.

Pre-Verdict Checklist

  • 1. All plan acceptance/success criteria validated against diff — FAIL — The Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 machine-readable-gate criterion is now implemented but not safely reachable through its own documented invocation.
  • 2. No-Gos from plan — none violated — PASS — All four ANTI rows green: hooks validators untouched, pyproject.toml/uv.lock untouched, no raw Redis on Popoto keys, no Claude co-authorship trailer. Refs #3065, not Closes.
  • 3. New except Exception blocks — each has logger/raise/swallow-ok — FAIL — agent/verification_parser.py:967 logs and returns None, but its fail-open direction is wrong for its one consumer (blocker 2).
  • 4. New integration tests — exercise serialization boundary (not in-memory only) — PASS — test_merge_predicate.py's gate_lane writes through a real PipelineLedger and reads back through the real predicate.
  • 5. Plan internal consistency — spike findings match task steps — PASS
  • 6. No hardcoded secrets or debug artifacts — PASS
  • 7. New public APIs — docstrings present — PASS — check_column_indices's docstring records both false positives it must keep rejecting and why the previous pinning over-corrected. That is the right thing to have written down.
  • 8. Breaking changes — migration path documented — PASS — passed: boolCheckOutcome remains a clean removal; the reader sweep is an ANTI row and both skill docs are updated.
  • 9. Tests added for new behavior — PASS
  • 10. Tests cover the failure path (not just happy path) — FAIL — No test covers a malformed --issue/--repo/--pr value, which is exactly how the documented invocation fails (blocker 1).
  • 11. UI changes (if any) — screenshot captured — N/A — No HTML/CSS/JS/TSX/Vue/template files in the diff; the visual-proof gate is a no-op.
  • 12. Docs updated for user-facing changes — FAIL — See rubric 7: no MERGE-stage recovery documentation for either new refusal.

Blockers

  • scripts/validate_build.py:336def _opt(flag): ... if i + 1 < len(argv): return argv[i + 1]The writer's argv parser accepts the next flag as a value, and its three failure modes all defeat the gate this commit exists to arm. The documented production invocation is docs/sdlc/do-pr-review.md:72, with unquoted shell variables and no set -u. I reproduced all three at the PR head:

    • Empty $ISSUE_NUMBER--issue --pr 77. opt_issue == "--pr" clears the if not opt_repo or not opt_issue guard, then int(opt_issue) at line 406 raises ValueError: invalid literal for int() with base 10: '--pr' after the summary is printed, and the traceback changes the exit code. That directly contradicts the block's own comment at line 396: "never let a ledger failure change what the human is told ... does not touch the exit code, which belongs to the checks."
    • Empty $TARGET_REPO--repo --issue 4242. The write succeeds and prints RECORD: verification outcomes written for --issue#4242 (UNANCHORED). A real PipelineLedger row was created under repo --issue. (One judge did this for real against the shared Redis; I swept the row through the ORM.) A gate that records under a nonexistent lane and reports success is worse than one that never fires.
    • plan_path = Path(argv[0]) at line 348: validate_build.py --record-outcomes plan.md prints Plan file not found: --record-outcomes / Nothing to validate. and returns 0 — a green exit having executed zero checks.

    Fix: use argparse. Failing that, reject any _opt value beginning with --, take plan_path from the first non-flag positional, and wrap the int() conversions so a bad value prints a RECORD: skipped line without touching the exit code. Add a failure-path test for each of the three; there is currently none. Also quote the variables in docs/sdlc/do-pr-review.md:72.

  • agent/verification_parser.py:967except Exception as exc: logger.debug(...); return NoneThe one read that fetches the blocking evidence is the one that fails open, and the predicate cannot tell an error from an absence. The docstring at line 939 states the intent plainly: "fails OPEN to None on any error or malformed blob". tools/merge_predicate.py:826 then does if aggregate is None: notes.append("... no recorded aggregate ... (reported, not enforced)"); return. So a Redis blip, a corrupt stage_states_json, or an import failure at merge time silently converts a recorded aggregate carrying a FAIL row into a non-enforced pass. This is inverted against every neighbour in the same module: group (c) at tools/merge_predicate.py:581 does except Exception as exc: failed.append(f"REVIEW verdict unavailable ..."), the module docstring at line 35 says group (d) is "closed on a substrate-present import failure", and group (e)'s own anchor path is fail-closed twice (lines 838 and 848). Fix: have the reader distinguish absent from errored — raise, or return a sentinel — and have _check_verification_outcomes failed.append on the error branch while keeping the genuine-absence branch as a note.

  • agent/sdlc_router.pygrep -c verification_outcomes agent/sdlc_router.py tools/sdlc_next_skill.py0 and 0Group (e) is a merge-refusal condition no dispatch rule can see, so a divergence between the verdict and the aggregate is a hard wedge rather than a re-route. The self-healing path the risk judge established relies on the REVIEW verdict and the aggregate going stale together: row 8f (agent/sdlc_router.py:2369, _rule_review_verdict_head_stale) sees the verdict and re-dispatches /do-pr-review. Nothing sees the aggregate. When the two diverge — a REVIEW re-run that records a verdict but skips § 4.5, an UNEVALUATED row from a timeout, a RECORD: FAILED line — group (c) passes, row 8f is inert, _rule_ready_to_merge (line 2156) fires, /do-merge is dispatched, and tools/merge_predicate.py:869 refuses with verification row '...' is UNEVALUATED — FAIL and UNEVALUATED both hold the PR. The router re-dispatches /do-merge unchanged until guard_g4_oscillation (line 700) blocks the lane for human intervention. That is the router↔predicate oscillation loop row 8f's own docstring (line 2122) says WS3d/Reliability risk: SDLC router — REVIEW=completed with no recorded verdict falls through rows 8c/8d/9 and misroutes to row 10 /do-merge #2062 existed to end, reintroduced on a new axis. Fix: give group (e) a router-side twin — a rule that reads _verification_outcomes, and on a stale-or-blocking aggregate routes to /do-pr-review (which re-records) instead of dead-ending at /do-merge. This lane is the one that gets to demonstrate the bug on itself if its own re-review records a verdict without re-running § 4.5.

Tech Debt

  • tests/unit/test_validate_build.py:615class TestRecordOutcomesHasAProductionCaller: — Every test in the class patches validate_build.record_verification_outcomes and asserts on main()'s call, which proves the flag path works, not that anything invokes the flag. The production caller is one markdown line (docs/sdlc/do-pr-review.md:72); grep -rn -- "--record-outcomes" tests/ finds hits only inside this file. Delete the flag from the doc and the suite stays green while the merge predicate returns to its always-aggregate is None branch — the exact regression the class docstring claims to catch. Fix: add a doc-pinning assertion; tests/unit/test_do_pr_review_tick_writes.py and tests/unit/test_do_pr_review_identity.py already establish the pattern in this repo.

  • docs/sdlc/do-merge.md, docs/sdlc/merge-troubleshooting.md, docs/sdlc/do-docs.mdgrep -n verification over all three (and the two global skill bodies) returns zero hits — An agent that hits verification outcomes: verification outcome predates PR head commit or verification row 'X' is UNEVALUATED at MERGE has no documented recovery. The remedy exists in exactly two places the MERGE-stage agent never reads: docs/sdlc/do-pr-review.md:79 and docs/features/machine-readable-dod.md:143. Given blocker 3 makes the divergent case a G4 wedge rather than a self-healing loop, one paragraph in merge-troubleshooting.md naming the re-record command is cheap insurance. Fix: name the refusal and the --record-outcomes re-run in merge-troubleshooting.md.

  • agent/verification_parser.py:113DEFAULT_TIMEOUT_S = 120 — The bound has no lever, and the disposition it feeds just became durably merge-blocking. Its own comment says "raise it if a legitimate suite starts brushing the ceiling rather than letting rows go UNEVALUATED" — but there is no env override, and main() at scripts/validate_build.py:370 calls check_verification_table(verification_table, check_results=graded) without the timeout parameter the function accepts, while _opt recognises only --repo/--issue/--pr. So the only production entry point cannot reach it. Measured on a quiet machine at this head, the worst plan row (test_sdlc_verdict.py, 33s) sits at 28% of the bound; a 3.6x contention slowdown flips it, which is exactly what happened to me. Note the direction is not all bad — the removed 30s/SKIP ceiling would have timed that row out on every run — but 30s→SKIP was non-blocking and 120s→UNEVALUATED is a durable merge refusal. This also violates the repo's provisional-magic-number rule (named, env-overridable). Fix: VERIFICATION_TIMEOUT_S env override plus a --timeout flag wired into the existing parameter, roughly six lines. Do not just raise the constant — that treats contention as a bound problem.

  • tools/merge_predicate.py:826if aggregate is None: notes.append("... reported, not enforced"); return — A lane where § 4.5 is simply never run merges unimpeded, and nothing mechanically enforces the invocation: it is prose in docs/sdlc/do-pr-review.md:72, and the global skill body delegates it softly ("a declared verification-table runner overrides"). The group (e) docstring at line 740 justifies its own existence as "That ruling lived only in plan prose, so PR Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 merged past it" — and the new gate's trigger condition now lives only in skill prose. I accept the deliberate choice documented at lines 753-760 ("a new fail-closed behavior here has no incident backing it"), which is why this is not a blocker. Fix: name it in the plan as a known open edge rather than presenting the Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 ruling as closed.

  • agent/session_runner/runner.py:1415 (carried from round 1, unchanged)context = build_decision_context(issue_number, stage_states, meta) — Up to five uncached network round trips run synchronously on the asyncio event loop. I traced the reachability this round: async def run (line 775) → _route_turn (1504) → _guard_completion (1455) → _load_ledger (1384). What it buys is next_skill, consumed only for nudge prose; it never changes decision.allow. Fix: restrict the nudge's context to the cheap non-network facts, or await asyncio.to_thread(...) under a short deadline and fall back to next_skill=None, which _reroute_message already handles.

  • tools/sdlc_next_skill.py:583 (carried, unchanged)repair_lane_slug(issue_number, target_repo=repo) — A read-intent path reachable from nudge-text assembly acquires a persistent write to the value naming the worktree, the branch, and the task list. The write's mechanics are well defended; this is placement, not correctness. Fix: thread allow_repair: bool = False through _verify_stage_artifacts(_live), set True only from the CLI decide() path.

  • tools/sdlc_next_skill.py:511 (carried, unchanged)patch_claimed = patch_marked and bool(lane_branch) and not merge_recorded — Dropping the pr_identifiable conjunct narrows the Post-merge, G8 dispatches /do-build on completed work: artifact verifier resolves PR None once the PR is closed #2757 protection for a merged-and-branch-deleted lane whose PR lookup failed. Fix: treat "no recorded pr_number and the PR lookup errored" as indeterminate rather than absent.

  • tools/sdlc_session_ensure.py:764 (carried, unchanged)if not reuse_run_id: — The lease heartbeat is still gated on reuse_run_id rather than on the _CandidateProvenance primitive this PR introduces, so a truthy reuse_run_id whose candidate failed validation falls back to a genuinely fresh mint with no heartbeat. Pre-existing on main, but this PR builds the primitive that expresses it correctly. Fix: if provenance == _CandidateProvenance.MINTED:.

  • agent/verification_parser.py:618 (carried, unchanged)re.match(r"(?:output\s*)?==\s*(\d+)\s*$", expected)output == N (gloss) is anchored out while output > N (gloss) two branches above is deliberately prefix-matched, though both are the same authoring idiom; three live rows use it. Under a gate that now blocks, those rows go UNEVALUATED. Fix: allow a trailing gloss uniformly on the numeric comparators, or accept the three rows explicitly.

  • tests/unit/test_verification_parser.py:561 (carried, unchanged)"""One plan in docs/plans/ carries a 4-column Verification table.""" — I re-swept every docs/plans/*.md check-table header on this head; none has a fourth column. The claim is still false, and the test still uses Check | Command | Expected | Notes while the real 4-column shape puts the extra column first. The real shape is now covered in test_validate_build.py, so this is a stale docstring rather than a coverage hole. Fix: correct the docstring, or move the assertion to the shape that exists.

Nits

  • agent/verification_parser.py:404command_idx, expected_idx = check_column_indices(header_cells) unpacks a tuple[int, int] | None with no None check. Safe at runtime (check_blocks was filtered through _is_check_table_header) but it is a type error at the seam and the invariant is implicit.
  • agent/sdlc_router.py:686 (carried) — the docs_status != STATUS_COMPLETED conjunct can never be false; the preceding if consumed the equal case.
  • agent/sdlc_router.py:2490 (carried) — the RECONCILE_DEADLOCK branch has no production coverage; add a docstring line saying it exists for the guard added next.
  • agent/sdlc_router.py:2586 (carried) — G4's Blocked returns before the decision_inputs attachment, so the one blocked verdict Cluster D names as its motivation is the one carrying no evidence.
  • tools/sdlc_next_skill.py:733 (carried)branch_exists narrowed to "on origin"; worth one sentence in docs/features/sdlc-router-decision-reconciliation.md.
  • tools/sdlc_session_ensure.py:106 (carried)_CandidateProvenance is a bare class of string constants while this same PR introduces CheckOutcome(StrEnum) for the same job.
  • tools/sdlc_next_skill.py:249 (carried)pr_number: object = None, heads: object = _UNSET defeat type checking on parameters used as int and dict.
  • tools/merge_predicate.py:874,895 (carried) — prefer {(row.get('name') or '<unnamed>')!r}.
  • tests/unit/sdlc_session_ensure/test_sdlc_session_ensure_readback_provenance.py:154,185,211,240 (carried) — four tests acquire real issue locks and never release them.

Miscellaneous

  • None

Acknowledged Deferrals (verified)

Review Delta (vs prior review on HEAD 2f09345)

  • ResolvedBlocker: record_verification_outcomes has no production caller. scripts/validate_build.py --record-outcomes is that caller, wired from docs/sdlc/do-pr-review.md:72, and the REVIEW-not-BUILD stage choice correctly answers the unanchored-record hazard round 1 raised.
  • ResolvedBlocker: the positional column contract breaks a live plan's verification gate. Verified by a 20-plan parity sweep against origin/main: zero differences.
  • ResolvedTech debt: CLAUDE.md principle 9 contradicts three artifacts. Restated to "each consumer owns its disposition; the build gate blocks today", which matches validate_build.py:401, do-build.md:189, and machine-readable-dod.md:110.
  • Partly resolvedTech debt: the 4-column header test does not test the shape it names. The real shape is now covered by TestLeadingIndexColumnIsACheckTable in test_validate_build.py; the misleading docstring at test_verification_parser.py:561 survives and is carried forward.
  • New — Blocker 1 (_opt argv parsing), blocker 2 (read_verification_outcomes fails open), blocker 3 (router has no view of _verification_outcomes), and tech debt 1-4. All four new tech-debt items are consequences of the gate going live; none existed while it was inert.
  • Unchanged — Tech debt 5-10 and all carried nits.
  • Withdrawn by me — the round-2 hypothesis that the REVIEW→DOCS→MERGE path strands every lane on a stale aggregate. Row 8f re-reviews on a stale verdict before merge, and the verdict and the aggregate stale together, so the normal path self-heals. Only the divergent case survives, as blocker 3.

Verification Results

Reproduced on the PR head in a provisioned worktree, not credited from the PR body:

$ .venv/bin/python scripts/validate_build.py docs/plans/sdlc-control-plane-asserted-facts.md
Result: 28 PASS, 0 FAIL, 1 UNEVALUATED, 0 SKIP   (exit 1)

UNEVALUATED: Verdict unit tests pass -- command timed out after 120s, so it never
produced a result to grade (this is not evidence that the check failed)

The one UNEVALUATED row is contention, not code: scripts/pytest-clean.sh tests/unit/test_sdlc_verdict.py -q -n 2 completes in 43s standalone with 134 passed. All four ANTI scope rows, both ruff rows, the six documentation-cascade file assertions, and the other five pytest-suite rows are green. The PR body's "29 PASS, 0 FAIL, 0 UNEVALUATED" and its 837-test narrow-scope count were not reproduced and are recorded as unverified rather than as satisfied.

Screenshots

Not applicable — no UI files in the diff.

… twin (Refs #3065)

Round-2 review returned three blockers, all in the arming of the gate
rather than its design.

The writer's argv parser accepted a flag as a value. The documented
invocation interpolates shell variables, so an empty one collapsed the
argument list and the naive reading took the next flag: an empty issue
number raised ValueError after the summary had printed and rewrote the
exit code the checks own, an empty repo wrote a real ledger row under a
repo named "--issue" and reported success, and a flag ahead of the plan
path exited 0 having run no checks at all. _parse_argv now rejects a
value that is itself a flag, takes the plan path from the first
non-flag positional, and reports an unparseable --issue/--pr instead of
raising through the report. All three modes have tests.

read_verification_outcomes failed open. It was the one read in the merge
path that fetched the blocking evidence, and the predicate could not
tell an error from an absence, so a store blip converted a recorded FAIL
into an unenforced pass -- inverted against every neighbouring group.
Absence still returns None; anything that prevents an answer now raises
VerificationOutcomesUnavailableError and the predicate refuses on it.

Group (e) was a merge refusal no dispatch rule could see. Row 8g is its
routing-side twin: a blocking or unfresh aggregate routes to
/do-pr-review, which re-records it, instead of dead-ending on a merge
the predicate is certain to refuse until G4 blocks the lane. Its
dispositions mirror the predicate's so the two cannot drift, and a test
asserts that agreement across the whole CheckOutcome enum.

Also from the review, in blast radius: the per-check bound gets a lever
(VERIFICATION_TIMEOUT_S, --timeout) now that a timeout is a durable
merge refusal rather than a non-blocking skip; merge-troubleshooting.md
documents both new refusals and their recovery; the REVIEW invocation is
pinned by test rather than by prose alone; the trailing-gloss rule is
symmetric across >, >= and == , which unblocks three live rows in other
plans; G4's block carries the evidence Cluster D exists to attach; and
four tests stop leaking real issue locks into the shared Redis.

Plan verification table: 29 PASS, 0 FAIL, 0 UNEVALUATED.
@valorengels

Copy link
Copy Markdown
Collaborator Author

Review: Changes Requested

Round 3, on head 76e8d30 (round 2 judged 2ed0f61, round 1 2f09345). Multi-judge consensus (code-quality, risk), rule any-blocker-wins. Both judges ran in the foreground against this head in a provisioned worktree; code-quality returned APPROVED with 2 tech-debt items, risk returned CHANGES REQUESTED with 3 blockers. I verified every blocker myself before crediting it, and dropped nothing. Consensus: CHANGES REQUESTED, 3 blockers.

Two of round 2's three blockers are genuinely fixed, and I reproduced all three failure modes rather than crediting the commit message.

  • Blocker 1 (_opt argv parser). _parse_argv is the right shape and all three reproduced modes now behave as the PR body claims. --issue --pr 77ARGS: ignoring --issue, no ValueError, exit unchanged. --repo --issue 4242ARGS: ignoring --repo, no ledger row. --record-outcomes with no positional → No plan path given, exit 1 (I checked the exit code directly, not through a pipe). docs/sdlc/do-pr-review.md:72 is quoted.
  • Blocker 2 (read_verification_outcomes fails open). Verified by injecting a ConnectionError into PipelineLedger.get: the reader now raises VerificationOutcomesUnavailableError instead of returning None, and _check_verification_outcomes failed.appends on that branch. Genuine absence still returns None. The direction now matches every neighbouring group in the module.

I also re-verified the round-2 tech debt marked fixed: TD 1's doc-pinning assertions really do read docs/sdlc/do-pr-review.md and would fail if the flag were deleted (test_review_addendum_invokes_the_recording_flag); TD 2's merge-troubleshooting.md section exists and names all four refusal strings; TD 3's lever works; TD 4's ### Risk 8 is written honestly; TD 9's gloss rule is symmetric (I graded output > 0 (gloss), output == 2 (gloss), output >= 1 (gloss) → PASS and bare == 2 (gloss) → UNEVALUATED, as documented); TD 10's docstring is corrected. Nits 1, 2, 3, 4, 8, 9 are all closed. The code-quality judge ran the differential you would want here: 88 distinct Expected cells from every docs/plans/*.md, graded under both the old and new evaluate_expectation across 8 (exit, output) samples — 24 differences, every one UNEVALUATED → graded, and zero PASS↔FAIL flips. The grammar rework introduces no silent misgrading on live plans.

I ran the plan's ## Verification table myself: 29 PASS, 0 FAIL, 0 UNEVALUATED, 0 SKIP, exit 0. The PR body's number reproduces on this head, and the --timeout lever is a real fix for round 2's contention row, not a raised constant. Narrow suites: test_sdlc_router.py, test_verification_parser.py, test_validate_build.py, test_merge_predicate.py, tests/unit/sdlc_session_ensure/, test_sdlc_router_reconciliation.py, test_sdlc_next_skill.py, test_lane_identity.py = 626 passed. ruff check . and ruff format --check . clean.

The blocker that matters most is blocker 3 again, and it is the same defect with the fix in place. Row 8g is well written and its dispositions are right. It is also structurally inert in production, for the same reason the code block two files over already documents about _verdicts. That is not a design objection; it is one line of threading.

The other two blockers are blast radius. This gate arms for every lane in the repo on merge, and I verified two populations that go from mergeable to refused with no self-healing route.

Rubric

  • 1. Plan vs. implementation match — fail — Task 8's routing-side twin exists and is correct in isolation, but never fires on the payload the router is actually handed (blocker 1).
  • 2. New code quality — pass — _parse_argv, the tri-state reader, and row 8g are all well constructed and well commented. The argv rewrite is exactly what round 2 asked for.
  • 3. Test coverage — fail — Every 8g test hand-builds a stage_states dict with _verification_outcomes injected. None goes through query_enriched, which is why an inert rule has a green suite (blocker 1). This is the same class TD 1 fixed for the writer and left open for the reader.
  • 4. Regression risk to existing callers — fail — 4 of 20 live plans and 2 open PRs are newly refused at MERGE (blockers 2 and 3).
  • 5. Data integrity — pass — aggregate is computed once outside write_outcomes, so the retried update_fn is deterministic and there is no lost-update hazard against _verdicts / _sdlc_dispatches / _run_identities in the shared blob.
  • 6. Security — pass — No new eval/exec, no request-derived subprocess input, no secrets. The merge predicate still never executes a plan-authored command.
  • 7. Documentation accuracy — fail — merge-troubleshooting.md now carries the remedy, but docs/sdlc/do-merge.md has zero mentions of verification or of that file, so the MERGE-stage agent still cannot reach it.
  • 8. PR body accuracy — pass — 29/0/0 exit 0 reproduces. The 455-test narrow-scope claim is consistent with the 626 I measured over a wider set. The round-2 "unverified" record is correctly retracted.
  • 9. Disclosed deferrals — pass — TD 5-8 and nits 5-7 are deferred with per-item reasons in the body; all were accepted in round 2 and are unchanged.
  • 10. Follow-up claims verified — n/a — No "filed as follow-up #N" claim; the body offers to file one.

Pre-Verdict Checklist

  • 1. All plan acceptance/success criteria validated against diff — FAIL — The routing-side half of the Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 gate is implemented but unreachable.
  • 2. No-Gos from plan — none violated — PASS — All four ANTI rows green in my own run: hooks validators untouched, pyproject.toml/uv.lock untouched, no raw Redis on Popoto keys, no Claude co-authorship trailer. Refs #3065, not Closes.
  • 3. New except Exception blocks — each has logger/raise/swallow-ok — PASS — verification_parser.py:1021 now logs and re-raises as VerificationOutcomesUnavailableError; that was round 2's blocker 2 and it is closed.
  • 4. New integration tests — exercise serialization boundary — FAIL — For the writer, yes. For the reader, no: the one seam that matters (query_enriched → router) has no test, and it is the seam that is broken.
  • 5. Plan internal consistency — spike findings match task steps — FAIL — ### Risk 8 argues fail-closed-on-absence was rejected because it "would block every lane whose plan predates this mechanism". The recorder converts that absence into a recorded UNEVALUATED for those same plans, producing the outcome the risk says it avoided (blocker 2).
  • 6. No hardcoded secrets or debug artifacts — PASS
  • 7. New public APIs — docstrings present — PASS — _parse_argv and VerificationOutcomesUnavailableError both document the incident that motivated them.
  • 8. Breaking changes — migration path documented — PASS
  • 9. Tests added for new behavior — PASS
  • 10. Tests cover the failure path (not just happy path) — PASS — The three argv failure modes round 2 asked for are each pinned now.
  • 11. UI changes (if any) — screenshot captured — N/A — No HTML/CSS/JS/TSX/Vue/template files in the diff; the visual-proof gate is a no-op.
  • 12. Docs updated for user-facing changes — FAIL — See rubric 7.

Blockers

  • tools/sdlc_stage_query.py:1001-1013stages = {k: v for k, v in raw_states.items() if k in ALL_STAGES} then for _router_key in ("_verdicts", "_sdlc_dispatches"):Row 8g never sees _verification_outcomes, so blocker 3 from round 2 is not actually fixed; the wedge it describes is still there. Both router callers take their stage_states from query_enriched()["stages"] — the CLI at tools/sdlc_next_skill.py:996 (stage_states = enriched.get("stages") or {}) and the in-process runner at agent/session_runner/runner.py:1403. That dict is filtered to ALL_STAGES and then has exactly two underscore keys threaded back in. _verification_outcomes is not one of them, and VERIFICATION_OUTCOMES_KEY in ALL_STAGES is False. Verified against the real function rather than by reading:

    $ python -c "... patch _load_raw_states to return a blob carrying _verification_outcomes ...
                 out = query_enriched(session_id='x')"
    stages keys: ['DOCS', 'MERGE', 'REVIEW', '_verdicts']
    aggregate survived query_enriched: False
    row 8g fires on the REAL payload:       False
    row 8g fires on a HAND-BUILT payload:   True
    

    So _rule_verification_outcomes_hold_pr returns False for every production lane, _rule_ready_to_merge fires, /do-merge is dispatched, tools/merge_predicate.py group (e) refuses — it reads the ledger directly via read_verification_outcomes, so it sees the aggregate — and the router re-dispatches /do-merge unchanged until guard_g4_oscillation blocks the lane for a human. That is precisely the oscillation loop row 8g was written to prevent.

    The comment at line 1005-1010 already states this failure mode in general terms for _verdicts and _sdlc_dispatches ("Without them here, those rules are structurally inert in the CLI path"). The new rule needed the same treatment and did not get it.

    Every test in TestVerificationOutcomes... (tests/unit/test_sdlc_router.py:~1985-2082) builds its stage_states by hand with the key injected, which is why 626 tests pass over an inert rule. This is structurally the same gap TD 1 closed on the writer side — and the fix should be the same shape: a test that goes through query_enriched, not around it.

    Fix: add _verification_outcomes (i.e. VERIFICATION_OUTCOMES_KEY) to the threading tuple at tools/sdlc_stage_query.py:1011, and add one test that drives row 8g from a query_enriched payload so deleting it from that tuple fails a test.

  • scripts/validate_build.py:463 + agent/verification_parser.py:896 — the if record_outcomes: block is unconditional, and aggregate_outcomes([], table) returns UNEVALUATEDA plan with no ## Verification table records a 0-row UNEVALUATED aggregate, which the merge predicate then refuses permanently. 4 of the 20 live plans in docs/plans/ are in that shape. Verified in the worktree:

    total plans: 20
    plans with zero executable checks: 4
      blue-sky-fog-planning.md, module-scope-env-reads-migration.md,
      resilience-simplification-three-tier.md, session-recovery-observation-audit.md
    aggregate of empty run: {'outcome': 'UNEVALUATED', 'counts': {...0,0,0}, 'malformed': 0, 'rows': []}
    

    scripts/validate_build.py:431 skips check_verification_table when the plan has no table, so graded stays [], but the recorder writes anyway. At MERGE that lands on tools/merge_predicate.py:900-908: verification outcomes: recorded outcome is 'UNEVALUATED', not PASS (0 row(s) recorded). The code comment there is aware of the case — "a run with no checks at all, which grades UNEVALUATED rather than a vacuous PASS. Refuse rather than guess what it meant" — but it was reasoned about as a defensive branch, not as the state a fifth of the repo's plans will actually be in.

    This contradicts the plan's own ### Risk 8, which rejects failing closed on absence precisely because it "would block every lane whose plan predates this mechanism, none of which ever recorded an aggregate". Recording an UNEVALUATED for those plans reaches the same destination by a different road, and there is no self-heal: row 8g routes to /do-pr-review, which re-records the identical aggregate, terminating only at G4.

    Fix: either do not record when the plan carries no check table (leave the deliberately-unenforced absence case as absence), or treat 0 checks and 0 malformed as non-blocking in the predicate. The first is closer to Risk 8's stated intent.

  • agent/verification_parser.py::evaluate_expectationTwo currently-mergeable open PRs become refused the moment this lands, on expectation forms the grammar rework did not cover. Round 2's TD 9 widened the grammar specifically to stop a blocking gate from holding live plans; the sweep stopped one form short. Verified by grading the live plan files at this head:

    promise-gate-recorded-obligations.md          19 checks, 0 malformed
        ungradeable: 'output `0`', 'output `0`'
    unblock-dependency-bumps-coupled-set-gate.md  43 checks, 0 malformed
        ungradeable: 'exit 0, JSON `"compatible": true`', 'output is 0'
    

    Those two plans track Redefine the promise gate: key on recorded obligations, not grammar — and run it on the path the PM actually uses #3027 (open PR Key the promise gate on recorded obligations, and run it on the path the PM actually uses #3093) and Unblock dependency bumps (anthropic/pydantic-ai), restore the dead worker_key regression guard, and stop duplicate nightly triage filing #3001 (open PR Coupled-set LLM dependency compatibility gate (Step 1 of #3001) #3089) — both lanes are in flight right now. Each ungradeable cell grades UNEVALUATED, which under the Render /ask-me questions as native Telegram polls, and stop nudging a session past its own question #3080 ruling holds the PR. The code-quality judge's 88-cell differential confirms these are the residue: the rework moved 24 cells from UNEVALUATED to graded and changed no verdicts, but output `N` and output is N were not among them.

    Fix: accept the backticked-integer form (output `N` ) and output is N in the numeric branches, and pin all four cells above as regression cases the way TD 9's three were pinned. Alternatively, coordinate the Expected-cell edits on main before this merges — but that is cross-lane work this PR would be imposing, and two more grammar branches are cheaper.

Tech Debt

  • docs/sdlc/do-merge.mdgrep -c "merge-troubleshooting\|verification" docs/sdlc/do-merge.md0 — TD 2 put the recovery section in docs/sdlc/merge-troubleshooting.md:102 ("Verification Outcomes Hold the PR"), and it is a good section — it names all four refusal strings and the --record-outcomes re-run. But the MERGE-stage agent reads docs/sdlc/do-merge.md, which references neither the topic nor the file. The only live cross-reference to merge-troubleshooting.md anywhere in docs/sdlc/, .claude/, tools/, agent/, or scripts/ is docs/sdlc/do-build.md:69, and that one is about validate_build exit codes. So the remedy is reachable only by lucky grep, from the one stage that needs it. Fix: one line in docs/sdlc/do-merge.md pointing at the section.

  • scripts/validate_build.py:415plan_text = plan_path.read_text() — An empty "$PLAN_PATH" produces a raw traceback from the invocation this patch's own comment prescribes. Path("") is Path(".") and Path(".").exists() is True, so the if not plan_path.exists() guard at line ~410 is bypassed. Reproduced:

    $ python scripts/validate_build.py "" --record-outcomes
    IsADirectoryError: [Errno 21] Is a directory: '.'
    

    It exits non-zero rather than false-passing, so it blocks rather than merges — but it is the same class _parse_argv's docstring says it exists to close, surfacing at the one seam the quoting advice cannot cover. Fix: plan_path.is_file(), or reject a blank positional in _parse_argv.

  • scripts/validate_build.py:~404 and agent/verification_parser.py:126--timeout and VERIFICATION_TIMEOUT_S validate integer-ness but not positivity, and 0 or a negative value silently converts every row to UNEVALUATED — now a durable merge refusal. -5 is not ---prefixed, so _parse_argv accepts it as a value. Reproduced at this head:

    $ python scripts/validate_build.py docs/plans/sdlc-control-plane-asserted-facts.md --timeout 0
    Result: 6 PASS, 0 FAIL, 23 UNEVALUATED, 0 SKIP   (exit 1)
    

    Because --record-outcomes writes regardless of exit code, a VERIFICATION_TIMEOUT_S=0 in a launchd env would persist an all-UNEVALUATED aggregate that holds the lane. Fix: reject <= 0 at both entry points.

  • agent/verification_parser.py:942-960 — A transient resolve_pr_head_sha failure at RECORD time writes an aggregate with no head_sha, and tools/merge_predicate.py:846 then refuses it with "no usable head_sha ... freshness is indeterminate". The record does not distinguish "never anchored" from "anchor attempt failed", so a gh outage during REVIEW produces a durable refusal whose only escape is a manual re-run — and with row 8g inert (blocker 1), the lane dead-ends at G4 rather than re-reviewing. Fixing blocker 1 downgrades this considerably. Fix: stamp the failed-anchor case distinctly, or have the recorder decline to write an unanchored record at REVIEW.

  • docs/sdlc/merge-troubleshooting.md:124 vs docs/sdlc/do-docs.md:14 — The documented remedy for an UNEVALUATED row is "fix the row", but do-docs.md:14 mandates that plan files stay on main and never appear in a feature-branch PR. So the Expected-cell edit lands on main while docs/sdlc/do-pr-review.md:72 grades "$PLAN_PATH" in the lane worktree, which keeps grading the old cell until main is merged into the branch. Neither doc names that step. Fix: one sentence in the troubleshooting section.

  • agent/session_runner/runner.py:1415 (carried, unchanged, disclosed) — up to five uncached network round trips on the asyncio event loop. Accepted as deferred in round 2; unchanged here.

  • tools/sdlc_next_skill.py:583 (carried, unchanged, disclosed)repair_lane_slug reachable from a read-intent path. Accepted as deferred.

  • tools/sdlc_next_skill.py:511 (carried, unchanged, disclosed)patch_claimed dropped the pr_identifiable conjunct. Accepted as deferred.

  • tools/sdlc_session_ensure.py:764 (carried, unchanged, disclosed) — lease heartbeat gated on reuse_run_id rather than _CandidateProvenance. Accepted as deferred.

Nits

  • agent/verification_parser.py:~1017except VerificationOutcomesUnavailableError: raise is unreachable: nothing inside that try raises it (both raise sites are below the block). Harmless future-proofing, but it reads as a live branch.
  • tests/unit/test_sdlc_router.py:2070test_router_and_predicate_agree_on_the_blocking_set never imports tools.merge_predicate; it re-spells {FAIL, UNEVALUATED} locally and compares the router to itself. A drift in the predicate would not fail it. Import the predicate's own notion of the blocking set, or the name overstates what it pins.
  • agent/sdlc_router.py:2180 — the docstring says "The dispositions mirror the predicate's, so the two cannot disagree", but _check_verification_outcomes also refuses on rows not being a list, a non-dict row, and malformed > 0, none of which 8g inspects. Unreachable from the sanctioned writer, so it is a docstring overclaim rather than a live divergence.
  • scripts/validate_build.py:_parse_argv — accepts single-dash tokens as flag values (--repo -x{'--repo': '-x'}) despite the docstring saying a value that "is itself a flag" is rejected; --issue -5 yields a negative issue number.
  • scripts/validate_build.py:_parse_argv — silently last-wins on a repeated flag and silently drops extra positionals.
  • scripts/validate_build.py--help is honored only at sys.argv[1]; validate_build.py --record-outcomes --help prints ARGS: ignoring --help.
  • tools/sdlc_next_skill.py:733, tools/sdlc_session_ensure.py:106, tools/sdlc_next_skill.py:249 (carried nits 5, 6, 7) — deferred with stated reasons in round 2; unchanged and still accepted.

Miscellaneous

  • None

Acknowledged Deferrals (verified)

  • "This is one PR where the plan recommended two." — Ruled accepted in rounds 1 and 2; unchanged. I am not reopening it: splitting now would mean rewriting history, and the disclosure was made up front.
  • "docs/features/README.md carries a two-row sort fix that is not this lane's work." — Ruled accepted; unchanged and content-free.
  • "Happy to file these as a single follow-up issue if the reviewer prefers them tracked." — Yes, please file one for the four carried tech-debt items (TD 5-8) and carried nits 5-7. They are genuinely structural and each wants its own lane; a tracking issue is better than a third round of the same list.
  • Refs #3065, not Closes — verified again; SDLC control plane routes on asserted facts, not read facts (consolidates 51 self-filed pipeline issues) #3065 remains OPEN and the merge-lane note is unchanged.

Review Delta (vs prior review on HEAD 2ed0f61)

  • ResolvedBlocker: _opt accepts a flag as a value. All three failure modes reproduced as fixed, including the exit code on the flag-first case.
  • ResolvedBlocker: read_verification_outcomes fails open. Verified by fault injection; the predicate now failed.appends on the error branch and keeps absence as a note.
  • ResolvedTD 1, 2, 3, 4, 9, 10 and nits 1, 2, 3, 4, 8, 9. Each re-checked; TD 1's doc-pinning assertion and TD 9's symmetric gloss rule are both genuinely load-bearing now.
  • Not resolvedBlocker: the router cannot see _verification_outcomes. Row 8g was written and is correct in isolation, but query_enriched filters the key out before the router ever sees it, so the wedge is unchanged. Carried as blocker 1 with the one-line location.
  • New — Blocker 2 (0-row UNEVALUATED on the 4 live plans with no check table) and blocker 3 (two in-flight PRs, Key the promise gate on recorded obligations, and run it on the path the PM actually uses #3093 and Coupled-set LLM dependency compatibility gate (Step 1 of #3001) #3089, refused on ungradeable expectation forms). Both are consequences of the gate arming for real; neither existed while it was inert.
  • Withdrawn by me — nothing from round 2. The one round-2 correction I want to preserve: the PR body's verification numbers, recorded as unverified last round, do reproduce on this head.

Verification Results

Reproduced on the PR head in a provisioned worktree, not credited from the PR body:

$ .venv/bin/python scripts/validate_build.py docs/plans/sdlc-control-plane-asserted-facts.md
Result: 29 PASS, 0 FAIL, 0 UNEVALUATED, 0 SKIP   (exit 0)

All four ANTI scope rows, both ruff rows, the six documentation-cascade assertions, and all six pytest-suite rows are green. Round 2's single UNEVALUATED row (Verdict unit tests pass) passes here, confirming its diagnosis as contention and the --timeout lever as the right response.

Narrow-scoped test run: test_sdlc_router.py, test_verification_parser.py, test_validate_build.py, test_merge_predicate.py, tests/unit/sdlc_session_ensure/, test_sdlc_router_reconciliation.py, test_sdlc_next_skill.py, test_lane_identity.py = 626 passed in 59s. ruff check . → all checks passed; ruff format --check . → 1445 files already formatted.

Screenshots

Not applicable — no UI files in the diff.

…gate (Refs #3065)

Round 3 found the gate armed but aimed wrong in three places.

Row 8g never fired. Both router callers take stage_states from
query_enriched()["stages"], which filters to ALL_STAGES and threads
exactly two underscore keys back in. _verification_outcomes was not one
of them, so the rule returned False for every real lane while 626 tests
passed over it -- every 8g test hand-built its payload with the key
already injected. The key is now threaded, and a seam test drives row 8g
from a real query_enriched payload, so deleting it from the tuple fails
a test rather than silently disarming the rule. Verified by removing it
and watching the test fail.

A plan with no verification table recorded a 0-row UNEVALUATED, which
the predicate then refused forever. Four of the twenty live plans are in
that shape. Absence of a contract is not a failed contract: the runner
now declines to record when the plan declares no check table, and says
so, which is what Risk 8 claimed the design already did.

Two in-flight PRs were about to be refused on expectation forms the
grammar did not cover -- output `N`, output is N, and a gloss set off
from exit N by a comma. All three are legitimate authoring. The gloss
rule is now stated once as a rule: a gloss delimited by a comma or a
paren/backtick span is ignored, a bare trailing word stays ambiguous and
UNEVALUATED. `no output` is accepted as an exact synonym of `empty
output`.

Regraded every Expected cell in docs/plans against origin/main's
grammar across eight (exit, output) samples: 58 differences, and zero on
any cell the old grammar actually recognized. Old code returned a bare
False on fallthrough, so its "FAIL" on an unrecognized form was a lie
rather than a verdict; every difference is that lie being corrected.

Also: the blocking set is defined once and shared, so the router and the
predicate cannot drift on which outcomes hold a PR; a non-positive
timeout is rejected at both entry points; do-merge.md points at the
recovery section; and the troubleshooting remedy names the step where
plan edits land on main while the lane grades its own worktree.

Carried tech debt and nits are tracked in #3125.

Plan verification table: 29 PASS, 0 FAIL, 0 UNEVALUATED, exit 0.
@valorengels

Copy link
Copy Markdown
Collaborator Author

Sequencing decision from the owner, recorded so this doesn't sit at 6 days again.

The 3-call-site fix for #3249/#3260 lands first. It edits agent/sdlc_router.py at G3 leg 1 (:518), G6 (:976) and row 10 (:2029) — the same file this PR adds 464 lines to — and it has live decide_next_dispatch probe evidence for both the pre-fix hole and the post-fix landing.

This PR is not closed. After that lands, it needs a real rebase onto it and a re-justification of what remains: the two changes are the same idea at different scales (neither should decide from evidence it did not actually read), so some of these 6999 lines across 38 files may be subsumed. Specifically worth re-checking on rebase: whether this PR's routing changes preserve the absent-pr_head_sha fail-closed behavior, or reintroduce the merge-on-absent-evidence hole that #3260 exists to close.

No review effort should go into this PR in its current conflicted state. Refs #3065, #3249, #3260.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant