Collection module loading fix - #2
Closed
Paul Payne (payneio) wants to merge 2 commits into
Closed
Conversation
…e directories for discovery
Brian Krabach (bkrabach)
added a commit
to microsoft/amplifier-module-resolution
that referenced
this pull request
Nov 4, 2025
Fixes two critical bugs in GitSource.resolve(): 1. Cache Key Collision: Include subdirectory in cache key hash to prevent different modules from same repo overwriting each other in cache 2. Path Resolution: Respect uv's behavior where #subdirectory= installs content FROM subdirectory TO target (doesn't recreate structure) Changes: - sources.py:123-127 - Include subdirectory in cache key generation - sources.py:132 - Return cache_path directly (uv already installed here) - sources.py:150 - Update error message for clarity - test_sources.py - Add tests demonstrating bugs fixed - test_sources.py:164-188 - Update old test to match new behavior - SPECIFICATION.md - Document correct cache key generation - README.md - Update subdirectory examples and behavior notes - DISCOVERIES.md - Add entry with prevention patterns Impact: Enables collection + module coexistence pattern where both live in same repo with different subdirectories. Fixes tutorial-analyzer and other toolkit tools using subdirectory pattern. Testing: - All 26 existing tests pass - 2 new tests demonstrate bugs fixed - Verification script confirms unique cache keys and correct paths - Backward compatible (non-subdirectory modules unaffected) Original-Author: Paul Payne <paul@payne.io> GitHub: @payneio PR: microsoft/amplifier-app-cli#2 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Collaborator
|
Thank you Paul Payne (@payneio) - applied to new libs that are split across separate repos now. |
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.
This pull request introduces improvements to resource discovery and context handling in the Amplifier CLI, as well as a fix for cache key generation for git-sourced modules. The most important changes focus on making resource directories more discoverable after package installation, ensuring profile markdown is added to context, and preventing cache collisions for modules with subdirectories.
Resource discovery improvements:
profiles,context,agents,scenario-tools, andmodulesare now symlinked from package subdirectories to the collection root during installation, making them easier to discover and use.Context handling enhancements:
Module resolution and caching fixes:
Discussion:
Problem Discovery
Initial symptom: The amplifier-collection-issues tool failed to load with the error:
Failed to load module 'tool-issue': Module 'tool-issue' found at /home/payne/.amplifier/module-cache/6dc3a016d9d1/main but failed to load
The error was confusing because:
Root Cause Analysis
Through investigation, we discovered two related bugs in GitSource's resolve() method:
Bug 1: Cache Key Collision
Problem: The cache key only used url@ref, ignoring the subdirectory component:
Original (buggy)
cache_key = hashlib.sha256(f"{self.url}@{self.ref}".encode()).hexdigest()[:12]
Why this breaks: When installing from the same repo:
Both got the same cache key (6dc3a016d9d1), causing the module to overwrite the collection or vice versa.
Bug 2: Incorrect Path Appending
Problem: After uv installed the package, code incorrectly appended the subdirectory path:
Original (buggy)
final_path = cache_path / self.subdirectory if self.subdirectory else cache_path
return final_path
Why this breaks: When using uv pip install with #subdirectory=path, uv installs the package content FROM the subdirectory TO the target directory directly. It doesn't recreate the subdirectory
structure in the target.
So the code was looking for:
~/.amplifier/module-cache/6dc3a016d9d1/amplifier_collection_issues/modules/tool-issue/
But uv had actually installed to:
~/.amplifier/module-cache/6dc3a016d9d1/
Solution Design
We chose a minimal, mechanism-focused fix aligned with kernel philosophy:
Fix 1: Include Subdirectory in Cache Key
Fixed
cache_key_input = f"{self.url}@{self.ref}"
if self.subdirectory:
cache_key_input += f"#{self.subdirectory}"
cache_key = hashlib.sha256(cache_key_input.encode()).hexdigest()[:12]
Why: Each unique combination of repo+ref+subdirectory now gets its own cache directory. No more collisions.
Result:
Fix 2: Remove Path Appending
Fixed
return cache_path # Don't append subdirectory - uv already installed to cache_path
Why: Respect uv's behavior. When #subdirectory= is specified, uv installs the package content FROM that subdirectory TO the target directly.
Added comments to make the uv behavior explicit:
Note: uv installs the package content FROM subdirectory TO cache_path directly
It does NOT create the subdirectory structure in the target
Why This Solution
- Fixes mechanism (cache key generation, path resolution)
- Doesn't add policy (doesn't change what gets cached or when)
- Maintains backward compatibility (existing non-subdirectory modules unaffected)
- amplifier run --profile amplifier-collection-issues:issue-aware "list issues" ✅ worked
- Tool loaded successfully and listed 30 issues
- Both collection and module in cache with unique keys
Alternative Approaches Considered
Why not use separate cache directories for collections vs modules?
Why not change how uv is called?
Why not add more complex path resolution logic?
Impact
This was a textbook "fix the mechanism" change - we identified incorrect assumptions in the cache key generation and path resolution, fixed them to match actual tool behavior (uv), and
restored correct operation without adding complexity.