feat(developer): bring back the agent-backend switch, with KAS - #6398
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS A developer-surface switch that says what it changes, scopes it ("new sessions only"), explains the dead option, and recovers cleanly from both failed reads and failed writes. Suggestions
[UX-REVIEWED] 2bc6c0a |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/i18n/locales/en.json:6325 -- False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All claims verified. Composing the review. First-Principles-Verdict: CONCERNS The switch earns its place, but the PATCH allowlist ships a third copy of the selectable-backend set when an importable second copy already sits in What this change shipsIntent: let an operator pick which agent new sessions run, from the dashboard — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 2bc6c0a |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound, well-gated control over an already-validated field — but the module spec now documents a switching story this PR quietly obsoletes. Watch
[DESIGN-REVIEWED] 2bc6c0a |
6e935d8 to
725ecab
Compare
725ecab to
e4b0329
Compare
e4b0329 to
9eb75b8
Compare
9eb75b8 to
36530c0
Compare
|
Round 2 on GPT — narrow-viewport overflow (blocking): fixed. The segmented control's track was Measured rather than eyeballed, at 320px: track Frontend Lint (Copy/paste detection): inherited, cleared by rebase. The two jscpd clones were both in Design Review suggestion (advisory, not taken here): a test asserting every schema-advertised value has a rendered option would catch a genuinely new backend id being silently hidden. Agreed that the gap is real, but the fix belongs with whatever adds a fourth id — a test today would assert over a three-id set that the component already covers exhaustively, so it would pass without constraining the case it is meant to catch. Screenshots re-captured against a correctly-staged build (an earlier set was captured from a stale |
36530c0 to
9435b7a
Compare
|
Round 3 on Why they were wrong, not just imprecise. The retired lines asserted per-agent facts — "isolates what it runs in an OS sandbox", "shares one process across sessions", "can be steered mid-turn", "does not report subagent progress yet" — in the view layer, where nothing could contradict them. Nothing in this branch measured any of it, and "Amazon internal only" was flatly untrue. A claim with no source of truth behind it will drift the moment a backend changes, silently, so removing them is the fix rather than editing them. What replaces them, borrowed from #6307's ACP-adapter card:
Those are #6307's own strings ( What I deliberately did NOT copy. #6307's card also shows Evidence. Screenshots re-captured (the old four showed the retired copy, so they were stale). The harness now reads the rendered Behaviour is unchanged and re-proven on the new build: Gates, after rebasing onto main HEAD One locale gate caught this revision: |
The core has been able to run three agents for a long time -- Kiro CLI, KAS and
Claude Code -- and shipped no way to pick between them. `agent.acp_backend` was
validated end to end (`_normalize_acp_backend` against `ACP_BACKENDS_SELECTABLE`)
and published in the config schema, but the UI that used to switch providers was
deleted when the website collapsed to KiroACP-only, so the field was switchable in
principle and unreachable in practice.
This adds Developer > Agent Backend: a segmented control over the three agents,
writing `agent.acp_backend`.
Two things it does NOT do. It does not resurrect the old `ProviderPanel` --
Bedrock model ids, the Claude Code migration wizard and a one-member provider enum
are all gone. And it does not invent a vocabulary: the selectable set is read from
GET /api/config/schema, which the backend derives from the same field metadata the
PATCH validator uses, so the enabled options and the values the wire accepts cannot
disagree. Claude Code is the case that makes that worth doing -- this build does
not include it. Hiding it would imply it does not exist; enabling it would produce
a 400 from a control that looked live. It is listed, disabled, and says which it
is. A build that ships another agent lights it up with no frontend change
(asserted by a test).
Each agent's row carries one DERIVED status line rather than a prose capability
sentence. An earlier revision of this branch asserted per-agent facts -- OS
sandboxing, one process shared across sessions, mid-turn steering, subagent
progress -- in the view layer, where nothing could contradict them, and they were
wrong in the way unmeasured claims usually are. The vocabulary now comes from the
ACP-adapter card rather than being invented a second time: `Default. All features
supported.` for the backend whose descriptor is all-supported, `Experimental` for
one that is not, and `Not enabled in this build` for one this build cannot run.
Which line a row gets is derived, not authored -- not-enabled is read off the same
schema that gates the buttons, so a build widening the enum flips Claude Code's row
with no edit here (asserted by a test). Per-capability detail -- which feature is
supported, degraded or unverified per backend -- needs the descriptor table that
owns those facts and is deliberately NOT restated in the view.
Backend, both minimal:
* `agent.acp_backend` joins `_EDITABLE_CONFIG` -- without it every save came
back "field not editable". The values duplicate `ACP_BACKENDS_SELECTABLE`
as a literal because reaching `kiro_crew.acp.types` at module scope executes
the acp package init (client + runtime) while the dict is being built, the
same cycle `_normalize_acp_backend` defers for; a parity test fails in both
directions so the literal cannot drift.
* the field joins the `refresh_defaults()` list, so a new session picks the
agent up without a gateway restart and a pre-warmed Kiro CLI process cannot
serve a session that asked for KAS. Deliberately NOT
`reload_provider_factory()`: switching the default agent must not kill
in-flight turns, and live sessions keep the agent they started on.
`SettingsButtonGroup` gains per-option `disabled` so a known-but-unavailable
choice can stay visible instead of the control silently changing shape between
builds -- and, with it, per-option `describedById`. Dimming carries "unavailable"
both visually and through the native `disabled` state, but the REASON is rendered
outside the component, so proximity was the only thing associating them, which is
no association at all for a screen reader. The option now points at its own status
row via `aria-describedby`. That ships with the `disabled` feature rather than
after it: every future consumer that disables an option inherits the same gap.
A failed config read no longer renders as a selection. `?? KIRO` is right for a
config that omits the key -- the shipped default really is Kiro CLI -- and wrong
for a read that FAILED, where the value is simply unknown and defaulting paints
Kiro CLI as pressed, telling an operator running KAS they are on Kiro. The tab now
offers a retry instead of guessing.
It also gains `flex-wrap` + `max-w-full` on the track. It was `inline-flex ...
w-fit` with no wrap, so at 320px a group whose labels are words rather than single
glyphs -- "Kiro CLI" / "Claude Code" / "KAS (kiro-agent)" -- pushed past the card
and hid choices off-screen. A control you cannot see is a control you cannot use.
Every consumer narrows the same way now; the long font-family group benefits too.
Measured rather than eyeballed: at 320px the track's scrollWidth equals its
clientWidth, the document does not scroll horizontally, and the buttons lay out on
two rows.
Placed under Developer rather than Settings on purpose:
`gen-settings-registry.mjs` scans `pages/settings/`, and indexing this into
Settings search would advertise it as an ordinary preference -- it changes which
agent binary runs.
Also hardens `ChatSidebar.tagFilter`'s remount assertion, which waited for one
session to appear and then SYNCHRONOUSLY asserted another was gone. That is a
barrier the test never established, so any branch changing React's batching --
adding i18n keys is the common one -- flips it red. The absence now gets its own
`waitFor`.
"Claude Code", "KAS" and "kiro-agent" are added to the i18n do-not-translate
glossary -- the changed-passthrough gate asks for exactly that rather than a
worked-around translation, and every occurrence of the three is new here, so no
existing locale value changes meaning.
9435b7a to
2bc6c0a
Compare
|
Local GPT-lane mirror (
|
|
Local Opus-lane mirror (
|
|
Follow-up for the deferred GPT finding is filed as #6502 (background sessions ignore |
What
Adds Developer → Agent Backend: a three-way switch over the agents that can run a session, writing
agent.acp_backend.Why
The core has been able to run three agents for a long time — Kiro CLI, KAS and Claude Code — and shipped no way to pick between them.
agent.acp_backendwas already validated end to end (_normalize_acp_backendagainstACP_BACKENDS_SELECTABLE) and already published in the config schema, but the UI that used to switch providers was removed when the website collapsed to KiroACP-only. The field was switchable in principle and unreachable in practice.This is not a revival of the old
ProviderPanel: Bedrock model ids, the Claude Code migration wizard, and a one-member provider enum stay gone.What each option says
Each agent gets one derived status line, not a prose capability sentence.
An earlier revision of this branch wrote per-agent claims — OS sandboxing, one process shared across sessions, mid-turn steering, subagent progress — directly in the view layer, where nothing could contradict them. They were wrong in the way unmeasured claims usually are, so they are gone.
The vocabulary is now borrowed from the ACP-adapter card in #6307 rather than invented a second time:
Default. All features supported.ExperimentalNot enabled in this buildWhich line a row gets is derived, not authored: not-enabled is read off the same schema that gates the buttons, so a build widening the enum flips Claude Code's row from not-enabled to Experimental with no edit here. There is a test for that.
Per-capability detail — which individual feature is supported, degraded or unverified per backend — needs the descriptor table that owns those facts (#6307's
acp/backends.py). It is deliberately not restated in this view, because a copy here could disagree with the gateway and nothing would go red.All three lines are always shown: the reader is choosing between the agents, so showing only the selected one's status would hide the comparison the control exists for.
The choices come from the server
The selectable set is read from
GET /api/config/schema, which the backend derives from the same field metadata thePATCH /api/config/kirocrewvalidator uses — so the enabled options and the values the wire accepts cannot disagree.Claude Code is the case that makes this worth doing: this build does not include it. Hiding it would imply it does not exist; enabling it would produce a 400 from a control that looked live. So it is listed, disabled, and says which it is. A build that ships another agent lights it up with no frontend change — there is a test for that.
While the schema request is still in flight every option stays enabled rather than flashing disabled and then live; the PATCH allowlist is the real gate, so an optimistic enable can only ever cost one visible refusal.
Backend (two minimal changes)
agent.acp_backendjoins_EDITABLE_CONFIG. Without it every save came backfield not editable. The values duplicateACP_BACKENDS_SELECTABLEas a literal because reachingkiro_crew.acp.typesat module scope executes thekiro_crew.acppackage init (client + runtime) while the dict is being built — the same import cycleconfig.loader._normalize_acp_backenddefers for.test/test_agent_backend_editable.pyfails in both directions if the two ever drift, and also pins the schema enum to the same set.refresh_defaults()list. A new session then picks the agent up without a gateway restart, and a pre-warmed Kiro CLI process cannot serve a session that asked for KAS. Deliberately notreload_provider_factory(): switching the default agent must not kill in-flight turns, and live sessions keep the agent they started on.Shared component
SettingsButtonGroupgains per-optiondisabled, so a known-but-unavailable choice can stay visible instead of the control silently changing shape between builds. Additive and backwards-compatible.Placement
Under Developer, not Settings, on purpose:
gen-settings-registry.mjsscanspages/settings/, and indexing this into Settings search would advertise it as an ordinary preference. It changes which agent binary runs.Drive-by test hardening
ChatSidebar.tagFilter's remount case waited for one session to appear and then synchronously asserted another was gone — a barrier the test never established. Any branch that changes React's batching (adding i18n keys is the common one) flips it red, which is exactly what this branch did locally. The absence now gets its ownwaitFor.i18n
10 keys across all 12 catalogs,
en-XAregenerated (the reword retired 5 keys and added 4, so the tab is one key lighter than the previous revision).Claude Code,KASandkiro-agentare added to the do-not-translate glossary — thechanged-passthroughgate asks for exactly that instead of a worked-around translation, and every occurrence of the three is new in this PR, so no existing locale value changes meaning. Four locale style gates were caught and fixed locally: a droppedKiroproduct name in German (\bKiro\bneeds a boundary, andKiroshas none), the formalआपthatstyle/hi.md§4 forbids,回合wherestyle/zh-CN.md§2 pins turn to轮次, and — new in this revision — three stacked的in one zh-CN clause, whichstyle/zh-CN.md§3 caps at two (启动时的代理became启动时所用的代理with the clause split on;).Verification
tsc -b --forcevitest run(full)eslint src/ --max-warnings 664npm run i18n:checkblack/flake8/isort/mypy-n 4)¹
DiagramLightbox.zoom.test.tsxneedsfs/promises.glob(Node 22). Local Node is 20, and it fails identically without this diff — nothing in it touches that sweep.Evidence
Captured from a throwaway pod. Each step re-reads
GET /api/config/kirocrewafterwards, so the evidence shows the write landing, not only the control repainting.1. Shipped default —
agent.acp_backend == "", Kiro CLI pressed, Claude Code disabled and saying so.2. Switched to KAS — a real click, and the server then reports
agent.acp_backend == "kas".3. Switched back — config returns to
"".4. At a 320px viewport — the selector wraps to two rows instead of overflowing the card.
A screenshot alone cannot prove the absence of overflow — a clipped row still looks fine — so the harness measures it: the track's
scrollWidthequals itsclientWidth(244), the document does not scroll horizontally (320 == 320), no button's right edge passes the track, and the buttons land on 2 rows.The harness also forces a click on the disabled Claude Code option, past the
disabledattribute, so the check is that nothing is written rather than that the DOM refused the pointer:Because the point of this revision is the copy, the harness reads the rendered
<dl>back rather than leaving it to the eye, and separately asserts the removed prose is absent from the live DOM — not merely absent from the bundle:The
[copy]line searches the page text forOS sandbox,steered mid-turn,AnthropicandNot available in this build; the empty list is the assertion that none of the retired wording survived into the shipped bundle.Follow-up (not in this PR)
There is currently no per-agent setting to surface: the one genuinely agent-scoped knob, MCP Tool Search, already has a toggle in Developer → Config, and reasoning effort is a model-family setting in Settings → Chat rather than an agent one. If Tool Search moves here later it should move, not be mirrored — it is a no-op on the Claude backend, so presenting it as global is slightly wrong today.
no linked issue: this restores a control the UI lost when the website collapsed to KiroACP-only; there was no filed issue tracking it. The
#6307references above are to that PR as the source of the status vocabulary, not to an issue this PR resolves. The one issue this work did produce — #6502 — is a deferred follow-up that this PR deliberately does NOT fix, so it must stay open on merge.