fix: green the test suite — 2 real product bugs, 13 stale tests, no CI exclusions - #253
Merged
Conversation
…I exclusions Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
bkrabach
added a commit
that referenced
this pull request
Aug 3, 2026
…bound all waits (#254) The `pytest -m integration` job added in #251 and wired up in #253 hangs forever on the macOS runner. Root-caused on real macOS hardware to a test-harness bug (not a product bug): an un-drained pty master wedges the child on macOS. Both symptoms are harness bugs. The product is correct on macOS. Root cause: macOS wedges an exiting pty child whose output queue is never drained — not slowly, unreapably. A wedged child lands in ps state `?Es` (Exiting, session leader, controlling terminal already revoked). The fix introduces a shared pty harness (`tests/pty_harness.py`) that: - Drains the pty master via a dedicated thread on a dup() so the caller's master_fd keeps blocking-write semantics - Replaces parent-side sleeps with `wait_for_marker()` readiness handshake so sends land inside the child's live window - Bounds all `waitpid` calls — no blocking waits that can hang forever - Removes the un-drained-pty + SIGKILL condition that was a landmine in test_stdout_offload_freeze_integration.py:198 CI hardening: `timeout-minutes: 10` on both jobs. Typical runtime is 20-30s. A hung job now fails loudly in 10 minutes rather than burning runner hours. Verification: 5 consecutive integration runs on macOS, all green, no stray `?Es` children left behind. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
This was referenced Aug 3, 2026
bkrabach
added a commit
that referenced
this pull request
Aug 3, 2026
…255) An agent can declare agents: -- a Smart Single Value that controls which sub-agents its spawned session may delegate to. merge_configs() honors it. The runtime-registry propagation block added later then silently undid it. This commit: - Applies agent_config declarations to the live registry propagation (not just merge_configs) - Ensures same-name local-wins collision avoidance is preserved - Adds comprehensive spawn-level test coverage for all declaration forms - Fixes PR #178's original complaint about allowlists under-delivering Blast radius: zero. Surveyed all 749 config files under ~/.amplifier/cache/ -- 51 agents: occurrences exist, all are dict-shaped agent rosters. Zero access-control declarations in the installed ecosystem, so nothing changes for deployed systems. Preserves from #253: fresh-dict-and-rebind (no cross-session mutation), deepcopy per agent, local-wins. Follow-up to #178 and #253. Generated with [Amplifier](https://github.com/microsoft/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.
What
The 15 pre-existing failures excluded from CI in #251 are now fixed. Zero
--deselectremain.Triage was per-test: is this a stale test, or is it catching a real bug? Two were real.
Real product bugs (2)
1.
provider addsilently did nothing when a stale key sat inkeys.env_claimed_env_vars()unioned inKeyManager().stored_keys(), so any name present in~/.amplifier/keys.envcounted as "claimed" for collision detection. Sinceprovider removedeliberately leaves the key behind (design §8 risk 6), andprovider additself writes it, the first instance of a provider type collided with a credential nobody owned.Reproduced end-to-end with the real CLI, clean
HOME, one staleOPENAI_API_KEY=line inkeys.env:A collision announced against an instance that does not exist, a garbage per-instance var name derived from the module id, and on EOF/non-tty it exits 0 having added nothing. It also made design §5.4.4 (stale-credential warn-and-reuse) unreachable dead code, because
claimed ⊇ stored_keys()meanthas_stored_key(chosen_name)could never be true in production.Fix: split the two notions.
_config_claimed_env_vars()(an instance actually references the name via${VAR}) drives collision detection;_claimed_env_vars()(+ keys.env) stays as-is fornormalize_provider_secrets, where overwriting a saved secret is the real hazard.2.
spawn_sub_session()mutated the parent session's live configmerge_configs()deep-copies theagentsdict only when non-empty, andmerge_agent_dicts()starts from a shallowparent.copy()— so an empty parentagentsdict arrived here as the parent's own object.setdefault()-then-mutate wrote the live agent registry straight into the running parent's config and handed the child the same dict. Observed:Empty today, populated tomorrow. Fix: build a fresh dict and rebind.
Stale tests (13)
test_always_render_final_response,test_cleanup_observability,test_session_lifecycle_eventsmain._process_runtime_mentions; renamed to publicprocess_runtime_mentionsin5b8e995(#210, 2026-07-06). Broken since then.test_handler_methods::TestLoadSkillPromptWithArgs1e6bed1— deliberately, so fork skills receive$ARGUMENTSvia theargumentsparameter. The product is right.Patch targets repointed; prompt expectations updated to the current exact string (kept as
==, not loosened to a substring check).Non-vacuousness verified — each fixed test was confirmed to still fail when the product is broken: reintroduced the pre-#256 render-suppression gate (render tests failed), reintroduced the duplicate
SESSION_ENDemit (assert 2 == 1), swappedCLEANUP_RENDER_END/CLEANUP_STORE_BEGINorder (ordering test failed, unrelated ones correctly still passed), reverted the load_skill f-string (both prompt tests failed).4 new regression tests were added that fail against the product at
ee19d4a— so the two real bugs cannot silently return.CI
--deselectremoved. The full suite runs on every push and PR, both platforms.integrationjob.pyproject.tomlsetsaddopts = -m "not integration", so the 13 tests that fork a real pty and probe real termios state were skipped by default and ran nowhere — including in fix: probe tty fd pollability instead of platform-checking; add CI (Linux + macOS) #251's new CI. They are exactly the tests guarding the dedicated-tty-input mechanism, so they now get their own job on ubuntu + macOS.(main @ ee19d4a for comparison: 15 failed, 1259 passed)
Known follow-up, NOT fixed here
The #233 agent-propagation block in
session_spawner.pysilently overrides an agent's declaredagentspolicy:merge_configs()honorsagents: "none"by settingresult["agents"] = {}, then the propagation loop re-adds every live coordinator agent — re-enabling delegation the agent explicitly disabled. Same for list-form allowlists. Observed: child config{'mode_agent': {'module': 'agent-x'}}for an agent declaringagents: "none".Left out deliberately — it is a delegation-semantics change needing its own verification, not a test fix. The fix is small (gate propagation on
agent_config.get("agents")being absent or"all"). Recorded here because this repo has issues disabled.Follow-up to #247, #250, #251.