system_interface: relocate test-only DI seams off prod-facing signatures#185
Draft
weishi-imbue wants to merge 2 commits into
Draft
system_interface: relocate test-only DI seams off prod-facing signatures#185weishi-imbue wants to merge 2 commits into
weishi-imbue wants to merge 2 commits into
Conversation
Reimplements the DI/test-seam refactor against the FastAPI->Flask migration that landed on main (PR #175 merged it in). The prior approach used FastAPI Depends/dependency_overrides, neither of which exists under flask-sock; this version achieves the same goal with the Flask state container. - create_application drops the agent_manager / claude_auth_service / welcome_resender test-only params. It unconditionally builds and starts the live AgentManager (conftest's AgentManager.start no-op keeps that cheap and observe-free under test). Tests that need a seeded manager build the app, then set state_of(app).agent_manager; auth tests set state_of(app).claude_auth_service / .welcome_resender. - SystemInterfaceState.broadcaster is now derived from agent_manager (a property) instead of a stored field, so a single manager seed repoints the broadcaster too and the two can never diverge. The is_agent_manager_owned flag is gone; the app always owns the manager it builds, so shutdown() always stops it. - Agent messaging: the discover/send collaborators move off the private _send_message_to_agent's defaulted params onto a MngrMessenger FrozenModel that AgentManager owns. The send_message free function and _send_message_to_agent are gone; tests construct MngrMessenger(discover=, send=) directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9aa6145 to
eb9d5d2
Compare
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.
Stacked on #175 (
wz/message-location-from-observe) — base this PR against that branch, notmain. The branch has been merged up to the latest #175 (which itself pulled inmain, including the new latchkey HTTP-client feature).What & why
Every injection point reviewed in #175 (
_send_message_to_agent(discover=, send=),create_application(agent_manager=, claude_auth_service=, welcome_resender=), and thepreconfigured_*app-state keys) is test-only —main.pypasses none of them. They exist to satisfy this package's anti-mock ratchets (nomonkeypatch.setattr, cappedunittest.mock), so collaborators must be injected explicitly rather than patched. The problem is placement: a seam on a public/private call signature or a factory parameter leaks test concerns into the prod surface.This PR keeps the dependencies injectable (ratchet-mandated) but moves the injection points onto FastAPI's own DI, so the prod signatures read as if there were no tests. No prod behavior changes; no ratchet-count changes.
Changes
MngrMessenger— thediscover/sendcollaborators move off the_send_message_to_agentdefaulted-param signature onto aFrozenModelthatAgentManagerowns. Prod callsmessenger.send_to_agent(...); the four branch-table tests constructMngrMessenger(discover=, send=)directly. Thesend_messagefree function and_send_message_to_agentare deleted.Depends—ClaudeAuthServiceandWelcomeResendermove fromcreate_applicationparams +app.statereads toDepends(providers.get_claude_auth_service)/Depends(get_welcome_resender). Tests substitute fakes viaapp.dependency_overrides.ClaudeAuthServicestays long-lived per app (the OAuth subprocess must survive between/startand/submit-code); only its wiring changed.Depends(seam fully removed) — every handler, the two non-route helpers (_find_agent,_get_or_create_watcher, threaded the manager from their callers), and theservice_dispatcherroutes now resolve the liveAgentManagerthroughDepends(get_agent_manager), and its pairedWebSocketBroadcasterthroughDepends(get_broadcaster)(derived from the manager as a sub-dependency, so one override covers both consistently). The lifespan unconditionally builds/starts/owns the real manager; suppressingmngr observein tests is handled independently byconftest'sAgentManager.startno-op. Tests setapp.dependency_overrides[get_agent_manager], socreate_applicationcarries no agent-manager test param, nopreconfigured_*key, and the lifespan has no test branch.get_agent_manageris typed onHTTPConnection(the base ofRequestandWebSocket) so one provider resolves for both HTTP and WebSocket routes. The service-dispatcher websocket is registered via a wrapper, so the wrapper (the actual route) resolves the manager viaDependsand passes it to the helper.One deviation from the original spec
The spec's
get_claude_auth_service()returned a freshClaudeAuthService()per request, which breaks production (the OAuth subprocess must survive across the separate/startand/submit-coderequests;Dependscaches only within one request). The provider reads the long-lived instance offapp.stateinstead — still deletes the factory param while preserving the invariant.Tests
cd apps/system_interface && uv run pytest→ 485 passed, 10 skipped, coverage 87.55%. The 3 remaining non-passes are pre-existing environmental sandbox issues, not regressions (all pass in CI):test_start_observe_spawns_long_lived_subprocess— fails identically on the base branch; this sandbox's mngr profilesettings.tomllacksis_allowed_in_pytest = true.test_e2e.py(2) — Chromium not installed (deferred-install); the fixtures run past theirdependency_overridessetup and only the browser launch fails.Ratchet counts unchanged (
dependency_overridesis neithersetattrnorunittest.mock).🤖 Generated with Claude Code