feat(gate-state): durable per-issue gate state on the issue substrate (#166) - #177
Conversation
Spec Review (Iteration 1)Verdict: approved Acceptance criteria vs. diff
Process note (non-blocking)Plan-gate contrarian iteration 3 raised two majors and hit the contrarian cap before full resolution (see "Proceeding After Contrarian Cap (Plan)"):
Scope check: all changed files ( |
Code Review (Iteration 1)Verdict: changes requested. Validation baseline is clean: The findings below are in the state machine itself, not in the plumbing. 1. BLOCKING —
|
…ill missing probe rows Code review on PR #177 found four state-machine defects in the gate-state substrate: - gateStateProbeCommandLine's jq counted ALL comments into `total`, so any issue with one ordinary comment and zero gate-state blocks reported read-failed instead of absent -- the common case, not an edge case. `total` now runs through the same title-gated filter `blocks` uses. - diffGateStateIntent ordered same-run supersession on `epoch`, but RUN_EPOCH is assigned once per run and is identical on every boundary a run posts, so 'superseded' was unreachable in production. Added GATE_STATE_WRITE_SEQ, a monotonic per-run write counter embedded on the payload as `write_seq`, and moved ordering onto it. - fetchGateStateBlocks' diagnostic log misread a live chunk's response that simply omitted a queried issue as absent rather than read-failed. Missing issues are now backfilled with the same read-failed stub a dead chunk's issues get. - verifyGateState's mismatch/read-failed outcomes only logged; they now also push a VERIFY_SKIPS entry so a failed self-validation reaches the batch PR's Verification Gaps section. Also fixes a stale doc comment (verifyGateState claimed a concurrent run's write could report 'superseded'; diffGateStateIntent's own run check forbids that) and reconciles gate-hygiene.md's `total` wording. Validation: node --check, lint-engine (clean, 2 lockstep pairs in sync), node --test (723/723 pass, 5 new).
PR Review Fix (iteration 1)Commit: e88650d Addressed all four code-review findings from the "## Code Review (Iteration 1)" comment:
Validation: |
Spec Review (Iteration 2)Verdict: approved Re-reviewed after the code-review fix commit ( What changed since iteration 1's approval
Verification
Acceptance criteria (issue #166) — unchanged from iteration 1, re-confirmed at HEADAll criteria previously marked "met" in the iteration-1 review remain met; none regressed. The four fixes above make the found/absent/read-failed distinguishability and idempotent-write criteria more precisely satisfied than at iteration 1, not less. No consumer wiring was added (still out of scope per the issue), no Scope checkNo scope creep. The fix commit is narrowly targeted at the four code-review findings; nothing beyond that landed. Consistency with iteration 1This does not reverse the iteration-1 spec approval — it reconfirms it. The two still-open non-blocking notes from iteration 1 ( |
Code Review (Iteration 2)Verdict: changes requested. Validation baseline is clean at Iteration-1 findings: all four re-verified fixed
Finding 7 from iteration 1 (the four engine-only fields in 1. MAJOR — every hard read failure on an issue with prior-work evidence logs as
|
…sful read Every hard read failure (dead chunk, non-zero gh exit, truncated stdout) also produced blocks.length===0 and total===0, so it printed the same "absent (unexpected: ...)" line as a genuine falsifiable-absent read. On a resume, where hasGateStatePriorWork is true for exactly the issues this substrate serves, that made read failures indistinguishable from suspicious absences. Add the readOk = row.exit_ok === true && parsed.ok === true guard, mirroring attachGateStateBlocks' own definition, so only a genuine successful-but-empty read prints the suspicious line and every read failure falls through to a plain "read-failed". Add prior-work-bearing test variants for the dead-chunk, omitted-row, non-zero-exit, and truncated-stdout cases. Also fix the stale "advisory only (log lines, never a result mutation)" call-site comment on the verify sweep (it now also pushes to VERIFY_SKIPS) and rewrap one over-length prose line in gate-hygiene.md. Addresses PR #177 code review iteration 2 (issue #166).
PR Review Fix (iteration 2)Commit: 480b01d
Verified: |
Spec Review (Iteration 3)Verdict: approved Re-reviewed after the code-review fix commit ( Iteration-2 code-review findings vs. this commit
Verification
Acceptance criteria (issue #166) — unchanged from iterations 1-2, re-confirmed at HEADAll criteria remain met; none regressed. The read/absent distinguishability criterion is now precisely satisfied at the log level as well as the stored-field level (iteration 2's fix handled the field, this commit handles the log — the same disagreement iteration 1 originally named). Scope checkNo scope creep. The commit is narrowly targeted at the three iteration-2 findings; nothing beyond that landed. Consistency with iterations 1-2This does not reverse either prior spec approval — it reconfirms them. The two still-open non-blocking notes from iteration 1 ( |
Code Review (Iteration 3)Verdict: approved. Validation baseline is clean at Iteration-2 findings: all three re-verified fixed
Independent re-check at HEADBeyond the fix commit, I re-walked the parts of the substrate a second fix pass could plausibly have destabilized:
Non-blocking notes (no change required for this merge)
Not flagged, deliberately: the missing |
Task 1 of issue #166: the pure JS layer for durable per-issue gate state, mirroring the CONSOLIDATION_* marker subsystem end to end. Adds GATE_STATE_TITLE/GATE_STATE_SCHEMA plus buildGateStatePayload/ buildGateStateComment/parseGateStateComment (title-gated, fence-extracted JSON, canonical marker as the last line, never throws), parseGateStateProbeRow (structurally rules out a truncated read being misread as absence), selectGateState (found/absent/malformed/read-failed with explicit trust-before-last-wins and a falsifiable-absent cross-check against prior-work evidence), isTrustedGateStateAuthor (self_login primary, claim_authors fallback restricted to fresh-or-batch-matching claims), deriveRunEpoch, diffGateStateIntent (match/mismatch/superseded), and attachGateStateBlocks. Declares RUN_EPOCH beside ENGINE_OWNED and wires it into __seed. Adds GATE_STATE_PROBE_SCHEMA/GATE_STATE_VERIFY_SCHEMA and four optional gate-state fields on PREFLIGHT_SCHEMA. All above the TICKETMILL-TEST-HARNESS-SPLIT marker; no write/read call sites wired yet (later tasks). tests/gate-state.test.js: 33 new unit tests covering the build/parse round trip (incl. apostrophe/newline-bearing free text), every parse rejection path, all four select states with both falsifiable-absent branches, positional trust-before-last-wins selection, trust rules, staleness, and all three diff verdicts. node --test: 678 passed, 0 failed. Ran scripts/lint-engine.js --fix per the LOCKSTEP-EDIT rule, syncing .claude/workflows/ticketmill.js.
selectGateState only routed to 'read-failed' on zero blocks when total was also 0 and hasPriorWork was true. A self-contradictory probe result (zero blocks but total>0) fell through to 'absent' regardless of prior-work evidence, silently presenting a truncated/corrupted read as genuine absence -- the exact failure mode this design exists to prevent. Check total>0 unconditionally, before hasPriorWork. Also fixes a backwards doc reference (CONSOLIDATION_MEMBER_TITLE/ CONSOLIDATION_GROUP_TITLE are defined below GATE_STATE_TITLE, not above) and adds a covering unit test.
Adds the non-fatal postGateState(ctx, boundary) helper (issue #166 task 2), modeled on the cap-note-plan/cap-note-approach stages: stageOpts('probe'), NOTE_SCHEMA, exactly one try, log-only on a dead agent or posted!==true. Posts the durable "## Gate State" comment via `gh issue comment <n> --repo <r> --body-file -` fed by a QUOTED heredoc, deliberately breaking from postConsolidationMarkers' `--body "..."` idiom so free text pulled from ctx.settled (which may carry apostrophes/backticks/$) reaches gh as literal bytes instead of being handed to the shell for interpolation. Wires it at four boundaries in implementIssue()/reviewAndMerge(): - 'approach', once after the approach-gate loop closes (covers all four of its break exits, before the plan stage's own fail() returns). - 'plan', once after the plan-gate loop closes (same four-break coverage, before IMPLEMENT). - 'pr-review-iN', once per pr-review iteration, right after recordGateOutcome(ctx, 'pr-review', ...) — kept inside the loop since reviewAndMerge returns from inside it at the nothing-to-fix and cap-reached breaks. - 'pr-review-iN-aborted', immediately above the reviewer-death `return fail(ctx, 'needs_human', ...)` — the only boundary a process_pr resume can reach, since that path calls reviewAndMerge directly and never runs the approach/plan loops. ctx.gate_state_intent is set only when posted===true; every other outcome sets ctx.gate_state_post_failed = boundary and pushes a ctx.deferred note, so a routine non-fatal post failure can never look like corruption to Task 4's future verify sweep. Threaded through fail(), the success return, and the resume_point==='skip' return (always null there — shape totality, no boundary can fire on that path). Updates tests/pr-review-gate.test.js's scripted responders/key-order assertions for the new in-loop and aborted gate-state calls, and adds tests/gate-state-post.test.js covering non-fatal failure in both directions, the pinned posting idiom, one-post-per-iteration, approach-then-plan ordering, a dead post changing no loop outcome, the process_pr-resume aborted-boundary scenario, a STOP trip posting nothing new for the iteration that never runs, and skip-path shape totality. node --test tests/*.test.js: 690/690 green. Lockstep copy (.claude/workflows/ticketmill.js) kept in sync; scripts/lint-engine.js clean. Ref #166
… one helper
Five tests repeated the same
keys.filter(function (k) { return k.indexOf('gate-state-') === 0 })
inline; extract it to a gateStateKeys() helper next to the file's existing
stageKeyOf() helper. No behavior change — 11/11 in this file, 690/690 overall.
Ref #166
…nd RUN_EPOCH Wires the durable per-issue gate-state READ path (issue #166 task 3): - fetchGateStateBlocks(issueNumbers, priorWorkByIssue): a READ-ONLY, DRY_RUN-safe probe mirroring fetchConsolidationMarkers' shape but pinning the claim probe's deterministic jq idiom (one gh command per issue, jq computes the exact {total, blocks} return shape) instead of a bare `gh issue view --json comments`, so a truncated read is structurally a parse failure, never a fake absence. Chunked at MAX_GATE_STATE_PROBE_CHUNK (5) issues per agent call; a dead chunk marks only its own issues read-failed via synthesized stub rows, never silently dropping them. self_login is reduced across chunks (first non-empty wins). Logs one line per issue naming found/absent/malformed/read-failed, plus a distinct greppable line for the falsifiable-absent case (zero blocks with prior-work evidence). - attachGateStateBlocks extended to a real join: it now takes the probe's raw rowsByIssue + reduced self_login and always computes the four PREFLIGHT_SCHEMA gate-state fields fresh, never trusting whatever the preflight object already carried for them (clobbers a hallucinated agent-supplied value the same way attachEngineOwnedIntentional never trusts an agent-asserted regime). - Wired at Select immediately after attachEngineOwnedIntentional, threading each preflight's own pr_number/worktree_exists/resume_point through as priorWork so the falsifiable-absent rule is evidence-driven. - RUN_EPOCH assigned right after outcomeGradeR/revisitRiskR are awaited, via the existing pure deriveRunEpoch over whichever probe's `now` is available; logs loudly when both are unavailable. Tests: tests/gate-state-read.test.js covers the chunking/dead-chunk/ self_login-reduction/truncated-stdout/partial-coverage behavior; the pre-existing single-arg attachGateStateBlocks test in tests/gate-state.test.js is updated to the new always-clobber contract. Full suite 704/704 green, lockstep in sync. Refs #166
Task 3's fetchGateStateBlocks diagnostic log recomputed the exact three-condition prior-work check selectGateState already has inline. Extract it into one shared pure helper, hasGateStatePriorWork, so the falsifiable-absent logic exists in one place. Refs #166
…d-absent fetchGateStateBlocks' diagnostic-log branch fired whenever hasGateStatePriorWork(pw) was true, without checking parsed.total === 0 -- so a genuinely corrupted/truncated read (total>0, zero matching blocks, which selectGateState always treats as read-failed) that happened to coincide with prior-work evidence got logged as "absent (unexpected: ...)" instead of a plain read-failed. Require parsed.total === 0 in the branch condition so only the true falsifiable-absent case gets the "unexpected absent" wording. Updates the gate-state-read.test.js case that pinned the buggy behavior with jqRow(3, []) to use jqRow(0, []) (the genuine falsifiable-absent case) and adds a new case asserting the total>0 corrupted-read path logs plain read-failed, never the unexpected-absent wording.
Task 4 of issue #166: a single Report-phase stage, chunked at MAX_GATE_STATE_PROBE_CHUNK like fetchGateStateBlocks, that proves post -> GitHub -> read -> parse for the durable gate-state comment without ever showing the verifying agent the payload it's checked against (the prompt carries only issue numbers plus the same jq-pinned per-issue read Task 3 uses). JS runs parseGateStateProbeRow -> parseGateStateComment -> diffGateStateIntent against each result's gate_state_intent and logs one of six outcomes: match, mismatch, superseded, read-failed, post-failed, no-intent. Non-fatal end to end (advisory logging only, never mutates a result's status); wrapped in try/catch at the call site, placed before the token/friction rollups. Also updates GATE_STATE_VERIFY_SCHEMA (declared but unused by Task 1) from its original per-issue shape to the chunked rows[] shape this sweep actually needs, mirroring GATE_STATE_PROBE_SCHEMA. tests/gate-state-verify.test.js covers all six outcomes, a fully dead verify stage, one dead chunk of two leaving the other intact, and that the prompt never embeds the intent payload.
…be plumbing fetchGateStateBlocks and the new Report-phase verifyGateState sweep independently built the same chunk list, pinned the same jq idiom verbatim, generated the same dead-chunk stub rows, and normalized rows the same way. Extract chunkGateStateIssues, gateStateProbeCommandLine, deadGateStateChunkRows, and normalizeGateStateRow as shared helpers so the read-side probe and its self-validation sweep can't drift apart. No behavior change; 718/718 tests pass, lint-engine confirms lockstep.
Append a section to docs/architecture/gate-hygiene.md covering issue #166's "## Gate State" comment: why the payload is fenced JSON rather than consolidation's flat key:value lines, the four write boundaries (and the recorded decision to leave the STOP.tripped exit without one), append-only positional last-wins and the idempotence it buys for free, the four-state read contract and why `absent` must be falsifiable, the jq-pinned read idiom versus fetchConsolidationMarkers's bare read, trust-before-last-wins selection, the intent-only-on-success rule and the post-failed sweep outcome, the self_login/claim_authors trust model, RUN_EPOCH's derivation from existing wall-clock reads, seeded_from as a consumer-less discriminator, the group-identity gap that leaves group_id/members riding in every payload, and the deviation from the issue body's preflight-step wording toward a separate chunked probe. Refreshes the gate-hygiene.md summary row in index.md and the byte-identical AGENTS.md/CLAUDE.md pair. pipeline.md, metrics.md, and failure-semantics.md are untouched (hash-frozen by tests/architecture-provenance.test.js). node --test tests/*.test.js: 718/718 green; node scripts/lint-engine.js clean.
…ill missing probe rows Code review on PR #177 found four state-machine defects in the gate-state substrate: - gateStateProbeCommandLine's jq counted ALL comments into `total`, so any issue with one ordinary comment and zero gate-state blocks reported read-failed instead of absent -- the common case, not an edge case. `total` now runs through the same title-gated filter `blocks` uses. - diffGateStateIntent ordered same-run supersession on `epoch`, but RUN_EPOCH is assigned once per run and is identical on every boundary a run posts, so 'superseded' was unreachable in production. Added GATE_STATE_WRITE_SEQ, a monotonic per-run write counter embedded on the payload as `write_seq`, and moved ordering onto it. - fetchGateStateBlocks' diagnostic log misread a live chunk's response that simply omitted a queried issue as absent rather than read-failed. Missing issues are now backfilled with the same read-failed stub a dead chunk's issues get. - verifyGateState's mismatch/read-failed outcomes only logged; they now also push a VERIFY_SKIPS entry so a failed self-validation reaches the batch PR's Verification Gaps section. Also fixes a stale doc comment (verifyGateState claimed a concurrent run's write could report 'superseded'; diffGateStateIntent's own run check forbids that) and reconciles gate-hygiene.md's `total` wording. Validation: node --check, lint-engine (clean, 2 lockstep pairs in sync), node --test (723/723 pass, 5 new).
…sful read Every hard read failure (dead chunk, non-zero gh exit, truncated stdout) also produced blocks.length===0 and total===0, so it printed the same "absent (unexpected: ...)" line as a genuine falsifiable-absent read. On a resume, where hasGateStatePriorWork is true for exactly the issues this substrate serves, that made read failures indistinguishable from suspicious absences. Add the readOk = row.exit_ok === true && parsed.ok === true guard, mirroring attachGateStateBlocks' own definition, so only a genuine successful-but-empty read prints the suspicious line and every read failure falls through to a plain "read-failed". Add prior-work-bearing test variants for the dead-chunk, omitted-row, non-zero-exit, and truncated-stdout cases. Also fix the stale "advisory only (log lines, never a result mutation)" call-site comment on the verify sweep (it now also pushes to VERIFY_SKIPS) and rewrap one over-length prose line in gate-hygiene.md. Addresses PR #177 code review iteration 2 (issue #166).
…s two real posts The "approach then plan, in order" test only checked stage-call ordering, never the posted payloads' write_seq values -- a regression reverting the GATE_STATE_WRITE_SEQ ++ (added in an earlier PR-review round to fix same-run write ordering, since RUN_EPOCH is identical across all boundaries in a run) to a static value would have passed every test in the suite. Extract the real heredoc body postGateState sends to gh from the captured agent prompts and parse it with the real parseGateStateComment, then assert the plan boundary's write_seq is exactly one more than the approach boundary's -- proving the module-level counter, not a fixture, drove both values. Confirmed this fails (1/11) against a version of the increment reverted to a static write_seq: 1.
480b01d to
4c36b8f
Compare
Implementation CompleteBranch Reviews passed: spec review (approved) and code review (changes requested -> fixed -> approved), both through their full iteration history, plus the plan-gate/approach-gate contrarian loops on issue #166. Merge note: this PR was CONFLICTING after review; it was auto-rebased onto Deferred Suggestions for Follow-upFiled as separate issues from the review history on this PR:
|
Closes #166
Summary
Adds a durable, GitHub-native substrate for per-issue contrarian/review gate state so it survives run boundaries, mirroring the existing
CONSOLIDATION_*marker subsystem end to end. This tier is substrate-only: no consumer wiring yet.postGateState(ctx, boundary), a non-fatal stage modeled oncap-note-plan, posts a title-gated## Gate Stateissue comment: one non-directive human summary line plus a fenced JSON payload ({schema, repo, issue, run, batch, epoch, boundary, gate_budgets, settled}) inside a<details>wrapper, closing with the canonical<!-- ticketmill <repo>#<issue> -->marker. Called at four boundaries: after the approach-gate loop, after the plan-gate loop (all four exits), once per pr-review iteration, and on pr-review-death (covering theprocess_prresume path). Append-only, positional last-wins.fetchGateStateBlocks(issueNumbers, priorWorkByIssue), a jq-pinned, chunked (max 5 issues/call) read-only probe, wired at Select time viaattachGateStateBlocks, which always computes the four newPREFLIGHT_SCHEMAgate-state fields fresh (never trusting an agent-supplied value).RUN_EPOCHis derived once via the existing purederiveRunEpochand threaded through for staleness checks.TICKETMILL-TEST-HARNESS-SPLITmarker —buildGateStatePayload,buildGateStateComment,parseGateStateComment,parseGateStateProbeRow,isTrustedGateStateAuthor,deriveRunEpoch,gateStateEpochStale,selectGateState,diffGateStateIntent,attachGateStateBlocks— the agent only ever returns raw text, JS alone decides.verifyGateState(results)runs once per Report phase (not 2N), chunked, comparingparseGateStateCommentoutput against each result'sgate_state_intentand logging one of six outcomes (match/mismatch/superseded/read-failed/post-failed/no-intent) — proving post → GitHub → read → parse round-trips in the same run, non-fatal end to end.docs/architecture/gate-hygiene.mdcovering comment shape rationale, write boundaries, the four-state found/absent/malformed/read-failed contract, trust-before-last-wins selection, and the self-validation sweep.Key decisions
key:valuelines) becausesettled[]is an array of five-field objects with free text.scripts/lint-engine.jsforbidsDate.now());RUN_EPOCHis derived once via the existing purederiveRunEpochfrom thedate -uidiom already used elsewhere.STOP.trippedexit is deliberately left unwritten (recorded as an explicit decision, not an oversight).Token usage (approximate, this issue only): 876318 output tokens