fix(stt): type-check the config PUT's model and provider before the lookup

[leonlaiyc]

Problem / Motivation

PUT /api/config/stt applies thirteen optional fields. Eleven of them
type-check the value before using it. Two do not — and one of those two is a
dict-key lookup:

if "provider" in body and body["provider"] in _stt_providers():        # no isinstance
    stt["provider"] = body["provider"]
if "model" in body and body["model"] in _STT_MODEL_SIZES:              # no isinstance
    stt["model"] = body["model"]
if (
    "mlx_model" in body
    and isinstance(body["mlx_model"], str)                             # <- the sibling
    and body["mlx_model"] in _STT_MLX_MODELS
):
    stt["mlx_model"] = body["mlx_model"]

mlx_model and parakeet_model sit immediately below model with an
identically-shaped membership test and do guard it. So do
transcribe_region, transcribe_profile, language_code (isinstance(..., str))
and streaming, endpointing, dictation_panel (isinstance(..., bool)).

_STT_MODEL_SIZES is declared dict[str, str] (core.py:480), so
body["model"] in _STT_MODEL_SIZES hashes the caller's value.
{"model": {"size": "small"}} or {"model": ["small"]} therefore raises
TypeError: unhashable type out of the handler and reaches the client as a
500, where every guarded sibling would have quietly ignored the field.

Why it matters

The dashboard's Settings pane and anything else that speaks this API get a
server fault for a client mistake: an unhandled TypeError with a stack trace
in the log, an opaque 500 on the wire, and no way for a caller to tell "I sent
the wrong type" from "the gateway is broken". The eleven guarded fields on the
same endpoint answer the same mistake with a 200 and an ignored field, so today
the failure semantics depend on which key you got wrong.

Scope of the damage is worth stating precisely, because it is narrower than the
500 suggests: the raise lands inside _get_config_lock(), after config.json
has been read and before _atomic_json_write, so nothing is persisted and the
lock releases normally. This is a availability/ergonomics defect on the request,
not a torn config — the tests assert the stored section is byte-identical after
a rejected PUT.

What changed (motivation → approach → change)

The root cause is that the type check and the allowlist check were collapsed
into one membership test, which works only for values that are hashable. Adding
the isinstance restores the two-step form the other eleven fields use:

• model and provider get isinstance(body[...], str) before the membership
  test, in the same and-chain shape as mlx_model / parakeet_model.
• The behaviour on a bad value stays "ignore, don't fail", matching all
  eleven siblings — not a 400. A 400 here would be a behaviour change for every
  other field's contract on this endpoint, and a much larger review surface than
  the defect warrants.
• provider is checked against a list, so nothing raises there on main today.
  It is guarded anyway: it is model's immediate neighbour under the same
  contract, and guarding only model would re-create the arbitrary split this
  PR exists to remove. It also removes the latent 500 the moment
  _stt_providers() returns a set or a dict.

Deliberately not changed: stt["enabled"] = bool(body["enabled"]) two lines
above. bool() never raises, but it does silently accept anything —
{"enabled": "false"} currently enables STT, while the streaming /
endpointing / dictation_panel siblings require a real bool. That is a real
inconsistency, but fixing it would start rejecting clients that legitimately
send 1 or "true" today, which is a compatibility decision rather than a
crash fix and belongs in its own PR.

Tests

New test/test_stt_config_field_types.py, mirroring the harness in
test_stt_config_atomic.py (a MagicMock(spec=web.Request) PUT, with the GET
tail's host probes neutralised).

The fixture stubs ensure_ffmpeg_in_path as well as _stt_prereq_commands and
is_available, and that third one is there for a different reason: it does not
merely read the host, it writes os.environ["PATH"]
(transcribe.py: os.environ["PATH"] = d + os.pathsep + ...) whenever a
candidate directory holds an ffmpeg that PATH does not already list, and never
restores it. Left live, thirteen requests through this handler would hand every
later test in the pytest worker a mutated PATH.
test_the_fixture_neutralises_the_path_mutating_probe asserts the stub is
installed by identity rather than by comparing PATH before and after — a
PATH-equality check passes vacuously on a host whose ffmpeg is already on PATH
or absent from every candidate dir, so it would not notice the stub being
dropped. Without the stub that guard fails on any host
(assert <function ensure_ffmpeg_in_path> is not <function ensure_ffmpeg_in_path>).

Note for the reviewer: the sibling fixture in test_stt_config_atomic.py has
the same gap and is left alone — it is pre-existing and not this diff's to
change.

Each test seeds a known-good stt section with a valid PUT first, snapshots it,
then sends the wrong-typed PUT and asserts the stored section is unchanged.
The seeding matters: the first successful PUT is also what materializes
config.json with its defaults, so a snapshot taken on a fresh home would be
comparing a missing file against a defaulted one and would pass or fail for
reasons unrelated to the change.

The defect, and the controls, are labelled as such rather than presented as one
uniform block of coverage:

• test_unhashable_model_is_ignored_not_a_500 — {"size": "small"},
  ["small"], [{"size": "small"}]. This is the defect.
• test_a_wrong_typed_field_does_not_discard_the_valid_ones — a dict model
  alongside a valid language_code; the good field must still land.
• test_other_wrong_typed_model_values_stay_ignored — 3, True, None.
  Control: these are hashable, so main already rejects them; pinned so the
  new guard cannot change what "unknown model" does.
• test_non_string_provider_is_ignored — control for the same reason;
  _stt_providers() is a list, so none of these raise on main.
• test_valid_model_and_provider_still_persist and
  test_an_unknown_but_well_typed_model_is_still_ignored — controls that
  the guard neither rejects good values nor changes the unknown-value path.

Red-before, measured against pristine origin/main production code
(c7f5ba788) with the new file in place — 4 failed / 9 passed. Every
failure is the defect, at the line the diff changes, and the nine controls pass
exactly as they should:

core.py:611: TypeError: unhashable type: 'dict'
core.py:611: TypeError: unhashable type: 'list'
core.py:611: TypeError: unhashable type: 'list'
core.py:611: TypeError: unhashable type: 'dict'

Green-after: 14 passed. Blast radius: 320 passed / 18 skipped across
test_stt_config_field_types.py, test_stt_config_atomic.py,
test_stt_endpointing.py, test_stt_stream.py, test_stt_dictation_panel.py,
test_dashboard_handlers_core_coverage.py and test_transcribe.py.

flake8, isort, mypy and black are all clean on both files —
handlers/core.py is not on .github/black-baseline.txt, so it is checked
strictly, and it stays unchanged under black --check.

Manual verification

N/A — unit coverage sufficient: the defect is a request-body type contract on a
single handler, and the tests drive that handler directly with the exact JSON
shapes that reproduce it, then read the persisted config back.

Related Issues

Self-reported while auditing membership tests against unvalidated JSON values.
Same defect class as #5977 (api_channel_post's mention / thread_id), in a
different subsystem; the two do not overlap and neither depends on the other.
No separate issue was filed.

Checklist

• At most two commits (one is the norm), 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)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

The fix is verified against the base: _STT_MODEL_SIZES is a dict (core.py:502), so the unguarded membership test at core.py:654 raises TypeError on an unhashable JSON value and escapes as a 500; the added isinstance guards restore the two-step check every guarded sibling on this handler already uses, keeping the pre-existing ignore-don't-fail contract. Tests pin both the defect and the unchanged control paths. No contract change, fully reversible, right layer.

Design-Verdict: PASS

Real 500-on-client-error fixed at its root cause, in the pattern the sibling fields already use, with the endpoint's contract deliberately unchanged.

[DESIGN-REVIEWED] 73fcb5f

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The diff is straightforward and sound: it adds isinstance(..., str) guards before two membership checks. The _STT_MODEL_SIZES membership test is against a dict, so an unhashable body["model"] (JSON object/array) would raise TypeError: unhashable type and turn a partial config update into a 500. The guards are additive, match the existing skip-invalid-apply-siblings contract used by sibling fields, and run under the config lock with atomic write. No behavioural regression, no rule violation, and nothing new is grounded to the Step 2 bar.

No findings.

[OPUS-REVIEWED] 73fcb5f

[github-actions]

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

Premise-level review of 73fcb5f793f171bf455670e64441d3ee6ad3aa84 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 claims verified against the base. Composing the review now.

First-Principles-Verdict: PASS

A real 500-on-request crash fixed at its cause — the collapsed type-plus-allowlist test — with the last crashing sibling covered and zero new surface.

What this change ships

Intent: stop a wrong-typed model/provider in the STT settings PUT from returning a 500 instead of being ignored like every other field — a FIX.

1. A JSON object/array sent as model is now silently ignored, not a 500 — justified (verified: _STT_MODEL_SIZES is a dict at core.py:502, so body["model"] in hashes the caller's value).
2. A non-string provider is now ignored by the same guard — justified as the same root cause's sibling (same collapsed membership test, two lines above), though nothing crashes there today.
3. New test file pinning the field-type contract, 13 tests — justified; drives the real handler and asserts the stored section is unchanged.

The fix restores the exact two-step spelling already used at chat_voice.py:72,78 — no second spelling, no new mechanism. Sibling count for the crash class (body[...] in, grepped repo-wide: 6 hits): 2 fixed here, 2 already guarded (chat_voice.py), 2 tuple-membership (whatsapp_setup.py:76, weixin_qr.py:430) which cannot hash and cannot crash. Nothing is left unfixed.

Watch

• The description was written against a different revision than this diff: it quotes mlx_model/parakeet_model guards that do not exist in this repo's handler, names tests (test_the_fixture_neutralises_the_path_mutating_probe, test_non_string_provider_is_ignored) absent from the shipped file, and claims "14 passed" where the diff contains 13. The code claims all check out against base; refresh the description so the shipped tests are the ones it defends.

[FIRST-PRINCIPLES-REVIEWED] 73fcb5f

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 73fcb5f

[bolichen97]

Tier 1 auto-approve: fix. 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 (2 files, root cause: STT config PUT did a dict-membership lookup on unhashable/non-string model/provider, turning a partial update into a 500). 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: the STT config PUT looked up body["model"] / body["provider"] in a dict catalog without a type guard, so a JSON object or array field raised TypeError: unhashable type and turned a partial update into a 500 -- an isinstance guard now precedes each membership lookup, keeping the existing skip-invalid-field-apply-valid-siblings contract, with 121 lines of field-type tests. Hardens an existing parse path; introduces no new deserialization. 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: isinstance guard added before two dict-membership lookups on an existing PUT body, so a JSON object or array raises no unhashable-type 500; skip-the-field-keep-valid-siblings follows the existing config contract, no new field or endpoint is consumed. 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.