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 ae974e1..a29b9f5 100644 --- a/Sources/mcs/Core/UpdateChecker.swift +++ b/Sources/mcs/Core/UpdateChecker.swift @@ -7,7 +7,7 @@ import Foundation /// remotes produce no output, matching the design goal of non-intrusive checks. struct UpdateChecker { let environment: Environment - let shell: ShellRunner + let shell: any ShellRunning /// Default cooldown interval: 24 hours. static let cooldownInterval: TimeInterval = 86400 @@ -168,42 +168,245 @@ struct UpdateChecker { // MARK: - Pack Checks + /// Classification of an upstream commit-SHA change. Drives whether the noise filter + /// suppresses the update notification. + enum UpstreamChange: Equatable { + case suppressed + case material([String]) + 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`. + /// 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 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 { + // `.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: .whitespacesAndNewlines) } + .filter { !$0.isEmpty } + guard !cleaned.isEmpty else { return .suppressed } + + if cleaned.contains(Constants.ExternalPacks.manifestFilename) { + return .material([Constants.ExternalPacks.manifestFilename]) + } + + let material = cleaned.filter { !isNoisePath($0) } + 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`. + /// + /// **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). + /// + /// 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 { + // `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 + + 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: fetchArgs, + workingDirectory: workDir, + additionalEnvironment: Self.gitNoPromptEnv + ) + guard fetchResult.succeeded else { return .unknown(.fetchFailed) } + + let diffResult = shell.run( + environment.gitPath, + arguments: ["diff", "--name-only", "HEAD", diffTarget], + workingDirectory: workDir, + additionalEnvironment: Self.gitNoPromptEnv + ) + 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`. 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 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. 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) - - DispatchQueue.concurrentPerform(iterations: gitEntries.count) { index in - let entry = gitEntries[index] - let ref = entry.ref ?? "HEAD" - let result = shell.run( - environment.gitPath, - arguments: ["ls-remote", entry.sourceURL, ref], - additionalEnvironment: Self.gitNoPromptEnv - ) + // 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 + } + let lsRemote = shell.run( + environment.gitPath, + arguments: ["ls-remote", entry.sourceURL, entry.ref ?? "HEAD"], + additionalEnvironment: Self.gitNoPromptEnv + ) - guard result.succeeded, - let remoteSHA = Self.parseRemoteSHA(from: result.stdout) - else { - return - } + guard lsRemote.succeeded, + let remoteSHA = Self.parseRemoteSHA(from: lsRemote.stdout), + remoteSHA != entry.commitSHA + else { + return + } - if remoteSHA != entry.commitSHA { - results[index] = PackUpdate( + let pendingUpdate = PackUpdate( identifier: entry.identifier, displayName: entry.displayName, localSHA: entry.commitSHA, remoteSHA: remoteSHA ) + + switch classifyUpstreamChange(entry: entry) { + case .suppressed: + buf[index] = .advance(identifier: entry.identifier, newSHA: remoteSHA) + case .material, .unknown: + buf[index] = .emit(pendingUpdate) + } } } - return results.compactMap(\.self) + 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 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. + /// + /// 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)]) { + do { + 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) + } + } catch { + if Environment.isDebugMode { + let message = "mcs: registry advance write failed: \(error.localizedDescription)\n" + FileHandle.standardError.write(Data(message.utf8)) + } + } } // MARK: - CLI Version Check @@ -383,15 +586,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..217a649 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 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("..") + && ref.range(of: #"^[a-zA-Z0-9._/+-]+$"#, options: .regularExpression) != nil +} + // MARK: - Errors /// Errors that can occur during pack fetch operations. diff --git a/Sources/mcs/ExternalPack/PackHeuristics.swift b/Sources/mcs/ExternalPack/PackHeuristics.swift index a4fd818..a3d0561 100644 --- a/Sources/mcs/ExternalPack/PackHeuristics.swift +++ b/Sources/mcs/ExternalPack/PackHeuristics.swift @@ -79,7 +79,7 @@ enum PackHeuristics { } /// Directories at the pack root that are infrastructure, not pack content. - private static let ignoredDirectories: Set = [ + static let ignoredDirectories: Set = [ ".git", ".github", ".gitlab", ".vscode", "node_modules", "__pycache__", ".build", ] @@ -168,6 +168,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 +178,13 @@ enum PackHeuristics { "Makefile", "Dockerfile", ".dockerignore", ] + /// 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]) + private static func checkRootLevelContentFiles( manifest: ExternalPackManifest, packPath: URL diff --git a/Sources/mcs/ExternalPack/PackRegistryFile.swift b/Sources/mcs/ExternalPack/PackRegistryFile.swift index cc8d97f..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 { @@ -31,6 +32,17 @@ struct PackRegistryFile { } return PathContainment.safePath(relativePath: localPath, within: packsDirectory) } + + /// 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 { + var copy = self + copy.commitSHA = sha + return copy + } } 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/TestHelpers.swift b/Tests/MCSTests/TestHelpers.swift index 939bb02..0c4beb2 100644 --- a/Tests/MCSTests/TestHelpers.swift +++ b/Tests/MCSTests/TestHelpers.swift @@ -81,17 +81,28 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { let environment: Environment + /// 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 queue for `run()`. Only safe for sequential call orders. var runResults: [ShellResult] = [] - /// Sequential results for `shell()`: pops first element when non-empty, falls back to `result`. + /// 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] = [:] + + /// Positional queue for `shell()`. Same precedence model as `runResults`. var shellResults: [ShellResult] = [] /// Controls what `commandExists()` returns. Defaults to `true`. @@ -102,6 +113,8 @@ final class MockShellRunner: ShellRunning, @unchecked Sendable { } func commandExists(_ command: String) -> Bool { + lock.lock() + defer { lock.unlock() } commandExistsCalls.append(command) return commandExistsResult } @@ -114,6 +127,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 +136,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 +152,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 +325,15 @@ func makeLocalRegistryEntry( ) } +/// 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 = Environment(home: home).packsDirectory.appendingPathComponent(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 202e99e..bb51b11 100644 --- a/Tests/MCSTests/UpdateCheckerTests.swift +++ b/Tests/MCSTests/UpdateCheckerTests.swift @@ -237,6 +237,567 @@ 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) + } + + @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 { + 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 { + UpdateChecker(environment: Environment(home: home), shell: mock) + } + + @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") + + let mock = MockShellRunner(environment: Environment(home: tmpDir)) + mock.runResults = [ + ShellResult(exitCode: 0, stdout: "", stderr: ""), + ShellResult(exitCode: 0, stdout: "README.md\nLICENSE\n", stderr: ""), + ] + + let checker = makeChecker(home: tmpDir, mock: mock) + let result = checker.classifyUpstreamChange(entry: makeEntry()) + + #expect(result == .suppressed) + #expect(mock.runCalls.count == 2) + + 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", "origin/HEAD"]) + #expect(mock.runCalls[1].workingDirectory == expectedWorkDir) + } + + @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("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) } + 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"]) + #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)") + 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("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") + 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(.fetchFailed)) + #expect(mock.runCalls.count == 1) + } + + @Test("diff fails → .unknown(.diffFailed)") + 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(.diffFailed)) + } + + @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) + } + + @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 = [ + 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]) + + #expect(updates.isEmpty, "Noise-only upstream commit must not produce a notification") + + let registry = PackRegistryFile(path: env.packsRegistry) + let data = try registry.load() + #expect(registry.pack(identifier: "pack-a", in: data)?.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) + } + + @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") + 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) + + 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") + } +} + // MARK: - Parsing Tests struct UpdateCheckerParsingTests { @@ -263,6 +824,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 = """ 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.