refactor(agents): decompose create_agent_internal behind a characterization-test net (#1484)#1691
Merged
Conversation
Pin the observable behavior of `create_agent_internal` BEFORE the #1484 phase-helper decomposition, so the pure move can be proven byte-identical. New `tests/unit/test_1484_create_agent_characterization.py` (40 tests) extends the `test_fork_to_own` harness (patch.dict sys.modules, whole `db` object swapped, `monkeypatch.delitem` purge) with the mocks crud drifted past: the tuple-returning `resolve_github_pat` (#162), the ephemeral pre-gates/reserve (ent#69), entitlement, rate_limiter, pull_mode, and agent_runtime_state. Coverage: local/github(predefined+dynamic)/no-template/request=None happy paths asserting the exact `containers_run` kwargs (network='trinity-agent-network', cap_drop, tmpfs, mem_limit/nano_cpus), the full base+github env key-set and the config/credentials/workspace bind mounts; Claude subscription auto-assign vs non-Claude skip; ephemeral SUCCESS with deterministic token_hex proving the suffixed name threads to every sink + clear_agent_breakers-before-container ordering + volume-less ghost; the ephemeral gate ORDER (earliest-wins overlap); PAT tier persistence (global/per_user/fork); #1664 volume-base 409 preceding the #1667 probe; #1667 refuse-vs-adopt; existence guard; role quota; runtime validation; non-fatal side-effect failures still 200; a 4-case rollback matrix injecting failure AFTER a successful container; and two PINNED-LEAK tests (ephemeral-429 MCP-key leak, docker-unavailable else MCP-key+git-config leak) that assert the leak PERSISTS (preserved exactly, AC#3). Also repairs `test_fork_to_own`'s `_load_crud` (D7): add the `resolve_github_pat` tuple stub that 17d0c8e silently broke (8 github-path tests ValueError'd on tuple-unpack). No assertion changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) Decompose the pre-docker half of `create_agent_internal` into named phase helpers (fenced section), keeping behavior byte-identical (pinned by the #1484 characterization suite). Each helper returns its handles into the orchestrator's existing local variables, so the untouched docker try-block stays coherent. Extracted (AC-forced fat blocks + rollback-adjacent seams): - `_apply_ephemeral_pregates` (ent#69 ghost gates + name suffix) - `_guard_leftover_workspace_volume` (#1667) - template resolution: `_resolve_template` → `_parse_github_ref`, `_resolve_github_repo_and_pat`, `_apply_fork_to_own`, `_validate_github_access`, `_reserve_git_instance` (S7 #382), `_resolve_local_template` - `_stage_config_files` (cred staging + agent-config.yaml + #1197 resource normalize) - env: `_build_env_vars` → `_build_base_env`, `_apply_subscription_env`, `_apply_gemini_and_otel_env`, `_apply_mcp_and_auth_env`, `_apply_github_env` Adds the `_TemplateResolution` dataclass (set-once template outputs) and the `_RollbackHandles` dataclass (used by the docker-block extraction next). The whole github+fork phase stays OUTSIDE the docker try-block (FORK_* 4xx errors must not flatten to 500). CC-trivial linear gates (name sanitize, existence guard, clear_agent_breakers, role quota, runtime finalize, MCP-key mint, ephemeral-slot reserve) stay inline. No signature change; callers unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ck (#1484) Complete the #1484 decomposition: extract the docker try-block phases and the except/else rollback, and (to satisfy AC #4's hard CC<20 / <100-SLOC gate on the orchestrator itself) the remaining fat inline gates. Behavior stays byte-identical (pinned by the characterization suite; 148 create-path tests green). Docker-block helpers: - `_build_volume_mounts` → `_workspace_volume_mount`, `_shared_folder_mounts`, `_public_volume_mount` - `_create_agent_container` (network='trinity-agent-network' HARD-CODED here — AC #5) - `_broadcast_agent_created` - `_register_agent` (ownership + spawn edge + subscription + avatar + perms) - `_materialize_agent_files` (persistent-state #383 + data_paths #1169 + auto-sync) Rollback (AC #3, preserved exactly, read the orchestrator-populated `_RollbackHandles`): - `_rollback_failed_creation` (git-config + ephemeral slot + MCP key; NO container/volume cleanup — left for the cleanup watchdog) - `_release_ephemeral_on_no_docker` (slot only; MCP-key + git-config leak preserved) Gate helpers (extracted to bring the orchestrator under CC 20 / 100 SLOC): `_check_name_availability` (#834/#1664), `_enforce_role_quota` (QUOTA-001), `_mint_agent_mcp_key`, `_reserve_ephemeral_slot`. The `if docker_client: try/except/else` stays INLINE in the orchestrator so what is caught is byte-identical. Result (radon): create_agent_internal CC 187→10, 945→87 SLOC; every helper CC≤16, SLOC≤64. No function exceeds CC 20 or 100 SLOC. No signature change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-robust (#1484) `test_create_path_clears_breakers_before_the_container_exists` used a whole-file `src.index("await containers_run(")` to assert the create path clears breakers before creating the container. The #1484 decomposition moved the raw `containers_run(` calls into phase-helper defs ABOVE the orchestrator, so the file-level text order flipped while the RUNTIME order is unchanged. Assert the ordering within the orchestrator body against the `_create_agent_container` phase call instead; the runtime clear→run order is additionally pinned behaviorally by the #1484 characterization suite (test_case6). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… phase layout (#1484) - Network Topology: the stale `crud.py:583` hard-code citation is corrected to cite the three container-create sites BY FUNCTION (drift-proof): crud.py::_create_agent_container, lifecycle.py::_provision_folders_and_run_agent_container, system_agent_service.py::SystemAgentService._create_system_agent. - Services catalog: add a crud.py entry documenting the #1484 orchestrator + fenced phase-helper + _RollbackHandles pattern (with the "do not re-monolith / keep helpers <100 SLOC / CC<20 / github+fork stays outside the try / network hard-coded in _create_agent_container" invariants) so it isn't re-monolithed; names the creation_phases.py module split as the deferred #1028 follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1484) /sync-feature-flows: #1484 is a pure decomposition (no new capability), so no new flow doc is created. Add a phase-helper note to the canonical agent-creation flow (agent-lifecycle.md) — create_agent_internal is now a thin orchestrator over fenced phase-helpers + _RollbackHandles; the numbered Business-Logic line refs are now stale (helper-relocated), canonical structure in architecture.md — plus a dated Revision-History row. Append the corresponding Recent Updates row to the feature-flows.md index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Resolve by running |
vybe
approved these changes
Jul 19, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: characterization-first pure move (CC 187→10), rollback matrix included, repairs the #1686 test_fork_to_own harness breakage on dev tip. Security greps clean, no packaging impact, all checks green. Approving.
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.
Fixes #1484
Summary
Decomposes
create_agent_internal— the single fattest function in agent creation (CC 187 → 10, orchestrator SLOC ~860 → 87) — behind a characterization-test net, as a pure move with byte-identical behavior. Callers and the public signature are unchanged._*), each ≤ CC 16 / ≤ 64 SLOC: ephemeral pre-gate, template/fork resolution, config staging, env build, volume mounts, container create, register, materialize.if docker_client: try/except/elseand the CC-trivial gates stay inline, so what is caught is byte-identical (thegithub:+fork phase stays OUTSIDE the try, soFORK_*4xx don't flatten to 500)._RollbackHandlesdataclass (agent MCP key + git-config reservation + ephemeral slot) — the except/else read only that._create_agent_container(AC Setup improvements #5).Test-first evidence
tests/unit/test_1484_create_agent_characterization.py) was written and proven green against the unmodifiedcreate_agent_internalBEFORE any extraction — it is the behavior contract for the pure move (exactcontainers_runkwargs, full base+github env key-set, bind mounts, Claude auto-assign vs non-Claude skip, the ephemeral gate ORDER, PAT-tier persistence, bug: #1581 orphan-volume sweep can destroy a live renamed agent's home volume (rename keeps old volume name+label) #1664/bug: agent creation silently reuses a pre-existing Docker volume of the same name (get-then-create, no adopt/refuse decision) #1667 volume-base gates, existence guard, role quota, non-fatal side-effect failures still 200).delete_git_config/ …) asserting the orphaned container is never stop/removed.Verification
test_1474(unrelated, red ondevtip too).D7 note — harness repair
This PR also repairs the
test_fork_to_ownharness that #1686 (17d0c8ef, per-user GitHub credentials) silently broke ondev— itsresolve_github_pattuple swap left 8 tests red on thedevtip; they are green here (one-line tuple-stub fix folded into the extended harness the characterization suite shares).Follow-ups (for Andrii to file)
elsepath leaks the MCP key + the git-config reservation. Both are pinned by tests marked# PINS A KNOWN LEAKthat assert the leak PERSISTS — so a future fix flips them deliberately.creation_phases.py(deferred; this PR keeps them fenced incrud.py)..claudetest-runner catalog entry for the new suite is prepared locally but left uncommitted (the.claudesubmodule pointer is intentionally not touched);tests/registry.jsonIS updated in this PR.CodeQL note
The 8
py/path-injectionHIGH alerts CodeQL raised against this PR were all verified false positives / move-not-introduction and dismissed (no code change, per the PR #1203 precedent): 3 (alerts 238-240) consume the return of the_safe_local_template_pathbarrier (regex allowlist +resolve()/is_relative_to()), 5 (alerts 110-114) build/tmppaths fromconfig.namewhichsanitize_agent_namecharset-restricts — the latter 5 are pre-existing OPEN alerts onmainsince 2026-05-19, mis-counted as new because the large diff broke CodeQL's line-mapping (the gate's own "code changes too large" caveat). All sinks are byte-identical to theorigin/devmonolith. The CodeQL check is now green.🤖 Generated with Claude Code