fix: hoist the empty-message 400 above every /api/chat dispatch branch (#4223)

[patrigao]

Problem / Motivation

The busy-slot branch of POST /api/chat guards slot.queue_append and the queue_push WebSocket broadcast on a non-empty message, but its {"ok": true, "queued": true} response returns unconditionally. The idle path's if not message: 400 sits below the busy branch, so it is unreachable while a turn is running. A programmatic caller (app token, curl, integration) that POSTs an empty message to a busy slot — possibly with attachments in meta — therefore receives an acceptance receipt for work that was silently dropped: nothing queued, no broadcast, attachments lost. The dashboard composer cannot hit this (it always inlines staged files into the wire text), so the affected surface is the HTTP API contract.

Why it matters

Every API caller that trusts queued: true is misinformed, and the response gives no way to distinguish this from a real enqueue — the failure is invisible until the user notices their attachment never arrived. Two review lanes on PR #4180 independently identified this server response as the root cause and asked for this separate filing.

What changed (motivation → approach → change)

Symptom: a false queued: true receipt for empty busy-slot sends. Root cause: guard placement — the single if not message: 400 sat below the early-returning busy branch, so any branch above it bypassed the check. Change: hoist that one guard above every dispatch branch (steer/queue, crew, subagent-hold, new turn), so no current or future branch can bypass it, and delete the now-redundant refusal copies. The 400 carries code: message_required — the backend-owned string already established for this refusal in handlers/messaging.py (AGENTS.md backend-string convention) — giving the error one machine-readable shape regardless of slot state. queued: true now implies a real queue_append on every path.

The first pushed revision cloned the refusal into the busy branch; the First Principles reviewer correctly flagged that this left the bug-causing placement intact, and the hoist is its suggested subtraction (2 refusal sites → 1 guard). Behavior is input-identical: the steer path already required a non-empty message, the crew and subagent-hold branches sat below the old guard, and the agent/theme side-effect ordering before the guard is unchanged.

Deliberately not done: queuing attachment-only sends (the larger behavioral alternative) — the honest refusal is consistent with the side-chat path, which also 400s on empty text. The two sibling {"ok": true, "queued": true} returns were verified truthful: the requeue-after-kill return fires only after the turn's finally requeued the steer (non-empty by the steer gate), and the subagent-hold return now sits below the hoisted guard.

Pre-push review (two model-pinned read-only subagents mirroring the codex-review and claude-review contracts): 0 blocking findings; 3 advisories fixed in this diff (comment framing corrected to programmatic callers, test docstrings reworded to present-tense invariants, code unified), 1 noted out of scope (see below). Server-side First Principles concern addressed by the hoist.

Tests

test/test_queue_during_subagents.py::TestApiChatBusySlotEmptyMessage (new, on a real slot made busy via a live never-done task, so slot.running is authentic):

• test_attachment_only_send_gets_honest_400 — busy slot + empty message + meta attachments → 400 with code: message_required, queue_depth == 0, zero queue_push broadcasts. Fails on the pre-fix code with assert 200 == 400 (verified red-first).
• test_nonempty_message_still_queued — busy slot + non-empty message → queued: true, one queue entry, exactly one queue_push broadcast.

Gates: isort / flake8 / mypy clean; full pytest run 55,239 passed on the initial head, affected suites (127 tests incl. the error-code contract) re-run green after the hoist. 459 failures in the run were reproduced byte-identically on pristine main in the same sandbox (missing editable-install entry-point metadata plus jq/docker/userns host drift) and are unrelated to this diff.

Manual verification

N/A — unit coverage sufficient: the change is a single guarded JSON response in one handler branch, and both the refusal and the preserved enqueue path are locked by handler-level tests against a real TestServer.

Related Issues

Closes #4223

Out-of-scope note for reviewers: restoreComposerAfterFailedSend in website/src/pages/ChatPage.tsx does not restore staged files after a failed send. Moot for this server-side fix (the composer cannot produce an empty-text send today), but worth a follow-up if the frontend ever allows one. The client guard added by PR #4180 is not in main, so no website/ change rides here; removing that guard once both land belongs to a follow-up.

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, internal API error-shape fix
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 14018b076da4dbf0f5521948beac2b2b72a40547 — updated in place on each push; does not block merge.

Diff is small and well-contained; the handler context confirms the guard sits after slot resolution/ownership checks (unchanged ordering) and above every dispatch branch, the steer/requeue sibling returns are genuinely non-empty-gated, and message_required reuses the existing code from handlers/messaging.py:294. The error-code baseline moved in the ratchet's permitted direction. Real harm (false queued: true receipt to API callers), root-cause fix (guard placement, not a per-branch patch), alternative (queuing attachment-only sends) weighed and declined with a consistent rationale, red-first tests against an authentically busy slot.

Design-Verdict: PASS

Root-cause fix at the right layer: one hoisted guard makes every dispatch branch honest, with the alternative weighed and the ratchet moved correctly.

[DESIGN-REVIEWED] 14018b0

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 14018b076da4dbf0f5521948beac2b2b72a40547 — this comment is updated in place on each push.

Review details

The candidate discovery pass found no candidates. I've independently verified the control flow:

• message = body.get("message", "").strip() (line 160) — empty/whitespace bodies normalize to "".
• The new guard (line 275) sits after color_theme/agent/app-ownership side effects but before every dispatch branch.
• Old busy path: empty message entered the busy branch, skipped both the steer sub-path (if body.get("steer") and message) and the queue block (if message:), then returned {"queued": True} — a false receipt. This is the bug being fixed.
• Old idle/crew/subagent/new-turn paths: all sat after the old guard at line 92, so they never received empty messages. The new guard preserves that exactly.

No legitimate empty-message flow is lost, the code: message_required field satisfies the backend-owned string contract, and the diff is a strict correctness improvement. No independent Step-2 defect surfaced.

No findings.

[OPUS-REVIEWED] 14018b0

Verdict parsed from the review's SHA-scoped output markers for commit 14018b076da4dbf0f5521948beac2b2b72a40547.

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 14018b076da4dbf0f5521948beac2b2b72a40547 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 14018b0

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Advisory premise-level review of 14018b076da4dbf0f5521948beac2b2b72a40547 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

Reading complete. I verified the guard placement, grepped for sibling empty-message guards across handlers, checked the error-code ratchet mechanics, and confirmed the message_required code precedent at handlers/messaging.py:294. Final review:

First-Principles-Verdict: PASS

One placement fix at cause level — the guard now precedes every early-returning branch, so a false queued: true is structurally impossible.

What this change ships

Intent: stop /api/chat from telling programmatic callers their empty-message send was queued when it was silently dropped — a FIX.

1. Empty message to a busy slot now gets a 400, not a false acceptance receipt — the fix, justified.
2. The empty-message 400 now carries code: message_required — justified (AGENTS.md backend-string invariant; code reused from handlers/messaging.py:294, not a second spelling).
3. The duplicate refusal copy in the busy branch is deleted, one guard remains — justified subtraction.
4. Error-code baseline regenerated — justified (test_baseline_is_not_stale mandates it).

Root-cause check: the cause named (guard below an early-returning branch) is the thing removed, not patched around. Sibling scan for the same defect shape — a message/question validity guard sitting below a dispatch branch: handlers/side.py:618-621, handlers/messaging.py:294, and spec_builder/backend/routes.py:3033-3035 all guard before dispatch; chat_orchestrator.py:739 queues a constant. 0 unfixed siblings. The two surviving {"ok": True, "queued": True} returns (chat_handlers.py:331, :457) both sit below the hoisted guard, so both are truthful.

Subtractions

• Drop the now-dead and message in if body.get("steer") and message: at chat_handlers.py:306 — the hoisted guard makes message non-empty on every line below it, and the PR deleted the equivalent if message: in the queue path but left this one.

[FIRST-PRINCIPLES-REVIEWED] 14018b0

[patrigao]

Disposition for First Principles CONCERNS on 059701c — fixed in 14018b0.

The concern held: the first revision cloned the refusal into the busy branch and left the root-cause placement (guard below early-returning branches) intact. Applied the suggested subtraction verbatim: hoisted the single if not message 400 above if slot.running or slot._in_stage_execution: and deleted both refusal copies (the busy-branch one and the old below-branch one) — 2 sites → 1 guard covering every dispatch branch, current and future.

Behavior is input-identical: the steer path already required non-empty text, the crew and subagent-hold branches sat below the old guard, and the 404/409 ordering plus the agent/theme side effects before the guard are unchanged. Affected suites (127 tests incl. the busy-slot receipt tests and the error-code contract) re-ran green.

[bolichen97]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix — hoists the empty-message 400 guard above all /api/chat dispatch branches, clear root cause, 3 files.

[bolichen97]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix — hoists empty-message 400 validation above all /api/chat dispatch branches, clear root cause (#4223).