Skip to content

fix: allow identical copyPackFile source across components#325

Merged
bguidolim merged 1 commit intomainfrom
bruno/fix-duplicate-dest-identity-validator
Apr 11, 2026
Merged

fix: allow identical copyPackFile source across components#325
bguidolim merged 1 commit intomainfrom
bruno/fix-duplicate-dest-identity-validator

Conversation

@bguidolim
Copy link
Copy Markdown
Collaborator

Summary

PR #320 introduced intra-pack duplicate-destination validation that incorrectly rejected a legitimate pattern: one physical hook script registered against multiple hook events. Because hookEvent is singular per component, authors must declare N components — all copying the same source to the same destination — which is an identity, not a collision. This PR tightens the validator so it only throws when sources actually differ.

Changes

  • ExternalPackManifest.validate() now tracks (componentID, source) per destination group. For groups with more than one entry, an allSatisfy check collapses the identity case (every entry copies the same source → benign, skipped). Real conflicts (distinct sources for the same destination) still throw ManifestError.duplicateDestination.
  • Replaced the "generic" string fallback with ExternalCopyFileType.generic.rawValue to drop the stringly-typed literal.
  • Removed a force-unwrap on the dictionary lookup by iterating sorted key-value pairs directly.
  • Added allowSameSourceDestinationAcrossHookEvents test covering two hook components sharing both source and destination. Existing rejectDuplicate* tests still cover the real-collision branch.

Context

Real-world hit: the mcs-memory pack declares hook-sync-memories (SessionStart) and hook-reindex-memories (UserPromptSubmit), both copying hooks/sync-memories.shsync-memories.sh. After PR #320, mcs sync --global warned:

[WARN] Skipping pack 'memory': Invalid manifest for pack 'memory':
  Duplicate copyPackFile destination 'sync-memories.sh' (fileType: hook)
  in components: memory.hook-sync-memories, memory.hook-reindex-memories

This is the same file being registered against two hook events, not two files fighting for one destination. The validator should only catch the latter.

A more architecturally elegant alternative — letting hookEvent accept an array so one component registers for multiple events — was considered but deferred. That change ripples through ~13 files (HookRegistration.event, encoder/decoder, sync strategies, PackArtifactRecord.hookCommands, doctor checks, export/discovery, collision resolver) and deserves its own PR.

ExternalCopyPackFileConfig has exactly three fields (source, destination, fileType?) with no hidden mode/permissions/ownership, so source equality is a complete identity check.

Test plan

  • swift test --filter MCSTests.ExternalPackManifestTests — 92/92 pass
  • swift test — 978/978 pass
  • swiftformat --lint . and swiftlint pass without violations
  • End-to-end: swift run mcs pack validate ~/Developer/mcs-memory[OK] memory — all checks passed
  • Existing conflict tests (rejectDuplicateCopyPackFileDestinations, rejectDuplicateGenericFileTypeDestinations) still reject the real-collision branch
Checklist for engine changes

- Treat same-source duplicate (destination, fileType) as identity, not conflict
- Unblocks one hook script registered against multiple hook events
- Add test for two hook components sharing source and destination
@bguidolim bguidolim merged commit 8860a5c into main Apr 11, 2026
5 checks passed
@bguidolim bguidolim deleted the bruno/fix-duplicate-dest-identity-validator branch April 11, 2026 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant