Repository navigation
Conversation
… round ceiling An orchestrator places a review, which mints the reviewer's dispatch as a sibling of the implementer's; findings and verdicts are accepted only on the key that placed dispatch minted, bound to the review and the checkpoint it reads. The claimant records fixes naming the findings they answer. Round count and ceiling are computed from thread_entries: round 3 only when a round-2 finding is introduced by a round-1 fix checkpoint, never round 4, and a ceiling with a material finding records review_ceiling and escalates the delivery stage. Refusals have their own codes, never self_review_blocked. Ships the MCP tools (2.106.0). The runner contract bump (AC-45.3.8) remains.
1. Ceiling: placement applied the reverse of the dispatch ceiling. Both now call
Dispatches.lineage_within_caller?/3, the one copy; the caller must be the
implementer's parent or an ancestor. The on-chain check it made unreachable is gone.
2. Round 3 is decided at the round-2 verdict: only fixes with a lower seq count.
3. A verdict revokes every other open review of its round; a verdict refused
review_round_superseded revokes its own dispatch.
4. Moduledoc and MCP text: a verdict resend is 401 once the key is revoked; the
answer-from-row path remains for a failed revocation only.
5. The ceiling escalation goes through Escalations.escalate_as_control/3, which shares
escalate/3's transition, replay and stale_stage recovery minus the claimant check.
6. Judgement idempotency is scoped to the review, in the replay lookup and in a
review-scoped unique index; the author-scoped index now excludes judgements.
7. LoopctlWeb.ThreadHTTP is the one render/param helper for both thread controllers;
location_untrusted is rendered on every thread read.
8. The judgement CHECK refuses a NULL finding_ids.
9. An implementer dispatch that does not resolve is 409 unresolvable_dispatch_lineage.
10. The review payload carries the latest 100 fixes with fixes_truncated, their
findings read by join rather than an id list.
Mutations (bin/mutate.sh, 0 = caught): 62 run, 59 caught. M33 survives (RLS on
agents), N02 survives against the dispatch controller suite (its ceiling test does
not probe the prefix direction; caught by the review suite as N01).
1. A caller with an empty lineage places a review only as the operator, decided by
Placement.may_mint_session_dispatch/2 (now public, the one copy); a legacy key is
409 caller_lineage_required.
2. The reviewer may not be the agent of any dispatch on the implementer's lineage or
the review's own ancestry, checked at placement, under the record lock, and on
every judgement write.
3. The author-scoped idempotency lookup excludes judgements, matching the partial index.
4. Placement is prepare then commit; the round and the separation are re-decided under
the thread lock when the review is recorded.
5. review_round_superseded and reviewer_not_separate end the review and revoke its
dispatch, for findings and verdicts.
6. The record revokes the minted dispatch on a refusal, on busy when no row landed
(a row that did land is answered), and on a raise before re-raising.
7. The ceiling escalation takes a new control-only review_ceiling stage edge with a
control label; the runner-reportable set is unchanged.
8. The review authority (a review of this story) is decided before the payload.
9. Reviews uses Threads.story/2 and Threads.checkpoint_of/3; its copies are gone.
10. The dispatch ceiling suite now tests the prefix direction.
11. The migration's down refuses, with an explicit Ecto.MigrationError, when judgement
keys shared across reviews would break the per-author index it restores.
Owner
Author
|
Closed after review round 3, the ceiling, still found material trust-path defects: the custody halt was not enforced on judgement writes, separation missed descendants of the implementer, public Threads helpers let any module write a verdict, and the ceiling's stage move was best-effort. This is the second authorship design to fail at the ceiling (after #901). The common cause is that placement minted a raw credential and handed it to the orchestrator, and each round found another hole in that credential's ceiling, separation or lifecycle. US-45.3 is being rewritten on feature/us-45.3-review-dispatch-v2: a review is a runner review dispatch loopctl places (AC-45.3.8), and judgements arrive over the runner socket attributed to the dispatch ledger row, so no credential is ever handed out. This branch stays on GitHub as the record. |
mkreyman
added a commit
that referenced
this pull request
Sep 27, 2026
… 1.21.0) (#907) * US-45.3: review a change thread as a placed runner dispatch (contract 1.21.0) A review is a runner dispatch of kind review, placed by Placement.place_review on the story's latest checkpoint of the current claim. Its findings and its one verdict arrive over the runner socket as review_finding and review_verdict, bound to the runner's accepted review-kind ledger row; no API key is minted for or returned to anyone. Replaces the minted-reviewer-key approach of #901 and #905. - Threads.record_review / record_judgement / record_fix: one entry point per write, each re-checking its binding under the thread lock; separation (claimant, checkpoint author, implementer lineage chain) at placement and on every judgement; custody halt enforced on judgement writes. - Rounds counted from thread_entries: one verdict per review, round 3 only when a round-2 finding names a checkpoint a round-1 fix carried, never round 4. - A round-2 verdict with a material finding records a review_ceiling escalation; ReviewCeilingWorker moves the stage over the new review_ceiling edge until it lands or the claim moves. - HTTP: POST thread/reviews (orchestrator+), GET thread/reviews/:id, POST thread/fixes (claimant). MCP 2.106.0: thread_request_review, thread_review_get, thread_fix. - Migration 20260926160000: thread_reviews, judgement columns and CHECKs on thread_entries, runner_dispatches.kind widened to review. - REVIEW_WALL_CLOCK_SECONDS / REVIEW_MAX_TURNS documented in FLY_SECRETS. * US-45.3: review round 1 fixes on PR 907 - A judgement from a review whose claim ended (new epoch, or no claimant after a force-unclaim) is refused review_claim_ended, finally, and writes nothing, so no stale ceiling escalation; placement refuses the same. - Rounds, the round-3 rule and the ceiling count only the current claim epoch's reviews, so a story claimed again starts at round 1. - A resent verdict answers the recorded escalation and re-runs the idempotent slot release and ceiling enqueue. - The verdict enqueues a unique ReviewCeilingWorker job instead of moving the stage inside the channel; an enqueue failure is logged, never raised. - A fix counts toward round 3 only if the thread recorded it by the time round 2 was placed (thread_reviews.placed_at_seq). - Placement runs the push pre-checks (socket, draining, kind, repo, subscription, free slot) before recording the review and its chain entry. - session_ended frees a review session's slot and does nothing else; contract 1.21.0 text, ack and digest updated in place. - Two placements sharing a dispatch_id on different stories answer dispatch_id_conflict from the unique index instead of a 500. - OpenAPI and CHANGELOG corrected; migration down refuses while any review exists. - RunnerThreadSession: the ledger read, epoch check and 422 translation RunnerThreads and RunnerReviews shared as copies. - The completed rounds are read once per judgement write and reused for closed, superseded and the ceiling. * US-45.3: review round 2 fixes on PR 907 - One implement-kind rule on every implement-only ledger consumer: a review dispatch carries the implementer's story and claim epoch, so epoch and story fences cannot tell it apart. DispatchLedger gains where_implement_kind/1 beside implement_kind?/1 from one constant, applied to RunnerStages.apply/3 (stage reports), session_ended_with/4 (the lease reclaim's redrive), reply_story_lock/3 (so only an implement acceptance takes the write lock that re-anchors the claim lease) and place/4's resume of a ledger id (another kind is dispatch_id_conflict). - place_review answers a retry of a recorded placement from its row before any pre-check, with no new chain entry and no second push unless the ledger never recorded the push; another story's id is dispatch_id_conflict. - A fix may answer only findings of reviews under the story's current claim. - A review session that reported session_ended answers resends only; a new judgement is dispatch_not_accepted. - session_ended reads the ledger row once (RunnerStages.held_dispatch/3, busy-guarded) and routes on its kind; neither path reads it again. - The ReviewCeilingWorker cron comment states the enqueue and the backstop. * US-45.3: review round 3 fixes on PR 907 - Ledger session accessors are kind-scoped: accepted_session/4 answers an implement row unless the caller names a kind, and the private held/4 must be told its kind. RunnerThreadSession.read/5 is implement by default and review asks for review. The per-consumer kind checks this made redundant are deleted (RunnerStages, RunnerThreads, RunnerReviews, TriageVerdict); the session-end writes keep their under-lock kind checks, now tested. - The review payload is cast before the review row and chain entry exist. - A resend of a judgement is answered during a custody halt; the halt binds new judgements only. - Freeing a review session's slot never raises in the channel. - A retry answers from the row only for a sent or accepted dispatch; a refused or superseded one is review_dispatch_refused. - Only a material round-2 finding introduced by a fix opens round 3. - The loser of a concurrent same-story placement answers from the row and never pushes. - A malformed runner_id or dispatch_id in the body is 422 invalid_uuid. - The verdict no longer frees the slot; session_ended does. Contract 1.21.0 text edited in place, v1.json regenerated, digest updated. - The slot pre-check reads the one runner's row. - A migration rollback test: down refuses and drops nothing while a review exists, and with none drops and restores thread_reviews.
This was referenced Sep 28, 2026
Closed
mkreyman
added a commit
that referenced
this pull request
Sep 28, 2026
…reviews (#917) (#918) * US-45.9: an interactive session implements in a change thread and a runner reviews it (#917) The three rules that keep an interactive claim out of thread mode: review placement requires a dispatch-made claim, the merge gate reads an unplaced claim as pr, and the executor fences on a stage row a hand-made story lacks. The reviewer stays a runner, as Epic 45 decided after #901 and #905. * Review round 1 on #918: one route derivation and a claimant stage path The first draft special-cased three rules. Every reader of a claim route derives it from one query, and the stage row only moves on runner reports, so the story now records an interactive claim route (mode, base branch, branch) where that query reads, tells placement and bulk claims apart by an explicit marker, lets the claimant report its own stage over the runner-reportable edges with head_sha, moves an intake-born row from queued at claim, starts the CI clock at the ci report, scopes the separation relaxation to claims with no lineage, requires lease renewal in the tool descriptions, fixes the separation test so it can run, and lists US-45.1 and US-45.7 as dependencies.
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
1 participant
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.3 (Epic 45, change threads, #882): review as its own dispatch, checkpoint-bound findings, fixes, and the round ceiling computed from the thread.
First slice. AC-45.3.8 (runner contract 1.21.0:
reviewindispatchable_kinds, the review payload and messages on the wire) is NOT in this PR. Publishingreviewas dispatchable needs loopctl to actually send it (a review dispatcher alongsideTriageDispatcher) and channel handlers for finding/verdict/fix messages, attributed to the runner's ledger row the way this PR attributes them to the minted key. That is its own change. The review gate is still running on this PR.How review authorship is proven
#901 inferred the judge from the calling key and was circumvented three times. Nothing here infers it.
Loopctl.Threads.Reviews.place/4MINTS the reviewer's dispatch and records it in a newthread_reviewstable (story, dispatch, agent, checkpoint, round).findingorverdictis accepted only whenDispatches.dispatch_for_api_key/2resolves the calling key to that exact dispatch row for this story. No lineage walk, no agent comparison. Everything else is403 review_dispatch_required.What it does
POST /stories/:id/thread/reviews, orchestrator+, human anchor, halt-checked in the context likePlacement):implementer_dispatch_id(same parent), so it is never on the implementer's chain;location, andintroduced_by: refused in round 1, required after it, a checkpoint of the story at or before the reviewed one ornone, stored canonicalised.review_round_superseded). The verdict revokes the review's dispatch, freeing its agent's single agent-role key slot for the next round.exact_role: :agent): the checkpoint fence (claimant, epoch, live lease); a checkpoint of the current claim recorded after every checkpoint its findings were found in; findings of completed rounds only.introduced_bynames a checkpoint carrying a fix; never round 4. When a round reaches the ceiling with a material finding, the verdict writes areview_ceilingescalation entry in its transaction, then escalates the delivery stage (best-effort, logged).GET /stories/:id/thread/reviews/:review_id): story and criteria, the checkpoint with the branch and parent commit, the latest entries, every fix with its findings, the rounds.self_review_blocked.thread_place_review,thread_review_get,thread_finding,thread_verdict,thread_fix. Findings and verdicts travel only onLOOPCTL_API_KEY, never the agent key.20260926160000:thread_reviews(RLS enabled) and nullable judgement columns onthread_entrieswith a shape CHECK. No manual step.Design decisions
thread_reviews.dispatch_idhas no FK, likethread_entries.dispatch_id: dispatches are on AdminRepo.409 review_parent_inactive. With orchestrator dispatches capped at four hours, a long-running story can hit this; the remedy is a fresh orchestrator dispatch under the same root, not a weaker placement.place/4is not idempotent (it mints a credential). A retried placement makes a second review for the same round, and only one of them can complete it.Checks
async: falseand COMMITTED, asPlacementTestis. A sandboxed thread write holds the tenant's audit-chain lock, and the AdminRepo mint then waits on it for ever.Mutations (
bin/mutate.sh), exit 0 = caughtthird_round_warranted?/3)agentsdoes the same job)All run with
--no-baselineafter a green run of the same check, and</dev/null. D01 rolled the migration back and forward under the mutation; the real index was restored the same way afterwards.Not covered by a test
tenant_idpredicate in the agent check survives mutation, because RLS onagentsalready hides another tenant's agent. It is kept by the repo's dual-scoping convention.role: :orchestratorplug is not mutated on its own: with it removed, the context's gate (M30) still answers 403.checkpoint_idbelonging to ANOTHER story of the same tenant. The test uses a random id.