#338: Filter non-material upstream commits from update notifications#343
Merged
#338: Filter non-material upstream commits from update notifications#343
Conversation
- Shallow git fetch + name-only diff after ls-remote detects a new SHA; when every changed path is deny-listed infrastructure (README, LICENSE, CHANGELOG, .github/, node_modules/, .build/, Makefile), the notification is suppressed and the registry baseline advances so the same commits don't re-trigger on future cooldown windows. - techpack.yaml changes always surface unfiltered — manifest edits can swap hook scripts and MCP commands, so suppressing them would be a supply-chain risk. - Fetch or diff failure surfaces the notification unfiltered (never-hide invariant); pipeline covered by pure classifier + mock-shell orchestrator + end-to-end PackRegistryFile write-back tests.
This was referenced Apr 26, 2026
…y, doc-rot - Collapse PackCheckOutcome+SHAAdvance into a private enum (.emit / .advance) so the unreachable "both set" state can no longer produce a stuck-update bug; UpstreamChange.unknown gains an UnknownReason payload (missingClone/fetchFailed/diffFailed) for telemetry; classifyDiffPaths uses .filter; never-hide invariant comment now correctly scopes to classifyUpstreamChange (the earlier ls-remote/parse-error guard is documented as a separate "couldn't determine if there's a change at all" path). - MockShellRunner is now thread-safe (NSLock around runResults / runCalls / shellResults) and gains runResultsByFirstArg for argument-keyed dispatch — required for parallel tests where DispatchQueue.concurrentPerform interleaves calls non-deterministically; preparePackDir lifted to TestHelpers; new tests cover multi-pack parallel classification, custom entry.ref propagation, missingClone path, local-pack filter at checkPackUpdates boundary, registry-write failure contract (suppression sticks but baseline doesn't advance), and cross-invocation persistence; existing tests pin exact fetch/diff arguments and working-directory. - applyRegistryAdvances catch now emits to stderr under MCS_DEBUG so silent registry-write failures aren't invisible during development; trim issue-#338 references from doc-comments (UpstreamChange, checkPackUpdates, withCommitSHA, infrastructureFilesForUpdateCheck) and drop the codebase-convention narration on UpdateChecker.shell.
There was a problem hiding this comment.
Pull request overview
Reduces spurious “pack update available” notifications by adding a post-SHA-change noise filter that suppresses infra-only upstream commits while preserving supply-chain visibility for techpack.yaml changes.
Changes:
- Add an UpdateChecker noise filter using shallow
git fetch+git diff --name-only, suppressing updates when only deny-listed infra paths change. - Advance
~/.mcs/registry.yaml’scommitSHAon suppressed changes so the same infra-only commits don’t re-trigger on subsequent cooldown windows. - Add targeted test coverage and update docs to describe the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Sources/mcs/Core/UpdateChecker.swift |
Implements diff-path classification, orchestrated fetch/diff, and registry baseline advancement. |
Sources/mcs/ExternalPack/PackHeuristics.swift |
Exposes ignoredDirectories and introduces infrastructureFilesForUpdateCheck (excluding techpack.yaml). |
Sources/mcs/ExternalPack/PackRegistryFile.swift |
Adds withCommitSHA(_:) helper for advancing registry baseline without moving checkout. |
Tests/MCSTests/UpdateCheckerTests.swift |
Adds unit + orchestrator tests for classification and baseline advancement behavior. |
Tests/MCSTests/TestHelpers.swift |
Makes MockShellRunner thread-safe and adds preparePackDir helper for git-working-dir tests. |
Tests/MCSTests/PackHeuristicsTests.swift |
Verifies update-check infra set excludes techpack.yaml and checks ignored dir set. |
docs/cli.md |
Documents the noise filter, manifest invariant, and never-hide-on-error behavior. |
docs/architecture.md |
Adds UpdateChecker architecture notes including noise-filter semantics and caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Mirror PackFetcher.update's nil-ref pattern: when entry.ref == nil, fetch without a positional ref and diff against origin/HEAD instead of passing HEAD as a remote ref (more reliable across git servers, matches the rest of the codebase).
- Validate entry.ref via the new module-level isValidGitRef predicate (extracted from PackFetcher.validateRef) before invoking ls-remote / fetch / diff — registry-corrupted refs starting with `-` would otherwise be interpreted as git options. Invalid refs silently skip the pack, consistent with ls-remote network-failure behavior. parseRemoteSHA now prefers the peeled `^{}` line for annotated tags so the registry SHA stays in sync with rev-parse HEAD.
- Centralize the MCS_DEBUG env check on Environment.isDebugMode; route preparePackDir through Environment.packsDirectory; trim verbose MockShellRunner doc-comments; drop a duplicate inline comment.
There was a problem hiding this comment.
Pull request overview
This PR reduces noisy “pack update available” notifications by distinguishing material upstream pack changes from infrastructure-only commits, and advancing the registry baseline when the change is non-material so the same commits don’t re-trigger.
Changes:
- Add an UpdateChecker “noise filter” that runs a shallow
git fetch+git diff --name-onlyand suppresses notifications when only deny-listed infra paths changed (while treatingtechpack.yamlas always material). - Advance
~/.mcs/registry.yamlcommitSHAon suppressed (infra-only) updates so they don’t re-trigger. - Add/update unit tests and documentation describing the new behavior, plus small supporting utilities (ref validation predicate, PackHeuristics set split, test helper locking).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/cli.md | Documents update-check noise filter behavior and invariants. |
| docs/architecture.md | Adds UpdateChecker architecture notes covering the new noise filter. |
| Tests/MCSTests/UpdateCheckerTests.swift | Adds comprehensive tests for diff-path classification, orchestrator behavior, registry advancement, and SHA parsing. |
| Tests/MCSTests/TestHelpers.swift | Makes MockShellRunner safe for concurrent use and adds a helper to prepare pack directories. |
| Tests/MCSTests/PackHeuristicsTests.swift | Tests the update-check infra set excludes techpack.yaml and exposes ignored dirs. |
| Sources/mcs/ExternalPack/PackRegistryFile.swift | Adds withCommitSHA helper to update the registry baseline without moving the checkout. |
| Sources/mcs/ExternalPack/PackHeuristics.swift | Exposes ignored directories and introduces infrastructureFilesForUpdateCheck (excluding techpack.yaml). |
| Sources/mcs/ExternalPack/PackFetcher.swift | Refactors ref validation into a reusable predicate (isValidGitRef). |
| Sources/mcs/Core/UpdateChecker.swift | Implements noise filtering, ref validation for git invocations, baseline advancement writes, and improved ls-remote SHA parsing for annotated tags. |
| Sources/mcs/Core/Environment.swift | Adds Environment.isDebugMode gate for developer-facing diagnostics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Wrap applyRegistryAdvances in withFileLock; lock contention skips silently and the next check retries (closes data-loss race vs. concurrent mcs pack add/update) - Trim diff paths with .whitespacesAndNewlines so trailing \r from core.autocrlf=true CRLF output doesn't bypass the deny-list; regression test added - Make PackEntry fields var so withCommitSHA copies-and-mutates instead of re-listing every stored property; surface invalid-ref skip via MCS_DEBUG; soften isValidGitRef doc-comment
There was a problem hiding this comment.
Pull request overview
Implements an UpdateChecker “noise filter” so upstream pack commits that only touch infrastructure files (README/CI/etc.) don’t trigger “pack update available” notifications, while ensuring techpack.yaml changes always surface for supply-chain safety.
Changes:
- Adds
git fetch --depth 1+git diff --name-only-based classification to suppress infra-only upstream commits and (when suppressed) advanceregistry.yaml’s baseline SHA. - Hardens ref handling via shared
isValidGitRefvalidation and improvesls-remoteparsing for annotated tags (prefer peeled^{}commit SHA). - Adds comprehensive Swift Testing coverage and updates docs to describe the new behavior.
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 |
|---|---|
| docs/cli.md | Documents the update-check noise filter behavior and invariants (never-hide on failure, techpack.yaml always material). |
| docs/architecture.md | Adds architecture-level description of UpdateChecker’s new noise filter and caching behavior. |
| Sources/mcs/Core/UpdateChecker.swift | Core implementation: diff-path classifier, fetch/diff orchestration, registry baseline advancement under lock, peeled-SHA parsing. |
| Sources/mcs/Core/Environment.swift | Adds Environment.isDebugMode gate for developer-facing stderr diagnostics. |
| Sources/mcs/ExternalPack/PackHeuristics.swift | Exposes ignore-dir list and adds infrastructureFilesForUpdateCheck that excludes techpack.yaml. |
| Sources/mcs/ExternalPack/PackRegistryFile.swift | Makes PackEntry mutable to support copy-and-mutate helper withCommitSHA. |
| Sources/mcs/ExternalPack/PackFetcher.swift | Refactors ref validation to reuse the new shared isValidGitRef predicate. |
| Tests/MCSTests/UpdateCheckerTests.swift | Adds unit + orchestration tests for noise classification, ref validation, failure fallthrough, and peeled tag SHA parsing. |
| Tests/MCSTests/TestHelpers.swift | Makes MockShellRunner concurrency-safe (lock + keyed dispatch) and adds helper to prepare pack clone dirs in tests. |
| Tests/MCSTests/PackHeuristicsTests.swift | Adds tests asserting update-check infra set excludes techpack.yaml and retains expected infra entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`resolvedPath` only validates the path shape — if the user `rm -rf`'d the clone, current code spawns git with a non-existent cwd and returns `.fetchFailed`. Add a `fileExists` check so the case classifies as `.missingClone` (its intended meaning), avoiding the doomed subprocess. Notification still surfaces — both reasons map to `.emit` at the call site.
Switch the parallel writes to UnsafeMutableBufferPointer so each thread touches a single element address with no Array COW/refcount machinery in the hot path. nonisolated(unsafe) is still needed (UnsafeMutableBufferPointer is not Sendable) but the data-race surface area is now obviously a flat slab of memory.
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
Upstream pack repos accumulate README, LICENSE, CI, and
.github/commits that never affect whatmcs syncinstalls. Every one of these fires the SessionStart hook's "pack update available" banner, training users to ignore it. This PR filters those commits out so the notification only fires on commits that actually change the install surface — closes the first half of #338.Changes
git ls-remotedetects a new SHA, the update check now runs a shallow fetch + name-only diff in the local pack clone and classifies the changed-path list. If every path is deny-listed infrastructure (README, LICENSE, CHANGELOG, CONTRIBUTING,.gitignore,.editorconfig,.dockerignore, package/lock files,Makefile,Dockerfile,.git/,.github/,.gitlab/,.vscode/,node_modules/,__pycache__,.build/), the notification is suppressed and the local pack-registry baseline advances to the new remote SHA so the same commits don't re-trigger on future cooldown windows. Diff output is trimmed with.whitespacesAndNewlinesso trailing\rfrom CRLF-configured git doesn't bypass the deny-list.techpack.yamlchanges are always material, regardless of what else is in the diff — a manifest edit can swap hook scripts, MCP server commands, or the install surface entirely, so silent suppression would be a supply-chain risk.entry.refis validated viaisValidGitRefbefore any git invocation. A corrupted registry ref (e.g. starting with-) is skipped without invoking git.withFileLock(at: ~/.mcs/lock). The SessionStart hook is not aLockedCommand(it stays non-blocking by design); without the lock it could racemcs pack add/updateand clobber a just-added entry. On contention the advance is skipped and the next check retries.parseRemoteSHAwalks everyls-remoteline and prefers the peeled^{}commit when present, so the registry stores the commit SHA (not the tag-object SHA). Without this, the registry would desync from the checkout'srev-parse HEADand triggerPackUpdater's "disk ahead of registry" recovery path.ignore:field ontechpack.yamland a validate-time remediation hint ship in follow-up PRs.Test plan
swift testpasses locallyswiftformat --lint .andswiftlintpass without violationsmcs sync,mcs doctor)Manual end-to-end:
mcs pack add <a test pack>; note thecommitSHAin~/.mcs/registry.yaml.~/.mcs/update-check.jsonto bust the cache; runmcs doctor→ expect no "pack update available" notification, and expectcommitSHAin~/.mcs/registry.yamlto advance to the new upstream SHA.mcs doctor→ expect the notification.techpack.yamlonly; bust the cache; runmcs doctor→ expect the notification.Checklist for engine changes
CLAUDE.md,docs/,techpack.yamlschema inExternalPackManifest.swift)