plans: refuse an HTTP decline once the plan is no longer pending - #250
plans: refuse an HTTP decline once the plan is no longer pending#250oranjeai wants to merge 1 commit into
Conversation
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.
Internal second-model review2 review rounds, 12 findings adjudicated (8 agreed and fixed, 1 disagreed with evidence, 3 recorded and carried) - gate spend $18.28 over 6 runsBefore opening this PR I put it through two independent review rounds: a cold read of the resulting
Finding 1 is why Finding 7 is the one I would most want a reviewer to know about, because no automated check could Finding 9, disagreed. Measured each cited site against this repo's own precedent instead of a Findings 10 to 12 are real and deliberately not fixed here. Both are pre-existing and outside Rounds and cost: 2 review rounds, 6 gate runs, $18.28 total. |
Pre-PR validation gatea-i checklist
Mutation matrix (each mutant asserts its edit applied exactly once and that the file still
The last mutant is why One correction worth recording: the plan predicted the after-the-write and after-
|
|
|
|
Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes |
2 participants
Problem
PATCH /api/plans/{plan_id}writes a caller-suppliedstatuswith no precondition of any kind,while all four sibling surfaces refuse a non-pending plan (tool
plan_decline,plan_approve,plan_update, andPOST /api/plans/{id}/approve, which returns 409).So a
{"status": "declined"}PATCH against a plan under active implementation succeeds: the rowflips to
declinedwhile the implementation session keeps running, and the route'stask_donesideeffect moves the task file into
done/under an agent still writing to it. The store'supdate_planis an unconditional
UPDATE ... WHERE id = ?, so nothing re-checks. No concurrency is involved: thetool surface refuses the identical call sequentially. The route was written as a generic
field-updater rather than the lifecycle transition it performs, so it inherited none of its siblings'
preconditions. Reachable from the UI, whose Decline button renders off a plan loaded once, never
re-polled.
Fix
Add the precondition where the others enforce it: right after loading the row, before any write. The
raise is the approve route's, with one word changed; the condition adds the
declinedscope:It precedes the write and the
task_doneinvocation, so a refused decline changes nothing. Onlydeclinedis guarded, since it is the only PATCH-reachable transition with a task-closing sideeffect; a test pins that scope. Intended behaviour change: this PATCH now returns 409 where it
returned
{"updated": true}. That success was a lie, and the shape matches the approve route'sexisting 409.
Web: the 409 was swallowed twice. The store logged it and set no
actionError; andhandleDeclinedid not await, so it cleared the form and the typed reason unconditionally, which read as success.
Compounding both, the only
actionErrorblock sat inside the revise form, so it neverrendered on decline.
updatePlannowreturns a boolean and sets
actionErrorvia the existingextractErrorMessage,handleDeclineawaits it and clears only on success, and the error block serves both forms.
Tests
tests/test_plan_decline_precondition.py(new, 15 cases): 409 for an implementing plan with the rowand the task asserted untouched, the pending happy path unchanged, every non-pending status
parametrized, a surface-parity assertion that tool and route agree per status, and a
supersededscope test.
Without the fix (clean export of
mainplus the new test file) 13 failed, 2 passed; with it15 passed.
Validation detail
Both arms re-run against the final tree. Base arm from a clean
git archive main | tar -xexport(not
git checkout main -- nerve/, which leaves files absent from that revision in place), with thenew test copied in: 13 failed, 2 passed, 0 collection errors. The 2 passing in both arms are the
happy-path and scope-limit regression guards.
Full suite: base
20 failed, 2935 passed->7 failed, 2948 passed. Comparing sorted FAILED-namesets, the fix introduces no new failure; the 7 residuals are pre-existing and identical in both
arms (
test_memu_bridgex6,test_telegram_sessionsx1), none plan-related.Mutation matrix on the guard, each mutant asserting its edit applied exactly once and still parses,
with the unmutated control green at both ends: deleted (the original defect), after the write, after
task_done, applied to all statuses, 400 instead of 409, predicate inverted, and lettingapprovedthrough. All 7 killed.ruff checkclean;npx tsc -bclean;npx eslint0 errors (its 1react-hooks/exhaustive-depswarning is pre-existing and identical on
main).web/has no test runner (package.jsonscriptsare
dev/build/lint/preview), so the three web changes are verified by typecheck, lint andreading, not by an automated kill.
Not addressed: the plan-status writers are still non-atomic read-then-write pairs, so a concurrent
approval can clobber a sibling transition. That needs a CAS on all four writers, not fixable
independently while approval's own write is unconditional. Tracked separately.