ISSUE-70: Show component-level diff when mcs pack update updates a pack - #361
Merged
Conversation
- 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
There was a problem hiding this comment.
Pull request overview
This PR adds an inline, component-level change summary to pack update flows so users can see what changed in an updated external pack (components/templates/prompts/doctor checks/configure script), not just the new commit SHA. It does this by snapshotting the pre-update manifest + referenced file hashes, computing a structured diff after fetching, and rendering that diff under mcs pack update and mcs update output.
Changes:
- Introduces
PackSnapshot+PackDiffto compute and render added/modified/removed entries (with truncation for long diffs). - Extends
PackUpdater.UpdateResult.updatedto carry an optional diff, and prints the diff in bothmcs pack updateandmcs update. - Centralizes “referenced pack file” path normalization via
ExternalPackManifest.referencedPathsand addsEquatableconformance needed for diffing.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/MCSTests/PackUpdaterTests.swift | Adds integration-style tests asserting update results include expected diffs (added/removed/modified/unreferenced/no-diff-available). |
| Tests/MCSTests/PackDiffTests.swift | Adds pure tests for diff construction and rendering behavior (ordering, paths, truncation). |
| Tests/MCSTests/ExternalPackManifestTests.swift | Adds a “surface drift” tripwire to keep diffing + referenced-path traversal in sync with manifest properties. |
| Sources/mcs/TechPack/PromptDefinition.swift | Adds Equatable to enable prompt diffing. |
| Sources/mcs/Sync/PackUpdater.swift | Captures a pre-fetch snapshot and returns .updated(updatedEntry, diff:) with optional diff; currently emits a “no summary” message. |
| Sources/mcs/Sync/PackDiff.swift | New diff/snapshot implementation and renderer for change summaries. |
| Sources/mcs/ExternalPack/PackHeuristics.swift | Switches heuristic referenced-path use to manifest.referencedPaths and simplifies root-copy detection. |
| Sources/mcs/ExternalPack/ExternalPackManifest.swift | Adds normalized referenced-path helpers and referencedPaths; adds Equatable to multiple manifest model types. |
| Sources/mcs/Commands/UpdateCommand.swift | Prints per-pack diffs under mcs update success lines when available and non-empty. |
| Sources/mcs/Commands/PackCommand.swift | Prints per-pack diffs under mcs pack update success lines when available and non-empty. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+168
to
+172
| diff = PackDiff.between(old: beforeSnapshot, new: after) | ||
| } else { | ||
| diff = nil | ||
| output.dimmed("\(entry.displayName): no change summary available") | ||
| } |
Comment on lines
+29
to
+39
| 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 | ||
| } | ||
| } |
- 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
…ate 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
Sources/mcs/Sync/PackDiff.swift:194
PackDiffcurrently diffssupplementaryDoctorChecksonly by the YAML declaration (Equatable) and never consults file hashes, so content-only edits to a.shellScriptdoctor check’s script file (and itsfixScriptfile) won’t show up as~ modifiedchanges. This contradicts the PR goal of reporting edits to referenced files, and it also won’t work until those doctor-check script paths are included inExternalPackManifest.referencedPaths/PackSnapshothashing.
entries += diffKeyed(
kind: .doctorCheck,
old: old.manifest.supplementaryDoctorChecks ?? [],
new: new.manifest.supplementaryDoctorChecks ?? [],
key: \.name
)
Sources/mcs/Sync/PackDiff.swift:73
- The
PackDiffdoc comment lists only components/templates/doctor checks, but the implementation also diffs prompts and the configure script. This makes the documentation misleading for callers/users reading the source.
/// What changed in a pack between two revisions, at the granularity a pack author declares:
/// components, template sections, and supplementary doctor checks.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #70.
mcs pack updatereported only a commit SHA, so an update told you a pack had changed but never what changed. The only existing visibility was the trust prompt, which lists scripts, and only when trust is actually needed. Updates now print what moved — added, removed, and modified components, templates, prompts, doctor checks, and the configure script — where "modified" means either a rewritten declaration or an edited file that a declaration points at.Changes
mcs update; long lists cap at ten lines with a remainder count.Test plan
swift testpasses locallyswiftformat --lint .andswiftlintpass without violationsmcs sync,mcs doctor)Not verified against a live pack:
Environmentresolves home throughNSHomeDirectory(), which ignores aHOMEoverride, so a sandboxed run reads the real~/.mcsand updating for real would mutate the reviewer's own packs. To check by hand, push three commits to a scratch pack — one adding a component, one editing only a hook script body, one removing a component — then runmcs pack update <id>after each and expect a+, a~naming the script, and a-respectively.