chore: e2e-first testing; delete the unit suite, relocate release and contract guards - #109
Merged
David Koleczek (DavidKoleczek) merged 7 commits intoAug 4, 2026
Conversation
CI has been red on main for several commits, on both jobs. pyright: resources.py deferred-imports amplifier_module_hooks_mode and amplifier_module_tool_skills.discovery. These are resolved at bundle-prepare time and added to sys.path then, so they are never in the venv on any machine. Both call sites already treat failure as expected (try/except ImportError, and a guarded RuntimeError after _ensure_discovery_importable). Suppressed at the import site with a comment explaining why, rather than declaring dev dependencies that would pin a git revision of something resolved at runtime. vitest: transport.test.ts timed out at 15s. Not flake and not a slow test. Transport.terminate() resolved on the child's 'close' event, which fires only once every stdio pipe hits EOF. Those write-ends are inherited by grandchildren, so a forked `sleep` kept them open and terminate() blocked for the orphan's lifetime (60s), not the child's. A previous commit raised the timeout to 15s, which could never have worked against a 60s block. terminate() still settles on 'close' whenever the pipes are closable, so the frame-delivery guarantee for well-behaved children is unchanged. On 'exit' the child is already dead and its output already in the pipe, so after a 250ms drain grace the stdio handles are destroyed and the promise settles. The grace timer is unref'd so it cannot hold the event loop open. Transport is exported from src/index.ts, so this was a real hang in the published amplifier-agent-ts package, not just a test artifact. The test now spawns `sh -c "sleep 60 & wait"` to force the fork deterministically, and asserts terminate() completes in under 5s. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Nine files under tests/ were never unit tests. They were release-path and
contract guards that happened to be stored as pytest files. Moving them to
homes that match what they actually do, so that tests/ can mean exactly one
thing: the e2e contract.
Placement follows established convention rather than a single dumping ground:
stdout discipline -> ruff T20 + TID251 banned-api on sys.stdout
was tests/test_stdout_discipline.py, a hand-rolled
AST scan. src/amplifier_agent_lib has zero violations
today, so the rule drops in clean. amplifier_agent_cli
is exempt: it owns stdout by design.
codegen freshness -> scripts/verify-codegen.sh
regenerate into a temp tree and diff, the same shape
as kubernetes hack/verify-codegen.sh. Guards the
schemas that wrappers/typescript/scripts/gen-types.ts
turns into the published TS types.
wheel contents -> scripts/verify-wheel.py
now globs src/amplifier_agent_lib/bundle/**/*.md and
asserts every match ships. The previous tests checked
hardcoded filenames, which left bundle/skills (8 paths)
and bundle/modes (2 paths) verified by nothing.
version consistency -> scripts/verify-versions.py
replaces `assert PROTOCOL_VERSION == "0.3.0"`, which
pinned a literal rather than checking agreement. Now
compares 15 pins across the engine, both wrappers,
README, and the conformance fixtures.
wire parity -> wrappers/conformance/verify-parity.py
the driver now lives with the fixtures and runners it
drives, as in gRPC interop and OpenTelemetry. This is
the only cross-language check that exists.
verify-parity also closes a hole: parity alone accepts two runners that agree
on failure. It now treats `passed: false` in either runner as fatal. The
deleted mirror suites asserted success for a hardcoded list of seven fixtures
while ten exist, so three had drifted in unverified.
wrappers/conformance/test/ and tests/ are removed as superseded; neither ran in
CI. `pnpm test` there now runs verify-parity.py.
A Makefile provides the single command surface: `make check` is the fast gate,
`make verify` is the pre-PR gate, `make e2e` and `make eval` drive the contract
and behavior tiers.
Drops pytest-timeout (already unused, no --timeout or mark.timeout anywhere)
and jsonschema (used only by a deleted test). pytest-asyncio stays: amplifier-core
registers a pytest plugin that imports it unconditionally, so collection fails
without it.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Removes 125 in-process test files: tests/*.py, tests/cli/, tests/http/, tests/config/, tests/integration/, tests/bundle/, plus tests/conftest.py, tests/provider_env.py, tests/__init__.py, and wrappers/python-py/tests/. The suite tested implementation rather than contract, and its presence made `pytest tests/` mean two different things depending on whether a DTU happened to be warm. tests/e2e/conftest.py self-skips only when no DTU is available, and the dev skills guarantee a warm one at exactly the moment they told you to run the "fast gate", so that command silently ran all eight DTU suites. tests/e2e/ is unaffected: it imports nothing from the deleted files and does its own sys.path setup. Collection still yields 56 tests, and 45 of those (all but github_copilot, which needs GITHUB_TOKEN) were verified green in a DTU. wrappers/python-py/tests/ goes too: that package has never been released, has zero py-v* tags, and its tests were in neither root testpaths nor CI. Everything worth keeping was extracted to scripts/verify-* and wrappers/conformance/verify-parity.py in the previous commit. Also repoints two stale references to deleted files: admin/verify.py cited tests/test_runtime_hook_mount.py in a message that ships inside the wheel, and a modes e2e docstring pointed at tests/http/ as the place to add a unit test. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
ci.yml ran `pytest tests/ -q` as its Python gate. After the unit suite deletion that step collects only DTU e2e tests, which self-skip on a GitHub runner. It would have been a green gate verifying nothing. Replaced with the Makefile targets, one step each so failures are attributable, and a comment explaining why pytest is deliberately absent. ci.yml now also triggers on v*, py-v*, and wrapper-v* tags. It previously ran only on push-to-main and pull_request, so a tag push ran zero verification before publishing. Concurrency no longer cancels in-progress runs on tags. install-script.yml hardcoded `--tag v0.9.0` while pyproject is at 0.12.0, so it validated a three-versions-stale release and never the code under review. It now installs the pushed tag, or resolves the latest release the way a customer does. Dropped --no-prime so amplifier-agent-post-install actually runs, and added an assertion that priming produced a manifest: install.sh downgrades a priming failure to a warning and still reports success, so running it is not enough. Added a tag trigger, since path filters do not apply to tag pushes. publish-python.yml built and uploaded to PyPI with no verification at all. It now runs scripts/verify-wheel.py before the build and upload, so a wheel missing the protocol spec, schemas, or bundle content cannot publish. This lives inside the publish workflow rather than relying on ci.yml because the two run in parallel on a tag with no cross-workflow dependency. publish-wrapper.yml and release-notes.yml are unchanged. Note that release-notes.yml marking every wrapper-v* tag as a prerelease is load-bearing by accident: it is what keeps wrapper tags from winning the releases/latest lookup that install.sh depends on. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
The docs and dev skills are the primary drivers of automated development in this repo, so a stale gate command in them actively misleads future sessions. This sweeps every reference invalidated by the preceding commits. Two corrections were not staleness but live defects: Both dev skills prescribed `cli.py run --fresh` as the clean-box rung of the verification ladder, and gated their handoff checklist on it. That flag has never existed. cli.py sets ignore_unknown_options, so click accepts it and passes it to pytest, which exits 4 with "unrecognized arguments" -- after provision() has already rebuilt the DTU. Seven references corrected to `cli.py run <suite>` without --skip-setup, which already provisions fresh and is what docs/E2E_TESTING.md documented all along. The release sweep in start-release-process listed notes/ among directories to delete, which would have wiped the new coverage notes on the first release. Removed, marked durable, and given a row in the AGENTS.md layout table. Also fixed: bugfix/SKILL.md prescribed `pytest tests/ -m "not dtu"`, which now selects nothing and exits green; both skills presented `pytest tests/ -q` as a fast gate it never was; the release skills claimed the publish workflows run no tests, which is now only partly true; README claimed the engine refuses to run against an outdated storage layout, which it does not and AGENTS.md already said so; AGENTS.md contradicted itself on whether make e2e needs a DTU; two admin commands were missing from the console-script lists; and the tmux cleanup mechanic was rescoped to the eval harness, since the e2e image has no tmux. AGENTS.md gains a "three tiers" section stating plainly that there is no unit test tier and that this is intentional. notes/e2e-coverage-gaps.md records what lost coverage, as a queue for /amplifier-agent-new-feature rather than a silent loss. notes/foundation-pin-reproducibility.md records that amplifier-foundation is pinned to a branch and uv tool install does not read uv.lock, so the tested artifact and the shipped artifact are not provably the same code. No CI change closes that one. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
timeout-longwindow-integration.test.ts spawned a mock engine that slept 12 real
seconds, twice. That was 24s of the suite's 24.76s total and roughly 40% of CI
wall clock.
Its three cases are already covered by session-subprocess.test.ts, in the same
directory, using 300ms observation windows instead of real-time sleeps:
(k) timeoutMs: 0 -> no engine_hung same regression guard, same
explanation of the ?? behavior
(l) timeoutMs: undefined -> no engine_hung no silent default timer
(e) timeoutMs: 250 -> engine_hung fires
(j) timeoutMs: 150 -> engine_hung fires
The long sleep also did not test what its header claimed. It said 12s proved a
disabled timer was "truly OFF, not just delayed", but the silent default it
guarded against is DEFAULT_TIMEOUT_MS = 10 minutes (session.ts). A 12 second
window cannot detect a 10 minute timer. What it did catch, setTimeout(fn, 0)
firing immediately, happens in under 10ms.
vitest: 24.76s -> 1.28s, 121 tests passing.
make verify: 34s -> 11s.
What is genuinely uncovered now is the same contract over a real long-running
turn through the public spawnAgent() API rather than a mock engine through
SessionHandle. That is an e2e concern and is recorded in
notes/e2e-coverage-gaps.md. ISSUE-002 references repointed at the
session-subprocess cases that provide the positive and negative controls.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
3149 files under node_modules/ were committed because the root .gitignore had no rule for it. They include darwin-arm64 binaries (@esbuild, @rollup, fsevents), so running pnpm install on Linux rewrites hundreds of tracked files and pollutes every subsequent diff. node_modules is platform-specific install output, not source. wrappers/conformance already ignores its own copy; this adds the missing rule at the root. Reinstall with `pnpm install`. Nothing references the committed copy: CI installs deps itself, and the Makefile auto-installs conformance deps when absent. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
David Koleczek (DavidKoleczek)
changed the title
David Koleczek (DavidKoleczek)
marked this pull request as ready for review
August 4, 2026 21:47
Salil Das (sadlilas)
approved these changes
Aug 4, 2026
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
2 participants
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.
Stacked on #108.
Makes this repo safe for spec + e2e + eval driven development by removing the parts that contradict it, and relocating the parts that were mislabeled.
The problem
tests/held ~920 in-process tests alongside 56 DTU e2e tests. That madepytest tests/mean two different things depending on whether a DTU happened to be warm, and the dev skills guaranteed a warm one at exactly the moment they told you to run the "fast gate". CI had been red onmainfor several commits, so red carried no signal.Nine files in
tests/were never unit tests. They were release-path and contract guards stored as pytest files.Target state
There is no unit test tier. That is intentional and now stated plainly in AGENTS.md.
Guard relocation
Each guard went to a home matching what it does, rather than one folder:
Two got stronger in the move.
verify-wheel.pynow globsbundle/**/*.mdinstead of checking hardcoded filenames, closing a gap wherebundle/skills/(8 paths) andbundle/modes/(2 paths) were verified by nothing.verify-versions.pyreplacesassert PROTOCOL_VERSION == "0.3.0", which pinned a literal rather than checking that 15 pins across the engine, both wrappers, README, and the fixtures agree.Three real bugs found along the way
A hang in the published npm SDK.
Transport.terminate()resolved on'close', which fires only after every stdio pipe hits EOF. Grandchildren inherit those pipes, so a forked subprocess blockedterminate()for the orphan's lifetime. A previous commit had raised the vitest timeout to 15s treating it as slowness; the actual block was 60s, so no timeout value could have worked.Transportis exported fromsrc/index.ts, so this affected consumers.A phantom flag in both dev skills. They prescribed
cli.py run --freshand gated their handoff checklist on it. That flag has never existed.cli.pysetsignore_unknown_options, so click passes it to pytest, which exits 4 afterprovision()has already rebuilt the DTU.Parity accepted mutual failure.
verify-parity.pynow treatspassed: falsein either runner as fatal. The deleted mirror suites asserted success for a hardcoded list of seven fixtures while ten exist, so three had drifted in unverified.Release path is now gated
ci.ymldid not trigger on tags, so a tag push ran zero verification before publishing. It now does.publish-python.ymlrunsverify-wheel.pybefore upload.install-script.ymlhad--tag v0.9.0hardcoded while the repo sits at 0.12.0, so it validated a three-versions-stale release; it now installs the pushed tag and exercises bundle priming instead of skipping it.Verification
Validated in a live DTU built from this tree, not just on the dev box:
An adversarial docs audit executed every command in every doc and skill and checked every factual claim. Defects it found are fixed in this PR.
Speed
The long-window timeout test slept 12 real seconds twice, which was 24s of the 24.76s TS suite. Its cases are already covered by
session-subprocess.test.tswith 300ms windows. Its 12s window also could not detect the 10-minuteDEFAULT_TIMEOUT_MSit claimed to guard against.The five contract gates total ~3.6s. Remaining CI time is dependency installation.
Known, recorded, not fixed here
notes/foundation-pin-reproducibility.mdrecords thatamplifier-foundationis pinned to a branch anduv tool install --from git+does not readuv.lock, so the tested and shipped artifacts are not provably the same code. No CI change closes it.notes/e2e-coverage-gaps.mdqueues the behavior that lost coverage, as input to/amplifier-agent-new-feature, rather than letting it vanish silently.Also unfixed and pre-existing: the wheel published to PyPI is uninstallable on its own, because
amplifier-foundationcomes from[tool.uv.sources], which a wheel cannot carry. Git is the supported channel, but that artifact still publishes on everyv*tag.