Skip to content

Commit 9b31039

Browse files
DDKingerCopilot
andauthored
Fix the OpenCode hook row and name the CLIs that fail to install (#636)
* Hide the OpenCode hook row once its hooks are removed The Agent session tracking list gave OpenCode a different visibility rule than every other CLI: its row was driven by ShouldShowDetectedOrConfiguredHookRow (binaryOnPath || hasHookState), while Copilot, Claude, Gemini and Codex used HasHookState alone. With the opencode binary on PATH but its hooks uninstalled, the row therefore stayed behind after a successful removal, showing a Remove button that CanRemoveOpenCodeHooks kept permanently disabled — a dead row the user can neither act on nor dismiss, even though `wta hooks status` correctly reported the hooks as gone. Drive all five rows from HasHookState so a row disappears when its hooks are removed. Discoverability is unaffected: installation runs through the single "Install hooks" button, which already covers every detected CLI, so a detected-but-uninstalled row carried no action to begin with. Drops the now-unused ShouldShowDetectedOrConfiguredHookRow helper, the _openCodeHooksPresent field, and the CanRemoveOpenCodeHooks projection; the OpenCode Remove button binds to CanRemoveAgentHooks like the rest. Validated with a Debug build of Microsoft.Terminal.Settings.Editor (x64) and AgentHooksStatusTests, 25/25 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Name the CLIs that failed to install hooks Clicking "Install hooks" and having, say, Codex fail produced a single "Hooks installation failed. Check <log> for details." line. Every other CLI could have installed fine and the user had no way to tell which one broke, or that the rest succeeded, short of opening wta-install-hooks.log. wta already computes the answer. `run_install` folds two independent failure signals — the spawn errors the install commands reported, and the post-install status check that catches a command which claimed success but registered nothing — into a message naming each failing CLI with its reason. That message only ever reached stderr and the trace log, because the Settings UI shells out through RunWtaAndWait and sees a bare bool. Give `wta hooks install` the `--json` treatment `status` and `uninstall` already have, and consume it: * InstallReport / CliInstallResult (schema v1) report per-CLI installed / skipped / failed plus the failure reason. Failure wins over the status check, so a spawn error with a PREVIOUS plugin still on disk stays a failure instead of reading as installed. * A CLI that simply isn't on the machine is `skipped`, not `failed` — otherwise every uninstalled CLI would be named in the error line. * `outcome` is a plain string, so an outcome added later lands as "not a failure" rather than failing the whole parse. On the C++ side the install path now captures output independently of the exit code — wta prints the report and *then* exits non-zero, so the existing exit-0-only helper would have discarded exactly the payload we need. RunWtaCapture returns the combined output plus the exit code, and RunWtaCaptureStdout is a thin wrapper preserving its old contract. The summary falls back to the unattributed message whenever the report is unreadable or blames no particular CLI — a timeout, a crash before the report was written, or a failure that isn't per-CLI. Adds AIAgents_HooksInstallationFailedForSummary across all 16 locales. The CLI names are brand names rendered with the same display names as the rows above the summary, and stay untranslated. Validated with the full wta suite (1587 passing), AgentHooksStatusTests (33 passing), and Debug x64 builds of Microsoft.Terminal.Settings.Editor and the unit tests. Live check of both paths: `hooks install --json --cli codex` reports the real marketplace-repoint failure with its reason and exits 1; `--cli copilot` reports installed and exits 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Keep stderr out of the install report parse Copilot review found a real defect in the previous commit. `wta hooks install --json` prints its report to stdout and then lets `main()` return Err, which makes the Rust runtime write Error: hooks installation failed for: codex codex: codex plugin marketplace add failed: ... to stderr. RunWtaCapture merged both streams into one pipe, so on exactly the path the report exists to serve — a failing install — the captured text was JSON plus that trailer, and depending on flush order possibly the trailer first. ParseInstallReportJson would fail, the UI would fall back to the unattributed summary, and naming the failing CLI would never work. Verified against the real binary: stdout carries 278 bytes of pure JSON, stderr carries a 200-byte Error block. RunWtaCapture takes a `mergeStderr` flag (default true, so RunWtaCaptureStdout and its existing callers are untouched) and the install path passes false. Non-merged stderr goes to NUL rather than a second pipe: a second pipe would need its own drain to avoid deadlocking a chatty child, and every wta subcommand already mirrors its diagnostics into the wta logs, which is where the summary points the user anyway. Also from the review: HidesDetectedCliWithoutHooks claimed to guard the Settings row-visibility decision but only asserted that HasHookState ignores binaryOnPath, so reintroducing `binaryOnPath || HasHookState(...)` at the call site would have left it green. Add ShouldShowHookRow as the single named predicate the five ViewModel call sites share, and assert the row decision through it — including the partially-installed case, which must stay visible because it's the state the user most needs to act on. Two check-spelling findings: rename the unknown-CLI test fixture off an invented word, and fix "not an detail". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use unique_hfile for the NUL stderr handle Copilot review caught a latent handle bug in the previous commit. CreateFileW signals failure with INVALID_HANDLE_VALUE, but the NUL handle was held in a wil::unique_handle, whose invalid value is nullptr. A failed open would therefore have passed `if (!nullHandle)` and been handed to CreateProcessW as hStdError. wil::unique_hfile is the type whose invalid value is INVALID_HANDLE_VALUE, so the existing null check now actually rejects the failure. It's also what the rest of the repo uses for CreateFileW results (til/io.h, WindowEmperor.cpp, types/utils.cpp). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent fa18a35 commit 9b31039

27 files changed

Lines changed: 774 additions & 57 deletions

src/cascadia/TerminalSettingsEditor/AIAgents.xaml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -435,8 +435,8 @@
435435
</StackPanel>
436436
</Border>
437437
<!-- Per-CLI rows surface installed or partial hook state.
438-
OpenCode also appears when its CLI is detected so new
439-
users can see that hook integration is supported. -->
438+
A row disappears once its hooks are removed, so the
439+
Remove button is never shown with nothing to remove. -->
440440
<Border Visibility="{x:Bind ViewModel.ShowCopilotHookRow, Mode=OneWay}"
441441
BorderThickness="0,1,0,0"
442442
BorderBrush="{ThemeResource CardStrokeColorDefaultBrush}"
@@ -568,7 +568,7 @@
568568
<Button Grid.Column="1"
569569
x:Uid="AIAgents_HooksRemoveButton"
570570
Click="{x:Bind ViewModel.RemoveOpenCodeHooks}"
571-
IsEnabled="{x:Bind ViewModel.CanRemoveOpenCodeHooks, Mode=OneWay}"
571+
IsEnabled="{x:Bind ViewModel.CanRemoveAgentHooks, Mode=OneWay}"
572572
MinWidth="120" />
573573
</Grid>
574574
</Border>

src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,7 +1237,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
12371237
_showGeminiHookRow = false;
12381238
_showCodexHookRow = false;
12391239
_showOpenCodeHookRow = false;
1240-
_openCodeHooksPresent = false;
12411240
_copilotHooksSubtitle = {};
12421241
_claudeHooksSubtitle = {};
12431242
_geminiHooksSubtitle = {};
@@ -1258,12 +1257,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
12581257
_codexCliDetected = codex && codex->binaryOnPath;
12591258
_openCodeCliDetected = openCode && openCode->binaryOnPath;
12601259

1261-
_showCopilotHookRow = AgentHooks::HasHookState(copilot);
1262-
_showClaudeHookRow = AgentHooks::HasHookState(claude);
1263-
_showGeminiHookRow = AgentHooks::HasHookState(gemini);
1264-
_showCodexHookRow = AgentHooks::HasHookState(codex);
1265-
_openCodeHooksPresent = AgentHooks::HasHookState(openCode);
1266-
_showOpenCodeHookRow = AgentHooks::ShouldShowDetectedOrConfiguredHookRow(openCode);
1260+
_showCopilotHookRow = AgentHooks::ShouldShowHookRow(copilot);
1261+
_showClaudeHookRow = AgentHooks::ShouldShowHookRow(claude);
1262+
_showGeminiHookRow = AgentHooks::ShouldShowHookRow(gemini);
1263+
_showCodexHookRow = AgentHooks::ShouldShowHookRow(codex);
1264+
_showOpenCodeHookRow = AgentHooks::ShouldShowHookRow(openCode);
12671265

12681266
_copilotHooksSubtitle = _ComputeHooksSubtitle(copilot);
12691267
_claudeHooksSubtitle = _ComputeHooksSubtitle(claude);
@@ -1280,7 +1278,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
12801278
L"IsAnyAgentCliDetected",
12811279
L"CanInstallAgentHooks",
12821280
L"CanRemoveAgentHooks",
1283-
L"CanRemoveOpenCodeHooks",
12841281
L"ShowCopilotHookRow",
12851282
L"ShowClaudeHookRow",
12861283
L"ShowGeminiHookRow",
@@ -1394,9 +1391,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
13941391
const std::wstring hooksRemovedSummary{ RS_(L"AIAgents_HooksRemovedSummary") };
13951392
const std::wstring hooksInstalledSummary{ RS_(L"AIAgents_HooksInstalledSummary") };
13961393
const auto hooksLogDir = ::IntelligentTerminal::LogDirVersioned();
1394+
const auto hooksInstallLogPath = (hooksLogDir / L"wta-install-hooks.log").wstring();
13971395
const std::wstring hooksRemovalFailedSummary{ RS_fmt(L"AIAgents_HooksRemovalFailedSummary", hooksLogDir.wstring()) };
13981396
const std::wstring hooksInstallationFailedSummary{
1399-
RS_fmt(L"AIAgents_HooksInstallationFailedSummary", (hooksLogDir / L"wta-install-hooks.log").wstring())
1397+
RS_fmt(L"AIAgents_HooksInstallationFailedSummary", hooksInstallLogPath)
14001398
};
14011399
std::wstring summary;
14021400
bool ok = false;
@@ -1408,16 +1406,43 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
14081406
{
14091407
summary = locateWtaFailedSummary;
14101408
}
1411-
else
1409+
else if (isUninstall)
14121410
{
14131411
ok = ::Microsoft::Terminal::WtaProcess::RunWtaAndWait(wtaPath, wtaArgs, 60'000);
1412+
summary = ok ? hooksRemovedSummary : hooksRemovalFailedSummary;
1413+
}
1414+
else
1415+
{
1416+
// Ask for the structured report so a failure can name the CLIs
1417+
// that failed. wta prints it and *then* exits non-zero, so we
1418+
// capture output independently of the exit code — and keep
1419+
// stderr out of it, since the failing run also writes an
1420+
// `Error: ...` line there that would break the JSON parse.
1421+
const auto run = ::Microsoft::Terminal::WtaProcess::RunWtaCapture(wtaPath,
1422+
wtaArgs + L" --json",
1423+
60'000,
1424+
nullptr,
1425+
/* mergeStderr */ false);
1426+
ok = run.completed && run.exitCode == 0;
14141427
if (ok)
14151428
{
1416-
summary = isUninstall ? hooksRemovedSummary : hooksInstalledSummary;
1429+
summary = hooksInstalledSummary;
14171430
}
14181431
else
14191432
{
1420-
summary = isUninstall ? hooksRemovalFailedSummary : hooksInstallationFailedSummary;
1433+
// Fall back to the unattributed message whenever the report
1434+
// is unreadable or blames no particular CLI — a timeout, a
1435+
// crash before the report was written, or a failure that
1436+
// isn't per-CLI all land here.
1437+
summary = hooksInstallationFailedSummary;
1438+
if (const auto report = ::Microsoft::Terminal::AgentHooks::ParseInstallReportJson(run.output))
1439+
{
1440+
const auto failed = ::Microsoft::Terminal::AgentHooks::FormatFailedCliList(*report);
1441+
if (!failed.empty())
1442+
{
1443+
summary = RS_fmt(L"AIAgents_HooksInstallationFailedForSummary", failed, hooksInstallLogPath);
1444+
}
1445+
}
14211446
}
14221447
}
14231448

src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.h

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -179,9 +179,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
179179
{
180180
return _copilotCliDetected || _claudeCliDetected || _geminiCliDetected || _codexCliDetected || _openCodeCliDetected;
181181
}
182-
// Per-CLI "row visible" flags. Existing integrations appear when they
183-
// have hook state. OpenCode also appears when its CLI is detected so
184-
// users can discover and install the newly supported integration.
182+
// Per-CLI "row visible" flags. A CLI's row appears only while it has
183+
// hook state (fully or partially installed), so removing hooks makes
184+
// the row disappear — uniformly, for every CLI.
185185
bool ShowCopilotHookRow() const noexcept { return _showCopilotHookRow; }
186186
bool ShowClaudeHookRow() const noexcept { return _showClaudeHookRow; }
187187
bool ShowGeminiHookRow() const noexcept { return _showGeminiHookRow; }
@@ -207,10 +207,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
207207
{
208208
return !IsAgentSessionHooksPolicyLocked();
209209
}
210-
bool CanRemoveOpenCodeHooks() const noexcept
211-
{
212-
return _openCodeHooksPresent && !IsAgentSessionHooksPolicyLocked();
213-
}
214210
bool IsInstallingAgentHooks() const noexcept { return _installingAgentHooks; }
215211
winrt::hstring AgentHooksInstallSummary() const { return _agentHooksInstallSummary; }
216212
bool HasAgentHooksInstallSummary() const noexcept { return !_agentHooksInstallSummary.empty(); }
@@ -290,13 +286,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
290286
bool _geminiCliDetected{ false };
291287
bool _codexCliDetected{ false };
292288
bool _openCodeCliDetected{ false };
293-
// Row visibility. OpenCode additionally appears when its CLI is detected.
289+
// Row visibility — a CLI's row shows only while it has hook state.
294290
bool _showCopilotHookRow{ false };
295291
bool _showClaudeHookRow{ false };
296292
bool _showGeminiHookRow{ false };
297293
bool _showCodexHookRow{ false };
298294
bool _showOpenCodeHookRow{ false };
299-
bool _openCodeHooksPresent{ false };
300295
// Subtitle text per CLI; empty for fully-installed CLIs.
301296
winrt::hstring _copilotHooksSubtitle;
302297
winrt::hstring _claudeHooksSubtitle;

src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.idl

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,6 @@ namespace Microsoft.Terminal.Settings.Editor
157157
Boolean ShowOpenCodeHooksSubtitle { get; };
158158
Boolean CanInstallAgentHooks { get; };
159159
Boolean CanRemoveAgentHooks { get; };
160-
Boolean CanRemoveOpenCodeHooks { get; };
161160
Boolean IsInstallingAgentHooks { get; };
162161
String AgentHooksInstallSummary { get; };
163162
// True iff AgentHooksInstallSummary is non-empty — gates the

src/cascadia/TerminalSettingsEditor/Resources/de-DE/Resources.resw

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2914,6 +2914,10 @@
29142914
<value>Hooks-Installation fehlgeschlagen. Überprüfen Sie {0} auf Details.</value>
29152915
<comment>Failure summary shown when installing agent session tracking hooks fails. {Locked="Hooks","{0}"} {0} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
29162916
</data>
2917+
<data name="AIAgents_HooksInstallationFailedForSummary" xml:space="preserve">
2918+
<value>Hooks-Installation für {0} fehlgeschlagen. Überprüfen Sie {1} auf Details.</value>
2919+
<comment>Failure summary shown when installing agent session tracking hooks fails for specific CLIs. {Locked="Hooks","{0}","{1}"} {0} is replaced with a comma-separated list of CLI names (brand names, never translated); {1} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
2920+
</data>
29172921
<data name="AIAgents_AutoErrorDetectionTitle.Text" xml:space="preserve">
29182922
<value>Automatische Fehlererkennung</value>
29192923
</data>

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3035,6 +3035,10 @@ In this context, "agent" refers to an AI agent (e.g. Copilot, Claude, Gemini), n
30353035
<value>Hooks installation failed. Check {0} for details.</value>
30363036
<comment>Failure summary shown when installing agent session tracking hooks fails. {Locked="Hooks","{0}"} {0} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
30373037
</data>
3038+
<data name="AIAgents_HooksInstallationFailedForSummary" xml:space="preserve">
3039+
<value>Hooks installation failed for {0}. Check {1} for details.</value>
3040+
<comment>Failure summary shown when installing agent session tracking hooks fails for specific CLIs. {Locked="Hooks","{0}","{1}"} {0} is replaced with a comma-separated list of CLI names (brand names, never translated); {1} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
3041+
</data>
30383042
<data name="AIAgents_CustomModels.Header" xml:space="preserve">
30393043
<value>BYOK (bring your own key) providers</value>
30403044
<comment>Section header for user-configured model providers. {Locked="BYOK"}</comment>

src/cascadia/TerminalSettingsEditor/Resources/es-ES/Resources.resw

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2914,6 +2914,10 @@
29142914
<value>Error de instalación de Hooks. Consulte {0} para obtener detalles.</value>
29152915
<comment>Failure summary shown when installing agent session tracking hooks fails. {Locked="Hooks","{0}"} {0} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
29162916
</data>
2917+
<data name="AIAgents_HooksInstallationFailedForSummary" xml:space="preserve">
2918+
<value>Error de instalación de Hooks para {0}. Consulte {1} para obtener detalles.</value>
2919+
<comment>Failure summary shown when installing agent session tracking hooks fails for specific CLIs. {Locked="Hooks","{0}","{1}"} {0} is replaced with a comma-separated list of CLI names (brand names, never translated); {1} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
2920+
</data>
29172921
<data name="AIAgents_AutoErrorDetectionTitle.Text" xml:space="preserve">
29182922
<value>Detección automática de errores</value>
29192923
</data>

src/cascadia/TerminalSettingsEditor/Resources/fr-FR/Resources.resw

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2915,6 +2915,10 @@
29152915
<value>Échec de l’installation Hooks. Consultez {0} pour plus de détails.</value>
29162916
<comment>Failure summary shown when installing agent session tracking hooks fails. {Locked="Hooks","{0}"} {0} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
29172917
</data>
2918+
<data name="AIAgents_HooksInstallationFailedForSummary" xml:space="preserve">
2919+
<value>Échec de l’installation Hooks pour {0}. Consultez {1} pour plus de détails.</value>
2920+
<comment>Failure summary shown when installing agent session tracking hooks fails for specific CLIs. {Locked="Hooks","{0}","{1}"} {0} is replaced with a comma-separated list of CLI names (brand names, never translated); {1} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
2921+
</data>
29182922
<data name="AIAgents_AutoErrorDetectionTitle.Text" xml:space="preserve">
29192923
<value>Détection automatique des erreurs</value>
29202924
</data>

src/cascadia/TerminalSettingsEditor/Resources/it-IT/Resources.resw

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2914,6 +2914,10 @@
29142914
<value>Installazione di Hooks non riuscita. Controllare {0} per i dettagli.</value>
29152915
<comment>Failure summary shown when installing agent session tracking hooks fails. {Locked="Hooks","{0}"} {0} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
29162916
</data>
2917+
<data name="AIAgents_HooksInstallationFailedForSummary" xml:space="preserve">
2918+
<value>Installazione di Hooks per {0} non riuscita. Controllare {1} per i dettagli.</value>
2919+
<comment>Failure summary shown when installing agent session tracking hooks fails for specific CLIs. {Locked="Hooks","{0}","{1}"} {0} is replaced with a comma-separated list of CLI names (brand names, never translated); {1} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
2920+
</data>
29172921
<data name="AIAgents_AutoErrorDetectionTitle.Text" xml:space="preserve">
29182922
<value>Rilevamento automatico degli errori</value>
29192923
</data>

src/cascadia/TerminalSettingsEditor/Resources/ja-JP/Resources.resw

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2914,6 +2914,10 @@
29142914
<value>Hooks のインストールに失敗しました。詳細については {0} を確認してください。</value>
29152915
<comment>Failure summary shown when installing agent session tracking hooks fails. {Locked="Hooks","{0}"} {0} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
29162916
</data>
2917+
<data name="AIAgents_HooksInstallationFailedForSummary" xml:space="preserve">
2918+
<value>{0} の Hooks のインストールに失敗しました。詳細については {1} を確認してください。</value>
2919+
<comment>Failure summary shown when installing agent session tracking hooks fails for specific CLIs. {Locked="Hooks","{0}","{1}"} {0} is replaced with a comma-separated list of CLI names (brand names, never translated); {1} is replaced with the package-aware, versioned wta-install-hooks.log path at runtime.</comment>
2920+
</data>
29172921
<data name="AIAgents_AutoErrorDetectionTitle.Text" xml:space="preserve">
29182922
<value>エラーの自動検出</value>
29192923
</data>

0 commit comments

Comments
 (0)