fix(api): approve exactly the named tasks instead of the whole backlog (#1146) - #1152
Conversation
#1146) `ApproveTasksRequest` was exclusion-shaped — `excluded_task_ids`, no `task_ids` — and Pydantic drops unknown fields by default. So the intuitive inclusion payload POST /api/v2/tasks/approve {"task_ids": ["<one-task>"]} returned 200 and transitioned EVERY backlog task to READY: the exact inverse of the request, in silence. Reproduced against the pre-fix router — asking for one task leaves the other two READY where the caller expected BACKLOG. Found while writing #1068's API lifecycle driver, which did exactly this. Its test "covered" scoped approval and passed anyway, because the chosen task *was* READY — and so was everything else. Options 1 and 2 from the issue, together: - `task_ids` is now a real field meaning "approve exactly these". The semantics live in `runtime.approve_tasks(included_task_ids=...)`, not the router, so the CLI and any other surface inherit them (core-first). - `model_config = ConfigDict(extra="forbid")`, so the NEXT mis-shaped payload is a 422 rather than a silent reinterpretation. `{"taskIds": [...]}` no longer approves the backlog. Two ambiguities refuse rather than resolve into a mutation: both lists at once is 422, and an id that is not an approvable BACKLOG task is 422 — approving fewer tasks than named, quietly, is the same class of bug this fixes. Both assert that nothing changed status. The exclusion shape is untouched; this adds, it does not replace. tests/ui/test_batch_execution_offload.py stubbed approve_tasks with a lambda whose signature had to widen. Worth noting that a non-autospec stub is what let the drift be invisible until the call actually failed.
|
Claude finished @frankbria's task in 4m 58s —— View job GLM Precision Review — PR #1152Reviewed for concrete defects only (logic, security, data-loss, contract breaks). Style/architecture/naming/coverage are covered by the general review and intentionally not addressed here.
Findings
Notes on what I checked and cleared
|
ReviewSolid, well-scoped fix. The core logic is correct and the test coverage is genuinely good — it pins the "wrong result that looks right" failure mode (scoped approval where the untouched tasks happened to already be READY), not just the happy path. Correctness checks I traced through:
Minor nit (non-blocking): Design call I agree with: declining to do the repo-wide Test coverage: thorough — inclusion, exclusion (unchanged), both-lists-ambiguous, unknown-id, unknown-field, and the core-level No security or performance concerns; this is a pure request-validation/semantics fix with no new I/O paths. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Review finding, and the exclusion path had it too. `tasks.list_tasks` defaults to `limit=100` (#743) and `approve_tasks` took that default — so "approve everything" silently approved the first 100 of a larger backlog, and the new inclusion path would 422 a perfectly valid id that sorted past the cap. That is the same failure this PR exists to remove: quietly doing less than asked, with a 200. Three tests over a 105-task backlog, all three verified to fail without `limit=None`. My own `_statuses` helper had the identical bug — it read a single page, so the "everything is READY" assertion was true of 100 tasks out of 105.
|
Claude finished @frankbria's task in 3m 33s —— View job GLM Precision Review — PR #1152 (re-review after `04ccd65)Reviewed for concrete defects only (logic, security, data-loss, contract breaks). Style/architecture/naming/coverage are covered by the general review and intentionally not addressed here.
Outcome✅ GLM review: no new defects found. The single previously-reported finding is resolved. Prior finding — FIXED & verified. The What I re-verified and cleared (no action needed)
No security, data-loss, or contract concerns. This is a pure request-validation/semantics fix; the newest commit cleanly closes the one real defect the prior review raised. |
ReviewI reviewed the full current diff (both commits, including the GLM's My earlier correctness checks still hold against the current code: the mutual-exclusion guard (router Still open, still non-blocking: Scope discipline: declining the repo-wide Tests: the new No new security or performance concerns beyond what's already been discussed on this thread. This looks ready to merge from a correctness standpoint. |
Review finding — a second, older bug in the same shape
Right — and the exclusion path had it all along. Fixed with
All three verified failing against the un-fixed My own Checks
Demo — the inversion, before and afterThree BACKLOG tasks, asking for one: # main's router
POST /api/v2/tasks/approve {"task_ids": ["<task-0>"]} → 200
statuses: [READY, READY, READY] ← asked for one, got three
# this branch
POST /api/v2/tasks/approve {"task_ids": ["<task-0>"]} → 200
statuses: [READY, BACKLOG, BACKLOG]
POST /api/v2/tasks/approve {"taskIds": ["<task-0>"]} → 422, nothing changes
POST /api/v2/tasks/approve {"task_ids": ["<task-0>", "nope"]} → 422, nothing changes |
1 participant
Closes #1146.
The bug, reproduced
ApproveTasksRequestwas exclusion-shaped —excluded_task_ids, notask_ids— and Pydantic drops unknown fields by default. So the intuitive payload:returned 200 and transitioned every backlog task to READY. The exact inverse of the request, in silence.
Reproduced against the pre-fix router by running this PR's test on
main'stasks_v2.py:Ask for one task, get three.
Why it went unnoticed
Found while writing #1068's API lifecycle driver, which did exactly this. Its test "covered" scoped approval and passed anyway — the chosen task was READY, and so was everything else. A wrong result that looks right, which is the same shape as #1066, #1077 and #1085.
No production caller hits this route today (neither the web UI nor the CLI), which is why it was cheap to fix now and would have been expensive after a client existed.
The fix — options 1 and 2 from the issue, together
task_idsis a real field meaning "approve exactly these". The semantics live inruntime.approve_tasks(included_task_ids=...), not the router, so the CLI and any other surface inherit them — core-first.model_config = ConfigDict(extra="forbid"), so the next mis-shaped payload is a 422 rather than a silent reinterpretation.{"taskIds": [...]}no longer approves the backlog.Two ambiguities now refuse rather than resolve into a mutation:
{"task_ids": [a]}{"task_ids": [a], "excluded_task_ids": [b]}{"task_ids": [a, "nope"]}{"taskIds": [a]}The unknown-id case matters on its own: approving fewer tasks than named, quietly, is the same class of bug. Each of those rows has a test asserting no task changed status, not just the code.
The exclusion shape is untouched — this adds, it does not replace. Two tests pin that, including the empty-body "approve the whole backlog" behaviour.
Option 3 — not taken here
The issue offers a repo-wide sweep setting
extra="forbid"on every v2 request model. That is a much larger change with its own risk (any client sending a stray field starts getting 422s), and it deserves its own PR and its own audit of what currently gets dropped. Not filed as a follow-up yet — worth deciding whether the sweep is wanted before creating an issue for it.Testing
tests/ui/test_task_approval_shape_1146.py— 12 tests; verified the key one fails onmain's router (transcript above) and theextra="forbid"pair fails when that line is removedtests/ui/test_batch_execution_offload.py— two stub lambdas had to widen. Worth noting: a non-autospec stub is exactly what let the signature drift stay invisible until the call failedruffreported below