From 4293d680f2ebf88ec0eafa96e33f6505ad864976 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 31 Jul 2026 00:19:35 +0200 Subject: [PATCH 1/3] ISSUE-70: Show component-level diff when mcs pack update updates a pack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Snapshot the pack before the fetch resets the working tree — --depth 1 clones make the previous commit unreadable once reset --hard lands - Diff every manifest surface by declaration and by referenced-file content, so a script-body edit is reported even when the YAML is untouched - Move referencedPaths onto the manifest so the diff and ignore: validation share one definition of what a pack references --- Sources/mcs/Commands/PackCommand.swift | 5 +- Sources/mcs/Commands/UpdateCommand.swift | 5 +- .../ExternalPack/ExternalPackManifest.swift | 94 +++- Sources/mcs/ExternalPack/PackHeuristics.swift | 54 +-- Sources/mcs/Sync/PackDiff.swift | 261 +++++++++++ Sources/mcs/Sync/PackUpdater.swift | 52 ++- Sources/mcs/TechPack/PromptDefinition.swift | 2 +- .../MCSTests/ExternalPackManifestTests.swift | 30 ++ Tests/MCSTests/PackDiffTests.swift | 411 ++++++++++++++++++ Tests/MCSTests/PackUpdaterTests.swift | 228 +++++++++- 10 files changed, 1058 insertions(+), 84 deletions(-) create mode 100644 Sources/mcs/Sync/PackDiff.swift create mode 100644 Tests/MCSTests/PackDiffTests.swift diff --git a/Sources/mcs/Commands/PackCommand.swift b/Sources/mcs/Commands/PackCommand.swift index fdfd3f6f..0c276cd7 100644 --- a/Sources/mcs/Commands/PackCommand.swift +++ b/Sources/mcs/Commands/PackCommand.swift @@ -678,10 +678,13 @@ struct UpdatePack: LockedCommand { switch result { case .alreadyUpToDate: ctx.output.success("\(entry.displayName): already up to date") - case let .updated(updatedEntry): + case let .updated(updatedEntry, diff): ctx.registry.register(updatedEntry, in: &updatedData) updatedCount += 1 ctx.output.success("\(entry.displayName): updated (\(updatedEntry.shortSHA))") + if let diff, !diff.isEmpty { + ctx.output.plain(diff.render(style: ctx.output.style)) + } case .trustDeclined: ctx.output.info("\(entry.identifier): \(result.reason ?? "trust not granted") (will re-prompt next run)") case .fetchFailed, .manifestInvalid, .internalError: diff --git a/Sources/mcs/Commands/UpdateCommand.swift b/Sources/mcs/Commands/UpdateCommand.swift index 43a3e2f7..99601f4c 100644 --- a/Sources/mcs/Commands/UpdateCommand.swift +++ b/Sources/mcs/Commands/UpdateCommand.swift @@ -276,10 +276,13 @@ struct UpdateCommand: LockedCommand { switch result { case .alreadyUpToDate: output.dimmed(" \(entry.displayName): already up to date") - case let .updated(updatedEntry): + case let .updated(updatedEntry, diff): registryFile.register(updatedEntry, in: &updatedData) anyUpdated = true output.success(" \(entry.displayName): \(entry.shortSHA) → \(updatedEntry.shortSHA)") + if let diff, !diff.isEmpty { + output.plain(diff.render(style: output.style, indent: " ")) + } case .trustDeclined: output.info(" \(entry.identifier): \(result.reason ?? "trust not granted") (will re-prompt on next 'mcs update')") skipped.insert(entry.identifier) diff --git a/Sources/mcs/ExternalPack/ExternalPackManifest.swift b/Sources/mcs/ExternalPack/ExternalPackManifest.swift index 20d09093..7165c21e 100644 --- a/Sources/mcs/ExternalPack/ExternalPackManifest.swift +++ b/Sources/mcs/ExternalPack/ExternalPackManifest.swift @@ -233,7 +233,7 @@ extension ExternalPackManifest { private func validateIgnoreEntries() throws { guard let ignore, !ignore.isEmpty else { return } - let referenced = PackHeuristics.referencedPaths(from: self) + let referenced = referencedPaths for entry in ignore { if let rejection = Self.classifyIgnoreEntry(entry, referenced: referenced) { throw ManifestError.ignoreEntryLoadBearing(entry: entry, reason: rejection.reason) @@ -304,7 +304,7 @@ extension ExternalPackManifest { rejected: (_ entry: String, _ rejection: IgnoreEntryRejection) -> Void ) -> ExternalPackManifest { guard let ignore, !ignore.isEmpty else { return self } - let referenced = PackHeuristics.referencedPaths(from: self) + let referenced = referencedPaths var kept: [String] = [] for entry in ignore { if let rejection = Self.classifyIgnoreEntry(entry, referenced: referenced) { @@ -462,7 +462,7 @@ enum ManifestError: Error, Equatable, LocalizedError { /// /// Shorthand keys: `brew`, `mcp`, `plugin`, `shell`, `hook`, `command`, /// `skill`, `settingsFile`, `gitignore`. See `ShorthandKeys` for details. -struct ExternalComponentDefinition: Codable { +struct ExternalComponentDefinition: Codable, Equatable { var id: String let displayName: String let description: String @@ -734,7 +734,7 @@ enum ExternalInstallActionType: String, Codable { } /// Declarative install action types that can be expressed in YAML. -enum ExternalInstallAction: Codable { +enum ExternalInstallAction: Codable, Equatable { case mcpServer(ExternalMCPServerConfig) case plugin(name: String) case brewInstall(package: String) @@ -830,7 +830,7 @@ enum ExternalInstallAction: Codable { // MARK: - MCP Server Config /// Configuration for an MCP server declared in an external pack manifest. -struct ExternalMCPServerConfig: Codable { +struct ExternalMCPServerConfig: Codable, Equatable { let name: String let command: String? let args: [String]? @@ -868,7 +868,7 @@ enum ExternalScope: String, Codable { // MARK: - Copy Pack File Config /// Configuration for copying a file from the pack into the Claude directory. -struct ExternalCopyPackFileConfig: Codable { +struct ExternalCopyPackFileConfig: Codable, Equatable { let source: String let destination: String let fileType: ExternalCopyFileType? @@ -882,10 +882,86 @@ enum ExternalCopyFileType: String, Codable { case generic } +// MARK: - Referenced Pack Files + +/// Normalize a pack-relative path so equivalent expressions collapse to the same key. +/// Trims whitespace and strips a leading `./` so `hooks/foo.sh`, `./hooks/foo.sh`, and +/// ` hooks/foo.sh` are one path. +/// +/// Private so the only way to obtain a pack-relative path is through the `referencedSource` of +/// the declaration that owns it — `ignore:` validation, unreferenced-file linting, and +/// `PackDiff`'s content hashing all key on these strings, and a consumer that normalized +/// differently would silently drop a file from one side of a comparison. +private func normalizedPackPath(_ path: String) -> String { + var normalized = path.trimmingCharacters(in: .whitespacesAndNewlines) + if normalized.hasPrefix("./") { normalized = String(normalized.dropFirst(2)) } + return normalized +} + +extension ExternalComponentDefinition { + /// The pack-relative file this component installs, normalized, or `nil` when the + /// component's install action copies nothing (brew, MCP, plugin, shell, gitignore). + var referencedSource: String? { + switch installAction { + case let .copyPackFile(config): + normalizedPackPath(config.source) + case let .settingsFile(source): + normalizedPackPath(source) + case .brewInstall, .gitignoreEntries, .mcpServer, .plugin, .settingsMerge, .shellCommand: + nil + } + } + + /// Whether `source` copies the pack root wholesale (`.`), which would sweep in + /// `techpack.yaml`, `LICENSE`, and `README`. + var copiesPackRoot: Bool { + guard case let .copyPackFile(config) = installAction else { return false } + let normalized = normalizedPackPath(config.source) + return normalized.isEmpty || normalized == "." + } +} + +extension ExternalTemplateDefinition { + /// The pack-relative content file backing this template section, normalized. + var referencedSource: String { + normalizedPackPath(contentFile) + } +} + +extension ExternalConfigureProject { + /// The pack-relative configure script, normalized. + var referencedSource: String { + normalizedPackPath(script) + } +} + +extension ExternalPackManifest { + /// Every pack-relative path this manifest declares, normalized. + /// + /// Lives on the manifest because it is a property of what the pack declares, not a quality + /// heuristic: `validate()` rejects `ignore:` entries that would mask one of these, + /// `PackHeuristics` reports the files that are *not* here, and `PackSnapshot` hashes them. + var referencedPaths: Set { + var paths = Set() + for component in components ?? [] { + if let source = component.referencedSource { + paths.insert(source) + } + } + for template in templates ?? [] { + paths.insert(template.referencedSource) + } + if let configureProject { + paths.insert(configureProject.referencedSource) + } + return paths + } +} + // MARK: - Templates /// A template contribution declared in an external pack manifest. -struct ExternalTemplateDefinition: Codable { +struct ExternalTemplateDefinition: Codable, Equatable { var sectionIdentifier: String let placeholders: [String]? let contentFile: String @@ -895,14 +971,14 @@ struct ExternalTemplateDefinition: Codable { // MARK: - Configure Project /// Script-based project configuration hook. -struct ExternalConfigureProject: Codable { +struct ExternalConfigureProject: Codable, Equatable { let script: String } // MARK: - Doctor Checks /// A declarative doctor check definition for external packs. -struct ExternalDoctorCheckDefinition: Codable { +struct ExternalDoctorCheckDefinition: Codable, Equatable { let type: ExternalDoctorCheckType let name: String let section: String? diff --git a/Sources/mcs/ExternalPack/PackHeuristics.swift b/Sources/mcs/ExternalPack/PackHeuristics.swift index 4ac55b66..dc10a2d5 100644 --- a/Sources/mcs/ExternalPack/PackHeuristics.swift +++ b/Sources/mcs/ExternalPack/PackHeuristics.swift @@ -58,18 +58,10 @@ enum PackHeuristics { components: [ExternalComponentDefinition] ) -> [Finding] { var findings: [Finding] = [] - for component in components { - if case let .copyPackFile(config) = component.installAction { - var normalized = config.source.trimmingCharacters(in: .whitespaces) - if normalized.hasPrefix("./") { - normalized = String(normalized.dropFirst(2)) - } - if normalized.isEmpty || normalized == "." { - let msg = "Component '\(component.id)' uses source '.' which copies the entire pack root" - + " (including techpack.yaml, LICENSE, README)" - findings.append(Finding(severity: .error, message: msg)) - } - } + for component in components where component.copiesPackRoot { + let msg = "Component '\(component.id)' uses source '.' which copies the entire pack root" + + " (including techpack.yaml, LICENSE, README)" + findings.append(Finding(severity: .error, message: msg)) } return findings } @@ -100,46 +92,12 @@ enum PackHeuristics { "node_modules", "__pycache__", ".build", ] - /// Paths the manifest relies on for its install surface. - /// Used by `checkUnreferencedFiles`, by `ExternalPackManifest.validate()` to reject - /// load-bearing entries in the `ignore:` list (issue #338), and by the runtime safety - /// guard in `ExternalPackLoader` that strips forbidden `ignore:` entries defensively. - static func referencedPaths(from manifest: ExternalPackManifest) -> Set { - var paths = Set() - for component in manifest.components ?? [] { - switch component.installAction { - case let .copyPackFile(config): - paths.insert(normalizeReferencedPath(config.source)) - case let .settingsFile(source): - paths.insert(normalizeReferencedPath(source)) - default: - break - } - } - for template in manifest.templates ?? [] { - paths.insert(normalizeReferencedPath(template.contentFile)) - } - if let script = manifest.configureProject?.script { - paths.insert(normalizeReferencedPath(script)) - } - return paths - } - - /// Normalize a referenced path so equivalent expressions collapse to the same key. - /// Trims whitespace and strips a leading `./` so `ignore:` validation doesn't miss - /// the same file expressed as `hooks/foo.sh` vs `./hooks/foo.sh` vs ` hooks/foo.sh`. - private static func normalizeReferencedPath(_ path: String) -> String { - var normalized = path.trimmingCharacters(in: .whitespacesAndNewlines) - if normalized.hasPrefix("./") { normalized = String(normalized.dropFirst(2)) } - return normalized - } - private static func checkUnreferencedFiles( manifest: ExternalPackManifest, packPath: URL ) -> [Finding] { let fm = FileManager.default - let referencedPaths = referencedPaths(from: manifest) + let referencedPaths = manifest.referencedPaths let rootContents: [URL] do { @@ -232,7 +190,7 @@ enum PackHeuristics { packPath: URL ) -> [Finding] { let fm = FileManager.default - let referencedRootFiles = referencedPaths(from: manifest).filter { !$0.contains("/") } + let referencedRootFiles = manifest.referencedPaths.filter { !$0.contains("/") } let rootContents: [URL] do { diff --git a/Sources/mcs/Sync/PackDiff.swift b/Sources/mcs/Sync/PackDiff.swift new file mode 100644 index 00000000..3b59322b --- /dev/null +++ b/Sources/mcs/Sync/PackDiff.swift @@ -0,0 +1,261 @@ +import Foundation + +// MARK: - Snapshot + +/// A pack's declared surface plus the content hashes of the files it references, captured at a +/// single point in time. +/// +/// Exists because `PackFetcher` clones and fetches with `--depth 1`: once `update()` runs +/// `reset --hard`, the previous commit is unreachable and survives only until git gc prunes it. +/// Reading the old manifest back out of the object store (`git show :techpack.yaml`) +/// therefore works in testing and fails silently on long-lived installs. Capturing the working +/// tree *before* the reset sidesteps the object store entirely — at that moment the old manifest +/// and its files are ordinary files on disk. +struct PackSnapshot { + let manifest: ExternalPackManifest + /// Pack-relative path → SHA-256. Keys are normalized via `normalizedPackPath`. + /// A referenced path that is absent or unreadable is simply missing from the map, which + /// compares as a difference against a revision where it exists. + let fileHashes: [String: String] + + /// Read the manifest and hash every file it references. + /// + /// - Parameter loader: must be called with the same arguments on both sides of a diff. + /// `validate(at:sanitizeOutput:)` only strips `ignore:` entries when `sanitizeOutput` is + /// passed, so an asymmetric call would manufacture differences that do not exist. + static func capture(packPath: URL, loader: ExternalPackLoader) throws -> PackSnapshot { + let manifest = try loader.validate(at: packPath) + var hashes: [String: String] = [:] + for path in manifest.referencedPaths { + let fileURL = packPath.appendingPathComponent(path) + do { + hashes[path] = try FileHasher.sha256(of: fileURL) + } catch { + // Absent or unreadable: leave the key out. Its absence on one side and presence + // on the other is exactly the "changed" signal we want, and a manifest that + // references a missing file is a pack-validation problem, not a diff problem. + continue + } + } + return PackSnapshot(manifest: manifest, fileHashes: hashes) + } +} + +// MARK: - Diff + +/// What changed in a pack between two revisions, at the granularity a pack author declares: +/// components, template sections, and supplementary doctor checks. +struct PackDiff: Equatable { + /// One case per author-visible surface of `ExternalPackManifest`. Adding a surface to the + /// manifest means adding a case here and a `diffKeyed` call in `between` — otherwise the + /// new surface changes silently, which is the bug this whole type exists to remove. + enum Kind { + case component + case template + case prompt + case doctorCheck + case configureScript + + /// Label used in rendered output. + var label: String { + switch self { + case .component: "component" + case .template: "template" + case .prompt: "prompt" + case .doctorCheck: "doctor check" + case .configureScript: "configure script" + } + } + } + + enum Change: Equatable { + case added + case removed + /// `path` names the referenced file whose content changed, or is `nil` when the YAML + /// declaration itself changed. The two are distinguished because they tell the user + /// different things: a changed script is new behavior, a changed declaration is new wiring. + case modified(path: String?) + + /// Render order: additions, then modifications, then removals. + var order: Int { + switch self { + case .added: 0 + case .modified: 1 + case .removed: 2 + } + } + + var marker: String { + switch self { + case .added: "+" + case .modified: "~" + case .removed: "-" + } + } + + func color(style: ANSIStyle) -> String { + switch self { + case .added: style.green + case .modified: style.yellow + case .removed: style.red + } + } + } + + struct Entry: Equatable { + let kind: Kind + let name: String + let change: Change + } + + let entries: [Entry] + + var isEmpty: Bool { + entries.isEmpty + } + + // MARK: Construction + + /// Compare two snapshots of the same pack. + /// + /// Components key on `id`, templates on `sectionIdentifier`, prompts on `key`, doctor checks + /// on `name` — all post-`normalized()`, so ids read as `ios.lint-hook` rather than + /// `lint-hook`. Per-component `doctorChecks` are part of a component's own equality, so the + /// `doctorCheck` kind covers only the manifest-level `supplementaryDoctorChecks`. + /// + /// `configureProject` is a scalar, but keying the 0-or-1 element on its script path lets it + /// use the same mechanism as everything else: repointing the script at a different file + /// reads as a removal plus an addition, which is what actually happened, and an edit to the + /// script body is the same key with a different content hash. + static func between(old: PackSnapshot, new: PackSnapshot) -> PackDiff { + var entries: [Entry] = [] + + entries += diffKeyed( + kind: .component, + old: old.manifest.components ?? [], + new: new.manifest.components ?? [], + key: \.id, + changedPath: { oldComponent, newComponent in + changedPath( + newComponent.referencedSource ?? oldComponent.referencedSource, + old: old, new: new + ) + } + ) + + entries += diffKeyed( + kind: .template, + old: old.manifest.templates ?? [], + new: new.manifest.templates ?? [], + key: \.sectionIdentifier, + changedPath: { _, newTemplate in + changedPath(newTemplate.referencedSource, old: old, new: new) + } + ) + + entries += diffKeyed( + kind: .prompt, + old: old.manifest.prompts ?? [], + new: new.manifest.prompts ?? [], + key: \.key + ) + + entries += diffKeyed( + kind: .doctorCheck, + old: old.manifest.supplementaryDoctorChecks ?? [], + new: new.manifest.supplementaryDoctorChecks ?? [], + key: \.name + ) + + entries += diffKeyed( + kind: .configureScript, + old: old.manifest.configureProject.map { [$0] } ?? [], + new: new.manifest.configureProject.map { [$0] } ?? [], + key: \.referencedSource, + changedPath: { _, newScript in + changedPath(newScript.referencedSource, old: old, new: new) + } + ) + + return PackDiff(entries: entries) + } + + /// Three-way partition over a keyed collection. `changedPath` is consulted only for items + /// present on both sides, and only decides *why* something is modified — an item whose + /// declaration differs is modified regardless of what its files did. It defaults to "this + /// kind references no files", which is the doctor-check case. + private static func diffKeyed( + kind: Kind, + old: [T], + new: [T], + key: KeyPath, + changedPath: (T, T) -> String? = { _, _ in nil } + ) -> [Entry] { + let oldByKey = Dictionary(old.map { ($0[keyPath: key], $0) }, uniquingKeysWith: { first, _ in first }) + let newByKey = Dictionary(new.map { ($0[keyPath: key], $0) }, uniquingKeysWith: { first, _ in first }) + let oldKeys = Set(oldByKey.keys) + let newKeys = Set(newByKey.keys) + + var entries: [Entry] = [] + for name in newKeys.subtracting(oldKeys).sorted() { + entries.append(Entry(kind: kind, name: name, change: .added)) + } + for name in oldKeys.intersection(newKeys).sorted() { + guard let oldItem = oldByKey[name], let newItem = newByKey[name] else { continue } + if oldItem != newItem { + entries.append(Entry(kind: kind, name: name, change: .modified(path: nil))) + } else if let path = changedPath(oldItem, newItem) { + entries.append(Entry(kind: kind, name: name, change: .modified(path: path))) + } + } + for name in oldKeys.subtracting(newKeys).sorted() { + entries.append(Entry(kind: kind, name: name, change: .removed)) + } + return entries + } + + /// The path back, when its content hash differs between snapshots; `nil` otherwise. + private static func changedPath(_ path: String?, old: PackSnapshot, new: PackSnapshot) -> String? { + guard let path, old.fileHashes[path] != new.fileHashes[path] else { return nil } + return path + } + + // MARK: Rendering + + /// Default number of change lines shown before truncating. A pack rewrite can touch dozens + /// of components, and `mcs update` renders one of these per updated pack. + static let defaultRenderLimit = 10 + + /// Indented change lines, grouped added → modified → removed, for printing under the + /// existing "updated" success line. Returns `""` when there is nothing to show. + /// + /// - Parameter indent: leading whitespace, so the block nests under its pack line in both + /// `mcs pack update` (flush-left pack lines) and `mcs update` (already indented). + func render(style: ANSIStyle, indent: String = " ", limit: Int = defaultRenderLimit) -> String { + guard !isEmpty else { return "" } + + // Grouped rather than sorted: `sorted(by:)` is not stable, and the within-group order is + // already alphabetical from `diffKeyed`. `Dictionary(grouping:)` preserves that order + // inside each bucket, and deriving the ranks from the entries means nothing here has to + // know how many `Change` cases exist. + let byRank = Dictionary(grouping: entries, by: \.change.order) + let ordered = byRank.keys.sorted().flatMap { byRank[$0] ?? [] } + var lines = ordered.prefix(limit).map { entry -> String in + let change = entry.change + var line = "\(indent)\(change.color(style: style))\(change.marker)\(style.reset)" + + " \(entry.kind.label): \(entry.name)" + // Suppressed when the entry is already named by its path (the configure script), + // where the suffix would just repeat the name back. + if case let .modified(path) = change, let path, path != entry.name { + line += " \(style.dim)(\(path) changed)\(style.reset)" + } + return line + } + + let hidden = ordered.count - lines.count + if hidden > 0 { + lines.append("\(indent)\(style.dim)… and \(hidden) more change\(hidden == 1 ? "" : "s")\(style.reset)") + } + return lines.joined(separator: "\n") + } +} diff --git a/Sources/mcs/Sync/PackUpdater.swift b/Sources/mcs/Sync/PackUpdater.swift index 3597dc67..0f38f176 100644 --- a/Sources/mcs/Sync/PackUpdater.swift +++ b/Sources/mcs/Sync/PackUpdater.swift @@ -15,7 +15,10 @@ struct PackUpdater { /// (a systemic failure should be non-zero; a user trust-decline is a zero-exit outcome). enum UpdateResult { case alreadyUpToDate - case updated(PackRegistryFile.PackEntry) + /// `diff` is `nil` when the pre-update snapshot could not be taken (e.g. the *previous* + /// manifest no longer decodes) — distinct from an empty `PackDiff`, which means the + /// update genuinely changed nothing the pack declares. + case updated(PackRegistryFile.PackEntry, diff: PackDiff?) case fetchFailed(underlying: Error) // git fetch/pull or HEAD read failed (network/transport/ref/broken checkout) case manifestInvalid(underlying: Error) // fetched revision's techpack.yaml is unusable case trustDeclined // user declined the trust prompt — expected, zero exit @@ -29,6 +32,12 @@ struct PackUpdater { packPath: URL, registry: PackRegistryFile ) -> UpdateResult { + // Must happen before `fetcher.update` resets the working tree — see `PackSnapshot`. + // Unconditional, because whether an update will happen is not known until after the + // fetch, by which point the old tree is gone. Cost is one YAML decode plus a hash per + // referenced file, discarded on the `.alreadyUpToDate` path. + let beforeSnapshot = snapshot(packPath: packPath, registry: registry) + let fetchResult: PackFetcher.FetchResult? do { fetchResult = try fetcher.update(packPath: packPath, ref: entry.ref) @@ -49,23 +58,45 @@ struct PackUpdater { } if diskSHA != entry.commitSHA { return validateAndTrust( - entry: entry, packPath: packPath, registry: registry, commitSHA: diskSHA + entry: entry, packPath: packPath, registry: registry, commitSHA: diskSHA, + beforeSnapshot: beforeSnapshot ) } return .alreadyUpToDate } return validateAndTrust( - entry: entry, packPath: packPath, registry: registry, commitSHA: result.commitSHA + entry: entry, packPath: packPath, registry: registry, commitSHA: result.commitSHA, + beforeSnapshot: beforeSnapshot ) } + /// Snapshot the pack as it stands on disk, swallowing failure rather than propagating it. + /// + /// A pack whose *previous* manifest no longer decodes must still be updatable — refusing to + /// update it would strand the user on the broken revision, with no way forward short of + /// removing and re-adding the pack. Losing the diff is the correct price. + /// + /// Silent by design: this runs before the fetch, so the outcome may well be + /// `.alreadyUpToDate` or `.fetchFailed`, where a note about a missing change summary is + /// noise about output that was never going to be printed. `.updated` is the only case that + /// wanted a diff, so that is where the absence is worth mentioning. + private func snapshot(packPath: URL, registry: PackRegistryFile) -> PackSnapshot? { + let loader = ExternalPackLoader(environment: environment, registry: registry) + do { + return try PackSnapshot.capture(packPath: packPath, loader: loader) + } catch { + return nil + } + } + /// Validate the manifest, detect new scripts, prompt for trust, and build an updated entry. private func validateAndTrust( entry: PackRegistryFile.PackEntry, packPath: URL, registry: PackRegistryFile, - commitSHA: String + commitSHA: String, + beforeSnapshot: PackSnapshot? ) -> UpdateResult { let loader = ExternalPackLoader(environment: environment, registry: registry) let manifest: ExternalPackManifest @@ -129,7 +160,18 @@ struct PackUpdater { ) } - return .updated(updatedEntry) + // The after-snapshot goes through `snapshot` rather than reusing the `manifest` loaded + // above because a snapshot is manifest *plus* file hashes, and both sides must be + // produced by the same code path for their hash maps to be comparable. + let diff: PackDiff? + if let beforeSnapshot, let after = snapshot(packPath: packPath, registry: registry) { + diff = PackDiff.between(old: beforeSnapshot, new: after) + } else { + diff = nil + output.dimmed("\(entry.displayName): no change summary available") + } + + return .updated(updatedEntry, diff: diff) } } diff --git a/Sources/mcs/TechPack/PromptDefinition.swift b/Sources/mcs/TechPack/PromptDefinition.swift index aebbc900..58440fc5 100644 --- a/Sources/mcs/TechPack/PromptDefinition.swift +++ b/Sources/mcs/TechPack/PromptDefinition.swift @@ -1,4 +1,4 @@ -struct PromptDefinition: Codable { +struct PromptDefinition: Codable, Equatable { let key: String let type: PromptType let label: String? diff --git a/Tests/MCSTests/ExternalPackManifestTests.swift b/Tests/MCSTests/ExternalPackManifestTests.swift index 651defde..99c308a7 100644 --- a/Tests/MCSTests/ExternalPackManifestTests.swift +++ b/Tests/MCSTests/ExternalPackManifestTests.swift @@ -3397,4 +3397,34 @@ struct ExternalPackManifestTests { let manifest = ignoreManifest(ignore: ["techpack.yaml", " "]) #expect(manifest.silentlySanitizedIgnoreEntries().ignore == nil) } + + // MARK: - Surface drift tripwire + + /// Fails when a stored property is added to or removed from `ExternalPackManifest`. + /// + /// Two places hand-enumerate the manifest's author-visible surfaces, and neither is checked + /// by the compiler when a new one appears — a new surface would silently never be diffed and + /// never be treated as load-bearing: + /// + /// - `PackDiff.Kind` + the `diffKeyed` calls in `PackDiff.between` — what `mcs pack update` + /// reports as changed. + /// - `ExternalPackManifest.referencedPaths` — which files `ignore:` validation refuses to + /// mask, `PackHeuristics` treats as referenced, and `PackSnapshot` hashes. + /// - `ExternalPackLoader.findMissingReferencedFiles` — a third, narrower traversal. + /// + /// If this test fails, update those alongside the property, then update the list here. + @Test("Manifest surfaces are unchanged — update PackDiff.Kind and referencedPaths if this fails") + func manifestSurfacesUnchanged() { + let labels = Mirror(reflecting: ignoreManifest(ignore: nil)) + .children.compactMap(\.label) + + #expect(Set(labels) == [ + // Metadata — not diffed, references no files. + "schemaVersion", "identifier", "displayName", "description", "author", "minMCSVersion", + "ignore", + // Author-visible surfaces — each needs a `PackDiff.Kind` case, and any that names a + // file needs a `referencedSource` folded into `referencedPaths`. + "components", "templates", "prompts", "configureProject", "supplementaryDoctorChecks", + ]) + } } diff --git a/Tests/MCSTests/PackDiffTests.swift b/Tests/MCSTests/PackDiffTests.swift new file mode 100644 index 00000000..6d450134 --- /dev/null +++ b/Tests/MCSTests/PackDiffTests.swift @@ -0,0 +1,411 @@ +import Foundation +@testable import mcs +import Testing + +struct PackDiffTests { + // MARK: - Builders + + private func hookComponent( + id: String, + description: String = "A hook", + source: String = "hooks/lint.sh", + destination: String = "lint.sh" + ) -> ExternalComponentDefinition { + ExternalComponentDefinition( + id: id, + displayName: id, + description: description, + type: .hookFile, + installAction: .copyPackFile(ExternalCopyPackFileConfig( + source: source, destination: destination, fileType: .hook + )) + ) + } + + private func brewComponent(id: String, package: String = "jq") -> ExternalComponentDefinition { + ExternalComponentDefinition( + id: id, + displayName: id, + description: "A brew package", + type: .brewPackage, + installAction: .brewInstall(package: package) + ) + } + + /// `ExternalDoctorCheckDefinition`'s `let` optionals get no defaults in the synthesized + /// memberwise init, so spell the unused ones out once here rather than at each call site. + private func commandCheck(name: String, command: String) -> ExternalDoctorCheckDefinition { + ExternalDoctorCheckDefinition( + type: .commandExists, + name: name, + section: nil, + command: command, + args: nil, + path: nil, + pattern: nil, + scope: nil, + fixCommand: nil, + fixScript: nil, + event: nil, + keyPath: nil, + expectedValue: nil, + isOptional: nil + ) + } + + private func inputPrompt(key: String, label: String = "Enter a value") -> PromptDefinition { + PromptDefinition( + key: key, + type: .input, + label: label, + defaultValue: nil, + options: nil, + detectPatterns: nil, + scriptCommand: nil + ) + } + + private func manifest( + components: [ExternalComponentDefinition]? = nil, + templates: [ExternalTemplateDefinition]? = nil, + prompts: [PromptDefinition]? = nil, + configureProject: ExternalConfigureProject? = nil, + supplementaryDoctorChecks: [ExternalDoctorCheckDefinition]? = nil + ) -> ExternalPackManifest { + ExternalPackManifest( + schemaVersion: 1, + identifier: "test-pack", + displayName: "Test Pack", + description: "A test pack", + author: nil, + minMCSVersion: nil, + components: components, + templates: templates, + prompts: prompts, + configureProject: configureProject, + supplementaryDoctorChecks: supplementaryDoctorChecks, + ignore: nil + ) + } + + private func snapshot( + _ manifest: ExternalPackManifest, + fileHashes: [String: String] = [:] + ) -> PackSnapshot { + PackSnapshot(manifest: manifest, fileHashes: fileHashes) + } + + private let plainStyle = ANSIStyle(enabled: false) + + // MARK: - Components + + @Test("Component present only in the new manifest is added") + func componentAdded() { + let diff = PackDiff.between( + old: snapshot(manifest(components: [hookComponent(id: "test-pack.lint")])), + new: snapshot(manifest(components: [ + hookComponent(id: "test-pack.lint"), + brewComponent(id: "test-pack.jq"), + ])) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .component, name: "test-pack.jq", change: .added), + ]) + } + + @Test("Component present only in the old manifest is removed") + func componentRemoved() { + let diff = PackDiff.between( + old: snapshot(manifest(components: [ + hookComponent(id: "test-pack.lint"), + brewComponent(id: "test-pack.jq"), + ])), + new: snapshot(manifest(components: [hookComponent(id: "test-pack.lint")])) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .component, name: "test-pack.jq", change: .removed), + ]) + } + + @Test("Changed declaration reports modified with no file path") + func componentDeclarationModified() { + let diff = PackDiff.between( + old: snapshot(manifest(components: [brewComponent(id: "test-pack.jq", package: "jq")])), + new: snapshot(manifest(components: [brewComponent(id: "test-pack.jq", package: "yq")])) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .component, name: "test-pack.jq", change: .modified(path: nil)), + ]) + } + + @Test("Unchanged declaration with changed file content reports the file path") + func componentFileContentModified() { + let component = hookComponent(id: "test-pack.lint", source: "hooks/lint.sh") + let diff = PackDiff.between( + old: snapshot(manifest(components: [component]), fileHashes: ["hooks/lint.sh": "aaa"]), + new: snapshot(manifest(components: [component]), fileHashes: ["hooks/lint.sh": "bbb"]) + ) + + #expect(diff.entries == [ + PackDiff.Entry( + kind: .component, name: "test-pack.lint", + change: .modified(path: "hooks/lint.sh") + ), + ]) + } + + @Test("Declaration change wins over file change — one entry, no file path") + func declarationChangeTakesPrecedenceOverFileChange() { + let diff = PackDiff.between( + old: snapshot( + manifest(components: [hookComponent(id: "test-pack.lint", description: "old")]), + fileHashes: ["hooks/lint.sh": "aaa"] + ), + new: snapshot( + manifest(components: [hookComponent(id: "test-pack.lint", description: "new")]), + fileHashes: ["hooks/lint.sh": "bbb"] + ) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .component, name: "test-pack.lint", change: .modified(path: nil)), + ]) + } + + @Test("A file appearing where none was hashed before counts as changed") + func missingFileHashCountsAsChange() { + let component = hookComponent(id: "test-pack.lint", source: "hooks/lint.sh") + let diff = PackDiff.between( + old: snapshot(manifest(components: [component])), + new: snapshot(manifest(components: [component]), fileHashes: ["hooks/lint.sh": "bbb"]) + ) + + #expect(diff.entries == [ + PackDiff.Entry( + kind: .component, name: "test-pack.lint", + change: .modified(path: "hooks/lint.sh") + ), + ]) + } + + @Test("Component that references no file is never reported via file hashes") + func nonCopyingComponentIgnoresFileHashes() { + let component = brewComponent(id: "test-pack.jq") + let diff = PackDiff.between( + old: snapshot(manifest(components: [component]), fileHashes: ["hooks/lint.sh": "aaa"]), + new: snapshot(manifest(components: [component]), fileHashes: ["hooks/lint.sh": "bbb"]) + ) + + #expect(diff.isEmpty) + } + + // MARK: - Templates and doctor checks + + @Test("Template content change is attributed to its section") + func templateContentModified() { + let template = ExternalTemplateDefinition( + sectionIdentifier: "test-pack.main", + placeholders: nil, + contentFile: "templates/main.md", + dependencies: nil + ) + let diff = PackDiff.between( + old: snapshot(manifest(templates: [template]), fileHashes: ["templates/main.md": "aaa"]), + new: snapshot(manifest(templates: [template]), fileHashes: ["templates/main.md": "bbb"]) + ) + + #expect(diff.entries == [ + PackDiff.Entry( + kind: .template, name: "test-pack.main", + change: .modified(path: "templates/main.md") + ), + ]) + } + + @Test("Supplementary doctor checks diff by name") + func doctorCheckAddedAndRemoved() { + let diff = PackDiff.between( + old: snapshot(manifest(supplementaryDoctorChecks: [ + commandCheck(name: "git present", command: "git"), + ])), + new: snapshot(manifest(supplementaryDoctorChecks: [ + commandCheck(name: "jq present", command: "jq"), + ])) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .doctorCheck, name: "jq present", change: .added), + PackDiff.Entry(kind: .doctorCheck, name: "git present", change: .removed), + ]) + } + + // MARK: - Prompts + + @Test("Prompts diff by key") + func promptAddedAndRemoved() { + let diff = PackDiff.between( + old: snapshot(manifest(prompts: [inputPrompt(key: "apiKey")])), + new: snapshot(manifest(prompts: [inputPrompt(key: "token")])) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .prompt, name: "token", change: .added), + PackDiff.Entry(kind: .prompt, name: "apiKey", change: .removed), + ]) + } + + @Test("Prompt kept under the same key but redefined is modified") + func promptModified() { + let diff = PackDiff.between( + old: snapshot(manifest(prompts: [inputPrompt(key: "apiKey", label: "Old label")])), + new: snapshot(manifest(prompts: [inputPrompt(key: "apiKey", label: "New label")])) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .prompt, name: "apiKey", change: .modified(path: nil)), + ]) + } + + // MARK: - Configure script + + @Test("Adding a configure script is reported") + func configureScriptAdded() { + let diff = PackDiff.between( + old: snapshot(manifest()), + new: snapshot( + manifest(configureProject: ExternalConfigureProject(script: "scripts/setup.sh")), + fileHashes: ["scripts/setup.sh": "aaa"] + ) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .configureScript, name: "scripts/setup.sh", change: .added), + ]) + } + + @Test("Editing the configure script body is reported — the gap this closes") + func configureScriptContentModified() { + let configure = ExternalConfigureProject(script: "scripts/setup.sh") + let diff = PackDiff.between( + old: snapshot(manifest(configureProject: configure), fileHashes: ["scripts/setup.sh": "aaa"]), + new: snapshot(manifest(configureProject: configure), fileHashes: ["scripts/setup.sh": "bbb"]) + ) + + #expect(diff.entries == [ + PackDiff.Entry( + kind: .configureScript, name: "scripts/setup.sh", + change: .modified(path: "scripts/setup.sh") + ), + ]) + } + + @Test("Repointing the configure script reads as a removal plus an addition") + func configureScriptRepointed() { + let diff = PackDiff.between( + old: snapshot( + manifest(configureProject: ExternalConfigureProject(script: "scripts/old.sh")), + fileHashes: ["scripts/old.sh": "aaa"] + ), + new: snapshot( + manifest(configureProject: ExternalConfigureProject(script: "scripts/new.sh")), + fileHashes: ["scripts/new.sh": "bbb"] + ) + ) + + #expect(diff.entries == [ + PackDiff.Entry(kind: .configureScript, name: "scripts/new.sh", change: .added), + PackDiff.Entry(kind: .configureScript, name: "scripts/old.sh", change: .removed), + ]) + } + + @Test("Configure script content change renders without repeating the path") + func configureScriptRendersWithoutRedundantSuffix() { + let diff = PackDiff(entries: [ + PackDiff.Entry( + kind: .configureScript, name: "scripts/setup.sh", + change: .modified(path: "scripts/setup.sh") + ), + ]) + + #expect(diff.render(style: plainStyle) == " ~ configure script: scripts/setup.sh") + } + + // MARK: - Empty + + @Test("Identical snapshots produce an empty diff that renders as nothing") + func identicalSnapshotsAreEmpty() { + let same = manifest(components: [hookComponent(id: "test-pack.lint")]) + let diff = PackDiff.between( + old: snapshot(same, fileHashes: ["hooks/lint.sh": "aaa"]), + new: snapshot(same, fileHashes: ["hooks/lint.sh": "aaa"]) + ) + + #expect(diff.isEmpty) + #expect(diff.render(style: plainStyle).isEmpty) + } + + // MARK: - Rendering + + @Test("Render groups added, then modified, then removed") + func renderGroupsByChangeKind() { + let diff = PackDiff(entries: [ + PackDiff.Entry(kind: .component, name: "test-pack.gone", change: .removed), + PackDiff.Entry(kind: .component, name: "test-pack.lint", change: .modified(path: "hooks/lint.sh")), + PackDiff.Entry(kind: .component, name: "test-pack.new", change: .added), + ]) + + #expect(diff.render(style: plainStyle) == """ + + component: test-pack.new + ~ component: test-pack.lint (hooks/lint.sh changed) + - component: test-pack.gone + """) + } + + @Test("Render honors a custom indent") + func renderHonorsIndent() { + let diff = PackDiff(entries: [ + PackDiff.Entry(kind: .template, name: "test-pack.main", change: .added), + ]) + + #expect(diff.render(style: plainStyle, indent: " ") == " + template: test-pack.main") + } + + @Test("Render truncates past the limit with a remainder line") + func renderTruncatesPastLimit() { + let entries = (1 ... 5).map { + PackDiff.Entry(kind: .component, name: "test-pack.c\($0)", change: .added) + } + let rendered = PackDiff(entries: entries).render(style: plainStyle, limit: 3) + + #expect(rendered == """ + + component: test-pack.c1 + + component: test-pack.c2 + + component: test-pack.c3 + … and 2 more changes + """) + } + + @Test("Exactly at the limit renders no remainder line") + func renderAtLimitHasNoRemainder() { + let entries = (1 ... 3).map { + PackDiff.Entry(kind: .component, name: "test-pack.c\($0)", change: .added) + } + let rendered = PackDiff(entries: entries).render(style: plainStyle, limit: 3) + + #expect(!rendered.contains("more change")) + #expect(rendered.split(separator: "\n").count == 3) + } + + @Test("A single hidden change is singular") + func renderSingularRemainder() { + let entries = (1 ... 4).map { + PackDiff.Entry(kind: .component, name: "test-pack.c\($0)", change: .added) + } + let rendered = PackDiff(entries: entries).render(style: plainStyle, limit: 3) + + #expect(rendered.hasSuffix("… and 1 more change")) + } +} diff --git a/Tests/MCSTests/PackUpdaterTests.swift b/Tests/MCSTests/PackUpdaterTests.swift index 2b4c78a7..ae2829c9 100644 --- a/Tests/MCSTests/PackUpdaterTests.swift +++ b/Tests/MCSTests/PackUpdaterTests.swift @@ -119,19 +119,12 @@ struct PackUpdaterTests { ) } - /// Push a new commit to the remote and return its SHA. + /// Push a commit touching only an unreferenced file, and return its SHA. private func pushNewCommit(fixture: Fixture) throws -> String { + try pushFiles(fixture: fixture, files: ["README.md": "updated-\(UUID().uuidString)"]) + let workDir = fixture.tmpDir.appendingPathComponent("work") let shell = ShellRunner(environment: Environment(home: fixture.tmpDir)) - - try "updated-\(UUID().uuidString)".write( - to: workDir.appendingPathComponent("README.md"), - atomically: true, encoding: .utf8 - ) - try git(shell, ["-C", workDir.path, "add", "."], context: "git add") - try git(shell, ["-C", workDir.path, "commit", "-m", "update"], context: "git commit") - try git(shell, ["-C", workDir.path, "push"], context: "git push") - let shaResult = shell.run( shell.environment.gitPath, arguments: ["-C", workDir.path, "rev-parse", "HEAD"] ) @@ -144,18 +137,49 @@ struct PackUpdaterTests { /// Replace techpack.yaml with the given content and push it as a new commit. Used to /// drive the post-fetch validate/trust paths (manifest-invalid, trust-decline). private func pushManifest(fixture: Fixture, manifest: String) throws { + try pushFiles(fixture: fixture, files: ["techpack.yaml": manifest]) + } + + /// Write a set of pack-relative files and push them as one commit. + private func pushFiles(fixture: Fixture, files: [String: String]) throws { let workDir = fixture.tmpDir.appendingPathComponent("work") let shell = ShellRunner(environment: Environment(home: fixture.tmpDir)) - try manifest.write( - to: workDir.appendingPathComponent("techpack.yaml"), - atomically: true, encoding: .utf8 - ) + for (path, contents) in files { + let fileURL = workDir.appendingPathComponent(path) + try FileManager.default.createDirectory( + at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true + ) + try contents.write(to: fileURL, atomically: true, encoding: .utf8) + } try git(shell, ["-C", workDir.path, "add", "."], context: "git add") - try git(shell, ["-C", workDir.path, "commit", "-m", "update manifest"], context: "git commit") + try git(shell, ["-C", workDir.path, "commit", "-m", "push files"], context: "git commit") try git(shell, ["-C", workDir.path, "push"], context: "git push") } + /// A manifest declaring one skill component backed by `skills/docs.md`. + /// + /// Skill files are deliberately not trustable content (unlike hooks and commands), so this + /// exercises the referenced-file diff without tripping the trust prompt, which cannot be + /// answered from a non-interactive test. + private func skillManifest(description: String = "A docs skill") -> String { + """ + schemaVersion: 1 + identifier: test-pack + displayName: Test Pack + description: A test pack + components: + - id: docs + description: \(description) + type: skill + installAction: + type: copyPackFile + source: skills/docs.md + destination: docs.md + fileType: skill + """ + } + // MARK: - Tests @Test("returns alreadyUpToDate when disk SHA matches registry SHA") @@ -190,7 +214,7 @@ struct PackUpdaterTests { entry: entry, packPath: packPath, registry: fix.registry ) - guard case let .updated(updatedEntry) = result else { + guard case let .updated(updatedEntry, _) = result else { Issue.record("Expected .updated, got \(result)") return } @@ -222,7 +246,7 @@ struct PackUpdaterTests { entry: staleEntry, packPath: packPath, registry: fix.registry ) - guard case let .updated(updatedEntry) = result else { + guard case let .updated(updatedEntry, _) = result else { Issue.record("Expected .updated (stale recovery), got \(result)") return } @@ -382,6 +406,172 @@ struct PackUpdaterTests { #expect(result.isHardFailure) } + // MARK: - Change summary + + @Test("diff reports a component added by the update") + func diffReportsAddedComponent() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let entry = makeEntry(commitSHA: fix.initialSHA) + let packPath = fix.packsDir.appendingPathComponent("test-pack") + + try pushFiles(fixture: fix, files: [ + "techpack.yaml": skillManifest(), + "skills/docs.md": "# Docs\n", + ]) + + let result = fix.updater.updateGitPack( + entry: entry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(_, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(diff?.entries == [ + PackDiff.Entry(kind: .component, name: "test-pack.docs", change: .added), + ]) + } + + @Test("diff reports a component removed by the update") + func diffReportsRemovedComponent() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let packPath = fix.packsDir.appendingPathComponent("test-pack") + + // Establish a baseline revision that declares the component... + try pushFiles(fixture: fix, files: [ + "techpack.yaml": skillManifest(), + "skills/docs.md": "# Docs\n", + ]) + let baseline = fix.updater.updateGitPack( + entry: makeEntry(commitSHA: fix.initialSHA), packPath: packPath, registry: fix.registry + ) + guard case let .updated(baselineEntry, _) = baseline else { + Issue.record("Baseline update failed: \(baseline)") + return + } + + // ...then drop it. + try pushManifest( + fixture: fix, + manifest: """ + schemaVersion: 1 + identifier: test-pack + displayName: Test Pack + description: A test pack + """ + ) + + let result = fix.updater.updateGitPack( + entry: baselineEntry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(_, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(diff?.entries == [ + PackDiff.Entry(kind: .component, name: "test-pack.docs", change: .removed), + ]) + } + + @Test("diff attributes a content-only edit to the component's file") + func diffReportsChangedFileContent() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let packPath = fix.packsDir.appendingPathComponent("test-pack") + + try pushFiles(fixture: fix, files: [ + "techpack.yaml": skillManifest(), + "skills/docs.md": "# Docs\n", + ]) + let baseline = fix.updater.updateGitPack( + entry: makeEntry(commitSHA: fix.initialSHA), packPath: packPath, registry: fix.registry + ) + guard case let .updated(baselineEntry, _) = baseline else { + Issue.record("Baseline update failed: \(baseline)") + return + } + + // Manifest untouched — only the referenced file's body changes. + try pushFiles(fixture: fix, files: ["skills/docs.md": "# Docs\n\nNow with content.\n"]) + + let result = fix.updater.updateGitPack( + entry: baselineEntry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(_, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(diff?.entries == [ + PackDiff.Entry( + kind: .component, name: "test-pack.docs", + change: .modified(path: "skills/docs.md") + ), + ]) + } + + // The configure-script kind is covered at the pure layer in `PackDiffTests`, not here: a + // configure script is trustable content, so any pack declaring one sends `updateGitPack` + // through `promptForTrust`, which a non-interactive test can only decline — the `.updated` + // case a diff assertion needs is unreachable. `diffReportsChangedFileContent` above + // exercises the identical content-hash path through a skill file, which is not trustable. + + @Test("diff is empty when the update touches only unreferenced files") + func diffEmptyForUnreferencedFileChange() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let entry = makeEntry(commitSHA: fix.initialSHA) + let packPath = fix.packsDir.appendingPathComponent("test-pack") + + // pushNewCommit only rewrites README.md. + _ = try pushNewCommit(fixture: fix) + + let result = fix.updater.updateGitPack( + entry: entry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(_, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(diff?.isEmpty == true) + } + + @Test("update still succeeds with no diff when the previous manifest is unreadable") + func diffUnavailableWhenPreviousManifestBroken() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let entry = makeEntry(commitSHA: fix.initialSHA) + let packPath = fix.packsDir.appendingPathComponent("test-pack") + + // Corrupt the *installed* checkout, so the pre-update snapshot cannot be taken. + // The fetched revision is fine, so the update itself must still go through. + try "not: [valid".write( + to: packPath.appendingPathComponent("techpack.yaml"), + atomically: true, encoding: .utf8 + ) + let newSHA = try pushNewCommit(fixture: fix) + + let result = fix.updater.updateGitPack( + entry: entry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(updatedEntry, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(updatedEntry.commitSHA == newSHA) + #expect(diff == nil) + } + // MARK: - Helper property contract @Test("isHardFailure and reason classify every case correctly") @@ -392,8 +582,8 @@ struct PackUpdaterTests { // Non-failures #expect(!PackUpdater.UpdateResult.alreadyUpToDate.isHardFailure) #expect(PackUpdater.UpdateResult.alreadyUpToDate.reason == nil) - #expect(!PackUpdater.UpdateResult.updated(entry).isHardFailure) - #expect(PackUpdater.UpdateResult.updated(entry).reason == nil) + #expect(!PackUpdater.UpdateResult.updated(entry, diff: nil).isHardFailure) + #expect(PackUpdater.UpdateResult.updated(entry, diff: nil).reason == nil) #expect(!PackUpdater.UpdateResult.trustDeclined.isHardFailure) #expect(PackUpdater.UpdateResult.trustDeclined.reason?.contains("trust not granted") == true) From 579e2e04fce89d3a817bfbb84f75cbe80530e8ea Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 31 Jul 2026 00:56:58 +0200 Subject: [PATCH 2/3] Anchor the global gitignore path to the injected environment home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - GitignoreManager resolved the process home, so any sandboxed caller — including the test suite — read and wrote the real user's ~/.config/git/ignore - Pass HOME to git config --global so --global resolution stays inside the sandbox too - Seed the sandbox gitignore in the shared test fixture; without it every doctor run in a sandbox carried an unrelated "global gitignore not found" issue --- Sources/mcs/Core/GitignoreManager.swift | 17 +++++++++++------ Tests/MCSTests/GitignoreManagerTests.swift | 20 ++++++++++++++++++++ Tests/MCSTests/TestHelpers.swift | 18 ++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/Sources/mcs/Core/GitignoreManager.swift b/Sources/mcs/Core/GitignoreManager.swift index b4606e72..4f194182 100644 --- a/Sources/mcs/Core/GitignoreManager.swift +++ b/Sources/mcs/Core/GitignoreManager.swift @@ -14,19 +14,24 @@ struct GitignoreManager { /// Resolve the global gitignore file path. /// Checks `git config core.excludesFile`, falls back to `~/.config/git/ignore`. + /// + /// Every path here is anchored to `shell.environment.homeDirectory`, never to the process's + /// own home. In production the two are the same (`Environment()` derives from + /// `NSHomeDirectory()`), but they differ under an injected `Environment(home:)` — and this + /// type both reads *and writes* the file, so resolving the process home let a sandboxed + /// caller mutate the real user's global gitignore. `HOME` is passed to `git config` for the + /// same reason: `--global` resolves `$HOME/.gitconfig`, which would otherwise escape too. func resolveGlobalGitignorePath() -> URL { + let home = shell.environment.homeDirectory let result = shell.run( shell.environment.gitPath, - arguments: ["config", "--global", "core.excludesFile"] + arguments: ["config", "--global", "core.excludesFile"], + additionalEnvironment: ["HOME": home.path] ) if result.succeeded, !result.stdout.isEmpty { - let path = result.stdout.replacingOccurrences( - of: "~", - with: FileManager.default.homeDirectoryForCurrentUser.path - ) + let path = result.stdout.replacingOccurrences(of: "~", with: home.path) return URL(fileURLWithPath: path) } - let home = FileManager.default.homeDirectoryForCurrentUser return home .appendingPathComponent(".config") .appendingPathComponent("git") diff --git a/Tests/MCSTests/GitignoreManagerTests.swift b/Tests/MCSTests/GitignoreManagerTests.swift index e2639668..eb68de26 100644 --- a/Tests/MCSTests/GitignoreManagerTests.swift +++ b/Tests/MCSTests/GitignoreManagerTests.swift @@ -51,6 +51,26 @@ struct GitignoreManagerTests { let removed = try manager.removeEntry(".claude") #expect(removed == false) } + + // MARK: - Sandbox containment + + /// `GitignoreManager` both reads and writes the global gitignore, so resolving the process's + /// own home instead of the injected `Environment`'s let any sandboxed caller — including the + /// whole test suite — mutate the real user's `~/.config/git/ignore`. It also made + /// `GitignoreCheck` report whatever the developer's machine happened to have, which is how a + /// doctor test passed locally and on one CI runner while failing on another. + @Test("Global gitignore path stays inside the injected environment's home") + func resolvedPathIsContainedInEnvironmentHome() throws { + let home = try makeGlobalTmpDir(label: "gitignore-containment") + defer { try? FileManager.default.removeItem(at: home) } + + let manager = GitignoreManager(shell: ShellRunner(environment: Environment(home: home))) + let resolved = manager.resolveGlobalGitignorePath().resolvingSymlinksInPath().path + let sandbox = home.resolvingSymlinksInPath().path + + #expect(resolved.hasPrefix(sandbox)) + #expect(!resolved.hasPrefix(FileManager.default.homeDirectoryForCurrentUser.path)) + } } /// Test helper that bypasses git config resolution and operates on a fixed file path. diff --git a/Tests/MCSTests/TestHelpers.swift b/Tests/MCSTests/TestHelpers.swift index c481ce59..0b690374 100644 --- a/Tests/MCSTests/TestHelpers.swift +++ b/Tests/MCSTests/TestHelpers.swift @@ -357,9 +357,27 @@ func makeGlobalTmpDir(label: String = "global") throws -> URL { at: dir.appendingPathComponent(".mcs"), withIntermediateDirectories: true ) + try seedGlobalGitignore(home: dir) return dir } +/// Write mcs's core entries to the sandbox's global gitignore. +/// +/// `GitignoreCheck` resolves this file from the injected `Environment`'s home, so a sandbox +/// without one reports "global gitignore not found" and every doctor run inside it carries an +/// extra issue unrelated to what the test is asserting. Seeding it makes the sandbox look like a +/// machine `mcs sync` has already run on, which is the state these fixtures assume. +func seedGlobalGitignore(home: URL) throws { + let gitDir = home + .appendingPathComponent(".config") + .appendingPathComponent("git") + try FileManager.default.createDirectory(at: gitDir, withIntermediateDirectories: true) + try (GitignoreManager.coreEntries.joined(separator: "\n") + "\n").write( + to: gitDir.appendingPathComponent("ignore"), + atomically: true, encoding: .utf8 + ) +} + /// Create a temp home pre-configured with the Claude Code canonical layout: /// `.claude/` directory plus (optional) `.claude.json` sibling file. func makeClaudeHome(label: String = "claude-home", withJSON: Bool = true) throws -> URL { From e251695bd2fa6a7b1a58be40ce6cc01de0bde68d Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 31 Jul 2026 01:05:53 +0200 Subject: [PATCH 3/3] Diff directory-sourced components and order the summary after the update line - Hash directory sources into a combined per-file digest; skills ship as directories, so edits inside them were invisible to the change summary - Move the "no change summary available" notice to the callers, which print it after the success line instead of before - Cover both directory-content change and directory-unchanged cases --- Sources/mcs/Commands/PackCommand.swift | 4 +- Sources/mcs/Commands/UpdateCommand.swift | 4 +- Sources/mcs/Sync/PackDiff.swift | 49 ++++++++++- Sources/mcs/Sync/PackUpdater.swift | 7 +- Tests/MCSTests/PackUpdaterTests.swift | 103 +++++++++++++++++++++++ 5 files changed, 155 insertions(+), 12 deletions(-) diff --git a/Sources/mcs/Commands/PackCommand.swift b/Sources/mcs/Commands/PackCommand.swift index 0c276cd7..dc405769 100644 --- a/Sources/mcs/Commands/PackCommand.swift +++ b/Sources/mcs/Commands/PackCommand.swift @@ -682,9 +682,7 @@ struct UpdatePack: LockedCommand { ctx.registry.register(updatedEntry, in: &updatedData) updatedCount += 1 ctx.output.success("\(entry.displayName): updated (\(updatedEntry.shortSHA))") - if let diff, !diff.isEmpty { - ctx.output.plain(diff.render(style: ctx.output.style)) - } + ctx.output.packChangeSummary(diff) case .trustDeclined: ctx.output.info("\(entry.identifier): \(result.reason ?? "trust not granted") (will re-prompt next run)") case .fetchFailed, .manifestInvalid, .internalError: diff --git a/Sources/mcs/Commands/UpdateCommand.swift b/Sources/mcs/Commands/UpdateCommand.swift index 99601f4c..8bab376b 100644 --- a/Sources/mcs/Commands/UpdateCommand.swift +++ b/Sources/mcs/Commands/UpdateCommand.swift @@ -280,9 +280,7 @@ struct UpdateCommand: LockedCommand { registryFile.register(updatedEntry, in: &updatedData) anyUpdated = true output.success(" \(entry.displayName): \(entry.shortSHA) → \(updatedEntry.shortSHA)") - if let diff, !diff.isEmpty { - output.plain(diff.render(style: output.style, indent: " ")) - } + output.packChangeSummary(diff, indent: " ") case .trustDeclined: output.info(" \(entry.identifier): \(result.reason ?? "trust not granted") (will re-prompt on next 'mcs update')") skipped.insert(entry.identifier) diff --git a/Sources/mcs/Sync/PackDiff.swift b/Sources/mcs/Sync/PackDiff.swift index 3b59322b..d3692818 100644 --- a/Sources/mcs/Sync/PackDiff.swift +++ b/Sources/mcs/Sync/PackDiff.swift @@ -27,9 +27,8 @@ struct PackSnapshot { let manifest = try loader.validate(at: packPath) var hashes: [String: String] = [:] for path in manifest.referencedPaths { - let fileURL = packPath.appendingPathComponent(path) do { - hashes[path] = try FileHasher.sha256(of: fileURL) + hashes[path] = try contentHash(of: path, in: packPath) } catch { // Absent or unreadable: leave the key out. Its absence on one side and presence // on the other is exactly the "changed" signal we want, and a manifest that @@ -39,6 +38,33 @@ struct PackSnapshot { } return PackSnapshot(manifest: manifest, fileHashes: hashes) } + + /// Content hash for one referenced path, which may be a file *or* a directory. + /// + /// `copyPackFile` accepts either — `ComponentExecutor` branches on `isDirectory` the same + /// way, and skills in particular are commonly shipped as a directory. Hashing a directory + /// with `FileHasher.sha256(of:)` throws, which previously made every edit inside a + /// directory-sourced component invisible to the diff. + /// + /// Directories fold their per-file hashes into one digest over `path:hash` pairs sorted by + /// path, so the result is order-independent and changes when any contained file is added, + /// removed, or edited. Unreadable files contribute their path, so a file becoming + /// unreadable still registers as a change rather than vanishing from the digest. + private static func contentHash(of path: String, in packPath: URL) throws -> String { + let url = packPath.appendingPathComponent(path) + var isDirectory: ObjCBool = false + guard FileManager.default.fileExists(atPath: url.path, isDirectory: &isDirectory) else { + throw MCSError.fileOperationFailed(path: url.path, reason: "referenced path does not exist") + } + guard isDirectory.boolValue else { + return try FileHasher.sha256(of: url) + } + + let result = try FileHasher.directoryFileHashes(at: url) + let entries = result.hashes.map { "\($0.relativePath):\($0.hash)" } + + result.failures.map { "\($0.relativePath):" } + return FileHasher.sha256(data: Data(entries.sorted().joined(separator: "\n").utf8)) + } } // MARK: - Diff @@ -259,3 +285,22 @@ struct PackDiff: Equatable { return lines.joined(separator: "\n") } } + +extension CLIOutput { + /// Print a pack's change summary beneath the "updated" line the caller just emitted. + /// + /// Both the diff and the "unavailable" notice are printed here, by the caller, so they land + /// *after* the success line. Emitting the notice from `PackUpdater` put it before, since the + /// result has to be returned before the caller can announce the update. + /// + /// `nil` means the snapshot could not be taken; an empty diff means the update changed + /// nothing the pack declares, which needs no output at all. + func packChangeSummary(_ diff: PackDiff?, indent: String = " ") { + guard let diff else { + dimmed("\(indent)no change summary available") + return + } + guard !diff.isEmpty else { return } + plain(diff.render(style: style, indent: indent)) + } +} diff --git a/Sources/mcs/Sync/PackUpdater.swift b/Sources/mcs/Sync/PackUpdater.swift index 0f38f176..ef7e692c 100644 --- a/Sources/mcs/Sync/PackUpdater.swift +++ b/Sources/mcs/Sync/PackUpdater.swift @@ -163,12 +163,11 @@ struct PackUpdater { // The after-snapshot goes through `snapshot` rather than reusing the `manifest` loaded // above because a snapshot is manifest *plus* file hashes, and both sides must be // produced by the same code path for their hash maps to be comparable. - let diff: PackDiff? + // Reporting a missing summary is the caller's job: it has to happen *after* the + // "updated" line, which cannot be printed until this result is returned. + var diff: PackDiff? if let beforeSnapshot, let after = snapshot(packPath: packPath, registry: registry) { diff = PackDiff.between(old: beforeSnapshot, new: after) - } else { - diff = nil - output.dimmed("\(entry.displayName): no change summary available") } return .updated(updatedEntry, diff: diff) diff --git a/Tests/MCSTests/PackUpdaterTests.swift b/Tests/MCSTests/PackUpdaterTests.swift index ae2829c9..3ec45184 100644 --- a/Tests/MCSTests/PackUpdaterTests.swift +++ b/Tests/MCSTests/PackUpdaterTests.swift @@ -522,6 +522,109 @@ struct PackUpdaterTests { // case a diff assertion needs is unreachable. `diffReportsChangedFileContent` above // exercises the identical content-hash path through a skill file, which is not trustable. + @Test("diff reports an edit inside a directory-sourced component") + func diffReportsChangedDirectoryContent() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let packPath = fix.packsDir.appendingPathComponent("test-pack") + // `source:` names a directory, which is how skills are commonly shipped. Hashing it as a + // file throws, which previously made every edit inside it invisible to the diff. + let manifest = """ + schemaVersion: 1 + identifier: test-pack + displayName: Test Pack + description: A test pack + components: + - id: docs + description: A docs skill + type: skill + installAction: + type: copyPackFile + source: skills/docs + destination: docs + fileType: skill + """ + + try pushFiles(fixture: fix, files: [ + "techpack.yaml": manifest, + "skills/docs/SKILL.md": "# Docs\n", + "skills/docs/reference.md": "Reference\n", + ]) + let baseline = fix.updater.updateGitPack( + entry: makeEntry(commitSHA: fix.initialSHA), packPath: packPath, registry: fix.registry + ) + guard case let .updated(baselineEntry, _) = baseline else { + Issue.record("Baseline update failed: \(baseline)") + return + } + + // Manifest untouched; one file *inside* the directory changes. + try pushFiles(fixture: fix, files: ["skills/docs/reference.md": "Reference, revised\n"]) + + let result = fix.updater.updateGitPack( + entry: baselineEntry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(_, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(diff?.entries == [ + PackDiff.Entry( + kind: .component, name: "test-pack.docs", + change: .modified(path: "skills/docs") + ), + ]) + } + + @Test("diff is empty when a directory-sourced component is untouched") + func diffEmptyWhenDirectoryContentUnchanged() throws { + let fix = try makeFixture() + defer { fix.cleanup() } + + let packPath = fix.packsDir.appendingPathComponent("test-pack") + let manifest = """ + schemaVersion: 1 + identifier: test-pack + displayName: Test Pack + description: A test pack + components: + - id: docs + description: A docs skill + type: skill + installAction: + type: copyPackFile + source: skills/docs + destination: docs + fileType: skill + """ + + try pushFiles(fixture: fix, files: [ + "techpack.yaml": manifest, + "skills/docs/SKILL.md": "# Docs\n", + ]) + let baseline = fix.updater.updateGitPack( + entry: makeEntry(commitSHA: fix.initialSHA), packPath: packPath, registry: fix.registry + ) + guard case let .updated(baselineEntry, _) = baseline else { + Issue.record("Baseline update failed: \(baseline)") + return + } + + _ = try pushNewCommit(fixture: fix) // README only — outside the directory + + let result = fix.updater.updateGitPack( + entry: baselineEntry, packPath: packPath, registry: fix.registry + ) + + guard case let .updated(_, diff) = result else { + Issue.record("Expected .updated, got \(result)") + return + } + #expect(diff?.isEmpty == true) + } + @Test("diff is empty when the update touches only unreferenced files") func diffEmptyForUnreferencedFileChange() throws { let fix = try makeFixture()