Skip to content

feat(orchestration): stop confirming closes of an agent's own children - #626

Merged
Juliusolsson05 merged 2 commits into
mainfrom
feat/orchestration-close-unconfirmed
Jul 28, 2026
Merged

feat(orchestration): stop confirming closes of an agent's own children#626
Juliusolsson05 merged 2 commits into
mainfrom
feat/orchestration-close-unconfirmed

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Orchestration MCP closes (close_agent, close_run) raised a confirmation dialog on every call. Those agents are disposable — the parent created them for a task, and closing them when it's done is the normal end of their lifecycle. A dialog that fires on routine housekeeping is how "are you sure?" stops meaning anything by the time it guards something that matters.

Result: routine fleet cleanup is now silent. The dialog survives only for the closes that genuinely reach further than the agent being named.

Why it isn't simply preConfirmed: true

That was the first attempt, and both reviewers blocked it. The safety argument was wrong.

The ownership gate (isVisibleToOrchestrationParent) scopes which session may be named, not which sessions die. closeSession kills a set, and the gate never sees that set. Two shapes reach past the target:

  • Linked agents. A user can run "Linked Agent…" on an orchestration child — orchestration children are ordinary Dispatch rows, so they're selectable. That linked agent carries no orchestration fields, so the gate cannot see it. closeLinkedChildren would have killed a hand-built session silently, and it wouldn't even appear in closedSessionIds.
  • Sole grid leaf. Closing a tab's only pane takes every detached session in that tab. Orchestration children live detached in the root's tab — so a close could take the caller itself, its siblings, and any Dispatch agent the user parked there, while reporting one id.

The decisive evidence: Agent Management already refuses both shapes outright (additionalCloseImpactclose_would_affect_additional_sessions) even though it has a dialog available. Orchestration's only equivalent protection was the dialog.

The fix: scope the silence, don't remove it

A new silentIfSoleTarget option resolves inside closeSession, where paneCloseTargets is in scope — the only code that computes the full set a close destroys, which a caller cannot know from outside.

  • Close expands to exactly the named session → silent. This is the routine case and the whole point of the PR.
  • Close expands further → dialog, naming the requesting agent.

CloseSessionOptions' doc calls preConfirmed an ASSERTION and "deliberately NOT a convenience for 'this close feels safe'" — it now records why orchestration doesn't qualify and what it uses instead.

Two reporting bugs fixed on the way

  • close_run told the parent every agent closed. It pushed to closedSessionIds unconditionally and relied on a decline throwing to reach its catch — but the gate resolves false, it never rejected. close_agent was worse: no branch at all. Both now branch on closeSession's boolean.
  • Agent Management reported a declined close as success. Its own comment claimed "declining rejects the tool call"; it didn't. That's the one surface where a decline is still possible, so it's where the lie mattered most.

close_agent also gained the catch close_run already had — a rejected backend kill runs after closeLinkedChildren, so a bare throw could report neither closed nor skipped with children already dead.

Tests

orchestrationClose.test.ts — 14 cases. The contract is "does it tell the truth", since a parent that believes a child closed stops waiting on it:

  • every close carries silentIfSoleTarget, never preConfirmed — a regression to the latter would destroy user sessions silently, and dropping it entirely would put dialogs back on routine cleanup
  • closed / didn't-close / threw each report correctly; skippedSessionIds omitted rather than empty
  • the runId filter, which is now close_run's sole blast-radius control and previously had zero coverage
  • non-agent kinds excluded, empty-run case, ownership gate refusals

Verification

  • npx tsc -b → exit 0
  • NODE_ENV=test npx vitest run256 files, 1775 tests, all passing
  • npm run check:keybindings → OK
  • npm run test:contract → satisfied

Earlier runs showed 1–4 failures around store.test.ts; all were 5s-timeout flakes under concurrent load from the review agents. Both reviewers independently confirmed they're pre-existing and outside this PR's import graph, and a clean run with no contention is fully green.

Not verified: the live MCP flow — the app was not launched.

🤖 Generated with Claude Code

Juliusolsson05 and others added 2 commits July 28, 2026 18:15
An orchestrating parent closing the children it spawned was raising a
confirmation dialog — once per close_agent, and once per run on close_run.
Those agents are disposable by construction: the parent created them for a
task, and closing them when the task is done is the normal end of their
lifecycle, not a destructive act on something the user built. A dialog that
fires on routine housekeeping is how "are you sure?" stops meaning anything
by the time it guards something that matters.

The safety property was never the dialog. It is the ownership gate:
isVisibleToOrchestrationParent restricts every orchestration read and close
to children the caller itself created, or descendants of its own root run.
That gate is untouched, and the WHY on OrchestrationCloseSession now says
that if it is ever loosened this decision has to be revisited with it.

Agent Management MCP is deliberately NOT changed. Its blast radius is every
agent in the caller's project rather than only its own children, so a human
still signs off there — which is also what the README's "destructive close
is restricted to an explicit current user request" describes.

This also fixes the reporting bug PR #625's review surfaced. The old run
loop pushed every id into closedSessionIds and relied on a decline THROWING
to reach its catch — but the confirmation gate resolves false, it never
rejected, so skippedSessionIds could not populate from a decline and the
parent was told agents closed that had not. closeOrchestrationAgent was
worse: it returned the id unconditionally with no branch at all. Both now
branch on closeSession's boolean. With no dialog a decline is impossible,
but an already-gone session still returns false and is correctly reported
as skipped.

The two wrapper adapters added in #625 to drop that boolean for a
Promise<void> contract are gone with the contract; OrchestrationCloseSession
returns Promise<boolean> and no longer carries requireConfirmation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…agent

Both reviewers returned BLOCK on the same finding, and they were right.

The safety argument was wrong. The ownership gate scopes WHICH SESSION MAY
BE NAMED, not WHICH SESSIONS DIE — closeSession kills a set the gate never
sees. Two shapes reach further than advertised:

  - closeLinkedChildren takes every session linked to the target. A user can
    run "Linked Agent…" on an orchestration child, and that linked agent
    carries no orchestration fields at all, so the gate cannot see it.
    Asserting preConfirmed destroyed a hand-built session silently, and it
    did not even appear in closedSessionIds.
  - Closing a tab's sole grid leaf takes every detached session in that tab.
    Orchestration children live detached in the root's tab, so a close could
    take the caller itself, its siblings, and Dispatch agents the user parked
    there — while reporting one id.

The decisive evidence: Agent Management already refuses both shapes outright
via additionalCloseImpact, *even though it has a dialog available*.
Orchestration's only equivalent protection was the dialog this PR removed.

So the fix is not to restore the dialog but to scope it. A new
silentIfSoleTarget option resolves inside closeSession, where paneCloseTargets
is in scope — the only code that computes the full set a close destroys. The
routine case (a detached child expanding to exactly itself) stays silent,
which is the entire point of the PR; the two reaching shapes fall back to a
dialog naming the requester. CloseSessionOptions' doc, which calls
preConfirmed an ASSERTION and "deliberately NOT a convenience for this close
feels safe", now says why orchestration does not qualify.

Also fixed: Agent Management reported a DECLINED close as a success. Its own
comment said "declining rejects the tool call" — it did not; the gate resolves
false. That is the one surface where a decline is still possible, so it is
where the lie mattered most. close_agent gained the catch close_run already
had, since a rejected backend kill runs after closeLinkedChildren and could
report neither closed nor skipped with children already gone.

Test gaps the reviewers found: the runId filter had zero coverage and is now
close_run's sole blast-radius control; non-agent kinds and the empty-run case
were unpinned; toBeUndefined cannot prove omission; and stateWith's cast
through unknown violated docs/testing/standard.md without the harness comment
that standard requires. All closed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Juliusolsson05
Juliusolsson05 merged commit 2ca6fe3 into main Jul 28, 2026
1 check passed
@Juliusolsson05
Juliusolsson05 deleted the feat/orchestration-close-unconfirmed branch July 28, 2026 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant