plans: refuse an HTTP decline once the plan is no longer pending

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

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)

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,14 +169,26 @@ export function PlanDetailPage() {
                 </button>
               </div>
 
+              {/* Shared action error: serves decline and revision alike, so a
+                  refusal is visible whichever form triggered it. */}
+              {actionError && (
+                <div className="flex gap-0">
+                  <div className="w-1 bg-red-500/40 rounded-full shrink-0" />
+                  <div className="pl-3 py-1 text-[12px] text-hue-red">{actionError}</div>
+                </div>
+              )}
+
               {showDeclineFeedback && (
                 <div className="space-y-2">
                   <div className="flex gap-0">
                     <div className="w-1 bg-red-500/40 rounded-full shrink-0" />
                     <div className="flex-1 pl-3">
                       <textarea
                         value={declineFeedback}
-                        onChange={e => setDeclineFeedback(e.target.value)}
+                        onChange={e => {
+                          setDeclineFeedback(e.target.value);
+                          if (actionError) clearActionError();
+                        }}
                         placeholder="Optional: why is this plan being declined? (leave empty to close without a reason)"
                         className="w-full p-3 text-[13px] bg-surface-raised border border-border-subtle rounded-lg text-text-secondary placeholder:text-placeholder focus:outline-none focus:border-red-500/50 resize-none"
                         rows={3}
@@ -210,12 +226,6 @@ export function PlanDetailPage() {
                       />
                     </div>
                   </div>
-                  {actionError && (
-                    <div className="flex gap-0">
-                      <div className="w-1 bg-red-500/40 rounded-full shrink-0" />
-                      <div className="pl-3 py-1 text-[12px] text-hue-red">{actionError}</div>
-                    </div>
-                  )}
                   <div className="flex justify-end">
                     <button
                       onClick={handleRevise}

web/src/stores/planStore.ts

@@ -30,7 +30,7 @@ interface PlanState {
   loadPlans: () => Promise<void>;
   setFilter: (f: string) => void;
   loadPlan: (id: string) => Promise<void>;
-  updatePlan: (id: string, status: string, feedback?: string) => Promise<void>;
+  updatePlan: (id: string, status: string, feedback?: string) => Promise<boolean>;
   approvePlan: (id: string) => Promise<{ impl_session_id: string } | null>;
   revisePlan: (id: string, feedback: string) => Promise<boolean>;
   clearActionError: () => void;
@@ -93,7 +93,7 @@ export const usePlanStore = create<PlanState>((set, get) => ({
   },
 
   updatePlan: async (id: string, status: string, feedback?: string) => {
-    set({ actionLoading: true });
+    set({ actionLoading: true, actionError: null });
     try {
       await api.updatePlan(id, { status, feedback });
       // Refresh
@@ -102,8 +102,13 @@ export const usePlanStore = create<PlanState>((set, get) => ({
         set({ selectedPlan: { ...sel, status, ...(feedback ? { feedback } : {}) } });
       }
       get().loadPlans();
+      return true;
     } catch (e) {
+      // The route refuses a non-pending decline with 409. Surface it so the
+      // caller can keep the form and the typed reason instead of clearing them.
       console.error('Failed to update plan:', e);
+      set({ actionError: extractErrorMessage(e, 'Failed to update plan') });
+      return false;
     } finally {
       set({ actionLoading: false });
     }