fix: own the module cache, and refresh it by resolving refs instead of deleting clones - #142
Conversation
amplifier-agent's module clones were written into ~/.amplifier/cache, a tree owned by amplifier-app-cli. Nothing in this repo referenced .amplifier to cause it: foundation resolves its storage root from AMPLIFIER_HOME and falls back to ~/.amplifier, and this app never set the variable. Bind AMPLIFIER_HOME to <agent home>/foundation before amplifier_foundation is imported, so foundation's own resolver writes into a root this app owns. Also relocate the recipes session_dir out of ~/.amplifier/projects, add a doctor check that fails if the bind regresses, and generalise PR #141's one-time stale-clone migration into the permanent cold-cache refresh -- now safe, because the directory is ours to delete from.
The hook writes to <agent home>/state/workspaces but its readers resolve the root only from AMPLIFIER_CONTEXT_INTELLIGENCE_BASE_PATH, defaulting to ~/.amplifier/projects. Unset, captures are written to one tree and looked for in another; the hook warns about exactly this at runtime. Set when absent, leaving a deliberate user setting alone.
provider-anthropic defaults its cross-process rate-limit state file to
os.path.expanduser("~") + ".amplifier" — built from the raw home directory, so
the AMPLIFIER_HOME bind cannot reach it. It does read config
rate_limit_state_path first, so default that at mount time.
Applied in build_provider_entry rather than bundle.md because provider entries
in the manifest carry no config: both call sites clear mount_plan[providers]
and mount exactly one provider via inject_provider.
The bump is not cosmetic. `update` compares versions and exits early with "Already up to date" when they match, so without a new release tag the reinstall never runs and no user receives this fix. Same coupling PR #141 identified, for a different reason: #141 needed the bump because post-install would early-return on a warm cache; this needs it because `update` never gets that far.
Completes the absorption of #141 so it can be closed: - uv.lock version bump, which #141 carried and this branch had missed - #141's stale-clone diagnosis folded into the 0.14.2 changelog entry - legacy clones in ~/.amplifier/cache surfaced by doctor and removable via cache clear --legacy #141 deleted those clones unconditionally. That was right while the directory was the one amplifier-agent used; after the relocation it is foundation's default for every Amplifier app, keyed with no per-app namespacing, so on a machine running app-cli they are its live clones and indistinguishable from our leftovers. Reported and offered rather than deleted.
Removes the doctor advisory and the `cache clear --legacy` flag added in the previous commit. amplifier-app-cli has a large user base, and for those users ~/.amplifier/cache is live, not stranded. From inside amplifier-agent the two populations are indistinguishable — clones are keyed sha256(git_url@ref)[:16] with no per-app namespacing — so any cleanup affordance is, for a substantial fraction of the people who would see it, a button that breaks a different application. A caveat in the help text does not fix that; it documents the damage. Asymmetric costs: not offering cleanup wastes some disk for agent-only users, who can rm -rf it themselves. Offering it breaks app-cli installs on this tool's suggestion. Reporting is dropped too — a report whose only actionable follow-up is a dangerous manual delete is the same footgun with extra steps.
Foundation keys each clone on sha256(git_url@ref) and reuses any directory that exists. With @main the key never changes while the commit it names does, so one directory serves every commit that branch will ever have. That is correct reasoning on a false premise: main is a pointer, not an identity. Previous fixes attacked the consequence by deleting the directory. This fixes the premise -- resolve the ref to its current commit via ls-remote before prepare(), so the key identifies the content. Unmoved branch resolves to the same key and downloads nothing; a moved one gets a new directory and clones fresh; offline leaves the ref floating and reuses the existing clone. No deletion anywhere in the refresh path. Not pinning: bundle.md still says @main and is never rewritten. Resolution happens on the user's machine, so module fixes still land without an engine release. Applied through Bundle.prepare(source_resolver=...), a documented foundation seam this app had never used. Also makes `update` check module drift when the engine is already current -- previously it exited early, so the only route to a module fix was an engine release that existed solely to move the cache.
Caught in DTU verification. Leaving the ref floating when a remote is unreachable was only equivalent to the old behaviour while no SHA-keyed directory existed. Once one does, @main and @<commit> hash to different cache keys, so the fallback pointed foundation at a directory that was never created and sent it to clone -- exactly when the network is gone. Fall back to the commit recorded in the local clone metadata instead, which reproduces the key of the directory that is actually there.
Salil Das (sadlilas)
changed the title
Review: amplifier-agent#142 (own the module cache, refresh by resolving refs)Date: 2026-08-24 VerdictThe central claim holds. amplifier-agent operates from Two issues should be resolved before merge, one of which is pre-existing and What was verified, and howRan the e2e suite inside a DTU with the skills#61 branch mirrored in, so the The 4 skips are the Direct observation inside the container: The 86 MB module cache, every module clone, and provider rate-limit state all The companion fix was confirmed to be the code actually executing, read off the With that in place the agent's remote skill clones land at Should be addressed before merge1. A live read of app-cli's tree, not covered by the guaranteeThis is pre-existing, introduced by It matters here because this PR is what establishes the guarantee, and the The spec's "Known residuals" section already does exactly the right thing for 2. Two bundle.md literals silently ignore AMPLIFIER_AGENT_HOME
With the variable unset they agree with That is the same writer/reader split root this PR just fixed, relocated one Severity is lower than issue 1: this is not app-cli pollution, so the headline Suggested fix: resolve both through the same accessor as everything else. The |
…li's tree Both raised by @DavidKoleczek on #142. 1. provider_sources.py read ~/.amplifier/openai-chatgpt-oauth.json directly. Pre-existing (PR #128), but this PR is what establishes the never-read-app-cli guarantee, and that was the read half. The provider accepts a token_file_path config key (provider.py:103), the same seam already used for anthropic's rate-limit file, so this is fixable rather than merely documentable. Includes a copy-forward so an existing login is not silently invalidated into a device-code prompt; copies, never moves. 2. bundle.md declared session_dir and base_path as literal YAML strings that nothing expands, so both ignored AMPLIFIER_AGENT_HOME. With it unset they equalled state_root() and the divergence was invisible -- including to every test either of us ran. Set it and the context-intelligence writer and reader split apart again, one level below where foundation_home fixed it. Now injected at runtime from state_root(), the same technique the vendored skills/modes dirs already use.
Raises MIN_AGENT_VERSION to 0.15.0; AGENT_PINNED_REF and AGENT_HARD_FLOOR follow. 0.15.0 is the first agent release that owns its own storage tree: it binds AMPLIFIER_HOME to ~/.amplifier-agent/foundation before importing amplifier_foundation, so module clones, provider rate-limit state and the ChatGPT OAuth token stop landing in ~/.amplifier, a directory owned by amplifier-app-cli and shared unnamespaced with every other Amplifier application on the machine. It also resolves floating @main module refs to concrete commits before preparing the bundle, so an upstream module fix reaches an existing install. Upstream: microsoft/amplifier-agent#142, released as v0.15.0.
2 participants
Summary
Two problems with one root, fixed together because fixing either alone leaves the other broken.
~/.amplifier— a tree owned by amplifier-app-cli.Ships as 0.15.0.
Problem 1 — the dependency was invisible to grep
A repo-wide search for
.amplifierreturns 94 hits and every one is skills/modes discovery or an e2e fixture. Nothing in this repository caused the coupling. It was created by an absent argument atbundle/loader.py:Neither call passes a cache root, so foundation's default applied —
get_amplifier_home(), which readsAMPLIFIER_HOMEand otherwise falls back to~/.amplifier. This repo never set it:grep -rn "AMPLIFIER_HOME" .returned 0 matches.Fix: bind
AMPLIFIER_HOMEto<agent home>/foundationbeforeamplifier_foundationis imported. Ordering is load-bearing —session/finder.py:36computes a~/.amplifier-derived constant at import time — so the bind runs in the package__init__of bothamplifier_agent_libandamplifier_agent_http.The bind is unconditional. A user who exported
AMPLIFIER_HOMEfor app-cli would otherwise silently re-couple — the same bug, appearing only for users who customised their setup. Overrides that are honoured:$AMPLIFIER_AGENT_FOUNDATION_HOME(subtree),$AMPLIFIER_AGENT_HOME(whole tree).Also relocated: recipe session state (the only write into app-cli's tree), the context-intelligence reader root, and
provider-anthropic's rate-limit file — the last built fromexpanduser("~")+ a literal".amplifier", so the bind can't reach it; itsrate_limit_state_pathconfig is defaulted at mount time instead.Problem 2 — the cache key was being lied to
Foundation keys each clone at
sha256(git_url@ref)[:16]and returns any directory that already exists — no fetch, no ref comparison, no commit check.With
@mainthe key issha256(url@"main").mainis a pointer, not an identity. The string never changes while the commit it names does, so one directory serves every commit that branch will ever have — in practice the first one, for the life of the machine.Foundation's reuse rule is correct reasoning on a false premise. Every previous remedy — #141, and earlier revisions of this PR — attacked the consequence by deleting the directory. This attacks the premise.
Before
prepare(), resolve each floating ref to the commit it currently points at (git ls-remote— refs only, no repository data, no auth, measured at 0.386s) and rewrite the source to that SHA:This is not pinning.
bundle.mdstill says@mainand is never rewritten in the repository. Resolution happens on the user's machine against the branch as it stands at that moment, so a provider fix reaches users with no amplifier-agent release — preserving the non-goal indocs/spec/bundle-and-cache.md. Pinning at release time was considered and rejected precisely because it would gate every module fix on an engine release.Applied through
Bundle.prepare(source_resolver=...)— a documented foundation extension point ("allows app-layer source override policy to be applied before activation") that amplifier-agent had never used, and that amplifier-app-cli already uses for its own settings overrides. Foundation supports the rewritten form natively via_clone_at_commit()for full 40-character SHAs.No deletion remains anywhere in the refresh path. Directories are immutable and content-addressed: one either is that commit or does not exist, so there is no half-updated state and no interrupted-fetch failure mode.
updateno longer ignores modulesModules move independently of engine releases, but
updateexited early on a version match — so the only route to a module fix was an engine release that existed solely to move the cache. It now re-resolves refs, compares against the commit foundation recorded beside each clone, names what moved, and re-primes. When nothing moved it costs one refs-only round trip per module and zero downloads.Pruning
A moved branch leaves its predecessor unreferenced. After a successful prepare, directories for repositories resolved in that pass whose commit is no longer current are removed. Conservative: only repositories resolved this pass are considered, the current commit is always kept, failures are ignored. Disk reclamation, not correctness.
Verification — clean DTU, no Gitea
Clean Incus container. Code fetched from the GitHub remote inside the container, this PR applied as a patch. Baseline: both trees absent.
1. Refs are resolved to commits
{ "ref": "7092d5d96374ee8846162dc7ab3e907c1dbcdaa7", "commit": "7092d5d96374ee8846162dc7ab3e907c1dbcdaa7", "git_url": "https://github.com/microsoft/amplifier-module-provider-anthropic" }refwas"main"before this change. 27/27 clones keyed by commit; none left floating.2. Unchanged branches download nothing
3. Drift is detected precisely
Planted a stale commit in exactly one clone's metadata, then ran
updatewith the engine already current:One module named. The other 26 untouched.
4. Offline degrades correctly — and this caught a real bug
The first implementation left unresolvable refs floating, described as "behaves exactly as it does today". That was true only while no SHA-keyed directory existed. Once one does, the two keys differ:
so the fallback pointed foundation at a directory that was never created and sent it to clone — precisely when the network is gone. Worse than the old behaviour, not equal to it. Fixed to fall back to the commit recorded in local clone metadata. Re-tested with DNS broken:
5. Isolation
An earlier isolation measurement passed prematurely — it ran before any successful agent turn. A real turn then created
~/.amplifiervia two paths outside foundation'sAMPLIFIER_HOMEcontract:provider-anthropic's rate-limit file (fixed here) andtool-skills' hardcoded cache dir (fixed upstream — see dependency below).Supersedes #141
#141 ships four files. All are accounted for:
pyproject.tomlversion bumpuv.lockbumpCHANGELOG.mdstale-clone diagnosispost_install.pyclone-deleting migrationClones left in
~/.amplifier/cacheare left strictly alone — not deleted, not reported, no flag to remove them. That directory is foundation's default for every Amplifier application, keyed with no per-app namespacing, so on a machine running amplifier-app-cli they are its live clones, indistinguishable from our leftovers. An earlier revision of this PR added adoctoradvisory and acache clear --legacyflag; both were removed. For app-cli's large user base that affordance is a button that breaks a different application, and a caveat in help text documents the damage rather than preventing it. Users who don't run app-cli canrm -rf ~/.amplifierthemselves.#141's open follow-up remains open and is not addressed here:
ModuleActivator._install_dependencies()returns on a distribution-name match before thepyproject.tomlfingerprint check that would catch changed dependency constraints. Both guards sit behindif not force, and no production caller in either repo ever passesforce=True.Compatibility ledger
Enumerated once from the org and frozen. Three direct consumers; 17 candidates refuted.
serve chat-completions, queries/v1/models/v1/models→claude-haiku-4-5-20251001;/v1/chat/completions→OPENCODE-SURFACE-OK;~/.amplifierabsent afterenv -iand exactlyPATH, HOME, USER, LANG, TERM, TMPDIR: roots resolved correctly. The bind derives fromHOME, not an inherited variable/home/node/.amplifier/cachefor "legacy bundle install machinery"mkdiris now dead weightPropagation is a separate axis from compatibility — none of these breaks, but each receives the fix differently: nanoclaw pins a SHA (
ARG AMPLIFIER_AGENT_REF=cef3fb53…) and needs a bump; paperclip installs latest and gets it free; opencode's version-floor self-heal means raising its floor is a policy choice, not a correctness need.Named intentional exceptions
provider_sources.py:440— reads~/.amplifier/openai-chatgpt-oauth.jsonto report whether a device-code login happened. The module hardcodes that same literal (oauth.py:60), so relocating would desynchronise reader from writer. Read-only; never emits token material.<cwd>/.amplifier/skills,~/.amplifier/skills,.amplifier/modes. Deliberate cross-harness interop.Known residuals (upstream)
registry.py:453—Path.home()/".amplifier"/"cache"as a walk stop-boundaryresolved.source_rootis falsy — not the normal pathsession/finder.py:36— module-levelDEFAULT_SESSIONS_ROOTconfigurator/_state_manager.py:756Checks
No test file added:
tests/holds e2e DTU suites only, per[tool.pytest.ini_options]. The repo's own e2e profile is Gitea-based and was deliberately not used; verification ran in a clean container fetching from the real remote.Dependency
Full isolation requires microsoft/amplifier-bundle-skills#61 —
tool-skillshardcodes~/.amplifier/cache/skillsand passes nocache_dir, so it cannot be fixed from this repository. Verified: with #61 applied,~/.amplifieris never created; without it, only that one subdirectory appears. Everything else in this PR holds either way.Merge order: #61 first (skills float on
@main, so it lands on next prepare), then this, taggedv0.15.0.