refactor: framework review 2026-08 — tool identity, lifecycle, sessions, jobs - #110
Merged
Conversation
Permission gating and tool assembly resolved tools by ``__name__``, so an unregistered callable named after a genuine tool inherited its capabilities — including EXEMPT — and was handed its services and its long-running contract. Identity is now a per-``ToolRegistry`` ``id(obj) -> (weakref, definition)`` map confirmed with ``is``: no name path, no equality, and a recycled address inherits nothing. Everything the framework issues is bound at its construction site (registered callables, factory service-bound variants, renamed wrappers, the native ADK skill tools). Anything unbound is denied, and left exactly as the application supplied it during assembly. Keeping the map on the instance is also what lets a short-lived registry and its closures be collected. A name may now mean only one thing: ``register()`` raises on any duplicate. Sharing is declared, never inferred — ``declare_tool(name, ...)`` states a contract with no backend-neutral implementation, and each backend registers its own with ``register_tool(..., variant_of=name)``. The ADK and LangGraph save_plan/get_plan/save_tasks/get_tasks are declared once in ``tools/_core/state_tools.py``; previously they contested the name and import order decided the winner. ``canonical_for()`` keeps assembly from ever yielding a declaration's absent ``func``, and ``replace=True`` retires the old definition's identities so they resolve to nothing. ``service_registry.KNOWN_SERVICE_KEYS`` lands here rather than with the tool declarations that use it: ``registry._validate_requires`` imports it at module scope, so the mechanism and its vocabulary cannot be separated. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
``BaseWorkflowManager._TOOL_SERVICE_MAP`` was a central name→service table, so a downstream tool that needed a framework service could only get one by editing the framework — and it matched on ``__name__``, which meant an application function named ``kb_search`` had a knowledge base built for it even though the permission engine would deny the call. Tools now declare their own needs with ``@register_tool(..., requires=...)``, validated at registration against ``service_registry.KNOWN_SERVICE_KEYS`` so only genuinely constructible services can be declared (``user_kb_manager`` is created together with ``kb_manager``, and the error says so). Detection reads that metadata off the registry by identity, so ``register(func, name=...)``'s original callable still declares its services while an unregistered lookalike declares nothing. There is still no mechanism for registering new service *types*. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
Multiple roots were silently accepted and all but one tree was unreachable — the runner starts from a single root, so those agents could never run. Duplicate names, dangling ``sub_agents`` references, self-references, delegation cycles and a child claimed by two parents were likewise only discovered as a half-built hierarchy, after model discovery and service creation had already paid for themselves. ``validate_agent_graph()`` now returns a validated ``AgentGraph`` (config map, dependency-ordered build order, root) and raises ``AgentGraphError`` naming the offending agents — before any allocation or network call, since a bad graph is a static configuration error. Agents are built in dependency order, so declaration order no longer changes the hierarchy. Prompt factories are resolved under the manager's settings rather than the global singleton: a factory may take no arguments (including all-defaulted ones) or exactly one settings argument. Other signatures, ``async def`` factories and non-string results are rejected by name instead of producing a coroutine object as an agent's instruction. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
…covery A manager's own model — ``Manager(model=...)``, ``reinitialize(model=...)``, or a ``settings.get_model()`` cached before discovery ran — was never in settings and so never validated: an unusable id reached the provider at first use, and a deprecated alias kept being sent even though ``check_model()`` already knew its replacement (only ``set_model()`` wrote one back). Every effective model now goes through one all-or-nothing pass via the internal ``_validate_settings_with_models()``, and the resolved ids are applied only after all of them validate. ``validate_settings()`` itself is unchanged and still returns None. Validation also runs *after* discovery: the static fallback list would otherwise reject a model that exists but postdates it. Discovery authority is tracked per provider, so a Google outage no longer makes the Claude listing non-authoritative — nor an Anthropic outage make Anthropic's hardcoded fallbacks authoritative. An unknown model is never silently swapped for a near neighbour; it is an error naming what was asked for. Also here, since they are the same listing path: the provider SDK clients are closed after a listing rather than left to GC with their connection pools open, and the blocking Anthropic listing runs on a worker thread so startup does not block the event loop. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
``BaseSettings(google_api_key="…")`` silently bound nothing. Each credential field declared only its environment-variable ``validation_alias``, so with ``populate_by_name`` off the field name was not an accepted input at all and ``extra="ignore"`` swallowed the kwarg — the setting kept its default and the caller got an unauthenticated client with no error. Every credential now accepts both names via ``AliasChoices``, with the env name first so a real environment variable still wins within a source. Because the field name is now accepted, a misspelled credential kwarg would be dropped just as quietly, so constructor kwargs that *look* like credentials but match no field are rejected by name. Credential values are also kept out of ``repr()``. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
A conversation's identity is the ``(app_name, user_id, session_id)`` triple, but the session hooks took only a session id and implicitly used the manager's default user — so listing, deleting, or reading the history of another user's session silently answered about the wrong conversation, or came back empty. ``session_exists``/``list_sessions``/``delete_session``/``recent_messages``/ ``load_session``/``save_session`` now take an optional ``user_id``, defaulting to ``settings.default_user`` only when the caller omits it, and ``on_session_end(session=SessionRef(...))`` reads the conversation it is given. Base-class helpers still call the backend hooks *without* ``user_id`` when it is the default, so a downstream override that never added the parameter keeps working. The in-flight ``(user, session)`` is a ContextVar set with a token by ``_workflow_context()``, so concurrent turns on one manager stay isolated and nesting restores the outer turn. A backend with no durable store now leaves ``supports_sessions`` False and the base hooks raise ``NotImplementedError`` rather than answering ``False``/``[]`` — an empty list read as "you have no saved sessions" when the truth was "this backend keeps none", which ``/sessions`` now says outright. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
A manager ran turns concurrently against one backend, and lifecycle mutation could land in the middle of one. ``process()``/``resume_with_job_result()`` now enter through ``_turn_admission()`` (turn lock) while ``initialize_services``/``reinitialize``/``cleanup`` hold the lifecycle lock *and* the turn lock. Lock order is lifecycle → turn, so a turn initializes before taking the turn lock — which is what keeps the two from deadlocking, and is also why a queued cleanup could run in between and leave the turn holding admission to a released backend (a ``None`` runner, surfacing as an AttributeError deep inside ADK). Admission therefore re-checks ``_backend_ready()`` while holding the turn lock and reinitializes once, or fails cleanly. Initialization is transactional. Services are built on a worker thread into a *local* dict and published only while the attempt still owns init: writing straight into the manager meant a cancelled attempt was followed, moments later, by that uncancellable thread publishing into a manager that had already been cleaned up. A cancelled attempt now releases whatever the thread went on to build, and a constructor that raises releases its predecessors — nothing was published, so nobody else could ever have closed them. ``cleanup()`` is idempotent and awaits an async ``close()`` on owned resources. A failed in-place reinitialization *keeps* the manager: it rolled itself back to uninitialized, but still owns the session service its own ``reinitialize(preserve_sessions=True)`` restored, and releasing it threw away the conversation for a failure the user could correct and retry. The HITL input callback becomes a per-manager ContextVar for the same reason: it is one manager with possibly two consumers, and a plain attribute let the second one capture a running turn's prompt and let either one unregister the other's callback. ``WorkflowController`` gains a derived ``WorkflowState`` that can never drift, serializes every lifecycle transition on its own lock, and never publishes after ``close()``. Construction in the init executor is shielded and tracked by a single-shot claim: cancelling the await used to cancel the asyncio future, after which asyncio silently discarded the manager the (uncancellable) thread returned. Shutdown runs in a task the controller owns and callers join under a shield, so a cancelled caller cannot abandon a teardown half-done — including the cleanup of an abandoned construction, which a later ``close()`` joins. ``_init_error`` is cleared on every success, so state, ``ensure_initialized()`` and the status bar can no longer disagree about a recovered controller. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
ADK appends the turn's input — the user message, or a resumed ``FunctionResponse`` — to the session while setting up the invocation, before the first event. There is therefore no point at which re-running a turn is side-effect free, and the harness's "Retry in Ns?" dialog was replaying turns whose input had already been persisted. The event source is now invoked exactly once and a surfaced rate limit fails the turn explaining that; transient retries belong to the provider client (ADK ``HttpRetryOptions``, Anthropic ``retry_max_attempts``). ``MessageProcessor.process()`` returns a typed ``TurnResult`` (COMPLETED/CANCELLED/FAILED/UNAVAILABLE) instead of a bare bool, so a caller can tell "the user cancelled" from "the turn failed". ``EventType.ERROR`` had no handler at all and was silently swallowed; it is now rendered as it arrives, with ``recoverable=True`` a warning that leaves the outcome to the stream and anything else failing the turn. Cancelling the caller used to leave the child consumer task driving the workflow while the turn was torn down around it; the consumer is now cancelled and awaited *before* the input callback is cleared, so no tool is left asking a question nobody owns. The HITL dialog's ``finally`` reopened a replacement events box even while unwinding, stranding a thinking box on screen — the box now reopens only on success and is finished exactly once. Session-fact extraction moves inside the ``background_init`` context: it needs the live session store and an LLM call, and leaving the context closes the manager first. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
The jobs directory is user-scoped, so two CLIs routinely hold the same records — and every mutator acted on its own in-memory snapshot. Delivery has an explicit lifecycle: ``JobRecord.resumed`` (a bool set *before* the turn ran) becomes ``resume_state`` (pending → resuming → delivered|failed) with ``resume_error`` and ``resume_owner``, claimed with ``begin_resume()`` and closed with ``complete_resume()`` on success, failure *and* cancellation. Both transitions happen under a cross-process ``flock`` against the record on disk, so two CLIs can no longer deliver one result into two conversations. A record found ``resuming`` at startup is recovered as failed rather than replayed — the interrupted turn may already have run tools. Execution is claimed the same way: ``exec_owner`` (``<host>:<pid>:<manager>``) is written *before* the backend is started, so a queued job cannot be launched twice, and an interrupted launch is failed rather than replayed. Every other metadata write reloads the durable record first — a plain state write carried this manager's stale resume fields and erased another process's live claim — and terminal transitions are monotonic, so a stale snapshot cannot rewrite a recorded success as CANCELLED. Reading distinguishes *deleted* from *unreadable*: a record another manager cleaned away is forgotten rather than resurrected, while an unparseable one is left untouched and fails closed. Startup recovery honours the same distinction rather than writing a verdict decided on state it failed to read. A foreign job is polled (backends publish outcomes durably) but its ``UNKNOWN`` — "I hold no handle for this" — is ignored, so an observer sees a job finish without marking a healthy one terminal. Whether a foreign job can be *cancelled* is now a declared backend capability rather than inferred from restart-safety, which answers a different question. ``InProcessBackend.close()`` no longer cancels submitted work. A queued job is already durably RUNNING under a live owner, and a cancelled future writes no exit-code sentinel — so no manager could ever resolve it, and it stayed RUNNING forever. If the cross-process lock cannot be taken, claims, launches, recovery, reconcile, clean and cancel all fail closed rather than writing unsynchronized. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
…upplied ``skill_scripts_enabled`` advertised ADK's ``run_skill_script`` to the model while no supported manager path wires a code executor, so every call it provoked answered ``NO_CODE_EXECUTOR``. A switch that cannot make the thing work is worse than no switch: the setting is removed and the tool is exposed exactly when ``make_skill_toolset`` is given an executor — the thing that actually makes it work. In practice scripts stay off; the parameter is there for a caller that owns one. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
CHANGELOG/README/CLAUDE.md for the preceding ten commits, plus the top-level exports the new contracts are addressed by — ``SessionRef``, ``TurnResult``, ``TurnStatus``, ``WorkflowState``, ``AgentGraphError`` — and a test that pins the import surface so a package reshuffle cannot quietly drop one. Claude-Session: https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh
This was referenced Aug 3, 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
1 participant
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.
Eight rounds of independent external review, implemented and split into 11 dependency-ordered commits. Every commit was verified on its own —
compileall, that commit's focused suites, andgit diff --check— in an isolated worktree, so the series is bisectable rather than just green at the tip.Architecture
Tool identity is unforgeable and registry-owned. Permission gating and tool assembly resolved tools by
__name__, so an unregistered callable named after a genuine tool inherited its capabilities — includingEXEMPT— and was handed its services and its long-running contract. Identity is now a per-ToolRegistryid(obj) → (weakref, definition)map confirmed withis: no name path, no equality, and a recycled address inherits nothing. Everything the framework issues is bound at its construction site; anything unbound is denied and passed through assembly untouched. Keeping the map on the instance is also what lets a short-lived registry and its closures be collected.One name, one tool — sharing is declared.
register()raises on any duplicate name.declare_tool(name, ...)states a contract with no backend-neutral implementation and each backend registers its own withregister_tool(..., variant_of=name). The ADK and LangGraphsave_plan/get_plan/save_tasks/get_taskspreviously contested the name and import order decided the winner — a bare"save_plan"in anAgentConfigcould hand an ADK agent a LangGraph tool.Tools declare their own services.
@register_tool(..., requires="kb_manager")replaces the central_TOOL_SERVICE_MAP, so a downstream tool can request any framework-provided service without editing the framework.Turn and lifecycle concurrency. A manager runs one turn at a time; lock order is lifecycle → turn, and admission re-checks backend readiness while holding the turn lock — a turn initializes before taking it, so a queued cleanup could otherwise land in between and admit the turn to a released backend. Initialization is transactional: services are built on a worker thread into a local dict and published only while the attempt still owns init.
Cross-process job coherence. The jobs directory is user-scoped, so two CLIs routinely hold the same records. Execution and delivery are both claimed under a cross-process
flockagainst the record on disk, terminal transitions are monotonic, and reads distinguish deleted (forget it) from unreadable (leave it alone, fail closed).No harness-level turn replay. ADK persists a turn's input during invocation setup, so there is no point at which re-running a turn is side-effect free. The "Retry in Ns?" dialog is gone; retries belong to the provider client.
Breaking / API changes
ToolRegistry.register()raises on duplicate namesdeclare_tool+variant_of; previously the last import silently wonToolRegistryuser_idsession_exists/list_sessions/delete_session/recent_messages/load_session/save_sessiongain an optional keyword;on_session_end(session=SessionRef(...))supports_sessionsFalse and base hooks raiseNotImplementedErrorinstead of answeringFalse/[]MessageProcessor.process()TurnResult(COMPLETED/CANCELLED/FAILED/UNAVAILABLE), notboolAgentConfiggraphssub_agents, cycles, shared children and more than one root now raiseAgentGraphErrorsettingsarg; other signatures,async def, and non-string returns are rejectedskill_scripts_enabledremovedrun_skill_scriptis exposed exactly whenmake_skill_toolsetis given a code executormake_skill_toolset(scripts_enabled=...)removedcode_executor=insteadBaseWorkflowManager._TOOL_SERVICE_MAPremoved@register_tool(..., requires=...)JobRecord.resumedresume_state(pending → resuming → delivered|failed)JobBackend.cancels_foreign_jobssurvives_restart_TOOL_SERVICE_MAP,lookup_definition(),_bind_service_tool_identitiesNew top-level exports:
SessionRef,TurnResult,TurnStatus,WorkflowState,AgentGraphError;declare_toolfromagentic_cli.tools;ResumeState/ResumeStateErrorfromagentic_cli.tools.jobs.Migration notes
declare_tool("name", description=..., capabilities=...)once in a shared module, then@register_tool(..., variant_of="name")in each backend. A bare-name reference to a declared-only tool raises "ambiguous" rather than guessing.requires=on their@register_tool; keys are validated at registration againstservice_registry.KNOWN_SERVICE_KEYS.user_kb_manageris not declarable —kb_managercreates both scopes.BaseWorkflowManagersubclasses that override session hooks keep working: base-class helpers still call them withoutuser_idwhen it is the default.skill_scripts_enabledin a settings file is now an unknown key. It never worked (no supported manager path wires an executor, so every call answeredNO_CODE_EXECUTOR); remove it.MessageProcessor.process()that treated the return as a bool should switch onTurnResult.status—CANCELLEDandFAILEDwere previously indistinguishable.BaseSettings(google_api_key=...)now actually binds. It silently bound nothing before, so anything that worked around that (setting the env var manually) can be simplified — and a misspelled credential kwarg is now an error rather than being dropped.Verification
src/agentic_cli/workflow/langgraph/has 0 changed files. The only LangGraph-adjacent change is avariant_of=kwarg on four@register_tooldecorators intools/langgraph/state_tools.py— tool registration metadata, not orchestration.Supersedes
Closes #107 and #108 — both are strict subsets of this branch (controller lifecycle, and ADK session
user_id).Known limitations
UNKNOWNpoll answer is filtered, so a backend returning a wrong non-UNKNOWNstate is still trusted.InProcessBackend.close()lets queued work finish, so a long backlog delays interpreter exit rather than being silently discarded."save_plan"before importing any backend yields "unknown tool" rather than "ambiguous".https://claude.ai/code/session_01BpktWb9vKN5exdT9MbLQYh