feat(session-control): add session_close to archive a peer session

[chenmingwei23]

What is the problem?

The session-control MCP surface lets one chat session create, stop, send to, and read a peer session -- but there is no way to close one. An agent can stand up a workstream with session_create and drive it, but when that workstream is finished the only thing it can do is session_stop (cancel the turn), which leaves the tab open. Cleaning up is left entirely to the human clicking the X. The natural mirror of "open a session" was simply missing.

Why this issue matters to the user

Session control exists so an agent can manage a fleet of sessions on the user's behalf (a watcher it spun up, a refactor it handed off). Without a close verb, every session the agent opens is a tab the user has to remember to dismiss -- the agent can make the mess but not tidy it. Over a long orchestration this accumulates dead tabs in the sidebar, which is exactly the friction the create/stop/send/read set was built to remove.

How our fix solves it

Adds session_close -- the tool-side equivalent of pressing the tab X.

• It reuses the dashboard's own close path, it does not reimplement it. api_chat_slot_delete carried a large, invariant-heavy close sequence (synchronous tombstone, auto-nudge-loop retirement before the awaits so no nudge resurrects the tab, app close-hook notification with rollback, persist-as-closed, per-tab session teardown). That sequence is extracted verbatim into a shared close_slot(state, slot, name) coroutine that raises a typed SlotCloseError on its three failure paths; the DELETE endpoint and session_close now share it, so the two can never diverge on the ordering that keeps a dismissed tab from waking back up.
• Symptom -> root cause: the symptom is "agent can't clean up its own sessions"; the root cause is that closing lived only inside an HTTP handler bound to request, unreachable from the session-control layer. The fix makes the close sequence a callable the layer can authorize and invoke, rather than copying its logic.
• Same authorization as the other target verbs. close_target routes through the existing deny-by-default authorize_target(operation="close"), so every guard already covering stop/read applies unchanged (self-target, unattended, incognito/temporary, app-scoped, channel-linked, channel-mirrored, crew-mode, cross-workspace). It is added to SESSION_CONTROL_TOOLS (strict caller-identity gate) and to CHANNEL_AGENT_BLOCKED_TOOLS.
• Non-destructive by design. Closing archives the conversation to history (closed=True) -- it can be reopened later. It is not a permanent delete, matching the X button. An in-flight turn is cancelled first (its work discarded), so the tool description tells the caller this is heavier than session_stop and to read the session first.
• Honest failure codes. The three close-path failures surface as their own SessionControlError (nudge_retire_failed / app_close_hook_failed / history_save_failed) at HTTP 500, so a caller can tell "the app refused the dismissal" from "history could not be saved". This required adding a 500 branch to the handler's _refusal status map (a close failure is a server error, not a client 400).

What tests we did

• New close_target unit tests: real end-to-end archive (slot leaves _slots, per-tab session torn down), self-target refusal through authorize_target, SlotCloseError -> SessionControlError code/status mapping with a denied audit line, and the prewarm-before-gate / gate-adjacent-to-act adjacency contract (mutation-guarded, mirroring the stop tests).
• New close route tests: forbidden without the internal secret, reaches the operation with it, and renders a close-path 500 as 500 (not a crash or a degraded 400).
• Updated the pinned ratchets that assert the tool set on purpose: the kirocrew-dashboard registration ratchet, the advertised-set test, and the refusal-status-mapping test (500 is now mapped; the degrade-to-400 case uses a genuinely unmapped status). The channel-containment ratchet derives from SESSION_CONTROL_TOOLS, so it covered session_close automatically.
• Ran the close-path invariant suite (test_slot_close_nudge_race.py, test_autonudge_dashboard_fire.py, test_unattended_slot_guardrails.py, test_ephemeral_sessions.py) to confirm the close_slot extraction preserved the DELETE endpoint's behavior -- all green (session-control + boundaries + folders + registration + channel: 277 passed; close-behavior suite: 198 passed).

Any other suggestions on the work

• The _refusal status map is now {403, 404, 409, 429, 500}; if more server-side failure shapes appear it may be worth deriving the set from the errors that can actually be raised rather than listing statuses.
• A frontend affordance is out of scope here -- this is the tool/endpoint only; the human X path is unchanged.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound reuse of the ✕ path, but the enforced scope is any workspace peer while every description says "a session you created."

Watch

• The commit message and tool description frame session_close as closing "a peer session it opened," yet authorize_target (deliberately uniform across verbs) enforces no creator containment — so an agent, including one steered by untrusted content, can dismiss a tab the human is actively working in and discard its in-flight turn, with the "tidy up sessions you created" guidance purely advisory. Creator provenance already exists (creator_slot_count), so a per-verb containment is buildable; if uniform scope is the intended design for the heaviest verb, say so in the spec rather than implying ownership scoping in the prose.

Suggestions

• The unrelated black-reformatting hunks (validation.py, channel.py, folder-resolver code) belong in their own commit per AGENTS.md's baseline-pruning rule; bundled here they widen this PR's blast radius on revert.

[DESIGN-REVIEWED] 1747253

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 17472535edb0132e978118d38c63139509cb39ba and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 1747253

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 17472535edb0132e978118d38c63139509cb39ba — 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 the load-bearing claims check out: notify_slot_close_undone and _restore_slot_nudge_loop pre-exist (apps/teardown.py:483, chat_handlers.py:3461), stop_target holds the gate-adjacent-to-act contract with no await window and send_to_target re-validates at the queue drain (#5911), so the new close re-check has no unfixed siblings — closing is the one verb whose act necessarily awaits after the gate. The session_send doc-table row fixes genuine staleness (the route and schema pre-exist unchanged). Review follows.

First-Principles-Verdict: CONCERNS

The point-of-no-return re-check — including a new flag that skips part of authorize_target — is nowhere in the description; a human should see it.

What this change ships

Intent: let an agent dismiss peer sessions it opened, instead of leaving dead tabs for the human. ADDITION.

1. Agent can close (archive) a peer session via session_close — justified
2. New /api/session-control/close strict-internal route — justified
3. Tab-✕ sequence extracted into shared close_slot; DELETE handler unchanged — justified, cause-level
4. Close re-authorizes synchronously before the pop (pre_pop_check, 409 target_replaced) — undeclared
5. authorize_target gains skip_enabled_check — undeclared; one consumer (_reassert_closeable)
6. Status-500 refusals now forward as 500 instead of degrading to 400 — justified
7. session_close channel-blocked and strict-identity-gated — justified (documented containment invariant)
8. Spec table gains the missing session_send row, counts corrected — rides along, fixes real staleness
9. Three files fully black-formatted and pruned from the baseline (~9 hunks unrelated to close) — rides along

Watch

• Items 4–5 are the change's most security-sensitive surface and the description never mentions them (its authorization paragraph claims only "every guard already covering stop/read applies unchanged"). Both are boundary-derived — the mirror/link window is real, stop/send cover theirs differently (stop_target has no await after its gate; send re-validates at drain, Queued prompts drain without re-validating the authorization that admitted them #5911) — and the spec documents them, but the omission means reviewers reading the PR text will not examine the one flag that weakens the auth gate.

Subtractions

• Defer item 9 — the pure-reformat hunks in validation.py, channel.py, mcp_dashboard.py plus their three .github/black-baseline.txt prunes — to its own commit, exactly as AGENTS.md prescribes for baseline files.

[FIRST-PRINCIPLES-REVIEWED] 1747253

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 17472535edb0132e978118d38c63139509cb39ba — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 1747253

Verdict parsed from the review's SHA-scoped output markers for commit 17472535edb0132e978118d38c63139509cb39ba.

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

[chenmingwei23]

Dispositions for head ad26f32a6 (was ac16e9381):

GPT 5.6 (BLOCKING) - close authorization becomes stale before archival --> FIXED.
Real and reachable by this module's own bar (create_session re-gates for exactly this class of race). The prescribed remedy (revert) would drop the feature; the correct fix is the one the module already uses: re-assert authorization adjacent to the mutation. close_slot now takes an optional synchronous pre_pop_check that runs immediately before the slot is popped (after the nudge-retirement and app-hook awaits); close_target passes a check that re-runs authorize_target, so a target that gains a channel mirror or link during those awaits is refused with that guard's own 403 and the close rolls back (retired nudge loop restored). The human tab-close path passes no check (the person owns the tab). New tests: test_close_reauthorizes_at_the_point_of_no_return (mirror appears mid-close -> mirrored_target 403, session not archived) and test_close_slot_pre_pop_abort_rolls_back_and_does_not_pop (abort leaves the slot in _slots, teardown never runs).

Design Review / Opus 4.8 / First Principles (spec not updated in same commit) --> FIXED.
docs/system-specs/modules/session-control.md updated: session_close row added to the operations table, tool/route count corrected, the "two verbs that take a target" prose now names every target-taking verb (stop, send, close, read), the channel-agent-containment and config notes are count-agnostic, and a new section documents that close archives (not deletes), is heavier than stop, and re-authorizes at the point of no return. (While there, the table was completed with the pre-existing session_send row it had also been missing.)

First Principles residual - one sibling copy of the close sequence in the bulk stale-archive pass --> ACCEPTED-AND-DEFERRED.
api_chat_slots_cleanup still hand-rolls tombstone/cancel/save rather than calling close_slot. Folding it in is genuinely larger: it has batch and dry-run semantics, a per-item running/collect branch, and an auto-nudge exemption rather than per-slot retirement, so a faithful reuse would have to reconcile those. Kept out of this PR's scope to avoid growing a diff that just converged on review; captured as a follow-up. This PR's extraction has exactly the 2 counted consumers it claims.

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision 17472535edb0132e978118d38c63139509cb39ba touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[chenmingwei23]

Dispositions for head 7214b1c0f (was ad26f32a6) -- both GPT 5.6 findings on the pre-pop re-check FIXED. Both are real and consistent with this module's own discipline (introduced by the round-1 re-authorization), so fixed rather than overridden.

BLOCKING 1 -- reauthorization can synchronously reload config on the event loop (no-blocking-call-on-event-loop) --> FIXED.
The pre-pop check ran authorize_target -> synchronous session_control_enabled() -> KiroCrewConfig.load(), and a config edit during close_slot's awaits would make that a blocking read on the loop. pre_pop_check is now async and close_slot awaits it; close_target's callback await prewarm_enabled_check() off-loop FIRST (the last suspension), then runs the synchronous authorize_target, and close_slot pops with no further await -- so the live mirror/link read stays adjacent to the pop. Same prewarm-then-adjacent-gate discipline stop_target/send_to_target already use.

BLOCKING 2 -- reauthorization does not verify the original slot identity (crash/data-loss) --> FIXED.
A concurrent close+reopen can re-mint the same key onto a different session while close_slot awaits; authorize_target resolves by key, so it would authorize the replacement while close_slot pops name and tears down/saves the original. The callback now compares the re-resolved slot to the captured one by identity (live is not slot) and aborts with target_replaced (409) -- the same identity-not-presence guard create_session uses for its re-minted-key window.

Tests added: test_close_reauthorizes_at_the_point_of_no_return (mirror appears mid-close -> mirrored_target 403, not archived), test_close_aborts_if_the_key_was_reminted_during_the_close (re-mint -> target_replaced 409, replacement survives), test_close_slot_pre_pop_abort_rolls_back_and_does_not_pop (abort leaves slot in _slots, teardown never runs). Spec updated to describe the off-loop prewarm and the identity comparison. Local: 293 tests green, lint/mypy/docs-lint clean.

[chenmingwei23]

Disposition for head 8dec00fe4 (was f8919683b) -- GPT 5.6 finding FIXED.

BLOCKING -- await reopens the retired-loop race before removal (crash/data-loss) --> FIXED.
Real: making pre_pop_check awaited (the round-2 fix for the on-loop config read) put an await back between the last nudge retirement and the slot pop, which is exactly the window close_slot closes so a concurrent monitor_start cannot arm a loop whose timer then rehydrates the archived tab. The prescribed remedy (revert the hunk) drops the feature; the correct fix is the one close_slot already uses for its own awaited step -- the app-hook path retires the loop a second time AFTER its await, adjacent to the pop. Generalized that: after a successful pre_pop_check, close_slot retires the nudge loop once more and pops with no further await. Both the re-authorization (round 1/2) and the retirement-adjacent-to-removal invariant now hold. New test test_close_slot_retires_the_nudge_loop_again_after_the_pre_pop_check pins the order (retire -> check -> retire, then pop). Spec updated. Local: 140 session-control/nudge-race tests green, lint/mypy clean.

[buluoray]

Reviewed at head 17472535e. 0 blocking, 5 yellow, 6 blue.

The design is right and the extraction is honest. Almost every finding below is prose that no longer matches the code, plus one real coverage gap on the PR's own central claim. I verified behaviour against source rather than against the description; what I checked and what I could not check is listed at the end.

---

Y1 — the line this whole PR is about has no test pinning its position

The pre_pop_check() call site inside close_slot must sit after the nudge-retirement and app-close-hook awaits and immediately before the pop. That positioning is the entire outcome of four blocking review rounds — round 3's awaited form was blocked because an await reopened the retired-loop window, and round 4's because a late retirement's await let a mirror land after re-authorization.

I moved pre_pop_check() to before the app-close-hook await — reopening exactly that window — and all 170 relevant tests stayed green.

The cause is visible in the test itself. test_close_reauthorizes_at_the_point_of_no_return monkeypatches close_slot away entirely and calls pre_pop_check() from its own fake:

async def _fake_close(_state, _slot, _name, *, pre_pop_check=None):
    state.sessions.set_mirror_link(slot_history_key(target), "C123", "T1")
    assert pre_pop_check is not None, "close_target must arm a pre-pop re-check"
    pre_pop_check()  # re-runs authorize_target -> raises for the new mirror

So it proves the check is armed and can abort — never that the real close_slot invokes it at the point where staleness could have been introduced. That falsifies the PR body's claim of "the prewarm-before-gate / gate-adjacent-to-act adjacency contract (mutation-guarded, mirroring the stop tests)": the arming is guarded, the adjacency is not.

The shipped position is correct. What is missing is the guard against a future refactor moving it. A test driving the real close_slot with an app close hook that acquires a channel mirror during its await would pin it — that mutation is the one that currently passes silently.

Y2 — the commit message describes the design that was rejected

close_target re-authorizes at the point of no return (async off-loop config prewarm, then authorize_target, with a slot-identity comparison)

The shipped _reassert_closeable is a plain def with skip_enabled_check=True and no prewarm. The async-prewarm form is the round-3 design that was blocked, and a comment in this same commit says why: "an async prewarm-then-check would put an await back before the pop and reopen the very window this closes."

After the squash this text is permanent on main, advertising the unsafe shape as the implementation.

Y3 — the .py and the .md contradict each other inside one commit

src/kiro_crew/dashboard/session_control.py module docstring says authorization is checked "for the three operations that take a target — stop, close and read". Four call it: send_to_target passes operation="send" (session_control.py:1396). The docs/system-specs/modules/session-control.md shipped in this same commit correctly says "(stop, send, close, read)", so the .py is the wrong one.

It also undercuts its own adjacent safety claim — "a guard cannot be present on one verb and missing on another" — by hiding that send is guarded.

Y4 — the conductor invariant comment claims exhaustiveness it no longer has

The comment above _CONDUCTOR_DASHBOARD_GRANTS in src/kiro_crew/agent.py opens "Every verb this server exposes, against that rule:" and enumerates eight. session_close is now a ninth and is absent. It belongs as WITHHELD next to session_stop, whose entry reasons from exactly the property close has more of ("ends another session's in-flight turn and DISCARDS its work").

No security regression — the grant tuple was correctly left unchanged, so session_close is mounted-but-approval-gated. The gap is documentation only, but it is the enumeration a future reader trusts to be complete.

Y5 — enforced scope is wider than every description (seconding Design Review)

The commit message says "a peer session it opened"; the tool description says "a peer session you created and are done with". authorize_target enforces no creator containment. Any persistent, non-app, non-channel, same-workspace peer is reachable — including the tab the human is actively typing in.

I do not read this as blocking, and I would not add creator containment: session_stop already reaches the identical set and discards the identical turn, and the close archives recoverably. The right resolution is for the spec to state the uniform scope outright instead of letting the prose imply ownership scoping.

---

Blue

• src/kiro_crew/dashboard/server.py:413 — comment says "the three routes behind the session_create / session_stop / session_read_message MCP tools" and now sits directly above five. Already stale in base (missing send); this PR inserted the close route under it untouched.
• Same module docstring headline says "Four operations" — there are five (session_send omitted). The next sentence names send explicitly, so it reads as a miscount rather than a claim.
• The config prewarm's position is unguarded: moving await prewarm_enabled_check() below the gate passes clean. The adjacency test records sel and authorize but never the config prewarm, so assert order[:1] == ["sel"] pins only the SEL prewarm.
• skip_enabled_check=True is unguarded — flipping it to False is invisible to the suite.
• ~80 lines (≈9–10% of the diff) are optional black reformat of three baselined files. The baseline prune was required only as a consequence of choosing to reformat; the gate's own docstring says nothing demands a baselined file be reformatted. AGENTS.md wants that in its own commit, which the one-commit rule makes impossible — so the clean option was simply not to reformat. Both advisory bots raised this.
• docs/system-specs/modules/README.md:24 index text still says "opening, stopping, and reading another".

---

What I verified against source

• Extraction fidelity. close_slot is behaviour-preserving for the existing DELETE endpoint: identical ordering, await points, SEL logging, and _sync_dashboard_slots / push_slots_update / push_refresh calls; all four status=500 returns map to byte-identical {error, code} bodies; App Kit ownership checks correctly stayed in the handler; pre_pop_check is inert there (defaults None).
• Rollback symmetry. The pre-pop abort restores exactly what its peer paths restore that applies at its position — {nudge loop, app notification}, minus the _slots re-insert since the pop has not happened. The _RECENT_CLOSES tombstone is left set on every failure path, identical to origin/main, TTL self-healing — not a regression.
• skip_enabled_check is a safe minimal widening. Keyword-only (so no positional-truthy leak), one call site, bypasses 1 of 18 refusal branches — the global session_control_enabled() read — with all 17 containment/identity guards still running. The rationale is real rather than rationalization: KiroCrewConfig.load()'s fingerprint stat()s config.json on every call, so even a cache hit is on-loop disk I/O.
• The "no blocking I/O" docstring claim holds even on refusal. deny() dispatches its SEL write through _sel_off_loop → run_in_executor, and authorize_target is a synchronous def that structurally cannot await.
• Blast radius is complete on every enumeration that gates authority; no SCOPE_CATALOG, policy, agent-config, or frontend list names these tools at all.
• Local gates all pass: diff-scoped black, subprocess-encoding, isort, flake8 (11 files), mypy --platform linux (1210 files clean), harness parity, docs-lint (247 md).
• Tests: 304 pass across the close-path invariant suites; the PR body's green claim reproduces. Mutations a/b/d/g are pinned. One flake seen once — test_session_control_boundaries.py::TestRefusalStatusMapping::test_a_mapped_status_is_forwarded[500] — passes alone (24/24) and on back-to-back repeats; it smells like the pre-existing shared-event-loop isolation issue in conftest.py, not this diff.
• The .md table fixed a real pre-existing gap: base omitted session_send entirely despite it shipping as both a tool and a route.

What I could not verify

Runtime behaviour under real concurrency — I confirmed the code structure and rollback ordering, not the live timing of the mirror/nudge races. Whether the conductor should wield session_close is a charter decision, not a code fact.

---

Summary. Y1 is the one I would ask for before merge: it is the only guard protecting a correctness property that took four blocking rounds to establish, and it currently does not exist. Y2 matters because the text becomes permanent on main. Y3, Y4 and the first server.py blue are one-line edits in files already open in this change. Y5 is a spec-wording ask, not a change request. None of it is a reason to block.

[buluoray]

Approving at head 17472535e — 67 checks SUCCESS, 4 skipped, none failing, and both blocking bots report no findings.

My detailed review is above: 0 blocking, 5 yellow, 6 blue. Every lane that examined behaviour came back clean — the close_slot extraction is byte-faithful for the existing DELETE path, the rollback set at the pre-pop abort is symmetric with its peers, skip_enabled_check bypasses exactly one of eighteen refusal branches with all seventeen containment/identity guards intact, and the conductor grant tuple was correctly left alone so session_close stays mounted-but-approval-gated.

I am approving rather than holding on Y1 because the shipped pre_pop_check() position is correct — what is missing is only the regression guard that would keep it correct. Two follow-ups I would still like to see land, neither of which needs to gate this merge:

1. The adjacency test driving the real close_slot with an app close hook that acquires a channel mirror during its await. Moving pre_pop_check() above that await today leaves 170 tests green, so the property four blocking rounds established is currently unguarded, and the PR body's "mutation-guarded" claim does not hold for the adjacency (only for the arming).
2. The commit message, which still describes the async off-loop prewarm form — the round-3 design that was blocked, and which a comment in this same commit explains is unsafe. That text becomes permanent on main after the squash, so it is worth fixing before merge even though it changes no behaviour.

The Y3/Y4 prose fixes (the .py "three operations" count, the conductor WITHHELD entry for session_close) are one-line edits in files already open in this change. Y5 is a spec-wording ask: state the uniform scope outright rather than implying creator containment that authorize_target does not enforce.

[chenmingwei23]

First Principles CONCERNS (advisory, non-blocking) on 17472535e -- both items ACCEPTED-AND-DEFERRED, tracked in #7219:

• Watch (description doesn't surface the point-of-no-return re-check + skip_enabled_check): behavior is correct and documented in the module spec + code; this is a description/reviewability clarification, deferred.
• Subtraction (incidental black reformat of validation.py/channel.py/mcp_dashboard.py + baseline prunes should be their own commit per AGENTS.md): commit-hygiene follow-up, deferred.

Neither blocks this PR. CI is green (GPT/Opus green post-rebase; override not needed).

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #6237 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6237: REBASE. A fifth session verb shipped after the branch was cut and must become a fifth op; as written the collapse silently removes it. Files: src/kiro_crew/mcp_dashboard.py.
• PR #5248 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5248: REBASE. Only the formatting half of the PR's baseline change has landed; the handlers_channel.py baseline line and its reflow are still the PR's own work. Files: .github/black-baseline.txt, src/kiro_crew/channel.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.