Skip to content

Stop attributing pane-less agent events to the focused pane - #657

Open
Yuandi (DDKinger) wants to merge 3 commits into
mainfrom
dev/yuazha/fix-send-event-pane-fallback
Open

Stop attributing pane-less agent events to the focused pane#657
Yuandi (DDKinger) wants to merge 3 commits into
mainfrom
dev/yuazha/fix-send-event-pane-fallback

Conversation

@DDKinger

@DDKinger Yuandi (DDKinger) commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Problem

wtcli send-event filled a missing --pane by calling GetActivePane(). The active
pane is not "the pane this agent runs in" — it is wherever the user is looking right
now, so any caller that cannot identify its own pane had its events attributed to an
unrelated pane.

For agent.session.start that is destructive rather than merely inaccurate: the
orphan-handover branch in agent_sessions.rs demotes the previous owner of a reused
pane to Ended and clears its binding. So a pane-less hook firing while the user
happens to be looking at a tracked pane silently killed that pane's session, rebound
the pane to an agent that was never in it, and left Enter on that row focusing a
stranger's pane.

The callers that omit --pane are the legacy PowerShell hook bundles, which pass -p
only when they can read WT_SESSION. Those bundles live in each CLI's plugin cache,
are never rewritten by a Terminal upgrade, and their auto-refresh can fail — so this is
live behaviour for un-refreshed users, not a historical path.

Fix

Publish an empty pane_id ("source pane unknown") instead of guessing. WTA already
models this: pane_known is false, so it skips the handover, leaves active_by_pane
untouched, and routes by cli_source. Losing attribution beats inventing a wrong one.

Also: hook-debug/state-logger.ps1 (the one in-tree caller that never passed -p) now
passes $env:WT_SESSION, and the two docs that advertised the fallback are corrected.

Verification

Driven end-to-end through the frozen legacy script
(test/e2e/fixtures/legacy-hook-bundle/send-event.ps1) against a live Terminal, so the
real chain runs: conditional $paneArg, JSON wrapping, wtcli lookup order, async
ShellExecuteEx dispatch. Same payload; only the resolved wtcli and WT_SESSION vary.

wtcli WT_SESSION published pane_id
shipped set E1F685F3-… control
shipped absent E1F685F3-… the bug
rebuilt set E1F685F3-… unchanged
rebuilt absent "" fixed

All four exit 0, so the hook contract holds either way.

Sampling a real hook-trace.log from that bundle (150 invocations, 3 CLIs): 121 had
WT_SESSION and are byte-identical under this change; 29 did not, and 22 of those 29
were agent.session.start — i.e. the destructive shape fired 22 times on one trace.
All 151 dispatched events carried a real agent_session_id, so rows stay keyed by the
agent's own session id and keep their title, cwd, CLI badge and
Working/Idle/Attention. Only the pane binding changes, and it was wrong before.

bx on wtcli is clean under /W4 /WX; full WTA suite 1646 passed / 0 failed.

Known behaviour change

A pane-less row is Live with no pane, which decide_enter_action maps to
NotResumable (already pinned by live_with_empty_pane_not_resumable), and since
SessionOrigin defaults to Unknown such a row does render in the picker. So Enter on
it now reports "not resumable" instead of focusing a pane. That is the intended trade:
the previous behaviour focused an unrelated pane, and only got there by first evicting
that pane's own session.

Not affected: WSL panes (WT_SESSION is forwarded via WSLENV), the agent-hook
bridge and its E2E, the product event listener (cli_channel.rs subscribes without
-t), Set-AutofixState (always passes -SourcePane), and autofix (keyed off
OSC 133, never off hook pane_id).

Tests

  • E2E Feature.LegacyHookBundle (checklist C278, mapped in release-coverage-map.psd1):
    a legacy hook with WT_SESSION cleared but WT_COM_CLSID intact must still publish,
    and must not carry the focused pane's id. The arrival assertion is deliberate so the
    case cannot pass by the event never being sent.
  • Unit app_tests.rs: a pane-less agent.session.start must not end or unbind a session
    that owns a real pane, and the empty string must never become an active_by_pane key
    (so PaneClosed cannot resolve to it and two pane-less sessions cannot evict each other).

`wtcli send-event` filled a missing `--pane` with `GetActivePane()`. That
is not "the pane this agent runs in" but "wherever the user is looking
right now", so every caller that cannot identify its own pane had its
events bound to a stranger's pane.

For `agent.session.start` that is destructive rather than merely wrong:
WTA's orphan-handover branch demotes the previous owner of a reused pane
to Ended, so the focused pane's real session was silently killed and
rebound to an agent that was never in it -- after which Enter on that row
in the session list focuses someone else's pane.

#571 removed the in-product exposure by moving the shipped hook bundles to
`wtcli agent-hook`, which requires WT_SESSION and drops the event when it
is absent. But `send-event` was deliberately kept as the transport for
bundles installed *before* #571, which live in each CLI's plugin cache and
are never rewritten by an upgrade. Those bundles still pass `-p` only when
they can read WT_SESSION, so for un-refreshed users the bug survived #571
intact -- and C270 froze that path in place without covering the pane-less
half of it.

Publish an empty `pane_id` instead. That is the case WTA already models:
`pane_known` is false, so it skips the handover, leaves `active_by_pane`
untouched, and routes the event by `cli_source`. Losing attribution is
strictly better than inventing a wrong one.

Verified A/B against a live Terminal with the same payload and no `-p`:
the shipped pre-fix binary published `pane_id` = the active pane GUID, the
rebuilt one publishes `pane_id` = "". Passing `-p` is unchanged, so C270
stays green.

Also fixes the one in-tree caller that never passed `-p` at all
(`hook-debug/state-logger.ps1`) and the docs that advertised the fallback.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cddec735-3efa-4641-8a4d-65a1b5628bb7
Copilot AI lite review requested due to automatic review settings August 23, 2026 11:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a legacy compatibility hazard in wtcli send-event: when callers omit --pane, events are no longer attributed to the currently focused pane (which could corrupt WTA’s pane/session binding), and are instead published with an empty pane_id to explicitly represent “source pane unknown”.

Changes:

  • Update wtcli send-event to publish an empty pane_id when --pane is omitted (remove focused-pane fallback).
  • Update the in-tree debug hook script to pass -p $env:WT_SESSION when available.
  • Add E2E coverage for the “legacy hook without WT_SESSION” case and update docs + release checklist mappings to reflect the new contract.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/wta/wt-agent-hooks/hook-debug/state-logger.ps1 Passes -p when WT_SESSION is present; otherwise publishes unattributed.
test/e2e/tests/Feature.LegacyHookBundle.Tests.ps1 Adds E2E case asserting pane-less legacy hooks still publish and do not hijack focused pane attribution.
test/e2e/release-coverage-map.psd1 Maps new E2E case to release coverage checklist text.
src/tools/wtcli/wtcli_functions.h Documents that empty pane_id is valid and represents unknown source pane.
src/tools/wtcli/main.cpp Removes focused-pane fallback for send-event; emits empty pane_id when --pane omitted.
doc/wtcli-commands.md Updates send-event documentation to describe empty pane_id behavior and rationale.
doc/specs/llm-agent-event-integration.md Updates examples/notes to require explicit -p for attribution and describe empty pane_id semantics.
doc/release-check-list.md Adds checklist item covering the pane-less legacy hook behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/tools/wtcli/main.cpp
The wtcli change is only safe because an empty `pane_id` is inert in the
session reducer. That was true by construction (`pane_known`) but untested
from the routing layer, so nothing would catch a future change that made
an empty pane a real lookup key again.

Two cases: a pane-less `agent.session.start` must not end or unbind a
session that owns a real pane (the exact damage `GetActivePane()` used to
cause), and the empty string must never enter `active_by_pane`, so a
`PaneClosed` cannot resolve to it and two pane-less sessions cannot evict
each other.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cddec735-3efa-4641-8a4d-65a1b5628bb7
Copilot AI review requested due to automatic review settings August 23, 2026 11:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

doc/wtcli-commands.md:40

  • The send-event examples use -p 3, but --pane is described (and parsed) as a GUID string (see GuidFromString / ResolveSessionId). Passing 3 would publish a non-GUID pane_id, which could be misinterpreted downstream. Update the example to use a real GUID (e.g. $WT_SESSION) or an explicit GUID placeholder.
| `send-event` | `se` | Publish an event using the `agent_event` envelope: sets `type=event`, `method=agent_event`, fills `params.event` from `-e` and `params.pane_id` from `-p`. Omitting `-p` publishes an empty `pane_id` meaning "source pane unknown" — it is **not** attributed to the focused pane, because guessing a pane corrupts session-to-pane binding, while an unattributed event is routed by `cli_source` instead. Extra params come from the trailing JSON object. | `wtcli send-event -p 3 -e agent.task.completed '{"exit_code":0}'` | ❌ Not called from in-tree code. Kept as the transport for pre-#571 hook bundles (guarded by `Feature.LegacyHookBundle.Tests.ps1`) and as the public CLI surface for external agents in `doc/specs/llm-agent-event-integration.md`. |

doc/specs/llm-agent-event-integration.md:226

  • This usage example still shows wtcli send-event -p 3 ..., but --pane is a pane GUID string (typically $WT_SESSION). Using 3 is not a valid GUID and would publish an invalid pane_id. Update the example to pass $WT_SESSION (or a GUID literal) to match the actual contract.
# Simple lifecycle event, attributed to this pane
wtcli send-event -p "$WT_SESSION" -e agent.started '{"agent":"copilot-cli","version":"1.2.0"}'

# Task completion with exit code
wtcli send-event -p 3 -e agent.task.completed '{"task_id":"abc","exit_code":0,"summary":"Built successfully"}'

src/tools/wtcli/main.cpp:910

  • The error message says "Invalid JSON for --json", but send-event takes a positional json argument (there is no --json option here). Consider rewording to something like "Invalid JSON for 'json' argument" or "Invalid JSON event params" to avoid confusing users.
        if (!wtcli::BuildSendEventJson(sendEventType, sendEventJson, sendEventPaneTarget, evt))
        {
            fprintf(stderr, "Invalid JSON for --json: value must be a JSON object (e.g. '{\"key\":\"val\"}')\n");
            exitCode = 1;

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cddec735-3efa-4641-8a4d-65a1b5628bb7
Copilot AI review requested due to automatic review settings August 23, 2026 12:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

doc/specs/llm-agent-event-integration.md:226

  • The example uses wtcli send-event -p 3 ..., but send-event now treats -p/--pane as a literal pane GUID string (no ResolveSessionId fallback). Using 3 would publish pane_id="3" and likely misroute/break session-to-pane binding. Suggest updating the example to use $WT_SESSION (or an explicit GUID placeholder) to match the documented contract.
wtcli send-event -p "$WT_SESSION" -e agent.started '{"agent":"copilot-cli","version":"1.2.0"}'

# Task completion with exit code
wtcli send-event -p 3 -e agent.task.completed '{"task_id":"abc","exit_code":0,"summary":"Built successfully"}'

doc/wtcli-commands.md:40

  • The send-event table row still shows an example -p 3, but -p/--pane is documented as a GUID and the implementation now forwards the string as-is (no numeric/target resolution). Consider changing the example to -p "$WT_SESSION" (or a GUID placeholder) to avoid readers publishing a non-GUID pane_id.
| `send-event` | `se` | Publish an event using the `agent_event` envelope: sets `type=event`, `method=agent_event`, fills `params.event` from `-e` and `params.pane_id` from `-p`. Omitting `-p` publishes an empty `pane_id` meaning "source pane unknown" — it is **not** attributed to the focused pane, because guessing a pane corrupts session-to-pane binding, while an unattributed event is routed by `cli_source` instead. Extra params come from the trailing JSON object. | `wtcli send-event -p 3 -e agent.task.completed '{"exit_code":0}'` | ❌ Not called from in-tree code. Kept as the transport for legacy PowerShell hook bundles (guarded by `Feature.LegacyHookBundle.Tests.ps1`) and as the public CLI surface for external agents in `doc/specs/llm-agent-event-integration.md`. |

src/tools/wtcli/main.cpp:910

  • Error message says "Invalid JSON for --json" but send-event takes a positional json argument (and there is no --json flag for this input). This could mislead users when diagnosing failures; consider updating the message to reference the positional argument or just "Invalid JSON: value must be a JSON object ...".
            fprintf(stderr, "Invalid JSON for --json: value must be a JSON object (e.g. '{\"key\":\"val\"}')\n");
            exitCode = 1;

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.

2 participants