Centralize agent logging behind scoped callbacks and remove artifact_root tramp data#23935
Centralize agent logging behind scoped callbacks and remove artifact_root tramp data#23935luisorofino wants to merge 1 commit into
Conversation
|
14218e4 to
480793d
Compare
Validation ReportAll 21 validations passed. Show details
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 480793d456
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| """Run a single agent turn with no tools available, firing scoped callbacks. | ||
| The caller is responsible for token accounting.""" | ||
| await self._callbacks.fire_before_agent_send(self._scope, prompt, 1) | ||
| response = await self._agent.send(prompt, allowed_tools=[]) |
There was a problem hiding this comment.
Report run_once send failures to scoped callbacks
When the single-turn path is used for the phase memory step, an agent.send() failure here propagates without firing fire_agent_error, unlike start(). In that scenario the run-wide AgentLogger never writes the error/failed-finish records for the phase, so the centralized agent log shows the last successful ReAct loop but omits the failing memory LLM call that aborted the phase; wrap this send in the same BaseException notification pattern used by start().
Useful? React with 👍 / 👎.
What does this PR do?
Reworks how the
ddev/aiagent framework does run logging, replacing path-threading with a single, identity-aware, callback-driven logger. No user-facing behavior changes — this is an internal architecture refactor of the agent runtime.Highlights:
AgentScope(agent/scope.py) — a small(owner_id, AgentRole)value carrying explicit agent identity, whereAgentRoleis one ofPHASE,SUBAGENT, orGOAL_REVIEWER. This identity now travels with each event instead of being reconstructed from a path.callbacks/callbacks.py) — agent-tier callbacks now carry anAgentScope. Addedon_agent_start, renamedon_complete→on_agent_finishandon_error→on_agent_error, and addedCallbacks.with_set(...)for composition.AgentLogger(runtime/agent_log.py) — one long-lived observer, created once by the orchestrator, that demultiplexes the callback stream into one append-only JSONL file per agent keyed byAgentScope. It is the only thing that knows the log root.ReActProcessFactory(react/factory.py) — binds the runtime builder and the run-wideCallbacksonce and exposescreate(scope, agent_config, system_prompt). It injects itself into the runtime it builds so a nestedspawn_subagentcan create properly-scoped children. Replaces the looseruntime_builder+callbacksprimitives that callers previously had to re-assemble.ReActProcess(react/process.py) — now requires a scope, fireson_agent_start, stamps every callback with its scope, and gainedrun_once(...)for single-turn calls so phases no longer hand-fire callbacks for memory steps.spawn_subagent— no more manual path/log orchestration; it just derives a childAgentScopeand asks the injected factory for a scoped child process.artifact_rooteverywhere and removed the old per-agenttools/agents/agent_logger.py.Motivation
The previous design had a few related smells:
artifact_roottramp data. A filesystem path was threaded through the orchestrator → resources → phases → agent build → tool registry →spawn_subagent, purely so each layer could reconstruct a per-agent logging directory. Most layers didn't use the path; they just passed it deeper. Classic tramp data.spawn_subagentorchestrated directories and log lines, and phases manually logged memory steps.runtime_builderandcallbacksas separate primitives and had to re-derive how to construct child processes, duplicating setup.This PR pushes identity into the event stream (
AgentScope) and centralizes logging behind callbacks, so the path lives in exactly one place (the orchestrator) and everything downstream is identity-aware and path-free.Notes / follow-ups
The code is intentionally not perfect yet — this is a step, not the destination.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged