From 70f2a26328bdfeda969d95307eb78b39fe1001d3 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 15 Jul 2026 20:59:29 -0500 Subject: [PATCH 01/12] release: prepare 0.25.0-beta.5 (#1666) --- Directory.Build.props | 2 +- RELEASE_NOTES.md | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index 32e0f43ed..4f3788d69 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -9,7 +9,7 @@ enable true 0.25.0 - beta.4 + beta.5 - Preserve Git working context across sessions and subagents — A bounded, audience-aware Git working-context snapshot stays current in the system prompt, and coding subagents inherit recent-file/project context (#1630) - User-written AGENTS.md for application-specific agent guidance — Operators can author ~/.netclaw/identity/AGENTS.md, layered after Netclaw's embedded operating core and inherited by sub-agents (#1622) - Memory curation no longer overwrites existing documents on collision — Create-decision collisions now append new content instead of silently overwriting the existing document (#1637) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index c44b949ce..75d091dc9 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,5 +1,25 @@ # NetClaw Release Notes +## 0.25.0-beta.5 (2026-07-16) + +### Features +- **Timestamped HMAC verification for webhooks** — Webhook routes now verify request timestamps alongside HMAC signatures to prevent replay attacks, with configurable HMAC algorithm support ([#1660](https://github.com/netclaw-dev/netclaw/pull/1660)) +- **TSV support in content scanner** — Added `text/tab-separated-values` as a recognized MIME type, allowing TSV files to be scanned and processed by the content pipeline ([#1645](https://github.com/netclaw-dev/netclaw/pull/1645)) + +### Bug Fixes +- **Reminders rescan definitions before startup alerts** — Fixed: startup alerts could fire before reminder definitions were fully loaded, causing missed or duplicate reminders on restart ([#1653](https://github.com/netclaw-dev/netclaw/pull/1653)) +- **OpenAI client 2.12 compatibility** — Updated OpenAI provider plugin to work with the breaking API change in OpenAI SDK 2.12 (`OpenAIClientOptions` → `ResponsesClientOptions`) ([#1654](https://github.com/netclaw-dev/netclaw/pull/1654)) + +### Improvements +- **Tool execution pipeline refactoring** — Restructured the session tool execution pipeline with isolated invocation scope, composed execution pipeline, typed subagent context isolation, and proper lifecycle cleanup. Fixes subagent fatal spawn failures ([#1641](https://github.com/netclaw-dev/netclaw/pull/1641), [#1643](https://github.com/netclaw-dev/netclaw/pull/1643), [#1644](https://github.com/netclaw-dev/netclaw/pull/1644), [#1646](https://github.com/netclaw-dev/netclaw/pull/1646)) + +### Dependency Updates +- **Bump OpenAI client to 2.12** — Required for compatibility with the updated SDK API ([#1654](https://github.com/netclaw-dev/netclaw/pull/1654)) +- **Bump Microsoft.Extensions.AI** — 10.6.0 → 10.8.0 ([#1650](https://github.com/netclaw-dev/netclaw/pull/1650)) +- **Bump Microsoft.AspNetCore.DataProtection** — 10.0.9 → 10.0.10 ([#1657](https://github.com/netclaw-dev/netclaw/pull/1657)) +- **Bump Microsoft.NET.Test.Sdk** — 18.7.0 → 18.8.1 ([#1651](https://github.com/netclaw-dev/netclaw/pull/1651)) +- **Bump Mattermost.NET** — 5.0.0 → 5.0.3 ([#1626](https://github.com/netclaw-dev/netclaw/pull/1626)) + ## 0.25.0-beta.4 (2026-07-14) ### Features From 711eaf090f84b45592c2bd60984efda1e00baf07 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 16 Jul 2026 06:39:43 -0500 Subject: [PATCH 02/12] fix(daemon): bound the daemon-stop session drain and give CLI stop kill headroom (#1673) Two related shutdown-race defects: 1. The SIGTERM/daemon-stop CoordinatedShutdown drain task called SessionDrainHelper.DrainAsync with CancellationToken.None for the operation token, so a session parked on interactive tool approval (which can never ack) hung the call for the full 200s Akka before-service-unbind phase timeout, leaking the abandoned drain task. Fixed by bounding the drain with a TimeProvider-driven CTS sized to DaemonConfig.BoundedDrainTimeout (GracefulShutdownBudget minus a 10s safety margin), so it always finishes before the phase timeout fires. The drain's existing timeout logging now also lists which sessions didn't drain. 2. `netclaw daemon stop` waited only 10s before force-killing, and even after fixing that to match the daemon's own 200s phase timeout, the CLI and daemon would race exactly at the boundary (production evidence: a daemon force-killed ~100ms from a clean exit). Fixed by giving the CLI a 15s grace window to poll for exit after its budget elapses before escalating to SIGKILL, and adding TimeoutStopSec= to the generated systemd unit so systemd itself can't SIGKILL the cgroup out from under a still-waiting `netclaw daemon stop` (ExecStop=). All the shutdown-timing constants now live in one place (DaemonConfig.GracefulShutdownBudget and friends) so the layering (bounded drain < Akka phase timeout < CLI budget+grace < systemd TimeoutStopSec) can't drift out of lockstep again. Fixes netclaw-dev/netclaw#1664, netclaw-dev/netclaw#1665 --- .../Daemon/DaemonPathEnvironmentFileTests.cs | 9 +++ src/Netclaw.Cli/Daemon/DaemonManager.cs | 57 ++++++++++++-- .../DaemonConfigTests.cs | 46 +++++++++++ src/Netclaw.Configuration/DaemonConfig.cs | 78 +++++++++++++++++++ .../DaemonShutdownConfigurationTests.cs | 47 +++++++++++ .../Services/DaemonRestartCoordinatorTests.cs | 36 +++++++++ .../DaemonShutdownConfiguration.cs | 32 ++++++++ src/Netclaw.Daemon/Program.cs | 40 ++++++---- .../Services/SessionDrainHelper.cs | 5 +- 9 files changed, 330 insertions(+), 20 deletions(-) create mode 100644 src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs create mode 100644 src/Netclaw.Daemon/DaemonShutdownConfiguration.cs diff --git a/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs b/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs index db130cdc5..78ff21425 100644 --- a/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs +++ b/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs @@ -108,6 +108,15 @@ public void BuildDaemonUnitContent_WiresEnvironmentFile_AndOmitsInlinePath() Assert.DoesNotContain("Environment=PATH=", unit, StringComparison.Ordinal); Assert.Contains("ExecStart=/opt/netclaw/netclawd", unit, StringComparison.Ordinal); Assert.Contains("ExecStop=/opt/netclaw/netclaw daemon stop", unit, StringComparison.Ordinal); + + // Producer/consumer contract (netclaw-dev/netclaw#1665): the generated unit's + // TimeoutStopSec= must track DaemonConfig.SystemdTimeoutStopSec, not a stale literal, + // so systemd never SIGKILLs the cgroup out from under a still-legitimately-waiting + // `netclaw daemon stop` (ExecStop=). + Assert.Contains( + $"TimeoutStopSec={(int)DaemonConfig.SystemdTimeoutStopSec.TotalSeconds}", + unit, + StringComparison.Ordinal); } [Fact] diff --git a/src/Netclaw.Cli/Daemon/DaemonManager.cs b/src/Netclaw.Cli/Daemon/DaemonManager.cs index 57ddaabba..5af8f115d 100644 --- a/src/Netclaw.Cli/Daemon/DaemonManager.cs +++ b/src/Netclaw.Cli/Daemon/DaemonManager.cs @@ -177,10 +177,29 @@ await http.PostAsync( process.Kill(); } - // Wait up to 10 seconds for graceful exit. - if (!await WaitForExitAsync(process, TimeSpan.FromSeconds(10), cancellationToken)) - { - // Timed out — hard cutoff. + // Wait for graceful exit. DaemonConfig.GracefulShutdownBudget matches the daemon's own + // Akka CoordinatedShutdown "before-service-unbind" phase timeout — where session + // draining (SessionDrainHelper.DrainAsync) actually happens — so the CLI does not give + // up on a daemon that is still legitimately draining in-flight sessions (TurnLlmTimeout + // defaults to 3 minutes). See DaemonConfig.GracefulShutdownBudget remarks for the full + // layering this must respect: bounded drain < Akka phase timeout < this budget + the + // grace window below < systemd's TimeoutStopSec= (netclaw-dev/netclaw#1664, #1665). + var exitedWithinBudget = await WaitForExitAsync(process, DaemonConfig.GracefulShutdownBudget, cancellationToken); + var exitedDuringGraceWindow = false; + if (!exitedWithinBudget) + { + // The daemon's own bounded drain (netclaw-dev/netclaw#1664) can time out at almost + // exactly this same budget boundary, and still needs to finish tearing down (actor + // system termination, PID file cleanup) afterward. Poll a short additional grace + // window before escalating to SIGKILL — production evidence (#1665) showed a + // daemon force-killed ~100ms from a clean exit because the CLI escalated the + // instant its budget elapsed, with no headroom at all. + exitedDuringGraceWindow = await WaitForExitAsync(process, DaemonConfig.CliForceKillGraceWindow, cancellationToken); + } + + if (!exitedWithinBudget && !exitedDuringGraceWindow) + { + // Both the budget and the grace window elapsed — hard cutoff. string? killError = null; if (OperatingSystem.IsLinux() || OperatingSystem.IsMacOS()) { @@ -203,7 +222,27 @@ await http.PostAsync( } CleanupPidFile(); - return new DaemonResult(true, $"Daemon stopped (was PID {pid})."); + + if (exitedWithinBudget) + return new DaemonResult(true, $"Daemon stopped (was PID {pid})."); + + if (exitedDuringGraceWindow) + { + // Clean exit — no kill needed — but worth surfacing: the daemon used its full + // graceful-shutdown budget, which usually means a session was still mid-LLM-call + // at shutdown. + return new DaemonResult(true, + $"Daemon stopped (was PID {pid}); exited during the " + + $"{DaemonConfig.CliForceKillGraceWindow.TotalSeconds:F0}s grace window after the " + + $"{DaemonConfig.GracefulShutdownBudget.TotalSeconds:F0}s graceful-wait budget elapsed " + + "(no kill needed)."); + } + + return new DaemonResult(true, + $"Daemon stopped (was PID {pid}), but did not exit gracefully within " + + $"{DaemonConfig.CliForceKillBudget.TotalSeconds:F0}s (budget + grace window) and had to be " + + "force-killed. This usually means a session was still mid-LLM-call at shutdown; if it " + + "recurs, check for stuck sessions before stopping the daemon."); } /// @@ -358,6 +397,13 @@ internal void RemoveDaemonEnvironmentFile() /// The - prefix makes systemd tolerant of a missing env file: a deleted PATH /// file degrades tool resolution (which SystemdUnitPathDoctorCheck flags) /// rather than preventing the entire daemon from starting. + /// + /// TimeoutStopSec= is — comfortably + /// longer than this 's own graceful-wait-plus-grace budget + /// () so systemd never SIGKILLs the whole + /// cgroup out from under ExecStop= (netclaw daemon stop) while that command is + /// still legitimately waiting on the daemon's own shutdown (netclaw-dev/netclaw#1665). + /// Existing installs only pick this up after re-running netclaw daemon install. /// internal static string BuildDaemonUnitContent(string binaryPath, string cliBinaryPath, string environmentFilePath) => $""" [Unit] @@ -368,6 +414,7 @@ internal static string BuildDaemonUnitContent(string binaryPath, string cliBinar Type=simple ExecStart={binaryPath} ExecStop={cliBinaryPath} daemon stop + TimeoutStopSec={(int)DaemonConfig.SystemdTimeoutStopSec.TotalSeconds} Restart=always RestartSec=5 Environment=DOTNET_ENVIRONMENT=Production diff --git a/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs b/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs index 7adb3a7d2..abc2f063f 100644 --- a/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs +++ b/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs @@ -287,4 +287,50 @@ public void Validator_rejects_invalid_trusted_proxy_entry() Assert.Contains(issues, issue => issue.Message.Contains("not-an-ip", StringComparison.OrdinalIgnoreCase)); } + + // ── Shutdown-budget layering (netclaw-dev/netclaw#1664, #1665) ─────────── + // + // Four surfaces derive from DaemonConfig.GracefulShutdownBudget and must stay strictly + // ordered: the daemon-stop drain bound, the Akka before-service-unbind phase timeout, the + // CLI's force-kill budget (graceful wait + grace window), and the generated systemd unit's + // TimeoutStopSec=. If any margin is changed inconsistently in the future — e.g. the CLI's + // grace window grows past the systemd teardown margin — this test fails instead of silently + // reintroducing the guaranteed-SIGKILL race these issues were filed against. + + [Fact] + public void ShutdownBudgetLayering_bounded_drain_finishes_before_the_akka_phase_timeout() + { + Assert.True( + DaemonConfig.BoundedDrainTimeout < DaemonConfig.GracefulShutdownBudget, + "the daemon-stop drain's own deadline must fire before the Akka phase timeout abandons the task outright"); + } + + [Fact] + public void ShutdownBudgetLayering_cli_waits_at_least_as_long_as_the_daemon_phase_timeout() + { + Assert.True( + DaemonConfig.GracefulShutdownBudget <= DaemonConfig.CliForceKillBudget, + "the CLI's graceful-wait budget must be at least the daemon's own Akka phase timeout"); + } + + [Fact] + public void ShutdownBudgetLayering_cli_force_kill_budget_stays_under_systemd_timeout_stop_sec() + { + Assert.True( + DaemonConfig.CliForceKillBudget < DaemonConfig.SystemdTimeoutStopSec, + "systemd's TimeoutStopSec= must exceed the CLI's own budget+grace, or systemd SIGKILLs " + + "the cgroup out from under a `netclaw daemon stop` (ExecStop=) that is still legitimately waiting"); + } + + [Fact] + public void ShutdownBudgetLayering_matches_the_documented_second_values() + { + // Pins the concrete values referenced in netclaw-dev/netclaw#1665's evidence trail + // (200s phase timeout, 230s TimeoutStopSec) so a change to any constant is a visible, + // deliberate diff rather than a silent drift. + Assert.Equal(TimeSpan.FromSeconds(190), DaemonConfig.BoundedDrainTimeout); + Assert.Equal(TimeSpan.FromSeconds(200), DaemonConfig.GracefulShutdownBudget); + Assert.Equal(TimeSpan.FromSeconds(215), DaemonConfig.CliForceKillBudget); + Assert.Equal(TimeSpan.FromSeconds(230), DaemonConfig.SystemdTimeoutStopSec); + } } diff --git a/src/Netclaw.Configuration/DaemonConfig.cs b/src/Netclaw.Configuration/DaemonConfig.cs index 6b881865d..d2d87182a 100644 --- a/src/Netclaw.Configuration/DaemonConfig.cs +++ b/src/Netclaw.Configuration/DaemonConfig.cs @@ -20,6 +20,84 @@ public sealed record DaemonConfig /// public const int DefaultPort = 5199; + /// + /// Worst-case time the daemon's graceful shutdown drain is allotted before something gives + /// up and forces termination. Sized to comfortably exceed SessionConfig.TurnLlmTimeout's + /// default (3 minutes) so a session mid-LLM-call during shutdown can finish draining instead + /// of being interrupted. + /// + /// Single source of truth shared by every shutdown-timing surface that must stay in + /// lockstep (netclaw-dev/netclaw#1664, #1665): + /// - Netclaw.Daemon's Akka coordinated-shutdown.phases.before-service-unbind.timeout + /// HOCON override (see DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon), + /// where SessionDrainHelper.DrainAsync actually drains sessions + /// - , the deadline the daemon-stop CoordinatedShutdown + /// task gives that same drain call — always strictly under this budget, so the drain + /// finishes (timed out or not) before the phase timeout above abandons it outright + /// - Netclaw.Daemon's generic-host HostOptions.ShutdownTimeout + /// - Netclaw.Cli.Daemon.DaemonManager's graceful-wait before force-kill in + /// netclaw daemon stop, followed by + /// - the generated systemd unit's TimeoutStopSec= + /// (; see DaemonManager.BuildDaemonUnitContent) + /// + /// #1664: the SIGTERM/daemon-stop drain previously waited unbounded + /// (CancellationToken.None), so a session parked on interactive tool approval — which + /// cannot ack within any budget — hung the call for the full phase timeout, leaking the + /// abandoned drain task. #1665: the CLI's force-kill wait equaled this exact budget with no + /// headroom, so whenever the drain legitimately used the full budget, the CLI guaranteed a + /// SIGKILL race the daemon could not win (evidence: a production daemon force-killed ~100ms + /// from a clean exit). + /// + public static readonly TimeSpan GracefulShutdownBudget = TimeSpan.FromSeconds(200); + + /// + /// Safety margin subtracted from to produce + /// , so the daemon-stop session drain always finishes — + /// timed out or not — strictly before the Akka before-service-unbind phase timeout + /// itself fires and abandons the drain task. + /// + public static readonly TimeSpan DrainSafetyMargin = TimeSpan.FromSeconds(10); + + /// + /// Additional time netclaw daemon stop polls for process exit after + /// elapses, before escalating to SIGKILL. Covers the + /// daemon's own post-drain teardown (actor system termination, PID file cleanup) so a + /// daemon that finishes right at the budget boundary is not killed out from under itself + /// (netclaw-dev/netclaw#1665). + /// + public static readonly TimeSpan CliForceKillGraceWindow = TimeSpan.FromSeconds(15); + + /// + /// Headroom added on top of for the systemd unit's + /// TimeoutStopSec= (), so systemd never SIGKILLs + /// the whole cgroup out from under ExecStop= (netclaw daemon stop) while that + /// command is still legitimately waiting on the daemon's own bounded shutdown. + /// + public static readonly TimeSpan SystemdTeardownMargin = TimeSpan.FromSeconds(30); + + /// + /// The deadline the daemon-stop CoordinatedShutdown task gives + /// SessionDrainHelper.DrainAsync. Always strictly less than + /// so the drain completes — as timed out if + /// necessary — before the Akka phase timeout fires. + /// + public static TimeSpan BoundedDrainTimeout => GracefulShutdownBudget - DrainSafetyMargin; + + /// + /// Total time netclaw daemon stop allows before escalating to SIGKILL: the + /// graceful wait (matching the daemon's own Akka phase timeout) plus + /// . + /// + public static TimeSpan CliForceKillBudget => GracefulShutdownBudget + CliForceKillGraceWindow; + + /// + /// The systemd unit's TimeoutStopSec= value: comfortably longer than + /// so systemd never SIGKILLs the whole cgroup out from + /// under ExecStop= (netclaw daemon stop) while that command is still + /// legitimately waiting on the daemon's own shutdown. + /// + public static TimeSpan SystemdTimeoutStopSec => GracefulShutdownBudget + SystemdTeardownMargin; + /// /// IP address the daemon binds to. Defaults to loopback (127.0.0.1). /// diff --git a/src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs b/src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs new file mode 100644 index 000000000..67a9e20b4 --- /dev/null +++ b/src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs @@ -0,0 +1,47 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Akka.Configuration; +using Netclaw.Configuration; +using Xunit; + +namespace Netclaw.Daemon.Tests; + +/// +/// Tests for , which +/// replaced the inline HOCON literal Program.cs used to prepend a hardcoded 200s +/// before-service-unbind phase timeout (netclaw-dev/netclaw#1664, #1665). Proves the emitted +/// HOCON tracks whatever resolves to instead +/// of drifting back to a literal that could disagree with the daemon-stop drain's own bound. +/// +public sealed class DaemonShutdownConfigurationTests +{ + [Theory] + [InlineData(200, 200)] + [InlineData(90, 90)] + public void BuildCoordinatedShutdownHocon_interpolates_the_given_budget_in_seconds( + int budgetSeconds, int expectedPhaseTimeoutSeconds) + { + var hocon = DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon(TimeSpan.FromSeconds(budgetSeconds)); + + var config = ConfigurationFactory.ParseString(hocon); + + Assert.Equal(TimeSpan.FromSeconds(expectedPhaseTimeoutSeconds), + config.GetTimeSpan("akka.coordinated-shutdown.phases.before-service-unbind.timeout")); + Assert.False(config.GetBoolean("akka.coordinated-shutdown.exit-clr")); + } + + [Fact] + public void BuildCoordinatedShutdownHocon_tracks_DaemonConfig_GracefulShutdownBudget() + { + var hocon = DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon(DaemonConfig.GracefulShutdownBudget); + + var config = ConfigurationFactory.ParseString(hocon); + + Assert.Equal( + DaemonConfig.GracefulShutdownBudget, + config.GetTimeSpan("akka.coordinated-shutdown.phases.before-service-unbind.timeout")); + } +} diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs index e2171c0b0..0e548af4f 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs @@ -194,6 +194,42 @@ public async Task SessionDrainHelper_propagates_caller_cancellation() await Assert.ThrowsAnyAsync(() => operation); } + [Fact] + public async Task SessionDrainHelper_daemon_stop_bound_times_out_instead_of_hanging_when_a_session_never_acks() + { + // Mirrors the daemon-stop CoordinatedShutdown drain task wired in Program.cs + // (netclaw-dev/netclaw#1664): a session whose in-flight turn is parked on interactive + // tool approval never acks PrepareForDaemonRestart. Previously this call passed + // CancellationToken.None for the operation token and hung until Akka's own 200s + // before-service-unbind phase timeout abandoned the task. The bounded CTS below — + // sized from DaemonConfig.BoundedDrainTimeout (GracefulShutdownBudget minus + // DrainSafetyMargin) and driven by TimeProvider exactly as Program.cs constructs it — + // must make the drain complete with a timed-out result well before that. + var time = new FakeTimeProvider(); + var activeIds = new[] { "slack/approval-parked" }; + var drain = new DrainControl(activeIds, activeIds); // never acknowledged + var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( + activeIds, + drain, + throwOnEnumeration: false))); + using var deadlineCts = new CancellationTokenSource(DaemonConfig.BoundedDrainTimeout, time); + + var operation = SessionDrainHelper.DrainAsync( + sessionManager, + "daemon-stop", + NullLogger.Instance, + deadlineCts.Token, + CancellationToken.None); + await drain.AllRequestsObserved; + time.Advance(DaemonConfig.BoundedDrainTimeout); + + var result = await operation; + + Assert.Single(result.AllSessionIds); + Assert.Empty(result.DrainedSessionIds); + Assert.Equal("slack/approval-parked", Assert.Single(result.TimedOutSessionIds).Value); + } + public async ValueTask DisposeAsync() { await _system.Terminate(); diff --git a/src/Netclaw.Daemon/DaemonShutdownConfiguration.cs b/src/Netclaw.Daemon/DaemonShutdownConfiguration.cs new file mode 100644 index 000000000..55d897e02 --- /dev/null +++ b/src/Netclaw.Daemon/DaemonShutdownConfiguration.cs @@ -0,0 +1,32 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Netclaw.Configuration; + +namespace Netclaw.Daemon; + +/// +/// Builds the Akka CoordinatedShutdown HOCON override that bounds the daemon's session-drain +/// phase. Extracted into its own testable method (rather than an inline string literal in +/// Program.cs's top-level statements) so a unit test can assert the interpolated timeout +/// tracks instead of drifting back to a +/// hardcoded literal (see remarks for why +/// that drift is the exact class of bug behind netclaw-dev/netclaw#1664 and #1665). +/// +internal static class DaemonShutdownConfiguration +{ + /// + /// Coordinated-shutdown HOCON: disables the CLR-exit side effect (the daemon's own + /// restart loop owns process lifetime, not CoordinatedShutdown) and sizes the + /// before-service-unbind phase — where session draining + /// (SessionDrainHelper.DrainAsync) actually runs — to . + /// + public static string BuildCoordinatedShutdownHocon(TimeSpan gracefulShutdownBudget) => $$""" + akka.coordinated-shutdown { + exit-clr = off + phases.before-service-unbind.timeout = {{(int)gracefulShutdownBudget.TotalSeconds}}s + } + """; +} diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index a81e6eb59..35ccbf7bf 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -419,7 +419,15 @@ static void ConfigureDaemonServices( services.Configure(options => { - options.ShutdownTimeout = TimeSpan.FromSeconds(30); + // Generic-host shutdown ceiling for all hosted services combined. Kept in lockstep + // with DaemonConfig.SystemdTimeoutStopSec (= GracefulShutdownBudget + teardown margin) + // rather than a bare, unrelated literal: a value shorter than the Akka + // before-service-unbind phase timeout below would silently reintroduce the same class + // of mismatch behind netclaw-dev/netclaw#1665 (budgets that look independent but must + // stay ordered). AkkaHostedService.StopAsync does not observe this cancellation token + // and awaits CoordinatedShutdown.Run to its own natural completion, so this value does + // not itself truncate the drain — it only keeps the documented layering consistent. + options.ShutdownTimeout = DaemonConfig.SystemdTimeoutStopSec; }); // Resolve models for session config @@ -990,16 +998,13 @@ static void ConfigureDaemonServices( { // Prevent coordinated shutdown from calling Environment.Exit(), // which would kill the process before the restart loop can iterate. - // The before-service-unbind phase needs a generous timeout because sessions - // mid-LLM-call (TurnLlmTimeout defaults to 3 minutes) must finish before - // passivation can begin. + // The before-service-unbind phase needs a generous timeout (DaemonConfig. + // GracefulShutdownBudget) because sessions mid-LLM-call (TurnLlmTimeout defaults to + // 3 minutes) must finish before passivation can begin. See DaemonConfig. + // GracefulShutdownBudget remarks for the full set of surfaces this must stay in + // lockstep with. akkaBuilder.AddHocon( - """ - akka.coordinated-shutdown { - exit-clr = off - phases.before-service-unbind.timeout = 200s - } - """, + DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon(DaemonConfig.GracefulShutdownBudget), HoconAddMode.Prepend); akkaBuilder = akkaBuilder.ConfigureLoggers(setup => @@ -1055,8 +1060,8 @@ static void ConfigureDaemonServices( // Runs in an early CoordinatedShutdown phase while actors are still alive. // If DaemonRestartCoordinator already drained sessions (config reload), the ingress // gate will be closed and this task skips its drain to avoid double-draining. - // The phase timeout (200s) is generous because sessions mid-LLM-call must finish - // before passivation can begin. + // The phase timeout (DaemonConfig.GracefulShutdownBudget) is generous because + // sessions mid-LLM-call must finish before passivation can begin. var cs = CoordinatedShutdown.Get(system); var sessionManager = registry.Get(); var ingressGate = sp.GetRequiredService(); @@ -1073,11 +1078,20 @@ static void ConfigureDaemonServices( try { + // Bounded strictly under DaemonConfig.GracefulShutdownBudget (the Akka phase + // timeout above) so the drain always completes -- timed out or not -- before + // the phase timeout itself fires and abandons this task outright. + // netclaw-dev/netclaw#1664: a session parked on interactive tool approval + // never acks PrepareForDaemonRestart, so an unbounded wait here (previously + // CancellationToken.None, CancellationToken.None) hung for the full 200s + // phase timeout with no timeout of its own, leaking the abandoned drain task. + using var drainDeadlineCts = new CancellationTokenSource(DaemonConfig.BoundedDrainTimeout, tp); + var drainResult = await SessionDrainHelper.DrainAsync( sessionManager, "daemon-stop", drainLogger, - CancellationToken.None, + drainDeadlineCts.Token, CancellationToken.None); lifecycleNotifier.NotifyShutdown("daemon-stop", drainResult.ToNotificationContext()); diff --git a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs index 14481c893..b9dbdc6b0 100644 --- a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs +++ b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs @@ -100,9 +100,10 @@ public static async Task DrainAsync( else { logger.LogWarning( - "Drain completed with {DrainedCount} session(s) drained and {TimedOutCount} timed out; timed-out sessions will recover from the last durable checkpoint.", + "Drain completed with {DrainedCount} session(s) drained and {TimedOutCount} timed out ({TimedOutSessionIds}); timed-out sessions will recover from the last durable checkpoint.", drained.Length, - timedOut.Length); + timedOut.Length, + string.Join(", ", timedOut.Select(static id => id.Value))); } return new DrainResult(sessionIds, drained, timedOut); From 51ccbb846e0df9894e1686c6c51211d193c08841 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 16 Jul 2026 07:20:36 -0500 Subject: [PATCH 03/12] fix(cli): reject numeric model modalities (#1677) --- src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs | 5 +++++ src/Netclaw.Cli/Model/ModelCommand.cs | 11 ++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs index b541261d7..d28880a29 100644 --- a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs @@ -439,6 +439,11 @@ public async Task Set_ClearModalities_RemovesExistingOverride() [InlineData("--input-modalities requires a value", "--input-modalities")] [InlineData("unknown argument '--input-modalites'", "--input-modalites", "Text")] [InlineData("invalid modalities", "--input-modalities", "3")] + [InlineData("invalid modalities", "--input-modalities", "1")] + [InlineData("invalid modalities", "--input-modalities", "2")] + [InlineData("invalid modalities", "--input-modalities", "4")] + [InlineData("invalid modalities", "--input-modalities", "8")] + [InlineData("invalid modalities", "--output-modalities", "1")] [InlineData("cannot be combined", "--context-window", "32768", "--clear-context-window")] public async Task Set_InvalidOptions_ReturnErrorWithoutWriting( string expectedError, diff --git a/src/Netclaw.Cli/Model/ModelCommand.cs b/src/Netclaw.Cli/Model/ModelCommand.cs index 44021cc61..c254c0040 100644 --- a/src/Netclaw.Cli/Model/ModelCommand.cs +++ b/src/Netclaw.Cli/Model/ModelCommand.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Globalization; using System.Text.Json; using Microsoft.Extensions.Configuration; using Netclaw.Cli.Config; @@ -306,11 +307,11 @@ private static bool TryParseModalities(string value, out ModelModality modalitie var result = ModelModality.None; foreach (var token in value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) { - // Enum.TryParse also accepts the underlying integer ("3" → Text|Image), but the contract - // advertised in the help text and error message is named flags only. Enum.IsDefined - // rejects a numeric token because its parsed value is not a single declared member, so a - // mistyped or scripted number is not silently coerced into a modality set. - if (!Enum.TryParse(token, ignoreCase: true, out ModelModality parsed) + // Enum.TryParse accepts underlying integers, including values that happen to be declared + // members ("1" → Text). The CLI contract is named flags only, so reject numeric tokens + // before parsing rather than relying on Enum.IsDefined to distinguish composites. + if (int.TryParse(token, NumberStyles.Integer, CultureInfo.InvariantCulture, out _) + || !Enum.TryParse(token, ignoreCase: true, out ModelModality parsed) || !Enum.IsDefined(parsed) || parsed == ModelModality.None) return false; From 20a1fa08eb2d23041dcb0ef974a7c450b13e08f4 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 16 Jul 2026 10:51:59 -0500 Subject: [PATCH 04/12] fix(cli): handle model migration errors without crashing (#1678) * fix(cli): handle model migration validation errors * test(configuration): use non-resetting path joins --- docs/spec/SPEC-004-cli-contract.md | 2 + docs/spec/SPEC-011-daemon-architecture.md | 3 +- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../references/diagnostics.md | 6 +- .../Doctor/DoctorFixServiceTests.cs | 80 +++++++++++++ .../Model/ModelCommandTests.cs | 110 ++++++++++++++++++ src/Netclaw.Cli/Config/ConfigFileHelper.cs | 9 +- src/Netclaw.Cli/Config/ModelEntryWriter.cs | 30 +++-- src/Netclaw.Cli/Doctor/DoctorFixService.cs | 9 -- src/Netclaw.Cli/Model/ModelCommand.cs | 24 ++-- src/Netclaw.Cli/Program.cs | 68 ++++++----- .../CrashLogWriterTests.cs | 66 +++++++++++ src/Netclaw.Configuration/CrashLogWriter.cs | 5 +- tests/smoke/scenarios/doctor.sh | 50 ++++++++ tests/smoke/scenarios/provider-model-cli.sh | 85 ++++++++++++++ 15 files changed, 482 insertions(+), 67 deletions(-) create mode 100644 src/Netclaw.Configuration.Tests/CrashLogWriterTests.cs diff --git a/docs/spec/SPEC-004-cli-contract.md b/docs/spec/SPEC-004-cli-contract.md index 259d6b219..ff05e9969 100644 --- a/docs/spec/SPEC-004-cli-contract.md +++ b/docs/spec/SPEC-004-cli-contract.md @@ -100,6 +100,8 @@ Behavior: - optional output: JSON for automation - exit code `0` for success - exit code `1` for validation, policy, or runtime failures +- expected model-configuration migration failures are validation failures: print actionable + output, return exit code `1`, and do not create crash logs or emit stack traces - exit code `2` for usage and argument errors ## Safety Rules diff --git a/docs/spec/SPEC-011-daemon-architecture.md b/docs/spec/SPEC-011-daemon-architecture.md index 3ca232b78..4218d7773 100644 --- a/docs/spec/SPEC-011-daemon-architecture.md +++ b/docs/spec/SPEC-011-daemon-architecture.md @@ -225,7 +225,8 @@ The daemon installs process-level exception handlers at startup for: - `AppDomain.CurrentDomain.UnhandledException` - `TaskScheduler.UnobservedTaskException` -On either path, Netclaw writes a crash log under `~/.netclaw/logs/crash-*.log` +On either path, Netclaw writes a crash log under `/logs/crash-*.log`; +`NETCLAW_HOME` defaults to `~/.netclaw` with process diagnostics and the latest known session/turn context. When DI is available, the daemon also emits an operational alert with type `daemon.crashing` (category `DaemonCrashed`) so configured webhook targets can diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 6ff82b499..119bab907 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.31.0" + version: "2.32.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md index ec9ffd12e..4439d4c74 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md +++ b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md @@ -11,8 +11,8 @@ When something seems wrong with Netclaw itself: 2. If doctor reports fixable issues, run `netclaw doctor --fix --dry-run` to preview auto-repairs (schema-driven: stale properties, enum coercion, missing defaults) 3. Run `netclaw status` via `shell_execute` — live runtime state from daemon -3. Check daemon logs at `~/.netclaw/logs/daemon-{yyyy-MM-dd}.log` -4. Check session logs at `~/.netclaw/logs/sessions/{sanitized-session-id}/session.log` +3. Check daemon logs at `/logs/daemon-{yyyy-MM-dd}.log` (`NETCLAW_HOME` defaults to `~/.netclaw`) +4. Check session logs at `/logs/sessions/{sanitized-session-id}/session.log` If `netclaw status` or `netclaw chat` prints `daemon not configured - please run netclaw init`, do not troubleshoot daemon reachability or model defaults. The @@ -75,7 +75,7 @@ debugging a daemon-wide problem → read `daemon.log`. | No LLM responses | `netclaw doctor`; verify provider credentials | | Missing tools | `netclaw mcp list`; check MCP connection state | | Memory recall degraded | `netclaw status` memory section | -| Daemon won't start | crash logs at `~/.netclaw/logs/crash-*.log` | +| Daemon won't start | crash logs at `/logs/crash-*.log` (`NETCLAW_HOME` defaults to `~/.netclaw`) | | Docker daemon cannot create `/home/netclaw/.netclaw/*` | Official image entrypoint repairs writable bind mounts to UID/GID `1654:1654`; if bypassed or read-only, run `sudo chown -R 1654:1654 ` or use a Docker named volume | | Discord/Slack channel offline | `netclaw status` shows the channel `disconnected` with a reason. Discord may also report `degraded` when Discord.Net says the socket is connected but the gateway is not ready, such as after a resumed session that Netclaw is replacing with a clean reconnect. A misconfigured channel (bad token, missing Discord Message Content intent) degrades only that channel — the daemon keeps running and other channels are unaffected. A transient network failure retries automatically; a config/permission failure stays offline until the operator fixes the config and restarts the daemon. | | `command not found` for `netclaw`/`dotnet`/a user tool from the shell tool when the daemon runs as a systemd service | The systemd `--user` service does not inherit your login-shell `PATH`; `netclaw daemon install` captures it into `~/.netclaw/config/daemon.env`. Run `netclaw doctor` (the **Systemd Unit PATH** check flags a missing/stale/legacy env file), then `netclaw doctor --fix` to rehydrate `PATH` from your current shell (or re-run `netclaw daemon install`), and finally `systemctl --user restart netclaw`. Installed a new tool after install? Its dir won't be seen until you re-run one of those and restart. Per-directory managers (`mise`/`asdf`/`direnv`) are not captured. | diff --git a/src/Netclaw.Cli.Tests/Doctor/DoctorFixServiceTests.cs b/src/Netclaw.Cli.Tests/Doctor/DoctorFixServiceTests.cs index 245bb3d22..98a3a96a0 100644 --- a/src/Netclaw.Cli.Tests/Doctor/DoctorFixServiceTests.cs +++ b/src/Netclaw.Cli.Tests/Doctor/DoctorFixServiceTests.cs @@ -5,12 +5,14 @@ // ----------------------------------------------------------------------- using Netclaw.Cli.Daemon; using System.Text.Json; +using Netclaw.Cli.Config; using Netclaw.Cli.Doctor; using Netclaw.Configuration; using Xunit; namespace Netclaw.Cli.Tests.Doctor; +[Collection(Netclaw.Cli.Tests.LegacyModelEnvironmentCollection.Name)] public sealed class DoctorFixServiceTests { // POSIX install dir: systemd units are always POSIX-style regardless of the host OS @@ -283,6 +285,84 @@ public async Task DoesNotThrow_WhenUnitEnvironmentFilePathIsMalformed() Assert.DoesNotContain(plan.Fixes, f => f.FilePath == paths.DaemonEnvironmentFilePath); } + [Fact] + public async Task LegacyEnvironmentOverride_BlocksMigrationWithoutChangingConfig() + { + var paths = NewPaths(); + const string config = + """ + { + "configVersion": 1, + "Models": { + "Main": { + "Provider": "vllm", + "ModelId": "qwen-vl" + } + } + } + """; + await File.WriteAllTextAsync(paths.NetclawConfigPath, config, TestContext.Current.CancellationToken); + const string envVar = "NETCLAW_Models__Main__ContextWindow"; + var previous = Environment.GetEnvironmentVariable(envVar); + + try + { + Environment.SetEnvironmentVariable(envVar, "65536"); + var service = ConfigOnlyService(paths); + + var exception = await Assert.ThrowsAsync( + () => service.BuildPlanAsync(TestContext.Current.CancellationToken)); + + Assert.Contains(envVar, exception.Message, StringComparison.Ordinal); + Assert.Equal(config, await File.ReadAllTextAsync( + paths.NetclawConfigPath, TestContext.Current.CancellationToken)); + } + finally + { + Environment.SetEnvironmentVariable(envVar, previous); + } + } + + [Fact] + public async Task LegacyEnvironmentOverride_DoesNotBlockAlreadyNamedModels() + { + var paths = NewPaths(); + const string config = + """ + { + "configVersion": 1, + "Models": { + "Definitions": { + "main": { + "Provider": "vllm", + "ModelId": "qwen-vl" + } + }, + "Roles": { + "Main": "main" + } + } + } + """; + await File.WriteAllTextAsync(paths.NetclawConfigPath, config, TestContext.Current.CancellationToken); + const string envVar = "NETCLAW_Models__Main__ContextWindow"; + var previous = Environment.GetEnvironmentVariable(envVar); + + try + { + Environment.SetEnvironmentVariable(envVar, "65536"); + var service = ConfigOnlyService(paths); + + var plan = await service.BuildPlanAsync(TestContext.Current.CancellationToken); + + Assert.DoesNotContain(plan.Fixes, fix => fix.FilePath == paths.NetclawConfigPath); + } + finally + { + Environment.SetEnvironmentVariable(envVar, previous); + } + } + private static NetclawPaths NewPaths() { var paths = new NetclawPaths(CreateTempBasePath()); diff --git a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs index d28880a29..3b4ec58b9 100644 --- a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs @@ -12,6 +12,7 @@ namespace Netclaw.Cli.Tests.Model; +[Collection(Netclaw.Cli.Tests.LegacyModelEnvironmentCollection.Name)] public sealed class ModelCommandTests : IDisposable { private readonly DisposableTempDir _dir = new(); @@ -618,6 +619,115 @@ public async Task Set_CorruptModalityButValidWindow_PreservesWindowEndToEnd() Assert.False(main.TryGetProperty("InputModalities", out _)); // corrupt override dropped } + [Fact] + public async Task Set_LegacyEnvironmentOverride_ReturnsErrorWithoutChangingConfig() + { + var config = ProvidersOnly(); + config["Models"] = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "my-ollama", + ["ModelId"] = "qwen3:30b" + } + }; + WriteConfig(config); + var original = File.ReadAllText(_paths.NetclawConfigPath); + const string envVar = "NETCLAW_Models__Main__ContextWindow"; + var previous = Environment.GetEnvironmentVariable(envVar); + + try + { + Environment.SetEnvironmentVariable(envVar, "65536"); + + var exitCode = await ModelCommand.RunAsync( + ["model", "set", "main", "my-ollama", "qwen3:8b"], _paths, output: _output); + + Assert.Equal(1, exitCode); + Assert.Contains( + $"Error: Cannot migrate Models while legacy environment override '{envVar}' is set.", + _output.ToString(), StringComparison.Ordinal); + Assert.Equal(original, File.ReadAllText(_paths.NetclawConfigPath)); + } + finally + { + Environment.SetEnvironmentVariable(envVar, previous); + } + } + + [Fact] + public async Task Set_ConflictingLegacyRoles_ReturnsErrorWithoutChangingConfig() + { + var config = ProvidersOnly(); + config["Models"] = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "my-ollama", + ["ModelId"] = "qwen3:30b", + ["ContextWindow"] = 32768 + }, + ["Fallback"] = new Dictionary + { + ["Provider"] = "my-ollama", + ["ModelId"] = "qwen3:30b", + ["ContextWindow"] = 65536 + } + }; + WriteConfig(config); + var original = File.ReadAllText(_paths.NetclawConfigPath); + + var exitCode = await ModelCommand.RunAsync( + ["model", "set", "compaction", "my-ollama", "qwen3:30b"], _paths, output: _output); + + Assert.Equal(1, exitCode); + Assert.Contains( + "Error: Legacy model roles conflict for my-ollama/qwen3:30b; align their metadata before migration.", + _output.ToString(), StringComparison.Ordinal); + Assert.Equal(original, File.ReadAllText(_paths.NetclawConfigPath)); + } + + [Fact] + public async Task Clear_LegacyEnvironmentOverride_ReturnsErrorWithoutChangingConfig() + { + var config = ProvidersOnly(); + config["Models"] = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "my-ollama", + ["ModelId"] = "qwen3:30b" + }, + ["Fallback"] = new Dictionary + { + ["Provider"] = "my-ollama", + ["ModelId"] = "qwen3:8b" + } + }; + WriteConfig(config); + var original = File.ReadAllText(_paths.NetclawConfigPath); + const string envVar = "NETCLAW_Models__Fallback__ContextWindow"; + var previous = Environment.GetEnvironmentVariable(envVar); + + try + { + Environment.SetEnvironmentVariable(envVar, "65536"); + + var exitCode = await ModelCommand.RunAsync( + ["model", "clear", "fallback"], _paths, output: _output); + + Assert.Equal(1, exitCode); + Assert.Contains( + $"Error: Cannot migrate Models while legacy environment override '{envVar}' is set.", + _output.ToString(), StringComparison.Ordinal); + Assert.Equal(original, File.ReadAllText(_paths.NetclawConfigPath)); + } + finally + { + Environment.SetEnvironmentVariable(envVar, previous); + } + } + private static Dictionary WithMainEntry(Dictionary main) { var config = ProvidersOnly(); diff --git a/src/Netclaw.Cli/Config/ConfigFileHelper.cs b/src/Netclaw.Cli/Config/ConfigFileHelper.cs index 2fd3f3fed..5094f53ce 100644 --- a/src/Netclaw.Cli/Config/ConfigFileHelper.cs +++ b/src/Netclaw.Cli/Config/ConfigFileHelper.cs @@ -177,14 +177,7 @@ private static void PreserveLegacyModelsBackup(string path, Dictionary__* and " + - "NETCLAW_Models__Roles__* first."); - } + ModelEntryWriter.ThrowIfLegacyEnvironmentOverride(); var backupPath = path + ".legacy-models.bak"; if (!File.Exists(backupPath)) diff --git a/src/Netclaw.Cli/Config/ModelEntryWriter.cs b/src/Netclaw.Cli/Config/ModelEntryWriter.cs index 0c3f7e697..341de4f16 100644 --- a/src/Netclaw.Cli/Config/ModelEntryWriter.cs +++ b/src/Netclaw.Cli/Config/ModelEntryWriter.cs @@ -37,10 +37,24 @@ internal static bool MigrateLegacy(Dictionary modelsSection) return false; if (!modelsSection.Keys.Any(key => key is "Main" or "Fallback" or "Compaction")) return false; + + ThrowIfLegacyEnvironmentOverride(); EnsureNamedShape(modelsSection); return true; } + internal static void ThrowIfLegacyEnvironmentOverride() + { + var legacyEnvironmentOverride = FindLegacyEnvironmentOverride(); + if (legacyEnvironmentOverride is null) + return; + + throw new ModelConfigurationException( + $"Cannot migrate Models while legacy environment override '{legacyEnvironmentOverride}' is set. " + + "Move model overrides to NETCLAW_Models__Definitions____* and " + + "NETCLAW_Models__Roles__* first."); + } + internal static bool ClearRole(Dictionary modelsSection, string roleKey) { var (_, roles) = EnsureNamedShape(modelsSection); @@ -123,12 +137,12 @@ private static (Dictionary Definitions, Dictionary Definitions, Dictionary Definitions, Dictionary(raw) - ?? throw new InvalidOperationException($"Models:{role} could not be parsed."); + ?? throw new ModelConfigurationException($"Models:{role} could not be parsed."); } catch (JsonException) { model = ReadLegacyIdentity(raw) - ?? throw new InvalidOperationException($"Models:{role} could not be repaired."); + ?? throw new ModelConfigurationException($"Models:{role} could not be repaired."); } var existingName = FindDefinition(definitions, model.Provider, model.ModelId); if (existingName is not null) @@ -167,7 +181,7 @@ private static (Dictionary Definitions, Dictionary(definitions[existingName])!; if (!Equivalent(existing, model)) { - throw new InvalidOperationException( + throw new ModelConfigurationException( $"Legacy model roles conflict for {model.Provider}/{model.ModelId}; " + $"align their metadata before migration."); } @@ -219,7 +233,7 @@ private static Dictionary GetDictionary(Dictionary BuildModelEntry( } } +internal sealed class ModelConfigurationException(string message) : Exception(message); + /// /// An operator's intent for an overridable, operator-owned model attribute on model set — /// a modality set () or the context window (). A plain diff --git a/src/Netclaw.Cli/Doctor/DoctorFixService.cs b/src/Netclaw.Cli/Doctor/DoctorFixService.cs index 57e5e19ce..d5cf22b5f 100644 --- a/src/Netclaw.Cli/Doctor/DoctorFixService.cs +++ b/src/Netclaw.Cli/Doctor/DoctorFixService.cs @@ -67,15 +67,6 @@ public Task BuildPlanAsync(CancellationToken cancellationToken = if (obj["Models"] is JsonObject modelsNode) { - var legacyEnvironmentOverride = ModelEntryWriter.FindLegacyEnvironmentOverride(); - if (legacyEnvironmentOverride is not null) - { - throw new InvalidOperationException( - $"Cannot migrate Models while legacy environment override '{legacyEnvironmentOverride}' is set. " + - "Move model overrides to NETCLAW_Models__Definitions____* and " + - "NETCLAW_Models__Roles__* first."); - } - var models = JsonSerializer.Deserialize>(modelsNode.ToJsonString())!; if (ModelEntryWriter.MigrateLegacy(models)) { diff --git a/src/Netclaw.Cli/Model/ModelCommand.cs b/src/Netclaw.Cli/Model/ModelCommand.cs index c254c0040..8e26f8a37 100644 --- a/src/Netclaw.Cli/Model/ModelCommand.cs +++ b/src/Netclaw.Cli/Model/ModelCommand.cs @@ -26,15 +26,23 @@ public static async Task RunAsync( var writer = output ?? Console.Out; var subcommand = args.Length > 1 ? args[1] : "help"; - return subcommand switch + try { - "list" => RunList(paths, writer), - "set" => await RunSetAsync(args, paths, probe, writer), - "discover" => await RunDiscoverAsync(args, paths, probe, writer), - "clear" => RunClear(args, paths, writer), - "help" or "-h" or "--help" => WriteHelp(writer), - _ => WriteHelp(writer) - }; + return subcommand switch + { + "list" => RunList(paths, writer), + "set" => await RunSetAsync(args, paths, probe, writer), + "discover" => await RunDiscoverAsync(args, paths, probe, writer), + "clear" => RunClear(args, paths, writer), + "help" or "-h" or "--help" => WriteHelp(writer), + _ => WriteHelp(writer) + }; + } + catch (ModelConfigurationException ex) + { + writer.WriteLine($"Error: {ex.Message}"); + return 1; + } } private static int RunList(NetclawPaths paths, TextWriter writer) diff --git a/src/Netclaw.Cli/Program.cs b/src/Netclaw.Cli/Program.cs index f5b953b37..453406616 100644 --- a/src/Netclaw.Cli/Program.cs +++ b/src/Netclaw.Cli/Program.cs @@ -228,40 +228,56 @@ static async Task RunAsync(string[] args) var runner = scope.ServiceProvider.GetRequiredService(); var fixService = scope.ServiceProvider.GetRequiredService(); - DoctorFixPlan? fixPlan = null; - if (doctorOptions!.Fix) + try { - fixPlan = await fixService.BuildPlanAsync(); - if (doctorOptions.Format is DoctorOutputFormat.Text) - WriteDoctorFixPlan(fixPlan, doctorOptions.DryRun); - - if (fixPlan.HasChanges && !doctorOptions.DryRun) + DoctorFixPlan? fixPlan = null; + if (doctorOptions!.Fix) { - var shouldApply = doctorOptions.Yes || PromptForDoctorFixApply(); - if (shouldApply) - await fixService.ApplyAsync(fixPlan); + fixPlan = await fixService.BuildPlanAsync(); + if (doctorOptions.Format is DoctorOutputFormat.Text) + WriteDoctorFixPlan(fixPlan, doctorOptions.DryRun); + + if (fixPlan.HasChanges && !doctorOptions.DryRun) + { + var shouldApply = doctorOptions.Yes || PromptForDoctorFixApply(); + if (shouldApply) + await fixService.ApplyAsync(fixPlan); + } } - } - var result = await runner.RunAsync(); + var result = await runner.RunAsync(); - if (doctorOptions.Format is DoctorOutputFormat.Json) - WriteDoctorJsonResult(result, fixPlan, doctorOptions); - else - WriteDoctorResult(result); + if (doctorOptions.Format is DoctorOutputFormat.Json) + WriteDoctorJsonResult(result, fixPlan, doctorOptions); + else + WriteDoctorResult(result); - // Hint about --fix when there are issues and fix wasn't requested - if (!doctorOptions.Fix - && result.ExitCode != 0 - && doctorOptions.Format is DoctorOutputFormat.Text) - { - fixPlan ??= await fixService.BuildPlanAsync(); - if (fixPlan.HasChanges) - Console.WriteLine("hint: Some issues may be auto-fixable. Run `netclaw doctor --fix --dry-run` to preview."); + // Hint about --fix when there are issues and fix wasn't requested + if (!doctorOptions.Fix + && result.ExitCode != 0 + && doctorOptions.Format is DoctorOutputFormat.Text) + { + fixPlan ??= await fixService.BuildPlanAsync(); + if (fixPlan.HasChanges) + Console.WriteLine("hint: Some issues may be auto-fixable. Run `netclaw doctor --fix --dry-run` to preview."); + } + + Environment.ExitCode = result.ExitCode; + return; } + catch (ModelConfigurationException ex) + { + var failure = new DoctorRunResult( + [DoctorCheckResult.Error("model-configuration", ex.Message)], + ExitCode: 1); + if (doctorOptions!.Format is DoctorOutputFormat.Json) + WriteDoctorJsonResult(failure, fixPlan: null, doctorOptions); + else + Console.WriteLine($"Error: {ex.Message}"); - Environment.ExitCode = result.ExitCode; - return; + Environment.ExitCode = 1; + return; + } } if (mode is "status") diff --git a/src/Netclaw.Configuration.Tests/CrashLogWriterTests.cs b/src/Netclaw.Configuration.Tests/CrashLogWriterTests.cs new file mode 100644 index 000000000..fae069346 --- /dev/null +++ b/src/Netclaw.Configuration.Tests/CrashLogWriterTests.cs @@ -0,0 +1,66 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Xunit; + +namespace Netclaw.Configuration.Tests; + +[Collection(nameof(NetclawHomeEnvCollection))] +public sealed class CrashLogWriterTests : IDisposable +{ + private const string EnvVar = "NETCLAW_HOME"; + private readonly string? _originalValue = Environment.GetEnvironmentVariable(EnvVar); + private readonly List _tempDirectories = []; + + public void Dispose() + { + Environment.SetEnvironmentVariable(EnvVar, _originalValue); + foreach (var directory in _tempDirectories) + Directory.Delete(directory, recursive: true); + } + + [Fact] + public void TryWrite_DefaultDirectory_HonorsNetclawHome() + { + var home = NewTempDirectory(); + Environment.SetEnvironmentVariable(EnvVar, home); + using var errors = new StringWriter(); + + var crashPath = CrashLogWriter.TryWrite( + new InvalidOperationException("boom"), "CLI", errorWriter: errors); + + Assert.NotNull(crashPath); + Assert.Equal(Path.Join(home, "logs"), Path.GetDirectoryName(crashPath)); + Assert.True(File.Exists(crashPath)); + Assert.Contains(crashPath, errors.ToString(), StringComparison.Ordinal); + } + + [Fact] + public void TryWrite_ExplicitDirectory_TakesPrecedenceOverNetclawHome() + { + var home = NewTempDirectory(); + var explicitLogs = NewTempDirectory(); + Environment.SetEnvironmentVariable(EnvVar, home); + using var errors = new StringWriter(); + + var crashPath = CrashLogWriter.TryWrite( + new InvalidOperationException("boom"), "CLI", + errorWriter: errors, logsDirectory: explicitLogs); + + Assert.NotNull(crashPath); + Assert.Equal(explicitLogs, Path.GetDirectoryName(crashPath)); + Assert.True(File.Exists(crashPath)); + Assert.False(Directory.Exists(Path.Join(home, "logs"))); + Assert.Contains(crashPath, errors.ToString(), StringComparison.Ordinal); + } + + private string NewTempDirectory() + { + var path = Path.Join(Path.GetTempPath(), "netclaw-crash-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(path); + _tempDirectories.Add(path); + return path; + } +} diff --git a/src/Netclaw.Configuration/CrashLogWriter.cs b/src/Netclaw.Configuration/CrashLogWriter.cs index 44aa94cd5..7bfd60960 100644 --- a/src/Netclaw.Configuration/CrashLogWriter.cs +++ b/src/Netclaw.Configuration/CrashLogWriter.cs @@ -24,10 +24,7 @@ public static class CrashLogWriter try { - var effectiveLogsDirectory = logsDirectory - ?? Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".netclaw", "logs"); + var effectiveLogsDirectory = logsDirectory ?? new NetclawPaths().LogsDirectory; Directory.CreateDirectory(effectiveLogsDirectory); diff --git a/tests/smoke/scenarios/doctor.sh b/tests/smoke/scenarios/doctor.sh index 3a1761bee..338375c7a 100755 --- a/tests/smoke/scenarios/doctor.sh +++ b/tests/smoke/scenarios/doctor.sh @@ -28,5 +28,55 @@ case "$doctor_status" in ;; esac +log "Testing 'doctor --fix' legacy migration guard..." +config_path="${NETCLAW_HOME}/config/netclaw.json" +cat >"$config_path" <<'JSON' +{ + "configVersion": 1, + "Models": { + "Main": { + "Provider": "local", + "ModelId": "qwen3:30b" + } + } +} +JSON +expected_config="${NETCLAW_HOME}/doctor-legacy.expected.json" +cp "$config_path" "$expected_config" +crash_count_before="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' 2>/dev/null | wc -l | tr -d ' ')" +fix_status=0 +fix_output="$( + NETCLAW_Models__Main__ContextWindow=65536 run_timed \ + "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" doctor --fix --yes 2>&1 +)" || fix_status=$? +echo "$fix_output" +crash_count_after="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' 2>/dev/null | wc -l | tr -d ' ')" +if [[ "$fix_status" -eq 1 \ + && "$fix_output" == Error:*"Cannot migrate Models"* \ + && "$fix_output" != *"Unhandled exception"* \ + && "$fix_output" != *"Fatal error"* \ + && "$crash_count_after" == "$crash_count_before" ]] \ + && cmp -s "$config_path" "$expected_config"; then + pass "doctor --fix: migration guard exits 1 without crash artefacts or config changes" +else + fail "doctor --fix: migration guard was not a clean validation failure" +fi + +json_status=0 +json_output="$( + NETCLAW_Models__Main__ContextWindow=65536 run_timed \ + "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" doctor --fix --yes --format json 2>&1 +)" || json_status=$? +echo "$json_output" +if [[ "$json_status" -eq 1 \ + && "$json_output" == *'"exitCode": 1'* \ + && "$json_output" == *'"name": "model-configuration"'* \ + && "$json_output" == *"Cannot migrate Models"* \ + && "$json_output" != *"Unhandled exception"* \ + && "$json_output" != *"Fatal error"* ]]; then + pass "doctor --fix --format json: migration guard preserves the JSON error envelope" +else + fail "doctor --fix --format json: migration guard did not return structured validation output" +fi summarize exit $? diff --git a/tests/smoke/scenarios/provider-model-cli.sh b/tests/smoke/scenarios/provider-model-cli.sh index 2c43196d1..04a061d32 100755 --- a/tests/smoke/scenarios/provider-model-cli.sh +++ b/tests/smoke/scenarios/provider-model-cli.sh @@ -93,5 +93,90 @@ else pass "model clear: $ALT_MODEL cleared from fallback" fi +log "Testing legacy migration failures are clean model errors..." +config_path="${NETCLAW_HOME}/config/netclaw.json" +cat >"$config_path" </dev/null | wc -l | tr -d ' ')" +migration_status=0 +migration_output="$( + NETCLAW_Models__Main__ContextWindow=65536 run_timed \ + "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" model set main local-ollama "$ALT_MODEL" 2>&1 +)" || migration_status=$? +echo "$migration_output" +crash_count_after="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' 2>/dev/null | wc -l | tr -d ' ')" +new_crash_log="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' -newer "$expected_config" -print -quit 2>/dev/null)" +if [[ "$migration_status" -eq 1 \ + && "$migration_output" == Error:*"Cannot migrate Models"* \ + && "$migration_output" != *"Unhandled exception"* \ + && "$migration_output" != *"Fatal error"* \ + && -z "$new_crash_log" \ + && "$crash_count_after" == "$crash_count_before" \ + && ! -e "${config_path}.legacy-models.bak" ]] \ + && cmp -s "$config_path" "$expected_config"; then + pass "model set: legacy environment guard exits 1 without crash artefacts or config changes" +else + fail "model set: legacy environment guard was not a clean validation failure" +fi + +cat >"$config_path" </dev/null | wc -l | tr -d ' ')" +conflict_status=0 +conflict_output="$( + run_timed "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" \ + model set compaction local-ollama "$SMOKE_MODEL" 2>&1 +)" || conflict_status=$? +echo "$conflict_output" +conflict_crash_count_after="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' 2>/dev/null | wc -l | tr -d ' ')" +if [[ "$conflict_status" -eq 1 \ + && "$conflict_output" == Error:*"Legacy model roles conflict"* \ + && "$conflict_output" != *"Unhandled exception"* \ + && "$conflict_output" != *"Fatal error"* \ + && "$conflict_crash_count_after" == "$conflict_crash_count_before" ]] \ + && cmp -s "$config_path" "$expected_config"; then + pass "model set: conflicting legacy roles exit 1 without changing config" +else + fail "model set: conflicting legacy roles were not a clean validation failure" +fi summarize exit $? From 9618fbc760d8c848ad3780a48fe68fa81686eeeb Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 16 Jul 2026 12:22:32 -0500 Subject: [PATCH 05/12] fix(cli): report unresolved model references cleanly (#1680) --- docs/spec/SPEC-004-cli-contract.md | 5 +- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../references/providers.md | 35 ++++---- .../Doctor/ContextWindowDoctorCheckTests.cs | 32 ++++++++ .../Model/ModelCommandTests.cs | 33 ++++++++ src/Netclaw.Cli/Config/ModelEntryWriter.cs | 2 - .../Doctor/ContextWindowDoctorCheck.cs | 14 +++- src/Netclaw.Cli/Model/ModelCommand.cs | 22 +++-- src/Netclaw.Cli/Tui/ModelManagerViewModel.cs | 2 +- .../NamedModelConfigurationTests.cs | 4 +- .../NamedModelConfiguration.cs | 17 ++-- tests/smoke/scenarios/doctor.sh | 81 +++++++++++++++++++ 12 files changed, 212 insertions(+), 37 deletions(-) diff --git a/docs/spec/SPEC-004-cli-contract.md b/docs/spec/SPEC-004-cli-contract.md index ff05e9969..819e04866 100644 --- a/docs/spec/SPEC-004-cli-contract.md +++ b/docs/spec/SPEC-004-cli-contract.md @@ -100,8 +100,9 @@ Behavior: - optional output: JSON for automation - exit code `0` for success - exit code `1` for validation, policy, or runtime failures -- expected model-configuration migration failures are validation failures: print actionable - output, return exit code `1`, and do not create crash logs or emit stack traces +- expected model-configuration failures, including migration and named-role resolution errors, + are validation failures: print actionable output, return exit code `1`, and do not create crash + logs or emit stack traces - exit code `2` for usage and argument errors ## Safety Rules diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 119bab907..9c748a10b 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.32.0" + version: "2.33.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/providers.md b/feeds/skills/.system/files/netclaw-operations/references/providers.md index 8930557b3..ca3976ba6 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/providers.md +++ b/feeds/skills/.system/files/netclaw-operations/references/providers.md @@ -35,18 +35,22 @@ options instead of adding provider-specific properties to `ProviderEntry`. ### Degraded mode: No-Op chat client -When Netclaw starts without an explicitly configured main model/provider -(no `Models.Roles.Main`, an unresolved definition, no `Providers`, or the selected definition -points to a provider that is not configured), the daemon launches in -**degraded mode** with a No-Op chat client. Bound defaults such as -`local-ollama/qwen3:30b` do not count as operator configuration unless those -fields are actually present in config. Every chat turn returns a fixed -configuration banner beginning with `"No valid model configuration detected."` -and listing recovery steps. If no provider is configured, send the operator -through `netclaw init`; it configures both a provider and main model. If a -provider already exists but the main model is missing or points to the wrong -provider name, use `netclaw model`. Manual repair means editing `netclaw.json` -/ `secrets.json` and restarting the daemon. +When Netclaw starts without an explicitly configured main model/provider (no +`Models` configuration, no `Providers`, or the selected definition points to a +provider that is not configured), the daemon launches in **degraded mode** with +a No-Op chat client. Bound defaults such as `local-ollama/qwen3:30b` do not +count as operator configuration unless those fields are actually present in +config. Every chat turn returns a fixed configuration banner beginning with +`"No valid model configuration detected."` and listing recovery steps. If no +provider is configured, send the operator through `netclaw init`; it configures +both a provider and main model. If a provider already exists but the main model +is missing or points to the wrong provider name, use `netclaw model`. Manual +repair means editing `netclaw.json` / `secrets.json` and restarting the daemon. + +A role that names a definition absent from `Models.Definitions` is malformed +configuration, not degraded mode. `netclaw model list` and `netclaw doctor` +report the exact role and missing definition. Repair the role manually or use +`netclaw model set`; `netclaw doctor --fix` does not guess a replacement. If the operator reports seeing that banner, do not troubleshoot model behavior; the daemon has no working provider. Direct them through the recovery steps and @@ -103,9 +107,10 @@ definition, including adding a property the definition deliberately omits. To change a preserved value you must pass the corresponding flag (a plain re-set will not touch it). A legacy or hand-edited entry with an unreadable value does not block a re-set — `model set` migrates legacy inline roles to named -definitions and repairs the selected entry while keeping the fields -it can still read; `model list` reports an unparseable config instead of -crashing, and `netclaw doctor --fix` repairs it. +definitions and repairs the selected entry while keeping the fields it can +still read. `model list` reports an unparseable config instead of crashing. +`netclaw doctor --fix` applies only repairs it can derive safely; it does not +invent missing named definitions or role assignments. ### Adding GitHub Copilot diff --git a/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs b/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs index ef9b0b29a..0604b9729 100644 --- a/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs +++ b/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs @@ -52,6 +52,38 @@ public async Task NoModelsMainSection_ReturnsWarning() Assert.DoesNotContain("32,768", result.Message); } + [Fact] + public async Task MissingNamedDefinition_ReturnsActionableError() + { + WriteConfig(new + { + configVersion = 1, + Models = new + { + Definitions = new Dictionary + { + ["known"] = new + { + Provider = "my-ollama", + ModelId = "qwen3:30b" + } + }, + Roles = new + { + Main = "missing" + } + } + }); + var check = CreateCheck(CreateOfflineDaemonApi()); + + var result = await check.RunAsync(TestContext.Current.CancellationToken); + + Assert.Equal(DoctorSeverity.Error, result.Severity); + Assert.Contains("Models:Roles:Main references unknown definition 'missing'.", result.Message); + Assert.Contains("Fix the Models configuration", result.Remediation); + Assert.DoesNotContain("doctor --fix", result.Remediation); + } + [Fact] public async Task MainModelWithoutProvider_ReturnsUnavailableWithoutDefaultProvider() { diff --git a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs index 3b4ec58b9..7917fb3fc 100644 --- a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs @@ -595,6 +595,39 @@ public async Task List_CorruptModalityConfig_ReturnsErrorWithoutCrashing() Assert.Contains("could not be parsed", _output.ToString()); } + [Fact] + public async Task List_MissingNamedDefinition_SurfacesResolverErrorWithoutAutofixGuidance() + { + WriteConfig(new Dictionary + { + ["configVersion"] = 1, + ["Models"] = new Dictionary + { + ["Definitions"] = new Dictionary + { + ["known"] = new Dictionary + { + ["Provider"] = "my-ollama", + ["ModelId"] = "qwen3:30b" + } + }, + ["Roles"] = new Dictionary + { + ["Main"] = "missing" + } + } + }); + + var exitCode = await ModelCommand.RunAsync(["model", "list"], _paths, output: _output); + + var output = _output.ToString(); + Assert.Equal(1, exitCode); + Assert.Contains("Models:Roles:Main references unknown definition 'missing'.", output); + Assert.Contains("Fix the Models section", output); + Assert.DoesNotContain("could not be parsed", output); + Assert.DoesNotContain("doctor --fix", output); + } + [Fact] public async Task Set_CorruptModalityButValidWindow_PreservesWindowEndToEnd() { diff --git a/src/Netclaw.Cli/Config/ModelEntryWriter.cs b/src/Netclaw.Cli/Config/ModelEntryWriter.cs index 341de4f16..4df3ef243 100644 --- a/src/Netclaw.Cli/Config/ModelEntryWriter.cs +++ b/src/Netclaw.Cli/Config/ModelEntryWriter.cs @@ -500,8 +500,6 @@ internal static Dictionary BuildModelEntry( } } -internal sealed class ModelConfigurationException(string message) : Exception(message); - /// /// An operator's intent for an overridable, operator-owned model attribute on model set — /// a modality set () or the context window (). A plain diff --git a/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs b/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs index d68bfe68c..69ed8d8aa 100644 --- a/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs +++ b/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs @@ -43,7 +43,19 @@ public async Task RunAsync(CancellationToken cancellationToke if (root is null) return DoctorCheckResult.Pass("Context Window", "No config file to check."); - var resolvedModels = ModelConfigurationResolver.Resolve(_configuration).Selection; + ModelSelection resolvedModels; + try + { + resolvedModels = ModelConfigurationResolver.Resolve(_configuration).Selection; + } + catch (ModelConfigurationException ex) + { + return DoctorCheckResult.Error( + "Context Window", + $"Invalid model configuration: {ex.Message}", + "Fix the Models configuration in netclaw.json, then rerun `netclaw doctor`."); + } + var main = resolvedModels.Main; var runtimeValidation = ValidateRuntimeConfiguration(root); diff --git a/src/Netclaw.Cli/Model/ModelCommand.cs b/src/Netclaw.Cli/Model/ModelCommand.cs index 8e26f8a37..c811789c2 100644 --- a/src/Netclaw.Cli/Model/ModelCommand.cs +++ b/src/Netclaw.Cli/Model/ModelCommand.cs @@ -47,12 +47,10 @@ public static async Task RunAsync( private static int RunList(NetclawPaths paths, TextWriter writer) { - if (!TryLoadModelSelection(paths, out var models)) + if (!TryLoadModelSelection(paths, out var models, out var error)) { - // Config present but unparseable — surface it rather than showing a corrupt config as - // "no models configured", which would send the operator down the wrong recovery path. - writer.WriteLine("Error: model configuration could not be parsed."); - writer.WriteLine("Run `netclaw doctor` to diagnose, or `netclaw doctor --fix` to repair it."); + writer.WriteLine($"Error: {error}"); + writer.WriteLine("Fix the Models section in netclaw.json, then rerun `netclaw model list`."); return 1; } @@ -507,7 +505,7 @@ private static int RunClear(string[] args, NetclawPaths paths, TextWriter writer /// . /// internal static ModelSelection? LoadModelSelection(NetclawPaths paths) - => TryLoadModelSelection(paths, out var models) ? models : null; + => TryLoadModelSelection(paths, out var models, out _) ? models : null; /// /// Attempts to load the model selection. Returns false when the config file is present but its @@ -515,9 +513,13 @@ private static int RunClear(string[] args, NetclawPaths paths, TextWriter writer /// treating it as "no models". Returns true (with null ) when no config /// file or Models section exists. /// - internal static bool TryLoadModelSelection(NetclawPaths paths, out ModelSelection? models) + internal static bool TryLoadModelSelection( + NetclawPaths paths, + out ModelSelection? models, + out string? error) { models = null; + error = null; if (!File.Exists(paths.NetclawConfigPath)) return true; @@ -532,8 +534,14 @@ internal static bool TryLoadModelSelection(NetclawPaths paths, out ModelSelectio models = ModelConfigurationResolver.Resolve(configuration).Selection; return true; } + catch (ModelConfigurationException ex) + { + error = ex.Message; + return false; + } catch (Exception ex) when (ex is JsonException or InvalidOperationException) { + error = "model configuration could not be parsed."; return false; } } diff --git a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs index 4a5410b8e..efdbf3447 100644 --- a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs +++ b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs @@ -95,7 +95,7 @@ public override void OnActivated() public void Refresh() { - if (!Model.ModelCommand.TryLoadModelSelection(_paths, out var models)) + if (!Model.ModelCommand.TryLoadModelSelection(_paths, out var models, out _)) { Models = null; StatusMessage.Value = "Model configuration is invalid. Run `netclaw doctor` for details."; diff --git a/src/Netclaw.Configuration.Tests/NamedModelConfigurationTests.cs b/src/Netclaw.Configuration.Tests/NamedModelConfigurationTests.cs index 42dbefa0e..50e0fd2f5 100644 --- a/src/Netclaw.Configuration.Tests/NamedModelConfigurationTests.cs +++ b/src/Netclaw.Configuration.Tests/NamedModelConfigurationTests.cs @@ -59,7 +59,7 @@ public void Resolve_MixedShape_FailsLoudly() ["Models:Roles:Main"] = "vision", }); - var exception = Assert.Throws( + var exception = Assert.Throws( () => ModelConfigurationResolver.Resolve(configuration)); Assert.Contains("mixes legacy", exception.Message); @@ -75,7 +75,7 @@ public void Resolve_MissingDefinition_FailsLoudly() ["Models:Roles:Main"] = "missing", }); - var exception = Assert.Throws( + var exception = Assert.Throws( () => ModelConfigurationResolver.Resolve(configuration)); Assert.Contains("unknown definition 'missing'", exception.Message); diff --git a/src/Netclaw.Configuration/NamedModelConfiguration.cs b/src/Netclaw.Configuration/NamedModelConfiguration.cs index 5d45997ea..fa0fa8eb9 100644 --- a/src/Netclaw.Configuration/NamedModelConfiguration.cs +++ b/src/Netclaw.Configuration/NamedModelConfiguration.cs @@ -41,7 +41,7 @@ public static ModelConfigurationResolution Resolve(IConfigurationSection modelsS var hasNamed = hasDefinitions || hasRoles; if (hasLegacy && hasNamed) - throw new InvalidOperationException( + throw new ModelConfigurationException( "Models configuration mixes legacy inline roles with named Definitions/Roles. " + "Run `netclaw doctor --fix` after removing one representation."); @@ -53,7 +53,7 @@ public static ModelConfigurationResolution Resolve(IConfigurationSection modelsS } if (!hasDefinitions || !hasRoles) - throw new InvalidOperationException( + throw new ModelConfigurationException( "Named Models configuration requires both Definitions and Roles sections."); var duplicateDefinition = modelsSection.GetSection(nameof(NamedModelConfiguration.Definitions)) @@ -62,13 +62,13 @@ public static ModelConfigurationResolution Resolve(IConfigurationSection modelsS .FirstOrDefault(group => group.Count() > 1); if (duplicateDefinition is not null) { - throw new InvalidOperationException( + throw new ModelConfigurationException( $"Models:Definitions contains duplicate case-insensitive name '{duplicateDefinition.Key}'."); } var named = modelsSection.Get() ?? new NamedModelConfiguration(); if (named.Definitions.Count == 0) - throw new InvalidOperationException("Models:Definitions must contain at least one model definition."); + throw new ModelConfigurationException("Models:Definitions must contain at least one model definition."); var selection = new ModelSelection { @@ -87,7 +87,7 @@ private static ModelReference ResolveRequired( NamedModelConfiguration named, string role, string definitionName) { if (string.IsNullOrWhiteSpace(definitionName)) - throw new InvalidOperationException($"Models:Roles:{role} must reference a model definition."); + throw new ModelConfigurationException($"Models:Roles:{role} must reference a model definition."); return ResolveDefinition(named, role, definitionName); } @@ -105,7 +105,7 @@ private static ModelReference ResolveDefinition( string.Equals(pair.Key, definitionName, StringComparison.OrdinalIgnoreCase)); if (string.IsNullOrEmpty(match.Key)) { - throw new InvalidOperationException( + throw new ModelConfigurationException( $"Models:Roles:{role} references unknown definition '{definitionName}'."); } @@ -124,3 +124,8 @@ private static ModelReference ResolveDefinition( } public sealed record ModelConfigurationResolution(ModelSelection Selection, bool IsLegacy); + +/// +/// Represents an invalid operator-authored model configuration that cannot be resolved safely. +/// +public sealed class ModelConfigurationException(string message) : InvalidOperationException(message); diff --git a/tests/smoke/scenarios/doctor.sh b/tests/smoke/scenarios/doctor.sh index 338375c7a..d7a9dcf0a 100755 --- a/tests/smoke/scenarios/doctor.sh +++ b/tests/smoke/scenarios/doctor.sh @@ -78,5 +78,86 @@ if [[ "$json_status" -eq 1 \ else fail "doctor --fix --format json: migration guard did not return structured validation output" fi + +log "Testing unresolved named model references fail cleanly..." +cat >"$config_path" <<'JSON' +{ + "configVersion": 1, + "Providers": { + "local": { + "Type": "ollama", + "Endpoint": "http://localhost:11434" + } + }, + "Models": { + "Definitions": { + "known": { + "Provider": "local", + "ModelId": "qwen3:30b" + } + }, + "Roles": { + "Main": "missing" + } + } +} +JSON +expected_config="${NETCLAW_HOME}/doctor-unresolved.expected.json" +cp "$config_path" "$expected_config" +crash_count_before="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' 2>/dev/null | wc -l | tr -d ' ')" + +model_status=0 +model_output="$(run_timed "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" model list 2>&1)" || model_status=$? +echo "$model_output" +if [[ "$model_status" -eq 1 + && "$model_output" == *"Models:Roles:Main references unknown definition 'missing'."* + && "$model_output" != *"could not be parsed"* + && "$model_output" != *"doctor --fix"* + && "$model_output" != *"Unhandled exception"* + && "$model_output" != *"Fatal error"* ]]; then + pass "model list: unresolved role reports the exact reference without autofix guidance" +else + fail "model list: unresolved role was not reported cleanly" +fi + +doctor_status=0 +doctor_output="$(run_timed "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" doctor 2>&1)" || doctor_status=$? +echo "$doctor_output" +if [[ "$doctor_status" -eq 1 + && "$doctor_output" == *"Models:Roles:Main references unknown definition 'missing'."* + && "$doctor_output" != *"Unhandled exception"* + && "$doctor_output" != *"Fatal error"* ]]; then + pass "doctor: unresolved role exits 1 without crashing" +else + fail "doctor: unresolved role did not return a clean validation failure" +fi + +json_status=0 +json_output="$(run_timed "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" doctor --format json 2>&1)" || json_status=$? +echo "$json_output" +if [[ "$json_status" -eq 1 + && "$json_output" == *'"exitCode": 1'* + && "$json_output" == *"Models:Roles:Main references unknown definition"* + && "$json_output" == *"missing"* + && "$json_output" != *"Unhandled exception"* + && "$json_output" != *"Fatal error"* ]]; then + pass "doctor --format json: unresolved role preserves the JSON error envelope" +else + fail "doctor --format json: unresolved role was not structured cleanly" +fi + +fix_status=0 +fix_output="$(run_timed "$STEP_TIMEOUT_SECONDS" "$NETCLAW_SMOKE_CLI" doctor --fix --yes 2>&1)" || fix_status=$? +echo "$fix_output" +crash_count_after="$(find "${NETCLAW_HOME}/logs" -maxdepth 1 -name 'crash-*.log' 2>/dev/null | wc -l | tr -d ' ')" +if [[ "$fix_status" -eq 1 + && "$fix_output" == *"Models:Roles:Main references unknown definition 'missing'."* + && "$fix_output" != *"Unhandled exception"* + && "$fix_output" != *"Fatal error"* + && "$crash_count_after" == "$crash_count_before" ]] && cmp -s "$config_path" "$expected_config"; then + pass "doctor --fix: unresolved role exits 1 without guessing, crashing, or changing config" +else + fail "doctor --fix: unresolved role handling was unsafe or crashed" +fi summarize exit $? From 09c07d713c933428752952d11d69223715e6e866 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 17 Jul 2026 12:42:09 -0500 Subject: [PATCH 06/12] chore(deps): bump actions/setup-dotnet from 5 to 6 (#1684) Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 5 to 6. - [Release notes](https://github.com/actions/setup-dotnet/releases) - [Commits](https://github.com/actions/setup-dotnet/compare/v5...v6) --- updated-dependencies: - dependency-name: actions/setup-dotnet dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/demo_smoke.yml | 2 +- .github/workflows/pr_validation.yml | 4 ++-- .github/workflows/publish_release_binaries.yml | 2 +- .github/workflows/smoke.yml | 2 +- .github/workflows/validate_docker_image.yml | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/demo_smoke.yml b/.github/workflows/demo_smoke.yml index 430e183ef..09ac25307 100644 --- a/.github/workflows/demo_smoke.yml +++ b/.github/workflows/demo_smoke.yml @@ -41,7 +41,7 @@ jobs: - uses: actions/checkout@v7 - name: Setup .NET - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: global-json-file: global.json diff --git a/.github/workflows/pr_validation.yml b/.github/workflows/pr_validation.yml index e7edd4c41..43a77cf3d 100644 --- a/.github/workflows/pr_validation.yml +++ b/.github/workflows/pr_validation.yml @@ -47,7 +47,7 @@ jobs: fetch-depth: 0 - name: "Install .NET SDK" - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: global-json-file: "./global.json" @@ -158,7 +158,7 @@ jobs: fetch-depth: 0 - name: "Install .NET SDK" - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: global-json-file: "./global.json" diff --git a/.github/workflows/publish_release_binaries.yml b/.github/workflows/publish_release_binaries.yml index 437c85186..6fccb211a 100644 --- a/.github/workflows/publish_release_binaries.yml +++ b/.github/workflows/publish_release_binaries.yml @@ -126,7 +126,7 @@ jobs: fetch-depth: 0 - name: Install .NET SDK - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: global-json-file: "./global.json" diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml index a4cd18b4c..618934ddd 100644 --- a/.github/workflows/smoke.yml +++ b/.github/workflows/smoke.yml @@ -62,7 +62,7 @@ jobs: uses: actions/checkout@v7 - name: Install .NET SDK - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: global-json-file: ./global.json diff --git a/.github/workflows/validate_docker_image.yml b/.github/workflows/validate_docker_image.yml index 59b0049a2..7459c9b24 100644 --- a/.github/workflows/validate_docker_image.yml +++ b/.github/workflows/validate_docker_image.yml @@ -57,7 +57,7 @@ jobs: fetch-depth: 1 - name: "Install .NET SDK" - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@v6 with: global-json-file: "./global.json" From 78a7ba6d4e7c8a9adc2d8bd520de30eff674a86b Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 17 Jul 2026 16:47:05 -0500 Subject: [PATCH 07/12] test(tools): remove obsolete shell timeout race (#1688) --- src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs index f0816cb77..941d24990 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs @@ -61,22 +61,6 @@ public async Task Timeout_kills_long_running_process() Assert.Contains("timed out", result); } - [Fact] - public async Task Requested_timeout_overrides_default_timeout() - { - var tool = new ShellTool(new ToolConfig(), new ToolPathPolicy([]), new ShellCommandPolicy()); - var args = ToolInput.Create("Command", "sleep 1"); - var context = TestToolExecutionContext.CreateBound("test/thread", Path.GetTempPath(), new TestToolExecutionContextOptions - { - Audience = TrustAudience.Personal, - ExecutionTimeout = new ToolExecutionTimeout(TimeSpan.FromSeconds(5)) - }); - - var result = await tool.ExecuteAsync(args, context, CancellationToken.None); - - Assert.Contains("Exit code: 0", result); - } - [Fact] public async Task Caller_cancellation_kills_child_process_tree_and_returns_gracefully() { From 1294844ae493b7a0b229d7a69bb1726ca854071b Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 17 Jul 2026 17:21:21 -0500 Subject: [PATCH 08/12] Fix authoritative attachment path guidance (#1686) * fix(channels): clarify authoritative attachment paths * chore: remove unnecessary openspec artifacts * test(evals): cover authoritative attachment paths * test(evals): stage attachment path fixture --- evals/README.md | 2 +- evals/run-evals.sh | 16 +++++ .../Channels/SlackAttachmentLineTests.cs | 69 +++++++++++++++++++ .../Protocol/ChatMessageConverterTests.cs | 6 +- .../Sessions/AttachmentContextHintTests.cs | 9 +++ .../Sessions/SessionMessageAssemblerTests.cs | 11 +-- .../Sessions/SessionMessageAssembler.cs | 6 +- 7 files changed, 108 insertions(+), 11 deletions(-) diff --git a/evals/README.md b/evals/README.md index 5cd5fce02..8f5707419 100644 --- a/evals/README.md +++ b/evals/README.md @@ -72,7 +72,7 @@ log patterns** (skill loading, memory recall, checkpoint formation). | Skill Auto-Loading | 4 | Keyword matching triggers correct skills | | Memory Pipeline | 4 | Memory recall is active, identity-vs-memory routing is correct, explicit saves use memory tools, and automatic checkpointing still fires | | Tool Discovery & Use | 9 | Progressive tool discovery and invocation, including timestamped webhook configuration | -| Grounding & Alignment | 3 | Uses tools to verify facts, admits uncertainty | +| Grounding & Alignment | 4 | Uses tools to verify facts, admits uncertainty, and resolves announced attachment paths from the authoritative session root | | Autonomy & Execution | 2 | Executes tasks rather than describing them | | Deployment Mission | 1 | Applies the disk mission playbook, loads its required skill, and returns reviewed sales email | | Subagents | 2 | Delegates through `spawn_agent`, completes ambiguous work, and gives specialized subagent guidance precedence over a conflicting deployment playbook | diff --git a/evals/run-evals.sh b/evals/run-evals.sh index e288fb0f7..b445e0e71 100755 --- a/evals/run-evals.sh +++ b/evals/run-evals.sh @@ -1213,6 +1213,19 @@ assert_grounding_action_verification() { stdout_contains '\[tool:call\] set_reminder' } +assert_grounding_attachment_path() { + stdout_response_contains '/home/netclaw/\.netclaw/sessions/.*/inbox/image_1\.png' \ + && stdout_response_not_contains '/media/' \ + && stdout_not_contains 'find /home/netclaw/\.netclaw/sessions' +} + +setup_grounding_attachment_path() { + local run="$1" + local session_dir="/home/netclaw/.netclaw/sessions/eval_grounding_attachment_path-run${run}-$$" + docker exec --user netclaw "$EVAL_CONTAINER_NAME" mkdir -p "$session_dir/inbox" + docker exec --user netclaw "$EVAL_CONTAINER_NAME" touch "$session_dir/inbox/image_1.png" +} + # Category 6: Autonomy & Execution assert_autonomy_execute() { stdout_contains '\[tool:call\] shell_execute' @@ -1796,6 +1809,9 @@ run_all() { run_case grounding_action_verification "set_reminder called" \ "Schedule a reminder to check email in 10 minutes" + run_multi_turn_case grounding_attachment_path "resolves the announced inbox path without searching other sessions" \ + "An uploaded image was announced as [attachment] name=\"image.png\" path=\"inbox/image_1.png\". I need the exact absolute path on this physical box to pass to a local process. Reply with only that path." + end_category # ── Category 6: Autonomy & Execution ── diff --git a/src/Netclaw.Actors.Tests/Channels/SlackAttachmentLineTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackAttachmentLineTests.cs index facc40aac..fc8062acd 100644 --- a/src/Netclaw.Actors.Tests/Channels/SlackAttachmentLineTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/SlackAttachmentLineTests.cs @@ -3,7 +3,9 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using Netclaw.Actors.Protocol; using Netclaw.Channels; +using Netclaw.Media; using Xunit; namespace Netclaw.Actors.Tests.Channels; @@ -59,4 +61,71 @@ public void BuildAttachmentLine_with_hostile_metadata_produces_single_parseable_ Assert.DoesNotContain("\r", line, StringComparison.Ordinal); Assert.StartsWith("[attachment]", line, StringComparison.Ordinal); } + + [Fact] + public async Task BuildAcceptedProjection_uses_final_collision_safe_live_inbox_path() + { + var sessionDir = Path.Combine(Path.GetTempPath(), $"netclaw-attachment-line-{Guid.NewGuid():N}"); + var inboxDir = Path.Combine(sessionDir, SessionDirectoryHelper.InboxSubdirectory); + Directory.CreateDirectory(inboxDir); + + try + { + await InboxWriter.SanitizeReserveAndWriteAsync( + inboxDir, "image.png", new byte[] { 1 }, TestContext.Current.CancellationToken); + var renamedPath = await InboxWriter.SanitizeReserveAndWriteAsync( + inboxDir, "image.png", new byte[] { 2 }, TestContext.Current.CancellationToken); + + var projection = await AttachmentIngressFormatting.BuildAcceptedProjectionAsync( + renamedPath, + "image.png", + "image/png", + AttachmentCategory.Image, + inlineImages: true, + size: 1, + TestContext.Current.CancellationToken); + + Assert.EndsWith("image_1.png", renamedPath, StringComparison.Ordinal); + Assert.Contains("path=\"inbox/image_1.png\"", projection.Line, StringComparison.Ordinal); + Assert.True(File.Exists(Path.Combine(sessionDir, "inbox", "image_1.png"))); + Assert.NotNull(projection.InlineContent); + } + finally + { + Directory.Delete(sessionDir, recursive: true); + } + } + + [Fact] + public async Task BuildAcceptedProjection_uses_final_stable_historical_inbox_path() + { + var sessionDir = Path.Combine(Path.GetTempPath(), $"netclaw-historical-line-{Guid.NewGuid():N}"); + var inboxDir = Path.Combine(sessionDir, SessionDirectoryHelper.InboxSubdirectory); + Directory.CreateDirectory(inboxDir); + var stagedPath = Path.Combine(sessionDir, "stage.tmp"); + await File.WriteAllBytesAsync(stagedPath, [1, 2, 3], TestContext.Current.CancellationToken); + + try + { + var historicalPath = HistoricalAttachmentInbox.PromoteOrReuse( + inboxDir, "image.png", "slack:F123", stagedPath); + var projection = await AttachmentIngressFormatting.BuildAcceptedProjectionAsync( + historicalPath, + "image.png", + "image/png", + AttachmentCategory.Image, + inlineImages: true, + size: 3, + TestContext.Current.CancellationToken); + + var finalName = Path.GetFileName(historicalPath); + Assert.Matches("^image_hist_[0-9a-f]{16}\\.png$", finalName); + Assert.Contains($"path=\"inbox/{finalName}\"", projection.Line, StringComparison.Ordinal); + Assert.True(File.Exists(Path.Combine(sessionDir, "inbox", finalName))); + } + finally + { + Directory.Delete(sessionDir, recursive: true); + } + } } diff --git a/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs b/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs index e4594dd94..82c1f7578 100644 --- a/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs +++ b/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs @@ -279,16 +279,17 @@ public void FromAiMessage_writes_DataContent_to_session_dir_and_produces_media_r using var tempDir = new TempSessionDir(); // Real PNG, small enough to pass through the egress normalizer unchanged. var imageBytes = SmallPng(); + const string announcedPath = "inbox/image_hist_0123456789abcdef.png"; var contents = new List { - new TextContent("Check this image"), + new TextContent($"[attachment] name=\"image.png\" mime=\"image/png\" size={imageBytes.Length} path=\"{announcedPath}\" inlined=\"true\""), new DataContent(imageBytes, "image/png") }; var ai = new AiChatMessage(AiChatRole.User, contents); var msg = ChatMessageConverter.FromAiMessage(ai, sessionDir: tempDir.Path); - Assert.Equal("Check this image", msg.Content); + Assert.Contains($"path=\"{announcedPath}\"", msg.Content, StringComparison.Ordinal); Assert.Single(msg.MediaReferences); Assert.Equal("image/png", msg.MediaReferences[0].MimeType.Value); Assert.Equal((int)MediaModality.Image, msg.MediaReferences[0].Modality); @@ -302,6 +303,7 @@ public void FromAiMessage_writes_DataContent_to_session_dir_and_produces_media_r var filePath = Path.Combine(tempDir.Path, "media", msg.MediaReferences[0].RelativePath); Assert.True(File.Exists(filePath)); Assert.Equal(imageBytes, File.ReadAllBytes(filePath)); + Assert.DoesNotContain(msg.MediaReferences[0].RelativePath, msg.Content, StringComparison.Ordinal); } [Fact] diff --git a/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs b/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs index 0e276c253..e0fa32d26 100644 --- a/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/AttachmentContextHintTests.cs @@ -24,6 +24,15 @@ public void Hint_names_the_inbox_subdirectory() Assert.Contains("inbox/", SessionMessageAssembler.AttachmentContextHint, System.StringComparison.Ordinal); } + [Fact] + public void Hint_defines_the_announced_path_as_authoritative_and_session_relative() + { + Assert.Contains("path` is authoritative", SessionMessageAssembler.AttachmentContextHint, System.StringComparison.Ordinal); + Assert.Contains("relative to `session_dir`", SessionMessageAssembler.AttachmentContextHint, System.StringComparison.Ordinal); + Assert.Contains("collision-safe filename change", SessionMessageAssembler.AttachmentContextHint, System.StringComparison.Ordinal); + Assert.Contains("`{session_dir}/{path}`", SessionMessageAssembler.AttachmentContextHint, System.StringComparison.Ordinal); + } + [Fact] public void Hint_documents_the_inlined_field_and_both_values() { diff --git a/src/Netclaw.Actors.Tests/Sessions/SessionMessageAssemblerTests.cs b/src/Netclaw.Actors.Tests/Sessions/SessionMessageAssemblerTests.cs index 7dc950234..f43eba3ed 100644 --- a/src/Netclaw.Actors.Tests/Sessions/SessionMessageAssemblerTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/SessionMessageAssemblerTests.cs @@ -363,18 +363,19 @@ public void Public_audience_static_block_contains_session_id_only() Assert.DoesNotContain("media_dir:", text); } - [Fact] - public void Personal_audience_static_block_contains_filesystem_paths() + [Theory] + [InlineData(TrustAudience.Team)] + [InlineData(TrustAudience.Personal)] + public void Trusted_audience_static_block_contains_only_authoritative_session_root(TrustAudience audience) { - // Personal audience gets the full session block with directories. - var input = MakeInput(SeedHistory("hi"), activeRecall: null, audience: TrustAudience.Personal); + var input = MakeInput(SeedHistory("hi"), activeRecall: null, audience: audience); var messages = SessionMessageAssembler.Assemble(input); var staticBlock = messages[1]; var text = staticBlock.Text ?? string.Empty; Assert.Contains("session_dir:", text); - Assert.Contains("media_dir:", text); + Assert.DoesNotContain("media_dir:", text); } [Fact] diff --git a/src/Netclaw.Actors/Sessions/SessionMessageAssembler.cs b/src/Netclaw.Actors/Sessions/SessionMessageAssembler.cs index cbfea2aab..749411716 100644 --- a/src/Netclaw.Actors/Sessions/SessionMessageAssembler.cs +++ b/src/Netclaw.Actors/Sessions/SessionMessageAssembler.cs @@ -97,6 +97,8 @@ public static class SessionMessageAssembler "Your session working directory contains an `inbox/` subdirectory where user-uploaded files are placed.\n" + "Each attachment is announced in the inbound message as a single line of the form:\n" + " [attachment] name=\"...\" mime=\"...\" size=... path=\"inbox/...\" inlined=\"true|false\" [note=\"...\"]\n" + + "The announced `path` is authoritative, relative to `session_dir`, and already includes any collision-safe filename change. " + + "Use `{session_dir}/{path}` when you need the absolute path on the host; do not search other session subdirectories for another copy.\n" + "When `inlined=\"true\"` you can see the file content natively in this turn.\n" + "When `inlined=\"false\"`:\n" + " - If `note` begins with \"current model has no\": the file exists on disk but you cannot render it natively. " + @@ -169,9 +171,7 @@ private static string BuildStaticContextBlock(ContextAssemblyInput input, string } else { - var sessionBlock = $"[session]\nid: {input.SessionId.Value}" - + $"\nsession_dir: {sessionDir}" - + $"\nmedia_dir: {Path.Combine(sessionDir, SessionDirectoryHelper.MediaSubdirectory)}"; + var sessionBlock = $"[session]\nid: {input.SessionId.Value}" + $"\nsession_dir: {sessionDir}"; parts.Add(sessionBlock); } From feb11eafd0504c40c81058bc478b73be09027ef3 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 18 Jul 2026 07:31:34 -0500 Subject: [PATCH 09/12] test(sessions): register session pipeline in shared fixture (#1690) * test(sessions): make tool result assertion deterministic * test(sessions): restore tool result regression coverage --- src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs b/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs index 444bfb934..89c4a39f4 100644 --- a/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs +++ b/src/Netclaw.Actors.Tests/Sessions/LlmSessionTestBase.cs @@ -8,6 +8,7 @@ using Akka.Persistence.Hosting; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; +using Netclaw.Actors.Channels; using Netclaw.Actors.Hosting; using Netclaw.Actors.Jobs; using Netclaw.Actors.Reminders; @@ -86,6 +87,8 @@ protected sealed override void ConfigureServices(HostBuilderContext context, ISe services.AddSingleton(); services.AddSingleton(NullNotificationSink.Instance); services.AddSingleton(NullReminderChannelNotifier.Instance); + services.AddSingleton(); + services.AddSingleton(sp => sp.GetRequiredService()); ConfigureSessionServices(services); services.AddLlmSessionCompositeRecords(); } From 310ffeabebfb005d2d07ce40a3b852c2731d41cb Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 18 Jul 2026 08:23:59 -0500 Subject: [PATCH 10/12] fix(approvals): show MCP arguments in prompts (#1689) * fix(approvals): show MCP arguments in prompts * fix(approvals): harden MCP invocation previews --- .../.system/files/netclaw-operations/SKILL.md | 15 +- .../DiscordApprovalPromptBuilderTests.cs | 49 +++ .../MattermostApprovalPromptBuilderTests.cs | 69 +++- .../SlackApprovalBlockBuilderTests.cs | 49 +++ .../Tools/ToolApprovalGateTests.cs | 97 +++++ .../Protocol/ApprovalOptionKeys.cs | 12 +- .../Sessions/LlmSessionActor.cs | 4 +- src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 23 +- .../DiscordApprovalPromptBuilder.cs | 61 ++- .../DiscordIngressMessages.cs | 5 +- .../MattermostApprovalPromptBuilder.cs | 51 ++- .../MattermostIngressMessages.cs | 5 +- .../SlackApprovalBlockBuilder.cs | 77 +++- .../SlackThreadBindingActor.cs | 5 +- src/Netclaw.Cli.Tests/Tui/ChatPageTests.cs | 90 ++++- .../chat-mcp-approval-large-collapsed.txt | 40 ++ .../chat-mcp-approval-large-expanded.txt | 40 ++ src/Netclaw.Cli/Tui/ChatPage.cs | 14 +- src/Netclaw.Cli/Tui/ChatViewModel.cs | 9 +- .../ShellApprovalMatcherTests.cs | 197 ++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 364 ++++++++++++++++++ src/Netclaw.Security/SecretOutputRedactor.cs | 27 ++ src/Netclaw.Tools.Abstractions/ToolName.cs | 7 + 23 files changed, 1240 insertions(+), 70 deletions(-) create mode 100644 src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-collapsed.txt create mode 100644 src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-expanded.txt diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 9c748a10b..e4143a137 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.33.0" + version: "2.35.0" --- # Netclaw Operations @@ -115,6 +115,19 @@ or MCP tools by capability before concluding a tool doesn't exist. Full guidance ## Approval Prompts +MCP approval prompts show a bounded, redacted preview of the call arguments. +Actual path- and URL-shaped values appear first and receive a larger preview so +the operator can verify location context without guessing from argument names. +URL credentials, query values, and fragments are redacted. +Large strings, binary data, and nested collections are summarized by size; +secret-like fields and token-shaped values are always redacted. Argument names +and values are escaped before display so server-controlled schema text cannot +break or spoof the approval prompt. MCP grants are tool-wide rather than +directory-scoped, so these prompts omit the misleading `Always here` option and +label the persistent choice `Always allow this tool` rather than the +shell-oriented `Always anywhere`. Other non-shell tools also omit `Always here` +because their approval matchers do not consume directory scope. + Approvals are typed `(verb, directory)` pairs in `tool-approvals.json`: - **verb** — the command head plus subcommand chain only (e.g. `git push`, diff --git a/src/Netclaw.Actors.Tests/Channels/DiscordApprovalPromptBuilderTests.cs b/src/Netclaw.Actors.Tests/Channels/DiscordApprovalPromptBuilderTests.cs index ac819290b..5ca89df6f 100644 --- a/src/Netclaw.Actors.Tests/Channels/DiscordApprovalPromptBuilderTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/DiscordApprovalPromptBuilderTests.cs @@ -279,6 +279,55 @@ private static ToolInteractionRequest V2Request( Options = options }; + [Fact] + public void Mcp_prompt_renders_invocation_without_shell_scope_chrome() + { + var request = V2Request( + "Dropbox/upload(destination_directory=\"/Finance/Q3\", contents=(90000 chars, 2000 lines))", + ["Dropbox/upload"], + cwd: null, + options: + [ + new ToolInteractionOption(ApprovalOptionKeys.ApproveOnceKey, ApprovalOptionKeys.ApproveOnceLabel), + new ToolInteractionOption(ApprovalOptionKeys.ApproveSessionKey, ApprovalOptionKeys.ApproveSessionLabel), + new ToolInteractionOption(ApprovalOptionKeys.ApproveEverywhereKey, ApprovalOptionKeys.ApproveMcpToolLabel), + new ToolInteractionOption(ApprovalOptionKeys.DenyKey, ApprovalOptionKeys.DenyLabel) + ]) with + { + ToolName = new Netclaw.Tools.ToolName("Dropbox/upload") + }; + + var text = DiscordApprovalPromptBuilder.BuildTextPrompt(request); + var (buttonText, buttons) = DiscordApprovalPromptBuilder.BuildButtonPrompt(request); + + Assert.Contains("MCP tool approval required", text); + Assert.Contains("Invocation:", text); + Assert.Contains("Allow this MCP tool invocation?", text); + Assert.Contains("**Invocation:**", buttonText); + Assert.Contains(buttons, button => button.Label == ApprovalOptionKeys.ApproveMcpToolLabel); + Assert.DoesNotContain("no working directory", text); + Assert.DoesNotContain("Always anywhere", text); + Assert.DoesNotContain("• Dropbox/upload", text); + } + + [Fact] + public void Mcp_resolution_describes_tool_scope_not_shell_location() + { + var request = V2Request("Dropbox/upload(path=\"/Finance/Q3\")", ["Dropbox/upload"], null, FullButtonRow()) with + { + ToolName = new Netclaw.Tools.ToolName("Dropbox/upload") + }; + + var text = DiscordApprovalPromptBuilder.BuildResolvedPromptText( + request, + ApprovalOptionKeys.ApproveEverywhere, + "user-1"); + + Assert.Contains("MCP tool approval resolved", text); + Assert.Contains("Always allowed: Dropbox/upload", text); + Assert.DoesNotContain("anywhere", text); + } + [Fact] public void V2_single_verb_collapses_into_header() { diff --git a/src/Netclaw.Actors.Tests/Channels/MattermostApprovalPromptBuilderTests.cs b/src/Netclaw.Actors.Tests/Channels/MattermostApprovalPromptBuilderTests.cs index 9c325df5d..2a2c15512 100644 --- a/src/Netclaw.Actors.Tests/Channels/MattermostApprovalPromptBuilderTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/MattermostApprovalPromptBuilderTests.cs @@ -70,15 +70,29 @@ public void BuildTextPrompt_omits_pattern_when_empty() [Fact] public void BuildDecisionStatus_formats_known_keys() { - Assert.Contains(ApprovalOptionKeys.ApproveOnceLabel, MattermostApprovalPromptBuilder.BuildDecisionStatus(ApprovalOptionKeys.ApproveOnce)); - Assert.Contains(ApprovalOptionKeys.ApproveAlwaysLabel, MattermostApprovalPromptBuilder.BuildDecisionStatus(ApprovalOptionKeys.ApproveAlways)); - Assert.Contains(ApprovalOptionKeys.DenyLabel, MattermostApprovalPromptBuilder.BuildDecisionStatus(ApprovalOptionKeys.Deny)); + var toolName = new Netclaw.Tools.ToolName("shell_execute"); + Assert.Contains(ApprovalOptionKeys.ApproveOnceLabel, MattermostApprovalPromptBuilder.BuildDecisionStatus(ApprovalOptionKeys.ApproveOnce, toolName)); + Assert.Contains(ApprovalOptionKeys.ApproveAlwaysLabel, MattermostApprovalPromptBuilder.BuildDecisionStatus(ApprovalOptionKeys.ApproveAlways, toolName)); + Assert.Contains(ApprovalOptionKeys.DenyLabel, MattermostApprovalPromptBuilder.BuildDecisionStatus(ApprovalOptionKeys.Deny, toolName)); + } + + [Fact] + public void BuildDecisionStatus_uses_MCP_persistent_label() + { + var status = MattermostApprovalPromptBuilder.BuildDecisionStatus( + ApprovalOptionKeys.ApproveEverywhere, + new Netclaw.Tools.ToolName("Dropbox/upload")); + + Assert.Contains(ApprovalOptionKeys.ApproveMcpToolLabel, status); + Assert.DoesNotContain(ApprovalOptionKeys.ApproveEverywhereLabel, status); } [Fact] public void BuildDecisionStatus_passes_through_unknown_key() { - var status = MattermostApprovalPromptBuilder.BuildDecisionStatus("custom_key"); + var status = MattermostApprovalPromptBuilder.BuildDecisionStatus( + "custom_key", + new Netclaw.Tools.ToolName("shell_execute")); Assert.Contains("custom_key", status); } @@ -355,6 +369,53 @@ private static ToolInteractionRequest CreateStandardRequest() ] }; + [Fact] + public void Mcp_prompt_renders_invocation_without_shell_patterns() + { + var request = CreateStandardRequest() with + { + ToolName = new Netclaw.Tools.ToolName("Dropbox/upload"), + DisplayText = "Dropbox/upload(destination_directory=\"/Finance/Q3\", contents=(90000 chars, 2000 lines))", + Patterns = ["Dropbox/upload"], + Options = + [ + new ToolInteractionOption(ApprovalOptionKeys.ApproveOnceKey, ApprovalOptionKeys.ApproveOnceLabel), + new ToolInteractionOption(ApprovalOptionKeys.ApproveSessionKey, ApprovalOptionKeys.ApproveSessionLabel), + new ToolInteractionOption(ApprovalOptionKeys.ApproveEverywhereKey, ApprovalOptionKeys.ApproveMcpToolLabel), + new ToolInteractionOption(ApprovalOptionKeys.DenyKey, ApprovalOptionKeys.DenyLabel) + ] + }; + + var text = MattermostApprovalPromptBuilder.BuildTextPrompt(request); + + Assert.Contains("MCP tool approval required", text); + Assert.Contains("**Invocation:**", text); + Assert.Contains("Allow this MCP tool invocation?", text); + Assert.Contains(ApprovalOptionKeys.ApproveMcpToolLabel, text); + Assert.DoesNotContain("**Pattern", text); + Assert.DoesNotContain("Always anywhere", text); + } + + [Fact] + public void Mcp_resolution_uses_contextual_persistent_grant_label() + { + var request = CreateStandardRequest() with + { + ToolName = new Netclaw.Tools.ToolName("Dropbox/upload"), + DisplayText = "Dropbox/upload(path=\"/Finance/Q3\")" + }; + + var text = MattermostApprovalPromptBuilder.BuildResolvedPromptText( + request, + ApprovalOptionKeys.ApproveEverywhere, + "user-1"); + + Assert.Contains("MCP tool approval resolved", text); + Assert.Contains($"**Decision:** {ApprovalOptionKeys.ApproveMcpToolLabel}", text); + Assert.DoesNotContain("Allow this MCP tool invocation?", text); + Assert.DoesNotContain("Always anywhere", text); + } + [Fact] public void Oversized_command_keeps_prompt_under_Mattermost_cap() { diff --git a/src/Netclaw.Actors.Tests/Channels/SlackApprovalBlockBuilderTests.cs b/src/Netclaw.Actors.Tests/Channels/SlackApprovalBlockBuilderTests.cs index 90739034a..1d45d37aa 100644 --- a/src/Netclaw.Actors.Tests/Channels/SlackApprovalBlockBuilderTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/SlackApprovalBlockBuilderTests.cs @@ -78,6 +78,55 @@ public void Multi_verb_uses_generic_header_with_bulleted_verbs() Assert.Contains("• `git status`", text); } + [Fact] + public void Mcp_prompt_renders_invocation_without_shell_scope_chrome() + { + var request = Request( + "Dropbox/upload(destination_directory=\"/Finance/Q3\", contents=(90000 chars, 2000 lines))", + ["Dropbox/upload"], + cwd: null, + options: + [ + new ToolInteractionOption(ApprovalOptionKeys.ApproveOnceKey, ApprovalOptionKeys.ApproveOnceLabel), + new ToolInteractionOption(ApprovalOptionKeys.ApproveSessionKey, ApprovalOptionKeys.ApproveSessionLabel), + new ToolInteractionOption(ApprovalOptionKeys.ApproveEverywhereKey, ApprovalOptionKeys.ApproveMcpToolLabel), + new ToolInteractionOption(ApprovalOptionKeys.DenyKey, ApprovalOptionKeys.DenyLabel) + ]) with + { + ToolName = new ToolName("Dropbox/upload") + }; + + var text = SlackApprovalBlockBuilder.BuildApprovalText(request); + var blocks = string.Join('\n', SlackApprovalBlockBuilder.BuildApprovalBlocks(request) + .OfType() + .Select(block => block.Text is Markdown markdown ? markdown.Text : string.Empty)); + + Assert.Contains("MCP tool approval required", text); + Assert.Contains("Allow this MCP tool invocation?", text); + Assert.Contains("*Invocation:*", blocks); + Assert.DoesNotContain("no working directory", text); + Assert.DoesNotContain("Always anywhere", text); + Assert.DoesNotContain("• `Dropbox/upload`", text); + } + + [Fact] + public void Mcp_resolution_describes_tool_scope_not_shell_location() + { + var request = Request("Dropbox/upload(path=\"/Finance/Q3\")", ["Dropbox/upload"], null, FullButtonRow()) with + { + ToolName = new ToolName("Dropbox/upload") + }; + + var text = SlackApprovalBlockBuilder.BuildResolvedApprovalText( + request, + ApprovalOptionKeys.ApproveEverywhere, + "U123"); + + Assert.Contains("MCP tool approval resolved", text); + Assert.Contains("Always allowed: Dropbox/upload", text); + Assert.DoesNotContain("anywhere", text); + } + [Fact] public void Messy_command_emits_complex_command_hint() { diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs index bde7a159e..e49302f4d 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs @@ -367,6 +367,74 @@ public void mcp_server_default_applies_when_no_exact_override() Assert.Equal("notion/create-pages", decision.ApprovalContext!.ToolName); } + [Fact] + public void mcp_approval_context_displays_arguments_without_netclaw_meta_fields() + { + var approvalPolicy = new ToolApprovalConfig + { + DefaultMode = ToolApprovalMode.Auto, + McpServerDefaults = new Dictionary(StringComparer.Ordinal) + { + ["notion"] = ToolApprovalMode.Approval + } + }; + var policy = CreateMcpApprovalPolicy(approvalPolicy); + var content = string.Join('\n', Enumerable.Repeat("large memory payload", 1_000)); + + var decision = policy.AuthorizeInvocation( + McpTool("notion", "create-pages"), + PersonalContext(), + new Dictionary + { + ["destination_path"] = "/Board/Quarterly Results", + ["content"] = content, + ["_rationale"] = "Create the requested report" + }); + + Assert.True(decision.NeedsApproval); + Assert.Contains("destination_path=\"/Board/Quarterly Results\"", decision.ApprovalContext!.DisplayText); + Assert.Contains($"content=({content.Length} chars, 1000 lines)", decision.ApprovalContext.DisplayText); + Assert.DoesNotContain("_rationale", decision.ApprovalContext.DisplayText); + Assert.DoesNotContain("Create the requested report", decision.ApprovalContext.DisplayText); + Assert.DoesNotContain( + decision.ApprovalContext.Options, + option => option.Key.Value == ApprovalOptionKeys.ApproveAlways); + Assert.Contains( + decision.ApprovalContext.Options, + option => option.Key.Value == ApprovalOptionKeys.ApproveEverywhere + && option.Label == ApprovalOptionKeys.ApproveMcpToolLabel); + Assert.DoesNotContain( + decision.ApprovalContext.Options, + option => option.Label == ApprovalOptionKeys.ApproveEverywhereLabel); + } + + [Fact] + public void mcp_declared_parameter_that_resembles_meta_remains_visible() + { + var approvalPolicy = new ToolApprovalConfig + { + DefaultMode = ToolApprovalMode.Auto, + McpServerDefaults = new Dictionary(StringComparer.Ordinal) + { + ["notion"] = ToolApprovalMode.Approval + } + }; + var policy = CreateMcpApprovalPolicy(approvalPolicy); + var function = AIFunctionFactory.Create( + (string rationale) => rationale, + "create-pages", + "create-pages"); + var tool = new McpToolAdapter(function, "notion", "create-pages"); + + var decision = policy.AuthorizeInvocation( + tool, + PersonalContext(), + new Dictionary { ["Rationale"] = "Customer-visible reason" }); + + Assert.True(decision.NeedsApproval); + Assert.Contains("Rationale=\"Customer-visible reason\"", decision.ApprovalContext!.DisplayText); + } + [Fact] public void mcp_exact_override_beats_server_default() { @@ -521,6 +589,35 @@ public void Non_interactive_tool_requires_approval_when_policy_requires_approval Assert.NotNull(decision.ApprovalContext); } + [Fact] + public void Non_shell_tool_omits_directory_scoped_persistence_option() + { + var config = new ToolConfig(); + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + DefaultMode = ToolApprovalMode.Approval + }; + var policy = new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false)); + + var tool = new Netclaw.Actors.Tests.Memory.FakeNetclawTool("file_read", "content"); + var decision = policy.AuthorizeInvocation(tool, PersonalContext()); + + Assert.True(decision.NeedsApproval); + Assert.DoesNotContain( + decision.ApprovalContext!.Options, + option => option.Key.Value == ApprovalOptionKeys.ApproveAlways); + Assert.Contains( + decision.ApprovalContext.Options, + option => option.Key.Value == ApprovalOptionKeys.ApproveEverywhere + && option.Label == ApprovalOptionKeys.ApproveEverywhereLabel); + } + [Fact] public void Non_safe_list_tool_returns_requires_approval_when_interactive_unsupported() { diff --git a/src/Netclaw.Actors/Protocol/ApprovalOptionKeys.cs b/src/Netclaw.Actors/Protocol/ApprovalOptionKeys.cs index 32a9ec3ec..695cb211c 100644 --- a/src/Netclaw.Actors/Protocol/ApprovalOptionKeys.cs +++ b/src/Netclaw.Actors/Protocol/ApprovalOptionKeys.cs @@ -43,6 +43,7 @@ public static class ApprovalOptionKeys public const string ApproveSessionLabel = "This chat"; public const string ApproveAlwaysLabel = "Always here"; public const string ApproveEverywhereLabel = "Always anywhere"; + public const string ApproveMcpToolLabel = "Always allow this tool"; public const string DenyLabel = "Deny"; /// @@ -67,12 +68,19 @@ public static bool IsDangerStyled(string optionKey) /// Maps an approval option key to its short human-readable label. Returns /// the key unchanged when it is not a recognized option. /// - public static string LabelFor(string optionKey) => optionKey switch + public static string LabelFor(string optionKey) => LabelFor(optionKey, isMcpTool: false); + + /// + /// Maps an approval option key to its context-aware label. MCP grants are + /// tool-scoped rather than directory-scoped, so the global persistence key + /// is rendered as "Always allow this tool" instead of "Always anywhere". + /// + public static string LabelFor(string optionKey, bool isMcpTool) => optionKey switch { ApproveOnce => ApproveOnceLabel, ApproveSession => ApproveSessionLabel, ApproveAlways => ApproveAlwaysLabel, - ApproveEverywhere => ApproveEverywhereLabel, + ApproveEverywhere => isMcpTool ? ApproveMcpToolLabel : ApproveEverywhereLabel, Deny => DenyLabel, _ => optionKey }; diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index fa92ea6ec..df8557770 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -3901,7 +3901,9 @@ private bool TryResolveTextApprovalResponse( ]; var options = optionKeys - .Select(key => new ToolInteractionOption(new ApprovalOptionKey(key), ApprovalOptionKeys.LabelFor(key))) + .Select(key => new ToolInteractionOption( + new ApprovalOptionKey(key), + ApprovalOptionKeys.LabelFor(key, new ToolName(pending.ToolName).IsMcp))) .ToArray(); if (!ToolInteractionResponseParser.TryParseApprovalResponse(msg.Text, options, out var selectedKey) diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 05405f379..28952072f 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -109,7 +109,10 @@ public ToolAccessDecision AuthorizeInvocation( return ToolAccessDecision.Deny("mcp_tool_not_allowed_for_audience_profile"); var mcpToolName = new ToolName(tool.Name); - return CheckApprovalGate(mcpToolName, context, arguments, DefaultApprovalMatcher.Instance); + var (_, approvalArguments) = ToolCallMeta.ExtractFrom( + arguments, + key => ToolArgumentValidator.ResolveMetaField(mcp, key)); + return CheckApprovalGate(mcpToolName, context, approvalArguments, McpApprovalMatcher.Instance); } var toolName = new ToolName(tool.Name); @@ -344,7 +347,9 @@ private ToolAccessDecision CheckApprovalGate( isMessy, isCwdShallow: IsCwdTooShallow(context.Approval.Cwd), allEffectiveDirsAreSessionScratch: AllCandidatesResolveToSessionScratch( - candidates, context.Approval.Cwd, context.SessionDirectory)); + candidates, context.Approval.Cwd, context.SessionDirectory), + supportsDirectoryScope: matcher is ShellApprovalMatcher, + isMcpTool: toolName.IsMcp); var approvalContext = new ToolApprovalContext( toolName.Value, @@ -408,12 +413,18 @@ private static bool AllCandidatesResolveToSessionScratch( /// to a directory that won't recur. This chat already provides /// the equivalent in-session semantics without polluting the persistent /// store. + /// No directory scope (all non-shell tools) — Always + /// here is omitted because these matchers grant independently of cwd. + /// For MCP tools, the remaining persistent choice is labeled Always + /// allow this tool because it persists a canonical-tool grant. /// /// private static IReadOnlyList BuildApprovalOptions( bool isMessy, bool isCwdShallow, - bool allEffectiveDirsAreSessionScratch) + bool allEffectiveDirsAreSessionScratch, + bool supportsDirectoryScope, + bool isMcpTool) { if (isMessy) { @@ -430,12 +441,14 @@ private static IReadOnlyList BuildApprovalOptions( new ToolApprovalOption(ApprovalOptionKeys.ApproveSessionKey, ApprovalOptionKeys.ApproveSessionLabel) }; - if (!isCwdShallow && !allEffectiveDirsAreSessionScratch) + if (supportsDirectoryScope && !isCwdShallow && !allEffectiveDirsAreSessionScratch) { options.Add(new ToolApprovalOption(ApprovalOptionKeys.ApproveAlwaysKey, ApprovalOptionKeys.ApproveAlwaysLabel)); } - options.Add(new ToolApprovalOption(ApprovalOptionKeys.ApproveEverywhereKey, ApprovalOptionKeys.ApproveEverywhereLabel)); + options.Add(new ToolApprovalOption( + ApprovalOptionKeys.ApproveEverywhereKey, + ApprovalOptionKeys.LabelFor(ApprovalOptionKeys.ApproveEverywhere, isMcpTool))); options.Add(new ToolApprovalOption(ApprovalOptionKeys.DenyKey, ApprovalOptionKeys.DenyLabel)); return options; diff --git a/src/Netclaw.Channels.Discord/DiscordApprovalPromptBuilder.cs b/src/Netclaw.Channels.Discord/DiscordApprovalPromptBuilder.cs index f05e1d305..34db8e458 100644 --- a/src/Netclaw.Channels.Discord/DiscordApprovalPromptBuilder.cs +++ b/src/Netclaw.Channels.Discord/DiscordApprovalPromptBuilder.cs @@ -6,6 +6,7 @@ using System.Text; using Netclaw.Actors.Protocol; using Netclaw.Channels; +using Netclaw.Tools; using static Netclaw.Actors.Sessions.SessionProtocol; namespace Netclaw.Channels.Discord; @@ -25,13 +26,14 @@ internal static class DiscordApprovalPromptBuilder public static string BuildTextPrompt(ToolInteractionRequest request) { var sb = new StringBuilder(); - sb.AppendLine("Netclaw approval required:"); + sb.AppendLine(request.ToolName.IsMcp ? "MCP tool approval required:" : "Netclaw approval required:"); sb.Append("Tool: ").AppendLine(request.ToolName.Value); - sb.Append("Action: ").AppendLine(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)); + sb.Append(request.ToolName.IsMcp ? "Invocation: " : "Action: ") + .AppendLine(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)); sb.AppendLine(BuildApproveHeader(request)); var verbs = ResolveDisplayVerbs(request); - if (verbs.Count > 1) + if (!request.ToolName.IsMcp && verbs.Count > 1) { foreach (var verb in verbs) sb.Append(" • ").AppendLine(verb); @@ -52,7 +54,9 @@ public static (string Text, IReadOnlyList Buttons) BuildButto ToolInteractionRequest request) { var sb = new StringBuilder(); - sb.AppendLine(":lock: **Tool approval required**"); + sb.Append(":lock: **") + .Append(request.ToolName.IsMcp ? "MCP tool approval required" : "Tool approval required") + .AppendLine("**"); AppendToolSummary(sb, request); sb.AppendLine(); @@ -87,9 +91,12 @@ public static string BuildResolvedPromptText( : ":white_check_mark:"; var sb = new StringBuilder(); - sb.Append(statusEmoji).AppendLine(" **Tool approval resolved**"); + sb.Append(statusEmoji) + .Append(request.ToolName.IsMcp ? " **MCP tool approval resolved**" : " **Tool approval resolved**") + .AppendLine(); sb.Append("**Tool:** `").Append(request.ToolName).AppendLine("`"); - sb.Append("**Action:** `").Append(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)).AppendLine("`"); + sb.Append(request.ToolName.IsMcp ? "**Invocation:** `" : "**Action:** `") + .Append(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)).AppendLine("`"); sb.Append("**").Append(BuildResolutionLine(request, selectedKey)).Append("**"); sb.Append(" (by <@").Append(senderId).Append(">)"); @@ -126,24 +133,36 @@ public static string BuildResolvedPromptTextWithoutRequest( : ":white_check_mark:"; var sb = new StringBuilder(); - sb.Append(statusEmoji).AppendLine(" **Tool approval resolved**"); + var isMcpTool = !string.IsNullOrEmpty(toolName) && new ToolName(toolName).IsMcp; + sb.Append(statusEmoji) + .Append(isMcpTool ? " **MCP tool approval resolved**" : " **Tool approval resolved**") + .AppendLine(); if (!string.IsNullOrEmpty(toolName) && !string.IsNullOrEmpty(displayText)) { sb.Append("**Tool:** `").Append(toolName).AppendLine("`"); - sb.Append("**Action:** `") + sb.Append(isMcpTool ? "**Invocation:** `" : "**Action:** `") .Append(ApprovalDisplayTextFormatter.Truncate(displayText, MaxDisplayTextChars)) .AppendLine("`"); } - sb.Append("**").Append(BuildGenericResolutionLine(selectedKey)).Append("**"); + sb.Append("**").Append(BuildGenericResolutionLine(selectedKey, isMcpTool)).Append("**"); sb.Append(" (by <@").Append(senderId).Append(">)"); return sb.ToString(); } - private static string BuildGenericResolutionLine(string selectedKey) - => selectedKey switch + private static string BuildGenericResolutionLine(string selectedKey, bool isMcpTool) + => isMcpTool + ? selectedKey switch + { + ApprovalOptionKeys.ApproveAlways or ApprovalOptionKeys.ApproveEverywhere => "Always allowed this MCP tool", + ApprovalOptionKeys.ApproveSession => "Allowed this MCP tool for this chat", + ApprovalOptionKeys.ApproveOnce => "Allowed once", + ApprovalOptionKeys.Deny => "Denied", + _ => "Resolved" + } + : selectedKey switch { ApprovalOptionKeys.ApproveAlways => "Saved: always here", ApprovalOptionKeys.ApproveEverywhere => "Saved: always anywhere", @@ -156,11 +175,12 @@ private static string BuildGenericResolutionLine(string selectedKey) private static void AppendToolSummary(StringBuilder sb, ToolInteractionRequest request) { sb.Append("**Tool:** `").Append(request.ToolName).AppendLine("`"); - sb.Append("**Action:** `").Append(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)).AppendLine("`"); + sb.Append(request.ToolName.IsMcp ? "**Invocation:** `" : "**Action:** `") + .Append(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)).AppendLine("`"); sb.Append("**").Append(BuildApproveHeader(request)).AppendLine("**"); var verbs = ResolveDisplayVerbs(request); - if (verbs.Count > 1) + if (!request.ToolName.IsMcp && verbs.Count > 1) { foreach (var verb in verbs) sb.Append(" • `").Append(verb).AppendLine("`"); @@ -180,6 +200,9 @@ private static void AppendToolSummary(StringBuilder sb, ToolInteractionRequest r /// private static string BuildApproveHeader(ToolInteractionRequest request) { + if (request.ToolName.IsMcp) + return "Allow this MCP tool invocation?"; + var verbs = ResolveDisplayVerbs(request); var location = ResolveHeaderLocation(request); @@ -195,6 +218,18 @@ private static string BuildApproveHeader(ToolInteractionRequest request) /// private static string BuildResolutionLine(ToolInteractionRequest request, string selectedKey) { + if (request.ToolName.IsMcp) + { + return selectedKey switch + { + ApprovalOptionKeys.ApproveAlways or ApprovalOptionKeys.ApproveEverywhere => $"Always allowed: {request.ToolName}", + ApprovalOptionKeys.ApproveSession => $"Allowed for this chat: {request.ToolName}", + ApprovalOptionKeys.ApproveOnce => "Allowed once", + ApprovalOptionKeys.Deny => "Denied", + _ => "Resolved" + }; + } + var verbs = string.Join(", ", ResolveDisplayVerbs(request)); var location = ResolveHeaderLocation(request); diff --git a/src/Netclaw.Channels.Discord/DiscordIngressMessages.cs b/src/Netclaw.Channels.Discord/DiscordIngressMessages.cs index a3841e228..f8b45d6b2 100644 --- a/src/Netclaw.Channels.Discord/DiscordIngressMessages.cs +++ b/src/Netclaw.Channels.Discord/DiscordIngressMessages.cs @@ -92,8 +92,11 @@ public PendingApprovalRequest( RequesterSenderId = requesterSenderId; RequesterPrincipal = requesterPrincipal; OptionKeys = [.. optionKeys]; + var isMcpTool = !string.IsNullOrEmpty(toolName) && new ToolName(toolName).IsMcp; Options = OptionKeys - .Select(key => new ToolInteractionOption(new ApprovalOptionKey(key), ApprovalOptionKeys.LabelFor(key))) + .Select(key => new ToolInteractionOption( + new ApprovalOptionKey(key), + ApprovalOptionKeys.LabelFor(key, isMcpTool))) .ToArray(); PromptMessageId = promptMessageId; ToolName = toolName; diff --git a/src/Netclaw.Channels.Mattermost/MattermostApprovalPromptBuilder.cs b/src/Netclaw.Channels.Mattermost/MattermostApprovalPromptBuilder.cs index 2fd466307..847888199 100644 --- a/src/Netclaw.Channels.Mattermost/MattermostApprovalPromptBuilder.cs +++ b/src/Netclaw.Channels.Mattermost/MattermostApprovalPromptBuilder.cs @@ -6,6 +6,7 @@ using System.Text; using Netclaw.Actors.Protocol; using Netclaw.Channels; +using Netclaw.Tools; using static Netclaw.Actors.Sessions.SessionProtocol; namespace Netclaw.Channels.Mattermost; @@ -28,8 +29,8 @@ public static (string Text, IReadOnlyList Attachments) Bui MattermostCallbackActionStore? actionStore = null) { var sb = new StringBuilder(); - sb.AppendLine(":lock: **Tool approval required**"); - AppendToolSummary(sb, request); + AppendApprovalTitle(sb, request); + AppendToolSummary(sb, request, includeApprovalQuestion: true); sb.AppendLine(); sb.Append("You can also reply with ").Append(FormatReplyLetters(request.Options)).Append(" in this thread."); @@ -62,7 +63,7 @@ public static (string Text, IReadOnlyList Attachments) Bui .ToList(); var attachment = new MattermostAttachment( - Fallback: $"Tool approval required — reply with {string.Join(", ", Enumerable.Range(0, actions.Count).Select(GetReplyLetter))}", + Fallback: $"{(request.ToolName.IsMcp ? "MCP tool approval required" : "Tool approval required")} — reply with {string.Join(", ", Enumerable.Range(0, actions.Count).Select(GetReplyLetter))}", Color: "#3AA3E3", Actions: actions); @@ -72,8 +73,8 @@ public static (string Text, IReadOnlyList Attachments) Bui public static string BuildTextPrompt(ToolInteractionRequest request) { var sb = new StringBuilder(); - sb.AppendLine(":lock: **Tool approval required**"); - AppendToolSummary(sb, request); + AppendApprovalTitle(sb, request); + AppendToolSummary(sb, request, includeApprovalQuestion: true); sb.AppendLine(); sb.AppendLine("Reply with:"); @@ -81,9 +82,9 @@ public static string BuildTextPrompt(ToolInteractionRequest request) return sb.ToString().TrimEnd(); } - public static string BuildDecisionStatus(string selectedKey) + public static string BuildDecisionStatus(string selectedKey, ToolName toolName) { - var label = ApprovalOptionKeys.LabelFor(selectedKey); + var label = ApprovalOptionKeys.LabelFor(selectedKey, toolName.IsMcp); return $"Recorded approval decision: {label}."; } @@ -95,11 +96,13 @@ public static string BuildResolvedPromptText( var statusEmoji = selectedKey == ApprovalOptionKeys.Deny ? ":no_entry:" : ":white_check_mark:"; - var decisionLabel = ApprovalOptionKeys.LabelFor(selectedKey); + var decisionLabel = ApprovalOptionKeys.LabelFor(selectedKey, request.ToolName.IsMcp); var sb = new StringBuilder(); - sb.Append(statusEmoji).AppendLine(" **Tool approval resolved**"); - AppendToolSummary(sb, request); + sb.Append(statusEmoji) + .Append(request.ToolName.IsMcp ? " **MCP tool approval resolved**" : " **Tool approval resolved**") + .AppendLine(); + AppendToolSummary(sb, request, includeApprovalQuestion: false); sb.Append("**Decision:** ").Append(decisionLabel); sb.Append(" (by @").Append(senderId).Append(')'); @@ -141,15 +144,18 @@ public static MattermostAttachment BuildResolvedAttachmentWithoutRequest( var statusEmoji = selectedKey == ApprovalOptionKeys.Deny ? ":no_entry:" : ":white_check_mark:"; - var decisionLabel = ApprovalOptionKeys.LabelFor(selectedKey); + var isMcpTool = !string.IsNullOrEmpty(toolName) && new ToolName(toolName).IsMcp; + var decisionLabel = ApprovalOptionKeys.LabelFor(selectedKey, isMcpTool); var sb = new StringBuilder(); - sb.Append(statusEmoji).AppendLine(" **Tool approval resolved**"); + sb.Append(statusEmoji) + .Append(isMcpTool ? " **MCP tool approval resolved**" : " **Tool approval resolved**") + .AppendLine(); if (!string.IsNullOrEmpty(toolName) && !string.IsNullOrEmpty(displayText)) { sb.Append("**Tool:** `").Append(toolName).AppendLine("`"); - sb.Append("**Action:** `") + sb.Append(isMcpTool ? "**Invocation:** `" : "**Action:** `") .Append(ApprovalDisplayTextFormatter.Truncate(displayText, MaxDisplayTextChars)) .AppendLine("`"); } @@ -166,12 +172,20 @@ public static MattermostAttachment BuildResolvedAttachmentWithoutRequest( Text: resolvedText); } - private static void AppendToolSummary(StringBuilder sb, ToolInteractionRequest request) + private static void AppendToolSummary( + StringBuilder sb, + ToolInteractionRequest request, + bool includeApprovalQuestion) { sb.Append("**Tool:** `").Append(request.ToolName).AppendLine("`"); - sb.Append("**Action:** `").Append(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)).AppendLine("`"); + sb.Append(request.ToolName.IsMcp ? "**Invocation:** `" : "**Action:** `") + .Append(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)).AppendLine("`"); - if (request.Patterns.Count > 0) + if (request.ToolName.IsMcp && includeApprovalQuestion) + { + sb.AppendLine("**Allow this MCP tool invocation?**"); + } + else if (request.Patterns.Count > 0) { if (request.Patterns.Count == 1) { @@ -188,6 +202,11 @@ private static void AppendToolSummary(StringBuilder sb, ToolInteractionRequest r AppendAdoptedContextSummary(sb, request); } + private static void AppendApprovalTitle(StringBuilder sb, ToolInteractionRequest request) + => sb.Append(":lock: **") + .Append(request.ToolName.IsMcp ? "MCP tool approval required" : "Tool approval required") + .AppendLine("**"); + private static void AppendAdoptedContextSummary(StringBuilder sb, ToolInteractionRequest request) { if (!request.HasAdoptedContext) diff --git a/src/Netclaw.Channels.Mattermost/MattermostIngressMessages.cs b/src/Netclaw.Channels.Mattermost/MattermostIngressMessages.cs index b0fd2183b..e306cb80e 100644 --- a/src/Netclaw.Channels.Mattermost/MattermostIngressMessages.cs +++ b/src/Netclaw.Channels.Mattermost/MattermostIngressMessages.cs @@ -85,8 +85,11 @@ public PendingApprovalRequest( RequesterSenderId = requesterSenderId; RequesterPrincipal = requesterPrincipal; OptionKeys = [.. optionKeys]; + var isMcpTool = !string.IsNullOrEmpty(toolName) && new ToolName(toolName).IsMcp; Options = OptionKeys - .Select(key => new ToolInteractionOption(new ApprovalOptionKey(key), ApprovalOptionKeys.LabelFor(key))) + .Select(key => new ToolInteractionOption( + new ApprovalOptionKey(key), + ApprovalOptionKeys.LabelFor(key, isMcpTool))) .ToArray(); PromptPostId = promptPostId; ToolName = toolName; diff --git a/src/Netclaw.Channels.Slack/SlackApprovalBlockBuilder.cs b/src/Netclaw.Channels.Slack/SlackApprovalBlockBuilder.cs index bb0d19f5f..99f6f9cfe 100644 --- a/src/Netclaw.Channels.Slack/SlackApprovalBlockBuilder.cs +++ b/src/Netclaw.Channels.Slack/SlackApprovalBlockBuilder.cs @@ -4,6 +4,7 @@ // // ----------------------------------------------------------------------- using Netclaw.Actors.Protocol; +using Netclaw.Tools; using SlackNet.Blocks; using static Netclaw.Actors.Sessions.SessionProtocol; @@ -28,13 +29,13 @@ public static string BuildApprovalText(ToolInteractionRequest request) { var lines = new List { - ":lock: *Tool approval required*", + $":lock: *{ApprovalTitle(request.ToolName)}*", $"> `{request.ToolName}`: `{ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)}`", BuildApproveHeader(request) }; var verbs = ResolveDisplayVerbs(request); - if (verbs.Count > 1) + if (!request.ToolName.IsMcp && verbs.Count > 1) { foreach (var verb in verbs) lines.Add($" • `{verb}`"); @@ -59,11 +60,13 @@ public static IReadOnlyList BuildApprovalBlocks(ToolInteractionRequest re { new SectionBlock { - Text = new Markdown(":lock: *Tool approval required*") + Text = new Markdown($":lock: *{ApprovalTitle(request.ToolName)}*") }, new SectionBlock { - Text = new Markdown($"*Tool:* `{EscapeMarkdown(request.ToolName.Value)}`\n*Request:* `{EscapeMarkdown(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars))}`"), + Text = new Markdown( + $"*Tool:* `{EscapeMarkdown(request.ToolName.Value)}`\n" + + $"*{RequestLabel(request.ToolName)}:* `{EscapeMarkdown(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars))}`"), Expand = true }, new SectionBlock @@ -73,7 +76,7 @@ public static IReadOnlyList BuildApprovalBlocks(ToolInteractionRequest re }; var verbs = ResolveDisplayVerbs(request); - if (verbs.Count > 1) + if (!request.ToolName.IsMcp && verbs.Count > 1) { var verbLines = verbs.Select(v => $"• `{EscapeMarkdown(v)}`"); blocks.Add(new SectionBlock @@ -134,7 +137,7 @@ public static string BuildResolvedApprovalText( return string.Join("\n", new[] { - $"{statusPrefix} *Tool approval resolved* by <@{EscapeMarkdown(senderId)}>", + $"{statusPrefix} *{ApprovalResolvedTitle(request.ToolName)}* by <@{EscapeMarkdown(senderId)}>", $"> `{request.ToolName}`: `{ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars)}`", BuildResolutionLine(request, selectedKey) }); @@ -154,13 +157,13 @@ public static IReadOnlyList BuildResolvedApprovalBlocks( { new SectionBlock { - Text = new Markdown($"{statusPrefix} *Tool approval resolved* by <@{EscapeMarkdown(senderId)}>") + Text = new Markdown($"{statusPrefix} *{ApprovalResolvedTitle(request.ToolName)}* by <@{EscapeMarkdown(senderId)}>") }, new SectionBlock { Text = new Markdown( $"*Tool:* `{EscapeMarkdown(request.ToolName.Value)}`\n" - + $"*Request:* `{EscapeMarkdown(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars))}`\n" + + $"*{RequestLabel(request.ToolName)}:* `{EscapeMarkdown(ApprovalDisplayTextFormatter.Truncate(request.DisplayText, MaxDisplayTextChars))}`\n" + $"*{EscapeMarkdown(resolutionLine)}*"), Expand = true } @@ -208,7 +211,7 @@ public static string BuildResolvedApprovalTextWithoutRequest( var lines = new List(3) { - $"{statusPrefix} *Tool approval resolved* by <@{EscapeMarkdown(senderId)}>" + $"{statusPrefix} *{ApprovalResolvedTitle(toolName)}* by <@{EscapeMarkdown(senderId)}>" }; if (!string.IsNullOrEmpty(toolName) && !string.IsNullOrEmpty(displayText)) @@ -217,7 +220,7 @@ public static string BuildResolvedApprovalTextWithoutRequest( $"> `{toolName}`: `{ApprovalDisplayTextFormatter.Truncate(displayText, MaxDisplayTextChars)}`"); } - lines.Add(BuildGenericResolutionLine(selectedKey)); + lines.Add(BuildGenericResolutionLine(selectedKey, IsMcpToolName(toolName))); return string.Join("\n", lines); } @@ -241,7 +244,7 @@ public static IReadOnlyList BuildResolvedApprovalBlocksWithoutRequest( { new SectionBlock { - Text = new Markdown($"{statusPrefix} *Tool approval resolved* by <@{EscapeMarkdown(senderId)}>") + Text = new Markdown($"{statusPrefix} *{ApprovalResolvedTitle(toolName)}* by <@{EscapeMarkdown(senderId)}>") } }; @@ -251,8 +254,8 @@ public static IReadOnlyList BuildResolvedApprovalBlocksWithoutRequest( { Text = new Markdown( $"*Tool:* `{EscapeMarkdown(toolName)}`\n" - + $"*Request:* `{EscapeMarkdown(ApprovalDisplayTextFormatter.Truncate(displayText, MaxDisplayTextChars))}`\n" - + $"*{EscapeMarkdown(BuildGenericResolutionLine(selectedKey))}*"), + + $"*{RequestLabel(toolName)}:* `{EscapeMarkdown(ApprovalDisplayTextFormatter.Truncate(displayText, MaxDisplayTextChars))}`\n" + + $"*{EscapeMarkdown(BuildGenericResolutionLine(selectedKey, IsMcpToolName(toolName)))}*"), Expand = true }); } @@ -260,15 +263,24 @@ public static IReadOnlyList BuildResolvedApprovalBlocksWithoutRequest( { blocks.Add(new SectionBlock { - Text = new Markdown($"*{EscapeMarkdown(BuildGenericResolutionLine(selectedKey))}*") + Text = new Markdown($"*{EscapeMarkdown(BuildGenericResolutionLine(selectedKey, isMcpTool: false))}*") }); } return blocks; } - private static string BuildGenericResolutionLine(string selectedKey) - => selectedKey switch + private static string BuildGenericResolutionLine(string selectedKey, bool isMcpTool) + => isMcpTool + ? selectedKey switch + { + ApprovalOptionKeys.ApproveAlways or ApprovalOptionKeys.ApproveEverywhere => "Always allowed this MCP tool", + ApprovalOptionKeys.ApproveSession => "Allowed this MCP tool for this chat", + ApprovalOptionKeys.ApproveOnce => "Allowed once", + ApprovalOptionKeys.Deny => "Denied", + _ => "Resolved" + } + : selectedKey switch { ApprovalOptionKeys.ApproveAlways => "Saved: always here", ApprovalOptionKeys.ApproveEverywhere => "Saved: always anywhere", @@ -299,6 +311,9 @@ private static string BuildGenericResolutionLine(string selectedKey) /// private static string BuildApproveHeader(ToolInteractionRequest request) { + if (request.ToolName.IsMcp) + return "Allow this MCP tool invocation?"; + var verbs = ResolveDisplayVerbs(request); var location = ResolveHeaderLocation(request); @@ -354,6 +369,18 @@ private static bool IsSessionScratchPath(string cwd) /// private static string BuildResolutionLine(ToolInteractionRequest request, string selectedKey) { + if (request.ToolName.IsMcp) + { + return selectedKey switch + { + ApprovalOptionKeys.ApproveAlways or ApprovalOptionKeys.ApproveEverywhere => $"Always allowed: {request.ToolName}", + ApprovalOptionKeys.ApproveSession => $"Allowed for this chat: {request.ToolName}", + ApprovalOptionKeys.ApproveOnce => "Allowed once", + ApprovalOptionKeys.Deny => "Denied", + _ => "Resolved" + }; + } + var verbs = string.Join(", ", ResolveDisplayVerbs(request)); var location = ResolveHeaderLocation(request); @@ -380,6 +407,24 @@ private static IReadOnlyList ResolveDisplayVerbs(ToolInteractionRequest ? request.CandidateVerbs : request.Patterns; + private static string ApprovalTitle(ToolName toolName) + => toolName.IsMcp ? "MCP tool approval required" : "Tool approval required"; + + private static string ApprovalResolvedTitle(ToolName toolName) + => toolName.IsMcp ? "MCP tool approval resolved" : "Tool approval resolved"; + + private static string ApprovalResolvedTitle(string? toolName) + => IsMcpToolName(toolName) ? "MCP tool approval resolved" : "Tool approval resolved"; + + private static string RequestLabel(ToolName toolName) + => toolName.IsMcp ? "Invocation" : "Request"; + + private static string RequestLabel(string toolName) + => new ToolName(toolName).IsMcp ? "Invocation" : "Request"; + + private static bool IsMcpToolName(string? toolName) + => !string.IsNullOrEmpty(toolName) && new ToolName(toolName).IsMcp; + private static void AppendAdoptedContextSummary(List lines, ToolInteractionRequest request) { if (!request.HasAdoptedContext) diff --git a/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs b/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs index b58891f38..678fbf2d1 100644 --- a/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs +++ b/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs @@ -1835,8 +1835,11 @@ public PendingApprovalRequest( RequesterSenderId = requesterSenderId; RequesterPrincipal = requesterPrincipal; OptionKeys = [.. optionKeys]; + var isMcpTool = !string.IsNullOrEmpty(toolName) && new ToolName(toolName).IsMcp; Options = OptionKeys - .Select(key => new ToolInteractionOption(new ApprovalOptionKey(key), ApprovalOptionKeys.LabelFor(key))) + .Select(key => new ToolInteractionOption( + new ApprovalOptionKey(key), + ApprovalOptionKeys.LabelFor(key, isMcpTool))) .ToArray(); PromptMessageTs = promptMessageTs; ToolName = toolName; diff --git a/src/Netclaw.Cli.Tests/Tui/ChatPageTests.cs b/src/Netclaw.Cli.Tests/Tui/ChatPageTests.cs index 501941f3e..0f071ef8b 100644 --- a/src/Netclaw.Cli.Tests/Tui/ChatPageTests.cs +++ b/src/Netclaw.Cli.Tests/Tui/ChatPageTests.cs @@ -4,10 +4,14 @@ // // ----------------------------------------------------------------------- using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.AI; using Netclaw.Actors.Protocol; +using Netclaw.Actors.Tools; using Netclaw.Cli.Daemon; using Netclaw.Cli.Tui; using Netclaw.Configuration; +using Netclaw.Security; +using Netclaw.Tools; using Termina; using Termina.Hosting; using Termina.Input; @@ -325,9 +329,89 @@ public async Task LongApprovalBody_RenderedFrameSnapshot() WriteSnapshot("chat-approval-expanded.txt", expanded); } + [Fact] + public async Task LargeMcpApproval_RenderedFrameSnapshot() + { + var content = string.Join('\n', Enumerable.Repeat("large memorizer payload that must not render", 2_000)); + var function = AIFunctionFactory.Create( + (string contents, string source_path, string destination_directory, string access_token) => "uploaded", + "upload", + "Upload a file to Dropbox"); + var tool = new McpToolAdapter(function, "Dropbox", "upload"); + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.AllowedMcpServers.Add("Dropbox"); + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + McpServerDefaults = new Dictionary(StringComparer.Ordinal) + { + ["Dropbox"] = ToolApprovalMode.Approval + } + }; + var policy = new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false)); + var executionContext = new ToolExecutionContext( + new ToolRunScope + { + Session = new ToolSessionScope.Bound("test-session", null), + Audience = TrustAudience.Personal, + InlineOutputBudget = InlineOutputBudget.Default, + InteractiveApproval = new InteractiveApprovalCapability.Unavailable() + }, + ToolExecutionTimeout.Default); + var decision = policy.AuthorizeInvocation( + tool, + executionContext, + new Dictionary + { + ["contents"] = content, + ["source_path"] = "/home/operator/reports/2026/Q3/quarterly-results-final.pdf", + ["destination_directory"] = "/Finance/Board Pack/2026/Q3", + ["access_token"] = "must-never-render", + ["_rationale"] = "Upload the requested board report" + }); + var approvalContext = Assert.IsType(decision.ApprovalContext); + var approval = BuildApproval(approvalContext.DisplayText, approvalContext.ToolName) with + { + Patterns = approvalContext.Patterns, + CandidateVerbs = approvalContext.CandidateVerbs, + Options = approvalContext.Options + .Select(option => new ToolInteractionOption(option.Key, option.Label)) + .ToArray() + }; + + var collapsed = await CaptureFrameAsync(approval, expand: false); + var expanded = await CaptureFrameAsync(approval, expand: true); + + WriteSnapshot("chat-mcp-approval-large-collapsed.txt", collapsed); + WriteSnapshot("chat-mcp-approval-large-expanded.txt", expanded); + + Assert.Contains("/Finance/Board Pack/2026/Q3", expanded); + Assert.Contains("quarterly-results-final.pdf", expanded); + Assert.Contains(content.Length.ToString(), expanded); + Assert.Contains("chars, 2000 lines", expanded); + Assert.Contains("MCP tool approval required", expanded); + Assert.Contains("Invocation:", expanded); + Assert.Contains(ApprovalOptionKeys.ApproveMcpToolLabel, expanded); + Assert.DoesNotContain("Patterns:", expanded); + Assert.DoesNotContain(ApprovalOptionKeys.ApproveEverywhereLabel, expanded); + Assert.DoesNotContain("large memorizer payload", expanded); + Assert.DoesNotContain("must-never-render", collapsed); + Assert.DoesNotContain("must-never-render", expanded); + Assert.DoesNotContain("_rationale", expanded); + Assert.DoesNotContain("Upload the requested board report", expanded); + } + private async Task CaptureFrameAsync(bool expand) + => await CaptureFrameAsync(BuildApproval(LongShellBody), expand); + + private async Task CaptureFrameAsync(ToolInteractionRequest approval, bool expand) { - var (terminal, app, _) = CreateHeadlessApp(BuildApproval(LongShellBody), out var input); + var (terminal, app, _) = CreateHeadlessApp(approval, out var input); if (expand) input.EnqueueKey(ConsoleKey.O, false, false, true); input.EnqueueKey(ConsoleKey.Q, false, false, true); @@ -363,7 +447,7 @@ private static void WriteSnapshot(string filename, string content) File.WriteAllText(Path.Combine(targetDir, filename), content); } - private static ToolInteractionRequest BuildApproval(string displayText) + private static ToolInteractionRequest BuildApproval(string displayText, string toolName = "shell_execute") { return new ToolInteractionRequest { @@ -371,7 +455,7 @@ private static ToolInteractionRequest BuildApproval(string displayText) TimestampMs = 0, Kind = "approval", CallId = new Netclaw.Tools.ToolCallId("test-call"), - ToolName = new Netclaw.Tools.ToolName("shell_execute"), + ToolName = new Netclaw.Tools.ToolName(toolName), DisplayText = displayText, Patterns = ["cd"], CandidateVerbs = ["cd"], diff --git a/src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-collapsed.txt b/src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-collapsed.txt new file mode 100644 index 000000000..4ab3447de --- /dev/null +++ b/src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-collapsed.txt @@ -0,0 +1,40 @@ +╭─Netclaw Chat─────────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│System: MCP tool approval required │ +│ Tool: Dropbox/upload │ +│Invocation: Dropbox/upload(source_path="/home/operator/reports/2026/Q3/quarterly-results-final.pdf", │ +│destination_directory="/Finance/Board Pack/2026/Q3", contents=(89999 chars, 2000 lines), access_token=***REDACTED***) │ +│ Options: Once, This chat, Always allow this tool, Deny │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─Input────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│MCP tool approval required: Dropbox/upload. │ +│Invocation: Dropbox/upload(source_path="/home/operator/reports/2026/Q3/quarterly-results-fina… [Ctrl+O to view full] │ +│Select an option and press Enter. │ +│1. Once │ +│2. This chat │ +│3. Always allow this tool │ +│4. Deny │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +[Up/Down] Select [Enter] Confirm [Ctrl+O] View full [PgUp/PgDn] Scroll [Ctrl+Q] Quit | Approval required | test-model \ No newline at end of file diff --git a/src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-expanded.txt b/src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-expanded.txt new file mode 100644 index 000000000..107d32af1 --- /dev/null +++ b/src/Netclaw.Cli.Tests/Tui/__snapshots__/chat-mcp-approval-large-expanded.txt @@ -0,0 +1,40 @@ +╭─Netclaw Chat─────────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│System: MCP tool approval required │ +│ Tool: Dropbox/upload │ +│Invocation: Dropbox/upload(source_path="/home/operator/reports/2026/Q3/quarterly-results-final.pdf", │ +│destination_directory="/Finance/Board Pack/2026/Q3", contents=(89999 chars, 2000 lines), access_token=***REDACTED***) │ +│ Options: Once, This chat, Always allow this tool, Deny │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +│ │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─Input────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│MCP tool approval required: Dropbox/upload. │ +│Invocation: Dropbox/upload(source_path="/home/operator/reports/2026/Q3/quarterly-results-final.pdf", │ +│destination_directory="/Finance/Board Pack/2026/Q3", contents=(89999 chars, 2000 lines), access_token=***REDACTED***) │ +│Select an option and press Enter. │ +│1. Once │ +│2. This chat │ +│3. Always allow this tool │ +│4. Deny │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +[Up/Down] Select [Enter] Confirm [Ctrl+O] Collapse [PgUp/PgDn] Scroll [Ctrl+Q] Quit | Approval required | test-model \ No newline at end of file diff --git a/src/Netclaw.Cli/Tui/ChatPage.cs b/src/Netclaw.Cli/Tui/ChatPage.cs index e4b88902a..8f4231170 100644 --- a/src/Netclaw.Cli/Tui/ChatPage.cs +++ b/src/Netclaw.Cli/Tui/ChatPage.cs @@ -462,9 +462,17 @@ private void HandleOutput(SessionOutput output) case ToolInteractionRequest msg: RemoveThinkingSpinner(); - _chatHistory.AppendLine($"System: Approval required for {msg.ToolName}", Color.Yellow); - _chatHistory.AppendLine($" {msg.DisplayText}", Color.White); - if (msg.Patterns.Count > 0) + _chatHistory.AppendLine( + msg.ToolName.IsMcp + ? "System: MCP tool approval required" + : $"System: Approval required for {msg.ToolName}", + Color.Yellow); + if (msg.ToolName.IsMcp) + _chatHistory.AppendLine($" Tool: {msg.ToolName}", Color.BrightBlack); + _chatHistory.AppendLine( + msg.ToolName.IsMcp ? $" Invocation: {msg.DisplayText}" : $" {msg.DisplayText}", + Color.White); + if (!msg.ToolName.IsMcp && msg.Patterns.Count > 0) _chatHistory.AppendLine($" Patterns: {string.Join(", ", msg.Patterns)}", Color.BrightBlack); _chatHistory.AppendLine($" Options: {string.Join(", ", msg.Options.Select(o => o.Label))}", Color.Yellow); _chatHistory.ScrollToBottom(); diff --git a/src/Netclaw.Cli/Tui/ChatViewModel.cs b/src/Netclaw.Cli/Tui/ChatViewModel.cs index 428e58199..6f463fb44 100644 --- a/src/Netclaw.Cli/Tui/ChatViewModel.cs +++ b/src/Netclaw.Cli/Tui/ChatViewModel.cs @@ -288,7 +288,9 @@ public string GetApprovalSummary() if (CurrentInteraction is not { } interaction) return "Approval required"; - return $"Approval required for {interaction.ToolName}."; + return interaction.ToolName.IsMcp + ? $"MCP tool approval required: {interaction.ToolName}." + : $"Approval required for {interaction.ToolName}."; } /// @@ -305,11 +307,12 @@ public string GetApprovalBody(int maxLineWidth) if (CurrentInteraction is not { } interaction) return string.Empty; - var patterns = interaction.Patterns.Count > 0 + var patterns = !interaction.ToolName.IsMcp && interaction.Patterns.Count > 0 ? $" Patterns: {string.Join(", ", interaction.Patterns)}" : string.Empty; - var fullBody = $"{interaction.DisplayText}{patterns}"; + var prefix = interaction.ToolName.IsMcp ? "Invocation: " : string.Empty; + var fullBody = $"{prefix}{interaction.DisplayText}{patterns}"; if (IsApprovalDetailVisible.Value) return Netclaw.Channels.ApprovalDisplayTextFormatter.Truncate(fullBody, MaxExpandedApprovalBodyChars); diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 3015d1bc7..3f68c194c 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Text.Json; using Netclaw.Configuration; using Netclaw.Tools; using Xunit; @@ -980,3 +981,199 @@ public void IsApproved_no_match() cwd: null)); } } + +public sealed class McpApprovalMatcherTests +{ + private readonly McpApprovalMatcher _matcher = McpApprovalMatcher.Instance; + + [Fact] + public void FormatForDisplay_without_arguments_returns_tool_name() + { + Assert.Equal( + "Dropbox/upload", + _matcher.FormatForDisplay(new ToolName("Dropbox/upload"), null)); + } + + [Fact] + public void FormatForDisplay_prioritizes_location_values_and_summarizes_large_strings() + { + var content = string.Join('\n', Enumerable.Repeat("memorizer payload line", 2_000)); + var display = _matcher.FormatForDisplay( + new ToolName("Dropbox/upload"), + new Dictionary + { + ["contents"] = content, + ["overwrite"] = true, + ["source_path"] = "/home/operator/reports/quarterly-results.pdf", + ["destination_directory"] = "/Finance/Board/2026/Q3" + }); + + Assert.Contains("source_path=\"/home/operator/reports/quarterly-results.pdf\"", display); + Assert.Contains("destination_directory=\"/Finance/Board/2026/Q3\"", display); + Assert.Contains($"contents=({content.Length} chars, 2000 lines)", display); + Assert.DoesNotContain("memorizer payload line", display); + Assert.True( + display.IndexOf("destination_directory", StringComparison.Ordinal) + < display.IndexOf("contents", StringComparison.Ordinal)); + Assert.True( + display.IndexOf("source_path", StringComparison.Ordinal) + < display.IndexOf("contents", StringComparison.Ordinal)); + } + + [Fact] + public void FormatForDisplay_handles_JsonElement_scalars_and_small_structures() + { + using var json = JsonDocument.Parse( + """{"mode":"replace","options":{"notify":true,"retries":3},"tags":["finance","board"]}"""); + var root = json.RootElement; + + var display = _matcher.FormatForDisplay( + new ToolName("Dropbox/upload"), + new Dictionary + { + ["mode"] = root.GetProperty("mode").Clone(), + ["options"] = root.GetProperty("options").Clone(), + ["tags"] = root.GetProperty("tags").Clone() + }); + + Assert.Contains("mode=\"replace\"", display); + Assert.Contains("options={\"notify\":true,\"retries\":3}", display); + Assert.Contains("tags=[\"finance\",\"board\"]", display); + Assert.DoesNotContain('\n', display); + } + + [Fact] + public void FormatForDisplay_redacts_top_level_nested_and_token_shaped_secrets() + { + using var json = JsonDocument.Parse( + """{"password":"nested-password","safe":"visible"}"""); + const string providerToken = "sk-1234567890abcdef"; + + var display = _matcher.FormatForDisplay( + new ToolName("service/create"), + new Dictionary + { + ["access_token"] = "plain-access-token", + ["options"] = json.RootElement.Clone(), + ["reference"] = providerToken + }); + + Assert.DoesNotContain("plain-access-token", display); + Assert.DoesNotContain("nested-password", display); + Assert.DoesNotContain(providerToken, display); + Assert.Contains("***REDACTED***", display); + Assert.Contains("visible", display); + } + + [Fact] + public void FormatForDisplay_preserves_both_ends_of_long_locator() + { + var path = "/source/" + new string('a', 1_100) + "/quarterly-results.pdf"; + + var display = _matcher.FormatForDisplay( + new ToolName("Dropbox/upload"), + new Dictionary { ["source_path"] = path }); + + Assert.Contains("/source/", display); + Assert.Contains("quarterly-results.pdf", display); + Assert.Contains($"[{path.Length} chars]", display); + } + + [Fact] + public void FormatForDisplay_redacts_uri_credentials_query_and_fragment() + { + const string signedUrl = "https://operator:password@example.com/reports/q3.pdf?signature=opaque-secret&expires=123#access-token"; + + var display = _matcher.FormatForDisplay( + new ToolName("Dropbox/upload"), + new Dictionary { ["callback"] = signedUrl }); + + Assert.Contains("https://example.com/reports/q3.pdf", display); + Assert.DoesNotContain("operator", display); + Assert.DoesNotContain("password", display); + Assert.DoesNotContain("opaque-secret", display); + Assert.DoesNotContain("access-token", display); + Assert.Contains("REDACTED", display); + } + + [Fact] + public void FormatForDisplay_escapes_untrusted_argument_names_and_inline_code_breakouts() + { + var display = _matcher.FormatForDisplay( + new ToolName("service/invoke"), + new Dictionary + { + ["safe\n```\u202E**Approve**"] = "value`spoof" + }); + + Assert.DoesNotContain('\n', display); + Assert.DoesNotContain('`', display); + Assert.DoesNotContain('\u202E', display); + Assert.Contains("\\u000A", display); + Assert.Contains("\\u0060", display); + Assert.Contains("\\u202E", display); + } + + [Fact] + public void FormatForDisplay_does_not_infer_value_sensitivity_from_argument_names() + { + var longValue = new string('x', 2_000); + var display = _matcher.FormatForDisplay( + new ToolName("service/invoke"), + new Dictionary + { + ["file_contents"] = longValue, + ["source_code"] = longValue, + ["target_payload"] = longValue + }); + + Assert.Equal(3, display.Split("2000 chars", StringSplitOptions.None).Length - 1); + Assert.DoesNotContain(new string('x', 50), display); + } + + [Fact] + public void FormatForDisplay_bounds_argument_and_collection_work() + { + using var json = JsonDocument.Parse( + $"[{string.Join(',', Enumerable.Range(0, 1_000))}]"); + var arguments = Enumerable.Range(0, 1_000) + .ToDictionary(i => $"argument_{i:D4}", i => (object?)json.RootElement.Clone()); + + var display = _matcher.FormatForDisplay(new ToolName("service/invoke"), arguments); + + Assert.True(display.Length <= 1_600); + Assert.Contains("12+ items", display); + Assert.Contains("arguments", display); + Assert.DoesNotContain("argument_0024", display); + } + + [Fact] + public void FormatForDisplay_bounds_oversized_names_and_redacts_their_values() + { + var oversizedKey = new string('k', 10_000); + var display = _matcher.FormatForDisplay( + new ToolName($"service/{new string('t', 10_000)}`\nspoof"), + new Dictionary { [oversizedKey] = "must-not-appear" }); + + Assert.True(display.Length <= 1_600); + Assert.DoesNotContain("must-not-appear", display); + Assert.DoesNotContain('`', display); + Assert.DoesNotContain('\n', display); + Assert.Contains("10015 chars", display); + Assert.Contains("10000\\u0020chars", display); + Assert.Contains("REDACTED", display); + } + + [Fact] + public void FormatForDisplay_reports_unserializable_value_without_throwing() + { + var value = new Func(() => 42); + + var display = _matcher.FormatForDisplay( + new ToolName("service/invoke"), + new Dictionary { ["callback"] = value }); + + Assert.Contains("value unavailable", display); + Assert.DoesNotContain("42", display); + } +} diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 3fd9a2c33..e23261422 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -3,7 +3,9 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Collections; using System.Text; +using System.Text.Json; using Netclaw.Configuration; using Netclaw.Tools; @@ -772,3 +774,365 @@ public bool IsMessy(ToolName toolName, IDictionary? arguments) public string FormatForDisplay(ToolName toolName, IDictionary? arguments) => toolName.Value; } + +/// +/// MCP approval matcher. Grant matching remains at the canonical tool-name +/// level, while the display text includes a bounded, redacted preview of the +/// server arguments so an operator can make an informed decision. +/// +public sealed class McpApprovalMatcher : IToolApprovalMatcher +{ + public static readonly McpApprovalMatcher Instance = new(); + + private const int MaxToolNameChars = 256; + private const int MaxArgumentNameChars = 160; + private const int MaxArguments = 24; + private const int MaxDisplayChars = 1_600; + private const int MaxNestedItems = 12; + private const int MaxNestedDepth = 3; + private const int MaxStructurePreviewChars = 360; + private const int MaxUrlParseChars = 4_096; + private const int MaxLineCountChars = 100_000; + private const int StandardStringPreviewChars = 240; + private const int LocatorStringPreviewChars = 1_000; + private const int LocatorPreviewHeadChars = 600; + private const int LocatorPreviewTailChars = 350; + private const string Redacted = "***REDACTED***"; + + private static DefaultApprovalMatcher Default => DefaultApprovalMatcher.Instance; + + public string GetApprovalModeKey(ToolName toolName, IDictionary? arguments) + => Default.GetApprovalModeKey(toolName, arguments); + + public bool IsFailClosedOnPersonal(ToolName toolName, IDictionary? arguments) + => Default.IsFailClosedOnPersonal(toolName, arguments); + + public IReadOnlyList ExtractPatterns(ToolName toolName, IDictionary? arguments) + => Default.ExtractPatterns(toolName, arguments); + + public IReadOnlyList ExtractCandidateVerbs(ToolName toolName, IDictionary? arguments) + => Default.ExtractCandidateVerbs(toolName, arguments); + + public IReadOnlyList ExtractCandidates( + ToolName toolName, + IDictionary? arguments) + => Default.ExtractCandidates(toolName, arguments); + + public bool IsApproved( + ToolName toolName, + IDictionary? arguments, + IReadOnlyList approvedEntries, + string? cwd) + => Default.IsApproved(toolName, arguments, approvedEntries, cwd); + + public bool IsMessy(ToolName toolName, IDictionary? arguments) + => Default.IsMessy(toolName, arguments); + + public string FormatForDisplay(ToolName toolName, IDictionary? arguments) + { + var displayToolName = FormatToolName(toolName.Value); + if (arguments is null || arguments.Count == 0) + return displayToolName; + + // MCP schemas may contain arbitrary property names and arbitrarily + // large collections. Sample before formatting so an approval prompt + // has a hard work and allocation ceiling even for a hostile server. + var sampled = arguments + .Take(MaxArguments) + .OrderBy(static pair => IsLocationLike(pair.Value) ? 0 : 1) + .ToList(); + + var display = new StringBuilder(Math.Min(MaxDisplayChars, displayToolName.Length + 256)); + display.Append(displayToolName).Append('('); + var rendered = 0; + + foreach (var pair in sampled) + { + var part = $"{FormatArgumentName(pair.Key)}={FormatArgument(pair.Key, pair.Value)}"; + var separatorChars = rendered == 0 ? 0 : 2; + if (display.Length + separatorChars + part.Length + 1 > MaxDisplayChars) + break; + + if (separatorChars > 0) + display.Append(", "); + + display.Append(part); + rendered++; + } + + var omitted = arguments.Count - rendered; + if (omitted > 0) + { + var omission = $"… (+{omitted} arguments)"; + var separator = rendered == 0 ? string.Empty : ", "; + var available = MaxDisplayChars - display.Length - 1; + if (separator.Length + omission.Length <= available) + display.Append(separator).Append(omission); + else if (available > 1) + display.Append('…'); + } + + display.Append(')'); + + return display.ToString(); + } + + private static string FormatArgument(string key, object? value) + { + if (SecretOutputRedactor.IsSecretKey(key) || value is SensitiveString) + return Redacted; + + if (value is byte[] bytes) + return $"({bytes.Length} bytes)"; + + if (value is string text) + return FormatString(text); + + if (value is IDictionary dictionary) + return $"({dictionary.Count} properties)"; + + if (value is ICollection collection) + return $"({collection.Count} items)"; + + if (value is IEnumerable) + return "(collection value)"; + + JsonElement element; + try + { + element = value is JsonElement json + ? json + : JsonSerializer.SerializeToElement(value, value?.GetType() ?? typeof(object)); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException) + { + return $"({value?.GetType().Name ?? "unknown"} value unavailable)"; + } + + return FormatJsonValue(key, element, depth: 0); + } + + private static string FormatJsonValue(string key, JsonElement value, int depth) + { + if (SecretOutputRedactor.IsSecretKey(key)) + return Redacted; + + return value.ValueKind switch + { + JsonValueKind.String => FormatString(value.GetString() ?? string.Empty), + JsonValueKind.Number => value.GetRawText(), + JsonValueKind.True => "true", + JsonValueKind.False => "false", + JsonValueKind.Null => "null", + JsonValueKind.Undefined => "(undefined)", + JsonValueKind.Object => FormatObject(value, depth), + JsonValueKind.Array => FormatArray(value, depth), + _ => "(unsupported value)" + }; + } + + private static string FormatString(string value) + { + if (TrySanitizeAbsoluteUri(value, out var sanitizedUri)) + return SerializeStringForDisplay(sanitizedUri); + + if (IsPathLike(value)) + return FormatPath(value); + + return FormatNonLocatorString(value); + } + + private static string FormatPath(string value) + { + if (value.Length <= LocatorStringPreviewChars) + return SerializeStringForDisplay(SecretOutputRedactor.Redact(value)); + + var preview = value[..LocatorPreviewHeadChars] + + $"…[{value.Length} chars]…" + + value[^LocatorPreviewTailChars..]; + return SerializeStringForDisplay(SecretOutputRedactor.Redact(preview)); + } + + private static string FormatNonLocatorString(string value) + { + if (value.Length <= StandardStringPreviewChars) + return SerializeStringForDisplay(SecretOutputRedactor.Redact(value)); + + return $"({value.Length} chars, {CountLinesForDisplay(value)} lines)"; + } + + private static string FormatObject(JsonElement value, int depth) + { + if (depth >= MaxNestedDepth) + return "(nested object)"; + + var properties = value.EnumerateObject().Take(MaxNestedItems + 1).ToList(); + if (properties.Count > MaxNestedItems) + return $"({MaxNestedItems}+ properties)"; + + var parts = properties + .OrderBy(static property => property.Name, StringComparer.Ordinal) + .Select(property => + $"{SerializeStringForDisplay(BoundArgumentName(property.Name))}:{FormatJsonValue(property.Name, property.Value, depth + 1)}"); + return BoundStructurePreview($"{{{string.Join(",", parts)}}}", properties.Count, "properties"); + } + + private static string FormatArray(JsonElement value, int depth) + { + if (depth >= MaxNestedDepth) + return "(nested array)"; + + var items = value.EnumerateArray().Take(MaxNestedItems + 1).ToList(); + if (items.Count > MaxNestedItems) + return $"({MaxNestedItems}+ items)"; + + var preview = $"[{string.Join(",", items.Select(item => FormatJsonValue(string.Empty, item, depth + 1)))}]"; + return BoundStructurePreview(preview, items.Count, "items"); + } + + private static string BoundStructurePreview(string preview, int count, string unit) + => preview.Length <= MaxStructurePreviewChars ? preview : $"({count} {unit})"; + + private static string FormatArgumentName(string key) + { + if (key.Length is > 0 and <= MaxArgumentNameChars && key.All(IsSafeArgumentNameChar)) + return key; + + // Approval displays are embedded in inline-code markup by the chat + // renderers. Encode every non-identifier code unit, including + // backticks, line breaks, ANSI controls, and bidi overrides, so an MCP + // schema cannot escape the invocation line or spoof prompt chrome. + var bounded = BoundArgumentName(key); + var escaped = new StringBuilder(bounded.Length + 2); + escaped.Append('"'); + foreach (var ch in bounded) + { + if (IsSafeArgumentNameChar(ch)) + escaped.Append(ch); + else + escaped.Append("\\u").Append(((int)ch).ToString("X4")); + } + + return escaped.Append('"').ToString(); + } + + private static string BoundArgumentName(string key) + => key.Length <= MaxArgumentNameChars + ? key + : $"{key[..120]}…[{key.Length} chars]"; + + private static string FormatToolName(string toolName) + { + var bounded = toolName.Length <= MaxToolNameChars + ? toolName + : $"{toolName[..200]}…[{toolName.Length} chars]"; + return EscapePresentationControls(bounded); + } + + private static bool IsSafeArgumentNameChar(char ch) + => ch is >= 'a' and <= 'z' + or >= 'A' and <= 'Z' + or >= '0' and <= '9' + or '_' or '-' or '.'; + + private static string SerializeStringForDisplay(string value) + { + var serialized = JsonSerializer.Serialize(value); + return EscapePresentationControls(serialized); + } + + private static string EscapePresentationControls(string value) + { + if (!value.Any(IsPresentationControl)) + return value; + + var escaped = new StringBuilder(value.Length); + foreach (var ch in value) + { + if (IsPresentationControl(ch)) + escaped.Append("\\u").Append(((int)ch).ToString("X4")); + else + escaped.Append(ch); + } + + return escaped.ToString(); + } + + private static bool IsPresentationControl(char ch) + => ch == '`' || char.IsControl(ch) || IsDirectionalOrLineControl(ch); + + private static bool IsDirectionalOrLineControl(char ch) + => ch is '\u061C' or '\u200E' or '\u200F' + or >= '\u2028' and <= '\u202E' + or >= '\u2066' and <= '\u2069' + or '\uFEFF'; + + private static bool TrySanitizeAbsoluteUri(string value, out string sanitized) + { + sanitized = string.Empty; + if (value.Length > MaxUrlParseChars + || !Uri.TryCreate(value, UriKind.Absolute, out var uri) + || string.IsNullOrEmpty(uri.Host)) + { + return false; + } + + try + { + var builder = new UriBuilder(uri) + { + UserName = string.Empty, + Password = string.Empty, + Query = string.IsNullOrEmpty(uri.Query) ? string.Empty : Redacted, + Fragment = string.IsNullOrEmpty(uri.Fragment) ? string.Empty : Redacted + }; + sanitized = SecretOutputRedactor.Redact(builder.Uri.AbsoluteUri); + return true; + } + catch (UriFormatException) + { + return false; + } + } + + private static bool IsPathLike(string value) + { + if (value.Length == 0 || value.IndexOfAny(['\r', '\n']) >= 0) + return false; + + return Path.IsPathRooted(value) + || value.StartsWith("~/", StringComparison.Ordinal) + || value.StartsWith("./", StringComparison.Ordinal) + || value.StartsWith("../", StringComparison.Ordinal) + || value.StartsWith("\\\\", StringComparison.Ordinal) + || value.Length >= 3 && char.IsAsciiLetter(value[0]) && value[1] == ':' + && value[2] is '\\' or '/'; + } + + private static bool IsLocationLike(object? value) + { + var text = value switch + { + string stringValue => stringValue, + JsonElement { ValueKind: JsonValueKind.String } jsonValue => jsonValue.GetString(), + _ => null + }; + + return text is not null + && (IsPathLike(text) || TrySanitizeAbsoluteUri(text, out _)); + } + + private static string CountLinesForDisplay(string value) + { + var lines = 1; + var charsToInspect = Math.Min(value.Length, MaxLineCountChars); + for (var i = 0; i < charsToInspect; i++) + { + if (value[i] == '\n') + lines++; + } + + return value.Length <= MaxLineCountChars ? lines.ToString() : $"{lines}+"; + } + +} diff --git a/src/Netclaw.Security/SecretOutputRedactor.cs b/src/Netclaw.Security/SecretOutputRedactor.cs index 75c37c89b..79b9a6e1d 100644 --- a/src/Netclaw.Security/SecretOutputRedactor.cs +++ b/src/Netclaw.Security/SecretOutputRedactor.cs @@ -13,8 +13,35 @@ namespace Netclaw.Security; /// public static partial class SecretOutputRedactor { + private const int MaxSecretKeyChars = 512; private const string Redacted = "***REDACTED***"; + private static readonly string[] SecretKeyFragments = + [ + "apikey", + "token", + "secret", + "password", + "authorization", + "credential", + "privatekey", + "signingkey", + "connectionstring" + ]; + + internal static bool IsSecretKey(string key) + { + // A hostile MCP schema can supply arbitrarily large property names. + // Fail closed instead of allocating an equally large normalized key + // just to decide whether its value is safe to display. + if (key.Length > MaxSecretKeyChars) + return true; + + var normalized = Netclaw.Tools.ToolArgumentHelper.NormalizeKey(key); + return SecretKeyFragments.Any(fragment => + normalized.Contains(fragment, StringComparison.OrdinalIgnoreCase)); + } + public static bool ContainsSecretLikeContent(string output) { if (string.IsNullOrEmpty(output)) diff --git a/src/Netclaw.Tools.Abstractions/ToolName.cs b/src/Netclaw.Tools.Abstractions/ToolName.cs index c12080888..c44b16432 100644 --- a/src/Netclaw.Tools.Abstractions/ToolName.cs +++ b/src/Netclaw.Tools.Abstractions/ToolName.cs @@ -12,6 +12,13 @@ namespace Netclaw.Tools; /// public readonly record struct ToolName(string Value) { + /// + /// MCP tools use the canonical operator-facing {server}/{tool} + /// identity. First-party tool names never contain the separator. + /// + public bool IsMcp => !string.IsNullOrEmpty(Value) + && Value.IndexOf('/', StringComparison.Ordinal) > 0; + public static explicit operator ToolName(string value) => new(value); public override string ToString() => Value; From dac46f9a45e085304c09e5f4cc6d766ebfc1164f Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 18 Jul 2026 12:47:46 -0500 Subject: [PATCH 11/12] feat(install): automate shell PATH integration (#1687) * feat(install): automate shell PATH integration for Unix installers Replace manual PATH instructions with automatic shell profile modification. The installer now detects the user's shell and writes a self-guarding env script (~/.netclaw/env) that is sourced from the appropriate RC file. Key changes: - Add --skip-shell flag to opt out of automatic shell modification - Detect shell via $SHELL (bash, zsh, fish supported; others get manual instructions) - Write ~/.netclaw/env with colon-affixed case guard (rustup/fzf pattern) - Append source line to correct RC: ~/.bashrc (Linux), ~/.profile (macOS bash), ~/.zshrc (zsh with ZDOTDIR support), ~/.config/fish/conf.d/netclaw.fish (fish) - Duplicate prevention: grep -xF guard before appending to RC - RC file validated with bash -n after modification Smoke tests (section 9): - Separate tests for bash-linux, zsh, and fish RC modification - --skip-shell flag: verify no RC/env file created - Unknown shell: verify graceful fallback message - ZDOTDIR: verify zsh respects ZDOTDIR for RC location - Duplicate prevention: second install adds no extra source lines * feat(install): automate PATH modification for Windows installer Replace manual PATH instructions with automatic User-scope PATH modification. The installer now prepends the install dir to the User PATH and broadcasts WM_SETTINGCHANGE to Explorer so new terminal windows pick up the change. Key changes: - Add -SkipShell switch to opt out of automatic PATH modification - Read User-scope PATH (not merged $env:PATH) to avoid Machine entry corruption - Prepend install dir to User PATH (highest priority) - Update current session's $env:PATH - Broadcast WM_SETTINGCHANGE via P/Invoke SendMessageTimeout - Guard against duplicates: normalize trailing backslash before comparison - Check 32K character limit before writing, warn if near limit Smoke tests: - PATH automation logic: duplicate detection with fake PATH strings - Trailing backslash normalization - Null User PATH handled as empty - -SkipShell flag: verify install completes with skipped message * fix(smoke): isolate real install test to temp HOME The real install test now uses a temp HOME directory so shell integration writes to the temp dir instead of the CI runner's real profile. This also lets us verify the env script was created and the RC file was modified. Adds 2 new assertions: env script exists, RC file sources env script. * fix(smoke): cross-platform install smoke test fixes - bash test: select .profile on macOS (login shell) vs .bashrc on Linux - fish test: unset XDG_CONFIG_HOME so config path stays within temp HOME - fish test: add duplicate prevention and re-install assertions - Windows: remove false-positive regex check on success message * fix(smoke): use @() array coercion for empty PATH entries count $entries4 is a 0-element array which PowerShell can coerce to $null in some contexts, causing '$entries4.Count' to throw ParentContainsErrorRecordException. $(@entries4).Count forces array coercion so .Count always works. * fix(install): validate PATH integration through real shells * fix(install): make manifest fallback portable * Harden installer PATH mutations * fix(smoke): compare persisted physical paths * fix(installer): harden shell path mutation * test(installer): support Windows PowerShell path joins * test(installer): normalize manifest fixture encoding * test(installer): capture expected PowerShell failures --- .github/workflows/pr_validation.yml | 9 + README.md | 11 +- scripts/install.ps1 | 159 +++++++++++++++-- scripts/install.sh | 255 +++++++++++++++++++++++--- scripts/smoke/install-smoke.ps1 | 246 +++++++++++++++++++++---- scripts/smoke/install-smoke.sh | 266 +++++++++++++++++++++++++++- 6 files changed, 874 insertions(+), 72 deletions(-) diff --git a/.github/workflows/pr_validation.yml b/.github/workflows/pr_validation.yml index 43a77cf3d..666ac34e7 100644 --- a/.github/workflows/pr_validation.yml +++ b/.github/workflows/pr_validation.yml @@ -184,6 +184,10 @@ jobs: with: fetch-depth: 1 + - name: "Install shell integration test dependencies" + if: runner.os == 'Linux' + run: sudo apt-get update && sudo apt-get install --yes fish zsh + - name: "install.sh smoke test" if: runner.os != 'Windows' shell: bash @@ -194,6 +198,11 @@ jobs: shell: pwsh run: ./scripts/smoke/install-smoke.ps1 + - name: "install.ps1 Windows PowerShell 5.1 smoke test" + if: runner.os == 'Windows' + shell: powershell + run: ./scripts/smoke/install-smoke.ps1 + semver-conformance: name: SemVer Conformance runs-on: ubuntu-latest diff --git a/README.md b/README.md index fdff5673b..b98f73e20 100644 --- a/README.md +++ b/README.md @@ -100,8 +100,14 @@ curl -sSL https://releases.netclaw.dev/install.sh | bash -s -- --channel beta # Pin a specific version (e.g. a prerelease) NETCLAW_VERSION=0.17.1 curl -sSL https://releases.netclaw.dev/install.sh | bash + +# Install without modifying your shell profile +curl -sSL https://releases.netclaw.dev/install.sh | bash -s -- --skip-shell ``` +By default, the Unix installer updates the detected Bash, zsh, or fish startup +configuration so new shells include Netclaw on `PATH`. + **macOS** (Apple Silicon — M1 or later — installs CLI + daemon to `~/.netclaw/bin`): ```bash @@ -119,8 +125,9 @@ available on macOS ([#1015](https://github.com/netclaw-dev/netclaw/issues/1015)) iwr -useb https://releases.netclaw.dev/install.ps1 | iex ``` -The `-Component cli|daemon`, `-Channel beta`, and `-Version` options work the same -way as their Linux counterparts (download the script and run it with the flag). +The installer adds Netclaw to your User PATH. The `-Component cli|daemon`, +`-Channel beta`, `-Version`, and `-SkipShell` options work the same way as their +Linux counterparts (download the script and run it with the flag). **Docker** (multi-arch: amd64/arm64): diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 5db509d7f..3633a3d86 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -7,6 +7,7 @@ # .\install.ps1 -InstallDir C:\tools\netclaw # .\install.ps1 -Channel beta # Opt into prereleases # .\install.ps1 -DryRun +# .\install.ps1 -SkipShell # Don't modify PATH # # -Channel beta installs the newest prerelease (or latest stable if no prerelease # exists). -Version pins an exact version and overrides -Channel (e.g. 0.19.0-beta.1). @@ -24,11 +25,37 @@ param( [string]$Channel = "stable", # Resolve and report what would be installed, but install nothing. - [switch]$DryRun + [switch]$DryRun, + + # Skip automatic PATH modification. + [switch]$SkipShell ) $ErrorActionPreference = "Stop" +function Remove-TrailingDirectorySeparators { + param([string]$Path) + + if ([string]::IsNullOrEmpty($Path)) { + return $Path + } + + $root = [System.IO.Path]::GetPathRoot($Path) + if ($Path -eq $root) { + return $Path + } + + $separators = [char[]]@( + [System.IO.Path]::DirectorySeparatorChar, + [System.IO.Path]::AltDirectorySeparatorChar) + $trimmed = $Path.TrimEnd($separators) + if ([string]::IsNullOrEmpty($trimmed) -and -not [string]::IsNullOrEmpty($root)) { + return $root + } + + return $trimmed +} + function Invoke-DownloadWithProgress { param( [string]$Uri, @@ -94,6 +121,11 @@ if (-not $InstallDir) { $InstallDir = $DefaultInstallDir } +$InstallDir = [System.IO.Path]::GetFullPath($InstallDir) +if ($InstallDir.Contains(';') -or $InstallDir.Contains("`r") -or $InstallDir.Contains("`n")) { + throw "InstallDir cannot contain semicolons, carriage returns, or newlines when used on PATH." +} + Write-Host "Netclaw installer" Write-Host " Platform: win-x64" Write-Host " Install dir: $InstallDir" @@ -256,21 +288,126 @@ try { } } - # Check PATH and offer to add if missing + # ── Add to PATH ── Write-Host "" - $userPath = [Environment]::GetEnvironmentVariable("PATH", "User") - $pathEntries = $userPath -split ';' | ForEach-Object { $_.TrimEnd('\') } - $installDirNormalized = $InstallDir.TrimEnd('\') - if ($pathEntries -contains $installDirNormalized) { - Write-Host "Installation complete! netclaw is already on your PATH." + if (-not $SkipShell) { + $installDirNormalized = Remove-TrailingDirectorySeparators $InstallDir + + # Read the raw registry value so an existing REG_EXPAND_SZ PATH keeps both + # its %VAR% references and its registry type when we prepend Netclaw. + # HKCU is writable by the current user and does not require elevation. + # CreateSubKey opens the normal existing key and also supports minimal + # profiles where the per-user Environment key has not been created yet. + $userEnvironmentKey = [Microsoft.Win32.Registry]::CurrentUser.CreateSubKey("Environment") + if ($null -eq $userEnvironmentKey) { + throw "Cannot create or open the current user's Environment registry key for PATH update." + } + + try { + $pathValueExists = $userEnvironmentKey.GetValueNames() -contains "Path" + $userPath = if ($pathValueExists) { + $userEnvironmentKey.GetValue( + "Path", + $null, + [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames) + } else { + "" + } + + if ($null -ne $userPath -and $userPath -isnot [string]) { + throw "The current user's PATH registry value is not a string." + } + + $userPathKind = if ($pathValueExists) { + $userEnvironmentKey.GetValueKind("Path") + } else { + [Microsoft.Win32.RegistryValueKind]::String + } + if ($userPathKind -notin @( + [Microsoft.Win32.RegistryValueKind]::String, + [Microsoft.Win32.RegistryValueKind]::ExpandString)) { + throw "The current user's PATH registry value has unsupported type $userPathKind." + } + + if ($userPathKind -eq [Microsoft.Win32.RegistryValueKind]::ExpandString ` + -and $installDirNormalized.Contains('%')) { + throw "InstallDir containing '%' cannot be safely added to an expandable User PATH. Choose a directory without '%' or rerun with -SkipShell." + } + + $userPathEntries = if ([string]::IsNullOrEmpty($userPath)) { @() } else { + $userPath -split ';' | + Where-Object { -not [string]::IsNullOrEmpty($_) } | + ForEach-Object { + $expandedEntry = [Environment]::ExpandEnvironmentVariables($_) + Remove-TrailingDirectorySeparators $expandedEntry + } + } + + $userPathChanged = $false + if ($userPathEntries -notcontains $installDirNormalized) { + $newUserPath = if ([string]::IsNullOrEmpty($userPath)) { + $installDirNormalized + } else { + "$installDirNormalized;$userPath" + } + + if ($newUserPath.Length -gt 32700) { + Write-Warning "User PATH is near its 32,767 character limit ($($newUserPath.Length) chars)." + Write-Host "Please manually add $InstallDir to your User PATH." + } else { + $userEnvironmentKey.SetValue("Path", $newUserPath, $userPathKind) + $userPathChanged = $true + } + } + } finally { + $userEnvironmentKey.Dispose() + } + + $processPath = $env:PATH + $processPathEntries = if ([string]::IsNullOrEmpty($processPath)) { @() } else { + $processPath -split ';' | + Where-Object { -not [string]::IsNullOrEmpty($_) } | + ForEach-Object { Remove-TrailingDirectorySeparators $_ } + } + if ($processPathEntries -notcontains $installDirNormalized) { + $env:PATH = if ([string]::IsNullOrEmpty($processPath)) { + $installDirNormalized + } else { + "$installDirNormalized;$processPath" + } + } + + if ($userPathChanged) { + if (-not ("NetclawInstaller.NativeMethods" -as [type])) { + Add-Type -Namespace NetclawInstaller -Name NativeMethods -MemberDefinition @' + [DllImport("user32.dll", SetLastError=true, CharSet=CharSet.Auto)] + public static extern IntPtr SendMessageTimeout(IntPtr hWnd, uint msg, + UIntPtr wParam, string lParam, uint fuFlags, uint uTimeout, out UIntPtr lpdwResult); +'@ + } + + $broadcastOutput = [UIntPtr]::Zero + $broadcastResult = [NetclawInstaller.NativeMethods]::SendMessageTimeout( + [IntPtr]0xFFFF, 0x001A, [UIntPtr]::Zero, "Environment", 2, 1000, [ref]$broadcastOutput) + if ($broadcastResult -eq [IntPtr]::Zero) { + Write-Warning "User PATH was updated, but Windows did not acknowledge the environment-change notification. New terminals may require sign-out or restart." + } + } + + if ($userPathEntries -contains $installDirNormalized) { + Write-Host "Installation complete! netclaw is already on your User PATH." + } elseif ($userPathChanged) { + Write-Host "Installation complete! netclaw was added to your User PATH." + } else { + Write-Host "Installation complete! netclaw is on PATH for this terminal only." + } } else { - Write-Host "Installation complete!" + Write-Host "Installation complete! (PATH modification skipped)" Write-Host "" - Write-Host "Add Netclaw to your PATH by running:" + Write-Host "Add this directory to your User PATH using Windows Environment Variables settings:" Write-Host "" - Write-Host " `$userPath = [Environment]::GetEnvironmentVariable('PATH', 'User')" - Write-Host " [Environment]::SetEnvironmentVariable('PATH', `"$InstallDir;`$userPath`", 'User')" + Write-Host " $InstallDir" Write-Host "" Write-Host "Then restart your terminal." } diff --git a/scripts/install.sh b/scripts/install.sh index bff6252a6..7f9b4ca74 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -6,6 +6,7 @@ # curl -sSL https://releases.netclaw.dev/install.sh | bash -s -- cli # CLI only # curl -sSL https://releases.netclaw.dev/install.sh | bash -s -- daemon # Daemon only # curl -sSL https://releases.netclaw.dev/install.sh | bash -s -- --channel beta # Opt into prereleases +# curl -sSL https://releases.netclaw.dev/install.sh | bash -s -- --skip-shell # Don't modify shell profile # INSTALL_DIR=/opt/netclaw curl -sSL https://releases.netclaw.dev/install.sh | bash # # Arguments: @@ -13,6 +14,7 @@ # --channel stable|beta — Release channel (default: stable). 'beta' installs the # newest prerelease (or latest stable if no prerelease exists). # --dry-run — Resolve and report what would happen; install nothing. +# --skip-shell — Skip automatic shell profile modification. # # Environment variables: # INSTALL_DIR — Install directory (default: ~/.netclaw/bin) @@ -36,9 +38,11 @@ COMPONENT="all" # "all", "cli", or "daemon" DRY_RUN=false # --dry-run: resolve and report what would happen, install nothing CHANNEL="stable" # release channel: "stable" (default) or "beta" (opt into prereleases) CHANNEL_EXPLICIT=false # true when --channel was explicitly passed +SKIP_SHELL=false # --skip-shell: don't modify shell profile while [ $# -gt 0 ]; do case "$1" in --dry-run) DRY_RUN=true; shift ;; + --skip-shell) SKIP_SHELL=true; shift ;; --channel) if [ $# -lt 2 ]; then echo "Error: --channel requires a value (stable|beta)" >&2; exit 1 @@ -46,7 +50,7 @@ while [ $# -gt 0 ]; do CHANNEL="$2"; CHANNEL_EXPLICIT=true; shift 2 ;; --channel=*) CHANNEL="${1#*=}"; CHANNEL_EXPLICIT=true; shift ;; all|cli|daemon) COMPONENT="$1"; shift ;; - *) echo "Usage: install.sh [all|cli|daemon] [--channel stable|beta] [--dry-run]" >&2; exit 1 ;; + *) echo "Usage: install.sh [all|cli|daemon] [--channel stable|beta] [--dry-run] [--skip-shell]" >&2; exit 1 ;; esac done @@ -133,11 +137,61 @@ json_field() { fi } +json_asset_field() { + local json="$1" version="$2" component="$3" rid="$4" field="$5" + + # Release manifests contain flat release and asset objects. Splitting object + # boundaries lets POSIX awk select an asset without depending on property order + # or GNU-only regular-expression extensions. + printf '%s\n' "$json" | tr '{' '\n' | tr '}' '\n' | awk \ + -v wanted_version="$version" \ + -v wanted_component="$component" \ + -v wanted_rid="$rid" \ + -v wanted_field="$field" ' + function string_field(record, name, value) { + if (index(record, "\"" name "\"") == 0) { + return "" + } + + value = record + sub(".*\"" name "\"[[:space:]]*:[[:space:]]*\"", "", value) + sub("\".*", "", value) + return value + } + + { + release_version = string_field($0, "version") + if (release_version != "") { + current_version = release_version + } + + if (current_version == wanted_version && + string_field($0, "component") == wanted_component && + string_field($0, "rid") == wanted_rid) { + print string_field($0, wanted_field) + exit + } + } + ' +} + +validate_install_dir_for_path() { + local install_dir="$1" + + # PATH uses ':' as its entry separator, and startup files are line-oriented. + # These names cannot be represented without changing their meaning. + if [[ "$install_dir" == *:* || "$install_dir" == *$'\n'* || "$install_dir" == *$'\r'* ]]; then + echo "Error: INSTALL_DIR cannot contain ':', carriage returns, or newlines when used on PATH." >&2 + return 1 + fi +} + # ── Main ── check_deps RID=$(detect_platform) INSTALL_DIR="${INSTALL_DIR:-$HOME/.netclaw/bin}" +validate_install_dir_for_path "$INSTALL_DIR" echo "Netclaw installer" echo " Platform: $RID" @@ -191,16 +245,8 @@ download_component() { url=$(echo "$MANIFEST" | jq -r ".releases[] | select(.version==\"$VERSION\") | .assets[] | select(.component==\"$component\" and .rid==\"$RID\") | .url") sha256=$(echo "$MANIFEST" | jq -r ".releases[] | select(.version==\"$VERSION\") | .assets[] | select(.component==\"$component\" and .rid==\"$RID\") | .sha256") else - # Fallback: extract URL and sha256 using grep (fragile but works for well-formed JSON) - # Find the block for this component+rid - local block - block=$(echo "$MANIFEST" | tr '\n' ' ' | grep -oP "\"component\"\\s*:\\s*\"${component}\"[^}]*\"rid\"\\s*:\\s*\"${RID}\"[^}]*}" | head -1) - if [ -z "$block" ]; then - # Try reversed order - block=$(echo "$MANIFEST" | tr '\n' ' ' | grep -oP "\"rid\"\\s*:\\s*\"${RID}\"[^}]*\"component\"\\s*:\\s*\"${component}\"[^}]*}" | head -1) - fi - url=$(echo "$block" | grep -oP '"url"\s*:\s*"\K[^"]+') - sha256=$(echo "$block" | grep -oP '"sha256"\s*:\s*"\K[^"]+') + url=$(json_asset_field "$MANIFEST" "$VERSION" "$component" "$RID" "url") + sha256=$(json_asset_field "$MANIFEST" "$VERSION" "$component" "$RID" "sha256") fi if [ -z "$url" ] || [ "$url" = "null" ]; then @@ -246,12 +292,19 @@ download_component() { return 1 fi - mkdir -p "$INSTALL_DIR" cp "$binary_path" "$INSTALL_DIR/$binary_name" chmod +x "$INSTALL_DIR/$binary_name" echo " Installed $binary_name to $INSTALL_DIR/" } +if [ "$DRY_RUN" = false ]; then + # Resolve symlinks before installing so the exact path persisted into shell + # startup files is the same path that passed delimiter validation. + mkdir -p "$INSTALL_DIR" + INSTALL_DIR="$(cd "$INSTALL_DIR" && pwd -P)" + validate_install_dir_for_path "$INSTALL_DIR" +fi + # Download requested components SUCCESS=true if [[ "$COMPONENT" == "all" || "$COMPONENT" == "cli" ]]; then @@ -304,20 +357,178 @@ if [ "$CHANNEL_EXPLICIT" = true ]; then fi fi -# PATH instructions -echo "" -if echo "$PATH" | tr ':' '\n' | grep -qx "$INSTALL_DIR"; then - echo "Installation complete! netclaw is already on your PATH." -else - echo "Installation complete!" +# ── Shell integration ───────────────────────────────────────────────────── +# Bash and zsh source a small POSIX env file. Fish gets native syntax in its +# dedicated conf.d file; fish cannot source POSIX `case ... esac` syntax. +ENV_SCRIPT="$HOME/.netclaw/env" + +shell_quote() { + printf "'" + printf '%s' "$1" | sed "s/'/'\\\\''/g" + printf "'" +} + +INSTALL_DIR_QUOTED="$(shell_quote "$INSTALL_DIR")" +ENV_SCRIPT_QUOTED="$(shell_quote "$ENV_SCRIPT")" +SOURCE_LINE=". $ENV_SCRIPT_QUOTED" +MANUAL_PATH_LINE="export PATH=$INSTALL_DIR_QUOTED\${PATH:+:\"\$PATH\"}" + +detect_shell() { + # $SHELL is inherited from the parent login shell — it reflects the user's + # configured shell even when this script is piped via `curl | bash`. + local shell_name + shell_name="$(basename "${SHELL:-/bin/sh}")" + echo "$shell_name" +} + +get_rc_file() { + local shell_name="$1" shell_path="$2" + local os + os="$(uname -s | tr '[:upper:]' '[:lower:]')" + + case "$shell_name" in + zsh) + local effective_zdotdir + # ZDOTDIR is often assigned without export in ~/.zshenv, so ask zsh + # for the value it actually uses rather than relying on Bash's env. + effective_zdotdir="$("$shell_path" -c "printf '%s' \"\${ZDOTDIR:-\$HOME}\"")" || return 1 + if [[ -z "$effective_zdotdir" || "$effective_zdotdir" != /* || \ + "$effective_zdotdir" == *$'\n'* || "$effective_zdotdir" == *$'\r'* ]]; then + return 1 + fi + echo "$effective_zdotdir/.zshrc" + ;; + bash) + if [ "$os" = "darwin" ]; then + # A login shell reads only the first existing file in this list. + if [ -f "$HOME/.bash_profile" ]; then + echo "$HOME/.bash_profile" + elif [ -f "$HOME/.bash_login" ]; then + echo "$HOME/.bash_login" + else + echo "$HOME/.profile" + fi + else + echo "$HOME/.bashrc" + fi + ;; + *) + echo "" + ;; + esac +} + +write_posix_env_script() { + mkdir -p "$(dirname "$ENV_SCRIPT")" + cat > "$ENV_SCRIPT" </dev/null; then + echo " Shell profile '$rc_file' already sources netclaw." + return 0 + fi + + if [ -s "$rc_file" ] && [ "$(tail -c1 "$rc_file" | wc -l)" -eq 0 ]; then + echo "" >> "$rc_file" + fi + + { + echo "# netclaw shell setup" + echo "$SOURCE_LINE" + } >> "$rc_file" + + echo " Modified '$rc_file' to add netclaw to PATH." +} + +write_fish_config() { + local fish_config_dir="${XDG_CONFIG_HOME:-$HOME/.config}/fish/conf.d" + local fish_config="$fish_config_dir/netclaw.fish" + mkdir -p "$fish_config_dir" + cat > "$fish_config" < checksum -> extract -> install path, plus -DryRun. # # Usage: pwsh -File scripts/smoke/install-smoke.ps1 -# Requires: PowerShell 7+, python (for the local HTTP server). +# powershell.exe -File scripts/smoke/install-smoke.ps1 +# Requires: PowerShell 5.1+ and python (for the local HTTP server). Set-StrictMode -Version Latest $ErrorActionPreference = "Stop" -$RepoRoot = (Resolve-Path (Join-Path $PSScriptRoot ".." "..")).Path -$InstallPs1 = Join-Path $RepoRoot "scripts" "install.ps1" +$PowerShellExecutable = if ($PSVersionTable.PSEdition -eq "Desktop") { + (Get-Command powershell.exe).Source +} else { + (Get-Command pwsh).Source +} + +$RepoRoot = (Resolve-Path (Join-Path (Join-Path $PSScriptRoot "..") "..")).Path +$InstallPs1 = Join-Path (Join-Path $RepoRoot "scripts") "install.ps1" $Version = "0.0.0" # stable -> manifest.latest $BetaVersion = "0.0.1-beta1" # prerelease -> manifest.latestPrerelease $Rid = "win-x64" @@ -22,9 +29,68 @@ $script:Fail = 0 function Pass([string]$m) { Write-Host "PASS: $m"; $script:Pass++ } function Fail([string]$m) { Write-Host "FAIL: $m"; $script:Fail++ } +function Invoke-CapturedPowerShell { + param([string[]]$Arguments) + + # Windows PowerShell 5.1 promotes a native child's stderr to an error record. + # Rejection tests need to inspect that output and exit code without aborting. + $previousErrorActionPreference = $ErrorActionPreference + try { + $ErrorActionPreference = "Continue" + $output = & $PowerShellExecutable @Arguments 2>&1 | Out-String + [PSCustomObject]@{ Output = $output; ExitCode = $LASTEXITCODE } + } finally { + $ErrorActionPreference = $previousErrorActionPreference + } +} + +function Get-UserPathRegistryState { + $key = [Microsoft.Win32.Registry]::CurrentUser.OpenSubKey("Environment", $false) + if ($null -eq $key) { throw "Cannot open the current user's Environment registry key." } + try { + $exists = $key.GetValueNames() -contains "Path" + [PSCustomObject]@{ + Exists = $exists + Value = if ($exists) { + $key.GetValue( + "Path", + $null, + [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames) + } else { + $null + } + Kind = if ($exists) { $key.GetValueKind("Path") } else { $null } + } + } finally { + $key.Dispose() + } +} + +function Set-UserPathRegistryState { + param( + [bool]$Exists, + [AllowNull()][string]$Value, + [AllowNull()][Microsoft.Win32.RegistryValueKind]$Kind + ) + + $key = [Microsoft.Win32.Registry]::CurrentUser.CreateSubKey("Environment") + if ($null -eq $key) { throw "Cannot create or open the current user's Environment registry key for update." } + try { + if ($Exists) { + $key.SetValue("Path", $Value, $Kind) + } else { + $key.DeleteValue("Path", $false) + } + } finally { + $key.Dispose() + } +} + $Work = Join-Path ([System.IO.Path]::GetTempPath()) ("netclaw-install-smoke-" + [Guid]::NewGuid().ToString('N')) $Serve = Join-Path $Work "serve" $BinDir = Join-Path $Work "bin" +$OriginalUserPath = Get-UserPathRegistryState +$OriginalProcessPath = $env:PATH New-Item -ItemType Directory -Path $Serve, $BinDir -Force | Out-Null $ServerProc = $null @@ -74,7 +140,9 @@ try { latestPrerelease = $BetaVersion releases = @($betaEntry, $stableEntry) } - $manifest | ConvertTo-Json -Depth 8 | Set-Content -Path (Join-Path $Serve "manifest.json") -Encoding utf8 + # The fixture is ASCII-only. Windows PowerShell 5.1 writes a BOM for + # -Encoding UTF8 while PowerShell 7 does not, so use an identical encoding. + $manifest | ConvertTo-Json -Depth 8 | Set-Content -Path (Join-Path $Serve "manifest.json") -Encoding ascii # 4. Serve the manifest + archives from localhost $python = Get-Command python3 -ErrorAction SilentlyContinue @@ -103,7 +171,7 @@ try { # 5. Dry-run check - resolves assets, installs nothing Write-Host "=== dry run ===" $dryDir = Join-Path $Work "dryrun-none" - $dryOut = & pwsh -NoProfile -File $InstallPs1 -InstallDir $dryDir -DryRun 2>&1 | Out-String + $dryOut = & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $dryDir -DryRun 2>&1 | Out-String Write-Host ($dryOut.TrimEnd()) if ($LASTEXITCODE -eq 0 ` -and $dryOut -match 'DRY RUN: would install netclaw ' ` @@ -121,14 +189,21 @@ try { # 6. Real install of the stand-in archives Write-Host "" Write-Host "=== real install ===" - $installDir = Join-Path $Work "installed" - $installOut = & pwsh -NoProfile -File $InstallPs1 -InstallDir $installDir 2>&1 | Out-String - Write-Host ($installOut.TrimEnd()) - if ($LASTEXITCODE -eq 0) { - Pass "install: exited 0" + + $invalidResult = Invoke-CapturedPowerShell -Arguments @( + "-NoProfile", "-File", $InstallPs1, + "-InstallDir", (Join-Path $Work "invalid;path"), "-DryRun") + if ($invalidResult.ExitCode -ne 0 -and $invalidResult.Output -match "cannot contain semicolons") { + Pass "PATH: unrepresentable Windows install directory rejected" } else { - Fail "install: exited $LASTEXITCODE" + Fail "PATH: Windows install directory containing ';' was accepted" } + + $installDir = Join-Path $Work "installed" + $existingUserEntry = Join-Path $Work "existing-user-bin" + Set-UserPathRegistryState $true $existingUserEntry ([Microsoft.Win32.RegistryValueKind]::String) + $installOut = & $InstallPs1 -InstallDir $installDir *>&1 | Out-String + Write-Host ($installOut.TrimEnd()) foreach ($name in @("netclaw", "netclawd")) { $exe = Join-Path $installDir "$name.exe" if ((Test-Path $exe) -and ((Get-Item $exe).Length -gt 0)) { @@ -138,20 +213,125 @@ try { } } - # 7. Verify PATH instruction uses User scope correctly (issue #1072) - # The printed instruction must NOT use $env:PATH (which merges Machine+User - # and corrupts the User PATH when written back). It must read User scope. + # 7. Verify the real installer changed User PATH without replacing the + # current process's inherited Machine PATH entries. + Write-Host "" + Write-Host "=== PATH automation ===" + $persistedPath = Get-UserPathRegistryState + $persistedEntries = @($persistedPath.Value -split ';') + if ($persistedEntries[0] -eq $installDir ` + -and $persistedEntries -contains $existingUserEntry ` + -and @($persistedEntries | Where-Object { $_ -eq $installDir }).Count -eq 1) { + Pass "PATH: install directory prepended once and existing User PATH preserved" + } else { + Fail "PATH: persisted User PATH has unexpected contents" + } + + $originalProcessEntries = @($OriginalProcessPath -split ';' | Where-Object { $_ }) + $currentProcessEntries = @($env:PATH -split ';' | Where-Object { $_ }) + $missingProcessEntries = @($originalProcessEntries | Where-Object { $currentProcessEntries -notcontains $_ }) + if ($currentProcessEntries[0] -eq $installDir ` + -and @($currentProcessEntries | Where-Object { $_ -eq $installDir }).Count -eq 1 ` + -and $missingProcessEntries.Count -eq 0) { + Pass "PATH: current process prepended once and inherited entries preserved" + } else { + Fail "PATH: current process lost inherited entries or contains duplicates" + } + + # A persisted entry must still repair a stale current process, and a + # trailing separator must not create a duplicate User PATH entry. + $env:PATH = $OriginalProcessPath + $userPathBeforeRepeat = Get-UserPathRegistryState + & $InstallPs1 -InstallDir "$installDir\" *>&1 | Out-Null + $userPathAfterRepeat = Get-UserPathRegistryState + $repeatProcessEntries = @($env:PATH -split ';' | Where-Object { $_ }) + if ($userPathAfterRepeat.Value -eq $userPathBeforeRepeat.Value ` + -and $userPathAfterRepeat.Kind -eq $userPathBeforeRepeat.Kind ` + -and $repeatProcessEntries[0] -eq $installDir ` + -and @($repeatProcessEntries | Where-Object { $_ -eq $installDir }).Count -eq 1) { + Pass "PATH: repeat install is idempotent and repairs current process" + } else { + Fail "PATH: repeat install changed User PATH or duplicated process entry" + } + + $env:NETCLAW_SMOKE_INSTALL_DIR = $installDir + $expandedUserPath = "%NETCLAW_SMOKE_INSTALL_DIR%;$existingUserEntry" + Set-UserPathRegistryState $true $expandedUserPath ([Microsoft.Win32.RegistryValueKind]::ExpandString) + $env:PATH = $OriginalProcessPath + & $InstallPs1 -InstallDir $installDir *>&1 | Out-Null + $expandedUserPathAfter = Get-UserPathRegistryState + if ($expandedUserPathAfter.Value -eq $expandedUserPath ` + -and $expandedUserPathAfter.Kind -eq [Microsoft.Win32.RegistryValueKind]::ExpandString) { + Pass "PATH: expandable User entry keeps its raw text and REG_EXPAND_SZ type" + } else { + Fail "PATH: expandable User entry or its registry type was rewritten" + } + + $literalPercentInstall = Join-Path $Work "%NETCLAW_LITERAL%\bin" + Set-UserPathRegistryState $true $existingUserEntry ([Microsoft.Win32.RegistryValueKind]::String) + $literalPercentOut = & $PowerShellExecutable -NoProfile -File $InstallPs1 ` + -InstallDir $literalPercentInstall 2>&1 | Out-String + $literalPercentState = Get-UserPathRegistryState + if ($LASTEXITCODE -eq 0 ` + -and $literalPercentState.Kind -eq [Microsoft.Win32.RegistryValueKind]::String ` + -and @($literalPercentState.Value -split ';')[0] -eq $literalPercentInstall) { + Pass "PATH: literal percent is preserved in a non-expanding User PATH" + } else { + Fail "PATH: literal percent was not preserved in a non-expanding User PATH" + Write-Host ($literalPercentOut.TrimEnd()) + } + + $expandablePath = "%SystemRoot%\System32" + $unsafePercentInstall = Join-Path $Work "%TEMP%\netclaw" + Set-UserPathRegistryState $true $expandablePath ([Microsoft.Win32.RegistryValueKind]::ExpandString) + $expandableStateBefore = Get-UserPathRegistryState + $unsafePercentResult = Invoke-CapturedPowerShell -Arguments @( + "-NoProfile", "-File", $InstallPs1, "-InstallDir", $unsafePercentInstall) + $expandableStateAfter = Get-UserPathRegistryState + if ($unsafePercentResult.ExitCode -ne 0 ` + -and $unsafePercentResult.Output -match "cannot be safely added to an expandable User PATH" ` + -and $expandableStateAfter.Value -eq $expandableStateBefore.Value ` + -and $expandableStateAfter.Kind -eq $expandableStateBefore.Kind) { + Pass "PATH: literal percent is rejected before mutating an expandable User PATH" + } else { + Fail "PATH: literal percent corrupted or changed an expandable User PATH" + } + + $env:PATH = "" + & $InstallPs1 -InstallDir $installDir *>&1 | Out-Null + if ($env:PATH -eq $installDir) { + Pass "PATH: empty process PATH does not create an empty entry" + } else { + Fail "PATH: empty process PATH produced unexpected contents" + } + $env:PATH = $OriginalProcessPath + + # 7c. Test -SkipShell flag Write-Host "" - Write-Host "=== PATH instruction check ===" - if ($installOut -match '\$env:PATH') { - Fail "PATH instruction: uses `$env:PATH (corrupts User PATH by merging Machine entries)" + Write-Host "=== -SkipShell flag ===" + $skipDir = Join-Path $Work "skip-install's" + $skipUserPathBefore = Get-UserPathRegistryState + $skipProcessPathBefore = $env:PATH + $skipOut = & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $skipDir -SkipShell 2>&1 | Out-String + if ($LASTEXITCODE -eq 0) { + Pass "-SkipShell: install completes without error" + } else { + Fail "-SkipShell: install failed (exit=$LASTEXITCODE)" + } + if ($skipOut -match "PATH modification skipped") { + Pass "-SkipShell: output mentions PATH modification skipped" } else { - Pass "PATH instruction: does not use `$env:PATH" + Fail "-SkipShell: missing 'skipped' message" } - if ($installOut -match "GetEnvironmentVariable\('PATH',\s*'User'\)") { - Pass "PATH instruction: reads from User scope" + $skipUserPathAfter = Get-UserPathRegistryState + if ($skipUserPathAfter.Value -eq $skipUserPathBefore.Value ` + -and $skipUserPathAfter.Kind -eq $skipUserPathBefore.Kind ` + -and $env:PATH -eq $skipProcessPathBefore ` + -and $skipOut -match [regex]::Escape("Add this directory to your User PATH") ` + -and $skipOut -match [regex]::Escape($skipDir)) { + Pass "-SkipShell: PATH is unchanged and manual guidance handles a literal install directory" } else { - Fail "PATH instruction: should read from User scope with GetEnvironmentVariable('PATH', 'User')" + Fail "-SkipShell: changed PATH or printed an unusable manual command" } # 8. Release channel resolution (dry-run) @@ -160,7 +340,7 @@ try { $shouldNotExist = Join-Path $Work "should-not-exist" function Assert-Resolves([string]$desc, [string]$want, [string[]]$extraArgs) { - $out = & pwsh -NoProfile -File $InstallPs1 -InstallDir $shouldNotExist -DryRun @extraArgs 2>&1 | Out-String + $out = & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $shouldNotExist -DryRun @extraArgs 2>&1 | Out-String $pattern = "(?m)^\s+Version:\s+$([regex]::Escape($want))\s*$" if ($LASTEXITCODE -eq 0 -and $out -match $pattern) { Pass "channel: $desc -> $want" @@ -176,8 +356,10 @@ try { Assert-Resolves "-Version pin overrides -Channel" $BetaVersion @("-Channel", "stable", "-Version", $BetaVersion) # An unknown channel must be rejected by the ValidateSet, not silently default. - & pwsh -NoProfile -File $InstallPs1 -InstallDir $shouldNotExist -DryRun -Channel bogus 2>&1 | Out-Null - if ($LASTEXITCODE -ne 0) { + $invalidChannelResult = Invoke-CapturedPowerShell -Arguments @( + "-NoProfile", "-File", $InstallPs1, "-InstallDir", $shouldNotExist, + "-DryRun", "-Channel", "bogus") + if ($invalidChannelResult.ExitCode -ne 0) { Pass "channel: unknown value rejected" } else { Fail "channel: unknown value should fail (exit=$LASTEXITCODE)" @@ -190,7 +372,7 @@ try { $freshDir = Join-Path $Work "fresh-beta" $freshConfigDir = Join-Path $Work "fresh-beta-config" $env:NETCLAW_CONFIG_DIR = $freshConfigDir - & pwsh -NoProfile -File $InstallPs1 -InstallDir $freshDir -Channel beta 2>&1 | Out-Null + & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $freshDir -Channel beta -SkipShell 2>&1 | Out-Null $freshConfig = Join-Path $freshConfigDir "netclaw.json" if ((Test-Path $freshConfig)) { $c = Get-Content -Raw $freshConfig | ConvertFrom-Json @@ -209,7 +391,7 @@ try { New-Item -ItemType Directory -Path $existConfigDir -Force | Out-Null '{"configVersion":1,"Daemon":{"ExposureMode":"local"}}' | Set-Content -Path (Join-Path $existConfigDir "netclaw.json") -Encoding UTF8 $env:NETCLAW_CONFIG_DIR = $existConfigDir - & pwsh -NoProfile -File $InstallPs1 -InstallDir $existDir -Channel beta 2>&1 | Out-Null + & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $existDir -Channel beta -SkipShell 2>&1 | Out-Null $c = Get-Content -Raw (Join-Path $existConfigDir "netclaw.json") | ConvertFrom-Json if ($c.Daemon.UpdateChannel -eq "beta" -and $c.Daemon.ExposureMode -eq "local") { Pass "config: -Channel beta patches existing config, preserves other Daemon keys" @@ -223,7 +405,7 @@ try { New-Item -ItemType Directory -Path $noflagConfigDir -Force | Out-Null '{"configVersion":1,"Daemon":{"UpdateChannel":"beta"}}' | Set-Content -Path (Join-Path $noflagConfigDir "netclaw.json") -Encoding UTF8 $env:NETCLAW_CONFIG_DIR = $noflagConfigDir - & pwsh -NoProfile -File $InstallPs1 -InstallDir $noflagDir 2>&1 | Out-Null + & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $noflagDir -SkipShell 2>&1 | Out-Null $c = Get-Content -Raw (Join-Path $noflagConfigDir "netclaw.json") | ConvertFrom-Json if ($c.Daemon.UpdateChannel -eq "beta") { Pass "config: plain upgrade preserves existing beta channel" @@ -237,7 +419,7 @@ try { New-Item -ItemType Directory -Path $downConfigDir -Force | Out-Null '{"configVersion":1,"Daemon":{"UpdateChannel":"beta"}}' | Set-Content -Path (Join-Path $downConfigDir "netclaw.json") -Encoding UTF8 $env:NETCLAW_CONFIG_DIR = $downConfigDir - & pwsh -NoProfile -File $InstallPs1 -InstallDir $downDir -Channel stable 2>&1 | Out-Null + & $PowerShellExecutable -NoProfile -File $InstallPs1 -InstallDir $downDir -Channel stable -SkipShell 2>&1 | Out-Null $c = Get-Content -Raw (Join-Path $downConfigDir "netclaw.json") | ConvertFrom-Json if ($c.Daemon.UpdateChannel -eq "stable") { Pass "config: -Channel stable overwrites existing beta" @@ -247,8 +429,11 @@ try { } finally { if ($ServerProc -and -not $ServerProc.HasExited) { $ServerProc.Kill() } + Set-UserPathRegistryState $OriginalUserPath.Exists $OriginalUserPath.Value $OriginalUserPath.Kind + $env:PATH = $OriginalProcessPath $env:MANIFEST_URL = $null $env:NETCLAW_CONFIG_DIR = $null + $env:NETCLAW_SMOKE_INSTALL_DIR = $null Remove-Item -Path $Work -Recurse -Force -ErrorAction SilentlyContinue } @@ -259,7 +444,6 @@ if ($script:Fail -gt 0) { exit 1 } Write-Host "install smoke (ps1): PASSED" -# Exit explicitly on the result, not on $LASTEXITCODE — the channel checks above run -# `pwsh -Channel bogus` (which exits non-zero by design), and without this the script -# would fall off the end and inherit that non-zero code despite all assertions passing. +# Deliberate rejection checks run child processes that exit nonzero, so return +# the assertion result explicitly instead of inheriting a child's exit code. exit 0 diff --git a/scripts/smoke/install-smoke.sh b/scripts/smoke/install-smoke.sh index d11c39176..6cee3ee14 100755 --- a/scripts/smoke/install-smoke.sh +++ b/scripts/smoke/install-smoke.sh @@ -111,6 +111,26 @@ rm -f "$MANIFEST_DEST" bash "$MANIFEST_GEN" "$VERSION" "$WORK/checksums-$VERSION" "$BASE_URL" >/dev/null bash "$MANIFEST_GEN" "$BETA_VERSION" "$WORK/checksums-$BETA_VERSION" "$BASE_URL" >/dev/null cp "$MANIFEST_DEST" "$SERVE/manifest.json" +python3 - "$SERVE/manifest.json" "$SERVE/manifest-rid-first.json" <<'PY' +import json +import sys + +with open(sys.argv[1], encoding="utf-8") as source: + manifest = json.load(source) +for release in manifest["releases"]: + release["assets"] = [ + { + "rid": asset["rid"], + "component": asset["component"], + "url": asset["url"], + "sha256": asset["sha256"], + "sizeBytes": asset["sizeBytes"], + } + for asset in release["assets"] + ] +with open(sys.argv[2], "w", encoding="utf-8") as destination: + json.dump(manifest, destination) +PY # ── 4. Serve the manifest + archives from localhost ────────────────────────── python3 -m http.server "$PORT" --bind 127.0.0.1 --directory "$SERVE" >/dev/null 2>&1 & @@ -169,6 +189,66 @@ check_detect "macOS x86_64 + Rosetta -> osx-arm64" Darwin x86_64 1 'DRY RUN: wou check_detect "Intel Mac rejected" Darwin x86_64 0 'Apple Silicon' 1 check_detect "unsupported OS rejected" freebsd x86_64 0 'Unsupported OS' 1 +set +e +invalid_path_out=$(INSTALL_DIR="$WORK/invalid:path" \ + MANIFEST_URL="$BASE_URL/manifest.json" \ + bash "$INSTALL_SH" --dry-run 2>&1) +invalid_path_rc=$? +set -e +if [ "$invalid_path_rc" -ne 0 ] && echo "$invalid_path_out" | grep -q "cannot contain ':'"; then + pass "PATH: unrepresentable Unix install directory rejected" +else + fail "PATH: Unix install directory containing ':' was accepted" +fi + +PHYSICAL_INSTALL="$WORK/physical:install" +SYMLINK_INSTALL="$WORK/safe-install-link" +SYMLINK_HOME="$WORK/symlink-home" +mkdir -p "$PHYSICAL_INSTALL" "$SYMLINK_HOME" +ln -s "$PHYSICAL_INSTALL" "$SYMLINK_INSTALL" +set +e +symlink_path_out=$(HOME="$SYMLINK_HOME" SHELL="$(command -v bash)" \ + INSTALL_DIR="$SYMLINK_INSTALL" MANIFEST_URL="$BASE_URL/manifest.json" \ + bash "$INSTALL_SH" cli 2>&1) +symlink_path_rc=$? +set -e +if [ "$symlink_path_rc" -ne 0 ] \ + && echo "$symlink_path_out" | grep -q "cannot contain ':'" \ + && [ ! -e "$SYMLINK_HOME/.netclaw/env" ] \ + && [ ! -e "$SYMLINK_HOME/.bashrc" ] \ + && [ ! -e "$PHYSICAL_INSTALL/netclaw" ]; then + pass "PATH: physical install path is validated before install or shell mutation" +else + fail "PATH: unrepresentable symlink target was installed or persisted" + echo "$symlink_path_out" | indent +fi + +# Exercise the dependency-free parser with a valid manifest whose asset fields +# are in reverse order. A private mirror need not preserve JSON property order. +NO_JQ_BIN="$WORK/no-jq-bin" +mkdir -p "$NO_JQ_BIN" +for cmd in awk bash curl cut grep head mktemp rm sed sha256sum shasum tar tr uname; do + command_path=$(command -v "$cmd" 2>/dev/null || true) + if [ -n "$command_path" ]; then + ln -s "$command_path" "$NO_JQ_BIN/$cmd" + fi +done +set +e +no_jq_out=$(PATH="$NO_JQ_BIN" \ + MANIFEST_URL="$BASE_URL/manifest-rid-first.json" \ + INSTALL_DIR="$WORK/no-jq-install" \ + /bin/bash "$INSTALL_SH" --dry-run 2>&1) +no_jq_rc=$? +set -e +if [ "$no_jq_rc" -eq 0 ] \ + && echo "$no_jq_out" | grep -q "DRY RUN: would install netclaw .*/$VERSION/" \ + && echo "$no_jq_out" | grep -q "DRY RUN: would install netclawd .*/$VERSION/"; then + pass "manifest: jq-less parser selects stable assets with reordered fields" +else + fail "manifest: jq-less reordered-field parse failed (exit=$no_jq_rc)" + echo "$no_jq_out" | indent +fi + # Dry run must not create the install directory. if [ -d "$WORK/should-not-exist" ]; then fail "dry-run: created an install directory (should install nothing)" @@ -177,11 +257,16 @@ else fi # ── 6. Mechanical check: a real install on the host's native RID ───────────── +# Uses a temp HOME so shell integration writes to the temp dir, not the +# CI runner's real profile — and we can verify the RC was modified. echo "" echo "=== real install (host RID, stand-in archive) ===" -INSTALL_DIR="$WORK/installed" +INSTALL_HOME="$WORK/installed-home" +INSTALL_DIR="$INSTALL_HOME/.netclaw/bin" +mkdir -p "$INSTALL_HOME" set +e -install_out=$(MANIFEST_URL="$BASE_URL/manifest.json" INSTALL_DIR="$INSTALL_DIR" \ +install_out=$(HOME="$INSTALL_HOME" \ + MANIFEST_URL="$BASE_URL/manifest.json" INSTALL_DIR="$INSTALL_DIR" \ bash "$INSTALL_SH" 2>&1) install_rc=$? set -e @@ -201,6 +286,26 @@ for name in netclaw netclawd; do fi done +# Verify shell integration actually ran +INSTALL_ENV="$INSTALL_HOME/.netclaw/env" +if [ -f "$INSTALL_ENV" ]; then + pass "real install: env script created" +else + fail "real install: env script not found at $INSTALL_ENV" +fi + +# The RC file depends on $SHELL — check whichever one was created +RC_MODIFIED=false +for rc in "$INSTALL_HOME/.bashrc" "$INSTALL_HOME/.zshrc" "$INSTALL_HOME/.profile" "$INSTALL_HOME/.config/fish/conf.d/netclaw.fish"; do + if [ -f "$rc" ] && grep -qxF ". '$INSTALL_ENV'" "$rc" 2>/dev/null; then + pass "real install: $(basename "$rc") sources env script" + RC_MODIFIED=true + fi +done +if [ "$RC_MODIFIED" = false ]; then + fail "real install: no RC file sources env script (SHELL=$SHELL)" +fi + # ── 7. Release channel resolution (dry-run) ────────────────────────────────── echo "" echo "=== release channel resolution ===" @@ -256,7 +361,7 @@ set +e fresh_out=$(MANIFEST_URL="$BASE_URL/manifest.json" \ INSTALL_DIR="$FRESH_DIR" \ CONFIG_DIR="$FRESH_CONFIG_DIR" \ - bash "$INSTALL_SH" --channel beta 2>&1) + bash "$INSTALL_SH" --channel beta --skip-shell 2>&1) fresh_rc=$? set -e if [ "$fresh_rc" -eq 0 ] && [ -f "$FRESH_CONFIG_DIR/netclaw.json" ]; then @@ -284,7 +389,7 @@ set +e exist_out=$(MANIFEST_URL="$BASE_URL/manifest.json" \ INSTALL_DIR="$EXIST_DIR" \ CONFIG_DIR="$EXIST_CONFIG_DIR" \ - bash "$INSTALL_SH" --channel beta 2>&1) + bash "$INSTALL_SH" --channel beta --skip-shell 2>&1) exist_rc=$? set -e if [ "$exist_rc" -eq 0 ] && command -v jq >/dev/null 2>&1; then @@ -297,6 +402,7 @@ if [ "$exist_rc" -eq 0 ] && command -v jq >/dev/null 2>&1; then fi else fail "config: --channel beta on existing config (exit=$exist_rc)" + echo "$exist_out" | indent fi # 8c. Plain upgrade (no --channel) leaves existing beta config alone @@ -308,7 +414,7 @@ set +e noflag_out=$(MANIFEST_URL="$BASE_URL/manifest.json" \ INSTALL_DIR="$NOFLAG_DIR" \ CONFIG_DIR="$NOFLAG_CONFIG_DIR" \ - bash "$INSTALL_SH" 2>&1) + bash "$INSTALL_SH" --skip-shell 2>&1) noflag_rc=$? set -e if [ "$noflag_rc" -eq 0 ] && command -v jq >/dev/null 2>&1; then @@ -320,6 +426,7 @@ if [ "$noflag_rc" -eq 0 ] && command -v jq >/dev/null 2>&1; then fi else fail "config: plain upgrade (exit=$noflag_rc)" + echo "$noflag_out" | indent fi # 8d. --channel stable on existing beta overwrites to stable @@ -331,7 +438,7 @@ set +e down_out=$(MANIFEST_URL="$BASE_URL/manifest.json" \ INSTALL_DIR="$DOWNGRADE_DIR" \ CONFIG_DIR="$DOWNGRADE_CONFIG_DIR" \ - bash "$INSTALL_SH" --channel stable 2>&1) + bash "$INSTALL_SH" --channel stable --skip-shell 2>&1) down_rc=$? set -e if [ "$down_rc" -eq 0 ] && command -v jq >/dev/null 2>&1; then @@ -343,6 +450,153 @@ if [ "$down_rc" -eq 0 ] && command -v jq >/dev/null 2>&1; then fi else fail "config: --channel stable on existing beta (exit=$down_rc)" + echo "$down_out" | indent +fi + +# ── 9. Shell integration (PATH automation) ─────────────────────────────────── +echo "" +echo "=== shell integration ===" + +assert_path_once() { + local desc="$1" observed_path="$2" install_dir="$3" + local count + count=$(printf '%s' "$observed_path" | tr ':' '\n' | grep -cxF "$install_dir" || true) + if [ "$count" -eq 1 ]; then + pass "$desc: install directory appears exactly once on PATH" + else + fail "$desc: install directory appears $count times on PATH" + fi +} + +run_unix_installer() { + local shell_path="$1" home="$2" install_dir="$3" + shift 3 + SHELL="$shell_path" HOME="$home" \ + MANIFEST_URL="$BASE_URL/manifest.json" INSTALL_DIR="$install_dir" \ + CONFIG_DIR="$home/.netclaw/config" \ + bash "$INSTALL_SH" "$@" +} + +# Bash: run the generated startup path through Bash itself, then repeat the +# install to prove both profile mutation and PATH evaluation are idempotent. +BASH_HOME="$WORK/shell-bash" +BASH_INSTALL="$BASH_HOME/netclaw install's/bin" +mkdir -p "$BASH_HOME" +if [ "$(uname -s)" = "Darwin" ]; then + BASH_RC="$BASH_HOME/.bash_profile" + printf '# existing bash profile' > "$BASH_RC" + printf '# profile must remain untouched\n' > "$BASH_HOME/.profile" +else + BASH_RC="$BASH_HOME/.bashrc" + printf '# existing bash rc' > "$BASH_RC" +fi + +if run_unix_installer "$(command -v bash)" "$BASH_HOME" "$BASH_INSTALL" >/dev/null \ + && run_unix_installer "$(command -v bash)" "$BASH_HOME" "$BASH_INSTALL" >/dev/null; then + BASH_INSTALL_PHYSICAL=$(cd "$BASH_INSTALL" && pwd -P) + bash_path=$(PATH="/usr/bin:/bin" HOME="$BASH_HOME" \ + bash --noprofile --rcfile "$BASH_RC" -i -c 'printf "%s" "$PATH"' 2>/dev/null) + assert_path_once "bash" "$bash_path" "$BASH_INSTALL_PHYSICAL" + bash_empty_path=$(PATH="" HOME="$BASH_HOME" \ + /bin/bash --noprofile --rcfile "$BASH_RC" -i -c 'printf "%s" "$PATH"' 2>/dev/null) + if [ "$bash_empty_path" = "$BASH_INSTALL_PHYSICAL" ]; then + pass "bash: empty PATH does not introduce a current-directory entry" + else + fail "bash: empty PATH produced '$bash_empty_path'" + fi + source_count=$(grep -cF "$BASH_HOME/.netclaw/env" "$BASH_RC" || true) + if [ "$source_count" -eq 1 ]; then + pass "bash: profile source line is idempotent" + else + fail "bash: profile contains $source_count netclaw source lines" + fi + if [ "$(uname -s)" = "Darwin" ] && ! grep -qF netclaw "$BASH_HOME/.profile"; then + pass "bash-macos: existing .bash_profile wins over .profile" + fi +else + fail "bash: installer failed" +fi + +# Zsh: resolve a non-exported ZDOTDIR from .zshenv, then execute the selected +# startup file under zsh so a Bash-compatible false positive cannot pass. +if command -v zsh >/dev/null 2>&1; then + ZSH_HOME="$WORK/shell-zsh" + ZDOT_DIR="$ZSH_HOME/custom-zdotdir" + ZSH_INSTALL="$ZSH_HOME/netclaw install's/bin" + mkdir -p "$ZDOT_DIR" + printf "ZDOTDIR='%s'\n" "$ZDOT_DIR" > "$ZSH_HOME/.zshenv" + printf '# existing zsh config\n' > "$ZDOT_DIR/.zshrc" + if (unset ZDOTDIR; run_unix_installer "$(command -v zsh)" "$ZSH_HOME" "$ZSH_INSTALL" >/dev/null) \ + && (unset ZDOTDIR; run_unix_installer "$(command -v zsh)" "$ZSH_HOME" "$ZSH_INSTALL" >/dev/null); then + ZSH_INSTALL_PHYSICAL=$(cd "$ZSH_INSTALL" && pwd -P) + zsh_path=$(PATH="/usr/bin:/bin" ZDOTDIR="$ZDOT_DIR" \ + zsh -f -c 'source "$ZDOTDIR/.zshrc"; print -rn -- "$PATH"') + assert_path_once "zsh" "$zsh_path" "$ZSH_INSTALL_PHYSICAL" + if [ ! -e "$ZSH_HOME/.zshrc" ]; then + pass "zsh: non-exported ZDOTDIR is authoritative" + else + fail "zsh: installer touched ~/.zshrc despite ZDOTDIR" + fi + else + fail "zsh: installer failed" + fi +else + echo "SKIP: zsh executable not available" +fi + +# Fish owns a native conf.d file. Execute that file with fish, not Bash. +if command -v fish >/dev/null 2>&1; then + FISH_HOME="$WORK/shell-fish" + FISH_INSTALL="$FISH_HOME/netclaw install's/bin" + FISH_RC="$FISH_HOME/.config/fish/conf.d/netclaw.fish" + if XDG_CONFIG_HOME="$FISH_HOME/.config" \ + run_unix_installer "$(command -v fish)" "$FISH_HOME" "$FISH_INSTALL" >/dev/null \ + && XDG_CONFIG_HOME="$FISH_HOME/.config" \ + run_unix_installer "$(command -v fish)" "$FISH_HOME" "$FISH_INSTALL" >/dev/null; then + FISH_INSTALL_PHYSICAL=$(cd "$FISH_INSTALL" && pwd -P) + fish_path=$(PATH="/usr/bin:/bin" fish --no-config -c \ + "source '$FISH_RC'; string join : -- \$PATH") + assert_path_once "fish" "$fish_path" "$FISH_INSTALL_PHYSICAL" + else + fail "fish: installer failed" + fi +else + echo "SKIP: fish executable not available" +fi + +# Opt-out under a supported shell must print a self-contained command instead +# of referring to an env file that was not made. +MANUAL_HOME="$WORK/shell-skip" +MANUAL_INSTALL="$MANUAL_HOME/netclaw install's/bin" +mkdir -p "$MANUAL_HOME" +manual_out=$(run_unix_installer "$(command -v bash)" "$MANUAL_HOME" "$MANUAL_INSTALL" --skip-shell) +manual_command=$(printf '%s\n' "$manual_out" | sed -n 's/^ \{0,4\}\(export PATH=.*\)$/\1/p' | head -1) +if [ -n "$manual_command" ] && [ ! -e "$MANUAL_HOME/.netclaw/env" ]; then + MANUAL_INSTALL_PHYSICAL=$(cd "$MANUAL_INSTALL" && pwd -P) + manual_path=$(PATH="/usr/bin:/bin" bash -c "$manual_command; printf '%s' \"\$PATH\"") + assert_path_once "skip" "$manual_path" "$MANUAL_INSTALL_PHYSICAL" + manual_empty_path=$(PATH="" /bin/bash -c "$manual_command; printf '%s' \"\$PATH\"") + if [ "$manual_empty_path" = "$MANUAL_INSTALL_PHYSICAL" ]; then + pass "skip: manual command preserves an empty PATH without adding current directory" + else + fail "skip: manual command produced '$manual_empty_path' from an empty PATH" + fi +else + fail "skip: missing usable manual PATH command or created shell files" +fi + +# Unsupported shells get shell-neutral guidance; emitting Bash syntax for an +# arbitrary shell would make the suggested command actively misleading. +UNKNOWN_HOME="$WORK/shell-unknown" +UNKNOWN_INSTALL="$UNKNOWN_HOME/netclaw install's/bin" +unknown_out=$(run_unix_installer /bin/unknownshell "$UNKNOWN_HOME" "$UNKNOWN_INSTALL") +UNKNOWN_INSTALL_PHYSICAL=$(cd "$UNKNOWN_INSTALL" && pwd -P) +if echo "$unknown_out" | grep -qF "$UNKNOWN_INSTALL_PHYSICAL" \ + && ! echo "$unknown_out" | grep -q 'export PATH=' \ + && [ ! -e "$UNKNOWN_HOME/.netclaw/env" ]; then + pass "unknown: guidance is shell-neutral and no shell files are created" +else + fail "unknown: guidance is shell-specific or shell files were created" fi # ── Summary ────────────────────────────────────────────────────────────────── From 75e46be141d9a9a4ef821b7220a26a291329d4d1 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 18 Jul 2026 13:56:43 -0500 Subject: [PATCH 12/12] release: prepare 0.25.0 stable (#1692) --- Directory.Build.props | 2 +- RELEASE_NOTES.md | 83 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index 4f3788d69..874555a41 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -9,7 +9,7 @@ enable true 0.25.0 - beta.5 + - Preserve Git working context across sessions and subagents — A bounded, audience-aware Git working-context snapshot stays current in the system prompt, and coding subagents inherit recent-file/project context (#1630) - User-written AGENTS.md for application-specific agent guidance — Operators can author ~/.netclaw/identity/AGENTS.md, layered after Netclaw's embedded operating core and inherited by sub-agents (#1622) - Memory curation no longer overwrites existing documents on collision — Create-decision collisions now append new content instead of silently overwriting the existing document (#1637) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 75d091dc9..5aab47bad 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,5 +1,88 @@ # NetClaw Release Notes +## 0.25.0 (2026-07-18) + +This stable release concludes the 0.25.0 beta cycle (five beta releases from 0.25.0-beta.1 through beta.5) and adds a round of final polish focused on installation, CLI reliability, and daemon shutdown robustness. + +### Features +- **Automated shell PATH integration for installers** — Unix and Windows installers now automatically update the user's shell profile or PATH registry on install. Unix installers detect the active shell (bash, zsh, fish) and source a self-guarding `~/.netclaw/env` script from the correct RC file with duplicate prevention. Windows installers modify the User-scope PATH and broadcast `WM_SETTINGCHANGE`. Use `--skip-shell` (`-SkipShell` on Windows) to opt out ([#1687](https://github.com/netclaw-dev/netclaw/pull/1687)) +- **Timestamped HMAC verification for webhooks** — Webhook routes now verify request timestamps alongside HMAC signatures to prevent replay attacks, with configurable HMAC algorithm support ([#1660](https://github.com/netclaw-dev/netclaw/pull/1660)) +- **TSV support in content scanner** — Added `text/tab-separated-values` as a recognized MIME type, allowing TSV files to be scanned and processed by the content pipeline ([#1645](https://github.com/netclaw-dev/netclaw/pull/1645)) +- **Preserve Git working context across sessions and subagents** — A bounded, audience-aware Git working-context snapshot (branch, worktree, repository, upstream, changed files) now stays current in the system prompt, and coding subagents inherit recent-file/project context so parent sessions merge back only confirmed successful child edits. Measured 20% → 100% success rate on a linked-worktree coding eval ([#1630](https://github.com/netclaw-dev/netclaw/pull/1630)) +- **User-written `AGENTS.md` for application-specific agent guidance** — Operators can now author `~/.netclaw/identity/AGENTS.md`, layered after NetClaw's embedded operating core and inherited by sub-agents, to give the running agent deployment-specific mission and workflow guidance. Seeded with a minimal scaffold during init without overwriting existing guidance ([#1622](https://github.com/netclaw-dev/netclaw/pull/1622)) +- **Discord DM reminder delivery** — Reminders can now be delivered to Discord DMs via improved `DiscordReminderTargetResolver` ([#1609](https://github.com/netclaw-dev/netclaw/pull/1609)) +- **Named model configuration & provider runtime validation** — New `NamedModelConfiguration` and `ProviderRuntimeValidation` types, config schema updates, and CLI wizard improvements for provider/model setup ([#1610](https://github.com/netclaw-dev/netclaw/pull/1610)) +- **SkillServer native sub-agent sync** — Optional native manifest sidecar sync for server-managed sub-agents, keeping RFC skill sync primary while downloading verified native artifacts when available. Local sub-agent files load before server-feed files so user-authored definitions always win ([#1539](https://github.com/netclaw-dev/netclaw/pull/1539)) +- **GitHub Enterprise Copilot support** — Authenticate GitHub Copilot tokens against GHE instances and route requests to the correct data residency endpoint ([#1509](https://github.com/netclaw-dev/netclaw/pull/1509), [#1512](https://github.com/netclaw-dev/netclaw/pull/1512), [#1555](https://github.com/netclaw-dev/netclaw/pull/1555)) +- **Slack native processing status** — Real-time processing indicators in Slack instead of generic "working" messages ([#1524](https://github.com/netclaw-dev/netclaw/pull/1524)) +- **Reminder failure visibility** — Operators can now see when reminders fail or are skipped ([#1503](https://github.com/netclaw-dev/netclaw/pull/1503)) +- **Degraded startup mode** — Daemon starts with a "no valid model" banner when no provider is configured, instead of failing host startup ([#1540](https://github.com/netclaw-dev/netclaw/pull/1540)) +- **Synced skill resources via shell** — Skill resources synced from the cloud can now execute via shell commands ([#1551](https://github.com/netclaw-dev/netclaw/pull/1551)) +- **DwarfStar (ds4) provider support** — New openai-compatible backend strategy supporting DwarfStar models ([#1349](https://github.com/netclaw-dev/netclaw/pull/1349)) +- **Inherit embedded AGENTS.md for sub-agents** — Sub-agents now inherit the parent session's embedded operating rules from AGENTS.md ([#1490](https://github.com/netclaw-dev/netclaw/pull/1490)) +- **Show advertised skill count for remote skill servers** — The Skill Sources config screen now displays how many skills each remote server advertises ([#1452](https://github.com/netclaw-dev/netclaw/pull/1452)) + +### Bug Fixes +- **MCP arguments shown in approval prompts** — Fixed: approval prompts now display full MCP tool arguments for better operator context ([#1689](https://github.com/netclaw-dev/netclaw/pull/1689)) +- **CLI reports unresolved model references cleanly** — Fixed: CLI no longer crashes with opaque errors when a model reference cannot be resolved ([#1680](https://github.com/netclaw-dev/netclaw/pull/1680)) +- **CLI handles model migration errors without crashing** — Fixed: model migration validation errors are now reported gracefully instead of crashing the CLI ([#1678](https://github.com/netclaw-dev/netclaw/pull/1678)) +- **CLI rejects numeric model modalities** — Fixed: model modality values are now validated as strings, rejecting numeric input ([#1677](https://github.com/netclaw-dev/netclaw/pull/1677)) +- **Daemon-stop session drain bounded** — Fixed: `netclaw daemon stop` now properly bounds the session drain timeout, preventing hangs on interactive-tool-paused sessions and giving the CLI adequate headroom ([#1673](https://github.com/netclaw-dev/netclaw/pull/1673)) +- **Reminders rescan definitions before startup alerts** — Fixed: startup alerts now fire only after reminder definitions are fully loaded, preventing missed or duplicate reminders on restart ([#1653](https://github.com/netclaw-dev/netclaw/pull/1653)) +- **OpenAI client 2.12 compatibility** — Fixed: updated OpenAI provider plugin to work with OpenAI client 2.12+ changes ([#1654](https://github.com/netclaw-dev/netclaw/pull/1654)) +- **Attachment path guidance** — Fixed: authoritative attachment path guidance now points to the correct session media directory ([#1686](https://github.com/netclaw-dev/netclaw/pull/1686)) +- **Windows actor checks made deterministic** — Fixed: Windows actor lifecycle tests were non-deterministic ([#1661](https://github.com/netclaw-dev/netclaw/pull/1661)) +- **Ollama smoke test pinned** — Fixed: pinned Ollama installer to a specific release for smoke test stability ([#1659](https://github.com/netclaw-dev/netclaw/pull/1659)) +- **VHS process group timeout** — Fixed: full VHS process group now times out properly during smoke tests ([#1655](https://github.com/netclaw-dev/netclaw/pull/1655)) +- **Screenshot regression determinism** — Fixed: screenshot regression suite now uses pixel comparison with blank/partial retry and Termina seam fixes ([#1451](https://github.com/netclaw-dev/netclaw/pull/1451)) +- **Legacy model environment overrides serialized** — Fixed: legacy model environment overrides are now properly serialized ([#1656](https://github.com/netclaw-dev/netclaw/pull/1656)) +- **Subagent token usage tracked** — Sub-agent token usage is now properly tracked and reported ([#1597](https://github.com/netclaw-dev/netclaw/pull/1597)) +- **Subagent fail-closed for unattended approvals** — Fixed: subagents fail safely when approvals are required but no human is present ([#1616](https://github.com/netclaw-dev/netclaw/pull/1616)) +- **Memory core: curation documents stable** — Fixed: memory curation documents were unstable across writes, causing cross-session memory recall issues ([#1575](https://github.com/netclaw-dev/netclaw/pull/1575)) +- **Model set/picker preserves modalities** — Fixed: model selection UI now preserves model modalities correctly ([#1610](https://github.com/netclaw-dev/netclaw/pull/1610)) +- **Slack processing status serialization** — Fixed: Slack processing status now serializes correctly for all states ([#1556](https://github.com/netclaw-dev/netclaw/pull/1556)) +- **Autonomous sessions can write workspace directory** — Fixed: autonomous sessions could not write to the workspace directory ([#1498](https://github.com/netclaw-dev/netclaw/pull/1498)) +- **Reminder duplicate-execution guard** — Fixed: Mode A reminder session wedge prevented the duplicate guard from releasing ([#1500](https://github.com/netclaw-dev/netclaw/pull/1500)) +- **Reset stops daemon first** — Fixed: `netclaw reset` now stops the daemon before proceeding and shows a progress screen ([#1494](https://github.com/netclaw-dev/netclaw/pull/1494)) +- **MCP tool errors display cleanly** — Fixed: MCP errors now show as attributed messages instead of raw JSON dumps ([#1510](https://github.com/netclaw-dev/netclaw/pull/1510)) +- **TUI: auto-advance the add-skill-server flow** — Fixed: skill server probe flow now auto-advances on successful connection ([#1458](https://github.com/netclaw-dev/netclaw/pull/1458)) +- **TUI: auto-start init health checks** — Fixed: init health checks now start automatically ([#1454](https://github.com/netclaw-dev/netclaw/pull/1454)) +- **TUI: discoverable Done rows** — Added "Done" back-out rows across Security & Access menus and config screens ([#1448](https://github.com/netclaw-dev/netclaw/pull/1448), [#1441](https://github.com/netclaw-dev/netclaw/pull/1441)) +- **TUI: session browser selection highlight** — Fixed: session browser now shows selection highlight ([#1531](https://github.com/netclaw-dev/netclaw/pull/1531)) +- **TUI: identity redo timezone loop** — Fixed: identity redo would loop on timezone; also fixed session browser regression ([#1518](https://github.com/netclaw-dev/netclaw/pull/1518)) +- **TUI: flaky host crash during reset** — Fixed: thread-unsafe ReactiveProperty access during reset caused crashes ([#1525](https://github.com/netclaw-dev/netclaw/pull/1525)) +- **Subagent terminal result summaries** — Fixed: subagent terminal results now display properly ([#1519](https://github.com/netclaw-dev/netclaw/pull/1519)) +- **UTF-8 BOM in skill frontmatter** — Fixed: skill scanner now strips UTF-8 BOM before parsing YAML frontmatter ([#1583](https://github.com/netclaw-dev/netclaw/pull/1583)) +- **Block system and external skill mutations** — Fixed: skill system now blocks mutations to system and externally-synced skills ([#1457](https://github.com/netclaw-dev/netclaw/pull/1457)) +- **Self-monitoring spawn_agent liveness respected** — Fixed: tool pipeline now respects self-monitoring spawn_agent liveness ([#1456](https://github.com/netclaw-dev/netclaw/pull/1456)) + +### Breaking Changes +- **Removed silent local-ollama fallback** — Users without any provider configured will now see a "no valid model" banner instead of an automatic fallback to local Ollama. Explicit provider configuration is now required for full functionality ([#1540](https://github.com/netclaw-dev/netclaw/pull/1540)) + +### Improvements +- **Tool execution pipeline refactoring** — Restructured the session tool execution pipeline with isolated invocation scope, composed execution pipeline, typed sub-agent context isolation, and proper lifecycle cleanup ([#1641](https://github.com/netclaw-dev/netclaw/pull/1641), [#1643](https://github.com/netclaw-dev/netclaw/pull/1643), [#1644](https://github.com/netclaw-dev/netclaw/pull/1644), [#1646](https://github.com/netclaw-dev/netclaw/pull/1646)) +- **Logical skill access and authoritative inventory refresh** — Skill loading now resolves through logical `skill_load`/`skill_read_resource` access with native > managed-feed > external precedence. Startup, sync, watcher, and `skill_manage` inventory rebuilds centralized through one live-source refresher ([#1634](https://github.com/netclaw-dev/netclaw/pull/1634)) +- **Session actor decomposed into transient-state handlers** — `LlmSessionActor` decomposed into smaller, focused handlers for improved maintainability and testability ([#1496](https://github.com/netclaw-dev/netclaw/pull/1496)) +- **Log stream partitioned by session** — Each session now gets its own `session.log` while `daemon.log` remains sparse. Cleaner logs and better observability with OTEL union support ([#1499](https://github.com/netclaw-dev/netclaw/pull/1499)) +- **Memory core redesign** — Shared curation evaluator unified across both memory write pipelines so they can never diverge. July 2026 audit: revived curation LLM, balanced prompt, and recall precision re-tune ([#1575](https://github.com/netclaw-dev/netclaw/pull/1575), [#1568](https://github.com/netclaw-dev/netclaw/pull/1568)) + +### Security +- **Pinned Microsoft.OpenApi to 2.7.5** — Addresses CVE-2026-49451 ([#1543](https://github.com/netclaw-dev/netclaw/pull/1543)) +- **Suppressed GHSA-2m69-gcr7-jv3q (SQLitePCLRaw CVE-2025-6965)** — Temporarily suppressed until upstream patches available. Tracking: dotnet/efcore#38257 ([#1444](https://github.com/netclaw-dev/netclaw/pull/1444)) + +### Dependency Updates +- **OpenAI SDK** — Updated to 2.12 (required for API compatibility) ([#1654](https://github.com/netclaw-dev/netclaw/pull/1654)) +- **Microsoft.Extensions.AI** — 10.6.0 → 10.8.0 ([#1650](https://github.com/netclaw-dev/netclaw/pull/1650)) +- **Microsoft.AspNetCore.DataProtection** — 10.0.9 → 10.0.10 ([#1657](https://github.com/netclaw-dev/netclaw/pull/1657)) +- **Microsoft.NET.Test.Sdk** — 18.7.0 → 18.8.1 ([#1651](https://github.com/netclaw-dev/netclaw/pull/1651)) +- **Mattermost.NET** — 5.0.0 → 5.0.3 ([#1626](https://github.com/netclaw-dev/netclaw/pull/1626)) +- **SkillServer** — `Netclaw.SkillClient` 0.4.0-beta.4 → 0.4.0 (stable) ([#1638](https://github.com/netclaw-dev/netclaw/pull/1638)) +- **Anthropic SDK** — 12.30.0 → 12.35.1 ([#1488](https://github.com/netclaw-dev/netclaw/pull/1488), [#1561](https://github.com/netclaw-dev/netclaw/pull/1561)) +- **Akka** — Akka.Cluster.Sharding and Akka.Persistence 1.5.69 → 1.5.70 ([#1560](https://github.com/netclaw-dev/netclaw/pull/1560)) +- **Testcontainers** — 4.12.0 → 4.13.0 ([#1563](https://github.com/netclaw-dev/netclaw/pull/1563)) +- **YamlDotNet** — 18.0.0 → 18.1.0 ([#1516](https://github.com/netclaw-dev/netclaw/pull/1516)) +- **Verify.XunitV3** — 31.19.1 → 31.20.0 ([#1446](https://github.com/netclaw-dev/netclaw/pull/1446)) + ## 0.25.0-beta.5 (2026-07-16) ### Features