feat(mcp-pool): Toggle all switch for poolable MCP servers

[chenmingwei23]

1. What is the problem?

The MCP Pool page lists every MCP server across the user's agent configs with a
per-row poolable switch, and that row switch is the only way to change the
allowlist. Turning pooling on for a whole install therefore costs one click per
server — 34 clicks on my machine, 48 rows on a fully-configured one — and each
click is its own POST /api/mcp-gateway/servers/poolable, its own config.json
rewrite, and its own in-process pool re-apply.

2. Why this issue matters to the user

Poolability is close to an all-or-nothing decision in practice: a user who wants
the shared gateway wants their stdio servers pooled, and a user backing out wants
them all off. The page made the common case the most laborious one, and doing it
by hand also means N sequential re-applies (each one relinks sessions) where one
would do. There was no way to see the aggregate state either — you had to read
34 switches to answer "is pooling on?".

3. How our fix solves it

Symptom: no way to flip the whole list; 34 clicks, 34 config writes.

Cause: the surface only ever exposed the per-row write, and the endpoint only
accepted a single name.

Fix, in two halves:

• Frontend (website/src/pages/settings/McpPoolableServers.tsx): a Toggle all
  row at the top of the list with a N of M pooled subline. Its state is derived
  from the eligible rows only — poolableEligible() drops the rows this UI can
  never write (denylisted, non-stdio, or poolable solely via the agent-JSON
  poolable:true escape hatch). Without that filter a single denylisted server
  would pin the toggle-all switch off forever and make its click a permanent no-op.
  toggleAllTargets() sends only the eligible rows that disagree with the next
  state, so a half-on list costs a write for the difference rather than a rewrite
  of every row. Row switches are held while the bulk write is in flight, and the
  toggle-all switch is held while any row write is — the two would otherwise race the
  same allowlist.
• Backend (src/kiro_crew/dashboard/handlers/mcp.py): the poolable endpoint
  now also accepts {"names": [...], "poolable": bool}. One config write under
  the existing config lock, one _mcp_gateway_apply_poolable(), one SEL record
  naming every server touched. The single-name form is untouched — same
  validation order, same response shape — so nothing else that calls it changes.
  _MAX_POOLABLE_BATCH (200) fences a hand-rolled request from turning one write
  into an unbounded one.

Because the write is one atomic request, the allowlist can never land
half-flipped, which is also why the error path reports the whole set instead of
per-row failures.

4. What tests we did

• New frontend unit tests (8) over the three exported helpers: eligibility
  filtering, checked-state with and without locked rows, unchecked when nothing
  is eligible, and target selection in both directions including the no-op case.
• New backend tests (8, TestGatewaySetPoolableBatch): the batch validation
  matrix (non-list, non-string member, empty, whitespace-only, traversal name,
  missing boolean), oversized batch, non-object body, add/remove semantics
  including a name absent from the allowlist, apply awaited exactly once for
  the whole batch, the audit naming every server, and a regression test that the
  single-name form still answers with name.
• Local gates, all green: pytest (262 passed across every suite touching
  poolable), isort/flake8/mypy clean, tsc -b clean, full vitest
  869/869 files with I18N_BASE_REF set, npm run i18n:check all 12 checks OK,
  eslint 0 errors.
• Pod e2e on an isolated kirocrew pod (127.0.0.1:7964, 5 servers): one click
  took the page from 0 of 5 poolable servers pooled to 5 of 5 poolable servers pooled with all five row
  switches following, and a second click returned it to 0 of 5 — asserted on
  aria-checked and the subline text, not just eyeballed.

all off	one click later

If the images do not render, they are in the Files changed tab under
temp-screenshots/mcp-pool-toggle-all/.

5. Any other suggestions on the work

UX Review's three advisory suggestions, dispositioned:

• Subline denominator mismatch — FIXED at 54ce4c018. n_of_m_pooled now
  reads {{on}} of {{total}} poolable servers pooled in all 13 catalogs, so the
  count names its own scope instead of implying every visible row. Screenshots
  re-captured against the new string and re-pinned.
• Rename Toggle all to Pool all — REBUTTED. The label is the one thing
  the requester specified by name ("add a toggle called toggle all"), and the row
  label Pool {{name}} names a single server's action while the toggle-all switch
  names the control. Happy to rename if the maintainers prefer the row verb to
  win.
• Binary switch from a mixed state — ACCEPTED AND DEFERRED as
  MCP Pool: Toggle all is binary, so clearing a mixed list pools everything first #2434. Clearing a mixed list does
  cost an on-then-off round trip that briefly pools excluded servers. Fixing it
  properly means changing the control to an indeterminate tri-state, which is a
  control-type change rather than a tweak; the batch endpoint already accepts an
  arbitrary subset, so no backend work is needed when it happens.

Two further notes:

• The subline uses a plain {{on}} of {{total}} string rather than a counted
  plural key, since both numbers are always present and no locale needs a
  singular form for the pattern.
• The batch endpoint is a natural fit for any future bulk poolable action
  (per-agent, per-transport); nothing here assumes "all".

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 629f377c82c750210d7bedb7a8e879601d34bedf — this comment is updated in place on each push.

Review details

The diff is clean. Validation on the batch path mirrors the single-name path (_is_valid_mcp_name rejects .. and enforces charset), the batch is bounded by _MAX_POOLABLE_BATCH, non-object bodies are rejected, and the config write remains atomic under the existing lock. Frontend uses i18n keys, lucide icons, and text sizes at/above the 10px floor. No guard removed without replacement; no reachable security or data-loss defect.

No findings.

[OPUS-REVIEWED] 629f377

Verdict parsed from the review's SHA-scoped output markers for commit 629f377c82c750210d7bedb7a8e879601d34bedf.

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real N-clicks/N-writes problem fixed at the root — an additive batch form on the existing endpoint, one atomic write, single form untouched.

The one design wrinkle (binary switch on a mixed list briefly pools excluded servers) is already acknowledged, correctly scoped as a tri-state control change, and tracked in #2434 with the backend already supporting arbitrary subsets — nothing further to act on here.

[DESIGN-REVIEWED] 629f377

[github-actions]

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of 629f377c82c750210d7bedb7a8e879601d34bedf — updated in place on each push; does not block merge.

Reviewing the frontend diff, the two committed screenshots, and the copy against the sibling MCP pool page conventions — done. Findings below.

UX-Verdict: PASS

One switch with an honest "N of M poolable servers pooled" subline, held rows during writes, an atomic error message — the flow reads cold and keeps its promises.

The earlier subline-denominator concern is fixed in this diff ({{on}} of {{total}} poolable servers pooled, re-shot in 01-all-off.png/02-all-on.png), the mixed-state binary-switch cost is acknowledged and tracked in #2434, and the remaining copy ("Could not update servers. No changes were made. Try again."), pending-state locking, and describedBy count exposure all hold up.

[UX-REVIEWED] 629f377

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 629f377c82c750210d7bedb7a8e879601d34bedf and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 629f377

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

[chenmingwei23]

Dispositioning UX Review's three suggestions at 54ce4c018:

1. Subline denominator mismatch — fixed. n_of_m_pooled now reads
   {{on}} of {{total}} poolable servers pooled across all 13 catalogs, so the
   denominator names the subset it counts instead of implying every visible row.
   Screenshots were re-captured against the new string on a fresh pod
   (0 of 5 poolable servers pooled → one click → 5 of 5 … → click →
   0 of 5 …, asserted on aria-checked and the subline text) and the PR body's
   raw URLs are re-pinned to this SHA.

2. Rename Toggle all → Pool all — rebutted. The label is the one detail
   the requester specified by name. The row label Pool {{name}} names an action
   on one server; the master row names the control that governs them, so I read
   this as a register difference rather than synonym drift. If maintainers would
   rather the row verb win, the rename is a one-line change across 13 catalogs
   plus a screenshot re-capture — say so and I'll do it.

3. Binary switch from a mixed state — accepted and deferred as MCP Pool: Toggle all is binary, so clearing a mixed list pools everything first #2434.
   Correct as described: from mixed, the switch reads off, so clearing a curated
   list costs an on-then-off round trip that briefly pools servers the user
   excluded. It is bounded (two writes, intent never lost) and the honest fix is
   an indeterminate tri-state control, which changes the control type rather than
   tweaking this one. The batch endpoint already accepts an arbitrary subset, so
   that work needs no backend change.

One local-only note for transparency: the full frontend suite exited non-zero
once on my machine with an unhandled window is not defined from
DevFleetPage.tsx:553 (a module-level toast timer firing after its jsdom
environment tore down), in a file this branch does not touch. It does not
reproduce in isolation (3/3 clean) and did not reproduce on the previous SHA's
full run. Watching CI's Frontend Tests to see whether it is real; if CI
reproduces it I will file it separately rather than fold it in here.

[chenmingwei23]

Three CI reds on 54ce4c018, all mine, all fixed at bc7507df4:

Backend Tests (3.10 shard 2) and Backend Tests (Windows shard 2) — one and the
same failure: test_no_new_error_response_without_a_code. The batch form added
five status=400 responses carrying only English prose, taking
handlers/mcp.py from 36 to 41 missing_code sites. The gate is right: that
prose lands untranslated in a localized page and, once shipped without an
identifier, freezes into the contract. Each new response now carries a
machine-readable code alongside the advisory prose — body_not_object,
names_not_string_list, names_required, names_too_many,
invalid_server_name. Pre-existing sites in this file are untouched; they stay
inventoried in error-code-baseline.json as Track B work.

Inclusive Language — two comments I wrote said "master switch". Replaced with
"toggle-all switch" in the component doc comment and the test comment, and in the
PR body prose for consistency.

Verified locally after the fix: test_error_code_contract.py +
test_handlers_mcp_coverage.py 121 passed, isort/flake8/mypy clean, tsc -b
clean, eslint 0, i18n:check 12/12, and the full frontend suite 869/869 exit 0.

That last one also settles the intermittent DevFleetPage.tsx teardown error I
flagged in my previous comment: it did not reproduce on this run, does not
reproduce in isolation (3/3), and lives in a file this branch does not touch, so
it is a pre-existing cross-file timer race rather than anything from this diff. I
have not filed it — if it shows up in CI I will, separately.

[chenmingwei23]

UX Review's CONCERNS round, dispositioned at 4fcdc7dd5:

could_not_update_all_try_again implied partial success — fixed. The write is
all-or-nothing, so the message now says so: "Could not update servers. No changes
were made. Try again." Updated across all 13 catalogs.

Mixed state reads as plain off — partially addressed, rest deferred. The
substantive half is real and I am not arguing it: at 4-of-5 pooled the switch sits
off, and a switch is a state indicator. What I can fix without changing the
control type is the accessible name's silence — the count line now carries an id
and is wired into the switch as aria-describedby, so an AT user hears
"0 of 5 poolable servers pooled" as the switch's description instead of inferring
state from position alone. (Chain verified by reading the code, not at runtime:
the id sits on the count div, describedBy is passed to Toggle, and Toggle
maps it to aria-describedby in website/src/components/ui.tsx.) The remaining
half — that a binary control cannot represent three states — is the tri-state
change tracked in #2434.

Rename toggle_all to "Pool all servers" — escalated to the requester, not
rebutted again. This is the second round raising it and the reasoning (domain-verb
consistency with Pool {{name}}, naming the click's outcome) is sound. I am not
overriding it on my own judgment either way, because "a toggle called toggle all"
is the requester's explicit wording and the label is the part he specified. Put to
him now; I will push the rename the moment he says so — it is one value in 13
catalogs plus a screenshot re-capture, so treat this as pending-decision rather
than declined.

Gates after the change: tsc -b clean, full frontend suite 869/869 exit 0,
i18n:check 12/12, eslint 0. Screenshots re-pinned to this SHA and verified;
their visible text is unchanged by this round.

[chenmingwei23]

Both suggestions from the PASS round are fixed at 629f377c8.

Dead control when nothing is eligible. The row rendered as a permanently
disabled switch over "0 of 0 poolable servers pooled" for anyone whose servers are
all HTTP or denylisted. The row is now not rendered at all when eligible.length
is 0, and the switch's disabled no longer carries the emptiness case — the
render guard owns it, so there is one place that decides rather than two.

Escape-hatch rows contradicting the count. A poolable:true agent-JSON row
shows its switch on while sitting outside the count's denominator, so the subline
could read "0 of 3 pooled" directly above a visibly pooled row. Rather than
concatenate a parenthetical onto a translated sentence, there is now a second
complete key — n_of_m_pooled_plus_agent, "{{on}} of {{total}} poolable servers
pooled, +{{extra}} via agent config" — selected when pooledViaAgentConfig() is
non-zero. Each catalog entry stays a whole sentence, which is what translators can
actually work with, and the fragment never has to survive a word-order change.

pooledViaAgentConfig() counts locked-and-pooled rows only. Three tests cover it,
including that denylisted and HTTP rows are excluded — they are locked but never
pooled, and counting them would invent servers in the pool. Mutation-verified:
relaxing the predicate to srv.poolable fails the suite (1 failed / 15 passed),
and it returns to 16/16 on restore.

Gates: tsc -b clean, full frontend suite 869/869 exit 0, i18n:check 12/12,
eslint 0. Screenshots re-pinned to this SHA; their visible text is unchanged,
because the pod that produced them has five eligible rows and no escape-hatch row,
so neither new path is on screen.