Skip to content

US-45.5: the merge executor squashes a thread's allowed checkpoint as a GitHub App - #911

Merged
mkreyman merged 6 commits into
masterfrom
feature/us-45.5-merge-executor
Sep 27, 2026
Merged

US-45.5: the merge executor squashes a thread's allowed checkpoint as a GitHub App#911
mkreyman merged 6 commits into
masterfrom
feature/us-45.5-merge-executor

Conversation

@mkreyman

@mkreyman mkreyman commented Sep 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

US-45.5: the merge executor squashes a thread's allowed checkpoint as a GitHub App

Story: docs/user_stories/epic_45_change_threads/us_45.5.json (AC-45.5.1..9, TC-45.5.1..10). PRD §4 items 3-5.

The review gate has not run yet; this PR is opened on the first green commit so CI and review run concurrently.

What it does

  • Loopctl.Workers.ThreadMergeWorker (Oban default, unique per story over available/scheduled/executing/retryable, period: :infinity, 8 attempts) runs Loopctl.Delivery.MergeExecutor. MergePrecondition enqueues it on every recorded THREAD-mode allow (replays included, so re-asking the gate re-enqueues after a lost insert).
  • The executor trusts nothing from the enqueue: stage ci, a merge_gate_allowed_sha, the allow's own event naming a checkpoint and base_sha, the claim placed in thread mode, and Threads.claim_checkpoints/2's judged head being exactly the allowed checkpoint (AC-45.5.1). Anything else is {:skipped, reason} with no forge call.
  • Squash (AC-45.5.2): forge tree vs recorded tree_sha (tree_mismatch, escalated), freshness (AC-45.5.8: base head must equal the allow's base_sha and be contained by the checkpoint), empty_change, one commit with the checkpoint's tree and the base head as its only parent, merge_commit_sha recorded by compare-and-set (Threads.record_merge_commit/5) BEFORE update_ref(force: false).
  • Lost ack (AC-45.5.3): a retry whose recorded merge_commit_sha is an ancestor of the base head is already_merged and moves the story to merged. A retry on an unmoved base reuses the recorded commit instead of minting another.
  • Base-update path (AC-45.5.4/7): a stale base or a non-fast-forward refusal merges the base INTO the thread branch as the App (POST /repos/:repo/merges); a merge whose first parent is the checkpoint is recorded by Threads.record_base_update/4 (control-plane, no HTTP route, bypasses the claimant fence) which takes the new {:ci, :ci, :base_updated} edge in the SAME transaction via Stages.follow_base_update/4: head replaced, allow and head-keyed fields cleared, review/custody untouched, counted in attempts. advance/4 refuses the edge from every caller. A conflict goes back over base_moved while the claim is live, and escalates claim_not_live otherwise (the gate's own rule for a moved thread head, so nothing loops).
  • AC-45.5.9: claim_checkpoints/2 judges a base_update that IS the last-allowed checkpoint or whose parent chain reaches it (Stages.last_allow_query/2); any other base update is invisible. The gate then needs green CI on that exact sha (existing US-45.6 read).
  • AC-45.5.5: Loopctl.Delivery.MergeMessage builds Story <number>: <title> reduced to one printable line (\p{C}, \p{Z}, whitespace collapsed, 72-codepoint bound), the thread URL, and a Loopctl-Story: <id> trailer; no entry text.
  • AC-45.5.6: Loopctl.Delivery.MergeForge behaviour, Req adapter GitHubAppMergeForge (App JWT RS256 via :public_key, installation lookup per repo, token scoped to that repo and contents: write, 2s/5s timeouts, retry: false, redirect: false, failures classified by GitHubPullRequestSource.classify_failure/1). New env vars GITHUB_APP_ID / GITHUB_APP_PRIVATE_KEY documented in deploy/FLY_SECRETS.md; unset, the executor escalates app_unconfigured and makes no request. Mox mock Loopctl.MockMergeForge, config-based DI.
  • Failure design (SOUL rule 9) is in MergeExecutor's moduledoc: transient faults retry and escalate forge_unavailable on the last attempt; two runs cannot overlap (unique incl. executing) and if they could, the ref CAS and the merge_commit_sha CAS keep it to one squash; a lost base-merge ack leaves an unrecorded branch head that the retry sends back over base_moved rather than adopting.

Also: the egress chokepoint allowlist and docs/egress-guard.md gain the adapter (it carries the story number and title to the tenant's own repository), CHANGELOG, docs/agent-delivery-loop.md, and the MergePrecondition moduledoc.

No new endpoint, so no MCP tool.

Size

One PR, not split: the brief's PR-1 set includes AC-45.5.8 (freshness), which cannot be met without the base-update path, and a PR-1 that escalated instead was ruled out. Production code is about 1.1k lines of which a large share is doc comments; the rest is tests.

Tests

  • test/loopctl/delivery/merge_precondition_integration_test.exs — new merge executor (US-45.5) block (committed-tenant, like the gate's own thread tests): TC-45.5.1, .2, .4, .5, .6, .8, .9, conflicts, first-parent mismatch, empty change, transient/final attempt through ThreadMergeWorker.perform/1, and the enqueue wiring through enforce/3 with inline Oban. The existing thread block stubs the App as transiently unreachable, since every thread allow now runs the executor inline.
  • test/loopctl/threads_test.exs — record_base_update/4 (TC-45.5.8 at the context level, idempotency, refusals), TC-45.5.10, record_merge_commit/5 CAS, and tenant isolation for both writes.
  • test/loopctl/delivery/stage_machine_test.exs TC-45.5.7; stages_test.exs edge refused to advance/4; merge_message_test.exs TC-45.5.3; github_app_merge_forge_test.exs (JWT verified with the public key, non-ff/ruleset 422, merge 201/204/409, compare 404); thread_merge_worker_test.exs (unique config).

Mutation table

Every assertion added, proved with ~/workspace/claude-config/bin/mutate.sh (exit 0 = the check failed under the mutation). Run sequentially. Checks are the whole test file named.

id mutation check file exit
M1 merge_precondition.ex: enqueue on thread allow replaced by :ok integration 0
M37 merge_precondition.ex: enqueue clause matches no mode integration 0
M2 executor: allow must name the judged checkpoint (guard to is_binary) integration 0
M3 executor: forge tree vs recorded tree ignored integration 0
M4 executor: freshness ignores base_sha (first run) integration 1
M4b same, after TC-45.5.9 made the checkpoint contain the moved base integration 0
M5 executor: merge_commit_sha not recorded before the ref integration 0
M6 executor: recorded ancestor not read as already_merged integration 0
M7 executor: empty_change not refused integration 0
M8 executor: non-fast-forward escalates instead of base update integration 0
M9 executor: conflict escalates instead of base_moved integration 0
M10 executor: claim-live check forced true integration 0
M11 executor: final attempt does not escalate integration 0
M12 worker: final? wiring forced false integration 0
M13 executor: non-transient refusal not escalated integration 0
M14 executor: thread branch head not compared to checkpoint integration 0
M15 executor: base merge first parent not checked integration 0
M16 executor: recorded squash commit not reused integration 0
M17 stages: follow_base_update allow guard removed threads 0
M18 stages: explicit stage guard (redundant with CAS) threads 1
M19 stages: explicit row-epoch guard threads 2
M20 stages: explicit story-epoch guard threads 1
M21 stages: advance/4 admits base_updated stages 0
M22 stages: follow_base_update in-transaction guard stages 0
M23 stage_machine: edge clears only head_sha stage_machine 0
M24 stage_machine: edge removed from table (first run) integration 1
M24b same, after follow_base_update asks the table integration 0
M25 threads: an allowed base_update not judged integration 0
M26 threads: parent reaching the allow not counted threads 0
M27 threads: every base_update judged threads 0
M28 threads: merge_commit_sha CAS ignores expected threads 0
M29 threads: parent-epoch fence removed (first run) threads 1
M29b same, after the ended-claim parent test threads 0
M30 merge_message: unprintable class reduced to newline (first run) merge_message 1
M30b same, hostile chars moved inside the 72-codepoint bound merge_message 0
M31 merge_message: subject bound 72 to 720 merge_message 0
M32 adapter: non-ff 422 mapped to api error adapter 0
M33 adapter: force true adapter 0
M34 adapter: compare 404 not mapped to not-ancestor adapter 0
M35 adapter: 409 not mapped to merge_conflict adapter 0
M36 worker: unique states without executing worker 0

Survivors and what was done: M4, M24, M29, M30 exposed weak tests, which were strengthened (the b rows). M18 and M20 showed explicit guards in follow_base_update/4 that the compare-and-set already enforces, so they were deleted rather than kept as dead checks; M19 was refused (its old text is not unique in stages.ex) and its guard was deleted for the same reason.

Check files: integration = test/loopctl/delivery/merge_precondition_integration_test.exs, threads = test/loopctl/threads_test.exs, stages / stage_machine / merge_message / adapter (github_app_merge_forge_test.exs) / worker (test/loopctl/workers/thread_merge_worker_test.exs).

Gate

The pre-commit gate ran green on the commit (format, compile, credo --strict, dialyzer, full suite).

Review round 1 (10 findings, fixed in 3401bea and d6c74e8)

  1. The base ref could move after the story left ci: Threads.record_merge_commit/6 now runs Stages.mergeable_in/4 under the story and stage-row locks (stage ci, same claim epoch, allowed sha is the checkpoint) and fails closed. After a successful ref update any failure to record merged escalates naming the sha; a later run finding the squash on the base with the story out of ci escalates over a new control-only edge merged_outside_ci. Residual window: the lock cannot span the HTTP ref update; the orphan escalation covers it.
  2. Final attempt: one finalize/4; every non-success on the last attempt escalates retries_exhausted (renamed from forge_unavailable), crashes included.
  3. Non-transient context errors at ci with an allow escalate with a named reason instead of skipping.
  4. A thread-mode already_merged verdict enqueues the executor, which adopts the merge (the recorded squash, or the checkpoint commit itself).
  5. Base churn is bounded: three consecutive base updates escalate base_churn.
  6. The installation token requests contents: write and workflows: write; docs say the App needs both.
  7. The worker is no longer unique while a job is executing; the fence and the ref compare-and-swap keep concurrent runs to one squash.
  8. The base update is a compare-and-swap: merge the base into a temporary loop/loopctl-base-update-* branch cut at the checkpoint, fast-forward the thread branch to it (force: false), record the base update only after the ref moved, delete the temp branch best effort. New forge callbacks create_ref/3, delete_ref/2.
  9. A 422 leaves the adapter as a classified atom only, never GitHub's text.
  10. The reason budget reads StageMachine.max_reason_length().

Session check: the story-epoch half of the fence could not fail (mutation survived); the stage row moves with the story in the same transaction on every claim and release, so the redundant half was removed (d6c74e8).

Mutations: every cited one re-run at the new text, plus N1-N23 for the fixes; all exit 0.

Review round 2 (10 findings, fixed in 2555cd3 and 3fb1bce)

Three findings traced to round 1's fix 7 (overlapping runs); it was reversed at the cause rather than guarded.

  • One run per story again (:executing back in the unique states). An allow recorded during a run is picked up at the end of every run, which snoozes the same job; the residual window after the final re-read, and any job killed on its last attempt, is re-driven by the new ThreadMergeSweepWorker (cron, every 5 minutes, thread stories at ci with an allow; pull-request stories are never swept).
  • A merge-commit compare-and-set loser converges on the next run; a transient read of the recorded squash retries rather than minting a new one; a transient failure writing merged after the squash landed retries and the retry adopts the merge.
  • The window between the executor moving the thread branch and recording the base update: the gate answers a head with the executor's exact shape (two parents, the allowed checkpoint first and a commit on the base second; CheckpointSource.base_update_of/3, one predicate shared with the executor) as the transient base_update_in_flight; a claimant's single-parent commit is still a moved head. The executor's retry adopts its own unrecorded base update only when GitHub re-merging the same commit into the checkpoint produces an identical tree.
  • Exits are handled like crashes on the last attempt; merges recorded on rows past merged raise no alarm; the ancestry compare uses per_page=1; DispatchPayload.placed_base_branch/2 is the one base-branch derivation; a misplaced comment moved back.

Mutations: every cited one re-run at the new text, plus R1-R20 and the in-flight shape pair; all exit 0.

Review round 3 (the ceiling; 9 findings, fixed in place in 2bade08, no round 4)

None of the findings changed the design, so they were fixed in place with mutation proof, following #902 and #910.

  1. The last attempt first asks whether the recorded squash is on the base: if so it adopts it (or escalates naming the sha), and only otherwise escalates retries_exhausted.
  2. One stage-aware escalation helper: merge_gate at ci, merged_outside_ci from queued or in-flight stages; finalize and the orphan path share it.
  3. The sweep selects only stories whose CURRENT claim's accepted implement dispatch is thread mode (DispatchLedger.route_rows_query/0, shared with the gate's route query), so pull-request stories never fill its batch.
  4. Recovery treats only a genuine non-match as a moved head; other forge errors retry or escalate with their reason.
  5. A claimant checkpoint's parent and the checkpoint a review reads are the latest of kind checkpoint; a base update is only ever the gate's judged head. The runner contract prose says a session must fetch the thread branch after a base_updated transition (runner handoff filed).
  6. A non-transient error on the gate's extra commit read answers a moved head, as before; only a transient one retries.
  7. Freshness is base head equals base_sha; the unreachable ancestry call was removed.
  8. StageMachine.bounded_reason/1 is the one truncation for stored reasons (gate, post-deploy verification, executor).
  9. Moduledoc arity fixed.

Mutations: 90 run sequentially, all exit 0 (cited re-runs plus T1-T13). Not provable: the executor's own wiring to bounded_reason/1, since none of its reasons can reach the bound.

@mkreyman
US-45.5: the merge executor squashes a thread's allowed checkpoint as…
ef31bd9 
… a GitHub App

A recorded thread-mode allow now enqueues ThreadMergeWorker (unique per story,
executing and retryable included), which runs MergeExecutor. It re-reads the
allow, the checkpoint the gate judges and the stage row, then squashes the
recorded checkpoint tree onto the base head as loopctl's GitHub App: tree
checked against the forge (tree_mismatch), base freshness against the allow's
base_sha, empty_change refused, merge_commit_sha recorded by compare-and-set
before the ref moves with force false. A retry whose recorded commit the base
already contains is already_merged.

A stale base or a refused fast-forward merges the base into the thread branch
as the App; Threads.record_base_update/4 (control-plane, no route) records the
base_update checkpoint and takes the new ci to ci base_updated edge in one
transaction, keeping review and custody and clearing the allow. A conflict
goes back over base_moved, or escalates claim_not_live. claim_checkpoints
judges a base_update that is, or reaches, the last allowed checkpoint.

New env vars GITHUB_APP_ID and GITHUB_APP_PRIVATE_KEY; unset, every allowed
thread story escalates app_unconfigured and nothing merges.
@mkreyman
US-45.5 review round 1: fence the merge, escalate every unresolved st…
3401bea 
…ate, CAS the base update

1. record_merge_commit is fenced under the story and stage-row locks (at ci,
   same claim, allow names the checkpoint) and fails closed; a merge whose
   stage write fails, or a recorded squash found on the base outside ci,
   escalates naming the sha (new merged_outside_ci edge from queued and the
   in-flight stages).
2. The last attempt escalates every unresolved outcome, crashes included,
   in one place (retries_exhausted).
3. Unresolvable states at ci escalate; only inapplicable ones skip.
4. A thread-mode already_merged verdict enqueues the executor, which adopts
   the recorded squash or the checkpoint commit itself.
5. Consecutive base updates of one change are bounded at 3 (base_churn).
6. The installation token asks for workflows: write as well; docs corrected.
7. The worker's uniqueness no longer includes executing jobs.
8. The base update merges into a temporary loop/ branch created at the
   checkpoint and fast-forwards the thread branch to it with force false.
9. A 422 body never leaves the adapter; only its classification does.
10. The reason bound is StageMachine.max_reason_length/0.
@mkreyman
US-45.5: mergeable_in reads the claim epoch from the stage row only
d6c74e8 
Every write that moves stories.claim_epoch rebinds the stage row in the same
transaction (follow_claim/4 on a claim, follow_release/5 on every release),
and the story is held FOR SHARE while the fence reads, so the story half of
the epoch check was redundant and could not be made to fail.
@mkreyman
US-45.5 review round 2: one run per story, recover own base updates, …
2555cd3 
…sweep lost merges

Root cause of findings 1, 2 and 7: round 1 removed executing from the merge
worker's unique states, which made overlapping runs possible. Restored; an
allow recorded during a run is caught instead by the run re-reading the allow
at its end and snoozing itself when it changed (an enqueue from inside its own
perform would be deduplicated into it).

- A losing merge_commit compare-and-set converges (skip) instead of escalating.
- A transient failure reading the recorded squash retries; only a 404 or a
  different commit mints a new one.
- A transient failure writing merged retries and the retry adopts the squash;
  the last attempt or a non-transient failure escalates naming the sha.
- The gate answers an unrecorded head whose first parent is the allowed
  checkpoint base_update_in_flight (a retry); the executor recognises its own
  unrecorded base update and adopts it only when GitHub re-merging the same base
  commit into the checkpoint yields the identical tree.
- Exits are caught like crashes on the last attempt; ThreadMergeSweepWorker
  (every five minutes) re-drives thread stories left at ci with an allow.
- A merge recorded on a row past merged raises no alarm.
- ancestor? reads the compare with per_page=1; classify_failure moved above the
  failure comment; the placed base branch has one derivation.
@mkreyman
US-45.5: a base update in flight has the executor's exact shape
3fb1bce 
The gate read any unrecorded head whose first parent was the allowed
checkpoint as the executor's base update in flight, so a claimant's ordinary
commit on top of the checkpoint waited as a transient fault instead of going
back as a moved head. One shared predicate, CheckpointSource.base_update_of/3,
now requires exactly two parents, the checkpoint first and a second on the
base branch; the gate and the executor's recovery both use it.
@mkreyman
US-45.5 review round 3: adopt a landed squash on the last attempt, es…
2bade08 
…calate from the current stage

1. The last attempt asks whether the recorded squash is on the base before
   escalating retries_exhausted, and adopts it (ci -> merged) or escalates
   naming the sha when that write fails.
2. One escalation helper chooses the edge from the story's current stage:
   merge_gate at ci, merged_outside_ci from queued or in flight for a merge,
   nothing (logged) anywhere else.
3. The sweep selects only stories whose current claim's accepted implement
   row was placed in thread mode, from the same base query as the gate's route.
4. Recovering an unrecorded head escalates or retries a forge failure; only a
   head of another shape or tree is a moved head.
5. A claimant checkpoint's parent and a review's checkpoint are the latest of
   kind checkpoint; the runner contract says a session must fetch the thread
   branch after a base update.
6. A non-transient failure of the gate's extra commit read is a moved head.
7. Freshness is base head == base_sha alone.
8. One bounded-reason function in StageMachine for the gate, post-deploy
   verification and the executor.
9. Moduledoc arity.
@mkreyman
mkreyman enabled auto-merge (squash) September 27, 2026 12:11
@mkreyman
mkreyman merged commit eb7164c into master Sep 27, 2026
16 checks passed
@mkreyman
mkreyman deleted the feature/us-45.5-merge-executor branch September 27, 2026 12:14
@mkreyman mkreyman mentioned this pull request Sep 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mkreyman