Fix Autofix event delivery and cross-tab Detected actions - #880
Conversation
Preserve stable pane identity across asynchronous VT dispatch and validate detected-action targets before replaying a failure. Add dual-tab UI-to-ACP regression coverage and update Autofix test oracles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
There was a problem hiding this comment.
🟡 Changes recommended
The new AutofixRouting ItE2E test can fail deterministically when it reads the fixture log before the file exists (Select-String on a non-existent path).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves Autofix correctness by ensuring VT-based failure events and “Detected” opt-in actions are routed to the intended pane/tab, preventing silent drops and cross-tab submissions. It also adds targeted regression coverage (unit + ItE2E) and updates the release checklist/README to reflect the new scenarios.
Changes:
- Capture and use stable pane SessionId for VT callback routing, and tag relevant protocol events with tab identity for proper helper-side isolation.
- Harden helper-side
autofix_execute_from_detectedhandling to reject untargeted/cross-tab/stale actions while preserving legacy pane-only compatibility when safe. - Add deterministic two-tab routing coverage and strengthen parser/WSL E2E assertions with scoped evidence.
File summaries
| File | Description |
|---|---|
| tools/wta/src/autofix_tests.rs | Adds reducer/unit tests for Detected action target isolation and legacy/stale handling. |
| tools/wta/src/app/autofix.rs | Scopes Detected execution to matching pane/tab and ignores cross-tab/untargeted actions. |
| tools/wta/src/app_events.rs | Wires WT event payload (pane_id/tab_id) into the updated Detected execution handler. |
| test/e2e/tests/Feature.AutofixRouting.Tests.ps1 | New ItE2E test covering two Detected tabs and ensuring diagnostics clicks submit only to the selected tab. |
| test/e2e/tests/Feature.AutofixParser.Tests.ps1 | Replaces prompt-content assertions with scoped helper/master evidence and pins observations to the correct helper/session. |
| test/e2e/tests/Feature.AutofixPane.Tests.ps1 | Pins WSL helper identity and scopes proposal/card interactions to the correct agent pane session. |
| test/e2e/README.md | Documents the new AutofixRouting suite and updates AutofixPane suite counts/coverage notes. |
| test/e2e/ItE2E/Public/Agent.ps1 | Extends Wait-TerminalActionProposal with -PaneSessionId to avoid cross-helper card ambiguity. |
| src/cascadia/TerminalApp/TerminalPage.h | Declares _FindTabIdForSessionId for session-id-based tab resolution. |
| src/cascadia/TerminalApp/TerminalPage.cpp | Captures stable SessionId for VT callbacks, resolves owning tab by SessionId on UI thread, and includes tab_id in Detected execute events. |
| doc/release-check-list.md | Adds release checklist item C295 for cross-tab Detected isolation coverage. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
Create the routing fixture log before launching helpers and reword the target-rejection diagnostic without adding dictionary exceptions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
There was a problem hiding this comment.
🔵 Needs a closer look
TerminalPage::_RegisterTerminalEvents now declares an unused weakTerm variable in the modified VT callback block, which can break builds when warnings are treated as errors.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/cascadia/TerminalApp/TerminalPage.cpp:8089
_FindTabIdForSessionIdcurrently walks every pane and calls_FindSessionIdForControl(control)for each node, which formats a GUID into a string each time. SinceTermControl::Connection().SessionId()is already awinrt::guid, you can parsesessionIdonce and compare GUIDs to avoid repeated string allocations/conversions on a hot VT-sequence path.
This issue also appears on line 8133 of the same file.
src/cascadia/TerminalApp/TerminalPage.cpp:8135
weakTermis now unused after switching the VT callback to capturepaneIdStrinstead of aTermControlweak_ref. Leaving this in place will trigger an unused-variable warning (and can fail builds if warnings are treated as errors).
{
winrt::weak_ref<TermControl> weakTerm{ term };
const auto paneIdStr = _FindSessionIdForControl(term);
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77616472-cc8d-4566-9050-60ab09248076
There was a problem hiding this comment.
🟡 Changes recommended
TerminalPage::_RegisterTerminalEvents leaves an unused local (weakTerm) that can break builds under warnings-as-errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
Summary
Isolates two Autofix correctness fixes from the separate GetPaneContext work. This PR does not include the new context API or its performance changes.
Regression coverage
Validation