test(e2e): add coexistence suite, --repo mirroring, and fix two suite defects - #145
Merged
Merged
Conversation
The harness could only mirror amplifier-agent, plus amplifier-core and amplifier-foundation when their working trees were dirty. A dependency whose changes needed exercising in the DTU had no way in, and a clean checkout parked on a branch was silently skipped because the criterion was working-tree dirtiness rather than "differs from upstream". `--repo <name>[@<ref>]` on up/run/refresh mirrors any additional repo to the Gitea mirror and injects its url_rewrites rule, so the DTU resolves it from there. Three resolution cases: a local checkout with no ref uses the existing working-tree snapshot, a local checkout with a ref pushes that ref's commit, and no local checkout clones from GitHub. Rules are injected into the staged temp copy of the profile, so the checked-in profile is never mutated, and with no --repo the profile is copied byte-identically. Source repos are never mutated either: every resolve and fetch runs inside a throwaway clone. Pushes only ever target the local Gitea container. This also retires the hand-maintained "add rules here" comment and the hardcoded warning check, since redirection is now derived from what was actually mirrored. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…-cli Every existing suite runs in a container where amplifier-app-cli was never installed, so ~/.amplifier is nearly empty. That is the easy case. On a real machine that tree holds app-cli's live module clones, and "amplifier-agent leaves them strictly alone" was asserted by inspection rather than by test. The suite installs app-cli inside the DTU, primes its bundle cache so the tree holds real clones, records the state of ~/.amplifier, exercises amplifier-agent hard, records again, and asserts nothing changed. It then runs app-cli once more, which is the user-facing failure the guarantee exists to prevent. Five tests: the before/after comparison, app-cli still working afterwards, remote skill clones landing in the agent's own tree, doctor's isolation guard actually firing rather than being vacuous, and clone directories being independent storage where their names collide. The install lives in the suite's own conftest rather than the DTU profile's setup_cmds, so only this suite pays the download and a normal run of anything else is unaffected. Two notes on the assertions. The comparison uses path, size and mtime rather than content hashes, because app-cli's cache is large and a content change without a size or mtime change is not a realistic failure mode for git clones. And the isolation guard is exercised through AMPLIFIER_AGENT_FOUNDATION_HOME, not by unsetting AMPLIFIER_HOME, which the library rebinds at import so that path cannot fail from a subprocess. Also documents --repo from the previous commit. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…liable Two pre-existing defects found while running the full suite. Both are independent of the coexistence work. raw_capture and shadowing both hardcoded port 9098 for their own in-DTU server. raw_capture's fixture is session-scoped, so it holds the port for the whole run and its teardown does not fire until the very end. Since collection order puts it first, shadowing could never bind and test_skill_shadow_reported_http errored with "address already in use". Deterministic, not flaky: each suite passed alone and failed together, reproduced identically three times. Ports now live in one place, tests/e2e/framework/ports.py, naming each port and its owner, so a new suite cannot silently reuse one. shadowing moves to 9097. raw_capture's teardown also used a pkill pattern that could match its own argv and kill itself, orphaning the server; the bracketed-digit idiom shadowing already used is promoted into that same module and both now share it. The streaming timing assertion measured total elapsed between the first and last content frame against a 0.5s floor. That scales with how much the model chooses to say, which the test does not control, so a genuinely streamed but short response failed: six frames spanning 0.428s, an 86ms mean gap, reported as "looks buffered". It now measures the largest gap between consecutive frames. A streamed response has at least one real pause; a buffered dump has every gap at the measurement floor. That property does not depend on response length. Threshold is 50ms, roughly 6x below the slowest healthy observation (299ms) and 38x above the 1.3ms floor, chosen from measurements across repeated runs rather than intuition. Max rather than mean because a leading burst of frames is normal and drags the mean down while the max stays healthy. streaming-mode now asserts SSE structure only, expressed as a third value in the CASES table so it is visible where the cases are defined. It intermittently emits all content frames within a few milliseconds at the end of an 8-10 second turn, about 3 times in 12 observations, while streaming-plain over the same model and provider was clean in 9 of 9. The cause is not established, so the case comment records the measurements, states that no cause is known, and notes that timing coverage on the active-mode path is given up until it is understood. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
David Koleczek (DavidKoleczek)
marked this pull request as ready for review
August 24, 2026 22:20
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.
What this does
Adds end-to-end coverage for a guarantee that previously had none, and fixes two pre-existing defects in the e2e suite found while running it.
Test infrastructure only. No engine, CLI, wrapper, or protocol changes.
Why
0.15.0 established that amplifier-agent operates from its own
~/.amplifier-agentand leaves amplifier-app-cli's~/.amplifierstrictly alone. Nothing intests/e2e/asserted that. Every existing suite runs in a container where app-cli was never installed, so~/.amplifieris nearly empty and the interesting case is never exercised. On a real machine that tree holds app-cli's live module clones.The coexistence suite
Installs amplifier-app-cli inside the DTU, primes its bundle cache so the tree holds 28 real clone directories, records the state of
~/.amplifier, exercises amplifier-agent hard (run,doctor,config show,update --check,skills list,modes list,cache clear, then a second real turn), records again, and asserts nothing changed.Five tests:
app-cli is installed from the suite's own
conftest.pyrather than the DTU profile'ssetup_cmds, so only this suite pays the download.Notes on two assertions that are less obvious than they look:
AMPLIFIER_AGENT_FOUNDATION_HOME, not by unsettingAMPLIFIER_HOME.amplifier_agent_lib/__init__.pyrebinds that variable at package import, so the unset branch cannot be reached from a subprocess and a test written that way would pass vacuously.--repofor the harnessThe harness could only mirror amplifier-agent, plus amplifier-core and amplifier-foundation when dirty. A dependency whose changes needed exercising in the DTU had no way in, and a clean checkout on a branch was skipped because the criterion was working-tree dirtiness rather than "differs from upstream".
--repo <name>[@<ref>]onup/run/refreshmirrors any additional repo and injects itsurl_rewritesrule. Rules go into the staged temp copy of the profile, so the checked-in profile is never mutated and a run without the flag copies it byte-identically. Source repos are never mutated: resolve and fetch run inside a throwaway clone. Pushes only ever target the local Gitea container.Two pre-existing defects fixed
Port collision.
raw_captureandshadowingboth hardcoded 9098.raw_capture's fixture is session-scoped, so it holds the port for the entire run and its teardown does not fire until the end; collection order puts it first, soshadowingcould never bind. Deterministic rather than flaky: each suite passed alone, failed together, reproduced identically three times. Ports now live intests/e2e/framework/ports.pynaming each port and its owner, andshadowingmoves to 9097.raw_capture's teardown also used apkillpattern that could match its own argv and kill itself, orphaning the server; the bracketed-digit idiomshadowingalready used is promoted into that module and shared.Streaming timing assertion. It measured total elapsed between first and last content frame against a 0.5s floor. That scales with how much the model says, which the test does not control, so a genuinely streamed short response failed: six frames spanning 0.428s, an 86ms mean gap, reported as buffered.
It now measures the largest gap between consecutive frames. A streamed response has at least one real pause; a buffered dump has every gap at the measurement floor. That does not depend on response length. Threshold 50ms, about 6x below the slowest healthy observation (299ms) and 38x above the 1.3ms floor, chosen from repeated measurement. Max rather than mean because a leading burst of frames is normal and drags the mean down while the max stays healthy.
One thing to look at
streaming-modenow asserts SSE structure only, with no timing assertion.It intermittently emits all content frames within a few milliseconds at the end of an 8-10 second turn, roughly 3 times in 12 observations. The connection opens promptly in those runs (first byte ~57ms), so the transport is not stalling, and
streaming-plainover the same model and provider was clean in 9 of 9.The cause is not established. The plain path being stable against the same API suggests a difference between the two paths rather than the API itself, but that has not been traced, so the case comment records the measurements and asserts no cause.
What this gives up is stated in the code: nothing now asserts streaming behavior on the active-mode path, and the suite docstring was corrected because it previously claimed coverage that no longer holds. Swapping the case back to
"stream"restores it once the burst is understood.Verification
The 4 skips are the
vllmsuite self-skipping with no server configured. Before this branch the same run gave 59 passed, 4 skipped, 1 error.Confirmed against merged
main, including the two follow-up fixes that landed with #142, so the before/after comparison covers the ChatGPT OAuth token copy-forward and theAMPLIFIER_AGENT_HOMEinjection.