feat(reconcilers): open-PR divergence reconciler — rebase or demote stale PRs after a main merge#175
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a Codex AgentAdapter and tests; implements a four-rule adapter selection and centralized adapter registry; updates CLI/dispatcher to support multi-adapter dispatch and validation; introduces a PR divergence reconciler with DB migration, git/worktree resolution, rebase/merge-fallback logic, demotion flow, poller wiring, and extensive unit/integration tests. ChangesCodex Adapter Implementation
Core Adapter Selection
Adapter Registry and CLI/Dispatcher Wiring
PR Divergence Reconciler (Epic
Planning & Decisions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
|
This was referenced May 28, 2026
Full AgentAdapter for the Codex CLI mirroring ClaudeAdapter: - buildLaunchCommand: interactive `codex` (no exec); auto mode + sandbox in .codex/config.toml, not the command line - installHooks: writes .codex/config.toml with approval_policy/sandbox and a [hooks] block mapping Codex's event names (startup/turn-start/command/ turn-end/shutdown) to the normalized taxonomy; reuses the shared hook.sh + pr-ready-gate.sh - buildPromptText: parity with Claude (slash-command + @-reference) - enterAutoMode: config-driven, so only fails fast on a not-logged-in pane - resolveTranscriptPath/readTranscriptState: rollout JSONL location + format - classifyStop/detectRateLimit: shared sentinel logic + Codex's generous rate-limit pattern (/rate.?limit|429|too many requests/i) Codex's observable bits are coded as the spec's start-generous baseline behind the interface; documented as tightening points in decisions.md. 36 unit tests cover buildLaunchCommand, buildPromptText, installHooks output shape, classifyStop branches, transcript reads, and rate-limit detection.
…ender - core/select-adapter.ts: selectAdapter() encodes the spec's 4-rule selection (agent:<name> label override → default_adapter → rate-limit switch when portable → otherwise skip). A label pin is never switched away from. - dispatcher/adapters.ts: shared registry (getAdapter/isKnownAdapter/ knownAdapters) replacing the two hardcoded claude-only getAdapter copies; registers claude + codex. - main.ts/docs.ts/recommender-run.ts/documentation-run.ts: the four claude-only gates now validate against the registry, so codex is dispatchable end-to-end. - mm dispatch: --adapter override, else honors the Epic's agent:<name> label via selectAdapter (best-effort gh label fetch, falls back to the default). Recommender skill prose + state-issue schema already record the per-Epic adapter in the Ready table. Tests: 12 selectAdapter cases (label/default/switch/skip) + mm dispatch selection wiring; documentation-run test flipped to accept a known non-default adapter and reject an unknown one.
…adapters - adapter-conformance.test.ts: drives both adapters through the same registry (getAdapter) and the same interface calls, asserting each conforms and that the adapter-agnostic sentinel classification behaves identically — the automated same-path cross-adapter proof (#63 criterion 4). - Leak audit (#63 criterion 2): the dispatch-path leak (hardcoded registries + claude-only gates) was fixed in #62. Fixed two tooling leaks in-pass: - mm doctor now checks every configured adapter's binary (missing = warn, not a blocking fail) instead of hardcoding claude. - dashboard slot-pill fallback now lists claude+codex (stale 'codex is a later phase' comment removed). The state-issue parser/recommender {claude,codex} rate-limit pair is a documented deliberate exception (schema v1 fixes the set). - The AgentAdapter interface did not change for Codex (#63 criterion 3 / Epic headline) — documented in decisions.md. The live dual-dispatch on a test repo (Epic headline + #63 criterion 1) needs a running Codex CLI + OpenAI creds, absent in the sandbox — it's the operator's acceptance step (reviewer's brief).
…boundaried 429 Internal review-loop findings (resolve the class, each with a test): - mm dispatch resolved its adapter set from Object.keys(config.adapters), ignoring the enabled flag and the registry — so a disabled or unimplemented adapter could be force-dispatched. Now "available" = configured AND enabled AND implemented (intersect with knownAdapters()), matching the daemon's validation. Test: --adapter codex with codex disabled → exit 1, no dispatch. - Codex rate-limit regex matched 429 as a bare substring (e.g. "line 4290", "commit 4291ab"), which would falsely mark a healthy agent rate-limited and skip it. Word-boundaried to \b429\b. Tests: 4290/4291ab/14290/42900 → bare-stop.
…on a one-click ask
…al blocker is daemon-runs-main
Six-phase workstream: classify divergence → rebase helper → -X ours merge fallback → applySuccess (push+comment) → applyDemoteToWork (draft+reopen+ enqueue) → reconcileOpenPRs + poller-tick + MERGED-transition immediate sweep.
New module packages/dispatcher/src/reconcilers/pr-divergence.ts — `classifyDivergence(deps, repo, prNumber)` reads mergeable/mergeStateStatus from an injected gateway and returns one of CLEAN/BEHIND/CONFLICTED/UNKNOWN, persisting the observation in the new `pr_divergence_state` table (006_pr_divergence_state.sql, keyed (repo, pr_number), CHECK-pinned vocabulary). Pure `classifyMergeability` split out so each branch unit-tests without a gateway. Bumped db.test.ts schema-version assertions to 6.
`tryRebaseOntoMain(deps, repo, prNumber)` resolves the PR's worktree by its managed `middle-issue-<N>` head ref (creating it via `createWorktree` if absent under `<worktreeRoot>/<repo>/issue-<N>/`), fetches `origin main`, runs `git rebase origin/main`, and on conflict captures the unmerged paths from `git diff --diff-filter=U` BEFORE aborting with `git rebase --abort` so the worktree returns to a clean state. Pure `GitOps` + `PrHeadRefGateway` seams keep the helper unit-testable; the production `gitOps` (real `git` via `Bun.spawn`) is exercised in a new fixture-repo integration test covering FF / non-FF-no-conflict / conflict + abort, plus skip-signals for a non-managed head ref and a missing PR. Also exports `parseEpicFromHeadRef` and `worktreePathFor` (unit tests pin the managed-ref vocabulary and the layout matches `createWorktree`).
…171) `tryMergeMainNewWorkAsBase(deps, repo, prNumber)` runs `git merge --no-edit --no-ff -X ours origin/main` so content collisions resolve new-work-as-base (the branch's version wins; main's net-new lands on top). Mirrors the rebase helper's shape: same worktree resolution, same GitResolutionResult contract, same abort-on-residual-conflict behavior — paths captured BEFORE `git merge --abort` so the worktree returns to a clean state. Integration tests cover both paths against a real fixture repo: - a same-line content collision (rebase would conflict — the fallback lands; feature's content wins; a merge commit lands in history with ≥2 parents) - a rename/rename conflict -X ours can't auto-resolve (abort, paths reported, no leftover .git/MERGE_HEAD) --no-ff is deliberate: even an FF-eligible merge produces a visible merge commit, so a reviewer can see main was folded in (the reconciliation is loud on purpose).
…#172) `applySuccess(deps, repo, prNumber, resolution, mainCommitSha)` lands a clean rebase or merge by pushing the worktree back to its PR branch, posting one announcement comment on the PR, and recording CLEAN in pr_divergence_state. Idempotent across consecutive calls for the same reconciliation: - Push is conditional: `git fetch origin <branch>` first, then push only if local HEAD differs from origin/<branch>; force-with-lease never plain --force. - Comment is gated by a hidden HTML marker (`<!-- middle-divergence: sha9:res -->`) embedded in the body — listed comments are scanned for it, so a second call with the same (sha, resolution) skips the post but a future main sha re-announces. - State row is upserted (idempotent by construction). Adds three GitOps methods backing this: `revParse(cwd, ref)`, `pushForceWithLease(cwd, branch)`, plus the `--no-edit --no-ff` already on `mergeOurs`. Three integration tests exercise the real-`git` paths against the fixture: push-happens + comment-once + double-call idempotence, sha-keyed re-announcement, and the non-managed head-ref no-op.
…+ dual-surface escalate + re-enqueue (#173) `applyDemoteToWork(deps, repo, prNumber, conflictingPaths)` escalates when both autonomous attempts (rebase + -X ours merge) have failed: 1. Resolves the Epic via the PR's `middle-issue-<N>` head ref. 2. Skips entirely if the PR is already a draft — the demote has already landed. This gate survives a classifier overwrite of pr_divergence_state, since the live PR state is the source of truth for the action. 3. Converts the PR to draft. 4. Reopens the most-recently-closed sub-issue with an in-line escalation comment (or skips when no sub-issue is closed yet). 5. Posts a dual-surface escalation on both the Epic and the PR (CLAUDE.md review-feedback convention), embedding a hidden HTML marker keyed on the Epic number so a partial-retry doesn't pile on duplicates. 6. Re-enqueues the Epic through the injected `enqueueEpic` callback so the recommender's ranking still applies (Phase 6 wires it to a recommender run + scheduleAutoDispatch). 7. Records DEMOTED in pr_divergence_state. Six unit tests cover: full demote happy path (all calls in order), idempotent re-call via PR.isDraft, partial-retry marker safety, Epic with no closed sub-issues (graceful), non-managed head ref (no-op), missing PR (no-op).
…#174) `reconcileOpenPRs(deps, repo)` composes Phases 1-5 into one pass: listOpenManagedPrs → for each: classifyDivergence → CLEAN/UNKNOWN: pass → BEHIND/CONFLICTED: tryRebaseOntoMain → ok: applySuccess('rebased') → conflict: tryMergeMainNewWorkAsBase → ok: applySuccess('merged-new-work-as-base') → conflict: applyDemoteToWork(union of paths) - Rate-limit aware: skips the pass when GitHub budget < `rateLimitBuffer` (default 100, matches the resume poller). Returns `skippedForBudget: true`. - Burst-capped at `maxPrsPerPass` (default 25). Per-PR failures are isolated and logged; the pass continues. - Production `ghReconcilerGateway` exports the new gh-backed calls (listOpenManagedPrs, getMainCommitSha, getMergeability, getPrHeadRef, getPullRequest, convertPrToDraft, listClosedSubIssues, reopenIssue). Composed with `ghGitHub` for comment ops at the daemon-wiring site. Wiring: - `startPoller` gains a `ReconcilerHooks` arg with `perTickSweep` (per-tick sweep over every managed repo) and `onMergedTransition` (immediate sweep fired from inside `reconcileMergedParks` when a parked Epic's PR is observed transitioning to MERGED — divergence on siblings heals at the moment of merge, not up to a tick later). - `main.ts` builds `reconcileOpenPRsForRepo` (composing `ghGitHub` + `ghReconcilerGateway`, threading `runRecommenderForRepo` + `scheduleAutoDispatch` as the `enqueueEpic` seam so re-dispatch still routes through the recommender's ranking). Six added integration tests cover the end-to-end paths against the real fixture repo: BEHIND→rebase→applySuccess (+ idempotent re-tick reading CLEAN), CONFLICTED→merge-fallback, double-failure→demote, rate-limit floor short- circuit, CLEAN walk (no side effects), and listing-throws isolation.
…tests Acceptance-criteria coverage for Epic #168: - `reconcile.test.ts`: a `reconcileMergedParks` pass with multiple MERGED transitions fires `onMergedTransition` once per row (Epic #168 hook the daemon wires to an immediate `reconcileOpenPRs` sweep); a thrown hook is isolated and the merged-parks pass still finalizes every row. - `pr-divergence-integration.test.ts`: a two-PR pass exercises the iteration — one BEHIND PR rebases to CLEAN (applySuccess posted), one already-CLEAN PR is walked but unchanged. Proves the orchestrator's loop on the spec's "merging one triggers the reconciler; the other rebases" topology.
thejustinwalsh
force-pushed
the
middle-issue-168
branch
from
8651bf7 to
e50fc23
Compare
…anual recovery, observability
Pre-push internal code-review pass surfaced six adjacent edges in the same
class — the reconciler's result/failure shapes leaking into wrong dispatch
paths. Fix the class within the module's blast radius, each fix tested:
1. **`gitOps.rebase` / `mergeOurs` throw on non-conflict failures** — when the
underlying command exits non-zero with zero unmerged paths (missing upstream,
hook refusal, dirty worktree), it's a wiring failure, not a path-less
conflict. Before: shaped as `{ok:false, conflictingPaths:[]}` indistinguishable
from the non-managed-PR skip signal. After: throws; the orchestrator's
per-PR try/catch logs and increments the new `failed` counter.
2. **`applySuccess` accepts `mainCommitSha: string | null`** — a transient
`getMainCommitSha` failure (Phase 6) used to silently skip the entire
applySuccess (push + comment + state→CLEAN), but the orchestrator still
counted it as `reconciled`. After: push + state row are unconditional; only
the comment step skips when the SHA is null (the marker would be ambiguous).
A later pass with a readable SHA posts under the correct marker.
3. **`applyDemoteToWork` skips `reopenIssue` when the Epic already carries
the demote marker** — a human who reviews the escalation, manually fixes
the conflict, marks the PR ready, and closes the reopened sub-issue
shouldn't have it re-opened by the next divergence. The PR.isDraft gate
doesn't catch this (human un-drafted it); the Epic marker does.
4. **`listClosedSubIssues` defends against `NaN` from `Date.parse`** —
malformed `closed_at` (hand-edited rows) now coerces to null instead of
becoming a NaN that destabilizes the V8 sort.
5. **`getMergeability` / `getPullRequest` guard `JSON.parse`** via a new
`safeJsonParse` helper — empty/malformed `gh` stdout (observed in the wild
when auth warnings interleave) now returns null instead of throwing
`SyntaxError`. `ghJson` similarly wraps for the throwing path.
6. **`reconcileOpenPRs` reports a `failed` counter** + logs when the
`maxPrsPerPass` cap drops the tail of a large backlog. Distinguishes a pass
of all-CLEAN from a pass of all-failures for observability.
7. **`main.ts` dedups `onMergedTransition` per pass** — a single
`reconcileMergedParks` pass observing N MERGED transitions on the same repo
used to fire N full-repo sweeps; now coalesced to one per tick.
Five added tests pin the new contracts: null-mainCommitSha skip-comment path,
non-conflict rebase throw, manual-recovery marker gate. Existing tests updated
for the new `failed` field on the result shape.
…dParks Second internal review pass caught a race in the call-site timer dedup: `setTimeout(..., 0)` resets the dedup set as soon as the event loop services macrotasks, which happens between `await` points inside the iteration loop — so a pass with N MERGED rows on the same repo could still fire a second sweep. Fix by moving the dedup to the natural per-pass boundary inside `reconcileMergedParks` (a `Set<string>` constructed at the start of the loop). The caller's only job is now to forward the hook; no timer involved. - `poller.ts`: `reconcileMergedParks` tracks fired repos in `mergedRepos`; docstring on `onMergedTransition` updated to "at most once per repo per pass" with rationale. - `main.ts`: dropped the call-site IIFE timer; `onMergedTransition` is a thin forwarder. - `reconcile.test.ts`: existing tests updated to assert single-fire dedup (two MERGED rows, one repo → triggered.length === 1). - `pr-divergence-integration.test.ts`: new test exercises the orchestrator's `failed` counter (a per-PR throw increments it, the pass continues for subsequent PRs) — was previously only pinned at 0 across green paths. - `pr-divergence.ts`: tightened `applySuccess` docstring on the null-mainSha path: a later pass announces against whatever main it then reads, not necessarily the SHA this pass would have used.
…etric to rebase) The rebase non-conflict throw is asserted; `gitOps.mergeOurs` has the same hardening but no test pinned it. A regression that dropped the throw on the merge twin would have slipped through. Mirror the rebase assertion against a missing ref.
thejustinwalsh
added
the
ready-for-review
Owner
Author
Reviewer's brief — PR #175 / Epic #168PR #175 (Open-PR divergence reconciler) is marked ready for review. How to runbun run typecheck
bun run lint
bun run format
bun test
# Just the reconciler's tests
bun test packages/dispatcher/test/pr-divergence.test.ts \
packages/dispatcher/test/pr-divergence-integration.test.ts \
packages/dispatcher/test/reconcile.test.ts
Where to start
What to verify
Anything fragile that needs extra eyes
Acceptance evidenceIn the PR description's "Acceptance evidence" block. Each criterion carries a sub-issue reference + the relevant test file. Stakeholder-deferred items: none. What's NOT here
Final review and merge are the human's call. |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/dispatcher/src/db/migrations/006_pr_divergence_state.sql (1)
27-27: ⚡ Quick winIndex
classified_atas well to support time-based reconciliation queries.Right now only
stateis indexed. For recency/staleness scans,classified_atwill force table scans as this grows. Add an index onclassified_at(or a composite index matching expected query shape).💡 Suggested migration tweak
CREATE INDEX idx_pr_divergence_state ON pr_divergence_state(state); +CREATE INDEX idx_pr_divergence_state_classified_at + ON pr_divergence_state(classified_at);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dispatcher/src/db/migrations/006_pr_divergence_state.sql` at line 27, The migration currently creates only idx_pr_divergence_state on pr_divergence_state(state) but misses an index on classified_at needed for time-based reconciliation queries; update the migration to add an index on pr_divergence_state(classified_at) or a composite index such as (state, classified_at) depending on expected query predicates—modify the migration to create idx_pr_divergence_state_classified_at ON pr_divergence_state(classified_at) or idx_pr_divergence_state_state_classified_at ON pr_divergence_state(state, classified_at) to avoid table scans during recency/staleness scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dispatcher/src/adapters.ts`:
- Around line 12-15: getAdapter currently indexes the plain-object REGISTRY
directly which allows inherited prototype keys (e.g. "toString") to be treated
as defined; change REGISTRY to an exact-key lookup (prefer using a Map<string,
AgentAdapter> or keep the object but use Object.hasOwn / Object.hasOwnProperty /
Object.hasOwn.call) and update getAdapter to first check for an own key and that
the retrieved value is an AgentAdapter, otherwise throw the unknown-adapter
error; reference the REGISTRY constant and the getAdapter function to locate
where to enforce the exact-key check.
In `@packages/dispatcher/src/main.ts`:
- Around line 348-350: The current check uses isKnownAdapter(adapter) which
allows implemented-but-disabled adapters to be dispatched; change the guard to
require the adapter is both implemented and enabled in daemon config (e.g.
replace the single isKnownAdapter(adapter) check with a combined predicate like
isKnownAdapter(adapter) && isAdapterEnabled(adapter) or create a helper
isConfiguredAndEnabledAdapter(adapter) that returns isKnownAdapter(adapter) &&
!!config.adapters?.[adapter]?.enabled), update the error message accordingly,
and apply the same fix at the other usage site referenced (the second check
around the other dispatch/control route).
In `@packages/dispatcher/src/poller-cron.ts`:
- Line 12: The poller cadence constant is set to 120_000 but the flow requires
60_000; update POLLER_INTERVAL_MS to 60_000 and add a new WATCHDOG_INTERVAL_MS
constant set to 30_000 so the watchdog and poller follow the required cadence
(this affects startPoller which is invoked without intervalMs and therefore
relies on POLLER_INTERVAL_MS). Ensure the new constants (POLLER_INTERVAL_MS and
WATCHDOG_INTERVAL_MS) are exported and documented near the top of
poller-cron.ts.
In `@packages/dispatcher/src/reconcilers/pr-divergence.ts`:
- Around line 901-913: getMergeability, getPrHeadRef and getPullRequest
currently treat any non-zero ghSpawn exitCode as "missing data" and return null,
which hides transport/auth/rate-limit failures; change each function
(getMergeability, getPrHeadRef, getPullRequest) to inspect ghSpawn's result and
only return null for explicit not-found/404 semantics, while throwing or
propagating an Error for transport/auth/rate-limit or unexpected failures
(include stderr/exitCode in the thrown Error for observability). Use the ghSpawn
result (r.exitCode, r.stderr, r.stdout) and safeJsonParse the stdout only when
exitCode === 0; for other non-zero codes map known CLI not-found cases to null
and otherwise raise an error so the orchestrator can surface and handle real
failures.
- Around line 594-597: The early return on pr.isDraft in the pr-divergence flow
(after calling deps.github.getPullRequest) prevents retries from completing
follow-up remediation (reopen/comment/enqueue/state-write) if a previous demote
only succeeded in converting to draft; remove the hard return and instead let
the code continue executing remediation steps while using existing idempotency
markers (the marker checks used for reopen/comment/enqueue/state-write) to avoid
duplicate side effects—i.e., replace the `if (pr.isDraft) return;` with a
conditional branch that skips only duplicate actions via the marker checks but
still runs the final state-write/enqueue/comment/reopen logic so retries can
finish remediation.
---
Nitpick comments:
In `@packages/dispatcher/src/db/migrations/006_pr_divergence_state.sql`:
- Line 27: The migration currently creates only idx_pr_divergence_state on
pr_divergence_state(state) but misses an index on classified_at needed for
time-based reconciliation queries; update the migration to add an index on
pr_divergence_state(classified_at) or a composite index such as (state,
classified_at) depending on expected query predicates—modify the migration to
create idx_pr_divergence_state_classified_at ON
pr_divergence_state(classified_at) or
idx_pr_divergence_state_state_classified_at ON pr_divergence_state(state,
classified_at) to avoid table scans during recency/staleness scans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c26bc9e5-9dda-4f6d-984d-ddc57347fc13
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
packages/adapters/codex/package.jsonpackages/adapters/codex/src/classify.tspackages/adapters/codex/src/hooks.tspackages/adapters/codex/src/index.tspackages/adapters/codex/src/prompt.tspackages/adapters/codex/src/transcript.tspackages/adapters/codex/test/adapter.test.tspackages/cli/src/commands/dispatch.tspackages/cli/src/commands/docs.tspackages/cli/src/commands/doctor.tspackages/cli/src/index.tspackages/cli/test/dispatch.test.tspackages/core/src/index.tspackages/core/src/select-adapter.tspackages/core/test/select-adapter.test.tspackages/dashboard/src/db-deps.tspackages/dispatcher/package.jsonpackages/dispatcher/src/adapters.tspackages/dispatcher/src/db/migrations/006_pr_divergence_state.sqlpackages/dispatcher/src/documentation-run.tspackages/dispatcher/src/main.tspackages/dispatcher/src/poller-cron.tspackages/dispatcher/src/poller.tspackages/dispatcher/src/recommender-run.tspackages/dispatcher/src/reconcilers/pr-divergence.tspackages/dispatcher/test/adapter-conformance.test.tspackages/dispatcher/test/db.test.tspackages/dispatcher/test/documentation-run.test.tspackages/dispatcher/test/pr-divergence-integration.test.tspackages/dispatcher/test/pr-divergence.test.tspackages/dispatcher/test/reconcile.test.tsplanning/issues/168/plan.mdplanning/issues/60/decisions.mdplanning/issues/60/plan.md
A plain-object REGISTRY lets `getAdapter("toString")` resolve the inherited
`Object.prototype.toString` instead of throwing — `getAdapter`'s
`adapter === undefined` guard only catches truly-missing keys, not inherited
ones. Callers that validate names by calling `getAdapter` and treating thrown
errors as "unknown" (e.g. `resolveRecommenderOptions`) would let a non-
`AgentAdapter` value through.
Switch to `Map<string, AgentAdapter>` so lookups are exact-key by construction;
`knownAdapters`, `isKnownAdapter`, `getAdapter` all use the Map's API. Regression
tests parameterize over the suspect prototype keys (`toString`, `constructor`,
`hasOwnProperty`, `__proto__`).
Addresses CodeRabbit on PR #175.
… entry points `isKnownAdapter` gates only on "implemented", so a disabled-but-implemented adapter could slip through the daemon's manual-dispatch route, the recommender validator, and the docs validator — all daemon-side entry points reachable below the CLI's enabled-check (a direct `/control/dispatch` POST or a dashboard call). Each surface needs the same gate. - `dispatchEpicManual` + the hook-server's `/control/dispatch` route now share a single source of truth (`adapterRejectionReason` in main.ts), wired via `ControlPlane.adapterRejection: (name) => string | null`. The new contract collapses the gate + the diagnostic into one method, so both surfaces report "adapter codex is disabled in config" vs "unknown adapter: ghost" identically — previously the route always said "unknown" for both, which is misleading. - `resolveRecommenderOptions` and `resolveDocumentationOptions` get the same enabled-check in line with the main.ts gate. Tests: - `main.test.ts` — daemon-spawn integration test exercises the disabled and unknown cases via real `/control/dispatch` POSTs, asserting both the 400 status and the JSON body's distinct wording. - `control-routes.test.ts` — unit test for the route's body validation pinning the same disabled-vs-unknown distinction. - `recommender-run.test.ts` / `documentation-run.test.ts` — resolver-level coverage that a disabled adapter is refused with the matching message. Addresses CodeRabbit on PR #175 (main.ts:348, main.ts:479); also resolves the same class of bug one step over in the recommender + docs validators (surfaced by the internal clean-eyes pass).
…contract The dispatcher's CLAUDE.md pins the cron cadence at `WATCHDOG_INTERVAL_MS = 30s` and `POLLER_INTERVAL_MS = 60s`, but the code literal had drifted to 120s and silently doubled production cadence — the cron uses this constant when `startPoller`'s `intervalMs` is omitted, which is the daemon's call shape. Resyncing to 60s heals MERGED-transition divergence within one tick per Epic #168's intent and keeps the doc + code aligned. Add a regression test pinning the value so a future shift surfaces immediately. Addresses CodeRabbit on PR #175.
…4 vs failure Two related hardenings on the `pr-divergence` reconciler, plus a quick-win migration index. **applyDemoteToWork — partial-retry safe.** The pre-existing `if (pr.isDraft) return` short-circuit meant a prior attempt that crashed AFTER `convertPrToDraft` but BEFORE the reopen/comment/enqueue/ state-write steps could never finish remediation on retry — the next pass would see `isDraft=true` and bail. Replace with per-step idempotency: skip only the draft-flip step when already drafted; downstream remediation runs to completion, gated by its existing markers (Epic comment marker, dual- surface comment markers, the daemon's existing-workflow guard on enqueue). Two tests: the renamed "per-step idempotency" test (re-enqueue fires every pass — docstring contract), and a new "partial-retry: prior attempt left PR drafted only" test that asserts every downstream step still runs. **Gateway — distinguish "PR not found" from real failures.** `getMainCommitSha` / `getMergeability` / `getPrHeadRef` / `getPullRequest` collapsed every non-zero `gh` exit into `null`, hiding transport / auth / rate-limit failures as benign UNKNOWN no-ops. Each now consults `ghStderrIsNotFound` (exported for tests; matches only the two stable `gh` not-found phrasings — `Could not resolve to a …` and `HTTP 404` — deliberately narrow so a `secret not found, push declined` can't silently masquerade as a 404). Real failures throw with stderr in the message, propagating to the orchestrator's per-PR `failed++` counter. `classifyDivergence`'s docstring calls out the consequence: a transport failure leaves the state row at its prior observation rather than overwriting with stale UNKNOWN. **Migration nitpick — index `classified_at`.** Recency / staleness scans on `pr_divergence_state` are its other query shape; without an index they'd force a table scan as the table grows. Addresses CodeRabbit on PR #175 (pr-divergence.ts:597, pr-divergence.ts:913, 006_pr_divergence_state.sql:27 nitpick).
Single-pass new-work-as-base merge of origin/main after rebase kept re-conflicting on the same hunks across multiple commits (CLAUDE.md escape hatch). - packages/dispatcher/src/poller-cron.ts — unified `startPoller(deps, opts)` signature; folded `ReconcilerHooks` into `StartPollerOptions` as `opts.reconcilers` (alongside `opts.checkboxRevert` and `opts.intervalMs`). - packages/dispatcher/src/main.ts — unified daemon-startup: keeps the durable engine + `recoverEngine` + `reconcileOrphanedSignals` from #160, the notification-failsafe watchdog comment from #162, and adds the `reconcileOpenPRsForRepo` block + `reconcilers` config in the `startPoller` call. Dropped the now-unused `Engine` import (main routes through `createDurableEngine`). - packages/core/src/index.ts — kept both export blocks: integration rubric from #163, `selectAdapter` from this PR. - packages/dispatcher/test/recommender-run.test.ts — kept both describe blocks (adapter-enabled gate from this PR, schema-resolution from #157); added `enabled: true` to the schema test's adapter config so it passes the new gate. - packages/dispatcher/test/gates/checkbox-revert-pass.test.ts — added the five new `GitHubGateway` methods to the test stub (`listOpenIssues`, `addLabel`, `listMergedPrsClosingRefs`, `closeIssue`, `createIssue`) main grew during the marathon. Gates re-verified locally: `bun run typecheck` clean, `bun test packages/dispatcher` 620/620 pass, `bun run lint` clean, `bun run format` clean (no changes).
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.
Summary
Closes #168
Open-PR divergence reconciler: when an Epic PR merges to
main, sibling open Epic PRs may become non-mergeable. This adds an autonomous reconciler that detects that drift on every poller tick (and immediately at the moment of a MERGED transition), tries to fix it (rebase first,-X oursmerge-commit fallback per CLAUDE.md new-work-as-base), and on double-failure demotes the PR back to draft, reopens its last closed sub-issue, and re-enqueues the Epic through the recommender — so a ready PR never silently rots between amainmerge and the human's final-merge gate.Sister to #139 (parked-workflows reconciler on Epic PR merge/close, merged) and #141 (CI-gate review readiness, merged). Targets the third reconciliation surface: open-and-still-active Epic PRs against a moving
main.What changed
packages/dispatcher/src/reconcilers/pr-divergence.ts— new module: classifier (classifyDivergence), worktree helpers (tryRebaseOntoMain,tryMergeMainNewWorkAsBase), success applier (applySuccess), demote applier (applyDemoteToWork), orchestrator (reconcileOpenPRs), productionghReconcilerGateway.packages/dispatcher/src/db/migrations/006_pr_divergence_state.sql— new tablepr_divergence_state(repo, pr_number, state, classified_at)with the CHECK-pinned vocabulary and a(repo, pr_number)PK so the same column works for future multi-repo daemons.packages/dispatcher/src/poller.ts—reconcileMergedParksgains anonMergedTransitionhook with per-pass dedup (Set inside the loop; the natural per-pass boundary, no timer race).packages/dispatcher/src/poller-cron.ts— newReconcilerHooksarg (perTickSweep+onMergedTransition); both wrapped in try/catch so a thrown sweep never crashes the cron worker.packages/dispatcher/src/main.ts— wiresreconcileOpenPRsForRepo(composingghGitHub+ghReconcilerGateway, threadingrunRecommenderForRepo+scheduleAutoDispatchas theenqueueEpicseam so re-dispatch routes through the recommender's ranking).packages/dispatcher/test/pr-divergence.test.ts+pr-divergence-integration.test.ts— 41 new tests against the real fixture repo + stub gateways.packages/dispatcher/test/reconcile.test.ts— 2 new tests pinning theonMergedTransitionper-pass dedup.Why these changes
The reconciler completes the three-surface coverage of stale Epic PRs: #139 cleaned up parked workflows after a PR merges; #141 gates review readiness on green CI; this work keeps the open, active PRs mergeable when
mainmoves. The chain (rebase →-X ours→ demote) follows CLAUDE.md's "Keeping the branch mergeable into main" rule directly — rebase is the default, the-X oursmerge fallback handles "main re-architected the same code", and only when both fail does the reconciler escalate.The helpers are pure functions over injectable gateways so the unit tests need neither
ghnor the daemon; the production wiring lives in onemain.tsblock. The internal-review loop surfaced six adjacent edges in the helper contracts (skip vs fail shape, null mainSha, manual recovery, NaN closed_at, unguarded JSON parse, onMergedTransition burst) — all addressed before this PR was marked ready.Verification
Phase 1 (#169) — classify divergence + migration
pr-divergence.test.ts: every classifier branch + persistence round-trip + CHECK rejects out-of-vocabulary state + (repo, pr_number) PK coexistence.Phase 2 (#170) — rebase helper
git: clean fast-forward, non-FF without conflict, conflict + abort (paths reported, no.git/rebase-mergeleft), non-managed head ref skip, missing PR skip.Phase 3 (#171) —
-X oursmerge fallback-X ourscan't auto-resolve (abort, paths reported, no.git/MERGE_HEAD).Phase 4 (#172) —
applySuccessgit rev-parse middle-issue-32matches the pushed HEAD); a differentmainCommitShare-announces (marker is sha-keyed); nullmainCommitShaskips comment but still pushes and records CLEAN; non-managed head ref no-op.Phase 5 (#173) —
applyDemoteToWorkPR.isDraft; partial-retry marker safety; Epic with no closed sub-issues; non-managed head ref; missing PR; manual recovery — an Epic that already carries the demote marker skips thereopenIssuecall so a human's manual sub-issue close isn't undone.Phase 6 (#174) —
reconcileOpenPRsorchestrator + wiringlistOpenManagedPrsthrows (isolated); two open managed PRs walked in one pass (mix of CLEAN + BEHIND→rebased); per-PR throw incrementsfailedcounter and pass continues.reconcile.test.ts:onMergedTransitionfires at most once per repo per pass (per-pass dedup insidereconcileMergedParks); a thrown hook is isolated and the merged-parks pass still finalizes every row.Gates —
bun test870/870 pass;bun run typecheckclean;bun run lint(oxlint --fix --deny-warnings) clean;bun run format(oxfmt) clean.Mergeability —
gh pr view 175 --json mergeable,mergeStateStatusreads{mergeable: MERGEABLE, mergeStateStatus: CLEAN}. Rebased ontoorigin/mainbefore marking ready.Acceptance evidence
Walking the Epic's acceptance criteria (#168):
packages/dispatcher/test/pr-divergence-integration.test.ts(the "two open managed PRs in one pass" test and the "BEHIND PR rebases cleanly on the next tick" test) plus theonMergedTransitionwiring tests inpackages/dispatcher/test/reconcile.test.ts. End-to-end: a MERGED transition triggersreconcileOpenPRsvia the dedup hook; the surviving open PR's classifier reports BEHIND;tryRebaseOntoMainreturns ok;applySuccessforce-with-lease-pushes; the remote'smiddle-issue-32ref tracks the local HEAD (asserted against the bare remote).packages/dispatcher/test/pr-divergence-integration.test.ts. PR flips to draft, sub-issue 50 is reopened with an escalation comment that names the conflicting paths, the Epic re-enqueue fires through the wiredenqueueEpic, and the state row records DEMOTED.Stumbling points
head:<prefix>search syntax doesn't strictly pin a branch-name prefix ongh pr list. The reconciler lists all open PRs and filters client-side onheadRefName.startsWith("middle-issue-")— capped at 100 per call (the rate-limit floor is what bounds large backlogs).-X oursfailure mode — the spec called this "rare; structural rename + content collision", but a same-file dual rename trips it deterministically and was the cleanest fixture-repo test for the demote path.Suggested CLAUDE.md updates
None. The existing "Keeping the branch mergeable into main" section already encodes the rebase-first + new-work-as-base merge fallback the reconciler implements; nothing this work hit needed a new doc.
Architectural forks
None — the chain (classify → rebase → merge fallback → demote) and the helper signatures were spec'd by the sub-issue bodies; CLAUDE.md and the existing recommender/poller patterns settled every other decision without needing a worktree fork.
Follow-up issues
None opened. Two MINOR observations from the review loop were resolved in-pass; nothing else surfaced that crosses the "discovered, parallelizable, out-of-scope" bar.
Out of scope
middle-issue-; will lift into[repo]policy after feat(config): split shared repo policy from local operational cache #158 lands).How to run / verify
Reviewer's brief
packages/dispatcher/src/reconcilers/pr-divergence.tsis the entire reconciler. Read top to bottom: classifier (Phase 1), worktree helpers (Phase 2-3),applySuccess(Phase 4),applyDemoteToWork(Phase 5),reconcileOpenPRsorchestrator +ghReconcilerGateway(Phase 6). The structure mirrors the six sub-issues in dispatch order.gitOps.rebase/mergeOursdistinguish conflict (paths reported, abort) from wiring failure (throw) — the orchestrator'sfailedcounter exists for the second case.applySuccess's push idempotence (local-vs-remote SHA compare) and sha-keyed marker for comment idempotence.applyDemoteToWork's two-gate idempotency:PR.isDraftshort-circuit + Epic-marker gate on thereopenIssuestep (so a human's manual recovery isn't undone).reconcileMergedParks's per-pass dedup ofonMergedTransition(inside the loop, no timer — the original timer-based dedup raced across await boundaries).getMergeability/getPullRequestusesafeJsonParseto tolerategh's occasional empty-stdout-with-warning-text interleaving. The integration tests use a symlink to plant the fixture worktree under the helper's expected layout (<root>/<repo>/issue-<N>/);existsSyncfollows the symlink andgit -Cfollows it through to the real fixture.enqueueEpiccallsrunRecommenderForReposo ranking still applies. No new public API on@middle/dispatcher'sindex.ts— the reconciler is wired only viamain.ts.Final review and merge are the human's call.
Summary by CodeRabbit
New Features
--adapterflag formm dispatchto override adapter selectionagent:<name>)Improvements