crew cr-f9bb4626 v2: 0.3.0 merge-hand-contract — manual integration with conflict resolve - #55
Conversation
Introduces a new MergeHand HandKind variant distinct from the existing Merger, with spawn-path dispatch, archetype metadata, CLI --role merge_hand support, and compose_merge_hand_prompt returning text distinct from compose_merger_prompt. The existing Merger role and its gradual-rollout semantics are untouched; sibling sparks build the real integration behaviour on top of this plumbing. ryve-10c8baee / [sp-476ef264] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…f264] Fill in the MergeHand prompt so the Hand integrates sub-PRs into the Epic branch deterministically. Ordering is pulled from the workgraph (ascending assigned_at from `ryve --json assign list <child>`, ties broken by spark id), every sub-PR is merged with `git merge --no-ff` to preserve per-PR history as a dedicated merge commit, and the HARD RULES block explicitly forbids force-push (including --force-with-lease), --no-verify, and any CI-bypass trailer (`[skip ci]`, `[ci skip]`, etc.). Spark ryve-6b261ad0 under epic ryve-476ef264.
…re event [sp-476ef264] Implement the Merge-Hand precondition checker invoked on merge_hand spawn. The checker returns a structured MergePreconditionReport with per-criterion CriterionStatus (Pass/NotApplicable/Fail) for the four contract clauses: (a) every child Assignment in Approved phase, (b) epic branch has no merge conflicts with any sub-PR, (c) CI passing on every sub-PR where CI is configured, (d) zero Assignments in Stuck phase. A Stuck child fails BOTH (a) and (d) — the Merge-Hand contract's "no partial merges" invariant means one stuck assignment anywhere in the epic blocks the spawn. Env-side criteria (b)/(c) are delegated to a dyn-safe MergePreconditionEnv trait; sibling sparks will replace the default NoopMergePreconditionEnv with real git/gh probes. On failure, spawn_hand short-circuits before any worktree is created or agent_sessions row is written, emits a single row to event_outbox under the v1 allow-listed 'epic.blocker_raised' event type (carrying the full structured report as an extra payload field for machine replay), and returns MergePreconditionsFailed. The existing outbox relay forwards the row to the epic's IRC channel without any new allow-list / renderer arms. Tests cover: pure data model (CriterionStatus, is_pass, failing, summary_reason), DB-backed checker behaviour across every criterion shape (stuck, not-approved, no-assignment, empty epic, env pass/fail, no parent), the outbox wiring, and the end-to-end gated spawn path (refused spawn emits exactly one outbox row + leaves zero agent_sessions rows). Spark ryve-5bd7749b. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd [sp-086a4432] Adds an Approved → Rejected transition gated to `merge_hand` with reason=`conflict`, locking out Head/Director overrides so conflict ownership stays with the Merge Hand by contract. Wires the Merge-Hand spawn path to drive that transition for every conflicting sub-PR surfaced by the precondition checker, then re-emits the existing `epic.blocker_raised` outbox row so IRC / replay see both halves of the handoff. The Merge Hand never resolves conflicts in place — its own spawn gate keeps refusing until every child Assignment cycles back to Approved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion [sp-9f6b0c96] Extend the MergeHand role (epic sp-476ef264) to cover the final stage of the Merge-Hand contract: the Epic PR lifecycle, the approval- and CI-gated Epic→main merge, and the Merged phase transition that closes out the Merge-Hand's own Assignment. Prompt (src/agent_prompts.rs): - Replace the skeleton-only deferral text with the full Epic PR → main workflow: collect children in Assignment-creation order, integrate sub-PRs with --no-ff, open/update the Epic PR with a body listing every child Assignment + its source PR, gate the merge on reviewDecision=APPROVED and green statusCheckRollup, execute via `gh pr merge --merge` (no --admin), and emit the Merged transition via `mark_assignment_merged`. - Re-state the forbidden primitives by name: force-push (including --force-with-lease), --no-verify, [skip ci] / [ci skip] trailers, and `gh pr merge --admin`. New unit tests lock each prohibition so a future edit can't soften the contract silently. Transition validator (data/src/sparks/transition.rs): - Lock `ReadyForMerge → Merged` to MergeHand-only by flipping `allows_override = false`, matching the Approved → Rejected conflict-handoff lockdown. Head/Director overrides are now refused on the Merged edge — the Merge-Hand contract names merge_hand as the ONLY actor permitted to mark an Assignment merged. - Add `mark_assignment_merged`, a convenience wrapper that pins actor_role = MergeHand so callers can't stamp a wrong role by accident. Mirrors `reject_approved_for_conflict`. - Unit + DB tests cover the new invariants: merge_hand succeeds, Hand / ReviewerHand are rejected, and Head/Director overrides cannot bypass the role lock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sp-51daf9b1] Pins every Merge-Hand contract acceptance clause against the data + git layers in one deterministic run: two children are walked through the transition validator to Approved with mirrored sub-PR numbers, the Merge-Hand precondition gate is replicated inline and passes, the test plays the Merge Hand to integrate both sub-branches with `git merge --no-ff` in Assignment-creation order, composes the Epic PR body using the template from `compose_merge_hand_prompt`, and drives the merge-spark's Assignment through `mark_assignment_merged`. Assertions cover: two merge commits visible on the Epic branch in creation order, Epic PR body lists both Assignment ids + spark ids + source PR numbers + the `git merge --no-ff` discipline phrase, the final `ReadyForMerge → Merged` event carries `actor_role=merge_hand`, no `epic.blocker_raised` outbox row fires on the happy path, and every phase-change event on the involved sparks appears in the expected order (both by `id` sequence and by ascending timestamp). The Merge Hand's subprocess side (git merges, `gh pr create`) is agent-driven in production and out of scope for an in-process test; this E2E stands in for the agent so the data-layer contract the agent must satisfy is pinned in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Manual conflict resolution on src/agent_prompts.rs::compose_merge_hand_prompt.
Root cause: sp-9f6b0c96 (Epic PR lifecycle) branched off main without
base-stacking onto sp-6b261ad0 (Sub-PR → Epic merge flow), so both
Hands rewrote the same compose_merge_hand_prompt body independently.
Resolution: combined both workflows into a single coherent prompt:
- sp-6b261ad0's sub-PR → Epic merge loop (WORKFLOW 1–8 + HARD RULES)
- sp-9f6b0c96's Epic PR → main lifecycle (EPIC PR LIFECYCLE A–G)
- Combined HARD RULES block with every forbidden primitive named
(assignment-creation order, --no-ff mandatory, no force-push incl.
--force-with-lease, no --no-verify, no CI bypass, no gh pr merge
--admin, conflict ALWAYS delegated via Approved→Rejected).
Tests: dropped the duplicate-named test (both sides had
`merge_hand_prompt_forbids_force_push_no_verify_and_ci_bypass`);
merged assertions from both into one comprehensive test covering
every forbidden primitive. Kept both sides' unique tests
(sub-PR-order, no-ff, Epic PR body, approval/CI gates, Merged
transition).
CI Format job runs stable `cargo fmt --check` which produced 3 diffs after the manual conflict resolution. Nightly fmt (run locally pre-push) missed them because of channel differences. Applied stable fmt; semantic content unchanged.
There was a problem hiding this comment.
Pull request overview
This PR manually integrates multiple “merge-hand-contract” child sparks into release/0.3.0, resolving a conflict by combining both the sub-PR→Epic merge workflow and the Epic PR→main lifecycle into a single coherent MergeHand implementation.
Changes:
- Introduces a new
HandKind::MergeHandrole, CLI plumbing, and a dedicatedcompose_merge_hand_prompt. - Adds a MergeHand spawn precondition gate with structured reporting, outbox emission, and conflict handoff behavior.
- Extends the assignment phase transition system with a MergeHand-only
Approved → Rejected (reason=conflict)edge and a MergeHand-only (non-overridable)ReadyForMerge → Mergedfinalize edge, plus new tests/E2E coverage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/merge_hand_e2e.rs |
New end-to-end test covering the happy-path “sub-PR → Epic branch → main” contract invariants. |
src/hand_spawn.rs |
Adds MergeHand kind, precondition checker + outbox emission, conflict handoff helper, and spawn-time gating. |
src/hand_archetypes.rs |
Marks MergeHand as write-capable and assigns archetype_id = "merge_hand". |
src/cli.rs |
Adds merge_hand to ryve hand spawn --role parsing and help text. |
src/agent_prompts.rs |
Adds the combined MergeHand prompt text (workflow + lifecycle + hard rules) and prompt tests. |
data/tests/transition_phase.rs |
Adds DB tests for conflict handoff and MergeHand-only merged finalize behavior. |
data/tests/github_applier.rs |
Updates expectations to allow refusal via Unauthorized after new transition gating. |
data/src/sparks/transition.rs |
Adds new transition rule metadata (allows_override), reason validation, and MergeHand-only helpers. |
data/src/sparks/error.rs |
Adds TransitionError::InvalidReason for reason-enforced transitions. |
| 8. Do NOT merge the Epic into `main`. The Epic branch is handed off \ | ||
| for human review.\n\n" |
There was a problem hiding this comment.
The MergeHand prompt is internally contradictory: WORKFLOW step 8 says “Do NOT merge the Epic into main”, but the later “EPIC PR LIFECYCLE” section instructs the MergeHand to merge Epic → main via gh pr merge. This can cause the agent to stop prematurely or refuse the intended merge step. Please reword step 8 to clarify that the Epic must not be merged until the PR lifecycle gates are satisfied (and/or that the merge must be done via GitHub PR, not a direct git merge), or remove step 8 entirely to avoid conflicting instructions.
| 8. Do NOT merge the Epic into `main`. The Epic branch is handed off \ | |
| for human review.\n\n" | |
| 8. Do NOT merge the Epic into `main` directly from your local \ | |
| checkout or via `git merge`. After pushing the Epic branch, hand \ | |
| it off for human review and the Epic PR lifecycle; the Epic may \ | |
| only be merged into `main` later via the GitHub PR flow once all \ | |
| required checks and approvals are satisfied.\n\n" |
There was a problem hiding this comment.
Addressed in 7db6e58 — real internal contradiction in the prompt after my manual conflict-merge. Reworded step 8 from "Do NOT merge the Epic into main" to "Do NOT merge the Epic into main DIRECTLY FROM THE SUB-PR LOOP — history-preserving Epic → main merge is driven separately by the EPIC PR LIFECYCLE section below via gh pr merge once required approvals and green CI are in." Now the two sections are consistent.
| //! 3. The Merge-Hand precondition checker is run against the merge-spark | ||
| //! with the default `NoopMergePreconditionEnv`. The gate passes — that | ||
| //! is the "spawn merge_hand" step: the gate is what refuses or admits a | ||
| //! Merge Hand subprocess, and we verify it admits here. |
There was a problem hiding this comment.
The module-level docstring says the test “runs the Merge-Hand precondition checker … with the default NoopMergePreconditionEnv” and verifies the gate admits the spawn, but the test never calls check_merge_preconditions/spawn_hand (it only re-implements two DB-level assertions with raw SQL). This makes the test description inaccurate and could mislead future maintainers about what’s actually covered. Please either update the docstring/step list to match the implementation, or restructure the test to exercise the real precondition checker (e.g., by moving the checker into a library crate that integration tests can import).
There was a problem hiding this comment.
Addressed in 7db6e58 — module doc reworded to describe the real approach: instead of calling check_merge_preconditions / spawn_hand (extensive env mocking), the test asserts the same post-conditions the gate observes via raw SQL (Assignments in approved phase, no Stuck) — same DB invariants, less mocking scaffolding.
| for child in &child_sparks { | ||
| match assign_repo::latest_assignment_for_spark(pool, &child.id).await { | ||
| Ok(asgn) => { | ||
| let phase = asgn | ||
| .assignment_phase |
There was a problem hiding this comment.
check_merge_preconditions performs one SQL query per child spark via assign_repo::latest_assignment_for_spark, which creates an N+1 query pattern (1 query to list children + N queries to fetch latest assignments). On large epics this can become noticeably slow on every merge_hand spawn. Consider fetching latest assignments for all child spark_ids in a single query (e.g., WHERE spark_id IN (...) with a GROUP BY spark_id/MAX(created_at) subquery) and then evaluating phases from that in-memory map.
There was a problem hiding this comment.
Acknowledged + documented inline in 7db6e58. N+1 is fine at v1 epic sizes (handful → few dozen children); a batched latest_assignments_for_sparks(pool, &ids) helper is tracked as a follow-up perf spark for when epics grow to 100+ children.
| if status.kind() == "fail" | ||
| && let Some(reason) = status.reason() | ||
| { | ||
| out.push((name, reason)); |
There was a problem hiding this comment.
MergePreconditionReport::failing checks status.kind() == "fail" and then calls status.reason(). Since CriterionStatus already has is_fail(), using the boolean predicate avoids stringly-typed logic and keeps the method correct if kind() formatting ever changes. Consider switching this conditional to use status.is_fail().
There was a problem hiding this comment.
Addressed in 7db6e58 — swapped status.kind() == "fail" for the typed status.is_fail() predicate. Now correct even if kind()'s string formatting ever changes.
c1 (agent_prompts.rs:1138) — WORKFLOW step 8 + EPIC PR LIFECYCLE
contradiction — REAL issue from my manual conflict-merge.
WORKFLOW step 8 said "Do NOT merge the Epic into main" but the
EPIC PR LIFECYCLE section I added just below instructs the
MergeHand to do exactly that via `gh pr merge` once approvals
and CI are green. An agent reading step 8 literally would refuse
the intended merge. Reworded step 8 to:
"Do NOT merge the Epic into main DIRECTLY FROM THE SUB-PR LOOP —
history-preserving Epic → main merge is driven separately by
the EPIC PR LIFECYCLE section below via `gh pr merge` once
required approvals and green CI are in. Never bypass that gate
with a bare `git merge` or `git push main` from this loop."
Now the prompt is internally consistent.
c2 (tests/merge_hand_e2e.rs:15) — module doc over-claimed the
test exercises `check_merge_preconditions` / `spawn_hand`, but
the test uses raw SQL to check the same DB invariants the gate
relies on (to avoid mocking the gate env). Reworded the doc to
describe the real approach and why.
c3 (hand_spawn.rs:617) — N+1 in check_merge_preconditions: one
`latest_assignment_for_spark` query per child. Acceptable at v1
sizes (handful → few dozen children); flagged inline with a
TODO pointing at a batched `latest_assignments_for_sparks(&ids)`
helper. Tracked as a follow-up perf spark.
c4 (hand_spawn.rs:463) — stringly-typed `status.kind() == "fail"`
replaced with the typed `status.is_fail()` predicate so the
check stays correct if kind()'s string formatting changes.
Local verification: cargo build --bin ryve clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
My earlier c4 fix switched the one in-tree caller from `status.kind() == "fail"` to `status.is_fail()`, which made `kind()` dead code in the release build. Clippy `-D warnings` caught this on CI. Kept `kind()` public (it's a stable stringly-typed representation intended for outbox payloads / IRC lines / CLI output) and added `#[allow(dead_code)]` with a comment pointing at the c4 context and the unit test that still exercises it. Release build clean under `-D warnings`.
…ract (#56) Three epics shipped on release/0.3.0: - ryve-12f09190 (chat-of-record) merged via PR #54 - ryve-06816a07 (channel projection) merged via PR #53 - ryve-476ef264 (merge-hand contract) merged via PR #55 (with manual bond-discipline conflict resolve) PR #56 Copilot review (3 items) addressed in 2d3846a: detect_active_seat_claim now uses a filtered SQL scan of #atlas (not a 1000-row tail cap), chat_of_record module doc accurately describes the DB-only contract (IRC wire emission deferred to 0.4.0), migration 023 comment inaccuracy documented but not edited (checksum-tracked).
Summary
Integrates the six merge-hand-contract children into release/0.3.0. The original automated Merger (Hand
9ef82731) hit a bond-discipline conflict insrc/agent_prompts.rs::compose_merge_hand_promptbetweenecho/0307f8d3(sp-6b261ad0 — Sub-PR → Epic merge flow) andecho/f61b243e(sp-9f6b0c96 — Epic PR lifecycle). Root cause: sp-9f6b0c96's Hand branched from main without base-stacking onto sp-6b261ad0's branch, so both rewrote the same function body.Manual resolution combined both Hands' workflows into a single coherent MergeHand prompt:
git merge --no-ff).gh pr mergewith--match-head-commit, Merged phase transition viamark_assignment_merged).--no-verify, CI bypass,gh pr merge --admin, conflict always delegated via Approved → Rejected).Children integrated
Local verification
Parent epic: ryve-476ef264.
🤖 Generated with Claude Code