fix(llm): fallback chain walk honors namespace-qualified model ids

[javenciu]

Problem / Motivation

A persisted agent.fallback_model chain entry that carries a
<namespace>::<bare-id> qualifier (the #8521 mismatch class — stored when a
catalog advertised the qualified spelling) is silently skipped by the
throttle-fallback walk when the session advertises the bare id.
next_fallback_candidate (src/kiro_crew/llm_helpers.py) filters the chain
by literal membership:

if adv and low not in adv:
    logger.debug("model fallback: skipping %r (not advertised)", cand)
    continue

so a chain entry the backend fully serves is treated as unadvertised, and the
walk falls through toward "auto"/chain exhaustion. On a partition that does
not serve "auto", the chain exhausts and the original throttle error
surfaces — the configured fallback never fires. Issue #8740 identified this as
the one literal-membership site left outside the #8616/#8737 fold.

Why it matters

Users who pinned a fallback while a catalog advertised qualified spellings get
no fallback at all at exactly the moment it exists for: sustained throttling
on the primary. The failure is silent (a debug-level skip log), so it reads as
"fallback feature does not work". The same stale-qualifier pins were already
judged worth folding at the substitute-send and picker sites (#8616, #8737);
the fallback walk was the remaining consumer of persisted model values still
comparing literally.

What changed (motivation → approach → change)

Observed symptom: a served-but-qualified chain entry is skipped as
unadvertised (reproducer below — the walk returns None on a chain whose only
entry the backend serves under its bare spelling).

Root cause: next_fallback_candidate judges membership with a literal set
lookup, and the issue's own analysis explains why the fold could not be a
one-line predicate swap: the walk's return value feeds two consumers with
conflicting spelling needs. FallbackState.next_candidate locates the applied
candidate with remaining.index(cand), so it needs the CHAIN's own spelling —
but the wire and every later served-model comparison (the
AcpClient.set_model explicit-pick guard, the silent no-op witness, the
TURN_FALLBACK_ATTR marker that probe_fallback_restore compares against the
served model, and fb_state.active/walked) need the ADVERTISED spelling. A
qualified spelling in the marker would make the restore probe read the session
as having moved off the fallback, clear the sticky state, and skip the slot
heal.

The change, keeping the two spellings from ever disagreeing:

• next_fallback_candidate judges membership and the active-model skip
  through resolve_pin_spelling (the shared fold from Namespace-qualified model ids: sibling comparison sites not covered by #8615 #8616/fix(models): fold namespaced pins at substitute and picker sites (#8616) #8737): full id
  first, one leading <namespace>:: peel on a miss. An entry absent under
  both spellings is still skipped (deny-parity), a verbatim-advertised
  qualified id is never peeled, and a qualified entry that resolves to the
  currently-failing model is skipped post-fold. The function still returns the
  chain's own spelling, preserving the remaining.index bookkeeping contract
  and all ten existing behavior pins.
• New fallback_wire_spelling is the single home of the chain→wire
  translation: advance_fallback_candidate computes it once per candidate and
  uses it for the in-loop active skip, the set_model call, the no-op
  witness, fb_state.active/walked, the published marker, the swap log, and
  the return value. Empty/unknown advertised set falls back to the entry's own
  spelling, matching the walk's existing fail-open stance.
• docs/system-specs/features/model-fallback.md candidate-walk section synced
  in the same commit.

Alternative rejected: returning a (chain_entry, wire_spelling) tuple from
next_fallback_candidate. It changes the function's contract and every
existing pin for the same information the shared fold can recompute
deterministically at the one wire consumer; the smaller surface keeps the fold
authoritative in one place.

Tests

Reproducer, before the fix (tests-only tree at base 235d36a62):

FAILED test/test_llm_helpers.py::TestAdvanceFallbackCandidateNamespacedChain::test_qualified_entry_applies_under_advertised_spelling - AssertionError: assert None == 'z-ai/glm-5.3-flash'
FAILED test/test_llm_helpers.py::TestNextFallbackCandidate::test_namespace_qualified_entry_folds_to_advertised_bare_id - AssertionError: assert None == 'openrouter::z-ai/glm-5.3-flash'
=================== 2 failed, 4 passed, 37 warnings in 4.61s ===================

After the fix: 6 passed, and the full targeted file 142 passed.

New pins and what each locks in:

• test_namespace_qualified_entry_folds_to_advertised_bare_id — a qualified
  entry the backend serves under its bare id is selected, and the CHAIN
  spelling is returned (bookkeeping contract).
• test_qualified_entry_peeling_to_active_model_is_skipped — post-fold active
  skip: a qualified entry resolving to the failing model cannot help.
• test_entry_absent_under_both_spellings_still_skipped — deny-parity: the
  fold does not weaken the advertised filter.
• test_verbatim_advertised_qualified_id_not_peeled — a full-id match wins;
  peeling never rewrites it.
• TestAdvanceFallbackCandidateNamespacedChain::test_qualified_entry_applies_under_advertised_spelling
  — end to end: set_model awaited with the bare spelling, marker
  (primary, bare), fb_state.active/walked carry the bare spelling, and
  pos advanced past the chain entry itself.
• TestAdvanceFallbackCandidateNamespacedChain::test_qualified_entry_peeling_to_failing_model_is_skipped
  — the walk exhausts rather than announcing a no-op swap when the chain names
  the failing model under a qualifier.

Neighboring suites green locally: test_subagent_turn_resilience.py +
test_model_selection_scenarios.py + test_api_models_entitlement.py
(84 passed), test_dashboard_chat.py -k fallback (16 passed). mypy, flake8,
isort, the black baseline gate, and docs-lint.sh all pass on the changed
files.

Manual verification

N/A — unit coverage sufficient: the walk, the wire call, the witness, and the
marker are all exercised through the same provider-double shape the existing
fallback tests use, and the seam has no UI or external-service surface.

Related Issues

Fixes #8740

Pattern harvest

Rule candidate: review-prompt
Pattern: "persisted model ids must be compared through the shared spelling
fold, never by literal membership; any value later compared against a SERVED
model must carry the advertised spelling". #8616 → #8737 → this PR closed the
enumeration one site at a time; a review prompt on new in advertised-style
membership tests over persisted model values stops the class from re-growing.

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

Per the template placeholder (CLA text pending): offered under the same terms as my prior merged contributions to this repository (#8835).

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 7582844604e248b31e6ae6fb5f40636e9af05815 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

Closes the last literal-membership consumer through the existing shared fold, keeps chain vs. wire spellings from diverging, and opens no one-way door.

[DESIGN-REVIEWED] 7582844

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 7582844

[github-actions]

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

Premise-level review of 7582844604e248b31e6ae6fb5f40636e9af05815 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 tree. I have what I need: the fold exists with five prior consumers, the restore-probe comparison at llm_helpers.py:725 confirms the wire-spelling rationale, and my sibling grep found no remaining literal-membership sites of this class. Final review:

First-Principles-Verdict: PASS

A real, silently-failing defect fixed at cause level with the existing shared fold; every item traces to the fix, one helper could shrink.

What this change ships

Intent: make a persisted namespace-qualified fallback entry actually fire when throttling hits, instead of being silently skipped. FIX.

1. A ns::id chain entry the backend serves bare now applies instead of exhausting the chain — justified (reproducer shown; llm_helpers.py:408 literal membership confirmed in base)
2. The applied fallback is sent, logged, and recorded under the advertised spelling — justified (probe_fallback_restore compares marker vs served model at llm_helpers.py:725; a qualified marker would clear sticky state)
3. A qualified entry resolving to the failing model is skipped post-fold — justified (deny-parity with prior behavior)
4. New public function fallback_wire_spelling — one consumer; see Subtractions
5. model-fallback.md synced same-commit — mandated by AGENTS.md
6. Six behavior pins added — justified

The fix reuses resolve_pin_spelling (acp/client.py:1791), the mechanism #8616/#8737 already established — not a second spelling. Sibling count: grepped (not )?in adv(ertised)? across src/; the remaining literal-membership hits are the "auto" sentinel checks, computer-use action ladders, and model_is_unusable itself (which documents at acp/client.py:1776-1783 why it deliberately does not fold). No unfixed siblings of the persisted-pin class found — the "last site" claim holds.

Subtractions

• Make fallback_wire_spelling module-private (or inline resolve_pin_spelling(cand, ids) or cand at the call site) — 1 consumer, advance_fallback_candidate in the same file; nothing outside the module needs the export.

[FIRST-PRINCIPLES-REVIEWED] 7582844

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

The single candidate does not survive falsification. Its scenario requires the primary/active to be stored in the qualified <namespace>::<bare-id> spelling while the backend advertises the bare id — the reverse of the fold's target direction. Tracing pre-diff vs. post-diff for that exact input (qualified primary as active_model, bare chain entry, bare advertised set):

• Pre-diff: low="z-ai/glm-5.3-flash" ≠ act="openrouter::z-ai/glm-5.3-flash", and low in adv → the bare entry is returned.
• Post-diff: served="z-ai/glm-5.3-flash" ≠ act → the bare entry is returned.

Identical outcome, so the diff introduces no regression here — the un-folded comparison target is pre-existing behavior the PR neither created nor worsened (the candidate itself concedes this and rates it low). Separately, (a) is not grounded: the served/active model read from provider_active_model and the marker (wire, now always advertised spelling) carry the bare spelling in practice; #8521's qualified spelling arrives via persisted config pins, not via the live served model that seeds primary, so the required input does not occur on this path. It fails the confidence-80 and concrete-input bar.

No new grounded defect found: the walk returns the chain's own spelling so remaining.index(cand) bookkeeping holds, wire/marker/records consistently carry the advertised spelling the restore probe compares against, and the post-fold active skip preserves deny-parity (an id unserved under both spellings still resolves to "" and is skipped).

[OPUS-REVIEWED] 7582844

[javenciu]

Re: Opus 4.8 suggestion — import resolve_pin_spelling from kiro_crew.acp.client directly

Commit 1 (0a830e7) had exactly that shape and failed Backend Lint & Type Check (3.12) on scripts/check_agent_sdk_boundary.py: adding a new symbol to the kiro_crew.acp.client import line is an added ACP edge on added lines, and the gate's baseline only covers pre-existing lines.

Per docs/request-for-change/rfc-crew-agent-sdk-boundary.md, kiro_crew.agent_sdk is the single import surface for application code, with kiro_crew.acp private to the driver behind it. Commit 2 (5c645c8) therefore reaches the helper through kiro_crew.agent_sdk.drivers.acp, matching the in-tree precedent at src/kiro_crew/session.py:114. The pre-existing direct acp imports in llm_helpers.py stay byte-identical to the baseline.

Happy to reroute if maintainers prefer a different shape, but as the gate stands, the suggested import is the one it rejects.

[javenciu]

Backend Tests (3.12, 4) failure — the #8893 flake, third occurrence, not this diff

test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery failed on shard 4 (run 34017247801). This is the exact test/assertion tracked in #8893, which already records its second occurrence on this PR's previous round (run 34014923783). This PR's diff is confined to llm_helpers.py model-fallback spelling and touches nothing in the snapshot/notification path; the test passes 5/5 locally at this exact head (5c645c8). The Coverage Gate failure is downstream of the same shard: the failed shard never staged/uploaded its coverage data.

Root cause is confirmed at source (details in #8893): both ordering tests patch os.open process-wide and fire their delivery trigger on the first O_CREAT open observed anywhere; on a loaded shard that first open can be a foreign thread's while the notification worker is still free, so the append runs immediately and the assertion reads scheduler noise as a broken ordering guarantee. A deterministic fix (destination-scoped triggers, no retry/sleep, plus a test that keeps the loaded-shard condition permanently reproduced) is built and heading into review.

No changes to this PR; requesting a rerun of the failed shard once maintainers deem it appropriate.

[javenciu]

CI note: the Backend Tests (3.12, 1) failure on this board is the known shard-runtime issue tracked in #8968 (test_channel_slots.py fails deterministically on any shard running >30 minutes; the frozen NOW diverges from the live-clock cutoff). This run's shard 1 ran 37 minutes (10:27:36Z to 11:04:46Z) and failed with the exact signature from that issue (assert 0 == 1 in TestReconcilePass). test_channel_slots.py is not part of this PR's diff (docs + llm_helpers.py + test_llm_helpers.py). Re-running the failed jobs.

[javenciu]

The Backend Tests (3.12, 4) failure on this PR (run 34035940758) is the known flaky ordering assert tracked in #8893 (TestNotificationCopyWhenNoLiveFileExists racing on loaded CI shards), not a regression from this change. Evidence:

• This PR touches only src/kiro_crew/llm_helpers.py, its test file, and a docs page. The failing module test/test_snapshot.py has zero references to llm_helpers, and snapshot.py / jsonl_util.py (the code under test) import nothing from it in either direction.
• Local reproduction at this PR head (7582844): test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery passes 5/5 consecutive runs.
• The Coverage Gate failure is a cascade of the failed shard (combine cannot complete), not an independent signal.

This is the 4th occurrence of the #8893 signature across recent CI runs on unrelated PRs. No code change is needed here; happy to have the shard re-run whenever convenient.