diff --git a/CHANGELOG.md b/CHANGELOG.md index ff1f5fed..c3a7d47b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,128 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.15.0] — 2026-08-24 + +### Fixed + +- **amplifier-agent no longer shares a module cache with amplifier-app-cli.** Module git + clones were written into `~/.amplifier/cache`, a tree owned by a different application. + Nothing in this repository referenced `.amplifier` to cause it: `amplifier_foundation` + resolves its storage root from `AMPLIFIER_HOME` and falls back to `~/.amplifier`, and this + app never set the variable — so the coupling was created by an *absent* argument at the + `load_bundle()` call, invisible to any search for the path itself. `AMPLIFIER_HOME` is now + bound to `/foundation` before `amplifier_foundation` is imported, so + foundation's own resolver writes into a root this application owns. Ordering is + load-bearing: `amplifier_foundation.session.finder` computes a `~/.amplifier`-derived + constant at import time. Override with `$AMPLIFIER_AGENT_FOUNDATION_HOME` (this subtree) + or `$AMPLIFIER_AGENT_HOME` (the whole tree). Design: `docs/spec/foundation-cache-ownership.md`. + +- **`amplifier-agent update` now actually picks up upstream module fixes, without + re-downloading everything.** Module sources are declared at a floating `@main`, but + foundation caches each clone at `sha256(git_url@ref)` and returns any directory that already + exists — it never fetches into it. With `@main` the key never changes while the commit it + names does, so one directory served every commit that branch would ever have: in practice the + first one, for the life of the machine. An upstream fix never reached an existing install, and + reinstalling did not help, because the reinstall rebuilt from the same frozen clone. + + That is correct reasoning on a false premise. Foundation is told the identity of the content + is `main`; `main` is a pointer, not an identity. Rather than deleting directories to work + around the consequence, amplifier-agent now fixes the premise: before `prepare()`, each + floating ref is resolved to the commit it currently points at (`git ls-remote` — refs only, no + repository data, no authentication) and the source is rewritten to that SHA. Foundation then + keys on something that genuinely identifies the content, and its reuse rule becomes correct: + + - branch unmoved → same SHA → same directory → **nothing is downloaded** + - branch moved → new SHA → new directory → cloned fresh, automatically + - offline → ref left floating → existing clone reused, exactly as before + + **This is not pinning.** `bundle.md` still says `@main` and is never rewritten in the + repository; resolution happens on the user's machine against the branch as it stands at that + moment. Ship a provider fix and the next resolution picks it up with no amplifier-agent + release involved — preserving the non-goal recorded in `docs/spec/bundle-and-cache.md`. + + Applied through `Bundle.prepare(source_resolver=...)`, a documented foundation extension point + that amplifier-agent had simply never used. Foundation already supports the rewritten form + natively via its `_clone_at_commit()` path for full 40-character SHAs. + + Supersedes the one-time clone-deleting migration proposed in #141, whose diagnosis this + follows. No deletion remains anywhere in the refresh path. + +- **`update` no longer reports "already up to date" without checking modules.** Modules move + independently of engine releases, so the old early exit meant the only route to a module fix + was an engine release that existed solely to move the cache. `update` now re-resolves refs, + compares them against the commit foundation recorded beside each clone, reports which + repositories moved, and re-primes — downloading only what actually changed. When nothing has + moved it costs one refs-only round trip per module and no downloads at all. + +- **Superseded clone directories are pruned.** Content-addressed directories mean 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 by + construction: only repositories resolved in this pass are considered, the current commit is + always kept, and failures are ignored — this reclaims disk in amplifier-agent's own tree and + is not load-bearing for correctness. + +- **The ChatGPT OAuth token no longer lives in amplifier-app-cli's tree.** `provider_sources` + read `~/.amplifier/openai-chatgpt-oauth.json` directly — pre-existing, but it was the *read* + half of the coupling this release claims to remove, and so the one residual the guarantee + could not honestly cover. The provider accepts a `token_file_path` config key + (`provider.py:103`), the same seam already used for provider-anthropic's rate-limit file, so + the token now lives at `/openai-chatgpt-oauth.json`. A pre-0.15.0 login is + **copied forward** on first use so it keeps working; the original is never moved or deleted, + because it sits in a directory this application does not own. The legacy path is still read + as a fallback if the copy could not be made. Raised in review by @DavidKoleczek. + +- **`$AMPLIFIER_AGENT_HOME` is now honoured by every storage root.** Two `bundle.md` values — + the recipes `session_dir` and context-intelligence's `base_path` — were literal YAML strings + that nothing expanded. With the variable unset they happened to equal `state_root()`, so the + divergence was invisible in normal use and in every test. Set it, and the + context-intelligence writer followed the literal while its reader followed `state_root()` — + the same writer/reader split this release fixes at the foundation level, reappearing one + level down. Both are now injected at runtime from `state_root()`, the same technique the + vendored skills and modes directories already use. Verified by running the full workload with + the variable set to a non-default path: 3831 files under the relocated root, **zero** under + the default one. Raised in review by @DavidKoleczek. + +- **Module clones left behind in `~/.amplifier/cache` are left strictly alone.** Clones written + there by earlier versions are no longer read or written by amplifier-agent, but nothing in + this release removes them, reports on them, or offers to remove them. That directory is + foundation's default for *every* Amplifier application, clone directories are keyed by + `sha256(git_url@ref)[:16]` with no per-application namespacing, and on a machine that also + runs amplifier-app-cli they are its **live** clones — indistinguishable from our leftovers + from inside this application. amplifier-agent therefore ships no route to touching them, + because any such route is a way for a user to break amplifier-app-cli without meaning to. + #141 removed them unconditionally, which was correct while that directory was the one + amplifier-agent itself used; it is not correct once it belongs to somebody else. Users who do + not run amplifier-app-cli and want the disk space back can delete the directory themselves. + +- **Recipe session state** moved out of `~/.amplifier/projects/{project}/recipe-sessions` — + the only *write* this application made into amplifier-app-cli's tree. + +- **Context-intelligence captures are now readable.** The hook writes to this app's + workspaces tree, but its readers resolve the root only from + `AMPLIFIER_CONTEXT_INTELLIGENCE_BASE_PATH`, defaulting to `~/.amplifier/projects` — so + captures were written to one tree and looked for in another. Now set when absent; an + explicit user setting is left alone. + +- **Provider rate-limit state** no longer lands in `~/.amplifier`. `provider-anthropic` + builds that path from `os.path.expanduser("~")` joined to a literal `".amplifier"` and + never consults `AMPLIFIER_HOME`, so the bind above cannot reach it; its + `rate_limit_state_path` config is now defaulted at mount time. + +### Added + +- **`doctor` reports `foundation isolation`**, failing if `AMPLIFIER_HOME` is unset, points + somewhere unexpected, or resolves inside `~/.amplifier`. Checked at runtime because the + regression it guards is silent: if the bind stops running, clones return to app-cli's tree + and nothing else in the system reports a problem. +- **`config show`** surfaces `foundation_home` and `module_cache_root`. + +### Known limitation + +- Remote **skill** clones still land in `~/.amplifier/cache/skills` until + microsoft/amplifier-bundle-skills#61 merges. That path is hardcoded in `tool-skills` and is + not reachable from bundle config, so it cannot be fixed from this repository. + ## [0.14.1] — 2026-08-21 ### Fixed diff --git a/docs/spec/foundation-cache-ownership.md b/docs/spec/foundation-cache-ownership.md new file mode 100644 index 00000000..2c465aba --- /dev/null +++ b/docs/spec/foundation-cache-ownership.md @@ -0,0 +1,284 @@ +# Foundation cache ownership + +amplifier-agent operates entirely from its own `~/.amplifier-agent` tree. This +document records how the module-acquisition path gets there, why it did not +before, and what the update mechanism does end to end. + +## The problem this replaces + +`~/.amplifier` is owned by **amplifier-app-cli** — a different application, with +a different release cadence and a cache design this project is deliberately +moving away from. Until this change, every `amplifier-module-*` git clone that +amplifier-agent triggered was written into `~/.amplifier/cache`. + +Nothing in this repository asked for that. The coupling was created by an +*absent* argument. `bundle/loader.py` calls: + +```python +bundle = await load_bundle(f"file://{target}") +... +prepared = await bundle.prepare(install_deps=install_deps) +``` + +Neither call passes a cache root, so foundation applied its own default. +Foundation resolves every location it owns through `get_amplifier_home()` +(`amplifier_foundation/paths/resolution.py:136-152`): + +```python +env_home = os.environ.get("AMPLIFIER_HOME") +if env_home: + return Path(env_home).expanduser().resolve() +return (Path.home() / ".amplifier").resolve() +``` + +amplifier-agent never set `AMPLIFIER_HOME` — `grep -rn "AMPLIFIER_HOME" .` +returned zero matches before this change — so the fallback applied on every +machine. + +This is worth stating plainly because it explains why the problem persisted: +**there was no `.amplifier` string in this repository to find.** A repo-wide +search for the literal returns 94 hits, and every one of them is skills/modes +discovery or an e2e fixture. The actual dependency was invisible to exactly the +kind of search you would run to look for it. + +PR #141 confirmed the location while treating a symptom. Its helper is explicit: + +```python +def _module_clone_root() -> Path: + """Mirrors foundation's own default (``~/.amplifier/cache``). This is *not* + persistence.cache_root(), which is amplifier-agent's own tree -- module clones + belong to foundation and are shared with other Amplifier apps on the same machine.""" + return Path.home() / ".amplifier" / "cache" +``` + +That PR deleted stale clones in place. It did not relocate them, and it accepted +the shared-ownership premise. This change rejects that premise. + +## Where `.amplifier-agent` lives and how the path resolves + +Unchanged, and already correct before this work: + +| Root | Resolves to | +|---|---| +| `amplifier_agent_home()` | `$AMPLIFIER_AGENT_HOME`, else `~/.amplifier-agent` | +| `cache_root()` | `/cache` | +| `config_root()` | `/config` | +| `state_root()` | `/state` | +| `prepared_bundle_dir()` | `/cache/prepared/` | + +This change adds one sibling: + +| Root | Resolves to | +|---|---| +| `foundation_home()` | `$AMPLIFIER_AGENT_FOUNDATION_HOME`, else `/foundation` | +| `module_cache_root()` | `/cache` | + +`foundation/` is a sibling of `cache/`, not a child of it, so the ownership +boundary is legible on disk: everything below `foundation/` is written by +foundation's resolver on foundation's schedule; everything beside it is written +by this application. + +## How modules are fetched from git + +Unchanged — foundation does it, using its documented public contract. This +change does not patch, wrap, or fork foundation's source resolution. It only +tells foundation *where* to work, by setting `AMPLIFIER_HOME` to +`foundation_home()` before `amplifier_foundation` is imported. + +The bind lives in `amplifier_agent_lib/foundation_home.py` and is invoked from +the package `__init__` of both `amplifier_agent_lib` and `amplifier_agent_http`. + +### Why the bind must precede import + +`amplifier_foundation/session/finder.py:36` computes a module-level constant at +import time: + +```python +DEFAULT_SESSIONS_ROOT: Path = Path.home() / ".amplifier" / "projects" +``` + +and `session/__init__.py:128` imports `finder` unconditionally. A bind applied +after that import would be too late for anything that constant feeds. Package +`__init__` is the earliest point that reliably runs before any +`amplifier_foundation` import in this codebase — all of which are either +function-local or in submodules. + +### Why the bind is unconditional + +`bind()` overwrites an inherited `AMPLIFIER_HOME` rather than deferring to it. +A user who exported `AMPLIFIER_HOME` to steer amplifier-app-cli would otherwise +silently re-couple amplifier-agent to app-cli's cache — reintroducing this exact +bug, and doing so only on the machines of users most likely to have customised +their setup. + +Two supported overrides remain, both honoured: + +- `$AMPLIFIER_AGENT_FOUNDATION_HOME` — relocate this subtree only. +- `$AMPLIFIER_AGENT_HOME` — relocate the whole application tree. + +## How pinning is expressed + +Unchanged, and deliberately floating. Every module source in `bundle.md` is +declared at `@main`: + +```yaml +providers: + - module: provider-anthropic + source: git+https://github.com/microsoft/amplifier-module-provider-anthropic@main +``` + +`docs/spec/bundle-and-cache.md` records this as an explicit non-goal: *"Module +sources are not pinned to tags or SHAs. Pinning would gate amplifier-agent +releases on module-repo state."* Upstream module updates are intended to flow +automatically. + +## How the cache is keyed, invalidated, and refreshed + +**Keyed** by foundation: each clone lands at +`/-`. + +**The premise that was false.** Foundation's rule is *a directory named after +this key already exists, so it is what you asked for*. That reasoning is sound; +the input was not. With `@main` the key is `sha256(url@"main")` — and `main` is +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, and in +practice serves the first one for the life of the machine. `resolve()` never +fetches, never compares refs, never checks the commit. + +**Fixed at the premise, not the consequence.** Before `prepare()`, +`bundle/pinning.py` asks each remote what its branch currently points at +(`git ls-remote` — refs only, no repository data, no authentication, measured at +~0.4s) and rewrites the ref to that commit. Foundation then keys on something +that genuinely identifies the content: + +| Situation | Key | Result | +|---|---|---| +| Branch unmoved | same | same directory reused — **nothing downloaded** | +| Branch moved | new | new directory — cloned fresh, automatically | +| Offline / timeout | ref left floating | existing clone reused, as today | + +Degradation is per module, not all-or-nothing: a single unreachable remote +leaves that one source floating and the rest still resolve. + +**This is not pinning.** `bundle.md` still declares `@main` and is never +rewritten in the repository. Resolution happens on the user's machine, against +the branch as it stands at that moment, so an upstream module fix reaches users +with no amplifier-agent release — preserving the non-goal in +`docs/spec/bundle-and-cache.md`. + +**The seam.** `Bundle.prepare(source_resolver=...)` is a documented foundation +extension point — *"callback (module_id, original_source) -> resolved_source … +allows app-layer source override policy to be applied before activation"* — and +its return value is written back into the spec foundation resolves, so the +rewritten ref reaches the cache-key computation. amplifier-app-cli already uses +this seam for settings overrides; amplifier-agent passed nothing. Foundation +supports the rewritten form natively through its `_clone_at_commit()` branch for +full 40-character SHAs. + +Resolution runs as one parallel batch before `prepare()` rather than inside the +callback, because the callback is synchronous and invoked once per module — +doing network I/O there would serialise ~29 round trips. + +**Nothing is deleted to make refresh work.** Directories are immutable and +content-addressed: one either is that commit or does not exist, so there is no +half-updated state to recover from and no interrupted-fetch failure mode. + +**Pruning** is disk reclamation, not correctness. After a successful prepare, +directories belonging to a repository resolved in that pass whose commit is no +longer current are unreferenced and removed. Only repositories resolved in this +pass are considered, the current commit is always kept, and failures are +ignored. + +## What the update command does, end to end + +`amplifier-agent update` (`admin/update.py`): + +1. Resolve the target ref — `--tag`, or `tag_name` from the GitHub releases API. +2. Detect the install method from PEP 610 `direct_url.json`. Only `uv-tool` + proceeds; `editable` and `other` print the equivalent manual command and exit. +3. Run `uv tool install --reinstall --force git+https://github.com/microsoft/amplifier-agent@`. +4. Run `amplifier-agent-post-install`. + +`amplifier-agent-post-install` (`post_install.py`): + +1. If the prepared-bundle cache for the running version exists **and** its + manifest exists, print `cache already prepared` and return. This is the + idempotence gate. +2. Otherwise — a cold cache, meaning either a fresh install or a version change + — delete every `amplifier-module-*` directory under `module_cache_root()`. +3. Prepare the bundle, which re-clones each module at current `main` and caches + the prepared artefact. + +Ordering matters: step 2 only *removes* clones; step 3 is what re-creates them. + +The refresh is not gated behind a one-shot migration marker. It does not need to +be: it is only reachable on a cold cache, which is precisely when a refresh is +wanted, and the version-keyed prepared-bundle cache already provides the +idempotence. A marker would add a second mechanism that can silently stop firing. + +## Migration for existing installs + +None required, and none performed. + +Existing clones under `~/.amplifier/cache` are **left alone**. They belong to +amplifier-app-cli, and deleting them is the cross-application side effect this +change exists to stop. On first run after upgrading, `module_cache_root()` is +empty, so foundation clones every module fresh into amplifier-agent's tree. + +The one-time cost is a full re-clone on the first prepare after upgrade — the +same cost PR #141 imposed deliberately, arrived at here as a consequence of +correct ownership rather than as a wipe. + +### No cleanup command, deliberately + +amplifier-agent ships **no route at all** to removing those clones — not a flag, +not a `doctor` advisory, not a report. An earlier revision of this change added +both a `doctor` line reporting them and a `cache clear --legacy` flag to remove +them, on the reasoning that stranded disk should not be silent. That was wrong, +and the reason is worth recording so it does not get re-added. + +amplifier-app-cli has a large user base, and for those users the directory is +not stranded at all — it is live. From inside amplifier-agent the two +populations are indistinguishable: clone directories are keyed +`sha256(git_url@ref)[:16]`, with no per-application namespacing to read. So any +cleanup affordance is, for a substantial fraction of the people who would see +it, a button that breaks a different application they depend on. A caveat in +the help text does not fix that; it just means the damage was documented. + +The asymmetry decides it. The cost of *not* offering cleanup is some wasted disk +for agent-only users, which they can reclaim with `rm -rf ~/.amplifier` if they +care. The cost of offering it is app-cli users breaking their own install by +following a suggestion this tool made. Those are not comparable, so the +affordance does not exist. + +## Verification + +`amplifier-agent doctor` gains `foundation isolation`, which fails if +`AMPLIFIER_HOME` is unset, points somewhere other than `foundation_home()`, or +resolves inside `~/.amplifier`. + +This is checked at runtime rather than asserted in a unit test because the +property that matters is about the installed process's environment, which is +where the import-order hazard lives. The regression this guards against is +silent by construction: if the bind stops running, foundation falls back to +`~/.amplifier`, every clone returns to app-cli's tree, and nothing else in the +system reports a problem. + +`amplifier-agent config show` surfaces `foundation_home` and +`module_cache_root` for the same reason. + +## Known residuals + +Three places in foundation hardcode `Path.home() / ".amplifier"` and bypass +`AMPLIFIER_HOME`. They are recorded here rather than fixed, because fixing them +belongs in an upstream foundation PR: + +| Location | What | Reachable from amplifier-agent? | +|---|---|---| +| `registry.py:453` | `cache_root` used as a directory-walk stop boundary in `_load_single` | Yes — `load_bundle` is defined in `registry.py`. Only consulted when `resolved.source_root` is falsy, which is not the normal path. | +| `session/finder.py:36` | `DEFAULT_SESSIONS_ROOT`, module-level | Evaluated at import (amplifier-agent imports `amplifier_foundation.session` for `diagnose_transcript`/`repair_transcript`), but never consulted: this app passes explicit transcript paths. | +| `configurator/_state_manager.py:756` | `settings.yaml` location | No — amplifier-agent imports five symbols from foundation and none reach the configurator. | + +None of the three causes a write to `~/.amplifier` on amplifier-agent's runtime +path. The isolation check in `doctor` and the filesystem assertion in the DTU +verification both confirm this empirically rather than by inspection. diff --git a/pyproject.toml b/pyproject.toml index 4d50b16d..5b6a5438 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = 'amplifier-agent' -version = '0.14.1' +version = '0.15.0' requires-python = '>=3.12' license = 'MIT' dependencies = [ diff --git a/src/amplifier_agent_cli/admin/cache_clear.py b/src/amplifier_agent_cli/admin/cache_clear.py index b3aac01f..8e585b14 100644 --- a/src/amplifier_agent_cli/admin/cache_clear.py +++ b/src/amplifier_agent_cli/admin/cache_clear.py @@ -58,6 +58,11 @@ def clear_cache() -> ClearResult: def main() -> int: """Print result of cache clear to stderr and return exit code 0. + Scope is deliberately this application's own tree only. Module clones + written into ``~/.amplifier/cache`` by versions before 0.14.2 are neither + removed nor reported: that directory is amplifier-app-cli's, and this + command must never be a route to damaging it. + Returns: 0 always (idempotent operation). """ diff --git a/src/amplifier_agent_cli/admin/config_show.py b/src/amplifier_agent_cli/admin/config_show.py index 313a94d5..d299c4a3 100644 --- a/src/amplifier_agent_cli/admin/config_show.py +++ b/src/amplifier_agent_cli/admin/config_show.py @@ -157,6 +157,7 @@ def config_group() -> None: ) def config_show(config_path: str | None) -> None: """Print resolved configuration as JSON with source annotations.""" + from amplifier_agent_lib.foundation_home import FOUNDATION_HOME_ENV, foundation_home, module_cache_root from amplifier_agent_lib.persistence import amplifier_agent_home payload: dict[str, Any] = { @@ -164,6 +165,13 @@ def config_show(config_path: str | None) -> None: "host_config": _resolve_host_config(config_path), "skills": _resolve_skills(config_path), "amplifier_agent_home": _annotate_env_or_default("AMPLIFIER_AGENT_HOME", amplifier_agent_home()), + # Surfaced because it is the one root amplifier-agent hands to another + # library. When a module misbehaves, the first question is which clone + # is on disk and where -- having to know that foundation resolves it + # from AMPLIFIER_HOME is exactly the buried detail that made the + # original app-cli coupling hard to see. + "foundation_home": _annotate_env_or_default(FOUNDATION_HOME_ENV, foundation_home()), + "module_cache_root": str(module_cache_root()), } click.echo(json.dumps(payload, indent=2)) diff --git a/src/amplifier_agent_cli/admin/doctor.py b/src/amplifier_agent_cli/admin/doctor.py index 13cc9efb..9ee5059a 100644 --- a/src/amplifier_agent_cli/admin/doctor.py +++ b/src/amplifier_agent_cli/admin/doctor.py @@ -20,6 +20,7 @@ import asyncio import hashlib import inspect +import os import sys import tempfile from dataclasses import dataclass @@ -31,6 +32,7 @@ from amplifier_agent_lib import __version__, persistence from amplifier_agent_lib.bundle import BUNDLE_MD from amplifier_agent_lib.bundle.cache import cache_dir_for_version +from amplifier_agent_lib.foundation_home import foundation_home _OK: str = "[ OK ]" _FAIL: str = "[FAIL]" @@ -104,6 +106,54 @@ def _check_python_version() -> tuple[bool, str]: return (True, f"{_OK} {label}") +def _check_foundation_isolation() -> tuple[bool, str]: + """Return (True, OK line) if foundation's storage root is inside our tree. + + This is the standing guard for the app-cli decoupling. The failure it + catches is silent by nature: if the ``AMPLIFIER_HOME`` bind in + ``amplifier_agent_lib.__init__`` ever stops running -- an import-order + regression, an entry point that bypasses the package ``__init__``, a + refactor that moves the call -- then foundation falls back to + ``~/.amplifier`` and every module clone quietly returns to amplifier-app-cli's + tree. Nothing else in the system would report a problem; the agent would + keep working, sharing a cache it does not own, exactly as it did before. + + Checked at runtime rather than asserted in a unit test because the property + that matters is about the *installed* process's environment, which is where + the import-order hazard actually lives. + """ + from amplifier_agent_lib.persistence import amplifier_agent_home + + bound = os.environ.get("AMPLIFIER_HOME") + expected = foundation_home() + + if not bound: + return ( + False, + f"{_FAIL} foundation isolation: AMPLIFIER_HOME unset " + f"(foundation would fall back to ~/.amplifier, which amplifier-app-cli owns)", + ) + + bound_path = Path(bound).expanduser() + if bound_path != expected: + return ( + False, + f"{_FAIL} foundation isolation: AMPLIFIER_HOME={bound_path} but expected {expected}", + ) + + # Belt and braces: an AMPLIFIER_AGENT_HOME pointed *at* ~/.amplifier would + # satisfy the equality check above while still colliding with app-cli. + if Path.home() / ".amplifier" in bound_path.parents or bound_path == Path.home() / ".amplifier": + return ( + False, + f"{_FAIL} foundation isolation: {bound_path} is inside ~/.amplifier (amplifier-app-cli's tree)", + ) + + rel = bound_path.relative_to(amplifier_agent_home()) if bound_path.is_relative_to(amplifier_agent_home()) else None + suffix = f" (/{rel})" if rel else "" + return (True, f"{_OK} foundation isolation: {bound_path}{suffix}") + + def _emit_bundle_shas() -> None: """Emit sha256-of-source-URL lines for every module declared in bundle.md. @@ -536,6 +586,8 @@ def doctor(strict: bool, quick: bool, emit_sha: bool) -> None: _check_writable("config home", cfg), _check_writable("cache home", cache), _check_writable("state home", state), + _check_foundation_isolation(), + _check_writable("foundation home", foundation_home()), ] for _ok, line in checks: diff --git a/src/amplifier_agent_cli/admin/update.py b/src/amplifier_agent_cli/admin/update.py index c2794c65..0f9bc981 100644 --- a/src/amplifier_agent_cli/admin/update.py +++ b/src/amplifier_agent_cli/admin/update.py @@ -111,6 +111,79 @@ def _parse_version(v: str) -> tuple[int, ...]: return tuple(parts) if parts else (0,) +def _refresh_modules(*, output: str) -> list[str]: + """Re-resolve floating module refs; re-prime if any branch has moved. + + Module sources float on ``@main`` by design, so they move independently of + engine releases. This makes ``amplifier-agent update`` able to deliver an + upstream module fix without a new engine version -- previously the only + route to one was an engine release that existed solely to move the cache. + + Costs nothing when nothing has changed: resolution is ``git ls-remote`` + (refs only, no repository data, no authentication) and the comparison reads + metadata foundation already wrote beside each clone. Downloads happen only + for repositories that actually moved, and only via the re-prime below. + + Never raises. Any failure -- offline, git missing, unreadable cache -- + leaves the install exactly as it was, which is the same state a user who + never ran ``update`` would be in. + + Returns: + Repository URLs whose branch moved. Empty when nothing changed, when + resolution could not run, or when no clones exist yet. + """ + try: + import asyncio + + from amplifier_agent_lib import __version__ as _version + from amplifier_agent_lib.bundle import BUNDLE_MD + from amplifier_agent_lib.bundle.cache import cache_dir_for_version + from amplifier_agent_lib.bundle.pinning import find_drifted_modules, resolve_floating_refs + from amplifier_agent_lib.foundation_home import module_cache_root + + async def _resolve() -> dict[str, tuple[str, str]]: + from amplifier_foundation import load_bundle + + # Parse only -- no prepare(), so nothing is cloned or installed. + bundle = await load_bundle(f"file://{BUNDLE_MD}") + pin = await resolve_floating_refs(bundle.to_mount_plan(), clone_root=module_cache_root()) + return find_drifted_modules(pin.pins, module_cache_root()) + + drifted = asyncio.run(_resolve()) + except Exception as exc: # pragma: no cover - defensive; never block update + if output != "json": + click.echo(f"Module refresh skipped ({exc.__class__.__name__}).") + return [] + + if not drifted: + if output != "json": + click.echo("Modules are current.") + return [] + + if output != "json": + click.echo(f"{len(drifted)} module(s) have upstream changes:") + for url in sorted(drifted): + _old, new = drifted[url] + click.echo(f" {url.rsplit('/', 1)[-1]} -> {new[:12]}") + + # Drop the prepared-bundle artefact so the next prepare is cold and picks up + # the new commits. Only the artefact is removed -- every module clone stays + # exactly where it is, and the ones that did not move are reused verbatim. + try: + cache_dir = cache_dir_for_version(_version) + manifest = cache_dir / "manifest.json" + manifest.unlink(missing_ok=True) + except OSError as exc: # pragma: no cover - defensive + if output != "json": + click.echo(f"Could not invalidate prepared cache ({exc}); next run will still use the old modules.") + return sorted(drifted) + + if output != "json": + click.echo("Re-priming bundle cache...") + subprocess.run(["amplifier-agent-post-install"], check=False) + return sorted(drifted) + + def _build_install_cmd(tag: str) -> list[str]: """Build the uv tool install argv for the given ref.""" return [ @@ -280,7 +353,15 @@ def update_command(check_only: bool, tag_override: str | None, force: bool, outp # install_method == "uv-tool" if not needs_install: + # The engine is current, but modules are not tied to engine releases: + # every module source floats on `@main`, so an upstream provider fix can + # land at any time without a new amplifier-agent version. Exiting here + # -- which is what this branch used to do -- meant `update` had no way to + # deliver one, and the only route to a module fix was an engine release + # nobody needed. Check for module drift before reporting "up to date". + drift = _refresh_modules(output=output) msg = "Already up to date. Use --force to reinstall." + payload["modules_updated"] = drift if output == "json": click.echo(json.dumps(payload)) else: diff --git a/src/amplifier_agent_cli/provider_sources.py b/src/amplifier_agent_cli/provider_sources.py index 4cc271c8..96df60c1 100644 --- a/src/amplifier_agent_cli/provider_sources.py +++ b/src/amplifier_agent_cli/provider_sources.py @@ -58,6 +58,7 @@ import os import sys from dataclasses import dataclass, field +from pathlib import Path from typing import Any, Final, TypedDict @@ -435,14 +436,28 @@ def resolve_credential_detailed(provider_name: str) -> CredentialResolution: # provider (see _CONFIG_CREDENTIAL_UNSUPPORTED in admin.auth) since there # is no static key to store. import json - from pathlib import Path - token_file = Path("~/.amplifier/openai-chatgpt-oauth.json").expanduser() + # Carry a pre-0.15.0 login forward before probing, so an existing user + # reports "resolvable" rather than being told to log in again. + migrate_oauth_token() + + # Reads this application's own tree. Before 0.15.0 this probed + # ``~/.amplifier/openai-chatgpt-oauth.json`` directly -- the read half of + # the app-cli coupling, and the one residual the ownership guarantee + # could not honestly claim. The legacy path is still consulted as a + # fallback so a user whose migration could not run (unreadable source, + # read-only home) still reports accurately instead of silently + # downgrading to "no token". + token_file = oauth_token_path() try: data = json.loads(token_file.read_text()) has_token = isinstance(data, dict) and bool(data.get("access_token") or data.get("refresh_token")) except (OSError, ValueError): - has_token = False + try: + data = json.loads(legacy_oauth_token_path().read_text()) + has_token = isinstance(data, dict) and bool(data.get("access_token") or data.get("refresh_token")) + except (OSError, ValueError): + has_token = False if has_token: return CredentialResolution( provider=provider_name, @@ -686,6 +701,107 @@ def provider_config_from_host(host_config: dict[str, Any] | None) -> dict[str, A return overlay or None +def _provider_state_defaults(provider_name: str) -> dict[str, Any]: + """Return config defaults that keep provider-written state inside our tree. + + Some provider modules persist state to disk and default that path to + amplifier-app-cli's ``~/.amplifier`` directory. ``provider-anthropic`` + writes cross-process rate-limit state:: + + # amplifier_module_provider_anthropic/__init__.py:829 + _default_shared_path = os.path.join( + os.path.expanduser("~"), ".amplifier", "rate-limit-state.json" + ) + self._shared_state_path: str = str( + self.config.get("rate_limit_state_path", _default_shared_path) + ) + + Note the construction: ``os.path.expanduser("~")`` joined to a literal + ``".amplifier"``. It never consults ``AMPLIFIER_HOME``, so the bind in + :mod:`amplifier_agent_lib.foundation_home` cannot reach it -- redirecting + foundation's root has no effect on a module that resolves its own path from + the raw home directory. The module does, however, read + ``config["rate_limit_state_path"]`` first, which is the seam used here. + + This is applied at mount time rather than in ``bundle.md`` because provider + entries in the manifest carry no config: both call sites clear + ``mount_plan["providers"]`` and mount exactly one provider through + :func:`inject_provider`, so manifest-level config for a provider would be + discarded before it ever reached the kernel. + """ + if provider_name == "anthropic": + from amplifier_agent_lib.persistence import state_root + + return {"rate_limit_state_path": str(state_root() / "rate-limit-state.json")} + + if provider_name == "openai-chatgpt": + return {"token_file_path": str(oauth_token_path())} + + return {} + + +def legacy_oauth_token_path() -> Path: + """Where the ChatGPT OAuth token lived before this application owned it. + + ``provider-openai-chatgpt`` hardcodes this as its module-level default + (``oauth.py:60``, ``TOKEN_FILE_PATH``), so every login before 0.15.0 wrote + here -- inside amplifier-app-cli's tree. + """ + return Path("~/.amplifier/openai-chatgpt-oauth.json").expanduser() + + +def oauth_token_path() -> Path: + """Where the ChatGPT OAuth token belongs: this application's own state root. + + The provider reads ``config["token_file_path"]`` before falling back to its + hardcoded default (``provider.py:103``), which is the seam used here -- the + same shape as ``rate_limit_state_path`` for provider-anthropic. + """ + from amplifier_agent_lib.persistence import state_root + + return state_root() / "openai-chatgpt-oauth.json" + + +def migrate_oauth_token() -> bool: + """Copy a pre-0.15.0 ChatGPT token into this application's tree, once. + + Relocating the token path without this would silently invalidate every + existing ChatGPT login: the provider would find nothing at the configured + path and ``login_on_mount`` would drop the user into an interactive + device-code flow on their next run. Trading a documented directory-ownership + gap for a surprise re-login is not an improvement. + + **Copies, never moves.** The original belongs to amplifier-app-cli's tree, + and this application does not delete from it -- which is the whole point of + the change this supports. A user who still runs app-cli keeps a working + token there. + + Idempotent and best-effort: returns ``False`` and leaves everything alone if + the destination already exists, the source does not, or the copy fails. A + failed migration degrades to the device-code flow, which is recoverable; + raising here would not be. + """ + import shutil + + destination = oauth_token_path() + if destination.exists(): + return False + + source = legacy_oauth_token_path() + if not source.is_file(): + return False + + try: + destination.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(source, destination) + # The token is a credential; mirror the 0600 the auth module uses. + destination.chmod(0o600) + except OSError: + return False + + return True + + def build_provider_entry( provider_name: str, model_override: str | None = None, @@ -751,6 +867,9 @@ def build_provider_entry( creds = resolve_provider_credentials(provider_name) priority = 1 config: dict[str, Any] = {"priority": priority, **creds} + # Applied before the extra_config overlay so a host that deliberately wants + # a shared path can still say so via host_config["provider"]["config"]. + config.update(_provider_state_defaults(provider_name)) if model_override is not None: config["default_model"] = model_override if effort_override is not None: diff --git a/src/amplifier_agent_http/__init__.py b/src/amplifier_agent_http/__init__.py index 2e763e25..4a147c31 100644 --- a/src/amplifier_agent_http/__init__.py +++ b/src/amplifier_agent_http/__init__.py @@ -9,3 +9,17 @@ See: amplifier-opencode-poc-plan.md """ + +from __future__ import annotations + +# Bind amplifier_foundation's storage root into amplifier-agent's own tree +# before anything in this package imports amplifier_foundation. +# +# amplifier_agent_lib's own ``__init__`` performs the same bind, but this +# package does not import amplifier_agent_lib at package scope, so relying on +# that would make correctness depend on which submodule an embedder happens to +# import first. ``bind()`` is idempotent, so doing it in both places costs +# nothing and removes the ordering hazard. +from amplifier_agent_lib.foundation_home import bind as _bind_foundation_home + +_bind_foundation_home() diff --git a/src/amplifier_agent_lib/__init__.py b/src/amplifier_agent_lib/__init__.py index e3f1fa9f..16be592e 100644 --- a/src/amplifier_agent_lib/__init__.py +++ b/src/amplifier_agent_lib/__init__.py @@ -25,4 +25,24 @@ # string in sync with ``pyproject.toml`` if it ever has to fire. __version__ = "0.3.0" +# Bind amplifier_foundation's storage root into this application's own tree +# BEFORE anything imports amplifier_foundation. +# +# Placement is deliberate on two axes: +# +# * After ``__version__`` is assigned, because ``foundation_home`` reaches +# ``persistence``, which imports ``__version__`` back out of this module. +# (``foundation_home`` defers that import into the function body, so this is +# belt-and-braces rather than the sole defence.) +# +# * At package-import time rather than inside a CLI entry point, because +# ``amplifier_foundation.session.finder`` computes a ``~/.amplifier``-derived +# module-level constant the moment it is imported. Binding lazily would +# leave that constant pointing into amplifier-app-cli's tree. +# +# See ``foundation_home`` for the full rationale. +from amplifier_agent_lib.foundation_home import bind as _bind_foundation_home + +_bind_foundation_home() + __all__ = ["__version__"] diff --git a/src/amplifier_agent_lib/_runtime.py b/src/amplifier_agent_lib/_runtime.py index e3ae9600..c09d97b1 100644 --- a/src/amplifier_agent_lib/_runtime.py +++ b/src/amplifier_agent_lib/_runtime.py @@ -145,15 +145,44 @@ def prepare_bundle_for_session( if mid and mid in merged_modules: entry["config"] = merged_modules[mid] - # Fix C: pre-seed workspace into hook-context-intelligence's own config. + # Storage roots declared in bundle.md are literal YAML strings that nothing + # expands, so they silently ignore $AMPLIFIER_AGENT_HOME. With the variable + # unset they happen to equal ``state_root()`` and everything works, which is + # exactly why the divergence is invisible in normal use. Set it, and the + # module writes to the literal while every reader in this application + # resolves through ``state_root()`` -- the same writer/reader split root that + # ``foundation_home`` exists to prevent, one level further down. + # + # Overwrite rather than default: the literal is a placeholder that documents + # intent, and the accessor is the single source of truth. A host that wants + # a different location has ``$AMPLIFIER_AGENT_HOME``, which is precisely what + # this makes work. + # + # Same technique the vendored skills/modes directories already use below -- + # inject an absolute path at runtime rather than trusting a manifest literal. + # Module-level ``state_root`` (not ``persistence.state_root``) so test-time + # monkeypatching propagates, matching the convention used elsewhere here. + state = state_root() + for entry in mount_plan.get("hooks") or []: if entry.get("module") == "hook-context-intelligence": hook_cfg = dict(entry.get("config") or {}) + # Fix C: pre-seed workspace into hook-context-intelligence's own config. hook_cfg["project_slug"] = workspace hook_cfg["workspace"] = workspace + hook_cfg["base_path"] = str(state / "workspaces") entry["config"] = hook_cfg break + for entry in mount_plan.get("tools") or []: + if entry.get("module") == "tool-recipes": + recipes_cfg = dict(entry.get("config") or {}) + # ``{project}`` is expanded by tool-recipes, not by us -- only the + # root ahead of it is substituted. + recipes_cfg["session_dir"] = str(state / "projects" / "{project}" / "recipe-sessions") + entry["config"] = recipes_cfg + break + # Inject the vendored built-in skills/modes directories with ABSOLUTE paths # so the RUNNING session discovers them deterministically. Absolute paths are # required because the @mention form declared in bundle.md is only diff --git a/src/amplifier_agent_lib/bundle/bundle.md b/src/amplifier_agent_lib/bundle/bundle.md index bbf6c55c..cfceac3c 100644 --- a/src/amplifier_agent_lib/bundle/bundle.md +++ b/src/amplifier_agent_lib/bundle/bundle.md @@ -175,7 +175,13 @@ tools: - module: tool-recipes source: git+https://github.com/microsoft/amplifier-bundle-recipes@main#subdirectory=modules/tool-recipes config: - session_dir: ~/.amplifier/projects/{project}/recipe-sessions + # Recipe session state is amplifier-agent's own data and belongs in + # amplifier-agent's own tree. This previously read + # ~/.amplifier/projects/{project}/recipe-sessions, which wrote into + # amplifier-app-cli's directory -- the only *write* this application made + # outside its own root. Matches the ~/.amplifier-agent literal already used + # for hook-context-intelligence's base_path below. + session_dir: ~/.amplifier-agent/state/projects/{project}/recipe-sessions auto_cleanup_days: 7 # Hooks declared inline. AAA-specific modifications from upstream behavioral-anchor: diff --git a/src/amplifier_agent_lib/bundle/loader.py b/src/amplifier_agent_lib/bundle/loader.py index cffce241..75687ca5 100644 --- a/src/amplifier_agent_lib/bundle/loader.py +++ b/src/amplifier_agent_lib/bundle/loader.py @@ -94,5 +94,42 @@ async def load_and_prepare_bundle( if agent_path and agent_path.exists(): bundle.agents[agent_name]["source_path"] = str(agent_path) - prepared = await bundle.prepare(install_deps=install_deps) + # Resolve floating ``@main`` refs to the commit they currently point at, + # before foundation computes its cache keys from them. + # + # Foundation keys each clone on ``sha256(git_url@ref)`` and reuses any + # directory that already exists. With ``@main`` the key never changes while + # the commit it names does, so one directory serves every commit that branch + # will ever have -- in practice, the first one, forever. Substituting the + # resolved commit makes the key an identity, at which point foundation's + # reuse rule becomes correct rather than something to work around: an + # unmoved branch resolves to the same key and downloads nothing, a moved one + # resolves to a new key and is cloned fresh. + # + # ``bundle.md`` is not rewritten -- the resolution happens here, on this + # machine, against the branch as it stands right now. Module updates still + # reach users without an amplifier-agent release. + # + # Degrades per module: anything that cannot be resolved (offline, timeout, + # deleted ref) is left floating and behaves exactly as it does today. + from amplifier_agent_lib.bundle.pinning import prune_superseded_clones, resolve_floating_refs + from amplifier_agent_lib.foundation_home import module_cache_root + + pin = await resolve_floating_refs(bundle.to_mount_plan(), clone_root=module_cache_root()) + + def _pin_source(_module_id: str, source: str) -> str: + return pin.mapping.get(source, source) + + # Passing ``None`` when nothing resolved keeps foundation on its untouched + # code path, so an offline run is byte-for-byte the behaviour it has today. + prepared = await bundle.prepare( + install_deps=install_deps, + source_resolver=_pin_source if pin.mapping else None, + ) + + # Only after a successful prepare: the just-resolved commits are now on + # disk, so any older generation of the same repository is unreferenced. + if pin.pins: + prune_superseded_clones(pin.pins, module_cache_root()) + return prepared diff --git a/src/amplifier_agent_lib/bundle/pinning.py b/src/amplifier_agent_lib/bundle/pinning.py new file mode 100644 index 00000000..d26b690c --- /dev/null +++ b/src/amplifier_agent_lib/bundle/pinning.py @@ -0,0 +1,487 @@ +"""Late-binding commit resolution for floating module refs. + +The problem this solves +----------------------- +``bundle.md`` declares every module at a floating ``@main``. Foundation caches +each clone at ``/-`` and, on a later +resolve, returns any directory that already exists and is structurally intact -- +it never fetches into it, never compares refs, never checks the commit. + +That behaviour is *correct reasoning on a false premise*. Foundation is told +the identity of the content is ``main``, so it caches under that name. But +``main`` is not an identity, it is a pointer: the hash never changes while the +commit it names does. One directory therefore serves every commit that branch +will ever have, and in practice serves the first one forever. + +Every previous remedy attacked the consequence -- delete the directory so the +next resolve has nothing to reuse. This module attacks the premise instead. + +What it does +------------ +Before ``Bundle.prepare()`` runs, ask each remote what its branch currently +points at (``git ls-remote``, which transfers no repository data) and rewrite +``@main`` to that commit SHA. Foundation then caches at +``sha256(git_url@)``, which *is* an identity: + +- branch has not moved -> same SHA -> same directory -> **nothing is downloaded** +- branch has moved -> new SHA -> new directory -> cloned fresh, automatically +- offline / unreachable -> ref left as ``@main`` -> existing clone is reused + +Nothing is ever deleted to make this work. Directories become immutable and +content-addressed: one either is that commit or does not exist, so there is no +half-updated state to recover from. + +Why this is not pinning +----------------------- +``bundle.md`` still says ``@main`` and is never rewritten in the repository. +The commit is resolved on the user's machine, against the branch as it exists +at that moment. Ship a provider fix to ``main`` and the next resolution picks +it up -- no amplifier-agent release involved. This deliberately keeps the +property recorded as a non-goal in ``docs/spec/bundle-and-cache.md``: module +sources are not pinned, and releases are not gated on module-repo state. + +Foundation supports the rewritten form natively -- ``sources/git.py`` has a +dedicated ``_clone_at_commit()`` branch for full 40-character SHAs, distinct +from the ``--branch`` clone path used for named refs. + +The seam +-------- +``Bundle.prepare(source_resolver=...)`` is a documented extension point: +*"Optional callback (module_id, original_source) -> resolved_source. Allows +app-layer source override policy to be applied before activation."* Its return +value is written back into the module spec that foundation then resolves, so +the rewritten ref reaches the cache-key computation. amplifier-app-cli already +uses this seam for its own settings overrides; amplifier-agent passed nothing. + +Resolution happens in one parallel batch *before* ``prepare()`` rather than +inside the callback, because the callback is synchronous and is invoked once +per module -- doing network I/O there would serialise ~29 round trips. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import json +import logging +import re +import shutil +from pathlib import Path +from typing import Any + +logger = logging.getLogger(__name__) + +__all__ = [ + "PinResult", + "collect_module_sources", + "find_drifted_modules", + "prune_superseded_clones", + "resolve_floating_refs", +] + +#: A full 40-character hex commit SHA -- the form foundation's +#: ``_clone_at_commit()`` path recognises. Sources already in this form are +#: left untouched: they are already content-addressed. +_FULL_SHA = re.compile(r"^[0-9a-fA-F]{40}$") + +#: Per-remote timeout. ``git ls-remote`` against GitHub completes in well +#: under a second; anything approaching this bound means the network is +#: degraded, and the fallback (leave the ref floating) is the right answer. +_LS_REMOTE_TIMEOUT_S = 10.0 + +#: Ceiling on the whole batch, so a pathological network cannot stall startup +#: for the sum of every per-remote timeout. +_BATCH_TIMEOUT_S = 30.0 + +#: Bound on concurrent git subprocesses. +_MAX_CONCURRENCY = 12 + +_GIT_PREFIX = "git+" + +#: Metadata foundation writes beside each clone. Records ``git_url``, ``ref``, +#: ``commit`` and ``cached_at``, which is what makes pruning possible without +#: any network access or bookkeeping of our own. +_CACHE_META_FILE = ".amplifier_cache_meta.json" + + +class PinResult: + """Outcome of one resolution pass. + + Attributes: + mapping: original source string -> rewritten source string. Only + contains entries that actually changed, so an empty mapping means + "resolve nothing", not "resolution failed". + resolved: number of sources successfully rewritten to a commit. + skipped: number left floating (unreachable remote, timeout, already + pinned, or a non-git source). + """ + + __slots__ = ("mapping", "pins", "resolved", "skipped") + + def __init__( + self, + mapping: dict[str, str], + resolved: int, + skipped: int, + pins: dict[str, str] | None = None, + ) -> None: + self.mapping = mapping + self.resolved = resolved + self.skipped = skipped + #: git_url -> commit SHA resolved this pass. Drives pruning: a clone + #: whose recorded url is a key here but whose recorded commit is not + #: the value is a superseded generation. + self.pins = pins or {} + + +def _split_source(source: str) -> tuple[str, str, str] | None: + """Split ``git+@#`` into ``(url, ref, fragment)``. + + Returns ``None`` for anything that is not a git source, or that carries no + explicit ref. A source without a ref resolves to the remote's default + branch, which is equally floating -- but rewriting it would require knowing + which branch that is, and ``ls-remote HEAD`` reports the commit without + naming the branch. Left alone rather than guessed at; the sources this + application ships all carry an explicit ref. + + The ``@`` is located by taking the last one and requiring no ``/`` after it, + which distinguishes ``.../repo@main`` from a userinfo ``https://user@host/...``. + """ + if not source.startswith(_GIT_PREFIX): + return None + + body = source[len(_GIT_PREFIX) :] + + fragment = "" + if "#" in body: + body, fragment = body.split("#", 1) + + if "@" not in body: + return None + url, _, ref = body.rpartition("@") + if not url or not ref or "/" in ref: + return None + + return url, ref, fragment + + +def _rebuild_source(url: str, sha: str, fragment: str) -> str: + """Reassemble a source string with *sha* substituted for the ref.""" + rebuilt = f"{_GIT_PREFIX}{url}@{sha}" + if fragment: + rebuilt = f"{rebuilt}#{fragment}" + return rebuilt + + +def _read_cached_commits(clone_root: Path) -> dict[str, str]: + """Return ``git_url -> newest cached commit`` from clone metadata on disk. + + Reads the ``.amplifier_cache_meta.json`` foundation writes beside every + clone. Purely local; no network, no bookkeeping of our own. + """ + if not clone_root.is_dir(): + return {} + try: + children = sorted(clone_root.iterdir()) + except OSError: + return {} + + newest: dict[str, tuple[str, str]] = {} # url -> (cached_at, commit) + for child in children: + if not child.is_dir(): + continue + try: + meta = json.loads((child / _CACHE_META_FILE).read_text(encoding="utf-8")) + except (OSError, ValueError): + continue + url, commit, cached_at = meta.get("git_url"), meta.get("commit"), meta.get("cached_at", "") + if not isinstance(url, str) or not isinstance(commit, str): + continue + stamp = cached_at if isinstance(cached_at, str) else "" + if url not in newest or stamp > newest[url][0]: + newest[url] = (stamp, commit) + + return {url: commit for url, (_stamp, commit) in newest.items()} + + +def collect_module_sources(mount_plan: dict[str, Any]) -> list[str]: + """Return every module source string in *mount_plan*, de-duplicated. + + Walks exactly the shape ``Bundle.prepare()`` walks -- session orchestrator + and context, the ``providers``/``tools``/``hooks`` lists, and the same four + again nested under each entry in ``agents`` -- so that anything foundation + will try to activate is offered for resolution. A section foundation walks + but this function misses would silently keep floating; the mirroring is + deliberate and should be kept in step. + """ + sources: list[str] = [] + seen: set[str] = set() + + def take(spec: Any) -> None: + if isinstance(spec, dict): + source = spec.get("source") + if isinstance(source, str) and source not in seen: + seen.add(source) + sources.append(source) + + def take_sections(container: dict[str, Any]) -> None: + session = container.get("session") + if isinstance(session, dict): + take(session.get("orchestrator")) + take(session.get("context")) + for section in ("providers", "tools", "hooks"): + entries = container.get(section) + if isinstance(entries, list): + for entry in entries: + take(entry) + + take_sections(mount_plan) + + agents = mount_plan.get("agents") + if isinstance(agents, dict): + for agent_def in agents.values(): + if isinstance(agent_def, dict): + take_sections(agent_def) + + return sources + + +async def _ls_remote(url: str, ref: str, semaphore: asyncio.Semaphore) -> str | None: + """Return the commit *ref* currently points at on *url*, or ``None``. + + ``git ls-remote`` performs a refs-only exchange -- no objects, no working + tree, no clone. Measured at ~0.4s against GitHub over plain HTTPS with no + authentication, which is what makes doing this for every module affordable. + """ + async with semaphore: + try: + proc = await asyncio.create_subprocess_exec( + "git", + "ls-remote", + url, + ref, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + except OSError as exc: # git missing, fork failure + logger.debug("ls-remote could not start for %s: %s", url, exc) + return None + + try: + stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=_LS_REMOTE_TIMEOUT_S) + except TimeoutError: + proc.kill() + # Reap the killed child so it does not linger as a zombie. + with contextlib.suppress(Exception): + await proc.wait() + logger.debug("ls-remote timed out for %s@%s", url, ref) + return None + + if proc.returncode != 0: + logger.debug("ls-remote failed for %s@%s (rc=%s)", url, ref, proc.returncode) + return None + + first = stdout.decode("utf-8", "replace").strip().splitlines() + if not first: + # Ref does not exist on the remote. Leaving it floating lets + # foundation surface its own, clearer error. + return None + + sha = first[0].split()[0].strip() + return sha if _FULL_SHA.match(sha) else None + + +async def resolve_floating_refs( + mount_plan: dict[str, Any], + clone_root: Path | None = None, +) -> PinResult: + """Resolve every floating module ref in *mount_plan* to a commit SHA. + + Never raises and never blocks indefinitely. Degradation is per module, not + all-or-nothing: one unreachable remote leaves that source alone while the + rest still resolve. + + **Offline falls back to the commit already on disk, not to ``@main``.** + Leaving the ref floating would be wrong once any SHA-keyed directory exists: + ``@main`` and ``@`` hash to different cache keys, so falling back + would point foundation at a directory that was never created and send it to + clone -- precisely when the network is unavailable. Pinning instead to the + commit recorded in the local clone metadata reproduces the key of the + directory that *is* there, so an offline run reuses it and succeeds. + + Args: + mount_plan: Bundle mount plan, as returned by ``Bundle.to_mount_plan()``. + clone_root: Foundation's clone directory, used for the offline fallback. + When omitted, unresolvable sources are left floating. + """ + sources = collect_module_sources(mount_plan) + + targets: list[tuple[str, str, str, str]] = [] # (source, url, ref, fragment) + skipped = 0 + for source in sources: + parts = _split_source(source) + if parts is None: + skipped += 1 + continue + url, ref, fragment = parts + if _FULL_SHA.match(ref): + # Already content-addressed; nothing to gain. + skipped += 1 + continue + targets.append((source, url, ref, fragment)) + + if not targets: + return PinResult({}, resolved=0, skipped=skipped) + + cached = _read_cached_commits(clone_root) if clone_root is not None else {} + + semaphore = asyncio.Semaphore(_MAX_CONCURRENCY) + tasks = [_ls_remote(url, ref, semaphore) for _, url, ref, _ in targets] + + try: + shas = await asyncio.wait_for( + asyncio.gather(*tasks, return_exceptions=True), + timeout=_BATCH_TIMEOUT_S, + ) + except TimeoutError: + logger.debug("ref resolution batch exceeded %.0fs; falling back to cached commits", _BATCH_TIMEOUT_S) + shas = [None] * len(targets) + + mapping: dict[str, str] = {} + pins: dict[str, str] = {} + for (source, url, _ref, fragment), sha in zip(targets, shas, strict=True): + resolved_sha = sha if isinstance(sha, str) else None + if resolved_sha is None: + # Unreachable remote: hold the line at whatever is already cloned. + fallback = cached.get(url) + if fallback is None: + skipped += 1 + continue + logger.debug("using cached commit for %s (remote unreachable)", url) + rebuilt = _rebuild_source(url, fallback, fragment) + if rebuilt != source: + mapping[source] = rebuilt + skipped += 1 + continue + + pins[url] = resolved_sha + rebuilt = _rebuild_source(url, resolved_sha, fragment) + if rebuilt != source: + mapping[source] = rebuilt + + return PinResult(mapping, resolved=len(mapping), skipped=skipped, pins=pins) + + +def prune_superseded_clones(pins: dict[str, str], clone_root: Path) -> int: + """Remove clone directories left behind by an earlier commit of a pinned repo. + + Content-addressed directories are the price of never mutating a clone in + place: each new commit produces a new directory and the previous one stops + being referenced. Without pruning, a frequently-updated module accumulates + a directory per commit ever seen. + + This is disk reclamation on amplifier-agent's own tree, not a correctness + mechanism -- the refresh works whether or not it runs. It is therefore + deliberately conservative: + + * only directories whose recorded ``git_url`` was resolved in *this* pass + are considered, so an unrelated app's clone -- or one belonging to a + module no longer in the bundle -- is never touched; + * the directory holding the just-resolved commit is always kept; + * failures are swallowed, since a directory that cannot be removed (open + handle on Windows, permissions) is a tidiness problem and nothing more. + + Args: + pins: ``git_url -> commit`` resolved this pass. + clone_root: Foundation's clone directory for this application. + + Returns: + Number of directories removed. + """ + if not pins or not clone_root.is_dir(): + return 0 + + removed = 0 + try: + children = sorted(clone_root.iterdir()) + except OSError: + return 0 + + for child in children: + if not child.is_dir(): + continue + meta_path = child / _CACHE_META_FILE + try: + meta = json.loads(meta_path.read_text(encoding="utf-8")) + except (OSError, ValueError): + # No metadata means foundation did not write this, or wrote it + # partially. Not ours to judge; leave it. + continue + url = meta.get("git_url") + commit = meta.get("commit") + if not isinstance(url, str) or not isinstance(commit, str): + continue + current = pins.get(url) + if current is None or commit == current: + continue + shutil.rmtree(child, ignore_errors=True) + if not child.exists(): + removed += 1 + logger.debug("pruned superseded clone %s (%s)", child.name, commit[:12]) + + return removed + + +def find_drifted_modules(pins: dict[str, str], clone_root: Path) -> dict[str, tuple[str, str]]: + """Return repositories whose branch has moved since their clone was taken. + + Compares freshly resolved commits against the ``commit`` foundation recorded + in each clone's ``.amplifier_cache_meta.json``. Entirely local: the network + cost was already paid by :func:`resolve_floating_refs`, and this reads files + that are already on disk. + + Lets ``amplifier-agent update`` answer "did anything actually change?" + without downloading a byte. A repository with no clone yet is not drift -- + there is nothing stale to report, and the next prepare will fetch it as a + matter of course. + + Args: + pins: ``git_url -> commit`` resolved this pass. + clone_root: Foundation's clone directory for this application. + + Returns: + ``git_url -> (cached_commit, current_commit)`` for repositories whose + on-disk clone is behind the branch tip. + """ + if not pins or not clone_root.is_dir(): + return {} + + try: + children = sorted(clone_root.iterdir()) + except OSError: + return {} + + # A repo can own several directories at once (one per commit seen). It has + # drifted only if NONE of them holds the current commit. + seen: dict[str, set[str]] = {} + for child in children: + if not child.is_dir(): + continue + try: + meta = json.loads((child / _CACHE_META_FILE).read_text(encoding="utf-8")) + except (OSError, ValueError): + continue + url = meta.get("git_url") + commit = meta.get("commit") + if isinstance(url, str) and isinstance(commit, str): + seen.setdefault(url, set()).add(commit) + + drifted: dict[str, tuple[str, str]] = {} + for url, current in pins.items(): + commits = seen.get(url) + if not commits or current in commits: + continue + # Report the most recently cached commit as the "from" side. + drifted[url] = (sorted(commits)[0], current) + + return drifted diff --git a/src/amplifier_agent_lib/foundation_home.py b/src/amplifier_agent_lib/foundation_home.py new file mode 100644 index 00000000..5664dd69 --- /dev/null +++ b/src/amplifier_agent_lib/foundation_home.py @@ -0,0 +1,185 @@ +"""Bind amplifier_foundation's storage root into amplifier-agent's own tree. + +Why this module exists +---------------------- +Every on-disk artefact amplifier-agent *knows* it owns already lives under +``amplifier_agent_home()`` (``~/.amplifier-agent`` by default, overridable via +``$AMPLIFIER_AGENT_HOME``) -- state, config, credentials, and the +prepared-bundle cache. One category did not: the git clones of the modules the +bundle declares. + +Those clones are created by ``amplifier_foundation``, not by this application. +Foundation resolves every location it owns through +``amplifier_foundation.paths.get_amplifier_home()``:: + + env_home = os.environ.get("AMPLIFIER_HOME") + if env_home: + return Path(env_home).expanduser().resolve() + return (Path.home() / ".amplifier").resolve() + +amplifier-agent never set ``AMPLIFIER_HOME``, so the fallback applied and every +``amplifier-module-*`` clone landed in ``~/.amplifier/cache`` -- a tree owned +and managed by *amplifier-app-cli*, a different application with a different +release cadence and a known-buggy cache design. Nothing in this repository +referenced ``.amplifier`` to make that happen; the coupling was created by the +*absence* of an argument at the ``load_bundle()`` call in +:mod:`amplifier_agent_lib.bundle.loader`, which is why it survived several +rounds of grep-driven cleanup. + +The consequences were not theoretical. Sharing a clone root with another +application means amplifier-agent cannot safely refresh its own module clones +(they may be in use by app-cli), cannot reason about their freshness, and +inherits any corruption app-cli introduces. + +What this module does +--------------------- +Sets ``AMPLIFIER_HOME`` to a directory inside amplifier-agent's own tree before +``amplifier_foundation`` is imported, so foundation's own resolver -- unchanged, +unpatched, using its documented public contract -- writes into a root this +application owns outright. + +Ordering is load-bearing +------------------------ +``amplifier_foundation.session`` evaluates a module-level constant at *import* +time:: + + # amplifier_foundation/session/finder.py:36 + DEFAULT_SESSIONS_ROOT: Path = Path.home() / ".amplifier" / "projects" + +and ``session/__init__.py`` imports ``finder`` unconditionally. A binding +applied after that import would be too late for anything that constant feeds. +:func:`bind` is therefore invoked at the top of both the ``amplifier_agent_lib`` +and ``amplifier_agent_http`` package ``__init__`` modules -- both of which +execute before any ``amplifier_foundation`` import in this codebase, all of +which are either function-local or in submodules. + +Unconditional by design +----------------------- +:func:`bind` overwrites any inherited ``AMPLIFIER_HOME``. That is deliberate +and is the entire point: a user who has exported ``AMPLIFIER_HOME`` to steer +amplifier-app-cli would otherwise silently re-couple amplifier-agent to app-cli's +cache -- reintroducing exactly the bug this module removes, and doing so only on +the machines of the users most likely to have customised their setup. Callers +who genuinely need to relocate amplifier-agent's foundation tree have two +supported levers, both honoured here: ``$AMPLIFIER_AGENT_FOUNDATION_HOME`` +(this subtree only) and ``$AMPLIFIER_AGENT_HOME`` (the whole application tree). +""" + +from __future__ import annotations + +import os +from pathlib import Path + +__all__ = [ + "FOUNDATION_HOME_ENV", + "FOUNDATION_SUBDIR", + "bind", + "foundation_home", + "module_cache_root", +] + +#: Environment variable foundation itself consults. Set by :func:`bind`. +_FOUNDATION_ENV = "AMPLIFIER_HOME" + +#: Environment variable the context-intelligence hook's *readers* consult. +#: Set by :func:`bind` only when absent -- see :func:`_bind_context_intelligence`. +_CONTEXT_INTELLIGENCE_ENV = "AMPLIFIER_CONTEXT_INTELLIGENCE_BASE_PATH" + +#: Escape hatch for relocating *only* the foundation subtree. +FOUNDATION_HOME_ENV = "AMPLIFIER_AGENT_FOUNDATION_HOME" + +#: Subdirectory of ``amplifier_agent_home()`` handed to foundation. +#: +#: Kept as a sibling of ``cache/``, ``state/`` and ``config/`` rather than +#: nested inside ``cache/`` so the ownership boundary is legible on disk: +#: everything below ``foundation/`` is written by foundation's resolver on +#: foundation's schedule, and everything beside it is written by this +#: application. ``amplifier-agent cache clear`` can then reason about the two +#: independently. +FOUNDATION_SUBDIR = "foundation" + + +def foundation_home() -> Path: + """Return the directory amplifier-agent hands to foundation as its home. + + Resolves in order: + + 1. ``$AMPLIFIER_AGENT_FOUNDATION_HOME`` -- relocate this subtree alone. + 2. ``amplifier_agent_home() / "foundation"`` -- which itself honours + ``$AMPLIFIER_AGENT_HOME``. + + Never ``~/.amplifier``: that tree belongs to amplifier-app-cli. + """ + override = os.environ.get(FOUNDATION_HOME_ENV) + if override: + return Path(override).expanduser() + + # Imported inside the function, not at module scope, to keep the import + # graph acyclic. ``persistence`` does ``from amplifier_agent_lib import + # __version__``, and :func:`bind` is called from that package's ``__init__`` + # -- a module-level import here would make the cycle's success depend on + # statement order inside ``__init__.py``. A function-local import makes the + # ordering irrelevant, because by the time anyone calls this the package is + # fully initialised. + from amplifier_agent_lib.persistence import amplifier_agent_home + + return amplifier_agent_home() / FOUNDATION_SUBDIR + + +def module_cache_root() -> Path: + """Return the directory holding foundation's git clones of modules. + + Mirrors foundation's own layout (``/cache``) as resolved by + ``SimpleSourceResolver`` and ``ModuleActivator``. Unlike the pre-decoupling + situation this path is owned by amplifier-agent, which is what makes it safe + for :mod:`amplifier_agent_lib.post_install` to delete entries from it. + """ + return foundation_home() / "cache" + + +def _bind_context_intelligence() -> None: + """Point the context-intelligence hook's *reader* root at our tree. + + ``hook-context-intelligence`` has a split root. Its writer honours the + ``base_path`` set in ``bundle.md`` (already amplifier-agent's own + ``state/workspaces``), but its readers -- the discover, recipe and + navigation skills -- resolve the root *only* from + ``AMPLIFIER_CONTEXT_INTELLIGENCE_BASE_PATH``, falling back to + ``~/.amplifier/projects``. + + Unset, that split is silent and wrong: captures are written into + amplifier-agent's tree and then looked for in amplifier-app-cli's, so every + read comes back empty. The hook itself detects the mismatch and warns at + runtime:: + + context-intelligence: writer base_path (/root/.amplifier-agent/state/workspaces) + and reader root (/root/.amplifier/projects) disagree -- ... captures written + under /root/.amplifier-agent/state/workspaces will be invisible to them. + + Set only when absent, which is a deliberate departure from the + unconditional ``AMPLIFIER_HOME`` bind above. The two cases are not alike: + inheriting app-cli's ``AMPLIFIER_HOME`` re-creates the shared-cache bug this + module exists to remove, whereas a user who has explicitly exported this + variable is making a considered choice to pool observability data across + applications. That is a legitimate thing to want, and it is their data. + """ + if not os.environ.get(_CONTEXT_INTELLIGENCE_ENV): + from amplifier_agent_lib.persistence import state_root + + os.environ[_CONTEXT_INTELLIGENCE_ENV] = str(state_root() / "workspaces") + + +def bind() -> Path: + """Bind third-party storage roots into amplifier-agent's own tree. + + Points ``AMPLIFIER_HOME`` at :func:`foundation_home` and the + context-intelligence reader root at this application's workspaces + directory, then returns the foundation home. + + Idempotent, and safe to call from multiple entry points. Must run before + ``amplifier_foundation`` is imported -- see the module docstring. + """ + home = foundation_home() + os.environ[_FOUNDATION_ENV] = str(home) + _bind_context_intelligence() + return home diff --git a/src/amplifier_agent_lib/post_install.py b/src/amplifier_agent_lib/post_install.py index 13a4a254..1f2bb106 100644 --- a/src/amplifier_agent_lib/post_install.py +++ b/src/amplifier_agent_lib/post_install.py @@ -1,6 +1,6 @@ -"""Post-install hook: prime the XDG prepared-bundle cache. +"""Post-install hook: prime the prepared-bundle cache. -Failures here NEVER fail the install — the runtime first-invocation path is +Failures here NEVER fail the install -- the runtime first-invocation path is the safety net. Entry-point (see pyproject.toml [project.scripts]): @@ -8,6 +8,16 @@ Usage (in curl/container install scripts): uv tool install amplifier-agent && amplifier-agent-post-install + +No module-clone deletion happens here. Earlier revisions deleted every +``amplifier-module-*`` directory before priming, because foundation returns an +existing clone without ever fetching into it and a floating ``@main`` ref keeps +the same cache key forever -- so wiping the directory was the only way to make +the next prepare pick up an upstream change. Since +:mod:`amplifier_agent_lib.bundle.pinning` resolves those refs to concrete +commits before foundation computes its cache keys, a moved branch now lands in +a *different* directory on its own. Deleting anything would only force a +re-download of code that has not changed. """ from __future__ import annotations @@ -23,7 +33,7 @@ async def main() -> int: """Prime the prepared-bundle cache for the current version. Returns: - Always 0 — failures are logged to stderr and swallowed so the installer + Always 0 -- failures are logged to stderr and swallowed so the installer never fails due to this hook. """ cache_dir = cache_dir_for_version(__version__) diff --git a/uv.lock b/uv.lock index 71f611af..e193da99 100644 --- a/uv.lock +++ b/uv.lock @@ -16,7 +16,7 @@ members = [ [[package]] name = "amplifier-agent" -version = "0.14.1" +version = "0.15.0" source = { editable = "." } dependencies = [ { name = "amplifier-foundation" },