diff --git a/src/Capacitor.Cli.Core/Config/ConfigMutator.cs b/src/Capacitor.Cli.Core/Config/ConfigMutator.cs index 8b22c5c59..974fa9f26 100644 --- a/src/Capacitor.Cli.Core/Config/ConfigMutator.cs +++ b/src/Capacitor.Cli.Core/Config/ConfigMutator.cs @@ -44,24 +44,29 @@ public static ProfileConfig LoadPure(string path) { /// silently treat it the same as absence (e.g. a gated identity check) use this instead of /// . public static bool TryLoadPure(string path, out ProfileConfig config) { - // A directory sitting at the config path is NOT absence — File.Exists alone would say - // false and this would silently fall through to "nothing configured yet" for what is - // actually an unreadable/misconfigured location. - if (Directory.Exists(path)) { - config = new() { Profiles = new() { ["default"] = new() } }; - return false; - } - - if (!File.Exists(path)) { - config = new() { Profiles = new() { ["default"] = new() } }; - return true; - } - + // Attempt the read directly rather than probing Directory.Exists/File.Exists first: BOTH + // return false on a permission-denied path (or an unreadable ancestor directory), which + // would silently degrade "inaccessible" into "absent, defaults are fine" — exactly wrong + // for a caller (the start gate) that must fail closed on unreadable evidence rather than + // proceed as though nothing were configured. string json; try { json = File.ReadAllText(path); - } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - config = new() { Profiles = new() { ["default"] = new() } }; + } catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) { + // A link at the exact path (dangling included), or a file/link ancestor, is + // structural evidence — unreadable, not absence. + if (PathEvidence.PathBlockedByFileOrLink(path)) { + config = FreshDefault(); + return false; + } + + config = FreshDefault(); + return true; + } catch { + // A directory sitting at the config path (UnauthorizedAccessException on read), a + // permission error, or any other I/O failure — unreadable EVIDENCE, never silently + // folded into absence. + config = FreshDefault(); return false; } @@ -71,9 +76,9 @@ public static bool TryLoadPure(string path, out ProfileConfig config) { // contract. try { using var doc = JsonDocument.Parse(json); - if (doc.RootElement.ValueKind != JsonValueKind.Object) throw new JsonException("config root is not a JSON object"); + if (!doc.RootElement.IsObject) throw new JsonException("config root is not a JSON object"); } catch (JsonException) { - config = new() { Profiles = new() { ["default"] = new() } }; + config = FreshDefault(); return false; } @@ -81,15 +86,29 @@ public static bool TryLoadPure(string path, out ProfileConfig config) { config = ConfigMigration.MigrateIfNeeded(json).Config; return true; } catch (JsonException) { - config = new() { Profiles = new() { ["default"] = new() } }; + config = FreshDefault(); return false; } } + static ProfileConfig FreshDefault() => new() { Profiles = new() { ["default"] = new() } }; + static void Publish(string path, ProfileConfig config) { var tmp = path + ".tmp-" + Guid.NewGuid().ToString("N")[..8]; File.WriteAllBytes(tmp, JsonSerializer.SerializeToUtf8Bytes(config, ProfileConfigJsonContextIndented.Default.ProfileConfig)); - File.Move(tmp, path, overwrite: true); + // Windows denies replace-into-place while any reader holds the destination without + // FILE_SHARE_DELETE; readers are short-lived, so retry briefly before surfacing. + for (var attempt = 0; ; attempt++) { + try { + File.Move(tmp, path, overwrite: true); + return; + } catch (Exception e) when (e is UnauthorizedAccessException or IOException && attempt < 49) { + Thread.Sleep(20); + } catch { + try { File.Delete(tmp); } catch { /* best effort */ } + throw; + } + } } } diff --git a/src/Capacitor.Cli.Core/PathEvidence.cs b/src/Capacitor.Cli.Core/PathEvidence.cs new file mode 100644 index 000000000..a00490fdf --- /dev/null +++ b/src/Capacitor.Cli.Core/PathEvidence.cs @@ -0,0 +1,24 @@ +namespace Capacitor.Cli.Core; + +/// Structural path-evidence probe shared by not-found-classification callers. +public static class PathEvidence { + /// True when a not-found read failure at is explained by a link at the + /// exact path, or a file/link ancestor, rather than a chain of directories never created. + public static bool PathBlockedByFileOrLink(string path) { + if (IsLink(path)) return true; + + var current = Path.GetDirectoryName(path); + while (!string.IsNullOrEmpty(current)) { + if (File.Exists(current)) return true; + if (Directory.Exists(current)) return false; + if (IsLink(current)) return true; + current = Path.GetDirectoryName(current); + } + return false; + } + + static bool IsLink(string path) { + try { return File.ResolveLinkTarget(path, returnFinalTarget: false) is not null; } + catch { return false; } + } +} diff --git a/src/Capacitor.Cli.Core/Setup/AgentDetection.cs b/src/Capacitor.Cli.Core/Setup/AgentDetection.cs index e66038118..94ff0767b 100644 --- a/src/Capacitor.Cli.Core/Setup/AgentDetection.cs +++ b/src/Capacitor.Cli.Core/Setup/AgentDetection.cs @@ -111,14 +111,22 @@ public static AgentDetectionResult Detect(AgentDetectionInputs i) { /// null/empty PATH. On Unix, requires at least one of the user/group/other execute bits; on /// Windows, walks PATHEXT (defaulting to .EXE/.CMD/.BAT) and accepts any file that exists. /// - public static bool BinaryOnPath(string binaryName, AgentDetectionInputs i) { + public static bool BinaryOnPath(string binaryName, AgentDetectionInputs i) => + BinaryOnPath(binaryName, i, IsExecutable); + + /// Stubbing seam for ; separator/ + /// extensions/comparer are all derived from 's platform, never the host's. + internal static bool BinaryOnPath(string binaryName, AgentDetectionInputs i, Func isExecutable) { if (string.IsNullOrEmpty(i.PathEnv)) return false; - var paths = i.PathEnv.Split(Path.PathSeparator); + var separator = i.IsWindows ? ';' : ':'; + var paths = i.PathEnv.Split(separator); var extensions = i.IsWindows ? WindowsExtensions(i.PathExt) : [""]; + var comparer = i.IsWindows ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal; return paths.Where(dir => !string.IsNullOrEmpty(dir)) - .Any(dir => extensions.Select(ext => Path.Combine(dir, binaryName + ext)).Any(path => IsExecutable(path, i.IsWindows))); + .Distinct(comparer) + .Any(dir => extensions.Select(ext => Path.Combine(dir, binaryName + ext)).Any(path => isExecutable(path, i.IsWindows))); } /// Convenience overload for CLI call sites that only need a single-binary current- diff --git a/src/Capacitor.Cli/Services/IServiceManager.cs b/src/Capacitor.Cli/Services/IServiceManager.cs index 4cd0f5aa2..4f0448c2b 100644 --- a/src/Capacitor.Cli/Services/IServiceManager.cs +++ b/src/Capacitor.Cli/Services/IServiceManager.cs @@ -73,11 +73,9 @@ interface IVerifyServiceManager { void WriteAndBootstrap(ServiceSpec spec, TimeSpan timeout); bool Uninstall(string serviceId, TimeSpan timeout, out string? error); bool Start(string serviceId, TimeSpan timeout, out string? error); - /// Bootstrap-only start for the gated start path: activate the ALREADY-WRITTEN unit - /// without the generic 's Loaded→kickstart branch. If the label turns out - /// Loaded (a foreign writer raced the gate's own confirmed-absent check), this must fail rather - /// than kickstart it — kickstarting an unverified loaded definition is exactly what the gate - /// exists to prevent. + /// Bootstrap-only start for the gated path: activates the already-written unit, but + /// fails — never kickstarts — when the label reads Loaded, closing the race with the gate's own + /// confirmed-absent check. bool StartBootstrapOnly(string serviceId, TimeSpan timeout, out string? error); bool Stop(string serviceId, TimeSpan timeout, out string? error); } diff --git a/src/Capacitor.Cli/Services/LaunchdServiceManager.cs b/src/Capacitor.Cli/Services/LaunchdServiceManager.cs index 371c8e036..2f2337413 100644 --- a/src/Capacitor.Cli/Services/LaunchdServiceManager.cs +++ b/src/Capacitor.Cli/Services/LaunchdServiceManager.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Runtime.InteropServices; namespace Capacitor.Cli.Services; @@ -48,7 +49,7 @@ public IReadOnlyList ListInstalled() { public ServiceStatus Status(string serviceId) { var path = LaunchdUnit.PlistPath(serviceId); if (!File.Exists(path)) return new ServiceStatus(ServiceState.NotInstalled, null); - var bin = LaunchdUnit.BinaryFromPlist(File.ReadAllText(path)); // ProgramArguments[0], not the Label + var bin = ReadBinaryPathSafe(path); // ProgramArguments[0], not the Label var (code, stdout, _) = _runProcess("launchctl", LaunchdUnit.PrintArgs(Uid(), serviceId)); return new ServiceStatus(LaunchdUnit.StatusFromPrint(code, stdout), bin); } @@ -57,9 +58,11 @@ public ServiceStatus Status(string serviceId) { public ServiceQuery Query(string serviceId, TimeSpan t) => QueryCore(serviceId, t); ServiceQuery QueryCore(string serviceId, TimeSpan? timeout) { - var path = LaunchdUnit.PlistPath(serviceId); - var unitPresent = File.Exists(path); - var bin = unitPresent ? LaunchdUnit.BinaryFromPlist(File.ReadAllText(path)) : null; + var path = LaunchdUnit.PlistPath(serviceId); + // File.Exists alone reads a DIRECTORY at the path (or an inaccessible ancestor) as + // absent — open directly so presence and unreadable-but-present are never conflated. + var unitPresent = LaunchdUnit.TryReadPlist(path, out _) != LaunchdUnit.PlistRead.Absent; + var bin = unitPresent ? ReadBinaryPathSafe(path) : null; var (code, stdout, stderr, timedOut) = RunCtl(timeout, LaunchdUnit.PrintArgs(Uid(), serviceId)); // A killed-on-timeout print told us nothing about the label — never let its kill exit code // masquerade as a real classification; report Unknown so nothing destructive follows. @@ -68,6 +71,17 @@ ServiceQuery QueryCore(string serviceId, TimeSpan? timeout) { return new ServiceQuery(probe, unitPresent, state, bin, probe == LabelProbe.Loaded ? LaunchdUnit.PidFromPrint(stdout) : null); } + /// Total plist-evidence read: File.ReadAllText + + /// together, contained so that ANY failure — an I/O error, malformed XML, a duplicate + /// ProgramArguments key — yields null rather than escaping. + /// and are total, never-throwing probes; the gate's own contained Phase-A + /// parse (ServiceVerify's _readPlist path) remains the sole authority for coded + /// evidence classification (malformed → evidence_unreadable). + static string? ReadBinaryPathSafe(string path) { + try { return LaunchdUnit.BinaryFromPlist(File.ReadAllText(path)); } + catch { return null; } + } + public void Install(ServiceSpec spec, bool startNow) { var plistPath = LaunchdUnit.PlistPath(spec.ServiceId); // idempotent: bootout an existing job (ignore failure), then rewrite + bootstrap. @@ -163,14 +177,13 @@ bool StartCore(string serviceId, TimeSpan? timeout, out string? error) { return true; } - /// - /// The gated start path's bootstrap-only verb (verify-only, no legacy no-timeout overload): - /// re-probes the label immediately before acting, and ONLY bootstraps when it reads Absent — - /// unlike , a Loaded probe here is a FAILURE, never a kickstart. This is - /// the re-check that closes the race between the gate's own confirmed-absent wait and this call: - /// a foreign writer that loaded the label in that window must not get silently kickstarted. - /// + /// launchd implementation of : + /// re-probes the label immediately before acting. Both launchctl calls share ONE deadline — + /// the probe gets the full , and the bootstrap gets only what's left + /// of it — rather than each getting the full budget (which would let the pair invade up to 2x + /// the caller's forward remainder, including its separately reserved rollback budget). public bool StartBootstrapOnly(string serviceId, TimeSpan timeout, out string? error) { + var sw = Stopwatch.StartNew(); var (probeExit, probeOut, probeErr, probeTimedOut) = RunCtl(timeout, LaunchdUnit.PrintArgs(Uid(), serviceId)); var probe = probeTimedOut ? LabelProbe.Unknown : LaunchdUnit.ClassifyPrint(probeExit, probeOut, probeErr); @@ -179,7 +192,13 @@ public bool StartBootstrapOnly(string serviceId, TimeSpan timeout, out string? e return false; } - var (bootstrapExit, _, bootstrapErr, bootstrapTimedOut) = RunCtl(timeout, LaunchdUnit.BootstrapArgs(Uid(), LaunchdUnit.PlistPath(serviceId))); + var remaining = timeout - sw.Elapsed; + if (remaining <= TimeSpan.Zero) { + error = "launchctl bootstrap timed out and was terminated"; + return false; + } + + var (bootstrapExit, _, bootstrapErr, bootstrapTimedOut) = RunCtl(remaining, LaunchdUnit.BootstrapArgs(Uid(), LaunchdUnit.PlistPath(serviceId))); if (bootstrapTimedOut) { error = "launchctl bootstrap timed out and was terminated"; return false; } if (bootstrapExit != 0) { error = $"launchctl bootstrap failed (exit {bootstrapExit}): {bootstrapErr.Trim()}"; diff --git a/src/Capacitor.Cli/Services/LaunchdUnit.cs b/src/Capacitor.Cli/Services/LaunchdUnit.cs index 7cca40f68..c28d1f73c 100644 --- a/src/Capacitor.Cli/Services/LaunchdUnit.cs +++ b/src/Capacitor.Cli/Services/LaunchdUnit.cs @@ -87,80 +87,97 @@ public static IReadOnlyList ProgramArguments(ServiceSpec spec) => : null; } - /// - /// The daemon binary baked into a plist — the first <string> of the - /// <array> PAIRED WITH the top-level <key>ProgramArguments</key>, - /// never just "the document's first <array>" — a decoy array planted anywhere - /// earlier in the document must not be read as the binary launchd will actually execute. Used - /// by daemon doctor to detect a moved binary. A DUPLICATE top-level - /// ProgramArguments key throws rather than silently - /// picking one — this file is never hand-edited, so two occurrences means a foreign/corrupt - /// writer, and callers that gate on this value must see that as unreadable evidence, not a guess. - /// - public static string? BinaryFromPlist(string plistXml) { - var topDict = XDocument.Parse(plistXml).Root?.Element("dict"); - if (topDict is null) return null; + /// Distinguishes genuine absence from present-but-unreadable evidence. + internal enum PlistRead { Absent, Ok, Unreadable } + + /// Discriminated read: a not-found blocked by structural evidence (a link at the exact + /// path, or a file/link ancestor) is , never . + internal static PlistRead TryReadPlist(string path, out string? content) { + try { + content = File.ReadAllText(path); + return PlistRead.Ok; + } catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) { + content = null; + return PathEvidence.PathBlockedByFileOrLink(path) ? PlistRead.Unreadable : PlistRead.Absent; + } catch { + content = null; + return PlistRead.Unreadable; + } + } - string? result = null; - var found = false; + /// Strict key/value walk: throws on any malformed key/value alternation. + static IEnumerable<(string Key, XElement Value)> KeyedElements(XElement dict, string context) { string? pendingKey = null; - - foreach (var el in topDict.Elements()) { - if (el.Name == "key") { pendingKey = el.Value; continue; } - - if (pendingKey == "ProgramArguments") { - if (found) throw new InvalidDataException("duplicate ProgramArguments key in plist"); - found = true; - result = el.Name == "array" ? el.Elements("string").FirstOrDefault()?.Value : null; + foreach (var el in dict.Elements()) { + if (el.Name == "key") { + if (pendingKey is not null) + throw new InvalidDataException($"{context}consecutive nodes ('{pendingKey}', '{el.Value}') in plist"); + pendingKey = el.Value; + continue; } + if (pendingKey is null) + throw new InvalidDataException($"{context}value with no preceding key in plist"); + + yield return (pendingKey, el); pendingKey = null; } + if (pendingKey is not null) + throw new InvalidDataException($"{context}dangling {pendingKey} with no value in plist"); + } + + /// The element paired with the top-level <key> named + /// , or null when that key never appears; a duplicate throws. + static XElement? TopLevelValue(XElement dict, string key) { + XElement? result = null; + var found = false; + foreach (var (k, value) in KeyedElements(dict, "")) { + if (k != key) continue; + if (found) throw new InvalidDataException($"duplicate {key} key in plist"); + found = true; + result = value; + } return result; } /// - /// The environment baked into a plist — the dict PAIRED WITH the top-level - /// <key>EnvironmentVariables</key>, walking the top-level dict's own key/value - /// pairs rather than searching the whole document. Returns empty rather than throwing when the - /// block is absent or empty; used by daemon service status --json to surface the baked - /// profile/server/consent evidence as UX-only fields. A DUPLICATE top-level - /// EnvironmentVariables key, or a duplicate KEY within the one block, both throw - /// rather than silently last-win — this file is never - /// hand-edited, so any of those means a foreign/corrupt writer, and the gate callers that read - /// identity evidence out of this map must see that as unreadable, not silently pick whichever - /// value happened to land last. + /// The daemon binary baked into a plist — the first <string> of the + /// <array> paired with the top-level <key>ProgramArguments</key> + /// (see ). Used by daemon doctor to detect a moved binary. + /// + public static string? BinaryFromPlist(string plistXml) { + var topDict = XDocument.Parse(plistXml).Root?.Element("dict"); + if (topDict is null) return null; + + var programArguments = TopLevelValue(topDict, "ProgramArguments"); + return programArguments?.Name == "array" ? programArguments.Elements("string").FirstOrDefault()?.Value : null; + } + + /// + /// The environment baked into a plist — the dict paired with the top-level + /// EnvironmentVariables key, walked strictly (see ). Empty + /// only when the block is genuinely absent; any other malformed shape throws + /// rather than degrading silently — this file is never + /// hand-edited, so a gate caller reading identity evidence here must see ambiguous evidence + /// as unreadable, never guessed. /// public static IReadOnlyDictionary EnvFromPlist(string plistXml) { var result = new Dictionary(StringComparer.Ordinal); var topDict = XDocument.Parse(plistXml).Root?.Element("dict"); if (topDict is null) return result; - var found = false; - string? pendingKey = null; + var envDict = TopLevelValue(topDict, "EnvironmentVariables"); + if (envDict is null) return result; // genuinely absent — a legitimate "no baked env" shape - foreach (var el in topDict.Elements()) { - if (el.Name == "key") { pendingKey = el.Value; continue; } - - if (pendingKey == "EnvironmentVariables") { - if (found) throw new InvalidDataException("duplicate EnvironmentVariables key in plist"); - found = true; - - if (el.Name == "dict") { - string? key = null; - foreach (var kv in el.Elements()) { - if (kv.Name == "key") key = kv.Value; - else if (kv.Name == "string" && key is not null) { - if (!result.TryAdd(key, kv.Value)) - throw new InvalidDataException($"duplicate EnvironmentVariables key '{key}' in plist"); - key = null; - } - } - } - } + if (envDict.Name != "dict") + throw new InvalidDataException("EnvironmentVariables is not a dict in plist"); - pendingKey = null; + foreach (var (key, value) in KeyedElements(envDict, "EnvironmentVariables has ")) { + if (value.Name != "string") + throw new InvalidDataException($"EnvironmentVariables key '{key}' is paired with a non-string value in plist"); + if (!result.TryAdd(key, value.Value)) + throw new InvalidDataException($"duplicate EnvironmentVariables key '{key}' in plist"); } return result; diff --git a/src/Capacitor.Cli/Services/ServiceVerify.cs b/src/Capacitor.Cli/Services/ServiceVerify.cs index aa7faa0b6..00979ee67 100644 --- a/src/Capacitor.Cli/Services/ServiceVerify.cs +++ b/src/Capacitor.Cli/Services/ServiceVerify.cs @@ -131,15 +131,25 @@ static TimeSpan ConfirmReserveFor(TimeSpan forward) { /// tests; the command wires the real resolution. readonly Func _profileViable = profileViable ?? (() => true); - /// Install-only seam for the on-disk plist read (final recheck + rollback's foreign-file - /// guard). Real launchd needs HOME to compute the path, so tests inject this directly. + /// Install-only seam for the on-disk plist read (Phase B's pre/post-readiness recheck + /// and install's final on-disk fingerprint recheck). Real launchd needs HOME to compute the + /// path, so tests inject this directly. readonly Func _readPlist = readPlist ?? (path => { try { return File.Exists(path) ? File.ReadAllText(path) : null; } catch { return null; } }); - /// Entry-recovery-only seam: distinguishes "absent" from "present but unreadable" when - /// returns null for both. - readonly Func _plistExists = plistExists ?? File.Exists; + /// Discriminated read shared by Phase A, marker recovery, and install rollback: unlike + /// composed with a separate exists check, a structural obstruction (a + /// directory, a dangling symlink) can never be misread as confirmed absence. + readonly Func _discriminatedPlistRead = + readPlist is null && plistExists is null + ? (path => { var status = LaunchdUnit.TryReadPlist(path, out var content); return (status, content); }) + : (path => { + var content = readPlist?.Invoke(path); + if (content is not null) return (LaunchdUnit.PlistRead.Ok, content); + var exists = plistExists?.Invoke(path) ?? false; + return (exists ? LaunchdUnit.PlistRead.Unreadable : LaunchdUnit.PlistRead.Absent, null); + }); /// The gated start seam: when non-null AND gateEnv(ConsentSeedVar) is /// PRESENT — any value, including empty, under the exact-value contract — @@ -207,21 +217,23 @@ public async Task StartVerifiedAsync(string serviceId) { if (gated) { var plistPath = LaunchdUnit.PlistPath(serviceId); - phaseAPlistContent = _readPlist(plistPath); + var (readStatus, content) = _discriminatedPlistRead(plistPath); + phaseAPlistContent = content; - // A malformed/truncated plist (exactly the foreign-writer race Phase B defends - // against, caught here instead) must not let XDocument.Parse's XmlException escape - // this method — that would abort to a generic, uncoded exit 1 instead of the gate's - // own contract. Unreadable evidence is EvidenceUnreadable, same as every other - // evidence read this gate performs. StartGateReason? reason; - try { - var unitEnv = phaseAPlistContent is not null ? LaunchdUnit.EnvFromPlist(phaseAPlistContent) : new Dictionary(); - var unitBinaryPath = phaseAPlistContent is not null ? LaunchdUnit.BinaryFromPlist(phaseAPlistContent) : null; - reason = EvaluateStartGate(unitEnv, unitBinaryPath, UnitIdentity.ResolveDaemonBinary(), _gateEnv!, _digestMatches); - unitEnv.TryGetValue(ExpectVar, out unitExpectation); - } catch { + // A fresh query that saw the unit but a read that didn't is contradictory evidence, not + // genuine absence — only agreement on absence may pass through to directive_missing. + if (readStatus == LaunchdUnit.PlistRead.Unreadable || (readStatus == LaunchdUnit.PlistRead.Absent && pre.UnitPresent)) { reason = StartGateReason.EvidenceUnreadable; + } else { + try { + var unitEnv = content is not null ? LaunchdUnit.EnvFromPlist(content) : new Dictionary(); + var unitBinaryPath = content is not null ? LaunchdUnit.BinaryFromPlist(content) : null; + reason = EvaluateStartGate(unitEnv, unitBinaryPath, UnitIdentity.ResolveDaemonBinary(), _gateEnv!, _digestMatches); + unitEnv.TryGetValue(ExpectVar, out unitExpectation); + } catch { + reason = StartGateReason.EvidenceUnreadable; + } } if (reason is { } r) { @@ -267,24 +279,13 @@ public async Task StartVerifiedAsync(string serviceId) { } } - var plistPath = LaunchdUnit.PlistPath(serviceId); - var recheckContent = _readPlist(plistPath); - // Same XmlException hazard as Phase A's parse, but here escaping is worse: it would // abort AFTER the marker write and boot-out, past the point Rollback runs, leaving the // service stopped with a stuck marker instead of the guaranteed unloaded-plist-retained // outcome. A recheck that can't be parsed/validated is exactly what drift means — the // content changed (to something unreadable) or can no longer be confirmed unchanged — // so route it into the same drift branch rather than letting it throw. - bool digestStillGood; - try { - var recheckBinary = recheckContent is not null ? LaunchdUnit.BinaryFromPlist(recheckContent) : null; - digestStillGood = recheckBinary is not null && _digestMatches(recheckBinary); - } catch { - digestStillGood = false; - } - - if (recheckContent != phaseAPlistContent || !digestStillGood) { + if (!RecheckPlistUnchanged(serviceId, phaseAPlistContent)) { ServiceTxnMarker.Write(serviceId, new TxnMarker(1, "start", "gate-drift", DescribeQuery(pre), "unloaded-plist-retained", null)); return await Rollback(serviceId, VerifyExit.StartGateDrift, VerifyExit.StartGateDriftToken); @@ -302,14 +303,9 @@ public async Task StartVerifiedAsync(string serviceId) { if (!attributionEnabled) Say("boot-refusal marker could not be cleared; coded attribution disabled"); } - // The gated path uses the bootstrap-only seam instead of the generic Start: StartCore's own - // Loaded-probe branch would silently KICKSTART a stale definition if a foreign writer - // re-loaded the label in the window between the confirmed-absent wait above and this call — - // exactly the race this gate exists to close. A bootstrap-only failure (including "the label - // turned out Loaded") is therefore fatal here, not a soft warning: the label state is no - // longer what Phase B proved, so roll back via the coded bootout-unknown exit instead of - // bootstrapping onto unverified ground. The ungated path keeps the soft-warning Start(), - // whose readiness poll — not the call's own return value — is the source of truth. + // Bootstrap-only (see IVerifyServiceManager.StartBootstrapOnly): any failure — including a + // Loaded probe — rolls back via BootoutUnknown rather than falling through to the ungated + // Start() below, whose own readiness poll is normally the source of truth. if (gated) { if (!manager.StartBootstrapOnly(serviceId, Remaining(deadline), out var bootstrapOnlyError)) { Say($"start: {bootstrapOnlyError}"); @@ -353,6 +349,18 @@ public async Task StartVerifiedAsync(string serviceId) { var (confirmed, confirmedPid) = await IsReadyAsync(serviceId, deadline, requirePid: pid); if (confirmedPid is not null) observedJobPids.Add(confirmedPid.Value); if (confirmed) { + // Post-readiness recheck (spec §3, mirrors install/replace's own final recheck): + // Phase B's pre-bootstrap recheck only bounds the recheck→exec race up to the + // moment bootstrap started — a foreign writer swapping the plist or the binary + // bytes AFTER that, but before this commit, must not ride readiness into a + // silent commit. Re-read the plist (contained) and compare against Phase A's own + // captured content, and re-check the digest one more time. + if (gated && !RecheckPlistUnchanged(serviceId, phaseAPlistContent)) { + ServiceTxnMarker.Write(serviceId, + new TxnMarker(1, "start", "gate-post-readiness-drift", DescribeQuery(pre), "unloaded-plist-retained", null)); + return await Rollback(serviceId, VerifyExit.StartGateDrift, VerifyExit.StartGateDriftToken); + } + ServiceTxnMarker.Write(serviceId, new TxnMarker(1, "start", "committed", DescribeQuery(pre), "unloaded-plist-retained", null)); onCommitted?.Invoke(); @@ -371,6 +379,23 @@ public async Task StartVerifiedAsync(string serviceId) { return AttributeReadinessTimeout(serviceId, rollbackExit, gated, attributionEnabled, unitExpectation, observedJobPids); } + /// Re-reads the plist and re-checks the digest (both contained — never an escaping + /// exception), comparing against Phase A's captured ; + /// false means drift. Shared by Phase B's pre-bootstrap and post-readiness rechecks — callers + /// own their own marker phase and rollback call. + bool RecheckPlistUnchanged(string serviceId, string? phaseAPlistContent) { + var content = _readPlist(LaunchdUnit.PlistPath(serviceId)); + bool digestGood; + try { + var binary = content is not null ? LaunchdUnit.BinaryFromPlist(content) : null; + digestGood = binary is not null && _digestMatches(binary); + } catch { + digestGood = false; + } + + return content == phaseAPlistContent && digestGood; + } + /// /// Shared readiness-timeout attribution tail for both and /// : attribute ONLY on the genuine readiness-timeout exit, @@ -837,19 +862,18 @@ public async Task InstallVerifiedAsync(ServiceSpec spec, bool replace, stri return null; } - var onDisk = _readPlist(onDiskPath); - if (onDisk is null) { - if (_plistExists(onDiskPath)) { - // Present but unreadable is NOT absent — the file exists and was never fingerprint- - // compared, so never guess it's our own gone residue. - Say(VerifyExit.RestoreVerificationToken); - return VerifyExit.RestoreVerification; - } + var (status, onDisk) = _discriminatedPlistRead(onDiskPath); + if (status == LaunchdUnit.PlistRead.Unreadable) { + // Present but unreadable is NOT absent — never guess it's our own gone residue. + Say(VerifyExit.RestoreVerificationToken); + return VerifyExit.RestoreVerification; + } + if (status == LaunchdUnit.PlistRead.Absent) { ServiceTxnMarker.Delete(serviceId); // confirmed absent — residue already gone return null; } - if (ServiceTxnMarker.Fingerprint(onDisk) != leftover.PlistFingerprint) { + if (ServiceTxnMarker.Fingerprint(onDisk!) != leftover.PlistFingerprint) { // A foreign writer touched the file after the death — surface, never pave. Say(VerifyExit.RestoreVerificationToken); return VerifyExit.RestoreVerification; @@ -1014,18 +1038,14 @@ async Task WaitForStopConfirmedAsync(string serviceId, DateTimeOffset dead /// is label-absent AND file-gone (unlike start, which retains the plist), bounded by the reserve. A /// foreign plist (fingerprint mismatch) is never touched — checked before the uninstall. async Task InstallRollback(string serviceId, string plistPath, string fingerprint, int reasonExit, string reasonToken) { - // Never uninstall what we can't verify is ours. A null read is either genuine absence OR a - // present-but-unreadable file (a lock-unaware writer replaced ours between bootstrap and - // rollback): only the confirmed-absent case (read null AND the file does not exist) may be - // cleared. Present-but-unreadable, and readable-but-foreign, both surface untouched — the - // same fail-closed rule RecoverLeftoverMarker applies at entry. - var onDisk = _readPlist(plistPath); - if (onDisk is null) { - if (_plistExists(plistPath)) { - Say(VerifyExit.RestoreVerificationToken); - return VerifyExit.RestoreVerification; - } - } else if (ServiceTxnMarker.Fingerprint(onDisk) != fingerprint) { + // Never uninstall what we can't verify is ours — unreadable-but-present and readable-but- + // foreign both surface untouched, the same fail-closed rule RecoverLeftoverMarker applies. + var (status, onDisk) = _discriminatedPlistRead(plistPath); + if (status == LaunchdUnit.PlistRead.Unreadable) { + Say(VerifyExit.RestoreVerificationToken); + return VerifyExit.RestoreVerification; + } + if (status == LaunchdUnit.PlistRead.Ok && ServiceTxnMarker.Fingerprint(onDisk!) != fingerprint) { Say(VerifyExit.RestoreVerificationToken); return VerifyExit.RestoreVerification; } diff --git a/test/Capacitor.Cli.Tests.Unit/Config/ConfigMutatorTests.cs b/test/Capacitor.Cli.Tests.Unit/Config/ConfigMutatorTests.cs index 4241a27ef..4cfcf73b2 100644 --- a/test/Capacitor.Cli.Tests.Unit/Config/ConfigMutatorTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Config/ConfigMutatorTests.cs @@ -64,6 +64,26 @@ public async Task Mutate_uses_unique_temp_names() { await Assert.That(File.Exists(ConfigPath + ".tmp")).IsFalse(); } + [Test] + public async Task Mutate_survives_a_transient_reader_holding_the_destination() { + await ConfigMutator.MutateAsync(c => c with { MachineId = "before" }); + + // Share-read only (no FILE_SHARE_DELETE): on Windows this blocks the replace-into-place + // until released, exercising Publish's retry; on Unix rename is unaffected. + var reader = new FileStream(ConfigPath, FileMode.Open, FileAccess.Read, FileShare.Read); + try { + var mutate = ConfigMutator.MutateAsync(c => c with { MachineId = "after" }); + await Task.Delay(100); + reader.Dispose(); + await mutate; + } finally { + reader.Dispose(); + } + + var final = await AppConfig.LoadProfileConfig(); + await Assert.That(final.MachineId).IsEqualTo("after"); + } + [Test] public async Task Legacy_v1_config_is_migrated_in_memory_and_persisted_through_the_mutation() { // minimal v1 flat config (no "version"/"profiles" — ConfigMigration's v1 shape) @@ -154,6 +174,84 @@ public async Task LoadPure_still_degrades_malformed_json_to_defaults() { await Assert.That(config.Profiles.ContainsKey("default")).IsTrue(); } + /// A non-not-found I/O failure, forced deterministically: the config path's PARENT + /// is a plain file rather than a directory, so opening through it fails structurally. + [Test] + public async Task TryLoadPure_parent_replaced_by_a_file_is_failure_not_absence() { + var root = Directory.CreateTempSubdirectory("kcap-trypure-").FullName; + var parentAsFile = Path.Combine(root, "not-a-directory"); + await File.WriteAllTextAsync(parentAsFile, "i am a file, not a directory"); + var path = Path.Combine(parentAsFile, "config.json"); + + var ok = ConfigMutator.TryLoadPure(path, out var config); + + await Assert.That(ok).IsFalse(); + await Assert.That(config.Profiles.ContainsKey("default")).IsTrue(); // still a usable default out param + } + + /// The immediate parent alone isn't enough: a path nested TWO levels under a + /// planted file needs the ancestor walk to climb past the never-created child directory. + [Test] + public async Task TryLoadPure_grandparent_replaced_by_a_file_is_failure_not_absence() { + var root = Directory.CreateTempSubdirectory("kcap-trypure-").FullName; + var grandparentAsFile = Path.Combine(root, "not-a-directory"); + await File.WriteAllTextAsync(grandparentAsFile, "i am a file, not a directory"); + var path = Path.Combine(grandparentAsFile, "child", "config.json"); + + var ok = ConfigMutator.TryLoadPure(path, out var config); + + await Assert.That(ok).IsFalse(); + await Assert.That(config.Profiles.ContainsKey("default")).IsTrue(); // still a usable default out param + } + + /// A dangling symlink segment (File.Exists/Directory.Exists both read + /// it as absent, since they follow to the missing target) must not let the ancestor walk skip + /// past it to a real directory further up. + [Test] + public async Task TryLoadPure_dangling_symlink_ancestor_is_failure_not_absence() { + Skip.When(OperatingSystem.IsWindows(), "symlink creation needs elevated privilege on Windows CI"); + + var root = Directory.CreateTempSubdirectory("kcap-trypure-").FullName; + var link = Path.Combine(root, "danglink"); + Directory.CreateSymbolicLink(link, Path.Combine(root, "never-created-target")); + var path = Path.Combine(link, "config.json"); + + var ok = ConfigMutator.TryLoadPure(path, out var config); + + await Assert.That(ok).IsFalse(); + await Assert.That(config.Profiles.ContainsKey("default")).IsTrue(); // still a usable default out param + } + + /// A dangling symlink AT the exact config path (not an ancestor) raises + /// , not — must still + /// classify as unreadable, never the takeover-safe "nothing configured yet". + [Test] + public async Task TryLoadPure_dangling_symlink_at_exact_path_is_failure_not_absence() { + Skip.When(OperatingSystem.IsWindows(), "symlink creation needs elevated privilege on Windows CI"); + + var root = Directory.CreateTempSubdirectory("kcap-trypure-").FullName; + var path = Path.Combine(root, "config.json"); + File.CreateSymbolicLink(path, Path.Combine(root, "never-created-target")); + + var ok = ConfigMutator.TryLoadPure(path, out var config); + + await Assert.That(ok).IsFalse(); + await Assert.That(config.Profiles.ContainsKey("default")).IsTrue(); // still a usable default out param + } + + /// A genuinely never-created parent chain must still read as absence — disambiguating + /// a blocked ancestor must not turn every missing directory level into a false failure. + [Test] + public async Task TryLoadPure_missing_parent_directory_is_still_absence() { + var root = Directory.CreateTempSubdirectory("kcap-trypure-").FullName; + var path = Path.Combine(root, "never-created", "config.json"); + + var ok = ConfigMutator.TryLoadPure(path, out var config); + + await Assert.That(ok).IsTrue(); + await Assert.That(config.Profiles.ContainsKey("default")).IsTrue(); + } + [Test] public async Task TryLoadPure_valid_file_is_success() { var path = Path.Combine(Directory.CreateTempSubdirectory("kcap-trypure-").FullName, "config.json"); diff --git a/test/Capacitor.Cli.Tests.Unit/Services/LaunchdStartStopTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/LaunchdStartStopTests.cs index 242998d7e..7f23e7011 100644 --- a/test/Capacitor.Cli.Tests.Unit/Services/LaunchdStartStopTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Services/LaunchdStartStopTests.cs @@ -212,6 +212,111 @@ await WithHome(async _ => { }); } + // ── Query containment: a malformed on-disk plist (duplicate key, truncated XML) must never + // escape Query as an uncoded failure — Query is a total, never-throwing probe. See + // ServiceVerifyStartGateProductionPathTests for the same shape carried through the gate. ── + + [Test] + public async Task Query_does_not_throw_on_a_duplicate_ProgramArguments_key_plist_and_reports_null_binary_path() { + Skip.When(OperatingSystem.IsWindows(), "Uid() P/Invokes libc's getuid, POSIX-only"); + + await WithHome(async path => { + const string duplicateKeyPlist = """ + + + + + Labelio.kurrent.kcap.daemon.test + ProgramArguments + /bin/kcap-daemon + + ProgramArguments + /bin/evil-daemon + + + + """; + File.WriteAllText(path, duplicateKeyPlist); + + var mgr = new LaunchdServiceManager(runProcess: (_, args) => + args[0] == "print" + ? (113, "", "Could not find service \"io.kurrent.kcap.daemon.test\" in domain for user gui: 501") + : (0, "", "")); + + var query = mgr.Query("test"); + + await Assert.That(query.BinaryPath).IsNull(); + await Assert.That(query.UnitPresent).IsTrue(); + await Assert.That(query.Probe).IsEqualTo(LabelProbe.Absent); + }); + } + + [Test] + public async Task Status_does_not_throw_on_a_malformed_plist_and_reports_null_binary_path() { + Skip.When(OperatingSystem.IsWindows(), "Uid() P/Invokes libc's getuid, POSIX-only"); + + await WithHome(async path => { + File.WriteAllText(path, "Truncated"); + + var mgr = new LaunchdServiceManager(runProcess: (_, args) => + args[0] == "print" + ? (113, "", "Could not find service \"io.kurrent.kcap.daemon.test\" in domain for user gui: 501") + : (0, "", "")); + + var status = mgr.Status("test"); + + await Assert.That(status.BinaryPath).IsNull(); + }); + } + + // ── StartBootstrapOnly budget discipline ── + + [Test] + public async Task StartBootstrapOnly_shares_one_deadline_across_probe_and_bootstrap_not_two_full_budgets() { + Skip.When(OperatingSystem.IsWindows(), "Uid() P/Invokes libc's getuid, POSIX-only"); + + await WithHome(async _ => { + var timeouts = new List(); + var mgr = new LaunchdServiceManager(runBounded: (_, args, timeout) => { + timeouts.Add(timeout); + if (args[0] == "print") { + Thread.Sleep(50); // consume a real, measurable slice of the shared budget + return (113, "", "Could not find service \"io.kurrent.kcap.daemon.test\" in domain for user gui: 501", false); + } + return (0, "", "", false); + }); + + var ok = mgr.StartBootstrapOnly("test", TimeSpan.FromSeconds(5), out var error); + + await Assert.That(ok).IsTrue(); + await Assert.That(error).IsNull(); + await Assert.That(timeouts.Count).IsEqualTo(2); + // The probe gets the full budget; the bootstrap call must get only what's LEFT of it — + // never another full 5s, which would let the pair invade up to 2x the caller's forward + // remainder (including the separately reserved rollback budget). + await Assert.That(timeouts[0]).IsEqualTo(TimeSpan.FromSeconds(5)); + await Assert.That(timeouts[1]).IsLessThan(TimeSpan.FromSeconds(5)); + }); + } + + [Test] + public async Task StartBootstrapOnly_reports_timeout_when_the_probe_alone_exhausts_the_budget() { + Skip.When(OperatingSystem.IsWindows(), "Uid() P/Invokes libc's getuid, POSIX-only"); + + await WithHome(async _ => { + var mgr = new LaunchdServiceManager(runBounded: (_, args, timeout) => + args[0] == "print" + ? (113, "", "Could not find service \"io.kurrent.kcap.daemon.test\" in domain for user gui: 501", true) // timed out + : (0, "", "", false)); + + var ok = mgr.StartBootstrapOnly("test", TimeSpan.FromSeconds(5), out var error); + + // A timed-out print reports Unknown, not Absent — bootstrap must never run. + await Assert.That(ok).IsFalse(); + await Assert.That(error).IsNotNull(); + }); + } + [Test] public async Task Start_probe_unknown_fails_without_issuing_bootstrap_or_kickstart() { Skip.When(OperatingSystem.IsWindows(), "Uid() P/Invokes libc's getuid, POSIX-only"); diff --git a/test/Capacitor.Cli.Tests.Unit/Services/LaunchdUnitTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/LaunchdUnitTests.cs index 7ba75829f..93e1f7f5f 100644 --- a/test/Capacitor.Cli.Tests.Unit/Services/LaunchdUnitTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Services/LaunchdUnitTests.cs @@ -169,6 +169,155 @@ public async Task EnvFromPlist_throws_on_a_duplicate_key_rather_than_last_win() await Assert.That(() => LaunchdUnit.EnvFromPlist(xml)).Throws(); } + // ── EnvFromPlist: malformed keyed-structure shapes that must throw rather than silently + // degrade (empty map / skipped pair / dodged duplicate detection). ── + + [Test] + public async Task EnvFromPlist_throws_when_EnvironmentVariables_is_paired_with_a_non_dict() { + const string xml = """ + + + + + Labelio.kurrent.kcap.daemon.bad + ProgramArguments + /bin/kcap-daemon + + EnvironmentVariables + not a dict at all + + + + """; + // A non-dict pairing is ambiguous/malformed evidence, not "no env vars" — it must throw + // rather than silently return an empty map. + await Assert.That(() => LaunchdUnit.EnvFromPlist(xml)).Throws(); + } + + [Test] + public async Task EnvFromPlist_throws_when_a_key_is_paired_with_a_non_string_value() { + const string xml = """ + + + + + Labelio.kurrent.kcap.daemon.bad + ProgramArguments + /bin/kcap-daemon + + EnvironmentVariables + KCAP_CONSENT_SEED_DEFAULT1 + + + + """; + // A relevant key paired with the wrong value type is malformed evidence, not absence — it + // must throw rather than silently skip the pair. + await Assert.That(() => LaunchdUnit.EnvFromPlist(xml)).Throws(); + } + + [Test] + public async Task EnvFromPlist_throws_on_consecutive_key_nodes_rather_than_silently_overwriting() { + const string xml = """ + + + + + Labelio.kurrent.kcap.daemon.bad + ProgramArguments + /bin/kcap-daemon + + EnvironmentVariables + KCAP_CONSENT_SEED_DEFAULT + KCAP_PROFILEwork + + + + """; + // The second must not silently overwrite the pending key — that would drop + // KCAP_CONSENT_SEED_DEFAULT entirely and dodge duplicate-key detection for whatever key it + // collided with. + await Assert.That(() => LaunchdUnit.EnvFromPlist(xml)).Throws(); + } + + // ── TopLevelValue: the same strict key/value alternation as EnvFromPlist's inner walk above, + // enforced at the OUTER (top-level dict) level. ── + + [Test] + public async Task EnvFromPlist_throws_on_two_consecutive_identical_top_level_keys_followed_by_one_value() { + const string xml = """ + + + + + Labelio.kurrent.kcap.daemon.dup + ProgramArguments + /bin/kcap-daemon + + EnvironmentVariables + EnvironmentVariables + KCAP_CONSENT_SEED_DEFAULTprompt + + + + """; + // Must still throw — pairing the lone value with whichever key landed last would evade + // duplicate-key detection. + await Assert.That(() => LaunchdUnit.EnvFromPlist(xml)).Throws(); + } + + [Test] + public async Task EnvFromPlist_throws_when_the_EnvironmentVariables_key_is_immediately_followed_by_another_key() { + const string xml = """ + + + + + Labelio.kurrent.kcap.daemon.dup + ProgramArguments + /bin/kcap-daemon + + EnvironmentVariables + Decoy + KCAP_CONSENT_SEED_DEFAULTprompt + + + + """; + // Pairing the dict with the LATER key instead would make EnvironmentVariables read as + // absent — the takeover-safe outcome a gate caller must never see for a malformed plist. + await Assert.That(() => LaunchdUnit.EnvFromPlist(xml)).Throws(); + } + + [Test] + public async Task BinaryFromPlist_throws_on_a_top_level_value_with_no_preceding_key() { + const string xml = """ + + + + + orphan value with no preceding key + ProgramArguments + /bin/kcap-daemon + + + + """; + await Assert.That(() => LaunchdUnit.BinaryFromPlist(xml)).Throws(); + } + + /// A genuinely never-created parent chain must still classify as Absent — link/file + /// evidence detection must not turn every missing directory level into Unreadable. + [Test] + public async Task TryReadPlist_missing_parent_directories_is_still_absent() { + var path = Path.Combine(Directory.CreateTempSubdirectory("kcap-plist-").FullName, "never-created", "sub", "x.plist"); + + var status = LaunchdUnit.TryReadPlist(path, out var content); + + await Assert.That(status).IsEqualTo(LaunchdUnit.PlistRead.Absent); + await Assert.That(content).IsNull(); + } + [Test] public async Task StatusFromPrint_maps_exit_and_state() { await Assert.That(LaunchdUnit.StatusFromPrint(exitCode: 1, stdout: "")).IsEqualTo(ServiceState.NotInstalled); diff --git a/test/Capacitor.Cli.Tests.Unit/Services/ProdPathFixture.cs b/test/Capacitor.Cli.Tests.Unit/Services/ProdPathFixture.cs new file mode 100644 index 000000000..e4d492f57 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/Services/ProdPathFixture.cs @@ -0,0 +1,49 @@ +using Capacitor.Cli.Core; +using Capacitor.Cli.Services; + +namespace Capacitor.Cli.Tests.Unit.Services; + +/// Shared HOME/lock-dir isolation for the production-path suites. +sealed class ProdPathFixture : IDisposable { + readonly string _id; + readonly string? _originalHome; + readonly string _home; + readonly string _lockDir; + + public string PlistPath => LaunchdUnit.PlistPath(_id); + public string DaemonPath { get; } + + public LaunchdServiceManager Manager { get; } + + public ProdPathFixture(string id) { + _id = id; + _originalHome = Environment.GetEnvironmentVariable("HOME"); + _home = Directory.CreateTempSubdirectory($"kcap-{id}-home-").FullName; + Environment.SetEnvironmentVariable("HOME", _home); + + _lockDir = Directory.CreateTempSubdirectory($"kcap-{id}-lock-").FullName; + DaemonLockPaths.OverrideDirectoryForTesting(_lockDir); + + DaemonPath = Path.Combine(_lockDir, "kcap-daemon"); + File.WriteAllText(DaemonPath, ""); + + Manager = new( + runProcess: (_, args) => PrintNotFound(args), + runBounded: (_, args, _) => { + var (code, stdout, stderr) = PrintNotFound(args); + return (code, stdout, stderr, false); + }); + } + + (int ExitCode, string StdOut, string StdErr) PrintNotFound(string[] args) => + args[0] == "print" + ? (113, "", $"Could not find service \"{LaunchdUnit.Label(_id)}\" in domain for user gui: 501") + : (0, "", ""); + + public void Dispose() { + DaemonLockPaths.OverrideDirectoryForTesting(null); + Environment.SetEnvironmentVariable("HOME", _originalHome); + try { Directory.Delete(_home, recursive: true); } catch { /* best effort */ } + try { Directory.Delete(_lockDir, recursive: true); } catch { /* best effort */ } + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyInstallProductionPathTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyInstallProductionPathTests.cs new file mode 100644 index 000000000..e40dac9e0 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyInstallProductionPathTests.cs @@ -0,0 +1,40 @@ +using Capacitor.Cli.Core; +using Capacitor.Cli.Services; + +namespace Capacitor.Cli.Tests.Unit.Services; + +// Real-manager counterpart to ServiceVerifyStartGateProductionPathTests, covering the same +// discriminated-read contract for InstallVerifiedAsync's marker recovery. +[NotInParallel(["HomeEnvVarMutation", nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting"])] +public class ServiceVerifyInstallProductionPathTests { + const string Id = "prodpath-install"; + + static ServiceSpec Spec(string daemonPath) => + new(Id, daemonPath, Path.Combine(Path.GetTempPath(), "prodpath-install-daemon.log"), + new Dictionary(), []); + + // File.Exists reads a directory as absent, so recovery must classify via the discriminated + // read, not existence, or it would delete the marker instead of retaining it. + [Test, NotInParallel] + public async Task Leftover_marker_with_a_directory_at_the_plist_path_is_restore_verification_marker_retained() { + Skip.When(OperatingSystem.IsWindows(), "launchd/HOME-based plist resolution is POSIX-only"); + + using var fx = new ProdPathFixture(Id); + Directory.CreateDirectory(LaunchdUnit.AgentsDir()); + Directory.CreateDirectory(fx.PlistPath); // a DIRECTORY sits at the plist path, not a file + + // The fingerprint value is irrelevant — recovery must never reach the fingerprint compare + // once the read is classified Unreadable. + ServiceTxnMarker.Write(Id, new TxnMarker(1, "install", "written", "stale", "no-unit", "irrelevant-fingerprint")); + + Task Hello(string _, TimeSpan __) => + Task.FromResult(new HelloProbeResult(false, null, null, null)); + + var sut = new ServiceVerify(fx.Manager, _ => 4242, Hello, TimeProvider.System); + + var exit = await sut.InstallVerifiedAsync(Spec(fx.DaemonPath), replace: false, expectedVersion: null); + + await Assert.That(exit).IsEqualTo(VerifyExit.RestoreVerification); + await Assert.That(ServiceTxnMarker.Exists(Id)).IsTrue(); + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartGateProductionPathTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartGateProductionPathTests.cs new file mode 100644 index 000000000..b075a57be --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartGateProductionPathTests.cs @@ -0,0 +1,174 @@ +using Capacitor.Cli.Core; +using Capacitor.Cli.Services; + +namespace Capacitor.Cli.Tests.Unit.Services; + +/// Proves 's default, unstubbed plist read never lets +/// a malformed or unreadable on-disk unit escape the start gate as an uncoded failure — end to end +/// through the REAL manager, complementing 's stubbed-seam +/// coverage of the same contract. +[NotInParallel(["HomeEnvVarMutation", nameof(DaemonLockPaths) + ".OverrideDirectoryForTesting"])] +public class ServiceVerifyStartGateProductionPathTests { + const string Id = "prodpath"; + + // A duplicate top-level ProgramArguments key — never written by LaunchdUnit.Plist itself, so + // this can only be a foreign/corrupt writer — with the gate's own consent directive baked so + // Phase A actually has evidence to re-parse. + const string DuplicateKeyPlist = """ + + + + + Labelio.kurrent.kcap.daemon.prodpath + ProgramArguments + /bin/kcap-daemon + + ProgramArguments + /bin/evil-daemon + + EnvironmentVariables + KCAP_CONSENT_SEED_DEFAULTprompt + + + + """; + + sealed class Fixture : IDisposable { + readonly ProdPathFixture _core = new(Id); + readonly TextWriter _originalErr = Console.Error; + + public string PlistPath => _core.PlistPath; + public LaunchdServiceManager Manager => _core.Manager; + + static Task Hello(string _, TimeSpan __) => + Task.FromResult(new HelloProbeResult(true, 1, "1.2.3", "kcap-daemon")); + + public async Task<(int Exit, string[] StdErrLines)> RunStartVerifiedAsync() { + var sut = new ServiceVerify(Manager, _ => 4242, Hello, TimeProvider.System, + gateEnv: k => k == "KCAP_CONSENT_SEED_DEFAULT" ? "prompt" : null); + + var capturedErr = new StringWriter(); + Console.SetError(capturedErr); + int exit; + try { + exit = await sut.StartVerifiedAsync(Id); + } finally { + Console.SetError(_originalErr); + } + + return (exit, capturedErr.ToString().Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries)); + } + + public void Dispose() { + Console.SetError(_originalErr); + _core.Dispose(); + } + } + + [Test] + public async Task Malformed_unit_on_disk_is_contained_through_the_real_manager_and_reaches_exit_28() { + Skip.When(OperatingSystem.IsWindows(), "launchd/HOME-based plist resolution is POSIX-only"); + + using var fx = new Fixture(); + Directory.CreateDirectory(LaunchdUnit.AgentsDir()); + File.WriteAllText(fx.PlistPath, DuplicateKeyPlist); + + // The `service status --json` style call: Query must NOT throw on this exact malformed + // unit — only report unreadable binary evidence. + var query = fx.Manager.Query(Id); + await Assert.That(query.BinaryPath).IsNull(); + await Assert.That(query.UnitPresent).IsTrue(); + + var (exit, _) = await fx.RunStartVerifiedAsync(); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGate); + } + + /// A unit that IS present but whose plist cannot be read must classify as + /// evidence_unreadable, never the takeover-safe directive_missing a genuinely + /// absent unit gets. Here: a real plist file with its read permission stripped — + /// File.Exists still reports it present, but the open throws. + [Test, NotInParallel] + public async Task Present_but_unreadable_unit_is_evidence_unreadable_not_directive_missing() { + Skip.When(OperatingSystem.IsWindows(), "launchd/HOME-based plist resolution and Unix file modes are POSIX-only"); + + using var fx = new Fixture(); + Directory.CreateDirectory(LaunchdUnit.AgentsDir()); + File.WriteAllText(fx.PlistPath, DuplicateKeyPlist); // content is irrelevant — the read never succeeds + File.SetUnixFileMode(fx.PlistPath, UnixFileMode.None); + + try { + // The unit-level presence signal Query reports must stay true — File.Exists sees the + // file regardless of its permission bits. + var query = fx.Manager.Query(Id); + await Assert.That(query.UnitPresent).IsTrue(); + + var (exit, lines) = await fx.RunStartVerifiedAsync(); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGate); + await Assert.That(lines).Contains("start_gate_reason=evidence_unreadable"); + } finally { + try { File.SetUnixFileMode(fx.PlistPath, UnixFileMode.UserRead | UnixFileMode.UserWrite); } catch { /* best effort */ } + } + } + + /// A DIRECTORY at the plist path is present by every real signal but unreadable as + /// content. File.Exists (which the presence check used to compose on) reads a + /// directory as ABSENT — it only follows through to files — so this must still classify + /// evidence_unreadable, never the takeover-safe directive_missing. + [Test, NotInParallel] + public async Task Directory_at_plist_path_is_evidence_unreadable_not_directive_missing() { + Skip.When(OperatingSystem.IsWindows(), "launchd/HOME-based plist resolution is POSIX-only"); + + using var fx = new Fixture(); + Directory.CreateDirectory(LaunchdUnit.AgentsDir()); + Directory.CreateDirectory(fx.PlistPath); // a DIRECTORY sits at the plist path, not a file + + // The unit-level presence signal Query reports must stay true even though + // File.Exists itself would say otherwise for a directory. + var query = fx.Manager.Query(Id); + await Assert.That(query.UnitPresent).IsTrue(); + + var (exit, lines) = await fx.RunStartVerifiedAsync(); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGate); + await Assert.That(lines).Contains("start_gate_reason=evidence_unreadable"); + } + + /// A dangling symlink AT the plist path — File.Exists reads it as absent (it + /// follows through to the missing target), but the open raises + /// with structural link evidence right at the path. Must classify evidence_unreadable, + /// never directive_missing. + [Test, NotInParallel] + public async Task Dangling_symlink_at_plist_path_is_evidence_unreadable_not_directive_missing() { + Skip.When(OperatingSystem.IsWindows(), "launchd/HOME-based plist resolution is POSIX-only"); + + using var fx = new Fixture(); + Directory.CreateDirectory(LaunchdUnit.AgentsDir()); + File.CreateSymbolicLink(fx.PlistPath, Path.Combine(LaunchdUnit.AgentsDir(), "never-created-target")); + + var (exit, lines) = await fx.RunStartVerifiedAsync(); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGate); + await Assert.That(lines).Contains("start_gate_reason=evidence_unreadable"); + } + + /// A dangling symlink standing in for an ANCESTOR directory of the plist path (here, + /// ~/Library itself) raises rather than + /// — must still classify evidence_unreadable, never + /// directive_missing. + [Test, NotInParallel] + public async Task Dangling_symlink_ancestor_of_plist_path_is_evidence_unreadable_not_directive_missing() { + Skip.When(OperatingSystem.IsWindows(), "launchd/HOME-based plist resolution is POSIX-only"); + + using var fx = new Fixture(); + var home = PathHelpers.HomeDirectory; + var library = Path.Combine(home, "Library"); + File.CreateSymbolicLink(library, Path.Combine(home, "never-created-target")); + + var (exit, lines) = await fx.RunStartVerifiedAsync(); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGate); + await Assert.That(lines).Contains("start_gate_reason=evidence_unreadable"); + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartTests.cs b/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartTests.cs index c3e95478e..a81edd49e 100644 --- a/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Services/ServiceVerifyStartTests.cs @@ -18,6 +18,10 @@ sealed class FakeServiceManager : IVerifyServiceManager { public int? RunningPid = 4242; public string? StopError; + /// Reported as on every + /// call. Defaults true; a genuinely-absent-unit test sets this false. + public bool UnitPresent = true; + /// When set, is reported on only the FIRST /// call and cleared thereafter — for scripting a bootout that fails once (e.g. a foreign /// writer momentarily holding the label) but succeeds on Rollback's own re-attempt. @@ -54,15 +58,15 @@ public ServiceQuery Query(string serviceId, TimeSpan timeout) { if (Started && HangQueryClock is not null) HangQueryClock.Advance(timeout); if (Stopped) { if (ProbeUnknownAfterStop) - return new ServiceQuery(LabelProbe.Unknown, true, ServiceState.Installed, "/bin/kcap-daemon", null); + return new ServiceQuery(LabelProbe.Unknown, UnitPresent, ServiceState.Installed, "/bin/kcap-daemon", null); if (RemainsLoadedUntilSecondStop && StopCalls < 2) - return new ServiceQuery(LabelProbe.Loaded, true, ServiceState.Running, "/bin/kcap-daemon", RunningPid); + return new ServiceQuery(LabelProbe.Loaded, UnitPresent, ServiceState.Running, "/bin/kcap-daemon", RunningPid); if (!RemainsLoadedAfterStop) - return new ServiceQuery(LabelProbe.Absent, true, ServiceState.Installed, "/bin/kcap-daemon", null); + return new ServiceQuery(LabelProbe.Absent, UnitPresent, ServiceState.Installed, "/bin/kcap-daemon", null); } if (Started) - return new ServiceQuery(LabelProbe.Loaded, true, ServiceState.Running, "/bin/kcap-daemon", RunningPid); - return new ServiceQuery(LabelProbe.Absent, true, ServiceState.Installed, "/bin/kcap-daemon", null); + return new ServiceQuery(LabelProbe.Loaded, UnitPresent, ServiceState.Running, "/bin/kcap-daemon", RunningPid); + return new ServiceQuery(LabelProbe.Absent, UnitPresent, ServiceState.Installed, "/bin/kcap-daemon", null); } public void WriteAndBootstrap(ServiceSpec spec, TimeSpan timeout) { } @@ -450,30 +454,42 @@ static string MinimalPlist(string binary, string consentSeedDefault, string? exp _ => null, }; - [Test] - public async Task Pre_mutation_gate_failure_returns_28_and_touches_nothing() { + // Contradictory evidence (query saw the unit; the read didn't) must classify + // evidence_unreadable, never directive_missing — that's reserved for genuine agreed-absence. + [Test, NotInParallel] + [Arguments(false, "directive_missing")] // query and read agree the unit is absent + [Arguments(true, "evidence_unreadable")] // query saw the unit but the read reports absent + public async Task Phase_a_absence_evidence_disambiguates_directive_missing_from_evidence_unreadable( + bool unitPresent, string expectedReason) { var dir = Directory.CreateTempSubdirectory().FullName; DaemonLockPaths.OverrideDirectoryForTesting(dir); + var originalErr = Console.Error; + var capturedErr = new StringWriter(); try { - var manager = new FakeServiceManager(); + var manager = new FakeServiceManager { UnitPresent = unitPresent }; Task Hello(string _, TimeSpan __) => Task.FromResult(new HelloProbeResult(true, 1, "1.2.3", "kcap-daemon")); - // The unit bakes nothing (readPlist "sees" no unit) while this invocation carries the - // consent-seed directive — Phase A's DirectiveMissing case — so the gate must fire - // before the fresh query's under-lock work does anything else. var sut = new ServiceVerify(manager, _ => 4242, Hello, TimeProvider.System, readPlist: _ => null, + plistExists: _ => false, gateEnv: k => k == "KCAP_CONSENT_SEED_DEFAULT" ? "prompt" : null); + Console.SetError(capturedErr); var exit = await sut.StartVerifiedAsync(Id); + Console.SetError(originalErr); await Assert.That(exit).IsEqualTo(VerifyExit.StartGate); + var lines = capturedErr.ToString().Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); + await Assert.That(lines).Contains($"start_gate_reason={expectedReason}"); await Assert.That(manager.Calls.Count).IsEqualTo(1); await Assert.That(manager.Calls.All(c => c == "query")).IsTrue(); await Assert.That(ServiceTxnMarker.Exists(Id)).IsFalse(); - } finally { DaemonLockPaths.OverrideDirectoryForTesting(null); } + } finally { + Console.SetError(originalErr); + DaemonLockPaths.OverrideDirectoryForTesting(null); + } } [Test] @@ -710,6 +726,105 @@ Task Hello(string _, TimeSpan __) => } finally { DaemonLockPaths.OverrideDirectoryForTesting(null); } } + /// The gated path re-checks the plist/digest once more after readiness is confirmed, + /// not just pre-bootstrap: content only drifts on the FIRST read taken after both readiness + /// probes already saw matching content. + [Test] + public async Task Post_readiness_recheck_detects_plist_drift_after_confirmed_ready_and_rolls_back_to_29() { + var dir = Directory.CreateTempSubdirectory().FullName; + DaemonLockPaths.OverrideDirectoryForTesting(dir); + try { + var manager = new FakeServiceManager(); + + Task Hello(string _, TimeSpan __) => + Task.FromResult(new HelloProbeResult(true, 1, "1.2.3", "kcap-daemon")); + + var reads = 0; + string? ReadPlist(string _) { + reads++; + // Phase A (read 1) and Phase B's own pre-bootstrap recheck (read 2) both see the + // same passing content — readiness is then genuinely, twice confirmed. A foreign + // writer swaps the plist only AFTER that, so the post-readiness recheck (read 3) + // must be what catches it. + return reads <= 2 + ? MinimalPlist("/bin/kcap-daemon", "prompt", GatedServerUrl) + : MinimalPlist("/bin/kcap-daemon-moved", "prompt", GatedServerUrl); + } + + var sut = new ServiceVerify(manager, _ => 4242, Hello, TimeProvider.System, + readPlist: ReadPlist, + gateEnv: GatedEnvWithIdentity(), + digestMatches: _ => true); + + var exit = await sut.StartVerifiedAsync(Id); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGateDrift); + // Bootstrap DID run — this is a post-mutation catch, unlike the Phase A/B drift tests + // above, which never start at all. + await Assert.That(manager.StartCalls).IsEqualTo(1); + await Assert.That(ServiceTxnMarker.Exists(Id)).IsFalse(); + await Assert.That(reads).IsGreaterThanOrEqualTo(3); + } finally { DaemonLockPaths.OverrideDirectoryForTesting(null); } + } + + /// The post-readiness recheck also re-verifies the digest, independent of plist + /// content drift — a swapped binary at the SAME baked path must still roll back to 29. + [Test] + public async Task Post_readiness_recheck_detects_digest_drift_after_confirmed_ready_and_rolls_back_to_29() { + var dir = Directory.CreateTempSubdirectory().FullName; + DaemonLockPaths.OverrideDirectoryForTesting(dir); + try { + var manager = new FakeServiceManager(); + + Task Hello(string _, TimeSpan __) => + Task.FromResult(new HelloProbeResult(true, 1, "1.2.3", "kcap-daemon")); + + var digestChecks = 0; + bool DigestMatches(string _) { + digestChecks++; + // Passes for Phase A and Phase B's pre-bootstrap recheck; fails from the third + // check onward — the post-readiness recheck. + return digestChecks <= 2; + } + + var sut = new ServiceVerify(manager, _ => 4242, Hello, TimeProvider.System, + readPlist: _ => MinimalPlist("/bin/kcap-daemon", "prompt", GatedServerUrl), + gateEnv: GatedEnvWithIdentity(), + digestMatches: DigestMatches); + + var exit = await sut.StartVerifiedAsync(Id); + + await Assert.That(exit).IsEqualTo(VerifyExit.StartGateDrift); + await Assert.That(manager.StartCalls).IsEqualTo(1); + await Assert.That(ServiceTxnMarker.Exists(Id)).IsFalse(); + } finally { DaemonLockPaths.OverrideDirectoryForTesting(null); } + } + + /// The UNGATED path (no consent-seed directive carried by the invoker) must never run + /// the post-readiness recheck — start behavior for a bare terminal invocation is + /// unchanged. + [Test] + public async Task Ungated_start_never_runs_the_post_readiness_recheck() { + var dir = Directory.CreateTempSubdirectory().FullName; + DaemonLockPaths.OverrideDirectoryForTesting(dir); + try { + var manager = new FakeServiceManager(); + var readPlistCalls = 0; + + Task Hello(string _, TimeSpan __) => + Task.FromResult(new HelloProbeResult(true, 1, "1.2.3", "kcap-daemon")); + + var sut = new ServiceVerify(manager, _ => 4242, Hello, TimeProvider.System, + readPlist: _ => { readPlistCalls++; return MinimalPlist("/bin/kcap-daemon", "prompt", GatedServerUrl); }); + // no gateEnv — ungated + + var exit = await sut.StartVerifiedAsync(Id); + + await Assert.That(exit).IsEqualTo(VerifyExit.Ok); + await Assert.That(readPlistCalls).IsEqualTo(0); + } finally { DaemonLockPaths.OverrideDirectoryForTesting(null); } + } + [Test] public async Task Bootstrap_only_never_kickstarts_a_label_that_turned_loaded_just_before_it() { var dir = Directory.CreateTempSubdirectory().FullName; diff --git a/test/Capacitor.Cli.Tests.Unit/Setup/AgentDetectionTests.cs b/test/Capacitor.Cli.Tests.Unit/Setup/AgentDetectionTests.cs index a99b8d83a..13e4bb363 100644 --- a/test/Capacitor.Cli.Tests.Unit/Setup/AgentDetectionTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/Setup/AgentDetectionTests.cs @@ -118,6 +118,68 @@ public async Task BinaryOnPath_returns_false_when_path_env_is_null_or_empty() { await Assert.That(AgentDetection.BinaryOnPath("claude", Inputs(pathEnv: ""))).IsFalse(); } + [Test] + public async Task BinaryOnPath_dedupes_a_repeated_PATH_entry() { + if (OperatingSystem.IsWindows()) return; + + var dir = Directory.CreateTempSubdirectory("kcap-detect-dedupe-").FullName; + var claude = Path.Combine(dir, "claude"); + await File.WriteAllTextAsync(claude, "#!/bin/sh\n"); + File.SetUnixFileMode(claude, UnixFileMode.UserRead | UnixFileMode.UserExecute); + + // The same directory repeated three times must still be found (and probed only once per + // Distinct dir, not once per occurrence). + var pathEnv = $"{dir}{Path.PathSeparator}{dir}{Path.PathSeparator}{dir}"; + await Assert.That(AgentDetection.BinaryOnPath("claude", Inputs(pathEnv: pathEnv))).IsTrue(); + } + + [Test] + public async Task BinaryOnPath_windows_dedupes_case_variant_directories() { + var probed = new List(); + bool CountingProbe(string path, bool isWindows) { probed.Add(path); return false; } + + var winInputs = new AgentDetectionInputs( + PathEnv: @"C:\Tools;c:\tools;C:\TOOLS", PathExt: ".EXE", IsWindows: true, Home: "/nonexistent"); + + AgentDetection.BinaryOnPath("claude", winInputs, CountingProbe); + + // Three case-variant spellings of the same directory collapse to ONE probed candidate + // (one extension × one deduped dir) under Windows' case-insensitive PATH identity. + await Assert.That(probed.Count).IsEqualTo(1); + } + + [Test] + public async Task BinaryOnPath_unix_dedupes_same_case_duplicate_directories() { + if (OperatingSystem.IsWindows()) return; + + var probed = new List(); + bool CountingProbe(string path, bool isWindows) { probed.Add(path); return false; } + + var inputs = new AgentDetectionInputs( + PathEnv: "/usr/bin:/usr/bin:/usr/bin", PathExt: null, IsWindows: false, Home: "/nonexistent"); + + AgentDetection.BinaryOnPath("claude", inputs, CountingProbe); + + await Assert.That(probed.Count).IsEqualTo(1); + } + + [Test] + public async Task BinaryOnPath_unix_probes_case_variant_directories_separately() { + if (OperatingSystem.IsWindows()) return; + + var probed = new List(); + bool CountingProbe(string path, bool isWindows) { probed.Add(path); return false; } + + var inputs = new AgentDetectionInputs( + PathEnv: "/usr/bin:/USR/BIN", PathExt: null, IsWindows: false, Home: "/nonexistent"); + + AgentDetection.BinaryOnPath("claude", inputs, CountingProbe); + + // Unix PATH identity is case-sensitive — these are two genuinely distinct directories, + // each probed once, unlike Windows' case-insensitive dedup above. + await Assert.That(probed.Count).IsEqualTo(2); + } + [Test] public async Task BinaryOnPath_skips_empty_path_entries_without_throwing() { if (OperatingSystem.IsWindows()) return; @@ -192,33 +254,14 @@ public async Task FromEnvironment_reads_the_real_process_PATH() { await Assert.That(inputs.IsWindows).IsEqualTo(OperatingSystem.IsWindows()); } - // ── BinaryOnPath(string): the single-arg convenience overload (FromEnvironment() + the pure - // walk), against the REAL process environment — what call sites moved off AgentDetector to. ── - - [Test, NotInParallel("PATH_env_mutation")] - public async Task BinaryOnPath_single_arg_returns_false_when_path_env_is_empty() { - var original = Environment.GetEnvironmentVariable("PATH"); - Environment.SetEnvironmentVariable("PATH", ""); - try { - await Assert.That(AgentDetection.BinaryOnPath("anything-at-all")).IsFalse(); - } finally { - Environment.SetEnvironmentVariable("PATH", original); - } - } - - [Test, NotInParallel("PATH_env_mutation")] - public async Task BinaryOnPath_single_arg_returns_false_when_path_env_is_null() { - var original = Environment.GetEnvironmentVariable("PATH"); - Environment.SetEnvironmentVariable("PATH", null); - try { - await Assert.That(AgentDetection.BinaryOnPath("anything-at-all")).IsFalse(); - } finally { - Environment.SetEnvironmentVariable("PATH", original); - } - } + // ── BinaryOnPath(string): the single-arg convenience overload is documented as nothing more + // than FromEnvironment() + the pure walk (see AgentDetection.cs) — FromEnvironment_reads_the_ + // real_process_PATH above already smoke-tests that wiring. The edge cases below (empty/null + // PATH, exec-bit requirement) are therefore driven through the pure 2-arg overload with + // injected inputs — never mutating real process PATH — so nothing here needs NotInParallel. ── - [Test, NotInParallel("PATH_env_mutation")] - public async Task BinaryOnPath_single_arg_unix_requires_any_execute_bit() { + [Test] + public async Task BinaryOnPath_pure_unix_requires_any_execute_bit() { if (OperatingSystem.IsWindows()) return; // Unix-only var tmp = Directory.CreateTempSubdirectory("kcap-agentprobe-").FullName; @@ -230,13 +273,39 @@ public async Task BinaryOnPath_single_arg_unix_requires_any_execute_bit() { File.SetUnixFileMode(exec, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); // 0700 File.SetUnixFileMode(nonExec, UnixFileMode.UserRead | UnixFileMode.UserWrite); // 0600 - var original = Environment.GetEnvironmentVariable("PATH"); - Environment.SetEnvironmentVariable("PATH", tmp); - try { - await Assert.That(AgentDetection.BinaryOnPath("agentprobe-exec")).IsTrue(); - await Assert.That(AgentDetection.BinaryOnPath("agentprobe-nonexec")).IsFalse(); - } finally { - Environment.SetEnvironmentVariable("PATH", original); - } + await Assert.That(AgentDetection.BinaryOnPath("agentprobe-exec", Inputs(pathEnv: tmp))).IsTrue(); + await Assert.That(AgentDetection.BinaryOnPath("agentprobe-nonexec", Inputs(pathEnv: tmp))).IsFalse(); + } + + // ── BinaryOnPath separator is derived from the injected platform, never the host-global + // Path.PathSeparator — pin both directions purely. ── + + [Test] + public async Task BinaryOnPath_windows_input_splits_on_semicolon_regardless_of_host_platform() { + var dir = Directory.CreateTempSubdirectory("kcap-detect-winsep-").FullName; + await File.WriteAllTextAsync(Path.Combine(dir, "claude.CMD"), "@echo off\n"); + + var otherDir = Directory.CreateTempSubdirectory("kcap-detect-winsep2-").FullName; + var winInputs = new AgentDetectionInputs( + PathEnv: $"{otherDir};{dir}", PathExt: ".EXE;.CMD", IsWindows: true, Home: "/nonexistent"); + + // A colon-joined PATH would be read as ONE bogus entry under a semicolon split and never + // find claude.CMD in `dir` — this only passes if the separator is genuinely ';' here. + await Assert.That(AgentDetection.BinaryOnPath("claude", winInputs)).IsTrue(); + } + + [Test] + public async Task BinaryOnPath_unix_input_splits_on_colon() { + if (OperatingSystem.IsWindows()) return; // exec-bit semantics only make sense on Unix + + var dir = Directory.CreateTempSubdirectory("kcap-detect-unixsep-").FullName; + var claude = Path.Combine(dir, "claude"); + await File.WriteAllTextAsync(claude, "#!/bin/sh\n"); + File.SetUnixFileMode(claude, UnixFileMode.UserRead | UnixFileMode.UserExecute); + + var otherDir = Directory.CreateTempSubdirectory("kcap-detect-unixsep2-").FullName; + var r = AgentDetection.Detect(Inputs(pathEnv: $"{otherDir}:{dir}", home: "/nonexistent")); + + await Assert.That(r.Claude.BinaryFound).IsTrue(); } }