From 8a6f10066bfd3843b2c01cdbf6a54b04a7e8e37d Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 24 Apr 2026 23:42:51 +0200 Subject: [PATCH 1/6] #338: Filter non-material upstream commits from update notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Shallow git fetch + name-only diff after ls-remote detects a new SHA; when every changed path is deny-listed infrastructure (README, LICENSE, CHANGELOG, .github/, node_modules/, .build/, Makefile), the notification is suppressed and the registry baseline advances so the same commits don't re-trigger on future cooldown windows. - techpack.yaml changes always surface unfiltered — manifest edits can swap hook scripts and MCP commands, so suppressing them would be a supply-chain risk. - Fetch or diff failure surfaces the notification unfiltered (never-hide invariant); pipeline covered by pure classifier + mock-shell orchestrator + end-to-end PackRegistryFile write-back tests. --- Sources/mcs/Core/UpdateChecker.swift | 158 ++++++++- Sources/mcs/ExternalPack/PackHeuristics.swift | 15 +- .../mcs/ExternalPack/PackRegistryFile.swift | 18 ++ Tests/MCSTests/PackHeuristicsTests.swift | 28 ++ Tests/MCSTests/UpdateCheckerTests.swift | 305 ++++++++++++++++++ docs/architecture.md | 6 + docs/cli.md | 3 + 7 files changed, 520 insertions(+), 13 deletions(-) diff --git a/Sources/mcs/Core/UpdateChecker.swift b/Sources/mcs/Core/UpdateChecker.swift index ae974e1..0047756 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -7,7 +7,10 @@ import Foundation /// remotes produce no output, matching the design goal of non-intrusive checks. struct UpdateChecker { let environment: Environment - let shell: ShellRunner + /// `any ShellRunning` so tests can inject `MockShellRunner` for the new + /// post-SHA-change git fetch/diff calls. Production call sites pass a concrete + /// `ShellRunner`, which upcasts implicitly. `ShellRunning: Sendable`. + let shell: any ShellRunning /// Default cooldown interval: 24 hours. static let cooldownInterval: TimeInterval = 86400 @@ -168,42 +171,173 @@ struct UpdateChecker { // MARK: - Pack Checks + /// Classification of an upstream commit-SHA change. Drives whether the noise filter + /// suppresses the update notification (issue #338). + /// + /// **Never-hide invariant:** `.suppressed` is the narrow special case. Every error + /// path (fetch fail, diff fail, missing clone, parse error) must fall through to + /// `.unknown`, which surfaces the notification as today. Do not invert this polarity. + enum UpstreamChange: Equatable { + case suppressed + case material([String]) + case unknown + } + + /// Pure classifier for the changed-path list returned by `git diff --name-only`. + /// No I/O — unit-testable in isolation. The orchestrator (`classifyUpstreamChange`) + /// runs git and calls into this. + /// + /// Rules, applied in order: + /// 1. If any path equals `techpack.yaml` → `.material` (supply-chain invariant — + /// manifest edits can swap hook scripts, MCP commands, install surface). + /// 2. A path is "noise" if its leading segment matches `PackHeuristics.ignoredDirectories` + /// (e.g. `.github/workflows/ci.yml`) OR its basename matches + /// `PackHeuristics.infrastructureFilesForUpdateCheck` (e.g. `README.md`, `LICENSE`). + /// 3. If all paths are noise → `.suppressed`. Any survivor → `.material(survivors)`. + static func classifyDiffPaths(_ paths: [String]) -> UpstreamChange { + let cleaned = paths + .map { $0.trimmingCharacters(in: .whitespaces) } + .filter { !$0.isEmpty } + guard !cleaned.isEmpty else { return .suppressed } + + if cleaned.contains(Constants.ExternalPacks.manifestFilename) { + return .material([Constants.ExternalPacks.manifestFilename]) + } + + var material: [String] = [] + for path in cleaned where !isNoisePath(path) { + material.append(path) + } + return material.isEmpty ? .suppressed : .material(material) + } + + private static func isNoisePath(_ path: String) -> Bool { + let leadingSegment = path.split(separator: "/", maxSplits: 1).first.map(String.init) ?? path + if PackHeuristics.ignoredDirectories.contains(leadingSegment) { + return true + } + // Basename match only when the path is a single segment — a file called `README.md` + // inside `hooks/` (i.e. `hooks/README.md`) is not suppressed; only pack-root infra is. + if !path.contains("/"), PackHeuristics.infrastructureFilesForUpdateCheck.contains(path) { + return true + } + return false + } + + /// Orchestrator — runs `git fetch` + `git diff --name-only` in the pack clone + /// and feeds the diff into `classifyDiffPaths`. Every error path falls through + /// to `.unknown` per the never-hide invariant. + func classifyUpstreamChange(entry: PackRegistryFile.PackEntry) -> UpstreamChange { + guard let workDirURL = entry.resolvedPath(packsDirectory: environment.packsDirectory) else { + return .unknown + } + let workDir = workDirURL.path + let ref = entry.ref ?? "HEAD" + + let fetchResult = shell.run( + environment.gitPath, + arguments: ["fetch", "--depth", "1", "origin", ref], + workingDirectory: workDir, + additionalEnvironment: Self.gitNoPromptEnv + ) + guard fetchResult.succeeded else { return .unknown } + + let diffResult = shell.run( + environment.gitPath, + arguments: ["diff", "--name-only", "HEAD", "FETCH_HEAD"], + workingDirectory: workDir, + additionalEnvironment: Self.gitNoPromptEnv + ) + guard diffResult.succeeded else { return .unknown } + + let paths = diffResult.stdout.split(separator: "\n").map(String.init) + return Self.classifyDiffPaths(paths) + } + + /// Per-iteration result of `checkPackUpdates`: an optional notification to emit + /// and an optional registry baseline advance (applied post-loop to serialize writes). + private struct PackCheckOutcome { + let update: PackUpdate? + let advance: SHAAdvance? + } + + private struct SHAAdvance { + let identifier: String + let newSHA: String + } + /// Check each git pack for remote updates via `git ls-remote`. /// Local packs are skipped. Checks run in parallel. Network failures are silently ignored per-pack. + /// + /// When a remote SHA differs, runs a noise filter (`classifyUpstreamChange`) that may + /// suppress the notification for README/CI/infra-only commits and advance the registry + /// baseline so the same commits don't re-trigger (issue #338). func checkPackUpdates(entries: [PackRegistryFile.PackEntry]) -> [PackUpdate] { let gitEntries = entries.filter { !$0.isLocalPack } guard !gitEntries.isEmpty else { return [] } // Each index is written by exactly one iteration — no data race. // nonisolated(unsafe) is needed because concurrentPerform's closure is @Sendable. - nonisolated(unsafe) var results = [PackUpdate?](repeating: nil, count: gitEntries.count) + nonisolated(unsafe) var results = [PackCheckOutcome?](repeating: nil, count: gitEntries.count) DispatchQueue.concurrentPerform(iterations: gitEntries.count) { index in let entry = gitEntries[index] let ref = entry.ref ?? "HEAD" - let result = shell.run( + let lsRemote = shell.run( environment.gitPath, arguments: ["ls-remote", entry.sourceURL, ref], additionalEnvironment: Self.gitNoPromptEnv ) - guard result.succeeded, - let remoteSHA = Self.parseRemoteSHA(from: result.stdout) + guard lsRemote.succeeded, + let remoteSHA = Self.parseRemoteSHA(from: lsRemote.stdout), + remoteSHA != entry.commitSHA else { return } - if remoteSHA != entry.commitSHA { - results[index] = PackUpdate( - identifier: entry.identifier, - displayName: entry.displayName, - localSHA: entry.commitSHA, - remoteSHA: remoteSHA + let pendingUpdate = PackUpdate( + identifier: entry.identifier, + displayName: entry.displayName, + localSHA: entry.commitSHA, + remoteSHA: remoteSHA + ) + + switch classifyUpstreamChange(entry: entry) { + case .suppressed: + results[index] = PackCheckOutcome( + update: nil, + advance: SHAAdvance(identifier: entry.identifier, newSHA: remoteSHA) ) + case .material, .unknown: + // .unknown → never-hide: surface the notification unfiltered. + results[index] = PackCheckOutcome(update: pendingUpdate, advance: nil) } } - return results.compactMap(\.self) + let advances = results.compactMap { $0?.advance } + if !advances.isEmpty { + applyRegistryAdvances(advances) + } + + return results.compactMap { $0?.update } + } + + /// Apply collected SHA advances to `registry.yaml` in one load→mutate→save round-trip. + /// Serializes the writes that were collected from the parallel `concurrentPerform` loop. + /// Silent on failure: next check will re-classify (same pattern as `saveCache`). + private func applyRegistryAdvances(_ advances: [SHAAdvance]) { + let registry = PackRegistryFile(path: environment.packsRegistry) + do { + var data = try registry.load() + for advance in advances { + guard let existing = registry.pack(identifier: advance.identifier, in: data) else { continue } + registry.register(existing.withCommitSHA(advance.newSHA), in: &data) + } + try registry.save(data) + } catch { + // Registry write failure is non-fatal — next check will classify again. + } } // MARK: - CLI Version Check diff --git a/Sources/mcs/ExternalPack/PackHeuristics.swift b/Sources/mcs/ExternalPack/PackHeuristics.swift index a4fd818..905c265 100644 --- a/Sources/mcs/ExternalPack/PackHeuristics.swift +++ b/Sources/mcs/ExternalPack/PackHeuristics.swift @@ -79,7 +79,9 @@ enum PackHeuristics { } /// Directories at the pack root that are infrastructure, not pack content. - private static let ignoredDirectories: Set = [ + /// Shared by `checkUnreferencedFiles` and `UpdateChecker`'s noise filter — a path + /// whose leading segment is in this set is never material to a pack install. + static let ignoredDirectories: Set = [ ".git", ".github", ".gitlab", ".vscode", "node_modules", "__pycache__", ".build", ] @@ -168,6 +170,9 @@ enum PackHeuristics { } /// Files at the pack root that are expected infrastructure, not content. + /// Used by `checkRootLevelContentFiles` to avoid warning about these files. + /// Intentionally includes `techpack.yaml` — see `infrastructureFilesForUpdateCheck` + /// below for the (deliberately different) set the update-check filter uses. private static let infrastructureFiles: Set = [ Constants.ExternalPacks.manifestFilename, "README.md", "README", "LICENSE", "LICENSE.md", "CHANGELOG.md", "CONTRIBUTING.md", ".gitignore", ".editorconfig", @@ -175,6 +180,14 @@ enum PackHeuristics { "Makefile", "Dockerfile", ".dockerignore", ] + /// Files treated as non-material by `UpdateChecker`'s noise filter (issue #338). + /// **Intentionally distinct from `infrastructureFiles`**: `techpack.yaml` is + /// excluded here because a manifest edit can change the entire install surface + /// (new hook scripts, different MCP commands). Silently suppressing manifest-only + /// commits would be a supply-chain attack vector. Do not deduplicate these sets. + static let infrastructureFilesForUpdateCheck: Set = + infrastructureFiles.subtracting([Constants.ExternalPacks.manifestFilename]) + private static func checkRootLevelContentFiles( manifest: ExternalPackManifest, packPath: URL diff --git a/Sources/mcs/ExternalPack/PackRegistryFile.swift b/Sources/mcs/ExternalPack/PackRegistryFile.swift index cc8d97f..0071480 100644 --- a/Sources/mcs/ExternalPack/PackRegistryFile.swift +++ b/Sources/mcs/ExternalPack/PackRegistryFile.swift @@ -31,6 +31,24 @@ struct PackRegistryFile { } return PathContainment.safePath(relativePath: localPath, within: packsDirectory) } + + /// Return a copy with `commitSHA` replaced. + /// Used when `UpdateChecker`'s noise filter advances the baseline without + /// updating the working-tree checkout (see issue #338). + func withCommitSHA(_ sha: String) -> Self { + PackEntry( + identifier: identifier, + displayName: displayName, + author: author, + sourceURL: sourceURL, + ref: ref, + commitSHA: sha, + localPath: localPath, + addedAt: addedAt, + trustedScriptHashes: trustedScriptHashes, + isLocal: isLocal + ) + } } struct RegistryData: Codable { diff --git a/Tests/MCSTests/PackHeuristicsTests.swift b/Tests/MCSTests/PackHeuristicsTests.swift index 6db046b..7e512df 100644 --- a/Tests/MCSTests/PackHeuristicsTests.swift +++ b/Tests/MCSTests/PackHeuristicsTests.swift @@ -684,4 +684,32 @@ struct PackHeuristicsTests { #expect(!dirWarnings.isEmpty) #expect(dirWarnings.allSatisfy { $0.severity == .warning }) } + + // MARK: - infrastructureFilesForUpdateCheck (issue #338) + + @Test("infrastructureFilesForUpdateCheck excludes techpack.yaml (supply-chain invariant)") + func updateCheckSetExcludesManifest() { + #expect(!PackHeuristics.infrastructureFilesForUpdateCheck.contains( + Constants.ExternalPacks.manifestFilename + )) + } + + @Test("infrastructureFilesForUpdateCheck still contains the other infra files") + func updateCheckSetContainsInfraFiles() { + let set = PackHeuristics.infrastructureFilesForUpdateCheck + #expect(set.contains("README.md")) + #expect(set.contains("LICENSE")) + #expect(set.contains("CHANGELOG.md")) + #expect(set.contains(".gitignore")) + #expect(set.contains("Makefile")) + } + + @Test("ignoredDirectories is accessible and contains expected entries") + func ignoredDirsAccessible() { + let dirs = PackHeuristics.ignoredDirectories + #expect(dirs.contains(".git")) + #expect(dirs.contains(".github")) + #expect(dirs.contains("node_modules")) + #expect(dirs.contains(".build")) + } } diff --git a/Tests/MCSTests/UpdateCheckerTests.swift b/Tests/MCSTests/UpdateCheckerTests.swift index 202e99e..35ad62b 100644 --- a/Tests/MCSTests/UpdateCheckerTests.swift +++ b/Tests/MCSTests/UpdateCheckerTests.swift @@ -237,6 +237,311 @@ struct UpdateCheckerPerformCheckTests { } } +// MARK: - Noise Filter Tests (issue #338) + +struct UpdateCheckerClassifyDiffPathsTests { + @Test("Empty diff → suppressed") + func emptyIsSuppressed() { + #expect(UpdateChecker.classifyDiffPaths([]) == .suppressed) + #expect(UpdateChecker.classifyDiffPaths(["", " "]) == .suppressed) + } + + @Test("README-only diff → suppressed") + func readmeOnlySuppressed() { + #expect(UpdateChecker.classifyDiffPaths(["README.md"]) == .suppressed) + } + + @Test("All-infra root files → suppressed") + func infraFilesSuppressed() { + let paths = ["README.md", "LICENSE", "CHANGELOG.md", ".gitignore", "Makefile"] + #expect(UpdateChecker.classifyDiffPaths(paths) == .suppressed) + } + + @Test("techpack.yaml change → always material (supply-chain invariant)") + func manifestAlwaysMaterial() { + #expect(UpdateChecker.classifyDiffPaths(["techpack.yaml"]) + == .material(["techpack.yaml"])) + // Even mixed with noise, manifest short-circuits to material. + #expect(UpdateChecker.classifyDiffPaths(["README.md", "techpack.yaml"]) + == .material(["techpack.yaml"])) + } + + @Test("Ignored-dir leading segment → suppressed") + func ignoredDirsSuppressed() { + #expect(UpdateChecker.classifyDiffPaths([".github/workflows/ci.yml"]) == .suppressed) + #expect(UpdateChecker.classifyDiffPaths(["node_modules/foo/bar.js"]) == .suppressed) + #expect(UpdateChecker.classifyDiffPaths([".build/debug/output"]) == .suppressed) + } + + @Test("Hook script change → material") + func hookScriptMaterial() { + let result = UpdateChecker.classifyDiffPaths(["hooks/session-start.sh"]) + #expect(result == .material(["hooks/session-start.sh"])) + } + + @Test("Mixed material + noise → material (only material paths)") + func mixedReturnsMaterialOnly() { + let result = UpdateChecker.classifyDiffPaths([ + "README.md", "hooks/session-start.sh", ".github/workflows/ci.yml", + ]) + #expect(result == .material(["hooks/session-start.sh"])) + } + + @Test("Infra basename inside subdir → material (basename match only applies to root)") + func nonRootInfraNotSuppressed() { + // `hooks/README.md` is NOT suppressed — only the pack-root README is. + let result = UpdateChecker.classifyDiffPaths(["hooks/README.md"]) + #expect(result == .material(["hooks/README.md"])) + } + + @Test("Unknown root dir (docs/) → material (Phase 1 has no author ignore: yet)") + func unknownDirMaterial() { + let result = UpdateChecker.classifyDiffPaths(["docs/guide.md"]) + #expect(result == .material(["docs/guide.md"])) + } + + @Test("Whitespace and empty lines are stripped") + func whitespaceStripped() { + let result = UpdateChecker.classifyDiffPaths([ + " README.md ", "", " ", "LICENSE", + ]) + #expect(result == .suppressed) + } +} + +struct UpdateCheckerOrchestratorTests { + private func makeEntry(identifier: String = "pack-a", commitSHA: String = "old123") + -> PackRegistryFile.PackEntry { + makeRegistryEntry(identifier: identifier, commitSHA: commitSHA) + } + + private func makeChecker(home: URL, mock: MockShellRunner) -> UpdateChecker { + let env = Environment(home: home) + return UpdateChecker(environment: env, shell: mock) + } + + private func preparePackDir(home: URL, identifier: String) throws { + let dir = home.appendingPathComponent(".mcs/packs/\(identifier)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + } + + @Test("fetch + diff succeed with noise → suppressed") + func fetchDiffNoiseSuppressed() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "", stderr: ""), // fetch + ShellResult(exitCode: 0, stdout: "README.md\nLICENSE\n", stderr: ""), // diff + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: makeEntry()) + + #expect(result == .suppressed) + #expect(mock.runCalls.count == 2) + #expect(mock.runCalls[0].arguments.contains("fetch")) + #expect(mock.runCalls[1].arguments.contains("diff")) + } + + @Test("fetch + diff succeed with material → material") + func fetchDiffMaterial() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "hooks/session-start.sh\n", stderr: ""), + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: makeEntry()) + + #expect(result == .material(["hooks/session-start.sh"])) + } + + @Test("fetch fails → unknown (never-hide, diff not called)") + func fetchFailsNeverHide() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 128, stdout: "", stderr: "fatal: unable to access"), + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: makeEntry()) + + #expect(result == .unknown) + #expect(mock.runCalls.count == 1) // diff should not be called after fetch fails + } + + @Test("diff fails → unknown (never-hide)") + func diffFailsNeverHide() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 128, stdout: "", stderr: "fatal: bad revision"), + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: makeEntry()) + + #expect(result == .unknown) + } + + @Test("git commands pass GIT_TERMINAL_PROMPT=0 to avoid credential prompts") + func credentialSuppression() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + _ = checker.classifyUpstreamChange(entry: makeEntry()) + + for call in mock.runCalls { + #expect(call.additionalEnvironment["GIT_TERMINAL_PROMPT"] == "0") + } + } +} + +struct UpdateCheckerPackUpdatesTests { + private func writeRegistry(at env: Environment, entries: [PackRegistryFile.PackEntry]) throws { + let registry = PackRegistryFile(path: env.packsRegistry) + var data = PackRegistryFile.RegistryData() + for entry in entries { + registry.register(entry, in: &data) + } + try registry.save(data) + } + + private func preparePackDir(home: URL, identifier: String) throws { + let dir = home.appendingPathComponent(".mcs/packs/\(identifier)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + } + + @Test("Noise-only upstream commit → no PackUpdate, registry commitSHA advances") + func noiseSuppressedAndBaselineAdvances() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let env = Environment(home: tmpDir) + let entry = makeRegistryEntry(identifier: "pack-a", commitSHA: "old123") + try writeRegistry(at: env, entries: [entry]) + + let mock = MockShellRunner(environment: env) + mock.runResults = [ + // ls-remote reports a new SHA + ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), + // fetch succeeds + ShellResult(exitCode: 0, stdout: "", stderr: ""), + // diff reports README-only (noise) + ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: [entry]) + + #expect(updates.isEmpty, "Noise-only upstream commit must not produce a notification") + + // Registry baseline advanced to the new SHA + let registry = PackRegistryFile(path: env.packsRegistry) + let data = try registry.load() + let updated = registry.pack(identifier: "pack-a", in: data) + #expect(updated?.commitSHA == "new456") + } + + @Test("Material upstream commit → PackUpdate emitted, registry NOT advanced") + func materialEmitsUpdateAndPreservesBaseline() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let env = Environment(home: tmpDir) + let entry = makeRegistryEntry(identifier: "pack-a", commitSHA: "old123") + try writeRegistry(at: env, entries: [entry]) + + let mock = MockShellRunner(environment: env) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "hooks/session-start.sh\n", stderr: ""), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: [entry]) + + #expect(updates.count == 1) + #expect(updates.first?.identifier == "pack-a") + #expect(updates.first?.remoteSHA == "new456") + + // Registry preserved at the old SHA — `mcs pack update` is responsible for advancing it. + let registry = PackRegistryFile(path: env.packsRegistry) + let data = try registry.load() + #expect(registry.pack(identifier: "pack-a", in: data)?.commitSHA == "old123") + } + + @Test("fetch failure during classification → never-hide (PackUpdate emitted)") + func fetchFailureSurfaces() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let env = Environment(home: tmpDir) + let entry = makeRegistryEntry(identifier: "pack-a", commitSHA: "old123") + try writeRegistry(at: env, entries: [entry]) + + let mock = MockShellRunner(environment: env) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), + ShellResult(exitCode: 128, stdout: "", stderr: "fatal: unable to access"), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: [entry]) + + #expect(updates.count == 1, "fetch failure must never hide a real upstream change") + } + + @Test("ls-remote SHA matches local → no classifier invocation, no update") + func noChangeSkipsClassifier() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let env = Environment(home: tmpDir) + let entry = makeRegistryEntry(identifier: "pack-a", commitSHA: "same789") + try writeRegistry(at: env, entries: [entry]) + + let mock = MockShellRunner(environment: env) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "same789\tHEAD\n", stderr: ""), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: [entry]) + + #expect(updates.isEmpty) + #expect(mock.runCalls.count == 1) // only ls-remote; no fetch/diff + } +} + // MARK: - Parsing Tests struct UpdateCheckerParsingTests { diff --git a/docs/architecture.md b/docs/architecture.md index 509f1b6..fdc8319 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -109,6 +109,12 @@ Wraps `claude mcp add/remove` and `claude plugin install/remove` CLI commands. M - **`project`**: team-shared — stored in `.mcp.json` in the project directory - **`user`**: cross-project — stored in `~/.claude.json` globally +### UpdateChecker (`Core/UpdateChecker.swift`) + +Detects upstream pack and CLI updates via `git ls-remote`, with a **noise filter** so non-material upstream commits (README, LICENSE, CI, `.github/`, etc.) don't trigger spurious notifications. When the remote SHA differs from the registry baseline, the checker does a shallow `git fetch` + `git diff --name-only` in the local pack clone and classifies the changed-path list against a built-in deny-list. If every path is infrastructure, the notification is suppressed and the registry `commitSHA` advances so the same commits don't re-trigger. `techpack.yaml` is always treated as material — manifest edits can swap the install surface entirely, and silently suppressing those commits would be a supply-chain attack vector. Filter failures (offline, fetch error) fall through to surfacing the notification unfiltered — the filter can only suppress, never manufacture silence. + +Results are cached in `~/.mcs/update-check.json` with a 24-hour cooldown. The SessionStart hook serves cached results on every session start; only `mcs check-updates` (without `--hook`) forces a fresh network check. + ## External Pack System External packs are directories containing a `techpack.yaml` manifest — either Git repositories cloned into `~/.mcs/packs/` or local directories registered in-place. The system has these layers: diff --git a/docs/cli.md b/docs/cli.md index aca15b3..1a080b0 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -174,6 +174,9 @@ mcs check-updates --json # Machine-readable JSON output **How it works:** - **Pack checks**: Runs `git ls-remote` per pack to compare the remote HEAD against the local commit SHA. Local packs are skipped. +- **Noise filter**: When the remote SHA differs, runs a shallow `git fetch` + `git diff --name-only` in the local pack clone and classifies the changed-path list. If every path is deny-listed infrastructure (README, LICENSE, CHANGELOG, `.github/`, `node_modules/`, `.build/`, `Makefile`, `.gitignore`, etc.), the notification is suppressed and `~/.mcs/registry.yaml`'s `commitSHA` advances to the new remote SHA — the same infrastructure-only commits won't re-trigger on future cooldown windows. Any non-deny-listed path makes the update material and surfaces the notification. +- **`techpack.yaml` always surfaces**: manifest edits can change the install surface entirely (new hook scripts, swapped MCP server commands, different component actions), so the filter never suppresses a commit that touches the manifest — supply-chain safety takes precedence over noise reduction. +- **Never-hide on filter failure**: if `git fetch` or `git diff` errors (offline, access revoked, repo corrupt), the notification surfaces unfiltered. The filter can only suppress; it can't manufacture silence on error. - **CLI version check**: Queries `git ls-remote --tags` on the mcs repository and compares the latest CalVer tag against the installed version. - **Cache**: Results are cached in `~/.mcs/update-check.json` (timestamp + results). Cached results are served if less than 24 hours old (no network request). When the cache is stale, a fresh network check runs and the results are cached. `mcs check-updates` (without `--hook`) always forces a fresh check; `mcs sync` and `mcs doctor` respect the cache. - **Cache invalidation**: `mcs pack update` deletes the cache so the next hook re-checks. CLI version cache self-invalidates when the user upgrades mcs. From 3e5aa2cea784d177ee85811d65515481a1761e8b Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Sun, 26 Apr 2026 20:55:58 +0200 Subject: [PATCH 2/6] Address PR review: type-safety, parallel-write coverage, observability, doc-rot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Collapse PackCheckOutcome+SHAAdvance into a private enum (.emit / .advance) so the unreachable "both set" state can no longer produce a stuck-update bug; UpstreamChange.unknown gains an UnknownReason payload (missingClone/fetchFailed/diffFailed) for telemetry; classifyDiffPaths uses .filter; never-hide invariant comment now correctly scopes to classifyUpstreamChange (the earlier ls-remote/parse-error guard is documented as a separate "couldn't determine if there's a change at all" path). - MockShellRunner is now thread-safe (NSLock around runResults / runCalls / shellResults) and gains runResultsByFirstArg for argument-keyed dispatch — required for parallel tests where DispatchQueue.concurrentPerform interleaves calls non-deterministically; preparePackDir lifted to TestHelpers; new tests cover multi-pack parallel classification, custom entry.ref propagation, missingClone path, local-pack filter at checkPackUpdates boundary, registry-write failure contract (suppression sticks but baseline doesn't advance), and cross-invocation persistence; existing tests pin exact fetch/diff arguments and working-directory. - applyRegistryAdvances catch now emits to stderr under MCS_DEBUG so silent registry-write failures aren't invisible during development; trim issue-#338 references from doc-comments (UpstreamChange, checkPackUpdates, withCommitSHA, infrastructureFilesForUpdateCheck) and drop the codebase-convention narration on UpdateChecker.shell. --- Sources/mcs/Core/UpdateChecker.swift | 109 ++++---- Sources/mcs/ExternalPack/PackHeuristics.swift | 11 +- .../mcs/ExternalPack/PackRegistryFile.swift | 8 +- Tests/MCSTests/TestHelpers.swift | 34 +++ Tests/MCSTests/UpdateCheckerTests.swift | 248 +++++++++++++++--- 5 files changed, 325 insertions(+), 85 deletions(-) diff --git a/Sources/mcs/Core/UpdateChecker.swift b/Sources/mcs/Core/UpdateChecker.swift index 0047756..6d950ad 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -7,9 +7,6 @@ import Foundation /// remotes produce no output, matching the design goal of non-intrusive checks. struct UpdateChecker { let environment: Environment - /// `any ShellRunning` so tests can inject `MockShellRunner` for the new - /// post-SHA-change git fetch/diff calls. Production call sites pass a concrete - /// `ShellRunner`, which upcasts implicitly. `ShellRunning: Sendable`. let shell: any ShellRunning /// Default cooldown interval: 24 hours. @@ -172,15 +169,19 @@ struct UpdateChecker { // MARK: - Pack Checks /// Classification of an upstream commit-SHA change. Drives whether the noise filter - /// suppresses the update notification (issue #338). - /// - /// **Never-hide invariant:** `.suppressed` is the narrow special case. Every error - /// path (fetch fail, diff fail, missing clone, parse error) must fall through to - /// `.unknown`, which surfaces the notification as today. Do not invert this polarity. + /// suppresses the update notification. enum UpstreamChange: Equatable { case suppressed case material([String]) - case unknown + case unknown(UnknownReason) + + /// Why a classification could not be made. Carries enough context for telemetry + /// without requiring callers to inspect the orchestrator's call sites. + enum UnknownReason: Equatable { + case missingClone + case fetchFailed + case diffFailed + } } /// Pure classifier for the changed-path list returned by `git diff --name-only`. @@ -191,9 +192,14 @@ struct UpdateChecker { /// 1. If any path equals `techpack.yaml` → `.material` (supply-chain invariant — /// manifest edits can swap hook scripts, MCP commands, install surface). /// 2. A path is "noise" if its leading segment matches `PackHeuristics.ignoredDirectories` - /// (e.g. `.github/workflows/ci.yml`) OR its basename matches - /// `PackHeuristics.infrastructureFilesForUpdateCheck` (e.g. `README.md`, `LICENSE`). + /// (e.g. `.github/workflows/ci.yml`) OR the path is a single segment that matches + /// `PackHeuristics.infrastructureFilesForUpdateCheck` (e.g. `README.md` at the pack + /// root, but not `hooks/README.md`). /// 3. If all paths are noise → `.suppressed`. Any survivor → `.material(survivors)`. + /// + /// Empty input maps to `.suppressed`: `git diff --name-only` only produces empty output + /// after a successful diff that found no changed paths, which is a "definitely no + /// material change" signal — not an unknown one. static func classifyDiffPaths(_ paths: [String]) -> UpstreamChange { let cleaned = paths .map { $0.trimmingCharacters(in: .whitespaces) } @@ -204,10 +210,7 @@ struct UpdateChecker { return .material([Constants.ExternalPacks.manifestFilename]) } - var material: [String] = [] - for path in cleaned where !isNoisePath(path) { - material.append(path) - } + let material = cleaned.filter { !isNoisePath($0) } return material.isEmpty ? .suppressed : .material(material) } @@ -224,12 +227,19 @@ struct UpdateChecker { return false } - /// Orchestrator — runs `git fetch` + `git diff --name-only` in the pack clone - /// and feeds the diff into `classifyDiffPaths`. Every error path falls through - /// to `.unknown` per the never-hide invariant. + /// Orchestrator — runs `git fetch` + `git diff --name-only` in the pack clone and feeds + /// the diff into `classifyDiffPaths`. + /// + /// **Never-hide invariant (orchestrator scope):** within this function, every operational + /// failure (missing clone, fetch failure, diff failure) returns `.unknown(reason)` so the + /// caller can surface the notification unfiltered. `.suppressed` is reserved for diffs + /// successfully classified as all-noise. The earlier `git ls-remote` and SHA-parsing path + /// in `checkPackUpdates` is a separate guard — when we can't determine whether there is an + /// upstream change at all, the closure exits without producing an outcome (consistent with + /// the file-level "Network failures are silently ignored" design goal). func classifyUpstreamChange(entry: PackRegistryFile.PackEntry) -> UpstreamChange { guard let workDirURL = entry.resolvedPath(packsDirectory: environment.packsDirectory) else { - return .unknown + return .unknown(.missingClone) } let workDir = workDirURL.path let ref = entry.ref ?? "HEAD" @@ -240,7 +250,7 @@ struct UpdateChecker { workingDirectory: workDir, additionalEnvironment: Self.gitNoPromptEnv ) - guard fetchResult.succeeded else { return .unknown } + guard fetchResult.succeeded else { return .unknown(.fetchFailed) } let diffResult = shell.run( environment.gitPath, @@ -248,30 +258,27 @@ struct UpdateChecker { workingDirectory: workDir, additionalEnvironment: Self.gitNoPromptEnv ) - guard diffResult.succeeded else { return .unknown } + guard diffResult.succeeded else { return .unknown(.diffFailed) } let paths = diffResult.stdout.split(separator: "\n").map(String.init) return Self.classifyDiffPaths(paths) } - /// Per-iteration result of `checkPackUpdates`: an optional notification to emit - /// and an optional registry baseline advance (applied post-loop to serialize writes). - private struct PackCheckOutcome { - let update: PackUpdate? - let advance: SHAAdvance? - } - - private struct SHAAdvance { - let identifier: String - let newSHA: String + /// Per-iteration result of `checkPackUpdates`. Sum type — exactly one of: + /// surface a notification, OR advance the registry baseline. The unreachable + /// "both" state in the prior optional-pair encoding would have produced a + /// stuck-update bug (notify + silently advance), so the type rules it out. + private enum PackCheckOutcome { + case emit(PackUpdate) + case advance(identifier: String, newSHA: String) } /// Check each git pack for remote updates via `git ls-remote`. /// Local packs are skipped. Checks run in parallel. Network failures are silently ignored per-pack. /// - /// When a remote SHA differs, runs a noise filter (`classifyUpstreamChange`) that may + /// When a remote SHA differs, runs the noise filter (`classifyUpstreamChange`) which may /// suppress the notification for README/CI/infra-only commits and advance the registry - /// baseline so the same commits don't re-trigger (issue #338). + /// baseline so the same commits don't re-trigger. func checkPackUpdates(entries: [PackRegistryFile.PackEntry]) -> [PackUpdate] { let gitEntries = entries.filter { !$0.isLocalPack } guard !gitEntries.isEmpty else { return [] } @@ -305,28 +312,38 @@ struct UpdateChecker { switch classifyUpstreamChange(entry: entry) { case .suppressed: - results[index] = PackCheckOutcome( - update: nil, - advance: SHAAdvance(identifier: entry.identifier, newSHA: remoteSHA) - ) + results[index] = .advance(identifier: entry.identifier, newSHA: remoteSHA) case .material, .unknown: - // .unknown → never-hide: surface the notification unfiltered. - results[index] = PackCheckOutcome(update: pendingUpdate, advance: nil) + results[index] = .emit(pendingUpdate) } } - let advances = results.compactMap { $0?.advance } + var updates: [PackUpdate] = [] + var advances: [(identifier: String, newSHA: String)] = [] + for outcome in results { + switch outcome { + case nil: continue + case let .emit(update): updates.append(update) + case let .advance(identifier, newSHA): advances.append((identifier, newSHA)) + } + } if !advances.isEmpty { applyRegistryAdvances(advances) } - - return results.compactMap { $0?.update } + return updates } /// Apply collected SHA advances to `registry.yaml` in one load→mutate→save round-trip. /// Serializes the writes that were collected from the parallel `concurrentPerform` loop. - /// Silent on failure: next check will re-classify (same pattern as `saveCache`). - private func applyRegistryAdvances(_ advances: [SHAAdvance]) { + /// + /// Failures are non-fatal: the registry stays at the old commit SHA, so the next check + /// re-runs the classifier on the same paths and either suppresses again (and re-attempts + /// the write) or surfaces the notification. We deliberately avoid `output.warn` here + /// because this runs inside the SessionStart hook path where the file-level "non-intrusive + /// checks" design goal precludes user-facing noise. To keep the failure mode visible during + /// development, we emit to stderr only when the `MCS_DEBUG` env var is set — production + /// runs stay silent. + private func applyRegistryAdvances(_ advances: [(identifier: String, newSHA: String)]) { let registry = PackRegistryFile(path: environment.packsRegistry) do { var data = try registry.load() @@ -336,6 +353,10 @@ struct UpdateChecker { } try registry.save(data) } catch { + if ProcessInfo.processInfo.environment["MCS_DEBUG"] != nil { + let message = "mcs: registry advance write failed: \(error.localizedDescription)\n" + FileHandle.standardError.write(Data(message.utf8)) + } // Registry write failure is non-fatal — next check will classify again. } } diff --git a/Sources/mcs/ExternalPack/PackHeuristics.swift b/Sources/mcs/ExternalPack/PackHeuristics.swift index 905c265..a3d0561 100644 --- a/Sources/mcs/ExternalPack/PackHeuristics.swift +++ b/Sources/mcs/ExternalPack/PackHeuristics.swift @@ -79,8 +79,6 @@ enum PackHeuristics { } /// Directories at the pack root that are infrastructure, not pack content. - /// Shared by `checkUnreferencedFiles` and `UpdateChecker`'s noise filter — a path - /// whose leading segment is in this set is never material to a pack install. static let ignoredDirectories: Set = [ ".git", ".github", ".gitlab", ".vscode", "node_modules", "__pycache__", ".build", @@ -180,11 +178,10 @@ enum PackHeuristics { "Makefile", "Dockerfile", ".dockerignore", ] - /// Files treated as non-material by `UpdateChecker`'s noise filter (issue #338). - /// **Intentionally distinct from `infrastructureFiles`**: `techpack.yaml` is - /// excluded here because a manifest edit can change the entire install surface - /// (new hook scripts, different MCP commands). Silently suppressing manifest-only - /// commits would be a supply-chain attack vector. Do not deduplicate these sets. + /// Used by `UpdateChecker`'s noise filter. Excludes `techpack.yaml` because + /// manifest edits can swap the install surface (hooks, MCP commands) — silently + /// suppressing them would be a supply-chain attack vector. Do not deduplicate + /// these sets. static let infrastructureFilesForUpdateCheck: Set = infrastructureFiles.subtracting([Constants.ExternalPacks.manifestFilename]) diff --git a/Sources/mcs/ExternalPack/PackRegistryFile.swift b/Sources/mcs/ExternalPack/PackRegistryFile.swift index 0071480..9741f7c 100644 --- a/Sources/mcs/ExternalPack/PackRegistryFile.swift +++ b/Sources/mcs/ExternalPack/PackRegistryFile.swift @@ -32,9 +32,11 @@ struct PackRegistryFile { return PathContainment.safePath(relativePath: localPath, within: packsDirectory) } - /// Return a copy with `commitSHA` replaced. - /// Used when `UpdateChecker`'s noise filter advances the baseline without - /// updating the working-tree checkout (see issue #338). + /// Return a copy with `commitSHA` replaced. The registry SHA and working-tree + /// HEAD are normally kept in lockstep (set together by clone/checkout). Updating + /// only the SHA reflects "we acknowledged this upstream commit but did not move + /// the checkout" — used when a non-material upstream commit is recognized but + /// not pulled. func withCommitSHA(_ sha: String) -> Self { PackEntry( identifier: identifier, diff --git a/Tests/MCSTests/TestHelpers.swift b/Tests/MCSTests/TestHelpers.swift index 939bb02..e55a743 100644 --- a/Tests/MCSTests/TestHelpers.swift +++ b/Tests/MCSTests/TestHelpers.swift @@ -81,6 +81,12 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { let environment: Environment + /// Lock around all mutable state. The mock is `@unchecked Sendable` and is invoked + /// from `DispatchQueue.concurrentPerform` in `UpdateChecker.checkPackUpdates`, where + /// multiple threads race on `runResults.removeFirst()` and `runCalls.append(...)`. + /// The lock keeps both the queue and the call-recording array internally consistent. + private let lock = NSLock() + var runCalls: [RunCall] = [] var shellCalls: [ShellCall] = [] var commandExistsCalls: [String] = [] @@ -89,8 +95,18 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { var result = ShellResult(exitCode: 0, stdout: "", stderr: "") /// Sequential results for `run()`: pops first element when non-empty, falls back to `result`. + /// Positional ordering — only safe for tests where shell calls happen in a known order. + /// For parallel tests where `concurrentPerform` may interleave calls, use + /// `runResultsByFirstArg` instead (order-free, argument-keyed dispatch). var runResults: [ShellResult] = [] + /// Argument-keyed dispatch for `run()`. When `arguments.first` matches a key here, + /// the mapped result is returned without consuming from `runResults`. Designed for + /// tests that exercise `DispatchQueue.concurrentPerform` paths where positional + /// ordering is non-deterministic — every ls-remote / fetch / diff call returns the + /// same canned response regardless of which iteration emitted it. + var runResultsByFirstArg: [String: ShellResult] = [:] + /// Sequential results for `shell()`: pops first element when non-empty, falls back to `result`. var shellResults: [ShellResult] = [] @@ -102,6 +118,8 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { } func commandExists(_ command: String) -> Bool { + lock.lock() + defer { lock.unlock() } commandExistsCalls.append(command) return commandExistsResult } @@ -114,6 +132,8 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { additionalEnvironment: [String: String], interactive: Bool ) -> ShellResult { + lock.lock() + defer { lock.unlock() } runCalls.append(RunCall( executable: executable, arguments: arguments, @@ -121,6 +141,9 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { additionalEnvironment: additionalEnvironment, interactive: interactive )) + if let firstArg = arguments.first, let scripted = runResultsByFirstArg[firstArg] { + return scripted + } if !runResults.isEmpty { return runResults.removeFirst() } @@ -134,6 +157,8 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { additionalEnvironment: [String: String], interactive: Bool ) -> ShellResult { + lock.lock() + defer { lock.unlock() } shellCalls.append(ShellCall( command: command, workingDirectory: workingDirectory, @@ -305,6 +330,15 @@ func makeLocalRegistryEntry( ) } +/// Create the on-disk pack clone directory at `/.mcs/packs//` so +/// `PackEntry.resolvedPath(packsDirectory:)` succeeds and the path-containment check +/// inside it doesn't fail. The directory is empty — tests that mock the shell don't +/// need a real git checkout, only a valid working directory to pass to git invocations. +func preparePackDir(home: URL, identifier: String) throws { + let dir = home.appendingPathComponent(".mcs/packs/\(identifier)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) +} + // MARK: - Temp Directory Helpers /// Create a bare temp directory with a UUID-unique name. diff --git a/Tests/MCSTests/UpdateCheckerTests.swift b/Tests/MCSTests/UpdateCheckerTests.swift index 35ad62b..c9ecb1a 100644 --- a/Tests/MCSTests/UpdateCheckerTests.swift +++ b/Tests/MCSTests/UpdateCheckerTests.swift @@ -310,19 +310,28 @@ struct UpdateCheckerClassifyDiffPathsTests { } struct UpdateCheckerOrchestratorTests { - private func makeEntry(identifier: String = "pack-a", commitSHA: String = "old123") - -> PackRegistryFile.PackEntry { - makeRegistryEntry(identifier: identifier, commitSHA: commitSHA) + private func makeEntry( + identifier: String = "pack-a", + commitSHA: String = "old123", + ref: String? = nil, + localPath: String? = nil + ) -> PackRegistryFile.PackEntry { + PackRegistryFile.PackEntry( + identifier: identifier, + displayName: identifier, + author: nil, + sourceURL: "https://example.com/\(identifier).git", + ref: ref, + commitSHA: commitSHA, + localPath: localPath ?? identifier, + addedAt: "2026-01-01T00:00:00Z", + trustedScriptHashes: [:], + isLocal: nil + ) } private func makeChecker(home: URL, mock: MockShellRunner) -> UpdateChecker { - let env = Environment(home: home) - return UpdateChecker(environment: env, shell: mock) - } - - private func preparePackDir(home: URL, identifier: String) throws { - let dir = home.appendingPathComponent(".mcs/packs/\(identifier)") - try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + UpdateChecker(environment: Environment(home: home), shell: mock) } @Test("fetch + diff succeed with noise → suppressed") @@ -333,8 +342,8 @@ struct UpdateCheckerOrchestratorTests { let mock = MockShellRunner(environment: Environment(home: tmpDir)) mock.runResults = [ - ShellResult(exitCode: 0, stdout: "", stderr: ""), // fetch - ShellResult(exitCode: 0, stdout: "README.md\nLICENSE\n", stderr: ""), // diff + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "README.md\nLICENSE\n", stderr: ""), ] let checker = makeChecker(home: tmpDir, mock: mock) @@ -342,8 +351,12 @@ struct UpdateCheckerOrchestratorTests { #expect(result == .suppressed) #expect(mock.runCalls.count == 2) - #expect(mock.runCalls[0].arguments.contains("fetch")) - #expect(mock.runCalls[1].arguments.contains("diff")) + + let expectedWorkDir = tmpDir.appendingPathComponent(".mcs/packs/pack-a").path + #expect(mock.runCalls[0].arguments == ["fetch", "--depth", "1", "origin", "HEAD"]) + #expect(mock.runCalls[0].workingDirectory == expectedWorkDir) + #expect(mock.runCalls[1].arguments == ["diff", "--name-only", "HEAD", "FETCH_HEAD"]) + #expect(mock.runCalls[1].workingDirectory == expectedWorkDir) } @Test("fetch + diff succeed with material → material") @@ -364,7 +377,41 @@ struct UpdateCheckerOrchestratorTests { #expect(result == .material(["hooks/session-start.sh"])) } - @Test("fetch fails → unknown (never-hide, diff not called)") + @Test("Custom entry.ref propagates through fetch arguments") + func customRefPropagates() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + _ = checker.classifyUpstreamChange(entry: makeEntry(ref: "v2.0")) + + #expect(mock.runCalls[0].arguments == ["fetch", "--depth", "1", "origin", "v2.0"]) + } + + @Test("resolvedPath nil (containment escape) → .unknown(.missingClone)") + func missingCloneReturnsUnknown() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + // localPath that escapes packsDirectory → PathContainment.safePath returns nil. + let entry = makeEntry(identifier: "pack-a", localPath: "../escape") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: entry) + + #expect(result == .unknown(.missingClone)) + #expect(mock.runCalls.isEmpty, "No git invocation when the clone path can't be resolved") + } + + @Test("fetch fails → .unknown(.fetchFailed); diff is not called") func fetchFailsNeverHide() throws { let tmpDir = try makeTmpDir(label: "classify") defer { try? FileManager.default.removeItem(at: tmpDir) } @@ -378,11 +425,11 @@ struct UpdateCheckerOrchestratorTests { let checker = makeChecker(home: tmpDir, mock: mock) let result = checker.classifyUpstreamChange(entry: makeEntry()) - #expect(result == .unknown) - #expect(mock.runCalls.count == 1) // diff should not be called after fetch fails + #expect(result == .unknown(.fetchFailed)) + #expect(mock.runCalls.count == 1) } - @Test("diff fails → unknown (never-hide)") + @Test("diff fails → .unknown(.diffFailed)") func diffFailsNeverHide() throws { let tmpDir = try makeTmpDir(label: "classify") defer { try? FileManager.default.removeItem(at: tmpDir) } @@ -397,7 +444,7 @@ struct UpdateCheckerOrchestratorTests { let checker = makeChecker(home: tmpDir, mock: mock) let result = checker.classifyUpstreamChange(entry: makeEntry()) - #expect(result == .unknown) + #expect(result == .unknown(.diffFailed)) } @Test("git commands pass GIT_TERMINAL_PROMPT=0 to avoid credential prompts") @@ -431,11 +478,6 @@ struct UpdateCheckerPackUpdatesTests { try registry.save(data) } - private func preparePackDir(home: URL, identifier: String) throws { - let dir = home.appendingPathComponent(".mcs/packs/\(identifier)") - try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) - } - @Test("Noise-only upstream commit → no PackUpdate, registry commitSHA advances") func noiseSuppressedAndBaselineAdvances() throws { let tmpDir = try makeTmpDir(label: "checkPackUpdates") @@ -448,11 +490,8 @@ struct UpdateCheckerPackUpdatesTests { let mock = MockShellRunner(environment: env) mock.runResults = [ - // ls-remote reports a new SHA ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), - // fetch succeeds ShellResult(exitCode: 0, stdout: "", stderr: ""), - // diff reports README-only (noise) ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), ] @@ -461,11 +500,9 @@ struct UpdateCheckerPackUpdatesTests { #expect(updates.isEmpty, "Noise-only upstream commit must not produce a notification") - // Registry baseline advanced to the new SHA let registry = PackRegistryFile(path: env.packsRegistry) let data = try registry.load() - let updated = registry.pack(identifier: "pack-a", in: data) - #expect(updated?.commitSHA == "new456") + #expect(registry.pack(identifier: "pack-a", in: data)?.commitSHA == "new456") } @Test("Material upstream commit → PackUpdate emitted, registry NOT advanced") @@ -538,7 +575,156 @@ struct UpdateCheckerPackUpdatesTests { let updates = checker.checkPackUpdates(entries: [entry]) #expect(updates.isEmpty) - #expect(mock.runCalls.count == 1) // only ls-remote; no fetch/diff + #expect(mock.runCalls.count == 1) + } + + @Test("Local packs are filtered out before any git invocation") + func localPacksSkippedFromGitChecks() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "git-pack") + + let env = Environment(home: tmpDir) + let gitEntry = makeRegistryEntry(identifier: "git-pack", commitSHA: "old999") + let localEntry = makeLocalRegistryEntry(identifier: "local-pack", localPath: "/tmp/local-pack") + try writeRegistry(at: env, entries: [gitEntry, localEntry]) + + let mock = MockShellRunner(environment: env) + mock.runResultsByFirstArg = [ + "ls-remote": ShellResult(exitCode: 0, stdout: "old999\tHEAD\n", stderr: ""), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: [gitEntry, localEntry]) + + #expect(updates.isEmpty) + // Exactly one ls-remote call (for the git pack) — the local pack is filtered out. + #expect(mock.runCalls.count == 1) + #expect(mock.runCalls[0].arguments == ["ls-remote", gitEntry.sourceURL, "HEAD"]) + } + + @Test("Multi-pack parallel run: each pack classified independently with the right SHA write") + func multiPackParallelClassification() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + let identifiers = ["pack-a", "pack-b", "pack-c"] + for id in identifiers { + try preparePackDir(home: tmpDir, identifier: id) + } + + let env = Environment(home: tmpDir) + let entries = identifiers.map { id in + makeRegistryEntry(identifier: id, commitSHA: "old-\(id)") + } + try writeRegistry(at: env, entries: entries) + + // Argument-keyed dispatch — `concurrentPerform` order is non-deterministic, so positional + // queues would race. Each ls-remote / fetch / diff returns the same canned response + // regardless of which iteration emitted it. + let mock = MockShellRunner(environment: env) + mock.runResultsByFirstArg = [ + "ls-remote": ShellResult(exitCode: 0, stdout: "shared-new-sha\tHEAD\n", stderr: ""), + "fetch": ShellResult(exitCode: 0, stdout: "", stderr: ""), + "diff": ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: entries) + + #expect(updates.isEmpty, "All three packs noise-only → no notifications") + #expect(mock.runCalls.count == 9, "3 packs × (ls-remote + fetch + diff)") + + // Each pack's registry SHA was advanced independently in the post-loop write. + let registry = PackRegistryFile(path: env.packsRegistry) + let data = try registry.load() + for id in identifiers { + #expect(registry.pack(identifier: id, in: data)?.commitSHA == "shared-new-sha") + } + } + + @Test("Registry-write failure: suppression sticks for this run, baseline does not advance") + func registryWriteFailureContract() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + let mcsDir = tmpDir.appendingPathComponent(".mcs") + defer { + // Restore writability before cleanup — read-only parent prevents removal otherwise. + _ = try? FileManager.default.setAttributes( + [.posixPermissions: 0o755], ofItemAtPath: mcsDir.path + ) + try? FileManager.default.removeItem(at: tmpDir) + } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let env = Environment(home: tmpDir) + let entry = makeRegistryEntry(identifier: "pack-a", commitSHA: "old123") + try writeRegistry(at: env, entries: [entry]) + + // Lock down the `.mcs/` directory so atomic write-and-rename can't create a temp file. + // File-level chmod 0444 is bypassed because `YAMLFile.save` writes-then-renames. + try FileManager.default.setAttributes( + [.posixPermissions: 0o555], ofItemAtPath: mcsDir.path + ) + + let mock = MockShellRunner(environment: env) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), + ] + + let checker = UpdateChecker(environment: env, shell: mock) + let updates = checker.checkPackUpdates(entries: [entry]) + + // Suppression still happens this run — no notification is emitted. + #expect(updates.isEmpty, "Suppression is independent of the write outcome") + + // But the registry SHA did NOT advance, so the next check will re-classify. + try FileManager.default.setAttributes( + [.posixPermissions: 0o755], ofItemAtPath: mcsDir.path + ) + let registry = PackRegistryFile(path: env.packsRegistry) + let data = try registry.load() + #expect(registry.pack(identifier: "pack-a", in: data)?.commitSHA == "old123") + } + + @Test("Cross-invocation persistence: advance from run 1 is honored in run 2") + func crossInvocationPersistence() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let env = Environment(home: tmpDir) + let entry = makeRegistryEntry(identifier: "pack-a", commitSHA: "old123") + try writeRegistry(at: env, entries: [entry]) + + // Run 1: noise commit, suppress + advance to "new456". + let mock1 = MockShellRunner(environment: env) + mock1.runResults = [ + ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "README.md\n", stderr: ""), + ] + let updates1 = UpdateChecker(environment: env, shell: mock1) + .checkPackUpdates(entries: [entry]) + #expect(updates1.isEmpty) + + // Reload entry from registry to pick up the advanced SHA. + let registry = PackRegistryFile(path: env.packsRegistry) + let advancedEntry = try #require(registry.pack(identifier: "pack-a", in: registry.load())) + #expect(advancedEntry.commitSHA == "new456") + + // Run 2: ls-remote returns the same SHA — no upstream change relative to the advanced + // baseline → classifier is not invoked, no notification. + let mock2 = MockShellRunner(environment: env) + mock2.runResults = [ + ShellResult(exitCode: 0, stdout: "new456\tHEAD\n", stderr: ""), + ] + let updates2 = UpdateChecker(environment: env, shell: mock2) + .checkPackUpdates(entries: [advancedEntry]) + + #expect(updates2.isEmpty) + #expect(mock2.runCalls.count == 1, "Only ls-remote — no fetch/diff after baseline caught up") } } From eacb09269ceaac4ae724e798683dfdcd9758566a Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Mon, 27 Apr 2026 10:01:45 +0200 Subject: [PATCH 3/6] Address Copilot + simplify-pass findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Mirror PackFetcher.update's nil-ref pattern: when entry.ref == nil, fetch without a positional ref and diff against origin/HEAD instead of passing HEAD as a remote ref (more reliable across git servers, matches the rest of the codebase). - Validate entry.ref via the new module-level isValidGitRef predicate (extracted from PackFetcher.validateRef) before invoking ls-remote / fetch / diff — registry-corrupted refs starting with `-` would otherwise be interpreted as git options. Invalid refs silently skip the pack, consistent with ls-remote network-failure behavior. parseRemoteSHA now prefers the peeled `^{}` line for annotated tags so the registry SHA stays in sync with rev-parse HEAD. - Centralize the MCS_DEBUG env check on Environment.isDebugMode; route preparePackDir through Environment.packsDirectory; trim verbose MockShellRunner doc-comments; drop a duplicate inline comment. --- Sources/mcs/Core/Environment.swift | 8 +++ Sources/mcs/Core/UpdateChecker.swift | 70 +++++++++++++++---- Sources/mcs/ExternalPack/PackFetcher.swift | 18 +++-- Tests/MCSTests/TestHelpers.swift | 35 +++++----- Tests/MCSTests/UpdateCheckerTests.swift | 78 +++++++++++++++++++--- 5 files changed, 164 insertions(+), 45 deletions(-) diff --git a/Sources/mcs/Core/Environment.swift b/Sources/mcs/Core/Environment.swift index 1e73145..f1b12c3 100644 --- a/Sources/mcs/Core/Environment.swift +++ b/Sources/mcs/Core/Environment.swift @@ -72,6 +72,14 @@ struct Environment { mcsDirectory.appendingPathComponent(Constants.ExternalPacks.packsDirectory) } + /// Whether the `MCS_DEBUG` env var is set. Used to gate developer-facing diagnostic + /// emits (e.g. stderr writes for non-fatal failures that would otherwise be silent). + /// Production runs are unaffected; CI and developer shells can opt in by exporting + /// `MCS_DEBUG=1`. + static var isDebugMode: Bool { + ProcessInfo.processInfo.environment["MCS_DEBUG"] != nil + } + /// YAML registry of installed external packs (`~/.mcs/registry.yaml`). var packsRegistry: URL { mcsDirectory.appendingPathComponent(Constants.ExternalPacks.registryFilename) diff --git a/Sources/mcs/Core/UpdateChecker.swift b/Sources/mcs/Core/UpdateChecker.swift index 6d950ad..43e4012 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -237,16 +237,35 @@ struct UpdateChecker { /// in `checkPackUpdates` is a separate guard — when we can't determine whether there is an /// upstream change at all, the closure exits without producing an outcome (consistent with /// the file-level "Network failures are silently ignored" design goal). + /// + /// Mirrors `PackFetcher.update` for the fetch shape: when `entry.ref == nil`, fetch without + /// a ref arg and diff against `origin/HEAD` (more reliable across git servers than passing + /// `HEAD` as a positional ref). When `entry.ref` is set, fetch that ref explicitly and diff + /// against `FETCH_HEAD`. func classifyUpstreamChange(entry: PackRegistryFile.PackEntry) -> UpstreamChange { guard let workDirURL = entry.resolvedPath(packsDirectory: environment.packsDirectory) else { return .unknown(.missingClone) } let workDir = workDirURL.path - let ref = entry.ref ?? "HEAD" + + let fetchArgs: [String] + let diffTarget: String + if let ref = entry.ref { + // Refs are read from `registry.yaml`; if user-edited or corrupted, a ref starting + // with `-` would be interpreted as a git option (argument injection). Validate + // before invoking git; treat invalid refs as a fetch failure so the never-hide + // invariant still surfaces a notification. + guard isValidGitRef(ref) else { return .unknown(.fetchFailed) } + fetchArgs = ["fetch", "--depth", "1", "origin", ref] + diffTarget = "FETCH_HEAD" + } else { + fetchArgs = ["fetch", "--depth", "1", "origin"] + diffTarget = "origin/HEAD" + } let fetchResult = shell.run( environment.gitPath, - arguments: ["fetch", "--depth", "1", "origin", ref], + arguments: fetchArgs, workingDirectory: workDir, additionalEnvironment: Self.gitNoPromptEnv ) @@ -254,7 +273,7 @@ struct UpdateChecker { let diffResult = shell.run( environment.gitPath, - arguments: ["diff", "--name-only", "HEAD", "FETCH_HEAD"], + arguments: ["diff", "--name-only", "HEAD", diffTarget], workingDirectory: workDir, additionalEnvironment: Self.gitNoPromptEnv ) @@ -289,10 +308,13 @@ struct UpdateChecker { DispatchQueue.concurrentPerform(iterations: gitEntries.count) { index in let entry = gitEntries[index] - let ref = entry.ref ?? "HEAD" + // Refs are user-controllable via `mcs pack add --ref` and persisted in `registry.yaml`; + // a corrupted ref starting with `-` would be interpreted as a git option (argument + // injection). Skip the pack silently — same behavior as ls-remote network failures. + if let ref = entry.ref, !isValidGitRef(ref) { return } let lsRemote = shell.run( environment.gitPath, - arguments: ["ls-remote", entry.sourceURL, ref], + arguments: ["ls-remote", entry.sourceURL, entry.ref ?? "HEAD"], additionalEnvironment: Self.gitNoPromptEnv ) @@ -353,7 +375,7 @@ struct UpdateChecker { } try registry.save(data) } catch { - if ProcessInfo.processInfo.environment["MCS_DEBUG"] != nil { + if Environment.isDebugMode { let message = "mcs: registry advance write failed: \(error.localizedDescription)\n" FileHandle.standardError.write(Data(message.utf8)) } @@ -538,15 +560,39 @@ struct UpdateChecker { // MARK: - Parsing Helpers - /// Extract the SHA from the first line of `git ls-remote` output. - /// Format: `\t\n` + /// Extract the commit SHA from `git ls-remote` output. Format: `\t\n`. + /// + /// For annotated tags, ls-remote returns the tag-object SHA on the first line and the + /// peeled commit SHA on a second line whose ref ends with `^{}`. Prefer the peeled line + /// when present so the returned SHA matches what the working tree's `rev-parse HEAD` + /// would resolve to — writing a tag-object SHA into `registry.yaml` desyncs the registry + /// from the checkout and trips PackUpdater's "disk ahead of registry" recovery path. static func parseRemoteSHA(from output: String) -> String? { let trimmed = output.trimmingCharacters(in: .whitespacesAndNewlines) guard !trimmed.isEmpty else { return nil } - let firstLine = trimmed.split(separator: "\n", maxSplits: 1).first.map(String.init) ?? trimmed - let sha = firstLine.split(separator: "\t", maxSplits: 1).first.map(String.init) - guard let sha, !sha.isEmpty else { return nil } - return sha + + var firstSHA: String? + for line in trimmed.split(separator: "\n", omittingEmptySubsequences: true) { + let parts = line.split(separator: "\t", maxSplits: 1) + guard parts.count == 2 else { continue } + let sha = String(parts[0]) + let ref = parts[1] + guard !sha.isEmpty else { continue } + if ref.hasSuffix("^{}") { + return sha + } + if firstSHA == nil { + firstSHA = sha + } + } + + // Fallback for malformed single-line output that has no tab — preserve historical behavior. + if firstSHA == nil { + let firstLine = trimmed.split(separator: "\n", maxSplits: 1).first.map(String.init) ?? trimmed + let sha = firstLine.split(separator: "\t", maxSplits: 1).first.map(String.init) ?? "" + return sha.isEmpty ? nil : sha + } + return firstSHA } /// Find the latest CalVer tag from `git ls-remote --tags --refs` output. diff --git a/Sources/mcs/ExternalPack/PackFetcher.swift b/Sources/mcs/ExternalPack/PackFetcher.swift index 677027a..ceed4fb 100644 --- a/Sources/mcs/ExternalPack/PackFetcher.swift +++ b/Sources/mcs/ExternalPack/PackFetcher.swift @@ -155,10 +155,7 @@ struct PackFetcher { /// Validate that a git ref is safe for use as a command argument. func validateRef(_ ref: String) throws { - guard !ref.hasPrefix("-"), - !ref.contains(".."), - ref.range(of: #"^[a-zA-Z0-9._/+-]+$"#, options: .regularExpression) != nil - else { + guard isValidGitRef(ref) else { throw PackFetchError.invalidRef(ref) } } @@ -177,6 +174,19 @@ struct PackFetcher { } } +// MARK: - Ref validation + +/// Predicate form of `PackFetcher.validateRef`. Returns `true` iff `ref` is safe to pass +/// directly as a positional argument to git: not a `-` option, no `..` range, and limited +/// to characters git accepts in branch/tag/commit names. Used by callers that want to +/// silently skip invalid refs (e.g. `UpdateChecker`'s noise filter — registry corruption +/// shouldn't surface as a git error). +func isValidGitRef(_ ref: String) -> Bool { + !ref.hasPrefix("-") + && !ref.contains("..") + && ref.range(of: #"^[a-zA-Z0-9._/+-]+$"#, options: .regularExpression) != nil +} + // MARK: - Errors /// Errors that can occur during pack fetch operations. diff --git a/Tests/MCSTests/TestHelpers.swift b/Tests/MCSTests/TestHelpers.swift index e55a743..0c4beb2 100644 --- a/Tests/MCSTests/TestHelpers.swift +++ b/Tests/MCSTests/TestHelpers.swift @@ -81,33 +81,28 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { let environment: Environment - /// Lock around all mutable state. The mock is `@unchecked Sendable` and is invoked - /// from `DispatchQueue.concurrentPerform` in `UpdateChecker.checkPackUpdates`, where - /// multiple threads race on `runResults.removeFirst()` and `runCalls.append(...)`. - /// The lock keeps both the queue and the call-recording array internally consistent. + /// Mock is `@unchecked Sendable` and may be called concurrently from + /// `DispatchQueue.concurrentPerform`; the lock makes the queue and call arrays consistent. private let lock = NSLock() var runCalls: [RunCall] = [] var shellCalls: [ShellCall] = [] var commandExistsCalls: [String] = [] - /// Result returned from `run()` and `shell()`. Defaults to success. + /// Default result when neither queue below produces a value. + /// Dispatch precedence in `run()`: `runResultsByFirstArg[arguments.first]` → + /// `runResults.removeFirst()` → `result`. var result = ShellResult(exitCode: 0, stdout: "", stderr: "") - /// Sequential results for `run()`: pops first element when non-empty, falls back to `result`. - /// Positional ordering — only safe for tests where shell calls happen in a known order. - /// For parallel tests where `concurrentPerform` may interleave calls, use - /// `runResultsByFirstArg` instead (order-free, argument-keyed dispatch). + /// Positional queue for `run()`. Only safe for sequential call orders. var runResults: [ShellResult] = [] - /// Argument-keyed dispatch for `run()`. When `arguments.first` matches a key here, - /// the mapped result is returned without consuming from `runResults`. Designed for - /// tests that exercise `DispatchQueue.concurrentPerform` paths where positional - /// ordering is non-deterministic — every ls-remote / fetch / diff call returns the - /// same canned response regardless of which iteration emitted it. + /// Argument-keyed dispatch for `run()`, keyed on `arguments.first`. Use this for tests + /// where `concurrentPerform` interleaves calls — every matching call returns the same + /// canned response regardless of order. var runResultsByFirstArg: [String: ShellResult] = [:] - /// Sequential results for `shell()`: pops first element when non-empty, falls back to `result`. + /// Positional queue for `shell()`. Same precedence model as `runResults`. var shellResults: [ShellResult] = [] /// Controls what `commandExists()` returns. Defaults to `true`. @@ -330,12 +325,12 @@ func makeLocalRegistryEntry( ) } -/// Create the on-disk pack clone directory at `/.mcs/packs//` so -/// `PackEntry.resolvedPath(packsDirectory:)` succeeds and the path-containment check -/// inside it doesn't fail. The directory is empty — tests that mock the shell don't -/// need a real git checkout, only a valid working directory to pass to git invocations. +/// Create the on-disk pack clone directory under `Environment(home:).packsDirectory` so +/// `PackEntry.resolvedPath(packsDirectory:)` resolves and `PathContainment.safePath` allows +/// it. The directory is empty — tests that mock the shell don't need a real git checkout, +/// only a valid working directory to pass to git invocations. func preparePackDir(home: URL, identifier: String) throws { - let dir = home.appendingPathComponent(".mcs/packs/\(identifier)") + let dir = Environment(home: home).packsDirectory.appendingPathComponent(identifier) try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) } diff --git a/Tests/MCSTests/UpdateCheckerTests.swift b/Tests/MCSTests/UpdateCheckerTests.swift index c9ecb1a..1a87410 100644 --- a/Tests/MCSTests/UpdateCheckerTests.swift +++ b/Tests/MCSTests/UpdateCheckerTests.swift @@ -334,8 +334,8 @@ struct UpdateCheckerOrchestratorTests { UpdateChecker(environment: Environment(home: home), shell: mock) } - @Test("fetch + diff succeed with noise → suppressed") - func fetchDiffNoiseSuppressed() throws { + @Test("nil ref: fetch without ref arg, diff against origin/HEAD (mirrors PackFetcher.update)") + func nilRefUsesOriginHEAD() throws { let tmpDir = try makeTmpDir(label: "classify") defer { try? FileManager.default.removeItem(at: tmpDir) } try preparePackDir(home: tmpDir, identifier: "pack-a") @@ -352,10 +352,11 @@ struct UpdateCheckerOrchestratorTests { #expect(result == .suppressed) #expect(mock.runCalls.count == 2) - let expectedWorkDir = tmpDir.appendingPathComponent(".mcs/packs/pack-a").path - #expect(mock.runCalls[0].arguments == ["fetch", "--depth", "1", "origin", "HEAD"]) + let expectedWorkDir = Environment(home: tmpDir).packsDirectory + .appendingPathComponent("pack-a").path + #expect(mock.runCalls[0].arguments == ["fetch", "--depth", "1", "origin"]) #expect(mock.runCalls[0].workingDirectory == expectedWorkDir) - #expect(mock.runCalls[1].arguments == ["diff", "--name-only", "HEAD", "FETCH_HEAD"]) + #expect(mock.runCalls[1].arguments == ["diff", "--name-only", "HEAD", "origin/HEAD"]) #expect(mock.runCalls[1].workingDirectory == expectedWorkDir) } @@ -377,7 +378,7 @@ struct UpdateCheckerOrchestratorTests { #expect(result == .material(["hooks/session-start.sh"])) } - @Test("Custom entry.ref propagates through fetch arguments") + @Test("Custom entry.ref propagates through fetch + diff against FETCH_HEAD") func customRefPropagates() throws { let tmpDir = try makeTmpDir(label: "classify") defer { try? FileManager.default.removeItem(at: tmpDir) } @@ -393,6 +394,21 @@ struct UpdateCheckerOrchestratorTests { _ = checker.classifyUpstreamChange(entry: makeEntry(ref: "v2.0")) #expect(mock.runCalls[0].arguments == ["fetch", "--depth", "1", "origin", "v2.0"]) + #expect(mock.runCalls[1].arguments == ["diff", "--name-only", "HEAD", "FETCH_HEAD"]) + } + + @Test("Invalid entry.ref (argument injection) → .unknown(.fetchFailed); no git invocation") + func invalidRefRejected() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: makeEntry(ref: "-rf")) + + #expect(result == .unknown(.fetchFailed)) + #expect(mock.runCalls.isEmpty, "Refs starting with `-` must never reach git") } @Test("resolvedPath nil (containment escape) → .unknown(.missingClone)") @@ -578,6 +594,35 @@ struct UpdateCheckerPackUpdatesTests { #expect(mock.runCalls.count == 1) } + @Test("Invalid registry ref → pack silently skipped (no ls-remote, no notification)") + func invalidRegistryRefSkipped() throws { + let tmpDir = try makeTmpDir(label: "checkPackUpdates") + defer { try? FileManager.default.removeItem(at: tmpDir) } + try preparePackDir(home: tmpDir, identifier: "pack-a") + + let env = Environment(home: tmpDir) + // ref = "-rf" simulates registry corruption — would be argument injection if passed to git. + let entry = PackRegistryFile.PackEntry( + identifier: "pack-a", + displayName: "pack-a", + author: nil, + sourceURL: "https://example.com/pack-a.git", + ref: "-rf", + commitSHA: "old123", + localPath: "pack-a", + addedAt: "2026-01-01T00:00:00Z", + trustedScriptHashes: [:], + isLocal: nil + ) + try writeRegistry(at: env, entries: [entry]) + + let mock = MockShellRunner(environment: env) + let updates = UpdateChecker(environment: env, shell: mock).checkPackUpdates(entries: [entry]) + + #expect(updates.isEmpty) + #expect(mock.runCalls.isEmpty, "Invalid ref must never invoke git, even ls-remote") + } + @Test("Local packs are filtered out before any git invocation") func localPacksSkippedFromGitChecks() throws { let tmpDir = try makeTmpDir(label: "checkPackUpdates") @@ -619,9 +664,6 @@ struct UpdateCheckerPackUpdatesTests { } try writeRegistry(at: env, entries: entries) - // Argument-keyed dispatch — `concurrentPerform` order is non-deterministic, so positional - // queues would race. Each ls-remote / fetch / diff returns the same canned response - // regardless of which iteration emitted it. let mock = MockShellRunner(environment: env) mock.runResultsByFirstArg = [ "ls-remote": ShellResult(exitCode: 0, stdout: "shared-new-sha\tHEAD\n", stderr: ""), @@ -754,6 +796,24 @@ struct UpdateCheckerParsingTests { #expect(sha == "abc123") } + @Test("parseRemoteSHA prefers the peeled ^{} commit for annotated tags") + func parsePrefersPeeledTag() { + // Annotated tag: first line is the tag-object SHA, second is the peeled commit. + // The peeled commit is what `rev-parse HEAD` would resolve to after a checkout — + // writing the tag-object SHA into registry.yaml would desync the registry. + let output = """ + tagobj11111111\trefs/tags/v1.0 + commit22222222\trefs/tags/v1.0^{} + """ + #expect(UpdateChecker.parseRemoteSHA(from: output) == "commit22222222") + } + + @Test("parseRemoteSHA returns first SHA when no peeled line is present (lightweight tag / branch)") + func parseFirstSHAWhenNoPeeled() { + let output = "abc123def\trefs/heads/main" + #expect(UpdateChecker.parseRemoteSHA(from: output) == "abc123def") + } + @Test("parseLatestTag finds the highest CalVer tag") func parseLatestTagMultiple() { let output = """ From 407e087710860a73baf7e4a848d786f98dc7285c Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Mon, 27 Apr 2026 10:16:20 +0200 Subject: [PATCH 4/6] Address review-pr findings (lock, CRLF, ergonomics) - Wrap applyRegistryAdvances in withFileLock; lock contention skips silently and the next check retries (closes data-loss race vs. concurrent mcs pack add/update) - Trim diff paths with .whitespacesAndNewlines so trailing \r from core.autocrlf=true CRLF output doesn't bypass the deny-list; regression test added - Make PackEntry fields var so withCommitSHA copies-and-mutates instead of re-listing every stored property; surface invalid-ref skip via MCS_DEBUG; soften isValidGitRef doc-comment --- Sources/mcs/Core/UpdateChecker.swift | 50 ++++++++++++------- Sources/mcs/ExternalPack/PackFetcher.swift | 8 +-- .../mcs/ExternalPack/PackRegistryFile.swift | 36 ++++++------- Tests/MCSTests/UpdateCheckerTests.swift | 11 ++++ 4 files changed, 62 insertions(+), 43 deletions(-) diff --git a/Sources/mcs/Core/UpdateChecker.swift b/Sources/mcs/Core/UpdateChecker.swift index 43e4012..3c004db 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -201,8 +201,11 @@ struct UpdateChecker { /// after a successful diff that found no changed paths, which is a "definitely no /// material change" signal — not an unknown one. static func classifyDiffPaths(_ paths: [String]) -> UpstreamChange { + // `.whitespacesAndNewlines` (not `.whitespaces`) so a trailing `\r` from CRLF + // output (git with `core.autocrlf=true`) gets stripped — otherwise `README.md\r` + // would miss the deny-list and surface as material. let cleaned = paths - .map { $0.trimmingCharacters(in: .whitespaces) } + .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } .filter { !$0.isEmpty } guard !cleaned.isEmpty else { return .suppressed } @@ -310,8 +313,16 @@ struct UpdateChecker { let entry = gitEntries[index] // Refs are user-controllable via `mcs pack add --ref` and persisted in `registry.yaml`; // a corrupted ref starting with `-` would be interpreted as a git option (argument - // injection). Skip the pack silently — same behavior as ls-remote network failures. - if let ref = entry.ref, !isValidGitRef(ref) { return } + // injection). Skip the pack — registry corruption is permanent (unlike a transient + // ls-remote failure), so emit to MCS_DEBUG so the silent skip is at least visible + // during development. Production hooks stay quiet per the "non-intrusive" design. + if let ref = entry.ref, !isValidGitRef(ref) { + if Environment.isDebugMode { + let message = "mcs: skipping update check for '\(entry.identifier)': invalid ref '\(ref)'\n" + FileHandle.standardError.write(Data(message.utf8)) + } + return + } let lsRemote = shell.run( environment.gitPath, arguments: ["ls-remote", entry.sourceURL, entry.ref ?? "HEAD"], @@ -358,28 +369,33 @@ struct UpdateChecker { /// Apply collected SHA advances to `registry.yaml` in one load→mutate→save round-trip. /// Serializes the writes that were collected from the parallel `concurrentPerform` loop. /// - /// Failures are non-fatal: the registry stays at the old commit SHA, so the next check - /// re-runs the classifier on the same paths and either suppresses again (and re-attempts - /// the write) or surfaces the notification. We deliberately avoid `output.warn` here - /// because this runs inside the SessionStart hook path where the file-level "non-intrusive - /// checks" design goal precludes user-facing noise. To keep the failure mode visible during - /// development, we emit to stderr only when the `MCS_DEBUG` env var is set — production - /// runs stay silent. + /// Acquires `~/.mcs/lock` non-blocking before the load→mutate→save. `CheckUpdatesCommand` + /// runs as a SessionStart hook and is not itself a `LockedCommand`; without the lock, a + /// concurrent `mcs pack add/update` (which is locked) could read the registry between our + /// load and save and have its write clobbered. On lock contention we skip silently — the + /// next check re-classifies and retries. + /// + /// All other failures are non-fatal too: the registry stays at the old commit SHA, so the + /// next check re-runs the classifier and either suppresses again (re-attempting the write) + /// or surfaces the notification. We avoid `output.warn` here because this runs inside the + /// SessionStart hook path where the "non-intrusive checks" design goal precludes user-facing + /// noise; failures emit to stderr only when `MCS_DEBUG` is set. private func applyRegistryAdvances(_ advances: [(identifier: String, newSHA: String)]) { - let registry = PackRegistryFile(path: environment.packsRegistry) do { - var data = try registry.load() - for advance in advances { - guard let existing = registry.pack(identifier: advance.identifier, in: data) else { continue } - registry.register(existing.withCommitSHA(advance.newSHA), in: &data) + try withFileLock(at: environment.lockFile) { + let registry = PackRegistryFile(path: environment.packsRegistry) + var data = try registry.load() + for advance in advances { + guard let existing = registry.pack(identifier: advance.identifier, in: data) else { continue } + registry.register(existing.withCommitSHA(advance.newSHA), in: &data) + } + try registry.save(data) } - try registry.save(data) } catch { if Environment.isDebugMode { let message = "mcs: registry advance write failed: \(error.localizedDescription)\n" FileHandle.standardError.write(Data(message.utf8)) } - // Registry write failure is non-fatal — next check will classify again. } } diff --git a/Sources/mcs/ExternalPack/PackFetcher.swift b/Sources/mcs/ExternalPack/PackFetcher.swift index ceed4fb..217a649 100644 --- a/Sources/mcs/ExternalPack/PackFetcher.swift +++ b/Sources/mcs/ExternalPack/PackFetcher.swift @@ -177,10 +177,10 @@ struct PackFetcher { // MARK: - Ref validation /// Predicate form of `PackFetcher.validateRef`. Returns `true` iff `ref` is safe to pass -/// directly as a positional argument to git: not a `-` option, no `..` range, and limited -/// to characters git accepts in branch/tag/commit names. Used by callers that want to -/// silently skip invalid refs (e.g. `UpdateChecker`'s noise filter — registry corruption -/// shouldn't surface as a git error). +/// directly as a positional argument to git: not a `-` option, no `..` range, and matches +/// a conservative subset of characters that git accepts (rejects e.g. `@`, which git allows +/// outside `@{}`). Used by callers that want to silently skip invalid refs — registry +/// corruption shouldn't surface as a git error. func isValidGitRef(_ ref: String) -> Bool { !ref.hasPrefix("-") && !ref.contains("..") diff --git a/Sources/mcs/ExternalPack/PackRegistryFile.swift b/Sources/mcs/ExternalPack/PackRegistryFile.swift index 9741f7c..d7eadf5 100644 --- a/Sources/mcs/ExternalPack/PackRegistryFile.swift +++ b/Sources/mcs/ExternalPack/PackRegistryFile.swift @@ -5,16 +5,17 @@ struct PackRegistryFile { let path: URL // ~/.mcs/registry.yaml struct PackEntry: Codable, Equatable { - let identifier: String - let displayName: String - let author: String? - let sourceURL: String // Git clone URL or original local path - let ref: String? // Git tag/branch/commit - let commitSHA: String // Exact commit (git) or "local" (local packs) - let localPath: String // Relative to ~/.mcs/packs/ (git) or absolute path (local) - let addedAt: String // ISO 8601 date - let trustedScriptHashes: [String: String] // relativePath -> SHA-256 - let isLocal: Bool? // nil/false = git pack, true = local filesystem pack + // `var` only to enable copy-and-mutate via `withCommitSHA`; treat as immutable elsewhere. + var identifier: String + var displayName: String + var author: String? + var sourceURL: String // Git clone URL or original local path + var ref: String? // Git tag/branch/commit + var commitSHA: String // Exact commit (git) or "local" (local packs) + var localPath: String // Relative to ~/.mcs/packs/ (git) or absolute path (local) + var addedAt: String // ISO 8601 date + var trustedScriptHashes: [String: String] // relativePath -> SHA-256 + var isLocal: Bool? // nil/false = git pack, true = local filesystem pack /// Whether this pack is a local filesystem pack (not cloned via git). var isLocalPack: Bool { @@ -38,18 +39,9 @@ struct PackRegistryFile { /// the checkout" — used when a non-material upstream commit is recognized but /// not pulled. func withCommitSHA(_ sha: String) -> Self { - PackEntry( - identifier: identifier, - displayName: displayName, - author: author, - sourceURL: sourceURL, - ref: ref, - commitSHA: sha, - localPath: localPath, - addedAt: addedAt, - trustedScriptHashes: trustedScriptHashes, - isLocal: isLocal - ) + var copy = self + copy.commitSHA = sha + return copy } } diff --git a/Tests/MCSTests/UpdateCheckerTests.swift b/Tests/MCSTests/UpdateCheckerTests.swift index 1a87410..8d01223 100644 --- a/Tests/MCSTests/UpdateCheckerTests.swift +++ b/Tests/MCSTests/UpdateCheckerTests.swift @@ -307,6 +307,17 @@ struct UpdateCheckerClassifyDiffPathsTests { ]) #expect(result == .suppressed) } + + @Test("CRLF line endings: trailing `\\r` is stripped before deny-list match") + func crlfStrippedBeforeMatch() { + // git with `core.autocrlf=true` can emit `path\r\n`. After splitting on `\n`, + // each path keeps a trailing `\r`. Without `.whitespacesAndNewlines` trimming, + // `README.md\r` would miss the deny-list and surface as material. + let result = UpdateChecker.classifyDiffPaths([ + "README.md\r", ".github/workflows/ci.yml\r", + ]) + #expect(result == .suppressed) + } } struct UpdateCheckerOrchestratorTests { From f973c4c8a1c980183b7e51f8fc969d2edc97e358 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Mon, 27 Apr 2026 10:34:42 +0200 Subject: [PATCH 5/6] Classify deleted pack clones as .missingClone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolvedPath` only validates the path shape — if the user `rm -rf`'d the clone, current code spawns git with a non-existent cwd and returns `.fetchFailed`. Add a `fileExists` check so the case classifies as `.missingClone` (its intended meaning), avoiding the doomed subprocess. Notification still surfaces — both reasons map to `.emit` at the call site. --- Sources/mcs/Core/UpdateChecker.swift | 8 +++++++- Tests/MCSTests/UpdateCheckerTests.swift | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/Sources/mcs/Core/UpdateChecker.swift b/Sources/mcs/Core/UpdateChecker.swift index 3c004db..c1e6c7e 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -246,7 +246,13 @@ struct UpdateChecker { /// `HEAD` as a positional ref). When `entry.ref` is set, fetch that ref explicitly and diff /// against `FETCH_HEAD`. func classifyUpstreamChange(entry: PackRegistryFile.PackEntry) -> UpstreamChange { - guard let workDirURL = entry.resolvedPath(packsDirectory: environment.packsDirectory) else { + // `resolvedPath` only validates the path shape; it doesn't stat the filesystem. If the + // clone was deleted out from under us (e.g. user `rm -rf`'d `~/.mcs/packs/foo`), classify + // as `.missingClone` instead of letting git fail with a bogus cwd — same outcome at the + // call site (notification surfaces) but accurate telemetry and one fewer subprocess. + guard let workDirURL = entry.resolvedPath(packsDirectory: environment.packsDirectory), + FileManager.default.fileExists(atPath: workDirURL.path) + else { return .unknown(.missingClone) } let workDir = workDirURL.path diff --git a/Tests/MCSTests/UpdateCheckerTests.swift b/Tests/MCSTests/UpdateCheckerTests.swift index 8d01223..bb51b11 100644 --- a/Tests/MCSTests/UpdateCheckerTests.swift +++ b/Tests/MCSTests/UpdateCheckerTests.swift @@ -438,6 +438,23 @@ struct UpdateCheckerOrchestratorTests { #expect(mock.runCalls.isEmpty, "No git invocation when the clone path can't be resolved") } + @Test("Clone directory absent on disk → .unknown(.missingClone), no git invoked") + func deletedCloneReturnsMissingClone() throws { + let tmpDir = try makeTmpDir(label: "classify") + defer { try? FileManager.default.removeItem(at: tmpDir) } + + // Registry says the pack is at `~/.mcs/packs/pack-a/`, but the directory was deleted. + // `preparePackDir` is intentionally NOT called here. + let entry = makeEntry(identifier: "pack-a") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: entry) + + #expect(result == .unknown(.missingClone)) + #expect(mock.runCalls.isEmpty, "No git invocation when the clone is missing on disk") + } + @Test("fetch fails → .unknown(.fetchFailed); diff is not called") func fetchFailsNeverHide() throws { let tmpDir = try makeTmpDir(label: "classify") From 27327133115268293e51ead5460592110807d8eb Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Mon, 27 Apr 2026 10:38:00 +0200 Subject: [PATCH 6/6] Write to raw buffer pointer in concurrentPerform loop Switch the parallel writes to UnsafeMutableBufferPointer so each thread touches a single element address with no Array COW/refcount machinery in the hot path. nonisolated(unsafe) is still needed (UnsafeMutableBufferPointer is not Sendable) but the data-race surface area is now obviously a flat slab of memory. --- Sources/mcs/Core/UpdateChecker.swift | 82 +++++++++++++++------------- 1 file changed, 43 insertions(+), 39 deletions(-) diff --git a/Sources/mcs/Core/UpdateChecker.swift b/Sources/mcs/Core/UpdateChecker.swift index c1e6c7e..a29b9f5 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -311,49 +311,53 @@ struct UpdateChecker { let gitEntries = entries.filter { !$0.isLocalPack } guard !gitEntries.isEmpty else { return [] } - // Each index is written by exactly one iteration — no data race. - // nonisolated(unsafe) is needed because concurrentPerform's closure is @Sendable. - nonisolated(unsafe) var results = [PackCheckOutcome?](repeating: nil, count: gitEntries.count) - - DispatchQueue.concurrentPerform(iterations: gitEntries.count) { index in - let entry = gitEntries[index] - // Refs are user-controllable via `mcs pack add --ref` and persisted in `registry.yaml`; - // a corrupted ref starting with `-` would be interpreted as a git option (argument - // injection). Skip the pack — registry corruption is permanent (unlike a transient - // ls-remote failure), so emit to MCS_DEBUG so the silent skip is at least visible - // during development. Production hooks stay quiet per the "non-intrusive" design. - if let ref = entry.ref, !isValidGitRef(ref) { - if Environment.isDebugMode { - let message = "mcs: skipping update check for '\(entry.identifier)': invalid ref '\(ref)'\n" - FileHandle.standardError.write(Data(message.utf8)) + // Each index is written by exactly one iteration. We write through a raw buffer pointer + // rather than the `Array` subscript so the parallel writes touch only distinct element + // addresses — no Array COW header / refcount machinery in the hot path. The `nonisolated(unsafe)` + // capture is still needed because `UnsafeMutableBufferPointer` is not Sendable. + var results = [PackCheckOutcome?](repeating: nil, count: gitEntries.count) + results.withUnsafeMutableBufferPointer { buffer in + nonisolated(unsafe) let buf = buffer + DispatchQueue.concurrentPerform(iterations: gitEntries.count) { index in + let entry = gitEntries[index] + // Refs are user-controllable via `mcs pack add --ref` and persisted in `registry.yaml`; + // a corrupted ref starting with `-` would be interpreted as a git option (argument + // injection). Skip the pack — registry corruption is permanent (unlike a transient + // ls-remote failure), so emit to MCS_DEBUG so the silent skip is at least visible + // during development. Production hooks stay quiet per the "non-intrusive" design. + if let ref = entry.ref, !isValidGitRef(ref) { + if Environment.isDebugMode { + let message = "mcs: skipping update check for '\(entry.identifier)': invalid ref '\(ref)'\n" + FileHandle.standardError.write(Data(message.utf8)) + } + return } - return - } - let lsRemote = shell.run( - environment.gitPath, - arguments: ["ls-remote", entry.sourceURL, entry.ref ?? "HEAD"], - additionalEnvironment: Self.gitNoPromptEnv - ) + let lsRemote = shell.run( + environment.gitPath, + arguments: ["ls-remote", entry.sourceURL, entry.ref ?? "HEAD"], + additionalEnvironment: Self.gitNoPromptEnv + ) - guard lsRemote.succeeded, - let remoteSHA = Self.parseRemoteSHA(from: lsRemote.stdout), - remoteSHA != entry.commitSHA - else { - return - } + guard lsRemote.succeeded, + let remoteSHA = Self.parseRemoteSHA(from: lsRemote.stdout), + remoteSHA != entry.commitSHA + else { + return + } - let pendingUpdate = PackUpdate( - identifier: entry.identifier, - displayName: entry.displayName, - localSHA: entry.commitSHA, - remoteSHA: remoteSHA - ) + let pendingUpdate = PackUpdate( + identifier: entry.identifier, + displayName: entry.displayName, + localSHA: entry.commitSHA, + remoteSHA: remoteSHA + ) - switch classifyUpstreamChange(entry: entry) { - case .suppressed: - results[index] = .advance(identifier: entry.identifier, newSHA: remoteSHA) - case .material, .unknown: - results[index] = .emit(pendingUpdate) + switch classifyUpstreamChange(entry: entry) { + case .suppressed: + buf[index] = .advance(identifier: entry.identifier, newSHA: remoteSHA) + case .material, .unknown: + buf[index] = .emit(pendingUpdate) + } } }