fix(dashboard): stop a stale agent-config snapshot resurrecting app MCP bridges

[chenmingwei23]

What is the problem?

PUT /api/agent/config persists a whole-file snapshot the dashboard's raw agent
editor read earlier. #6975 made on-disk state authoritative for a name the
submission omits -- an app bridge registered after the client's read is no
longer clobbered. The mirror case stayed open: a namespaced name the submission
still contains was written back verbatim.

So an editor tab that loaded while app demo was installed and running holds
demo:notes in its snapshot. Uninstall or disable the app -- _deregister_mcp_servers
removes the bridge -- then press Save in that old tab, and the entry comes back.

Why this issue matters to the user

The resurrected entry is a live MCP server in the config kiro-cli reads, so the
removed app's code becomes launchable again through it. And it does not go away:
reconcile_enabled_app_resources only re-registers enabled apps, so startup
reconciliation never revisits the namespace of an app that was disabled or
uninstalled. Nothing logged, nothing to notice -- the entry simply persists until
someone hand-edits the file.

The same shape reaches a third, worse state. _register_mcp_servers deliberately
refuses to write an HTTP server with no resolvable live port, and scrubs any stale
row for it, because a manifest's illustrative port is a reachable-looking dead URL
that kiro-cli dials on every request: one connect failure there is retried and
then surfaced as a hard error, breaking every kiro session on the host rather than
just that app's tools. A stale snapshot wrote that scrubbed row straight back.

How our fix solves it

The chain is: the PUT trusted the client's copy of a name in a region the client
does not author, and the platform's <app>:<server> region is written only by
_register_mcp_servers. So absence from disk is a verdict, not a gap:

• the app was uninstalled (bridge deregistered, app directory gone);
• the app is installed but disabled (same deregistration, never reconciled back);
• the app is installed and enabled but the row was skipped on purpose (the dead-URL
  case above).

_drop_unbacked_app_entries drops a submitted mcpServers name containing :
that the installed spec does not hold. Three consequences worth naming:

• The declared-name census is not consulted. The absent-axis rule must name an
  owner because it decides whether to keep something the client asked to remove;
  every candidate here is one the client is adding, which the on-disk map answers
  alone. Consulting the census would also rescue the third case, writing back
  exactly the URL the registration path scrubbed. It also means this rule does no
  manifest I/O and cannot raise app_ownership_unreadable, so it adds no new way
  for a PUT to fail.
• Host-owned names are excluded, including an edition extra whose key contains
  :. The host axis is unchanged.
• Both directions read the spec once, through the new _on_disk_mcp_servers,
  so they cannot disagree about their baseline. None (unreadable) and {} (read,
  no servers) are kept distinct, because only the new rule's verdict turns on the
  difference: against {} a namespaced submission is an addition, against None
  nothing is known and nothing is decided.

What this deliberately does not do: where the name is on disk and submitted,
the submitted row still wins untouched. That is the editor-snapshot-wins contract
kept in #5899 and re-affirmed for #6664, and reversing it is a separate ruling --
see the last section.

The cost, stated plainly: a name containing : can no longer be introduced through
this raw editor. The MCP page is the path that adds a server, after which the name
is on disk and this rule leaves it alone. Nothing becomes unremovable -- an entry
already on disk stays deletable through the absent-axis rule. The alternative was
to trust a colon-shaped key the client invented, which is indistinguishable from
the resurrection this exists to stop. Dropped names are logged at WARNING.

What tests we did

New section (d) in test/test_agent_config_merge_on_write.py, all through the live
handler:

• test_a_stale_snapshot_cannot_resurrect_an_uninstalled_apps_bridge -- the
  headline case; the user's own plain entry in the same submission is untouched.
• test_a_stale_snapshot_cannot_resurrect_a_disabled_apps_bridge -- also declares
  the same name in the user's own mcp.json, pinning that a scope declaration does
  not rescue it.
• test_a_stale_snapshot_cannot_rewrite_a_deliberately_skipped_http_bridge --
  installed, enabled and declaring the server, still dropped; this is the row that
  pins the census out of this axis.
• test_both_directions_decide_from_one_baseline_in_a_single_put -- one PUT that
  drops a stale addition and preserves an absent owned bridge.
• test_a_readable_spec_with_no_servers_still_drops_a_namespaced_addition --
  {} is an answer.
• test_an_unreadable_spec_still_persists_a_namespaced_entry -- best-effort holds,
  so this endpoint stays the repair path for a corrupt spec.
• test_a_plain_client_entry_the_client_adds_is_still_persisted and
  test_a_namespaced_edition_extra_the_client_resubmits_is_left_alone -- scope
  guards.

Mutation-verified against base: the five assertions of new behaviour all fail on
main with the intended messages, and the three scope guards pass on base as they
must. Full file 44/44 green, and 110/110 across the three test files that exercise
this endpoint (test_agent_config_merge_on_write.py,
test_api_agent_config_put_succeeds.py, test_json_object_body_guard.py).
test_app_owned_entry_present_in_the_snapshot_is_updated is green and unmodified,
which is the mechanical proof that the kept contract was not reversed. flake8,
isort, black and mypy clean on the touched files.

docs/system-specs/modules/app-kit-platform.md gains section 1a with the
present-axis decision table, so the spec does not disagree with the code.

Any other suggestions on the work?

• The substitution half is still open, filed as agent-config PUT: a stale snapshot can revert a live app bridge to a superseded definition #7470. Where the name is on disk
  and the submission carries a superseded definition of it (an older port or
  command), the submitted row wins and reverts a correction the registration path
  had made. Closing that reverses the twice-kept editor-snapshot-wins contract, so
  it needs a maintainer ruling rather than a fix decided in review. It is also
  materially less severe: unlike resurrection it self-heals, because an enabled
  app's bridge is re-registered with its live port on the next gateway start.
• Optimistic concurrency (the issue's second candidate shape) would close both
  halves at once by version/etag-ing the snapshot and having the editor reload on a
  stale write. That is a new API contract plus a client reload path -- feature work,
  and not something to fold into a defect fix. Carried in agent-config PUT: a stale snapshot can revert a live app bridge to a superseded definition #7470 alongside the
  substitution shape, so the two can be ruled on together.
• The dropped names go to the log only. Surfacing them in the PUT response so the
  editor can show "2 entries were not saved" would need _commit_agent_config to
  return more than its current changed flag, whose single-return shape is
  load-bearing in that function's documented contract; worth doing separately if
  the UX matters.

Pattern harvest

Rule candidate: review-prompt

Pattern: a whole-file PUT classifies the shared region in only ONE direction, so
the mirror direction persists a stale snapshot verbatim

This is the second defect in one span from the same root: a PUT that persists a
client snapshot over a file the gateway also writes. #6664 was the absent
direction, #7089 the present direction, and both were found by review rather than
by a gate. The mechanical form: for any endpoint accepting a whole-file snapshot
of a file with more than one writer, require the handler to classify every key of
the shared region in BOTH directions -- present in the submission and absent from
it -- and pin the classification as a table. The absent half alone reads as
complete and is not, which is exactly how this axis survived #6975's review.

A cheaper enforcement while that is unbuilt: a ratchet test asserting that the
region a non-client writer owns (here mcpServers keys containing :) is decided
by on-disk state on both axes, so adding a new owned region without a
present-axis rule fails the suite instead of shipping.

Closes #7089

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 7fac60828e617eefa56cc70c1f5433f217672fb4 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the design questions I probed — the colon-lexical region boundary, the deliberate census exclusion, the None/{} distinction, the deferred root cause — are resolved coherently in the diff, documented in the spec update, and the residual risks (silent drop feedback, superseded-definition half, optimistic concurrency) are already named and tracked by the author in #7470. Nothing survives the kill-filter.

Design-Verdict: PASS

Narrow, well-bounded closure of the present-axis resurrection; costs stated, alternatives weighed, residual halves explicitly filed rather than silently left open.

[DESIGN-REVIEWED] 7fac608

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 7fac60828e617eefa56cc70c1f5433f217672fb4 — 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: the kept-contract test exists at test/test_agent_config_merge_on_write.py:1034, mcp_server_alias does preserve colons in slash-free names (src/kiro_crew/mcp_utils.py:302), the _put helper pre-exists with all injection seams, and no other dashboard handler performs a whole-file client-snapshot write over a gateway-written file (grepped write_text(json.dumps|json.dump( across dashboard/handlers/ — one unrelated hit). No existing mechanism covers the present axis: _merge_unowned_servers only re-adds absent names, and the governance filter governs grants, not names.

First-Principles-Verdict: PASS

A reported resurrection defect (#7089) fixed at the mechanism, with its one removed capability declared, priced, and given a surviving path.

What this change ships

Intent: stop a stale dashboard editor snapshot from re-creating MCP bridges the platform removed — a FIX.

1. Saving an old editor tab no longer resurrects an uninstalled app's bridge — justified (reported defect, removed code stays launchable).
2. Nor a disabled app's bridge, which nothing ever removes again — justified (reconciliation provably skips disabled apps).
3. Nor a dead-URL bridge the platform scrubbed on purpose — justified (one dead URL breaks every kiro session).
4. New :-named servers can't be created via the raw editor; the MCP page still can — declared cost, derived (client copy is indistinguishable from a resurrection).
5. Dropped entries are logged at WARNING; the save still returns 200 — declared, with the response-surface follow-up filed.
6. Both PUT rules now decide from one spec read (_on_disk_mcp_servers, 2 consumers) — justified (rules disagreeing on baseline is a defect).
7. A readable spec with no mcpServers key counts as "no bridges", not "unknown" — justified, reachable state, test-pinned.
8. Spec gains section 1a — mandated by the AGENTS.md same-commit spec rule.

Root cause is honestly placed: the deeper cause (a whole-file snapshot PUT over a multi-writer file) is named, its remaining half — a submitted row superseding a platform correction — is counted (1 unfixed sibling, self-healing) and filed as #7470 rather than smuggled in. No duplicate mechanism exists: the declared-name census was considered and rejected on a concrete ground (it would rescue the scrubbed row), not on preference. No new config key, flag, or public surface ships; zero one-way doors beyond the declared editor restriction, which stays reversible.

[FIRST-PRINCIPLES-REVIEWED] 7fac608

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7fac60828e617eefa56cc70c1f5433f217672fb4 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7fac608

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7fac60828e617eefa56cc70c1f5433f217672fb4 — this comment is updated in place on each push.

Review details

I've analyzed the candidate and the surrounding code paths.

The candidate concerns the docstring/comment claim that _drop_unbacked_app_entries "cannot raise ... and adds no new way for a PUT to fail," arguing emission_eligible_mcp_servers() can raise PlatformCompositionError on a non-standalone host.

Falsifying it:

• emission_eligible_mcp_servers() → _extra_mcp_servers() → safe_context_call(current_context().mcp_tooling.extra_mcp_servers(), fallback={}) (agent.py:989). On the Default adapter — the only one in this public OSS fork — _extra_mcp_servers() returns {} and cannot raise (agent.py:973-994). The PlatformCompositionError re-raise only exists on a non-standalone edition, which is the de-Amazoned internal component that is not in this repository.
• So requirement (a) — "a concrete input or condition that occurs in practice" — is not met here; establishing it requires assuming edition code not present in this codebase. The candidate itself concedes "On the OSS standalone build ... this is unreachable" and rates its own confidence low.
• The narrower doc claim, "cannot raise AppOwnershipUnreadable," is accurate: _drop_unbacked_app_entries never calls _app_declared_server_names(), the only source of that exception.
• Even in the hypothetical edition, the outcome fails closed with byte-identical targets (no data loss/corruption) — a documentation-precision nit, not a reachable defect on a changed path.

This is below the 80 bar and is at root a comment-accuracy observation, which this pipeline does not treat as a finding. Dropped.

I re-derived the core logic (namespace : check, name not in existing, host exclusion via the emitter's set, mutation of the shared submitted/config["mcpServers"] dict, and ordering against the merge whose absent set cannot contain a dropped-because-off-disk name) and found no grounded defect to add.

No findings.

[OPUS-REVIEWED] 7fac608

Verdict parsed from the review's SHA-scoped output markers for commit 7fac60828e617eefa56cc70c1f5433f217672fb4.

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

[chenmingwei23]

Dispositions for both advisory lanes on e51d0baea, both acted on in 38b87a36c.

Design Review -- "the deferred optimistic-concurrency/etag follow-up (or the proposed ratchet test) is worth filing now so per-axis rules stop accreting in this handler." Agreed, filed: #7470. It carries the remaining content axis (a stale snapshot reverting a live bridge to a superseded definition), both candidate shapes with the contract reversal priced, and the ratchet test as the cheap gate to take even if neither shape is chosen. Worth correcting one thing in my own PR body while I am here: I wrote "third defect in one span" in the Pattern harvest section. It is the second (#6664 absent, #7089 present) -- fixed in the rewritten section.

First Principles -- "Shrink _drop_unbacked_app_entries's ~90-line docstring to the rule plus a pointer to app-kit-platform.md section 1a -- the same decision table and rationale now live in both, in prose, and two copies of one table diverge." Agreed and fixed: the docstring is down from ~90 to ~45 lines and now names the spec section as this axis's designated home, mirroring how the absent axis keeps its table in _app_declared_server_names with the spec pointing there.

What I deliberately kept rather than moved, because a reader of the code has to know it to not undo it:

• the three ways absence from disk is reached (uninstall / disable / deliberate HTTP skip) -- this is the justification for the rule, not a table row;
• why the declared-name census is NOT consulted. This is the part a later change is most likely to re-introduce "for consistency" with the absent axis, and doing so would rescue the third case and write the dead URL back. Same reason _merge_unowned_servers keeps its own census-precedence paragraph inline;
• the pointer to test_app_owned_entry_present_in_the_snapshot_is_updated, so the kept contract is discoverable from the function that could plausibly break it.

What moved out entirely: the cost paragraph, the host-exclusion paragraph and the order-independence paragraph are now one sentence each or live only in section 1a.

Re-verified after the trim: 60/60 across test_agent_config_merge_on_write.py and test_api_agent_config_put_succeeds.py, black/flake8/isort/mypy clean, single commit.

The PR Hygiene failure was mine and unrelated to either lane: the gate wants a line matching ^ *(Rule candidate|Not generalizable) *: and I had written **Rule candidate.**. The section now uses the template's exact Rule candidate: / Pattern: form.

[chenmingwei23]

GPT 5.6 BLOCKING on 38b87a36c (agents.py:217, "a missing mcpServers key bypasses stale-bridge removal") -- agreed, real, fixed in 8c2aee50e. No override sought.

I checked reachability rather than taking it on the label, and the finding is stronger than "a keyless spec is conceivable": the state is produced by this handler itself.

• _write_installed_config persists the submitted config verbatim, so a PUT whose submission omits mcpServers leaves the spec with no such key.
• apps/bridges.py::_deregister_mcp_servers reads mcp_data.get("mcpServers", {}) and deletes out of that, so when the key is absent it operates on a throwaway dict and never adds the key back.

So a spec can sit keyless while an old editor tab still carries demo:notes, and _on_disk_mcp_servers returning None for it made _drop_unbacked_app_entries stand down -- restoring an executable bridge for a disabled app, which is exactly the axis this PR exists to close. The bug was mine and it was in the one line that decides whether the rule runs at all.

Fix is the one GPT named. A missing key now answers {} (read cleanly, holds no bridge -- a definite answer); None is reserved for a spec that cannot be interpreted. Split explicitly:

if "mcpServers" not in on_disk:
    return {}                       # definite: holds no bridge
servers = on_disk["mcpServers"]
return servers if isinstance(servers, dict) else None

A mcpServers present but not an object deliberately stays None. The file parsed, but that value cannot be read, and deleting the client's entries on the strength of it is the same guess _merge_unowned_servers's submitted-side guard already refuses to make. Both rows are now in the spec table (section 1a) rather than only in prose.

Two tests, both mutation-verified against 38b87a36c (not just against main), which is the check that proves they cover the finding rather than the feature:

• test_a_spec_with_no_mcpservers_key_still_drops_a_namespaced_addition -- FAILS on 38b87a36c with "a spec with no mcpServers key was treated as unreadable...", passes now.
• test_a_malformed_mcpservers_value_on_disk_decides_nothing -- passes on both, pinning that the fix did not widen into the uninterpretable case.

The absent-axis rule is unaffected: it tests if not existing, for which {} and None are both falsy, so its verdicts are byte-identical either way.

Gates on 8c2aee50e: 62/62 across test_agent_config_merge_on_write.py and test_api_agent_config_put_succeeds.py, black/flake8/isort/mypy clean, still one commit.

Separately: Backend Tests (3.10, 3) went red on the previous head with an Event loop is closed annotation (.github#515) and no test name in the annotations. The run's logs were not yet downloadable while it was still in progress, so I have not attributed it yet -- I am not calling it a flake without the log. The new head re-runs that shard; I will read the log and report either way rather than let a red pass unexplained.

[chenmingwei23]

GPT 5.6 advisory on e3f0f23bd (agents.py:249, the rule sentence contradicts the best-effort path) -- agreed, fixed in 64cbe9f74. Docstring prose only; no behaviour change, no test change.

The finding is right and it matters more than a wording nit, because that sentence is the one a reader takes the rule from. It read "may the client CREATE a name in the app-namespace region? It may not", which is unconditional, while the implementation answers nothing when the installed spec is unreadable and lets the submission land -- a path documented two paragraphs lower and pinned by test_an_unreadable_spec_still_persists_a_namespaced_entry and test_a_malformed_mcpservers_value_on_disk_decides_nothing. So the docstring stated a stronger rule than the code enforces, which is exactly the drift the spec-in-the-same-commit rule exists to prevent.

Now: "Not while the installed spec is READABLE", plus one sentence saying why the qualifier is load-bearing rather than a hedge -- an unreadable spec answers nothing, so the submission lands as it did pre-fix. The spec's section 1a table already carried both rows, so the two are consistent again in both directions.

Also confirming the state this head arrives in, since the previous one is where the lanes converged: on e3f0f23bd all four lanes returned green on that exact SHA (Design Review PASS, First Principles PASS, GPT no blocking findings, Opus no blocking findings), every check run was success/skipped, and PR Readiness read "Eligible automated validation passed for this revision". The three earlier Backend Tests (*, 3) reds plus the Coverage Gate behind them were #7492's files.py log-site census drift on main, not this PR -- confirmed from the 3.10 and 3.12 shard logs naming test_the_census_holds_no_slack, by Coverage Gate's own log reading backend-test=failure -- failing closed, and by both shards passing on #7492 itself; they cleared when I rebased onto ecab0babe.

Gates on 64cbe9f74: 62/62 across test_agent_config_merge_on_write.py and test_api_agent_config_put_succeeds.py, plus test_security_posture.py::TestGateSideLogRedactorSpelling 5/5, black/flake8/isort/mypy clean, still one commit.

[chenmingwei23]

GPT 5.6 advisory on 64cbe9f74 (agents.py:250, "only _register_mcp_servers writes there" contradicts the custom-server path) -- agreed, and it is factually wrong as I wrote it. Fixed in 7fac60828, in both the docstring and the spec. Prose only; no behaviour change, no test change.

I verified the claim in code rather than accepting it: mcp_server_alias (mcp_utils.py:302) returns a name UNCHANGED when it contains no /, so a colon survives it (npm:foo stays npm:foo), and handlers/mcp.py::_sync_mcp_to_agent_unlocked writes that alias straight into this spec's mcpServers. So there are two legitimate writers of a colon-containing key here, not one, and the single-writer sentence was the weaker claim GPT says it is.

The reframing it suggested is the right one, so the region is now described as reserved from this endpoint rather than as having one writer. Both writers are named, and the sentence that matters is stated positively: a name either of them actually placed on disk is present, and therefore untouched -- what this rule withholds is only the ability to INTRODUCE such a name through the raw editor, where the client's copy is indistinguishable from a resurrection.

Worth being explicit that this changed no behaviour and exposed no gap. The rule was already keyed on presence on disk, never on which writer put it there, and the cost paragraph already said the MCP page is the path that adds a server "after which this rule sees it on disk and leaves it alone" -- consistent with the sync writer existing. Only the justification clause overstated. The same correction is applied to spec section 1a so the two do not drift.

On the loop: this is my last push for an advisory-only finding. Each of the last two pushes drew a fresh prose advisory (keyless-key BLOCKING -> readable-spec qualifier -> single-writer claim), and all four lanes have returned green on every one of those heads. Continuing to re-roll a full matrix per sentence is not convergence. Any further NON-blocking advisory I will disposition in writing here without amending; I will push again only for a genuine blocking finding or a red attributable to agents.py, test_agent_config_merge_on_write.py, or app-kit-platform.md.

Gates on 7fac60828: 62/62 across test_agent_config_merge_on_write.py and test_api_agent_config_put_succeeds.py, black/flake8/isort/mypy clean, still one commit.

[bolichen97]

CI fully green, all findings dispositioned. Approved.