fix(channels): reject a non-string mention or thread_id instead of 500ing

[leonlaiyc]

Problem / Motivation

POST /api/channels/{id}/messages used raw JSON mention and thread_id
values in dictionary membership checks before validating their types. A dict or
nested list therefore raised TypeError: unhashable type and surfaced as a 500,
while hashable wrong types such as an integer or boolean could be silently
accepted.

This is the residual identified by the Design review on #5618, which fixed the
same class of defect for content in this handler.

Why it matters

The endpoint is used by the dashboard, gateway, agents, and scripts. A malformed
client field should produce a stable coded 400, not an opaque server error or a
successful mutation with silently changed input. Inconsistent behavior also
makes client recovery and audit logs unreliable.

What changed (motivation → approach → change)

• Validate mention before any member lookup. It must be a string or an array
  containing only strings; otherwise the handler returns coded 400
  channel_message_mention_type_invalid.
• Validate thread_id before any message lookup. A non-string returns coded 400
  channel_message_thread_id_type_invalid.
• Reuse the handler's existing _agent_field_error response helper instead of
  adding duplicate response sinks.
• Keep well-typed optional values compatible: an empty mention normalizes to
  None, an empty-string thread_id remains "", and an unknown non-empty
  member or message id is accepted and normalized to None. Only wrong-typed
  values are refused.

On base bcf2efb732fe3f20f2691bd41b1dd3f1ce00b88d, the committed
error-code-baseline.json totals are missing_code=1262, opaque_body=18,
dynamic_status=43, and _compliant=1387. A fresh generator run on both current
main and the final PR tree reports the same totals and _compliant=1388; that
one-site display-count drift already exists on main and is unchanged by this
diff. The generated baseline is therefore left untouched rather than mixing an
unrelated refresh into this contributor change.

Open-PR overlap was audited globally. #5248 touches the same handler in a
different, much larger logic area and is harder to merge; #5977 is the
easier-first change. Other intersecting PRs only share the generated baseline,
which this final diff no longer changes. No contributor hunk is replaced.

The related raw source membership issue in handlers/taskrunner.py belongs to
a different endpoint and provenance gate. It should be handled as a separate
small PR rather than expanding this contributor's scope.

Tests

• Regression cases cover dict/list/int/bool mention, mixed and nested mention
  arrays, and dict/list/int/bool thread_id; all refuse before channel mutation.
• Compatibility cases cover unknown string ids plus both empty optional fields.
• Final strict suite: 123 passed, 1 platform-conditional skip, with
  RuntimeWarning and PytestUnraisableExceptionWarning promoted to errors.
• Focused final: 18/18 message validation tests and 24/24 with the error-code
  contract tests.
• isort, flake8, the Black ratchet, full mypy (1168 source files), and
  git diff --check pass.
• No retry, sleep, warning filter, timeout increase, or tolerance relaxation was
  used.

Manual verification

N/A — the defect is a request-body contract exercised directly with the exact
JSON shapes that previously crashed or were silently accepted.

Related Issues

Residual identified by the Design review on #5618. No separate issue was filed.

Checklist

• At most two commits (exactly one), with a Conventional Commits title
• Existing affected tests pass and regression coverage is included
• Self-review completed; repository style gates pass
• Documentation updated (not applicable: existing API contract is enforced)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of d8c4712d87941fba71d30b52bef79b9532f58ada via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix for a real 500, done in the handler's own established pattern (_agent_field_error, coded 400s), with the compat edges tested and stated.

[DESIGN-REVIEWED] d8c4712

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of d8c4712d87941fba71d30b52bef79b9532f58ada via the fork AI-review pipeline — 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 checks are done — the fix is verified against the base, the helper reuse is real, and I've counted siblings of the root cause. Here is the review:

First-Principles-Verdict: PASS

A named 500 (unhashable JSON in dict membership) gets a coded 400 via the handler's existing helper; every item is the fix or declared.

What this change ships

Intent: stop POST /api/channels/{id}/messages from 500ing or silently accepting a wrong-typed mention/thread_id. This is a FIX.

1. A dict/list mention now returns a coded 400 instead of a 500 — justified (base handlers_channel.py:297-298 hashes raw JSON against ch.members).
2. An int/bool mention now returns 400 instead of being silently passed/dropped — justified.
3. A non-string thread_id now returns 400 instead of 500 or silent drop — justified (ch._msg_index membership at base line 302).
4. An empty-string mention now normalizes to None instead of passing "" through — rides along on the is not None restructure, but declared in the description and inert downstream.
5. Two new error codes (channel_message_*_type_invalid) — justified: mandated by the AGENTS.md invariant that every new non-2xx JSON body carries a code.
6. Regression tests asserting refusal happens before channel mutation — justified.

Reuse checked: the response goes through the existing _agent_field_error (handlers_channel.py:318), the same shape api_channel_add_agent already uses — no second spelling.

Watch

Sibling count for "raw body value crashes the handler": I grepped dashboard handlers for raw body.get membership/method calls and found 2 unfixed — handlers/taskrunner.py:178 (unhashable source in a set, declared and deferred by the description) and handlers/updates.py:1738 (.upper() on a non-string → AttributeError 500, undeclared). The promised follow-up PR should take both.

[FIRST-PRINCIPLES-REVIEWED] d8c4712

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed d8c4712d87941fba71d30b52bef79b9532f58ada via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] d8c4712

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed d8c4712d87941fba71d30b52bef79b9532f58ada via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d8c4712

[bolichen97]

Tier 1 auto-approve: fix (2 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 with a clear root cause — a non-string mention or thread_id in the channel-message body hit a dict membership lookup and raised TypeError (500); now rejected with a 400 and a machine-readable code before any mutation, hardening existing parsing without adding a new input surface. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[bolichen97]

Tier 1 auto-approve: fix (2 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 with a clear root cause — mention membership is a dict lookup, so an unhashable JSON value raised TypeError and surfaced as a 500; the handler now rejects a non-string mention entry or thread_id with a 400 and a distinct error code before any mutation, and the existing drop-unknown-value behaviour is unchanged. No new parse surface: the same body fields are read and used the same way. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

[iamwhatever]

Tier 1 auto-approve: fix (2 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: an unhashable mention or non-string thread_id in the channel-post body raised TypeError on the ch.members / _msg_index dict lookup and surfaced as a 500 -- both are now type-checked into a 400 before any mutation. Narrows the input space reaching ch.post; no new field is read and the membership filter is unchanged. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.