plans: reconcile 'implementing' plans orphaned by a daemon restart

[oranjeai]

Description

A plan in implementing asserts a live in-process implementation run, but that
run is a bare asyncio.create_task. A SIGKILL / OOM / restart destroys it and
nothing reconciles plans at startup, so the row keeps asserting an obligation
nobody holds and the plan is terminally wedged: re-approval is refused (both
surfaces gate on pending), plan_propose is refused forever for that task
(get_pending_plan_task_ids counts implementing), and nothing but a
hand-crafted PATCH /api/plans/{id} recovers it.

Workflow runs made the same RAM-only trade and paired it with a startup recovery
pass (WorkflowRunService.start); sessions have one too
(recover_orphaned_sessions). Plans were the holdout. This adds the same
guarantee: get_implementing_plans + fail_orphaned_plan (a status-only CAS) in
db/plans.py, recover_orphaned_plans in plan_service.py, one call in the
gateway lifespan. failed, not pending: it is already what both approval
surfaces write when engine.run raises, and it unblocks plan_propose without
blindly re-running a plan over partial effects. No migration,
no new status, no frontend diff (the plan pages already style failed).

Plans whose implementation session is enrolled for resume and still eligible are
left implementing. That exclusion is correctness, not caution:
nerve restart --resume <impl-sid> is supported, and a resumed session completes
through task_done, which closes only an implementing plan -- sweeping it
would leave that session unable to ever close its own plan. Eligibility is
re-checked against the engine's own four skip predicates, since a skipped session
never resumes. The queue is only read, never drained.

Ordering is load-bearing: the pass runs before cron.start(), whose catch-up
can dispatch a planner run that would read a stale row and skip the task.

Validation

tests/test_plan_restart_recovery.py (18). Both directions against a base
export: 18/18 fail there, two behaviourally (the seeded plan is still
implementing after startup, and when cron starts); the other 16 fail on API
shape, weaker evidence. 19 mutants killed. Full suite: identical 7 pre-existing
failures before and after, +18 passed.

Out of scope, deliberately

• A plan reconciled to failed is not re-adopted if its session is later
  resumed: the task still completes, but the row keeps reading failed where
  today it would read done. Re-adopting needs durable provenance -- a
  legitimately failed plan also keeps its impl_session_id.
• Settling a plan whose resumed session ends without task_done (outcomes exist
  only after a full agent turn, unorderable before cron.start()).
• The task/plan mismatch on a late approval failure; task_done not enforcing
  implementation-session ownership; the plans-page filter list lacking failed
  and done (all pre-existing).

No related open issue found.

[oranjeai]

Internal second-model review (2 rounds, 5 findings adjudicated: 4 agreed and fixed, 1 disagreed with evidence)

Before this PR was opened it went through two independent review rounds: a cold
code review plus an independent second model (codex) reviewing the diff against
the PR body, with every finding adjudicated on the code rather than accepted or
rejected wholesale. Round 1 sent the change back for a fix; round 2 cleared it.

#	Finding	Severity	Verdict
1	resume-drain-order-test: the resume-queue exclusion is only sound while recovery reads the queue before resume_enrolled_sessions unlinks it, and nothing pinned that ordering. Hoisting the drain kept all tests green while every enrolled plan got swept.	major	❌ AGREE, fixed
2	docs-plans-status-table-omits-failed: a restart alone now produces failed, and the notification tells the operator to act, but docs/plans.md did not list the status.	nit	❌ AGREE, fixed
3	prbody-plus17-wrong-correction: four figures in this PR body were stale after the round-1 fix added a test (test count, base-arm count, API-shape count, added-passes).	blocker	❌ AGREE, fixed
4	test-commentary-volume: the new test module carries more explanatory prose than the repo norm.	nit	⚠️ AGREE, noted not blocking
5	startup-ingress-race: recovery could mark a newly approved, actively implementing plan failed, because MCP and Telegram ingress start before the sweep.	blocker	⚠️ DISAGREE, with evidence

Finding 1 (fixed). A new test drives the real lifespan with an enrolled,
eligible plan and a drainer stub that actually unlinks the queue, then asserts on
plan state rather than call order. It fails (assert 'failed' == 'implementing') against a tree where the awaited drain is hoisted above
recovery, while the other 17 tests still pass, so the ordering is now pinned by
something that can observe a violation. Its anti-vacuity guard is load-bearing:
deleting the drain entirely leaves the status assertion passing, and only the
existed is True check catches it. One discrimination limit is disclosed rather
than chased: hoisting create_task immediately before recovery still passes,
because _enrolled_resume_session_ids() is synchronous and the pass's first
statement, so no scheduling point exists between them.

Finding 5 (disagreed). The ordering fact is correct and confirmed: MCP
(server.py:307), the loopback listener (:326) and Telegram polling (:344)
all precede recovery (:357). The scenario needs one further necessary step,
though - an approval must complete inside that window - and each named ingress
fails it:

• Telegram has no handler that can approve a plan. The registered set
  (telegram.py:533-549) is 12 command handlers plus callback/message/reaction
  handlers; _handle_callback_query only parses sess:* and notif:<id>:<answer>
  and routes to notification_service.handle_answer, and the approval dispatcher
  registry (notifications/handlers.py) has no plan kind. A text message reaches
  plan_approve only through router._run_single -> engine.run, i.e. a full LLM
  turn that must choose to call the tool, whereas the sweep is 13 lines and about
  one DB round-trip later in the same coroutine.
• The gateway HTTP API - the only surface with a direct plan_approve
  (routes/plans.py:120-239) - is not listening yet. uvicorn's Server.startup()
  awaits lifespan.startup() before it ever reaches create_server, and
  run_server passes no pre-bound socket or fd.
• The MCP loopback listener runs with lifespan="off" on port 0, published to
  backends during this same lifespan, so no pre-existing client knows the port; the
  deferred /mcp/v1 mount answers 503 until the manager exists and is served by
  the socket that is not yet bound.

The suggested remedy is also not admissible as written: moving recovery above the
Telegram start would silently lose the operator alert, since _deliver_telegram
resolves the bot through the started channel (service.py:901-906) and returns
None with a warning otherwise - which is exactly the placement rationale in the
lifespan comment.

The residual the finding points at is real in principle, and it is the same class
as the trade already disclosed in the "Out of scope" section and pinned by a test:
a plan swept by one daemon is not re-adopted later. The sibling pass solves the
analogous problem with an in-memory liveness guard (sessions.py:766), and
engine.is_session_running is the matching primitive here - but adopting it is a
source change with its own design decision, so it is filed as a follow-up rather
than bolted on.

Finding 4 (noted, not blocking). Measured rather than asserted: prose density
in the new test module is 25.2 percent, the highest of the 50 test files at or
above 300 lines (median 10.3, next highest 22.1). The direction is right. Each
cited block is load-bearing for a future editor, though: the module docstring
records why the resume exclusion is correctness rather than caution, one comment
records why the imports sit inside function bodies (a module-level import of an
added symbol collapses both behavioural witnesses into a single collection error),
and the rest each name the discriminator their fixture exists to be. A trimming
pass is filed as a follow-up instead of a round spent on comments.

Gate spend across both rounds: $17.75 (2 runs). Design-review spend before
implementation: $35.87 (8 rounds). Total $53.62.

[oranjeai]

Pre-PR validation gate (a-i)

#	Question	Answer
a	Deterministic repro?	Yes. cd <base export> && pytest tests/test_plan_restart_recovery.py -- 18/18 fail on the pre-change tree, every run. Exactly two are behavioural: an implementing plan seeded before startup is still implementing after the gateway lifespan completes, and still implementing at the moment cron starts. The wedge itself is deterministic by construction (nothing in the codebase reads plan rows at startup: the only files touching plans are plan_service.py, handlers/plans.py, handlers/tasks.py, db/plans.py, routes/plans.py, all request-path, and there is no get_active_plans analogue of get_active_workflow_runs).
b	Root cause explained?	Plan approval hands the plan's liveness to asyncio.create_task and persists no reconciliation hook. The only writers that can move a plan off implementing are that in-process task (which writes failed when engine.run raises) and a later task_done. Both die with the process, so after a restart the row asserts an obligation nobody holds -- and get_pending_plan_task_ids counts implementing, so plan_propose refuses that task forever while both approval surfaces refuse re-approval (they gate on pending). The sibling subsystem that made the same RAM-only trade, workflow runs, pairs it with a startup recovery pass; sessions have one too. Plans were the only holdout.
c	Fix matches root cause?	Yes -- it adds the missing reconciliation hook rather than guarding a consumer. failed is not a new vocabulary word: both approval surfaces already write exactly that when the implementation run does not complete. Rejected alternatives, each for a measured reason: a migration for an error column (v040 is already contended on main and by open PRs; the reason lives in the log line and notification instead), reusing feedback (it is human-authored revision text rendered in the UI), putting the pass in engine.initialize() (nerve sync / nerve cron <job> build a full engine against the same ~/.nerve/nerve.db, so a 5-second CLI one-shot would fail the live daemon's in-flight plans), list_plans(status=...) (its limit=100 is a fail-open in a recovery path), and a periodic reconcile loop (the wedge is created only by process death).
d	Test intent preserved / new tests added?	No test was modified or weakened. 18 new tests: 3 store-level, 8 helper-level, 4 wiring-level (driving the real lifespan), plus queue-parse parity, non-consumption, and one pinning the accepted no-reclaim trade. The fourth wiring test pins recovery's queue read BEFORE the engine's drain: resume_enrolled_sessions unlinks the queue up front, so a recovery pass running after it would sweep every enrolled plan, and no other test sees that hoist.
e	Demonstrated in BOTH directions?	Yes. Base arm: git archive 94406ea | tar -x into a scratch dir (not git checkout <rev> -- tests/, which leaves stale files) with the new file copied in -- 18/18 fail, no collection error, and the two witnesses fail on plan state (AssertionError: assert 'implementing' == 'failed'), not on an import. That is only possible because the module imports no new symbol at module level. The other 16 fail on API shape (AttributeError) and are disclosed as the weaker evidence they are -- including the ordering test, whose base-arm failure is an AttributeError on RESUME_QUEUE_FILE, so it is not counted as a witness. It is instead demonstrated against a mutant that moves the awaited drain above recovery: it then fails on plan state (assert 'failed' == 'implementing') while all 17 others still pass. Fixed arm: 18/18 pass.
f	General across CODE paths?	Every carrier of the invariant "an implementing plan has a live obligation" was enumerated and dispositioned: both approval surfaces (covered), PATCH /api/plans/{id} (covered only because the CAS is status-only -- an owner-keyed predicate would leave its NULL-owner rows wedged forever, which one test pins), the legitimate task_done and _run_impl exits (the CAS loses those races safely, rowcount 0), get_pending_plan_task_ids (unblocked by failed), the UI (already styles failed; no frontend diff), non-daemon entrypoints (deliberately NOT covered -- that is the argument for lifespan), and nerve restart --resume (preserved, see the exclusion). review_loops.status also uses the literal 'implementing' but is a different table; the SQL names plans only.
g	Generalizes across INPUTS?	Every plan status exercised (pending / implementing / done / declined / superseded / failed), NULL vs set impl_session_id, 150 rows (past the limit=100 boundary that the rejected alternative would have truncated), zero rows (no notification at all), and a queue file with blank lines, padding, tabs, duplicates, no trailing newline, and an id containing internal whitespace -- the last is what discriminates the parity parse from raw.split(). All four resume skip classes plus an eligible control appear in ONE sweep, so the fixture cannot pass a preflight that checks only one predicate.
h	Backward compatible?	Nothing engaged: no schema change, no migration, no settings change, no default change, no serialization change, no new status value, no API shape change. The only observable difference is that a plan orphaned by a restart reads failed instead of implementing, and failed is already produced by both approval surfaces today, so every existing reader handles it. planStore.ts types status: string and there is no plan-status TS union, so tsc -b cannot break.
i	Invariants and contracts preserved?	The CAS is the atomicity contract: only the caller that actually flipped the row reports it, so a concurrent task_done between the read and the write is neither flipped nor announced (one test pins that no notification is sent in that case, which is what discriminates notify-on-flipped from notify-on-orphaned). Startup ordering is preserved on all four constraints (after notification wiring, after the Telegram channel so the alert can actually deliver, before cron.start(), inside a try/except so recovery can never take the daemon down). The resume queue's sole-drainer contract is preserved: read only, never unlinked. The two error cases go deliberately opposite ways -- a missing file sweeps (a definite answer from the sole writer), an OSError skips the sweep entirely, because this read and the engine's happen at different instants and failing closed costs exactly the status quo.

Mutation matrix: 19 mutants, 19 killed, 0 survivors, 0 vacuous. Each mutation
asserted to have applied exactly once with its marker present and still parsing
before any FAIL was trusted; the unmutated control ran green at both ends of the
matrix and the tree hash was verified restored after every arm. Killed: deleting
the lifespan call (only the lifespan test can see it), moving it after
cron.start() (only the ordering test can see it), dropping the CAS status
guard, an owner-keyed CAS, list_plans instead of the dedicated query, writing
pending instead of failed, notifying on the pre-CAS list (two variants),
deleting the resume exclusion, draining the queue, whitespace-splitting the
parse, collapsing the two error cases, removing the lifespan try/except,
reclaiming an already-failed plan, deleting the eligibility preflight, and
dropping each of its four predicates in turn.

No regression: full pytest tests/ on the same clone before the first edit
and after the last, compared by sorted FAILED test NAMES (not counts) -- an
identical set of 7 pre-existing failures (6 in test_memu_bridge.py, 1 in
test_telegram_sessions.py), with passed going 2933 -> 2951, i.e. exactly the 18
new tests. The real ~/.nerve/resume-after-restart was verified absent before
and after both suite runs.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[oranjeai]

cc @pufit — could you review this? A plan in implementing asserts a live in-process implementation run, but that run is a bare asyncio.create_task, so a restart/OOM leaves the row asserting an obligation nobody holds and the plan is terminally wedged (re-approval refused, plan_propose refused forever for that task). This adds the startup reconciliation pass that workflow runs and sessions already have, CASing orphans to failed and notifying, while sparing plans whose implementation session is enrolled via nerve restart --resume.

[oranjeai]

Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes
outside "critical performance problem" or "makes my work easier" are handled by the Nerve team.
This PR is a correctness fix in neither category, so it is closed unmerged. The analysis stays in
the description and comments if it is useful during the rewrite. No further action needed from me.