From 95e5c34198d0b177323823473c9bb56ef9d429a9 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 28 Jul 2026 00:06:32 -0400 Subject: [PATCH 1/2] fix: advertise cursor borrowed review regardless of build drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Borrowed-review capability was gated on the installed cursor-agent matching an exact-build certification record. Cursor auto-updates, so the record went stale within days: the daemon then advertised BorrowedReviewSupported=false, the server resolved workspace_mode=fallback, and reviewers read a stale committed base instead of the requester's working tree — silently, with no warning in the daemon log and no notice to the requester. Capability is now a property of the vendor's factory alone. The two matching pre-spawn throws go with it (advertisement and launch must agree); the sibling borrowed_snapshot_containment_mismatch check stays, since it validates a declared-containment invariant rather than a version fact. The certification record is demoted to CursorBorrowedReviewValidation / TryMatchValidatedBuild — a record of the last build the maintainer probe suite validated, gating nothing, with a non-match the expected steady state. One Information line per unattended vendor at daemon startup names the probed CliVersion (or the literal `unknown`), worded as a startup observation that goes stale on a vendor update. It deliberately does not evaluate agreement with the validation record, and nothing equivalent is logged per launch. Removing the gate also removes its macOS/arm64 preconditions, so borrowed review is now advertised on every platform where the vendor CLI is installed. That expansion is deliberate — the snapshot path is platform-neutral. Design: docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md --- src/Capacitor.Cli.Daemon/DaemonRunner.cs | 44 ++- .../Services/AcpHostedAgentRuntimeFactory.cs | 25 +- ...n.cs => CursorBorrowedReviewValidation.cs} | 35 ++- .../DaemonRunnerCursorAvailabilityTests.cs | 262 +++++++++++++++++- .../AcpHostedAgentRuntimeFactoryTests.cs | 109 ++++++++ .../CursorBorrowedReviewCertificationTests.cs | 22 -- .../CursorBorrowedReviewValidationTests.cs | 32 +++ 7 files changed, 480 insertions(+), 49 deletions(-) rename src/Capacitor.Cli.Daemon/Services/{CursorBorrowedReviewCertification.cs => CursorBorrowedReviewValidation.cs} (58%) delete mode 100644 test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewCertificationTests.cs create mode 100644 test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewValidationTests.cs diff --git a/src/Capacitor.Cli.Daemon/DaemonRunner.cs b/src/Capacitor.Cli.Daemon/DaemonRunner.cs index 04a83efd7..b860299a6 100644 --- a/src/Capacitor.Cli.Daemon/DaemonRunner.cs +++ b/src/Capacitor.Cli.Daemon/DaemonRunner.cs @@ -356,6 +356,11 @@ await Console.Error.WriteLineAsync( config.UnattendedVendors = ComputeUnattendedVendors(runtimeFactories); config.UnattendedVendorCapabilities = ComputeUnattendedVendorCapabilities(runtimeFactories, config); + // Which build of each unattended vendor was installed when this daemon started. Recorded at + // startup (like the Cursor-unavailable warning below) rather than per launch, and reported + // without any comparison against a validated-build record. + LogUnattendedVendorIdentities(logger, config.UnattendedVendorCapabilities); + // IsAvailable()==false silently omits cursor from SupportedVendors above — correct // behavior (the launch dialog just won't offer Cursor), but gave operators no clue WHY. One // Warning at startup (not per-launch) so a missing/misconfigured cursor-agent install is @@ -604,12 +609,12 @@ internal static IReadOnlyList ComputeUnattendedVendo "copilot" => (config.CopilotPath, CopilotLauncherPolicyVersion), _ => ("", $"{vendor}-unattended-v1") }; - var cursorArtifact = vendor == "cursor" - ? CursorBorrowedReviewCertification.TryCertify(cliPath) - : null; - var borrowedSupported = vendor == "cursor" - ? factory.SupportsBorrowedReviewFlow && cursorArtifact is not null - : factory.SupportsBorrowedReviewFlow; + // Trust-by-default: a vendor's borrowed-review capability is a property of its FACTORY, + // never of the installed build's identity. Gating this on an exact-build match made an + // ordinary vendor auto-update silently withdraw the capability (and the server then + // resolved workspace_mode=fallback, reviewing a stale committed base with nobody told). + // See docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md. + var borrowedSupported = factory.SupportsBorrowedReviewFlow; // Reviewer MODEL override support: advertised true ONLY when this vendor — already known // installed + unattended-certified by ComputeUnattendedVendors above — also carries a // runtime-owned resolver. Vendor-neutral: we read the factory's resolver + its policy @@ -628,6 +633,30 @@ internal static IReadOnlyList ComputeUnattendedVendo return capabilities; } + /// Rendered in place of a CliVersion the probe could not determine (spawn + /// failure, timeout, empty output, unparseable output). It is a reporting placeholder ONLY — an + /// unidentifiable build is still a trusted build and its capabilities are unaffected. + internal const string UnknownCliVersion = "unknown"; + + /// + /// One Information line per unattended vendor recording the CLI version probed at daemon + /// startup, so an operator reading the daemon's own log can tell which build was installed when + /// this daemon came up. Deliberately reports the version and NOTHING else: it does not compare + /// the installed build against any validated-build record (that would be exactly the automated + /// version-drift detection this design rejects), and no equivalent line is emitted per launch. + /// Pure over the computed capabilities — same reasoning as + /// — so it is testable without booting the DI host. + /// See docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md. + /// + internal static void LogUnattendedVendorIdentities( + ILogger logger, IEnumerable capabilities) { + foreach (var capability in capabilities) + LogUnattendedVendorIdentity( + logger, + capability.Vendor, + string.IsNullOrWhiteSpace(capability.CliVersion) ? UnknownCliVersion : capability.CliVersion); + } + internal static string? ProbeCliVersion(string cliPath) { try { using var process = Process.Start(new ProcessStartInfo { @@ -677,6 +706,9 @@ static bool MatchVersionTerm(Version version, string term) { [LoggerMessage(Level = LogLevel.Information, Message = "kcap daemon '{Name}' starting, connecting to {ServerUrl}")] static partial void LogDaemonStarting(ILogger logger, string name, string serverUrl); + [LoggerMessage(Level = LogLevel.Information, Message = "Unattended vendor '{Vendor}': CLI version {CliVersion}, as observed by probing the configured binary at daemon startup. That is a startup observation, not the build a later reviewer runs — if the vendor updates while this daemon keeps running, launches pick up the new build and this line stays stale until the daemon restarts.")] + static partial void LogUnattendedVendorIdentity(ILogger logger, string vendor, string cliVersion); + [LoggerMessage(Level = LogLevel.Warning, Message = "Cursor ACP runtime unavailable: cursor-agent CLI not found (looked for '{CursorPath}'). Cursor will not be offered as a hosted-agent vendor until this is fixed. Set KCAP_CURSOR_PATH to the cursor-agent executable, or install the Cursor CLI, then restart the daemon.")] static partial void LogCursorUnavailable(ILogger logger, string cursorPath); diff --git a/src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs b/src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs index ce7934283..632a2741f 100644 --- a/src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs +++ b/src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntimeFactory.cs @@ -49,7 +49,7 @@ internal sealed partial class AcpHostedAgentRuntimeFactory( descriptor.BorrowedReviewContainment == AcpBorrowedReviewContainment.IndependentSnapshot; public string? BorrowedReviewContainment => descriptor.BorrowedReviewContainment switch { AcpBorrowedReviewContainment.NativeToolClamp => "native-tool-clamp", - AcpBorrowedReviewContainment.IndependentSnapshot => CursorBorrowedReviewCertification.Containment, + AcpBorrowedReviewContainment.IndependentSnapshot => CursorBorrowedReviewValidation.Containment, _ => null }; @@ -65,7 +65,7 @@ public async Task StartAsync(RuntimeStartContext ctx, Cancel LogLaunching(ctx.AgentId, Vendor, ctx.Worktree.Path); AcpMetrics.Launches.Add(1); - ValidateBorrowedArtifact(ctx, descriptor, config); + ValidateBorrowedArtifact(ctx, descriptor); // Fail closed BEFORE _connectionSource spawns a child (a later gate would leak one). Null for // a non-review launch; the built MCP list for a valid review flow. @@ -228,10 +228,11 @@ internal static ProcessStartInfo BuildProcessStartInfo( } } - var binaryPath = descriptor.Vendor == "cursor" && ctx.IsBorrowedSnapshot - ? CursorBorrowedReviewCertification.TryCertify(descriptor.ResolveBinaryPath(config))?.LauncherPath - ?? throw new InvalidOperationException("cursor_borrowed_artifact_not_certified") - : descriptor.ResolveBinaryPath(config); + // Every vendor — borrowed-snapshot review included — spawns the ordinary configured binary. + // Resolving a borrowed reviewer through an exact-build record instead made a vendor + // auto-update hard-fail the launch. See + // docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md. + var binaryPath = descriptor.ResolveBinaryPath(config); var psi = new ProcessStartInfo(binaryPath, argv) { WorkingDirectory = ctx.Worktree.Path, RedirectStandardInput = true, @@ -247,14 +248,16 @@ internal static ProcessStartInfo BuildProcessStartInfo( return psi; } - static void ValidateBorrowedArtifact( - RuntimeStartContext ctx, AcpVendorDescriptor descriptor, DaemonConfig config) { + /// Pre-spawn check that the descriptor's DECLARED containment agrees with the launch + /// context it was handed — a code-level invariant (a snapshot-materialized launch reaching a + /// vendor that does not declare independent-snapshot containment is a wiring bug). It is + /// deliberately NOT a build-identity check: capability is advertised for whatever build is + /// installed, so nothing here may consult a version record. See + /// docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md. + static void ValidateBorrowedArtifact(RuntimeStartContext ctx, AcpVendorDescriptor descriptor) { if (!ctx.IsReviewFlow || !ctx.IsBorrowedSnapshot) return; if (descriptor.BorrowedReviewContainment != AcpBorrowedReviewContainment.IndependentSnapshot) throw new InvalidOperationException("borrowed_snapshot_containment_mismatch"); - if (descriptor.Vendor == "cursor" && - CursorBorrowedReviewCertification.TryCertify(descriptor.ResolveBinaryPath(config)) is null) - throw new InvalidOperationException("cursor_borrowed_artifact_not_certified"); } /// Builds Copilot CLI's process-level stdio MCP config. Copilot's ACP capability diff --git a/src/Capacitor.Cli.Daemon/Services/CursorBorrowedReviewCertification.cs b/src/Capacitor.Cli.Daemon/Services/CursorBorrowedReviewValidation.cs similarity index 58% rename from src/Capacitor.Cli.Daemon/Services/CursorBorrowedReviewCertification.cs rename to src/Capacitor.Cli.Daemon/Services/CursorBorrowedReviewValidation.cs index b03c71f2f..f0f0005f6 100644 --- a/src/Capacitor.Cli.Daemon/Services/CursorBorrowedReviewCertification.cs +++ b/src/Capacitor.Cli.Daemon/Services/CursorBorrowedReviewValidation.cs @@ -7,17 +7,42 @@ namespace Capacitor.Cli.Daemon.Services; internal sealed record CursorBorrowedReviewArtifact(string Version, string LauncherPath, string BundleDigest); -/// Exact-build certification for Cursor's borrowed-review process boundary. Ordinary -/// Cursor launches remain available when this check fails; only the borrowed capability is -/// withdrawn. The check is repeated immediately before spawning a borrowed reviewer. -internal static class CursorBorrowedReviewCertification { +/// +/// A RECORD of the last Cursor build the maintainer validation workflow probed for borrowed-review +/// behavior — reads, containment, and process boundary. It gates nothing. A non-match is the +/// expected steady state (Cursor auto-updates) and is not an error: borrowed-review capability is +/// advertised for whatever build is installed, and a borrowed launch spawns the ordinary configured +/// binary. No caller may use to decide capability, to select a +/// launcher path, or to derive any runtime state; its only production caller is the manually +/// invoked maintainer validation workflow. See +/// docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md. +/// +/// Version form. is the version-directory name the launcher +/// resolves into (~/.local/share/cursor-agent/versions/<name>/cursor-agent) — that is +/// what compares. Observed on the installed macOS build, +/// cursor-agent --version prints exactly the same string (2026.07.23-e383d2b), so this +/// form and the CliVersion the daemon probes and logs at startup agree today. They remain +/// independent facts — a future Cursor release printing a bare semver would diverge from the +/// directory name without anything being wrong, and nothing in production compares them. +/// +/// Cursor auto-updated past this record on 2026-07-23, so the digests below describe an older +/// build than the one most machines run. Under trust-by-default that is a stale note, not a fault — +/// refresh it by re-running the maintainer probe suite against the installed build, never by +/// recomputing the hashes alone (that would record a validation that never happened). +/// +internal static class CursorBorrowedReviewValidation { internal const string Version = "2026.07.20-8cc9c0b"; internal const string LauncherSha256 = "eed61c5224668c9236334c4c68936a16aecc37374b592f59e31eb50433817831"; // SHA-256 of sorted UTF-8 lines: " ./\n". internal const string BundleDigest = "1dd66852ef6c94a0344226fa733f6fc1f3552a8ccf16dd000e1e38134575e10b"; internal const string Containment = "independent-snapshot"; - internal static CursorBorrowedReviewArtifact? TryCertify(string configuredPath) { + /// Reports whether the configured launcher resolves to the exact build recorded above. + /// Informational only — see the type-level remarks. Returns for any + /// mismatch, and also on any platform other than macOS/arm64, which is where the maintainer + /// probe suite runs; neither outcome says anything about whether borrowed review is + /// supported. + internal static CursorBorrowedReviewArtifact? TryMatchValidatedBuild(string configuredPath) { if (!OperatingSystem.IsMacOS()) return null; if (RuntimeInformation.ProcessArchitecture != Architecture.Arm64) return null; try { diff --git a/test/Capacitor.Cli.Tests.Unit/Daemon/DaemonRunnerCursorAvailabilityTests.cs b/test/Capacitor.Cli.Tests.Unit/Daemon/DaemonRunnerCursorAvailabilityTests.cs index 477bc0308..bfe846f0e 100644 --- a/test/Capacitor.Cli.Tests.Unit/Daemon/DaemonRunnerCursorAvailabilityTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Daemon/DaemonRunnerCursorAvailabilityTests.cs @@ -1,6 +1,10 @@ // test/Capacitor.Cli.Tests.Unit/Daemon/DaemonRunnerCursorAvailabilityTests.cs +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using Capacitor.Cli.Core; using Capacitor.Cli.Daemon; using Capacitor.Cli.Daemon.Services; +using Microsoft.Extensions.Logging; namespace Capacitor.Cli.Tests.Unit.Daemon; @@ -18,10 +22,12 @@ public class DaemonRunnerCursorAvailabilityTests { sealed class FakeRuntimeFactory( string vendor, bool isAvailable, bool supportsUnattended = false, bool supportsBorrowedReviewFlow = false, - IReviewerModelResolver? reviewerModelResolver = null) : IHostedAgentRuntimeFactory { + IReviewerModelResolver? reviewerModelResolver = null, + string? borrowedReviewContainment = null) : IHostedAgentRuntimeFactory { public string Vendor { get; } = vendor; public bool SupportsUnattended { get; } = supportsUnattended; public bool SupportsBorrowedReviewFlow { get; } = supportsBorrowedReviewFlow; + public string? BorrowedReviewContainment { get; } = borrowedReviewContainment; public IReviewerModelResolver? ReviewerModelResolver { get; } = reviewerModelResolver; public bool IsAvailable() => isAvailable; @@ -85,7 +91,7 @@ await Assert.That(DaemonRunner.ComputeUnattendedVendors(factories)) } [Test] - public async Task ComputeUnattendedVendors_ExcludesAvailableCursor_WithoutCertification() { + public async Task ComputeUnattendedVendors_ExcludesAvailableCursor_WhenItDoesNotSupportUnattended() { IHostedAgentRuntimeFactory[] factories = [ new FakeRuntimeFactory("claude", isAvailable: true, supportsUnattended: true), new FakeRuntimeFactory("codex", isAvailable: true, supportsUnattended: true), @@ -140,11 +146,24 @@ await Assert.That(claude.LauncherPolicyVersion) await Assert.That(claude.BorrowedReviewSupported).IsFalse(); } + // === Trust-by-default borrowed-review advertisement === + // (docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md) + // + // These replace an earlier test that pinned the opposite rule ("a Cursor build that doesn't + // match the validated-build record advertises BorrowedReviewSupported=false"). That gate is the + // bug: Cursor auto-updates, the daemon silently withdrew borrowed capability, and the server + // then resolved workspace_mode=fallback — reviewing a stale committed base with nobody told. + + /// THE regression test for this bug. The configured Cursor path deliberately does not + /// resolve to the validated build (it does not resolve to any binary at all, which is a strictly + /// harder non-match than an updated build), and Cursor is STILL advertised borrowed-capable with + /// its containment token. Note the same inputs also drive the null-CliVersion path: an + /// unidentifiable build is still a trusted build. [Test] - public async Task ComputeUnattendedVendorCapabilities_WithMissingCursorArtifact_FailsClosed() { + public async Task ComputeUnattendedVendorCapabilities_CursorBuildNotMatchingValidationRecord_StillAdvertisesBorrowed() { IHostedAgentRuntimeFactory[] factories = [ new FakeRuntimeFactory("cursor", isAvailable: true, supportsUnattended: true, - supportsBorrowedReviewFlow: true), + supportsBorrowedReviewFlow: true, borrowedReviewContainment: "independent-snapshot"), ]; var capabilities = DaemonRunner.ComputeUnattendedVendorCapabilities( @@ -152,13 +171,246 @@ public async Task ComputeUnattendedVendorCapabilities_WithMissingCursorArtifact_ await Assert.That(capabilities).Count().IsEqualTo(1); await Assert.That(capabilities[0].Vendor).IsEqualTo("cursor"); - await Assert.That(capabilities[0].CliVersion).IsNull(); await Assert.That(capabilities[0].LauncherPolicyVersion) .IsEqualTo(DaemonRunner.CursorLauncherPolicyVersion); + await Assert.That(capabilities[0].BorrowedReviewSupported).IsTrue(); + await Assert.That(capabilities[0].BorrowedReviewContainment).IsEqualTo("independent-snapshot"); + // An unprobeable version does not cost the vendor its capability. + await Assert.That(capabilities[0].CliVersion).IsNull(); + } + + /// The gate that REMAINS: borrowed support is exactly the factory's own + /// , so a factory that does + /// not support borrowed review is still never advertised (and carries no containment token). + /// Cursor is used deliberately — the removed special case was Cursor-only, so proving the + /// remaining gate on Cursor proves it was not replaced by a blanket "always true". + [Test] + public async Task ComputeUnattendedVendorCapabilities_FactoryWithoutBorrowedSupport_IsNotAdvertised() { + IHostedAgentRuntimeFactory[] factories = [ + new FakeRuntimeFactory("cursor", isAvailable: true, supportsUnattended: true, + supportsBorrowedReviewFlow: false, borrowedReviewContainment: "independent-snapshot"), + ]; + + var capabilities = DaemonRunner.ComputeUnattendedVendorCapabilities( + factories, new DaemonConfig { CursorPath = "/definitely/missing/cursor-agent" }); + await Assert.That(capabilities[0].BorrowedReviewSupported).IsFalse(); await Assert.That(capabilities[0].BorrowedReviewContainment).IsNull(); } + /// The advertisement is a pure function of the factory flag for EVERY vendor token — + /// no residual per-vendor arm survives for Cursor or anyone else. + [Test] + [Arguments("cursor", true)] + [Arguments("cursor", false)] + [Arguments("copilot", true)] + [Arguments("copilot", false)] + [Arguments("claude", true)] + [Arguments("newvendor", true)] + public async Task ComputeUnattendedVendorCapabilities_BorrowedAdvertisementMirrorsTheFactoryFlag( + string vendor, bool supportsBorrowed) { + IHostedAgentRuntimeFactory[] factories = [ + new FakeRuntimeFactory(vendor, isAvailable: true, supportsUnattended: true, + supportsBorrowedReviewFlow: supportsBorrowed, + borrowedReviewContainment: "independent-snapshot"), + ]; + + var capabilities = DaemonRunner.ComputeUnattendedVendorCapabilities(factories, new DaemonConfig()); + + await Assert.That(capabilities[0].BorrowedReviewSupported).IsEqualTo(supportsBorrowed); + } + + // === Platform coverage === + // + // Dropping the gate also dropped the macOS/arm64 preconditions that lived inside the + // validated-build record, so borrowed review is now advertised on EVERY OS and architecture + // where the vendor CLI is installed. The design accepts that expansion: the snapshot path + // (WorktreeManager.CreateBorrowedSnapshotAsync / SyncFromSourceAsync) is platform-neutral git + + // managed file operations with explicit Windows branches, and neither the ACP factory nor the + // Cursor descriptor carries any other OS/arch gate. + // + // Coverage matrix for that claim: + // • linux-x64 — asserted, executed on CI's ubuntu-latest runner. + // • windows-x64 — asserted, executed on CI's windows-latest runner. + // • macos-arm64 — asserted, executed on maintainer machines (the suite's development host). + // • macos-x64 — INTENTIONALLY UNTESTED. It has no runner in this repo's CI matrix, and + // adding one is not worthwhile here: the unit suite has a known cluster of macOS-local + // temp-path/parallel-load flakes that would make a macOS CI leg permanently red, and macOS + // is already exercised on arm64 by maintainers. The residual risk is bounded by + // AdvertisementPathHasNoBuildIdentityGate below, which proves — from any host — that no + // platform-conditional build-identity check remains in either the advertisement or the + // launch path, so no macos-x64-specific behavior can differ from macos-arm64 here. + + /// Asserts the advertisement on whichever platform is executing, so the CI matrix + /// (linux-x64, windows-x64) plus maintainer runs (macos-arm64) each independently prove it. The + /// host is named in the assertion message so a platform-specific failure is legible. + [Test] + public async Task ComputeUnattendedVendorCapabilities_AdvertisesCursorBorrowed_OnTheExecutingPlatform() { + var host = $"{RuntimeInformation.RuntimeIdentifier} ({RuntimeInformation.OSDescription.Trim()})"; + IHostedAgentRuntimeFactory[] factories = [ + new FakeRuntimeFactory("cursor", isAvailable: true, supportsUnattended: true, + supportsBorrowedReviewFlow: true, borrowedReviewContainment: "independent-snapshot"), + ]; + + var capabilities = DaemonRunner.ComputeUnattendedVendorCapabilities( + factories, new DaemonConfig { CursorPath = "/definitely/missing/cursor-agent" }); + + await Assert.That(capabilities[0].BorrowedReviewSupported) + .IsTrue().Because($"borrowed review must be advertised on {host}"); + await Assert.That(capabilities[0].BorrowedReviewContainment) + .IsEqualTo("independent-snapshot").Because($"containment must be advertised on {host}"); + } + + /// + /// A source-level guard, in the shape of ReviewerModelVendorNeutralityGuardTests: neither + /// the advertisement path (DaemonRunner.cs) nor the launch path + /// (AcpHostedAgentRuntimeFactory.cs) may consult the validated-build record. This is what + /// makes the platform matrix above sound — a re-introduced gate would fail here from ANY host, + /// including for the macos-x64 leg no runner executes, whereas a behavioral assertion can only + /// speak for the platform it runs on. The record's own file and its tests are of course allowed + /// to name it. + /// + [Test] + public async Task AdvertisementAndLaunchPaths_HaveNoBuildIdentityGate() { + var daemonSrc = Path.Combine(RepoRoot(), "src", "Capacitor.Cli.Daemon"); + var guarded = new[] { + Path.Combine(daemonSrc, "DaemonRunner.cs"), + Path.Combine(daemonSrc, "Services", "AcpHostedAgentRuntimeFactory.cs"), + }; + + var violations = new List(); + foreach (var file in guarded) { + var lines = await File.ReadAllLinesAsync(file); + for (var i = 0; i < lines.Length; i++) { + if (lines[i].TrimStart().StartsWith("//", StringComparison.Ordinal)) continue; + if (lines[i].Contains("TryMatchValidatedBuild", StringComparison.Ordinal) || + lines[i].Contains("CursorBorrowedReviewArtifact", StringComparison.Ordinal)) + violations.Add($"{Path.GetFileName(file)}:{i + 1}: {lines[i].Trim()}"); + } + } + + await Assert.That(violations).IsEmpty(); + } + + /// Self-test for the guard above — it must actually detect a re-introduced gate rather + /// than pass because the file paths are wrong or the scan is inert. + [Test] + public async Task AdvertisementGuard_DetectsAReintroducedBuildIdentityGate() { + var dir = Directory.CreateTempSubdirectory("kcap-build-identity-guard-"); + try { + var file = Path.Combine(dir.FullName, "DaemonRunner.cs"); + await File.WriteAllLinesAsync(file, [ + "// a comment naming TryMatchValidatedBuild must NOT count", + "var artifact = CursorBorrowedReviewValidation.TryMatchValidatedBuild(cliPath);", + ]); + + var lines = await File.ReadAllLinesAsync(file); + var hits = lines + .Where(l => !l.TrimStart().StartsWith("//", StringComparison.Ordinal)) + .Count(l => l.Contains("TryMatchValidatedBuild", StringComparison.Ordinal)); + + await Assert.That(hits).IsEqualTo(1); + } finally { + try { dir.Delete(recursive: true); } catch { } + } + } + + /// Walks up from this file's compile-time path to the repo root, so the guard is + /// independent of the runner's working directory. + static string RepoRoot([CallerFilePath] string here = "") { + var dir = Path.GetDirectoryName(here); + while (dir is not null && !File.Exists(Path.Combine(dir, "Capacitor.slnx"))) + dir = Path.GetDirectoryName(dir); + + return dir ?? throw new InvalidOperationException($"Could not locate repo root walking up from {here}"); + } + + // === Startup identity logging === + + /// Records rendered log entries so the startup identity line can be asserted without + /// booting the DI host RunAsync builds. + sealed class CaptureLogger : ILogger { + public readonly List<(LogLevel Level, string Message)> Entries = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel level, EventId id, TState state, Exception? ex, Func formatter) + => Entries.Add((level, formatter(state, ex))); + } + + [Test] + public async Task LogUnattendedVendorIdentities_EmitsOneInformationLinePerVendor_NamingTheProbedVersion() { + var logger = new CaptureLogger(); + + DaemonRunner.LogUnattendedVendorIdentities(logger, [ + new("claude", "2.1.0", DaemonRunner.ClaudeLauncherPolicyVersion, false), + new("cursor", "2026.07.23-e383d2b", DaemonRunner.CursorLauncherPolicyVersion, true, "independent-snapshot"), + ]); + + var info = logger.Entries.Where(e => e.Level == LogLevel.Information).ToList(); + await Assert.That(info).Count().IsEqualTo(2); + await Assert.That(info).Contains(e => e.Message.Contains("claude") && e.Message.Contains("2.1.0")); + await Assert.That(info).Contains(e => e.Message.Contains("cursor") && e.Message.Contains("2026.07.23-e383d2b")); + } + + /// A probe that produced nothing usable renders the literal unknown — never a + /// blank, never an omitted line — and the vendor keeps its borrowed capability, since an + /// unidentifiable build is still a trusted build. + [Test] + [Arguments(null)] + [Arguments("")] + [Arguments(" ")] + public async Task LogUnattendedVendorIdentities_NullOrBlankProbe_RendersUnknown_AndDoesNotAffectCapability( + string? probed) { + var logger = new CaptureLogger(); + var capability = new UnattendedVendorCapability( + "cursor", probed, DaemonRunner.CursorLauncherPolicyVersion, true, "independent-snapshot"); + + DaemonRunner.LogUnattendedVendorIdentities(logger, [capability]); + + var line = logger.Entries.Single(e => e.Level == LogLevel.Information).Message; + await Assert.That(line).Contains("unknown"); + await Assert.That(capability.BorrowedReviewSupported).IsTrue(); + await Assert.That(capability.BorrowedReviewContainment).IsEqualTo("independent-snapshot"); + } + + /// The wording must mark the value as a daemon-startup observation, so nobody reads it + /// as the build a later reviewer actually ran — an update after startup makes it stale, which is + /// precisely the situation that caused this bug. + [Test] + public async Task LogUnattendedVendorIdentities_WordsTheVersionAsAStartupObservation_NotALaunchFact() { + var logger = new CaptureLogger(); + + DaemonRunner.LogUnattendedVendorIdentities(logger, [ + new("cursor", "2026.07.23-e383d2b", DaemonRunner.CursorLauncherPolicyVersion, true, "independent-snapshot"), + ]); + + var line = logger.Entries.Single().Message; + await Assert.That(line).Contains("daemon startup"); + await Assert.That(line).Contains("stale"); + } + + /// NEGATIVE test, and it is load-bearing: the startup line reports the version and + /// nothing else. Computing whether the installed build agrees with the validated-build record + /// would be automated version-drift detection — the thing this design deliberately does not do — + /// and would hand the demoted record a production caller. A test that demanded such an + /// assertion would drag the rejected behavior straight back in. + [Test] + public async Task LogUnattendedVendorIdentities_DoesNotCompareAgainstTheValidatedBuildRecord() { + var logger = new CaptureLogger(); + + DaemonRunner.LogUnattendedVendorIdentities(logger, [ + new("cursor", "2026.07.23-e383d2b", DaemonRunner.CursorLauncherPolicyVersion, true, "independent-snapshot"), + ]); + + var line = logger.Entries.Single().Message; + await Assert.That(line).DoesNotContain("2026.07.20-8cc9c0b"); + foreach (var token in new[] { "certif", "validated", "mismatch", "drift", "uncertified" }) + await Assert.That(line.Contains(token, StringComparison.OrdinalIgnoreCase)) + .IsFalse().Because($"the startup identity line must not evaluate build agreement ('{token}')"); + } + [Test] public async Task CliVersionAllowed_EvaluatesConfiguredRange() { await Assert.That(DaemonRunner.CliVersionAllowed("v1.2.3", ">=1.2.0 <2.0.0")).IsTrue(); diff --git a/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryTests.cs index 88ad21e89..14b519ccd 100644 --- a/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Services/AcpHostedAgentRuntimeFactoryTests.cs @@ -1061,6 +1061,115 @@ public async Task BuildProcessStartInfo_Cursor_RawBorrowedReviewRequiresSnapshot await Assert.That(ex.Message).Contains("snapshot materialization"); } + // ── Trust-by-default borrowed-snapshot launch ───────────────────────────────────────────── + // docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md + // + // A borrowed-snapshot Cursor launch used to resolve its binary through — and be re-gated on — an + // exact-build record, so an ordinary Cursor auto-update turned the launch into a hard + // `cursor_borrowed_artifact_not_certified` throw. Capability is now advertised for whatever + // build is installed, and the launch path must agree with that advertisement. + + /// A snapshot-materialized borrowed review flow: Work is the daemon-owned + /// snapshot worktree (what the orchestrator hands the factory) with the borrowed-snapshot + /// marker set. + static RuntimeStartContext BorrowedSnapshotContext() => + ReviewContext() with { Work = WorkLocation.OwnedWorktree, IsBorrowedSnapshot = true }; + + /// The borrowed-snapshot binary is the plainly configured one — the same path every + /// other vendor and every other launch shape uses. A configured path that matches no validated + /// build (this one resolves to nothing at all) must still be spawned verbatim. + [Test] + public async Task BuildProcessStartInfo_Cursor_BorrowedSnapshot_UsesTheConfiguredBinaryPath() { + var psi = AcpHostedAgentRuntimeFactory.BuildProcessStartInfo( + AcpVendorDescriptors.Cursor, + new DaemonConfig { CursorPath = "/definitely/not/a/validated/build/cursor-agent" }, + BorrowedSnapshotContext()); + + await Assert.That(psi.FileName).IsEqualTo("/definitely/not/a/validated/build/cursor-agent"); + await Assert.That(psi.ArgumentList.SequenceEqual(["acp", "--force", "--approve-mcps", "--trust"])).IsTrue(); + } + + /// THE launch-side regression test: a borrowed Cursor reviewer on a build that matches + /// no validated-build record SPAWNS instead of throwing + /// cursor_borrowed_artifact_not_certified. + [Test] + public async Task ReviewFlow_Cursor_BorrowedSnapshot_OnANonMatchingBuild_Spawns() { + var fake = new FakeAcpAgent(); + var spawns = 0; + var factory = new AcpHostedAgentRuntimeFactory( + descriptor: AcpVendorDescriptors.Cursor, + config: new DaemonConfig { CursorPath = "/definitely/not/a/validated/build/cursor-agent" }, + loggerFactory: NullLoggerFactory.Instance, + connection: new CaptureServerConnection(), + connectionSource: _ => { + Interlocked.Increment(ref spawns); + return (fake.ClientWriteStream, fake.ClientReadStream, new FakeAcpProcess()); + }); + + using var cts = new CancellationTokenSource(); + var fakeRunTask = fake.RunAsync(cts.Token); + + var started = await factory.StartAsync(BorrowedSnapshotContext(), cts.Token).WaitAsync(HangGuard); + + await Assert.That(Volatile.Read(ref spawns)).IsEqualTo(1); + + cts.Cancel(); + try { await fakeRunTask.WaitAsync(HangGuard); } catch (OperationCanceledException) { } + await started.Runtime.DisposeAsync(); + await fake.DisposeAsync(); + } + + /// The gate that REMAINS: a snapshot-materialized launch handed to a vendor that does + /// not declare independent-snapshot containment is a wiring bug, and still fails before spawn. + /// Copilot declares native-tool-clamp containment, so it is the realistic mismatch. + [Test] + public async Task ReviewFlow_BorrowedSnapshot_ContainmentMismatch_StillThrowsBeforeSpawn() { + var (factory, spawns) = CountingSpawnFactory(AcpVendorDescriptors.Copilot); + + var ex = await Assert.That(async () => await factory.StartAsync( + ReviewContext(["kcap-review"]) with { Work = WorkLocation.OwnedWorktree, IsBorrowedSnapshot = true }, + CancellationToken.None)) + .Throws(); + + await Assert.That(ex!.Message).IsEqualTo("borrowed_snapshot_containment_mismatch"); + await Assert.That(spawns()).IsEqualTo(0); + } + + /// NEGATIVE test: a launch logs no vendor version and no resolved binary path. Adding + /// either would be per-launch drift telemetry, which this design rejects — resolving a + /// meaningful path would require final-symlink inspection added specifically to reveal version + /// drift, and it would still be racy. A test demanding such a line would reintroduce the + /// rejected behavior. + [Test] + public async Task StartAsync_BorrowedSnapshot_LogsNoVendorVersionAndNoBinaryPath() { + var fake = new FakeAcpAgent(); + var loggerFactory = new CaptureLoggerFactory(); + var binaryPath = "/definitely/not/a/validated/build/cursor-agent"; + + var factory = new AcpHostedAgentRuntimeFactory( + descriptor: AcpVendorDescriptors.Cursor, + config: new DaemonConfig { CursorPath = binaryPath }, + loggerFactory: loggerFactory, + connection: new CaptureServerConnection(), + connectionSource: _ => (fake.ClientWriteStream, fake.ClientReadStream, new FakeAcpProcess())); + + using var cts = new CancellationTokenSource(); + var fakeRunTask = fake.RunAsync(cts.Token); + + var started = await factory.StartAsync(BorrowedSnapshotContext(), cts.Token).WaitAsync(HangGuard); + + foreach (var (_, message) in loggerFactory.Logger.Entries) { + await Assert.That(message.Contains(binaryPath, StringComparison.Ordinal)).IsFalse(); + await Assert.That(message.Contains("cursor-agent", StringComparison.Ordinal)).IsFalse(); + await Assert.That(message.Contains(CursorBorrowedReviewValidation.Version, StringComparison.Ordinal)).IsFalse(); + } + + cts.Cancel(); + try { await fakeRunTask.WaitAsync(HangGuard); } catch (OperationCanceledException) { } + await started.Runtime.DisposeAsync(); + await fake.DisposeAsync(); + } + /// A full StartAsync for the Copilot descriptor: handshake completes, the started /// runtime's Vendor is `copilot`, and an interactive launch sends `mcpServers: []`. [Test] diff --git a/test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewCertificationTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewCertificationTests.cs deleted file mode 100644 index 07bada987..000000000 --- a/test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewCertificationTests.cs +++ /dev/null @@ -1,22 +0,0 @@ -using Capacitor.Cli.Daemon.Services; - -namespace Capacitor.Cli.Tests.Unit.Services; - -public class CursorBorrowedReviewCertificationTests { - [Test] - public async Task BundleDigest_IgnoresTransientRunningDirectory() { - var root = Directory.CreateTempSubdirectory("cursor-certification-"); - try { - File.WriteAllText(Path.Combine(root.FullName, "cursor-agent"), "artifact"); - var before = CursorBorrowedReviewCertification.ComputeBundleDigest(root.FullName); - var running = Directory.CreateDirectory(Path.Combine(root.FullName, ".running")); - File.WriteAllText(Path.Combine(running.FullName, "12345"), ""); - - var after = CursorBorrowedReviewCertification.ComputeBundleDigest(root.FullName); - - await Assert.That(after).IsEqualTo(before); - } finally { - try { root.Delete(recursive: true); } catch { } - } - } -} diff --git a/test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewValidationTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewValidationTests.cs new file mode 100644 index 000000000..48b57e498 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/Services/CursorBorrowedReviewValidationTests.cs @@ -0,0 +1,32 @@ +using Capacitor.Cli.Daemon.Services; + +namespace Capacitor.Cli.Tests.Unit.Services; + +public class CursorBorrowedReviewValidationTests { + [Test] + public async Task BundleDigest_IgnoresTransientRunningDirectory() { + var root = Directory.CreateTempSubdirectory("cursor-validation-"); + try { + File.WriteAllText(Path.Combine(root.FullName, "cursor-agent"), "artifact"); + var before = CursorBorrowedReviewValidation.ComputeBundleDigest(root.FullName); + var running = Directory.CreateDirectory(Path.Combine(root.FullName, ".running")); + File.WriteAllText(Path.Combine(running.FullName, "12345"), ""); + + var after = CursorBorrowedReviewValidation.ComputeBundleDigest(root.FullName); + + await Assert.That(after).IsEqualTo(before); + } finally { + try { root.Delete(recursive: true); } catch { } + } + } + + /// The marker is informational: a build that does not match it must still return + /// WITHOUT throwing, because every production path now treats a non-match + /// as the ordinary steady state rather than an error. + [Test] + public async Task TryMatchValidatedBuild_NonMatchingPath_ReturnsNullWithoutThrowing() { + var path = Path.Combine(Path.GetTempPath(), "kcap-not-cursor-" + Guid.NewGuid().ToString("N")[..8]); + + await Assert.That(CursorBorrowedReviewValidation.TryMatchValidatedBuild(path)).IsNull(); + } +} From f5979886b5666126bd6f5882d493285e5d0214e7 Mon Sep 17 00:00:00 2001 From: realtonyyoung <6655045+realtonyyoung@users.noreply.github.com> Date: Tue, 28 Jul 2026 00:16:36 -0400 Subject: [PATCH 2/2] test: deterministic coverage for the borrowed-snapshot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the two-checkout orchestrator integration test the borrowed-snapshot path never had. It launches with RepoPath = a daemon-registered decoy checkout and BorrowCwd = a divergent requester checkout, so the load-bearing assertion is that WorktreeInfo.SourceRepo is the REQUESTER's git root — a single-directory test satisfies every content assertion while passing just as happily against the stale-base behavior it exists to prevent. Also asserts: branch-only commit, tracked-modified content and non-ignored untracked file present; .gitignore'd file and decoy content absent; snapshot root outside the source repo; reviewer mutation isolated; per-round refresh picks up requester changes AND drops reviewer-only files and git metadata. The "byte-unchanged" claim uses a real content-hash baseline (per-path SHA-256 plus unix mode) re-checked after snapshot creation, reviewer mutation, refresh and stop, plus HEAD sha + porcelain status for git state. SnapshotTree is not reused: it records entry names only, so it would miss in-place corruption. Docs: state the trust-by-default posture in CLAUDE.md and correct the README's "certified borrowed-context strategy" wording. CI matrix records why macOS is absent and how that platform is covered instead. --- .github/workflows/ci.yml | 8 + CLAUDE.md | 11 +- README.md | 2 +- .../AgentOrchestratorBorrowLaunchTests.cs | 209 ++++++++++++++++++ 4 files changed, 228 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c20f889ad..e0d63fe7d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,6 +21,14 @@ jobs: strategy: fail-fast: false matrix: + # linux-x64 + windows-x64. macOS is NOT in this matrix: the unit suite has + # a known cluster of macOS-local temp-path/parallel-load flakes that would + # make a macOS leg permanently red. macOS arm64 is covered by maintainer + # runs; macOS x64 is intentionally untested. Platform-sensitive daemon + # behavior that must hold everywhere (e.g. the borrowed-review capability + # advertisement, which no longer carries any OS/arch precondition) is + # additionally pinned by host-independent source guards in the unit suite — + # see DaemonRunnerCursorAvailabilityTests' platform coverage matrix. os: [ubuntu-latest, windows-latest] steps: diff --git a/CLAUDE.md b/CLAUDE.md index 97490b6a9..d8c738787 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,9 +13,18 @@ registers `kcap-flows` without auto-approval and tracks only newly-created globa the string unattended-vendor list for compatibility and additionally advertise structured per-vendor CLI/launcher-policy capabilities. Cursor serves borrowed review context from a daemon-owned snapshot (dirty tracked and non-ignored untracked files, refreshed between rounds), -because its zero-interaction modes may write; Claude borrowed-worktree review remains uncertified +because its zero-interaction modes may write; Claude has no borrowed-review containment strategy and therefore fails closed to an owned worktree. +Borrowed-review capability is **trust-by-default**: a vendor advertises it whenever its runtime +factory declares a containment strategy, for whatever build of the vendor CLI is installed and on +every platform. It is deliberately not gated on the installed binary matching a validated-build +record — a vendor auto-update would then silently withdraw the capability and reviewers would fall +back to a stale committed base. The daemon logs the CLI version it probed at startup (a startup +observation, not a launch-time fact) and does no automated drift detection; a defective vendor +release is handled by a human report and a corrected record. See +`docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md` in kcap-server. + ## Tech stack - .NET 10, NativeAOT compiled diff --git a/README.md b/README.md index 2713ade0d..08fe613e2 100644 --- a/README.md +++ b/README.md @@ -411,7 +411,7 @@ Stdio MCP server that lets coding agents start and interact with AI-powered agen It provides four generic tools: -- **`start_flow`** — start a new flow from either `definition_id` (the server's flow-definition catalog, e.g. `spec-review`, `code-review`, or a custom definition) **or** `definition_yaml` (an inline dynamic flow definition — the full YAML, same schema as catalog definitions); provide exactly one of the two. Catalog starts use the guarded v2 protocol. A `definition_yaml` flow has extra constraints: every participant must declare `workspace: none` and a concrete, priced model (no `default`), the server clamps `limits`/`mcp` to its own caps rather than trusting the definition, may reject the whole thing with a coded error, and requires a server with dynamic flows enabled. Also provide `target_kind`, `target_ref`, `target_title`, and `context`. Requester context (session ID, cwd, repo root, owner, name) is resolved automatically from the environment. Returns a `flow_run_id`. A single-participant definition starts its first round eagerly; a multi-participant definition starts **round-less** — the response carries no round, and each declared role's agent launches lazily on its first `send_to_participant`. Optional `mode`: by default, when the daemon runs on the same machine and the selected vendor advertises a certified borrowed-context strategy, the participant sees current tracked, dirty, and non-ignored untracked checkout content; the safety boundary is vendor-specific. Cursor runs in a daemon-owned snapshot refreshed before follow-up rounds, while direct borrowing is reserved for runtimes with a certified native read-only clamp. Pass `mode="context-only"` to opt out. Claude review flows currently use owned worktrees because borrowed-checkout review is not certified. Optional `vendor`: for reserved `spec-review`/`code-review`, explicitly selects the reviewer independently of the driver; when omitted the server uses `Flows:Review:DefaultVendor`. Custom single-participant catalog definitions retain their authored vendor unless explicitly overridden. The server records requested/applied vendor plus selection source, and rejects an unavailable or uncertified vendor without silent fallback. Dynamic (`definition_yaml`) flows reject a top-level vendor override because each participant declares its own vendor. Optional `model`: a per-run reviewer **model** override for a single-participant catalog review — REQUIRES `vendor` (the model is interpreted against that vendor; there is no vendor→model table anywhere in the CLI, so `model` without `vendor` is rejected locally before any request is sent) and is rejected on a `definition_yaml` (dynamic) or multi-participant start (each participant already pins its own model). Pass the vendor's own model id or alias verbatim, case-sensitive — the CLI never translates, canonicalizes, or guesses it. +- **`start_flow`** — start a new flow from either `definition_id` (the server's flow-definition catalog, e.g. `spec-review`, `code-review`, or a custom definition) **or** `definition_yaml` (an inline dynamic flow definition — the full YAML, same schema as catalog definitions); provide exactly one of the two. Catalog starts use the guarded v2 protocol. A `definition_yaml` flow has extra constraints: every participant must declare `workspace: none` and a concrete, priced model (no `default`), the server clamps `limits`/`mcp` to its own caps rather than trusting the definition, may reject the whole thing with a coded error, and requires a server with dynamic flows enabled. Also provide `target_kind`, `target_ref`, `target_title`, and `context`. Requester context (session ID, cwd, repo root, owner, name) is resolved automatically from the environment. Returns a `flow_run_id`. A single-participant definition starts its first round eagerly; a multi-participant definition starts **round-less** — the response carries no round, and each declared role's agent launches lazily on its first `send_to_participant`. Optional `mode`: by default, when the daemon runs on the same machine and the selected vendor declares a borrowed-context containment strategy, the participant sees current tracked, dirty, and non-ignored untracked checkout content; the safety boundary is vendor-specific. Cursor runs in a daemon-owned snapshot refreshed before follow-up rounds, while direct borrowing is reserved for runtimes with a native read-only tool clamp. The capability is advertised for whatever build of the vendor CLI is installed, on every platform where it is installed — it is not gated on a per-version validation record, since a vendor auto-update would then silently drop the participant back onto a stale committed base. Pass `mode="context-only"` to opt out. Claude review flows currently use owned worktrees because it declares no borrowed-review containment strategy. Optional `vendor`: for reserved `spec-review`/`code-review`, explicitly selects the reviewer independently of the driver; when omitted the server uses `Flows:Review:DefaultVendor`. Custom single-participant catalog definitions retain their authored vendor unless explicitly overridden. The server records requested/applied vendor plus selection source, and rejects an unavailable or uncertified vendor without silent fallback. Dynamic (`definition_yaml`) flows reject a top-level vendor override because each participant declares its own vendor. Optional `model`: a per-run reviewer **model** override for a single-participant catalog review — REQUIRES `vendor` (the model is interpreted against that vendor; there is no vendor→model table anywhere in the CLI, so `model` without `vendor` is rejected locally before any request is sent) and is rejected on a `definition_yaml` (dynamic) or multi-participant start (each participant already pins its own model). Pass the vendor's own model id or alias verbatim, case-sensitive — the CLI never translates, canonicalizes, or guesses it. - **`send_to_participant`** — send a follow-up message to a participant role declared by the flow definition (single-participant definitions use `"reviewer"`; the server rejects an unknown role, naming the valid ones). One round runs at a time per role — a second send to a busy role is rejected naming the busy round, while other roles remain addressable. Returns the new round's findings. - **`get_flow_status`** — get the current status (running, waiting, completed, failed) and last result of a flow run. - **`close_flow`** — mark a completed flow run as closed. diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorBorrowLaunchTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorBorrowLaunchTests.cs index a413ce2c5..04669b65e 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorBorrowLaunchTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorBorrowLaunchTests.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using Capacitor.Cli.Core; using Capacitor.Cli.Core.LocalIpc; using Capacitor.Cli.Daemon; @@ -95,6 +96,150 @@ await Assert.That(File.ReadAllText(Path.Combine(ctx.Worktree.Path, "README.md")) } } + // ── Borrowed-snapshot regression net (two distinct checkouts) ───────────────────────── + // docs/superpowers/specs/2026-07-27-ai1528-trust-by-default-borrowed-review-design.md + // + // Until the capability gate above was removed this path had, on the evidence in that design, + // never executed in production — the daemon advertised no borrowed support, the server resolved + // workspace_mode=fallback, and reviewers ran in an owned worktree at the last commit. Its + // correctness was therefore unevidenced, which is what this test supplies. + // + // TWO checkouts are mandatory. The orchestrator snapshots the borrow cwd's own canonical git + // root, deliberately INDEPENDENT of the launch command's registered RepoPath, so a test pointing + // both at one directory satisfies every content assertion below while proving nothing about + // which checkout was selected — it would pass just as happily against the stale-base behavior + // this test exists to prevent. The launch below registers the DAEMON checkout as RepoPath and + // borrows the REQUESTER checkout, and the load-bearing assertion is that the snapshot's + // SourceRepo is the requester's git root. + + [Test] + public async Task Borrowed_snapshot_is_built_from_the_requester_checkout_not_the_registered_repo() { + var (daemonRepo, cleanupDaemon) = CreateGitRepo(); + var (requesterRepo, cleanupRequester) = CreateGitRepo(); + try { + // The daemon-registered checkout is a decoy: everything in it is distinguishable from + // the requester's, so any content leaking from it is caught by value, not by absence. + File.WriteAllText(Path.Combine(daemonRepo, "README.md"), "daemon-checkout-decoy"); + File.WriteAllText(Path.Combine(daemonRepo, "daemon-only.txt"), "daemon-only"); + Git(daemonRepo, "add", "-A"); + Git(daemonRepo, "commit", "-q", "-m", "daemon decoy"); + + // The requester carries all four shapes the snapshot has to get right. + Git(requesterRepo, "checkout", "-q", "-b", "feature"); + File.WriteAllText(Path.Combine(requesterRepo, "branch-only.txt"), "branch-only-committed"); + Git(requesterRepo, "add", "-A"); + Git(requesterRepo, "commit", "-q", "-m", "branch-only commit"); + File.WriteAllText(Path.Combine(requesterRepo, "README.md"), "requester-modified"); + File.WriteAllText(Path.Combine(requesterRepo, "untracked.txt"), "requester-untracked"); + File.WriteAllText(Path.Combine(requesterRepo, ".gitignore"), "ignored.txt\n"); + File.WriteAllText(Path.Combine(requesterRepo, "ignored.txt"), "must-not-be-snapshotted"); + + var canonicalRequester = BorrowAuthorizer.Canonicalize(requesterRepo); + var canonicalDaemon = BorrowAuthorizer.Canonicalize(daemonRepo); + + var server = new CaptureServerConnection(); + var factory = new SpyHostedAgentRuntimeFactory("cursor") { + SupportsUnattended = true, + SupportsBorrowedReviewFlow = true, + BorrowedReviewRequiresIndependentSnapshot = true + }; + await using var orch = BuildOrchestrator( + server, new SpyPtyProcessFactory(), + new Dictionary(), + extraRuntimeFactories: [factory]); + + var baseline = ContentBaseline(canonicalRequester); + var gitState = GitState(canonicalRequester); + + var cmd = new LaunchAgentCommand( + "agent-two-checkout", "review", "default", null, + RepoPath: daemonRepo, null, null, + Vendor: "cursor", Kind: LaunchKind.ReviewFlow, + Borrowed: true, BorrowCwd: requesterRepo); + + await orch.HandleLaunchAgentForTest(cmd); + + await Assert.That(server.LaunchFailedCalls).IsEmpty(); + var ctx = factory.LastContext!; + + // (1) THE assertion that pins the fix: the snapshot was derived from the requester's git + // root, not the registered one. Everything below is secondary to this. + await Assert.That(ctx.Worktree.SourceRepo).IsEqualTo(canonicalRequester); + await Assert.That(ctx.Worktree.SourceRepo).IsNotEqualTo(canonicalDaemon); + await Assert.That(orch.GetAgentForTest(cmd.AgentId)!.BorrowedSnapshotSource) + .IsEqualTo(canonicalRequester); + + // (2) Contents: branch-only commit, the MODIFIED tracked file, and the untracked file — + // none of which exist in the registered checkout — and not the ignored file. + var snapshot = ctx.Worktree.Path; + await Assert.That(File.ReadAllText(Path.Combine(snapshot, "branch-only.txt"))) + .IsEqualTo("branch-only-committed"); + await Assert.That(File.ReadAllText(Path.Combine(snapshot, "README.md"))) + .IsEqualTo("requester-modified"); + await Assert.That(File.ReadAllText(Path.Combine(snapshot, "untracked.txt"))) + .IsEqualTo("requester-untracked"); + await Assert.That(File.Exists(Path.Combine(snapshot, "ignored.txt"))).IsFalse(); + await Assert.That(File.Exists(Path.Combine(snapshot, "daemon-only.txt"))).IsFalse(); + + // (3) The snapshot root is outside the source repo (not a linked worktree under it). + await Assert.That(snapshot).IsNotEqualTo(canonicalRequester); + await Assert.That(snapshot.StartsWith( + canonicalRequester.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar, + StringComparison.Ordinal)).IsFalse(); + + // Baseline check 1 of 4 — after snapshot creation. + await AssertBaselineUnchanged(canonicalRequester, baseline, gitState, "after snapshot creation"); + + // (4) Reviewer mutation stays inside the snapshot. + File.WriteAllText(Path.Combine(snapshot, "README.md"), "reviewer-clobbered"); + File.WriteAllText(Path.Combine(snapshot, "reviewer-created.txt"), "reviewer dropping"); + File.WriteAllText(Path.Combine(snapshot, ".git", "reviewer-metadata"), "reviewer dropping"); + // The droppings must genuinely exist first, or the "they disappear" assertions below + // would be vacuously true. + await Assert.That(File.Exists(Path.Combine(snapshot, "reviewer-created.txt"))).IsTrue(); + await Assert.That(File.Exists(Path.Combine(snapshot, ".git", "reviewer-metadata"))).IsTrue(); + + // Baseline check 2 of 4 — after reviewer mutation. + await AssertBaselineUnchanged(canonicalRequester, baseline, gitState, "after reviewer mutation"); + + // (5) Per-round refresh: the requester's new content appears, and every reviewer-only + // file AND its git metadata disappear. Asserting only "the reviewer's writes never + // reached the requester" would pass against a refresh that silently accumulates + // reviewer droppings round after round. + File.WriteAllText(Path.Combine(requesterRepo, "README.md"), "requester-modified-again"); + var refreshedBaseline = ContentBaseline(canonicalRequester); + var refreshedGitState = GitState(canonicalRequester); + // The deliberate edit is the ONLY difference — so re-basing the baseline here cannot + // launder a corruption that happened in between. + await Assert.That(BaselineDifferences(baseline, refreshedBaseline)).IsEquivalentTo(["README.md"]); + + await orch.HandleSendInputForTest(new SendInputCommand(cmd.AgentId, "next", null)); + + await Assert.That(File.ReadAllText(Path.Combine(snapshot, "README.md"))) + .IsEqualTo("requester-modified-again"); + await Assert.That(File.Exists(Path.Combine(snapshot, "reviewer-created.txt"))).IsFalse(); + await Assert.That(File.Exists(Path.Combine(snapshot, ".git", "reviewer-metadata"))).IsFalse(); + await Assert.That(File.ReadAllText(Path.Combine(snapshot, "branch-only.txt"))) + .IsEqualTo("branch-only-committed"); + await Assert.That(File.Exists(Path.Combine(snapshot, "ignored.txt"))).IsFalse(); + + // Baseline check 3 of 4 — after refresh. + await AssertBaselineUnchanged(canonicalRequester, refreshedBaseline, refreshedGitState, "after refresh"); + + // (6) Stop removes the daemon-owned snapshot and leaves the requester intact. + await orch.HandleStopAgentForTest(cmd.AgentId); + for (var i = 0; i < 100 && Directory.Exists(snapshot); i++) await Task.Delay(20); + await Assert.That(Directory.Exists(snapshot)).IsFalse(); + await Assert.That(Directory.Exists(canonicalRequester)).IsTrue(); + + // Baseline check 4 of 4 — after stop. + await AssertBaselineUnchanged(canonicalRequester, refreshedBaseline, refreshedGitState, "after stop"); + } finally { + cleanupRequester(); + cleanupDaemon(); + } + } + // ── A5: borrowed launch runs in the user's cwd and creates no daemon worktree ───────── [Test] @@ -309,6 +454,70 @@ await Assert.That(throwingFactory.LastWorktreePath!) // ── Helpers / test doubles ───────────────────────────────────────────────────────────── + /// + /// A CONTENT baseline: relative path → SHA-256 of the file's bytes (plus its unix mode, where the + /// platform has one, since the snapshot builder preserves modes). Deliberately not + /// , which records entry NAMES only and would miss in-place corruption + /// of a file it still sees listed. + /// + /// .git is excluded on purpose. Building a snapshot runs read-only git plumbing + /// (bundle create, ls-files) inside the source checkout, which may legitimately + /// refresh index stat metadata — hashing that would make the baseline flap for a reason unrelated + /// to the invariant. Git-state integrity is asserted separately and more meaningfully by + /// (HEAD sha + porcelain status). + /// + static SortedDictionary ContentBaseline(string root) { + var baseline = new SortedDictionary(StringComparer.Ordinal); + foreach (var path in Directory.EnumerateFiles(root, "*", SearchOption.AllDirectories)) { + var rel = Path.GetRelativePath(root, path).Replace(Path.DirectorySeparatorChar, '/'); + if (rel == ".git" || rel.StartsWith(".git/", StringComparison.Ordinal)) continue; + + var hash = Convert.ToHexString(System.Security.Cryptography.SHA256.HashData(File.ReadAllBytes(path))); + var mode = OperatingSystem.IsWindows() ? "" : $" mode={File.GetUnixFileMode(path)}"; + baseline[rel] = hash + mode; + } + + return baseline; + } + + /// Relative paths whose content/mode differs between two baselines, including paths + /// present in only one of them. + static List BaselineDifferences( + SortedDictionary before, SortedDictionary after) => + before.Keys.Union(after.Keys, StringComparer.Ordinal) + .Where(k => !before.TryGetValue(k, out var b) || + !after.TryGetValue(k, out var a) || + !string.Equals(b, a, StringComparison.Ordinal)) + .OrderBy(k => k, StringComparer.Ordinal) + .ToList(); + + /// HEAD sha + porcelain status — the requester's git state, asserted alongside the + /// content baseline (which skips .git) so a reviewer write into the source repository's + /// git directory could not pass unnoticed. + static string GitState(string repo) => + GitCapture(repo, "rev-parse", "HEAD") + "\n" + GitCapture(repo, "status", "--porcelain"); + + static string GitCapture(string cwd, params string[] args) { + var psi = new ProcessStartInfo("git", args) { + WorkingDirectory = cwd, + RedirectStandardOutput = true, + RedirectStandardError = true + }; + using var proc = Process.Start(psi)!; + var stdout = proc.StandardOutput.ReadToEnd(); + proc.WaitForExit(); + + return stdout; + } + + static async Task AssertBaselineUnchanged( + string root, SortedDictionary expected, string expectedGitState, string when) { + await Assert.That(BaselineDifferences(expected, ContentBaseline(root))) + .IsEmpty().Because($"the requester checkout must be byte-identical {when}"); + await Assert.That(GitState(root)) + .IsEqualTo(expectedGitState).Because($"the requester's git state must be unchanged {when}"); + } + /// Sorted list of every file-system entry (relative path) under , /// so a before/after comparison catches ANY addition or removal in the user's tree. static List SnapshotTree(string root) =>