fix(updates): check_bundle_status walks includes instead of dropping them - #361
Merged
Conversation
…them check_bundle_status() reported "All N source(s) up to date" over a cache stuck at an old commit. _collect_source_uris() dropped bundle.includes on the premise -- stated in a comment at updates/__init__.py:138-140 -- that included bundles "are registered as first-class bundles and will be checked independently". They are registered, but a `#subdirectory=` include lands with is_root=False, and host enumeration that keeps only root entries never reaches it. A repo whose only registry presence is a non-root sub-bundle entry was therefore checked by nobody, and GitSourceHandler.resolve() returns an existing cache verbatim with no fetch -- so a source the update path does not enumerate is a source that never moves. - collect_transitive_source_uris(): cycle-safe BFS over includes, reusing BundleRegistry._parse_include / _resolve_include_source / _load_from_path for resolution and GitSourceHandler._get_cache_path to read an already-cached bundle WITHOUT resolving (resolving downloads, which would make a deleted cache look healthy). - check_bundle_status() reports each transitive git source as a SourceStatus with cached vs remote commit, has_update, and a new `via` field naming the including bundle. Pinned refs (@sha / @tag) stay pinned, never updateable. summary therefore can no longer claim "All N source(s) up to date" while a transitive source has an update. - SourceStatus.via appended last with a default, so every existing construction is unchanged. - Stale comment replaced with what is actually true. The walk seeds from the bundle's _source_uri and re-reads that file from disk rather than trusting the in-memory bundle.includes: Bundle.compose() keeps only self.includes, so after load_bundle() composes, the returned bundle's .includes is the FIRST INCLUDED bundle's list, not its own. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…s on Windows CI's three Windows legs failed: every transitive source vanished. A `file:///C:/Users/x` seed parses to the path component "/C:/Users/x" -- rooted but driveless. Passed to Path() unnormalized it resolves against whatever the current drive happens to be, so the seed bundle file "does not exist", _cached_path_for() returns None, and the walk finds nothing at all. Normalized through the same shared helper FileSourceHandler.resolve() uses (strip_uri_drive_prefix), and #subdirectory= is now honoured for file URIs the same way that handler honours it. Adds a round-trip test over Path.as_uri() -- passes trivially on POSIX, and is a real guard on the Windows legs. Note for the app-side collapse: amplifier-app-cli's include_graph.py _local_path_for() has this same latent bug in its file:// branch. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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.
Defect
check_bundle_status()— a public foundation mechanism every host inherits, not just the CLI — reportedAll N source(s) up to dateover a cache stuck at an old commit.Measured on a real machine (2026-09-06):
~/.amplifier/cache/amplifier-d1dda27a16518560(git_url https://github.com/microsoft/amplifier,ref main) sat atc03a88bwhile upstreammainwas28588b9. That cache is reached only through amplifier-foundation'sbundle.mdincludegit+https://github.com/microsoft/amplifier@main#subdirectory=behaviors/amplifier-expert.yaml.Root cause
amplifier_foundation/updates/__init__.py:138-140(inside_collect_source_uris()):The premise is false in exactly the case that matters:
registry._load_single(..., auto_register=True)), but a#subdirectory=include lands withis_root: false(amplifier_foundation/registry.py:520-545), and host enumeration that keeps onlyis_root: trueentries (e.g.amplifier_app_cli/lib/bundle_loader/discovery.py:679-685) never reaches it. A repo whose only registry presence is a non-root sub-bundle entry is checked by nobody.GitSourceHandler.resolve()(amplifier_foundation/sources/git.py:697-709) returns an existing cache verbatim on a hit — no TTL, no fetch, ever. A source the update path does not enumerate is a source that never moves.A second, non-obvious root cause found while fixing this
Walking the in-memory
bundle.includes— the literal reading of the issue — does not work.Bundle.compose()(amplifier_foundation/bundle/_dataclass.py:185) keeps onlyincludes=list(self.includes), and_compose_includes()returnscomposed_includes.compose(bundle). So afterload_bundle()composes a bundle, the returned object's.includesis the first included bundle's list, not its own. Verified live below: foundation's post-composebundle.includes == [].The walk therefore seeds from
bundle._source_uriand re-reads that bundle file from disk (falling back tobundle.includesonly for a hand-constructed, never-loadedBundle).Change
amplifier_foundation/updates/__init__.pycollect_transitive_source_uris()— cycle-safe BFS overincludes:, reusingBundleRegistry._parse_include/_resolve_include_source/_load_from_pathandGitSourceHandler._get_cache_path. New_cached_path_for()reads an already-cached bundle without resolving (resolving downloads — a deleted cache must not look healthy).check_bundle_status()gains keyword-onlyregistryandinclude_transitive=True. Stale comment replaced with what is actually true.amplifier_foundation/sources/protocol.pySourceStatus.via: str | None = None— the including bundle's name;Nonefor direct sources. Appended last with a default, so every existing positional/keyword construction is byte-compatible.tests/test_transitive_include_status_72y.pyBehavioural consequences:
SourceStatuswith cached vs remote commit,has_update, andvia.is_pinnedrefs (@<sha>/@v1.2.3) are reported pinned, never updateable (GitSourceHandler.get_statusalready short-circuits; the walk just stops hiding them).status.summarycan no longer sayAll N source(s) up to datewhile any transitive sourcehas_update— transitive rows are instatus.sources, soupdateable_sourcescounts them.update_bundle()consequently now refreshes stale transitive caches too, which is the actual remedy for the measured staleness.What app-cli would delete
amplifier-app-cli PR #317 (merged
c120a36) addedamplifier_app_cli/utils/include_graph.pyto work around this app-side. Semantics here were matched to it deliberately (fragment-stripped URI as the update-target identity; visited-set keyed on the full URI including fragment; read-cache-never-resolve; reuse of the loader's own resolution). With this merged, app-cli can collapse:amplifier_app_cli/utils/include_graph.py—collect_transitive_git_sources(),check_transitive_sources(),transitive_statuses_for(),_local_path_for(),strip_uri_fragment(),TransitiveSource,TransitiveStatus→ all superseded bycollect_transitive_source_uris()+SourceStatus.via.amplifier update/amplifier bundle updatebecome plain reads ofcheck_bundle_status().sources(filter onvia is not Nonefor the "reached via an include" rows).refresh_transitive_source()can stay or becomeupdate_bundle(selective=[...])— it is a thinGitSourceHandler.update()wrapper either way.One deliberate difference: app-cli's
TransitiveSourcecarries bothparent(immediate includer) androot(the registered row it groups under, for table rendering).SourceStatus.viais the honest immediate includer only — grouping is a presentation concern for the host, not a property of the source.Gates
uv run pytest tests/ -qlocallytests/test_sources.py::TestFileSourceHandler::test_resolve_existing_file, is pre-existing on this host at base2ef5e12(verified by stashing all of my changes and re-running on a pristine tree:assert PosixPath('/tmp') == PosixPath('/tmp/tmpXXXX')— a/tmpresolution quirk of this machine, inFileSourceHandler, untouched here). It passes in CI on all six legs, confirming it is host-local and not a regression.tests/test_transitive_include_status_72y.py— 6 passedruff format --checkon changed filesruff checkon changed filesF401(ParsedURIimported but unused) on an untouched import line inupdates/__init__.py— left alone deliberately, since it is also a public re-export (amplifier_foundation.updates.ParsedURI) and removing it is an unrelated surface changeNew tests
test_non_root_include_with_stale_cache_is_reported— fixture repo C (root bundle name deliberately collides with an already-registered name, so C's repo gets no root entry; sub-bundlebehaviors/child.yamlregistersis_root=False), cache pre-seeded at commit one, fake remote advanced to commit two. Asserts: no root entry for that repo; exactly one transitive row;cached_commit == first,remote_commit == second,has_update is True,via == "bee";"source(s) up to date" not in summary. Ends with an in-place regression guard — the same bundle throughinclude_transitive=Falsereproduces the old green lie (has_updates is False, no row).test_pinned_include_is_reported_pinned_not_updateable—@<sha>include →is_pinned is True,has_update is False,status.has_updates is False.test_include_cycle_terminates— B includes C, C includes B, both cached git repos. Terminates, returns{uri_c: "bee"}.test_deleted_cache_is_not_resolved_back_into_existence— a missing cache yieldsNoneand stays missing; the walk never clones one back into a green row.test_cached_path_for_round_trips_a_file_uri—Path.as_uri()round-trips, including the Windowsfile:///C:/...form (see below).test_direct_sources_carry_no_via—viastaysNonefor directly-declared sources.Second commit: a Windows bug the first commit shipped
The first push failed all three Windows CI legs — every transitive source vanished there. Cause:
Path.as_uri()emitsfile:///C:/Users/xon Windows, whose path component parses to/C:/Users/x— rooted but driveless. Passed toPath()unnormalized it resolves against whatever the current drive happens to be, so the seed bundle file "does not exist",_cached_path_for()returnsNone, and the walk finds nothing at all — silently, with a green summary. Exactly the class of failure this PR exists to remove.Fixed by normalizing through the same shared helper
FileSourceHandler.resolve()already uses (strip_uri_drive_prefix), and honouring#subdirectory=for file URIs the way that handler does. Guarded by test 5 above.Note for the app-side collapse: amplifier-app-cli's
include_graph.py_local_path_for()has this same latent bug in itsfile://branch — worth fixing there if the collapse does not happen promptly.LIVE proof
Isolated
AMPLIFIER_HOME(a copy — the real~/.amplifierwas read, never written; verified untouched afterwards), containing a copy of the 22 caches foundation's includes reach. In that copy:git -C cache/amplifier-d1dda27a16518560 reset --hard c03a88bplus.amplifier_cache_meta.jsoncommitcorrected to match, and theis_root: trueregistry entry formicrosoft/amplifierremoved — reproducing "reachable only as a non-root include".Then
check_bundle_status()on the foundation bundle loaded from this worktree (PYTHONPATH), with no explicitcache_dirorregistry(zero-config path):The measured defect, reproduced and then fixed:
microsoft/amplifierhad no row at all pre-fix, and post-fix reportsc03a88b -> 28588b9,has_update: true,via: foundation. The depth-2 rows (via=amplifier-tester-behavior,via=digital-twin-universe-behavior) show the walk is genuinely transitive, not one-level.Honest caveat on this live run: the isolated home's direct sources were themselves stale, so the literal string
All N source(s) up to datedoes not appear in either summary here — the pre-fix summary already said10 update(s) available. What the live run proves is the missing row; the exactAll N source(s) up to dateregression is covered by assertion in test 1 ("source(s) up to date" not in status.summary).Closes work item
recipes-72y.Generated with Amplifier
Co-Authored-By: Amplifier 240397093+microsoft-amplifier@users.noreply.github.com