refactor(dashboard): sweep tranche 3 onto read_bounded_json (#5587)

[CrysisDeu]

Summary

Tranche 3 of the #5587 sweep. Routes the 55 bare await request.json() sites in chat_handlers.py and handlers/{mcp,cron,workflows,files}.py through the shared body guard read_bounded_json, so a valid-JSON-non-object body ([], "s", 5, null) answers 400 body_not_object instead of the AttributeError 500 the follow-on .get() raised.

Refs #5587 — partial fix, do not close. Out of scope per the issue: the four deliberately-divergent guards (handlers_channel._json_object, handlers/hooks.py::_json_object, handlers/artifacts.py::_read_json_body, handlers/session_storage.py::_json_body) and their fold decision.

Cap decisions (all recorded in the guard test's _CAP_REGISTER)

• 38 sites with fixed control-field bodies take the shared 64 KB cap.
• 3 sites take explicit per-route ceilings sized to their own declared bounds: api_crons_create / api_cron_update (_MAX_CRON_BODY_BYTES = 4×MAX_CRON_MESSAGE + 64 KB headroom, since a maximal multibyte message exceeds 64 KB) and _do_mcp_apply (_MCP_APPLY_MAX_BODY_BYTES = 1 MB, its change-count cap runs only after decoding).
• 14 sites whose bodies carry a chat message, queued-edit text, follow-up prompts, injected context/notes, workflow script source, NL intent, a full MCP server config, or file contents stay uncapped via explicit max_bytes=None.
• Zero new cap deferrals — the _CONTROL_FIELDS_CAP_PENDING ratchet stays at 13.

Behavior changes

• Non-object bodies: 400 body_not_object (was a 500, or a custom 400 message on a few routes).
• Capped sites answer 413 payload_too_large for oversized bodies, enforced before decoding.
• Sites that defaulted an ABSENT body keep doing so (allow_absent), but a present-and-malformed body is a 400 instead of silently collapsing to defaults (api_cron_enable, api_cron_ack, api_workflow_run_rerun — the same defect class dashboard: consolidate the two JSON-object body-guard helpers, then sweep the ~85 unshaped request.json() handlers #5587 originally fixed in api_memory_promote).
• SEL denial audits in handlers/files.py derive their error label from the guard response's code, so a 413 is audited as payload_too_large, not as a JSON parse failure.
• Frontend leg checked: no website/src code switches on the retired invalid_body / per-route invalid_json codes (remaining greps are unrelated i18n keys), so no frontend change rides along.

Testing

• Capped sites stream request.content, so every affected mocked-json harness now feeds real bytes via the new shared test/body_stream_helpers.py (BodyStreamPayload / attach_body), which also collapses what would have been ~13 copy-pasted stream stubs.
• New pinning tests: refused /interrupt body restores both _stop_state and _auto_run; oversized /api/mcp/apply body is 413 before decoding.
• Full backend suite: 77,363 passed; the 93 remaining reds are host-environmental on this dev box (identical set fails on a pristine main worktree at the same base).
• Gates: isort, flake8, mypy (1216 files), diff-scoped black / brand / subprocess-encoding / harness-parity all green. error-code-baseline.json regenerated per its own --update remedy (removed codeless 400 literals).

Pre-push blind review

Two model-pinned read-only lanes on the staged diff:

• GPT lane: 2 Medium findings, both fixed — /interrupt left _auto_run disabled on a refused body; _do_mcp_apply opted out of the cap despite bounded control data.
• Opus lane: 1 blocking finding fixed — cron create/update filed under the terminal uncapped reason despite a declared field bound (now explicit ceilings); 5 advisories adopted (narration comments, SEL 413 labels, dead mocks dropped, _Payload consolidation, user_meta narrowing comment), 1 verified-no-action.

Remaining files (tracked on #5587)

~131 bare sites remain across ~51 dashboard files, largest first: handlers/messaging.py (27), handlers/prompts.py (6), handlers/security.py (6), handlers/source_providers.py (6), handlers/core.py (5), handlers/updates.py (5), chat_folders.py (5), handlers/agents.py (4), handlers/hooks.py (4), handlers/side.py (4), plus ~41 files with 1–3 sites each.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 0a11a85158ca6a74a9a347b34af2f367a163f5e9 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Mechanical application of an established, register-audited guard pattern; per-site cap decisions are documented, pinned by tests, and fully reversible.

Suggestions

• _body_err_code recovers the machine code by re-parsing the guard's rendered response body; if the guard's wire shape ever shifts, SEL labels silently degrade to invalid_json_body — have read_bounded_json expose the code directly (attribute on the response or a third tuple slot) before more audit sites copy this.

[DESIGN-REVIEWED] 0a11a85

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0a11a85158ca6a74a9a347b34af2f367a163f5e9 — this comment is updated in place on each push.

Review details

All three candidates are low-confidence by their own discovery pass, and each falls apart under falsification:

Candidate 1 (batch-delete/queue-reorder 64 KB cap): the discovery pass itself could not establish a concrete input — it admits ">1600 items" is required and "the harm may be purely theoretical." That is a "might," not a real (a). The refusal is a 413, not a crash/data-loss/security hole. Drop.

Candidate 2 (cron enable/ack now 400 a malformed present body): I opened read_bounded_json — allow_absent=True still defaults an absent body (not request.can_read_body → {}), and only a present-but-malformed body 400s. This is deliberate, documented in the helper contract and comments ("sent nothing" vs "sent garbage"), and test-covered (test_cron_enable_treats_an_absent_body_as_defaults). No observable wrong outcome. Drop.

Candidate 3 (reset_conversation 400 on malformed body): same intentional-and-documented contract; the early return performs no teardown, preserving the reset-race invariant. Drop.

Step 2: I examined the only genuine logic — the api_chat_slot_interrupt rollback. It correctly saves prev_auto_run, restores both _stop_state and _auto_run on the refused-body and transport-failure paths, and gates the rollback on _stop_state == "soft_pending" so a concurrent escalation to "killing" is not erased. Both branches are test-covered. It is a fix, not a defect. Nothing new grounds to 80+.

No findings.

[OPUS-REVIEWED] 0a11a85

Verdict parsed from the review's SHA-scoped output markers for commit 0a11a85158ca6a74a9a347b34af2f367a163f5e9.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 0a11a85158ca6a74a9a347b34af2f367a163f5e9: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0a11a85158ca6a74a9a347b34af2f367a163f5e9 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is gathered. The mechanism (read_bounded_json, the _CAP_REGISTER guard test) pre-exists this PR; 0 class _Payload copies remain after the helper extraction; chat_pins.py:121 already re-parses the guard response the same way the new _body_err_code does; MAX_CRON_MESSAGE is 50,000 and the shipped constant is 12×, not the description's 4×.

First-Principles-Verdict: CONCERNS

The sweep is sound, but the malformed-body-now-400 flip lands on five chat-slot routes the description's three-route list never names.

What this change ships

Intent: stop valid-JSON-non-object bodies from turning 55 dashboard routes into 500s — a FIX (tranche 3 of issue #5587).

1. 55 routes answer 400 body_not_object instead of a 500 for non-object JSON — justified
2. 41 of them refuse oversized bodies with 413 before decoding — justified (helper's recorded design, cap register pins each choice)
3. Cron create/update get a sized ceiling _MAX_CRON_BODY_BYTES — justified; formula in description (4×) contradicts code (12×)
4. /api/mcp/apply gets a 1 MB pre-decode ceiling — justified (its change cap runs post-decode)
5. Present-but-malformed bodies on tolerant routes now 400, not silent defaults — declared for 3 routes, undeclared for 5 more
6. Refused /interrupt rolls back _stop_state and _auto_run — justified (closes a pre-existing leak)
7. files.py SEL audits label refusals by the guard's code — justified; re-parse duplicates a pattern at chat_pins.py:121
8. Retired invalid_body/invalid_json codes become body_not_object — declared, frontend consumers counted at zero
9. Shared test stream helper replaces the _Payload copies — justified (0 class _Payload remain; net deletion)
10. Error-code and coverage baselines regenerated — justified, mechanical

Watch

• The tolerant-flip list ("api_cron_enable, api_cron_ack, api_workflow_run_rerun") is incomplete: the same 200-with-defaults→400 change ships on api_chat_slot_create, api_chat_slot_end_wait, api_chat_slots_cleanup, api_chat_slot_resume, and api_chat_slot_reset_conversation (5 sites, each previously except → body = {}/None). Declared as a class, unenumerated — a client sending garbage to those routes notices.
• Description says the cron ceiling is "4×MAX_CRON_MESSAGE + 64 KB"; cron.py ships 12 * MAX_CRON_MESSAGE + 64 * 1024. Fix the description, not the code — 12 is the derived escape factor.
• _body_err_code (files.py:129) json-decodes the response the guard itself just serialized; chat_pins.py:117-128 is the second site doing this (count: 2, pattern json.loads(body_err.text)). Cause: read_bounded_json discards the code it built. Fine for this tranche; fold when the four divergent guards are folded.

[FIRST-PRINCIPLES-REVIEWED] 0a11a85

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0a11a85158ca6a74a9a347b34af2f367a163f5e9 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0a11a85

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 0a11a85158ca6a74a9a347b34af2f367a163f5e9: <one-sentence reason>

[bolichen97]

Description / code mismatch

Both wire-contract changes the Description declares are real in the code, but neither of the two specs that document those contracts is part of this change, so the authoritative prose now states the opposite of what ships.

1. The cron spec still documents the enable/ack default-on-malformed tolerance and the invalid_json code this change reverses

The Description says —

Sites that defaulted an ABSENT body keep doing so (allow_absent), but a present-and-malformed body is a 400 instead of silently collapsing to defaults (api_cron_enable, api_cron_ack, api_workflow_run_rerun — the same defect class #5587 originally fixed in api_memory_promote).

The code does — exactly that. api_cron_enable and api_cron_ack call _shared.read_bounded_json(..., allow_absent=True), so an ABSENT body still defaults, while a present scalar/array/null body is refused with 400 body_not_object and an unparseable body with 400 invalid_json. The spec paragraph is unchanged and contradicts this on both of its route-specific clauses: it states that POST /api/crons/{id}/enable and /ack "fall back to their defaults" on a body they cannot use, and that PATCH /api/crons/{id} and DELETE /api/lessons answer code: invalid_json for a non-object body — docs/system-specs/modules/learn-cron-dashboard.md:959-965.

Risk — AGENTS.md § Specification management is a MUST: "MUST update the spec in the SAME commit when an API, schema, or documented behavior changes." Left stale, the authoritative cron spec tells the next reader — and the next agent following the router table to it — that enable/ack silently default a non-object body and that update / lessons-delete answer invalid_json, which is the inverse of what ships. A contributor writing a client from the spec, or a later tranche reading it to decide a fold, gets the wrong contract, and the divergence is invisible because no gate compares prose to code.

Required change — In the same commit, rewrite the **JSON body shape** paragraph at docs/system-specs/modules/learn-cron-dashboard.md:959-965 to state the shipped contract: these routes go through _shared.read_bounded_json; a scalar/array/null body is 400 body_not_object, an unparseable body is 400 invalid_json, an over-cap body is 413 payload_too_large, and only an ABSENT body still defaults on /enable and /ack (allow_absent). While there, the POST /api/chat/slots/{slot}/end-wait code list at docs/system-specs/modules/learn-cron-dashboard.md:857-859 ("400 wait_id_required, 404 slot_not_found, 409 wait_not_in_flight") is now incomplete: chat_handlers.py::api_chat_slot_end_wait replaces its if not isinstance(body, dict): body = {} normalization with read_bounded_json(request, allow_absent=True), so that route gained 400 body_not_object / invalid_json.

2. The workflows spec still documents code: invalid_json for a non-object body on every workflow mutation route

The Description says —

Non-object bodies: 400 body_not_object (was a 500, or a custom 400 message on a few routes).

The code does — the /api/workflows/** mutation handlers route through _shared.read_bounded_json, which answers 400 body_not_object for a valid-JSON-non-object body and reserves invalid_json for a body it cannot parse at all. The spec paragraph documenting the wire contract for those routes still names code: invalid_json for a non-object body — docs/system-specs/modules/workflows.md:869-871.

Risk — That paragraph is the documented wire contract for the /api/workflows/** mutation surface, and it now names a machine-readable code the backend no longer emits for that input, so any client written from the spec branches on a code it will never see. Same AGENTS.md MUST as above. Unlike the cron case there is no compensating prose for this surface: body_not_object is documented once in the tree (docs/system-specs/modules/pptx-maker.md:186), and no doc states read_bounded_json's non-object contract centrally — its only other docs mention, docs/system-specs/modules/app-notifications.md:23, covers the size bound alone.

Required change — In the same commit, update docs/system-specs/modules/workflows.md:869-871 to name code: body_not_object for a valid-JSON-non-object body and invalid_json for an unparseable one. Recording the cap split in the same paragraph is welcome but is not what blocks: api_workflow_run_promote is the one capped route, so an over-cap body there is 413 payload_too_large, while the other seven read uncapped (max_bytes=None) because their bodies carry script source or a free-form intent.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #6066 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6066: REBASE. Purely mechanical collision on a generated file; whichever lands second regenerates the baseline with python test/test_error_code_contract.py --update. Files: error-code-baseline.json.
• This PR is OVERLAPPING with PR #2842. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7308: CONTINUE_DEVELOPMENT. Both change the same handler's body handling for different reasons; sequence them rather than closing either. Files: src/kiro_crew/dashboard/chat_handlers.py.
• This PR is OVERLAPPING with PR #5161. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7308: CONTINUE_DEVELOPMENT. Same nine handlers, same lines, complementary goals. Neither should be closed; sequence them and have the second author rebase onto the first. Files: src/kiro_crew/dashboard/handlers/mcp.py.
• This PR is OVERLAPPING with PR #7693. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7308: CONTINUE_DEVELOPMENT. Disjoint goals in the same handler bodies and the same two stop tests; both are wanted, so agree an order and rebase the second. Files: src/kiro_crew/dashboard/chat_handlers.py.
• This PR is OVERLAPPING with PR #7821. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7308: CONTINUE_DEVELOPMENT. Shared registry file, non-conflicting hunks, independent goals — no coordination needed. Files: test/test_json_object_body_guard.py.
• This PR is OVERLAPPING with PR #8302. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7308: CONTINUE_DEVELOPMENT. Adjacent edits in one handler with unrelated goals; a mechanical rebase for whichever lands second. Files: src/kiro_crew/dashboard/handlers/files.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.