From 26e5813a3a8c65e3fc612b8b6558400931877fd6 Mon Sep 17 00:00:00 2001
From: Oranje AI <293843428+oranjeai@users.noreply.github.com>
Date: Tue, 4 Aug 2026 00:55:45 +1200
Subject: [PATCH] plans: refuse an HTTP decline once the plan is no longer
pending
PATCH /api/plans/{plan_id} wrote a caller-supplied status with no
precondition of any kind, while all four sibling surfaces refuse a
non-pending plan (tool plan_decline/plan_approve/plan_update and
POST /api/plans/{id}/approve, which returns 409).
So a {"status": "declined"} PATCH against a plan under active
implementation succeeded: the row flipped to declined while the
implementation session kept running, and the route's task_done side
effect moved the task file into done/ under an agent still writing to
it. The store's update_plan is an unconditional UPDATE ... WHERE id = ?,
so nothing downstream re-checked. No concurrency is involved - the tool
surface refuses the identical call sequentially. Root cause is that the
route was written as a generic field-updater rather than as the
lifecycle transition it performs, so it inherited none of its siblings'
preconditions.
Add the precondition where the others enforce it, immediately after
loading the row and before any write. It is the approve route's guard
with one word changed, so both HTTP plan surfaces read identically, and
it precedes the write and the task_done invocation alike, so a refused
decline changes nothing. Only the declined transition is guarded, since
it is the only PATCH-reachable transition with a task-closing side
effect; a test pins that scope so it is not later mistaken for an
oversight.
Intended behaviour change: this PATCH now returns 409 where it returned
{"updated": true}. That old success was a lie, and the response shape
matches the approve route's existing 409.
The 409 was swallowed twice on the web side. The store logged it and set
no actionError; and handleDecline did not await, so it cleared the form
and the typed reason unconditionally, which read as success. Compounding
both, the only actionError block sat inside the revise form, so it could
never render on the decline path. updatePlan now returns a boolean and
sets actionError via the existing extractErrorMessage, handleDecline
awaits it and clears the form only on success, and the error block is
hoisted to a shared area serving both forms.
tests/test_plan_decline_precondition.py (new, 15 cases) covers the 409
for an implementing plan with the row and the task both asserted
untouched, the unchanged pending happy path, every non-pending status,
a surface-parity assertion that the tool handler and the route agree per
status, and a superseded scope test. approved is in that status list
although no writer produces it, because this same unvalidated route can
store it. Without the fix, on a clean git archive export of main plus
the new test file: 13 failed, 2 passed. With it: 15 passed. Full suite
goes 20 -> 7 failures with no new failure by sorted FAILED-name-set
comparison; the 7 residuals are pre-existing in both arms. A seven-mutant
matrix on the guard is fully killed with the unmutated control green at
both ends. ruff and tsc -b are clean and eslint reports 0 errors; web/
has no test runner, so the web changes are verified by typecheck, lint
and reading rather than an automated kill.
Not addressed here: the plan-status writers are still non-atomic
read-then-write pairs, so a concurrent approval can clobber a sibling
transition. That is a different invariant needing a compare-and-swap on
all four writers, and it is not independently fixable while approval's
own write is unconditional. Tracked separately.
---
nerve/gateway/routes/plans.py | 8 +
tests/test_plan_decline_precondition.py | 216 ++++++++++++++++++++++++
web/src/pages/PlanDetailPage.tsx | 32 ++--
web/src/stores/planStore.ts | 9 +-
4 files changed, 252 insertions(+), 13 deletions(-)
create mode 100644 tests/test_plan_decline_precondition.py
diff --git a/nerve/gateway/routes/plans.py b/nerve/gateway/routes/plans.py
index e3b2f655..2cf7cc70 100644
--- a/nerve/gateway/routes/plans.py
+++ b/nerve/gateway/routes/plans.py
@@ -64,6 +64,14 @@ async def update_plan(plan_id: str, req: PlanUpdateRequest, user: dict = Depends
if not plan:
raise HTTPException(status_code=404, detail="Plan not found")
+ # Guard: only pending plans can be declined. It precedes both the write
+ # and the task_done invocation below, so a refused decline changes nothing.
+ if req.status == "declined" and plan["status"] != "pending":
+ raise HTTPException(
+ status_code=409,
+ detail=f"Plan is '{plan['status']}', only 'pending' plans can be declined",
+ )
+
fields = {}
if req.status:
fields["status"] = req.status
diff --git a/tests/test_plan_decline_precondition.py b/tests/test_plan_decline_precondition.py
new file mode 100644
index 00000000..9a1ab814
--- /dev/null
+++ b/tests/test_plan_decline_precondition.py
@@ -0,0 +1,216 @@
+"""The HTTP decline surface must refuse a non-pending plan, like its siblings.
+
+``PATCH /api/plans/{plan_id}`` used to write a caller-supplied status with no
+precondition of any kind, while all four sibling surfaces (tool
+plan_decline/plan_approve/plan_update and ``POST .../approve``) refuse a
+non-pending plan. So a ``declined`` PATCH against a plan under active
+implementation succeeded: the store recorded ``declined`` and ``task_done``
+moved the task file into ``done/`` under a still-running session. No
+concurrency is involved: the tests here are plain sequential calls.
+"""
+
+from __future__ import annotations
+
+from types import SimpleNamespace
+from typing import Any
+
+import pytest
+import pytest_asyncio
+
+from nerve.db import Database
+
+# ``approved`` is here although no writer produces it: the same unvalidated
+# route can store it, and it is advertised as a plan status in three places.
+NON_PENDING = ["approved", "implementing", "declined", "superseded", "done", "failed"]
+
+
+class FakeEngine:
+ """Minimal engine for the route's tool-invocation path.
+
+ ``PATCH`` on decline reaches ``task_done`` through
+ ``get_tool_registry().invoke(...)`` and ``build_route_tool_context()``,
+ which read ``.registry``, ``.config`` and the bridge attributes. A real
+ registry is used so ``task_done`` genuinely runs and the "task was not
+ closed" assertions mean something.
+ """
+
+ def __init__(self, config: Any, db: Database) -> None:
+ from nerve.agent.tools import build_default_registry
+
+ self.config = config
+ self.db = db
+ self.registry = build_default_registry()
+ self._memory_bridge = None
+ self._xmemory_bridge = None
+ self._skill_manager = None
+ self.runs: list[dict[str, Any]] = []
+
+ async def run(self, session_id: str, user_message: str, source: str = "web") -> None:
+ self.runs.append({"session_id": session_id, "user_message": user_message})
+
+
+@pytest.mark.asyncio
+class TestHttpDeclinePrecondition:
+ @pytest_asyncio.fixture
+ async def app_setup(self, db: Database, tmp_path):
+ """FastAPI app + a task on disk under ``memory/tasks/active/``.
+
+ The on-disk task is what makes the side-effect assertions real:
+ ``task_done`` moves the file into ``done/``, so "was the task closed"
+ is observable in both the DB row and the filesystem.
+ """
+ from fastapi import FastAPI
+ from fastapi.testclient import TestClient
+
+ import nerve.config as cfg_mod
+ from nerve.agent import tools as tools_mod
+ from nerve.config import NerveConfig
+ from nerve.gateway.routes._deps import init_deps
+ from nerve.gateway.routes.plans import router as plans_router
+
+ cfg = NerveConfig()
+ cfg.workspace = tmp_path
+ cfg.auth.jwt_secret = "" # require_auth becomes a no-op
+ cfg_mod._config = cfg
+
+ task_id = "t-decline"
+ rel_path = "memory/tasks/active/t-decline.md"
+ task_md = tmp_path / rel_path
+ task_md.parent.mkdir(parents=True, exist_ok=True)
+ task_md.write_text("# Demo task\n\nBody.\n", encoding="utf-8")
+
+ await db.upsert_task(
+ task_id=task_id, file_path=rel_path, title="Demo task",
+ status="pending", content=task_md.read_text(encoding="utf-8"),
+ )
+ await db.create_plan(
+ plan_id="plan-1", task_id=task_id, content="the plan",
+ session_id="sess-proposer", version=1, plan_type="generic",
+ )
+
+ engine = FakeEngine(cfg, db)
+ tools_mod.init_tools(workspace=tmp_path, db=db, engine=engine)
+ init_deps(engine=engine, db=db) # type: ignore[arg-type]
+
+ app = FastAPI()
+ app.include_router(plans_router)
+
+ yield SimpleNamespace(
+ client=TestClient(app), db=db, engine=engine,
+ task_id=task_id, workspace=tmp_path, task_md=task_md,
+ )
+
+ cfg_mod._config = None
+
+ async def _assert_task_untouched(self, s) -> None:
+ """The task must still be open, on disk, and outside ``done/``."""
+ task = await s.db.get_task(s.task_id)
+ assert task["status"] == "pending"
+ assert task["file_path"] == "memory/tasks/active/t-decline.md"
+ assert s.task_md.exists()
+ assert not (s.workspace / "memory" / "tasks" / "done" / "t-decline.md").exists()
+
+ async def test_http_decline_of_an_implementing_plan_returns_409(self, app_setup):
+ """The reported defect: declining a plan under active implementation."""
+ s = app_setup
+ await s.db.update_plan("plan-1", status="implementing", impl_session_id="impl-live")
+
+ resp = s.client.patch("/api/plans/plan-1", json={"status": "declined"})
+
+ assert resp.status_code == 409
+ assert "implementing" in resp.json()["detail"]
+ # The row is untouched: status AND the live session pointer.
+ plan = await s.db.get_plan("plan-1")
+ assert plan["status"] == "implementing"
+ assert plan["impl_session_id"] == "impl-live"
+ # And the side effect never ran. "Returned 409" and "closed the task
+ # anyway" are indistinguishable from the status code alone.
+ await self._assert_task_untouched(s)
+
+ async def test_http_decline_of_a_pending_plan_still_works(self, app_setup):
+ """Regression guard: the happy path is unchanged."""
+ s = app_setup
+
+ resp = s.client.patch(
+ "/api/plans/plan-1", json={"status": "declined", "feedback": "not now"},
+ )
+
+ assert resp.status_code == 200
+ assert resp.json() == {"plan_id": "plan-1", "updated": True}
+ plan = await s.db.get_plan("plan-1")
+ assert plan["status"] == "declined"
+ assert plan["feedback"] == "not now"
+ assert plan["reviewed_at"]
+ # The task was closed, with the feedback in the note.
+ task = await s.db.get_task(s.task_id)
+ assert task["status"] == "done"
+ done_md = s.workspace / "memory" / "tasks" / "done" / "t-decline.md"
+ assert done_md.exists()
+ assert "not now" in done_md.read_text(encoding="utf-8")
+ assert not s.task_md.exists()
+
+ @pytest.mark.parametrize("status", NON_PENDING)
+ async def test_http_decline_is_refused_for_every_non_pending_status(
+ self, app_setup, status: str,
+ ):
+ """Covers every status the tool surface would also refuse."""
+ s = app_setup
+ await s.db.update_plan("plan-1", status=status)
+
+ resp = s.client.patch("/api/plans/plan-1", json={"status": "declined"})
+
+ assert resp.status_code == 409
+ assert f"Plan is '{status}'" in resp.json()["detail"]
+ plan = await s.db.get_plan("plan-1")
+ assert plan["status"] == status
+ await self._assert_task_untouched(s)
+
+ @pytest.mark.parametrize("status", NON_PENDING)
+ async def test_the_two_decline_surfaces_agree(self, app_setup, status: str):
+ """Parity: the tool handler and the route must make the same call.
+
+ This is the assertion that would have caught the original divergence,
+ so it is the one that keeps it from coming back.
+ """
+ from nerve.agent.tools.handlers.plans import plan_decline_handler
+ from nerve.gateway.routes._deps import build_route_tool_context
+
+ s = app_setup
+ await s.db.update_plan("plan-1", status=status)
+
+ tool_result = await plan_decline_handler(
+ build_route_tool_context(), {"plan_id": "plan-1"},
+ )
+ tool_refused = "only pending plans can be declined" in tool_result.content[0]["text"]
+
+ resp = s.client.patch("/api/plans/plan-1", json={"status": "declined"})
+ route_refused = resp.status_code == 409
+
+ assert tool_refused == route_refused, (
+ f"surface divergence on status={status!r}: "
+ f"tool refused={tool_refused}, route refused={route_refused}"
+ )
+ assert tool_refused, f"both surfaces should refuse a {status!r} plan"
+ # Neither surface may have written anything.
+ plan = await s.db.get_plan("plan-1")
+ assert plan["status"] == status
+ await self._assert_task_untouched(s)
+
+ async def test_other_statuses_are_unaffected(self, app_setup):
+ """The guard covers ``declined`` only, deliberately, not by oversight.
+
+ ``declined`` is the only PATCH-reachable transition with a
+ task-closing side effect. Widening this into a general status whitelist
+ is a separate concern; this test pins the scope so a future reader does
+ not mistake it for a gap.
+ """
+ s = app_setup
+ await s.db.update_plan("plan-1", status="implementing", impl_session_id="impl-live")
+
+ resp = s.client.patch("/api/plans/plan-1", json={"status": "superseded"})
+
+ assert resp.status_code == 200
+ plan = await s.db.get_plan("plan-1")
+ assert plan["status"] == "superseded"
+ # No task closure on this path either way.
+ await self._assert_task_untouched(s)
diff --git a/web/src/pages/PlanDetailPage.tsx b/web/src/pages/PlanDetailPage.tsx
index 7fb8f131..bc34d678 100644
--- a/web/src/pages/PlanDetailPage.tsx
+++ b/web/src/pages/PlanDetailPage.tsx
@@ -61,10 +61,14 @@ export function PlanDetailPage() {
}
};
- const handleDecline = () => {
- updatePlan(plan.id, 'declined', declineFeedback.trim() || undefined);
- setDeclineFeedback('');
- setShowDeclineFeedback(false);
+ const handleDecline = async () => {
+ // Keep the form and the typed reason on failure: the route refuses a
+ // decline once the plan is no longer pending (409).
+ const ok = await updatePlan(plan.id, 'declined', declineFeedback.trim() || undefined);
+ if (ok) {
+ setDeclineFeedback('');
+ setShowDeclineFeedback(false);
+ }
};
const handleRevise = async () => {
@@ -165,6 +169,15 @@ export function PlanDetailPage() {
+ {/* Shared action error: serves decline and revision alike, so a
+ refusal is visible whichever form triggered it. */}
+ {actionError && (
+
+
+
{actionError}
+
+ )}
+
{showDeclineFeedback && (
@@ -172,7 +185,10 @@ export function PlanDetailPage() {