US-45.1: the change-thread ledger (narrowed; replaces #901) - #902
Merged
Merged
Conversation
Replaces #901, which reached its review ceiling with material defects, all in one design choice: deciding who may judge a change by inferring it from the calling key. This ledger owns the record only. Findings, fixes, verdicts and round counting move to US-45.3, where the author is a review dispatch loopctl places. - thread_checkpoints and thread_entries, RLS enabled. - A checkpoint is recorded only for the current claimant (the new Delivery.Claimant, shared with Escalations) under the current claim_epoch with a live lease. The lease alone decides liveness, so a claimant that reported done is still the claimant. Checkpoints are keyed by (commit_sha, claim_epoch). - A replay is the same write or a 409: checkpoint_conflict, idempotency_key_reused. - Callers write message and review_requested only. checkpoint_id is an Ecto.UUID, cast to canonical form and checked against this story. The loopctl: key prefix is reserved. - Bodies and notes are bounded and secret-scanned. - Writes take a per-story advisory lock, the story FOR SHARE and the audit chain in one transaction; reads take no lock. - The read pages entries, bounded to int4. - claim_not_live is a fallback clause. ActorLabel and ClaimEpochParam each have one definition. - MCP thread_get, thread_checkpoint and thread_entry (2.105.0), with every refusal named. - US-45.1 and US-45.3 are rewritten to the split. Mutation evidence, bin/mutate.sh: 36, all red; see the PR body.
Review round 1 on #902: - A checkpoint's parent is the previous checkpoint of the same claim. A claim resuming at an earlier commit no longer gets a parent git calls its descendant. - A resend from the principal that recorded a checkpoint is answered from the row before the fence. A lost response followed by a lapsed lease no longer reads as "never recorded". Anyone else still meets the fence. - The entry -> checkpoint key is NO ACTION instead of RESTRICT, so a cascading tenant delete is checked at the end of the statement. thread_entries.checkpoint_id is indexed. - Idempotency keys are secret-scanned too, and every refusal emits [:loopctl, :threads, :secret_blocked] and a log line, as the coordination bus does. - limit=0 is 400, and a whitespace-only note is empty. - The moduledoc says what the audit chain records (not the body). The tier-capabilities comment and thread_get description no longer claim findings and fixes.
Review round 2 on #902: - Liveness is Progress.live_claim? plus the review marker (the newly public live_claim? and Claimant.live?). Previously it read the lease alone, so an implementer could add a checkpoint after request-review, after reporting, or on a NULL lease once verified. Fixes now belong to US-45.3, so the implementer adds nothing after hand-off. - claim_epoch is bounded to int4 (ClaimEpochParam), so an oversized value is a 400 rather than an encode error. A map-shaped query parameter is a 400 too. - A checkpoint's audit-chain event records its commit_sha, tree_sha, claim_epoch and seq, pinning which commit was adopted. - The read returns at most the latest page of checkpoints. - commit_sha and tree_sha must be one object format. - The 400 doc says a limit above the cap is clamped. The MCP tool refuses a missing claim_epoch locally. - replay_allowed keeps its one-line form, with the reason the fence fallback exists (to name the refusal). - US-45.1's AC-3 is updated to the new liveness rule. Not changed: the controller test stays async: false, for the reason StoryEscalationControllerTest documents. The key is resolved on AdminRepo and the story lives on the RLS Repo, so the tenant and key are committed rows.
Review round 3 on #902 (the ceiling; no further round): - thread_checkpoint and an agent-principal thread_entry go out on the key claim_story claims with (LOOPCTL_API_KEY when set, else LOOPCTL_AGENT_KEY), so a checkpoint is sent as the agent that holds the claim. - claim_not_live no longer tells the caller to renew. Renewing cannot undo a review request or a report, and the advice made an agent loop. - The read flags checkpoints_truncated when older checkpoints exist. The AC and docs say "latest page" instead of "all". - review_requested leaves the caller kinds. It belongs to the request-review flow, and a caller-written one could claim a request nobody made. - The OpenAPI publishes the claim_epoch int4 maximum (thread and merge-precondition, from ClaimEpochParam.max/0) and says body and note are bounded in bytes. - Progress.live_claim? is live until the lease is strictly past, the boundary lease_expired?/3 uses. - A replay reads the checkpoint's entry once (author and note), and checks the story still exists first.
This was referenced Sep 27, 2026
mkreyman
added a commit
that referenced
this pull request
Sep 27, 2026
…nce on decisions Fixed in place (no round 4; every fix carries mutation proof), following #902's round 3: none of the findings touched the trust model the rewrite settled. - Runs (findings 2, 3, 4, 7): the adapter reduces the push runs to each workflow's NEWEST run before bounding and before any jobs read, and returns those runs. CiEvidence takes the newest run per workflow from the runs, once per judgement: a newest run with no jobs yet holds a name pending; one that ended with no jobs (startup_failure) fails a name no job carries (run_<conclusion>). - Evidence (findings 1, 5): read_at is stamped when the read STARTS; evidence is copied onto the checkpoint only for a decision (allow, refuse), so CI-wait polls write nothing. - Latency (finding 6): the gate's three forge reads (two trees, CI evidence) run concurrently. - MCP (finding 9): required_checks refuses surrounding whitespace and duplicates locally, matching the server. - Counter (finding 8): kept clearing on a pure CI wait (round 2's decision); documented why the wait is still bounded — every answered poll is judged against the CI wait limit. Mutations: the cited set re-run (A06b, A40b, C10b retargeted) and D01-D05, D07-D09 (D03 re-proved as D03b after a complexity split), all 60 exit 0.
mkreyman
added a commit
that referenced
this pull request
Sep 27, 2026
… exact commit (#910) * US-45.6 (v2): a thread merges only on trusted CI for the checkpoint's exact commit Rewrite of #909, whose third review round still found material defects in one design element: which CI results the gate trusts and against which policy. - intake_sources.required_checks (migration 20260927100000), read LIVE by the gate, so a renamed job can be corrected for stories already in flight. A thread source must name at least one; local-gate and non-string names are refused. - Only a check run created by GitHub Actions satisfies a required name. Commit statuses are read and recorded, never trusted: the implementer's runner can post them. - A checkpoint changing .github/workflows/ or .github/actions/ (renames included) is refused ci_definition_changed for a human: Actions runs the workflow files of the commit under test. - Per name, the latest run of each check suite counts and every suite must pass. - A check still running or not reported is a CI wait: unevaluated, Retry-After 300, never counted toward the unevaluated bound (and it clears the count), refused required_check_timed_out 24 hours after the story ENTERED ci (Stages.entered_at/3). A wait holds back only an allow; any other refusal is decided at once. - Evidence (required runs and local-gate only) is copied onto the checkpoint's gate_evidence["ci"] under a row lock; a record read no later than the stored one is :superseded (the allow path waits and re-evaluates), an identical judgement is not rewritten. An allow whose copy met contention is unevaluated, never an escalation. - Required checks with nothing read fail closed (ci_evidence_not_read). - MCP 2.108.0: intake_source_enroll/update take required_checks; merge_precondition names the reasons. Mutations A01-A47 (A31 re-run as A31b after strengthening its test), all exit 0. * US-45.6 v2 review round 1: trust only the thread's own push-run jobs 1+2. Only a JOB of a GitHub Actions workflow run that a PUSH of the thread branch at the checkpoint's exact commit triggered satisfies a required check (Actions runs + jobs APIs, filtered by the API and again locally), and only by concluding success: a job skipped because a needs: failed, or neutral, fails. A check run created by any other workflow, or a status, is never trusted. check_evidence/3 now takes the branch. 3. Jobs are de-duplicated by id and a short list is refused as truncated; more than 10 workflow runs per push is refused rather than read. 4. Commit statuses are read best effort ({:unread, reason}), feeding only local_gate. 5. Stages.entered_at/3 runs under answering_busy with a bounded lock wait; contention is the ci_entry_unreadable fact, a retry. 6. Scope stated in the CiEvidence moduledoc: scripts the jobs run are code under review, judged by the thread review (US-45.3), not by this gate. 7. A diff that could not be listed refuses ci_definition_unknown (fail closed). 8. A story with no recorded ci entry measures its wait from the checkpoint's recording. 9. A required check name with surrounding whitespace is refused. 10. OpenAPI interpolates ci_wait_retry_after/0 and ci_wait_limit_seconds/0; MCP, verdict and CHANGELOG no longer restate the numbers. Token scope is now actions: read. Per-workflow judgement uses the highest job id (ids only grow); the separate newest-run filter was redundant and removed (mutation B02 showed it could not fail). Mutations A01-A47 (A14-A18, A20-A22, A25 superseded by the B set; A19 by B01; A30 retargeted) and B01, B03-B15, all exit 0. * US-45.6 v2 review round 2: every job in the newest run counts 1. Per workflow, only its newest run counts, and in it EVERY job carrying a required name must pass: matrix legs sharing a name are separate jobs (round 1 wrongly removed the newest-run step and judged the highest job id alone). 2. The ci entry time is read on exactly the path that reads CI, so a lock wait on it can no longer mask a merged or moved head's decision. 3. An identical evidence read that is later advances the stored read_at, so a slower read from in between is :superseded; an identical earlier read is :ok. 4. OpenAPI ci_evidence, the delivery-loop doc and the Source field comment describe the jobs design, not v1's check runs and statuses. 5. A composite action's action.yml anywhere in the diff is a CI definition change. 6. The jobs list's shape is judged before de-duplicating, so a malformed entry is unreadable_jobs, never a crash. 7. Per-run jobs reads run concurrently (4 at a time); the moduledoc restates the ceiling. 8. The migration names the manual step for thread sources enrolled before it; CHANGELOG gives the real column type. 9. ci_result/1 uses Map.fetch!: CI is judged once, in judge/1. Mutations re-run: A01-A47 and B01-B15 as retargeted, plus C01, C02, C04-C10 (C10 and A40 re-run as C10b/A40b after a test for the identical-earlier case). All exit 0. Finding 2's gating has no falsifiable test: which path reads the entry time is not observable from a test. * US-45.6 v2 review round 3 (the ceiling): runs judged from runs, evidence on decisions Fixed in place (no round 4; every fix carries mutation proof), following #902's round 3: none of the findings touched the trust model the rewrite settled. - Runs (findings 2, 3, 4, 7): the adapter reduces the push runs to each workflow's NEWEST run before bounding and before any jobs read, and returns those runs. CiEvidence takes the newest run per workflow from the runs, once per judgement: a newest run with no jobs yet holds a name pending; one that ended with no jobs (startup_failure) fails a name no job carries (run_<conclusion>). - Evidence (findings 1, 5): read_at is stamped when the read STARTS; evidence is copied onto the checkpoint only for a decision (allow, refuse), so CI-wait polls write nothing. - Latency (finding 6): the gate's three forge reads (two trees, CI evidence) run concurrently. - MCP (finding 9): required_checks refuses surrounding whitespace and duplicates locally, matching the server. - Counter (finding 8): kept clearing on a pure CI wait (round 2's decision); documented why the wait is still bounded — every answered poll is judged against the CI wait limit. Mutations: the cited set re-run (A06b, A40b, C10b retargeted) and D01-D05, D07-D09 (D03 re-proved as D03b after a complexity split), all 60 exit 0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
US-45.1 (Epic 45, change threads, #882): the thread ledger, narrowed. Replaces #901.
Why a rewrite, not a round 4
#901 ran three review rounds. Round 3 was the ceiling and still found material defects, and all of them sat in one design choice: deciding who may JUDGE a change (findings, verdicts, the round count) by inferring separation from the calling key. Each round found another way around that inference:
The ledger now owns the record only. Findings, fixes, verdicts and round counting move to US-45.3, where the author is a review dispatch loopctl places for the thread (the story files are rewritten in this PR to match).
What it does
thread_checkpointsandthread_entries, RLS enabled,tenant_idnever cast.claim_epochwith a live lease, throughDelivery.Claimant, now shared withEscalations.commit_sha,claim_epoch): a new claim resuming at a recorded commit records it again.checkpoint_conflictoridempotency_key_reused.messageandreview_requestedonly. The judgement kinds are refused 422, naming the review dispatch, and loopctl's own kinds 422.checkpoint_idis anEcto.UUID: canonicalised, refused at the changeset if malformed, and checked against this story.loopctl:key prefix is reserved.secret_blocked) and returned withbody_untrusted: true.GETpages entries (after_seq,limit,next_after_seq); a parameter outside non-negative int4 is 400.claim_not_liveis a fallback clause.ActorLabelandClaimEpochParamhave one definition each, used by the escalation and merge-precondition controllers too.thread_get,thread_checkpointandthread_entryship in 2.105.0, with every refusal named,keyHinton every call, andLOOPCTL_API_KEYaccepted for reads. The route snapshot is regenerated.Failure design
Checks
bin/mutate.sh), 36, all red:Ecto.UUIDtype, checkpoint-of-story.exact_roleplug, the untrusted marker, the route, the 409 status, the int4 bound, theclaim_not_livecode.keyHint, the API-key fallback, entry fields.mix format, so the mutation was re-pointed.idempotency_key_reused409 test was added.is_nil(agent_id)half in the claimant rule, now removed; the unclaimed guard is mutated instead.Review round 1 (all ten fixed, aaa9023)
thread_entries.checkpoint_idis indexed.[:loopctl, :threads, :secret_blocked].limit=0is 400.thread_getdescription are corrected..claude/makesmix formatskippriv/*/migrations. Recorded as KB c46133e5.Mutations after round 1: 40, all red. Six are new: parent-by-claim, replay-before-fence, replay-author, key secret scan, telemetry and
limit=0.Not covered by a test: the FK mode (a real tenant delete is itself blocked by
audit_chain's FK, and the one-statement emulation passed under RESTRICT too, so it was removed as non-discriminating), the index, and the lock behaviour.Review round 2 (fixed, c5e414d)
Two of these came from round-1 fixes: the unreachable fence-fallback path and the limit wording. That qualifies the PR for round 3.
Progress.live_claim?plus the review marker, so an implementer adds nothing after request-review, after reporting, or on a NULL lease once verified.claim_epochis bounded to int4, and a map-shaped query parameter is 400.claim_epochlocally.async: false, the repo's documented exception with the same reasonStoryEscalationControllerTestgives. Keys resolve on AdminRepo and the story lives on the RLS Repo, so tenant and key are committed rows that a concurrently running module's sweep could delete.Mutations after round 2: 47, all red. New ones:
claim_epochint4 bound;claim_epochcheck.Review round 3, the ceiling (all eight fixed, 86d9d89)
The ceiling allows no round 4. These eight were contained edges and documentation, not a design flaw like #901's, so they were fixed here and each is mutation-tested:
claim_storyclaims with.claim_not_live. The message no longer advises a renewal that cannot help.checkpoints_truncated. The read flags a cut checkpoint list.review_requested. It is no longer caller-writable.claim_epochmaximum and states that bodies and notes are bounded in bytes.live_claim?now agrees with the reclaimer at the exact lease instant.Mutations after round 3: 55, all red, including every round-3 mechanism. The probe-row mutation for truncation replaced one that could not observe the change.
Not covered by a test:
claimed_untilto the microsecond;