♻️ refactor: move agent engine to pkg/agent with Runner abstraction + cleanup#81
Merged
♻️ refactor: move agent engine to pkg/agent with Runner abstraction + cleanup#81
Conversation
Move internal/agent/engine/ to pkg/agent/ to eliminate internal package dependency from plugins. Introduce Runner type that encapsulates engine configuration (model, tools, system prompt, hooks) and provides Run/Continue methods, replacing the repeated Engine + LoopConfig boilerplate across GoRunner, Reviewer, and the Agent tool. Add ToolSetFromRegistry, ToolSetFromRegistryFiltered, and WrapTool helpers to eliminate duplicated closure-wrapping code in all three consumers.
Split the monolithic NewGoRunner() into focused helpers: - buildProviderRegistry: provider setup - buildToolRegistry: core + extra tool registration - buildAgentPresets: builtin skill extraction + preset loading - buildHookSet: hook construction NewGoRunner now reads as a clean orchestration sequence.
Replace manual ToolSet/defs construction with tools.Registry + agent.ToolSetFromRegistry. Remove the redundant reviewTool interface (identical to tools.Tool). ReviewerConfig now accepts tools.Tool directly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8398d67bc5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return ai.TextContent{Text: result}, nil | ||
| } | ||
| runner, _ := agent.NewRunner(agent.RunnerConfig{ |
There was a problem hiding this comment.
NewReviewer currently discards the error from agent.NewRunner, so invalid config (for example Providers == nil) leaves r.runner as nil; the subsequent r.runner.Run(...) in Review will panic instead of returning a normal review error. Before this refactor, the same misconfiguration path returned an error from engine.Run, so this introduces a crash-on-misconfig regression.
Useful? React with 👍 / 👎.
These are internal implementation details of the agent loop, not part of the public Runner API. Rename to executeToolCalls/toolCallbacks to reduce the exported API surface.
Extract the 510-line Pool god object into 4 files: - pool.go (~90 lines): struct, constructor, admin methods (SetFactory, Close) - pool_session.go (~170 lines): session lifecycle (Create, Resolve, Rotate, Archive, Get, List, History) - pool_chat.go (~155 lines): Chat streaming orchestration + streamEvents helper - pool_runner.go (~90 lines): getOrCreateRunner (runner lifecycle, liveness, model switching) No type or API changes — pure file split. All 77 existing tests pass.
Separate preset.go (250 lines) into: - preset.go (~65 lines): AgentPreset type, agentFrontmatter, PresetRegistry - preset_loader.go (~175 lines): filesystem discovery, YAML parsing, file loading Add preset_loader_test.go with 18 tests covering: - YAML frontmatter parsing (basic, tools, timeout, CRLF, errors) - File loading (valid, name fallback, missing description, invalid timeout) - Directory scanning (filters non-md, hidden, subdirs) - Multi-directory deduplication with priority ordering - PresetRegistry lookup/names/all
vaayne
changed the title
- WrapTool: propagate tool errors instead of swallowing them as text - NewRunner: defensively clone Tools map and ToolDefs slice for concurrency safety - NewReviewer: return (*Reviewer, error) to surface construction failures - getOrCreateRunner: re-check under lock before assigning runner to prevent duplicate runner creation race - Rename RunnerConfig.ToolDefs → ToolDefinitions for consistency - Remove unused AnnaHome field from LoadAgentPresetsConfig - Add preset loader tests for all 4 precedence tiers and path dedup
- Fix double user message: move mem.Ingest after mem.Assemble in pool_chat.go so GoRunner.Chat is the only place the user message is appended to the transcript - Fix silent error swallowing: streamAssistant now returns an error when StopReason=Error, so provider errors propagate to the user instead of showing "(empty response)" - Fix spurious stream errors: all three provider consumers (anthropic, openai, openai-response) now track terminal events and ignore benign sdkStream.Err() after successful completion - Merge consecutive same-role messages in Anthropic convertMessages to satisfy the alternating-role API requirement - Add trace hook plugin for LLM call and tool execution observability - Add Hooks section to admin plugins page - Populate HookMeta with session/agent/user context from GoRunner.Chat
vaayne
added a commit
that referenced
this pull request
Apr 17, 2026
… cleanup (#81) * ♻️ refactor: move agent engine to pkg/agent with Runner abstraction Move internal/agent/engine/ to pkg/agent/ to eliminate internal package dependency from plugins. Introduce Runner type that encapsulates engine configuration (model, tools, system prompt, hooks) and provides Run/Continue methods, replacing the repeated Engine + LoopConfig boilerplate across GoRunner, Reviewer, and the Agent tool. Add ToolSetFromRegistry, ToolSetFromRegistryFiltered, and WrapTool helpers to eliminate duplicated closure-wrapping code in all three consumers. * ♻️ refactor: extract helper functions from GoRunner constructor Split the monolithic NewGoRunner() into focused helpers: - buildProviderRegistry: provider setup - buildToolRegistry: core + extra tool registration - buildAgentPresets: builtin skill extraction + preset loading - buildHookSet: hook construction NewGoRunner now reads as a clean orchestration sequence. * ♻️ refactor: simplify Reviewer to use tools.Registry Replace manual ToolSet/defs construction with tools.Registry + agent.ToolSetFromRegistry. Remove the redundant reviewTool interface (identical to tools.Tool). ReviewerConfig now accepts tools.Tool directly. * ♻️ refactor: unexport ExecuteToolCalls and ToolCallbacks in pkg/agent These are internal implementation details of the agent loop, not part of the public Runner API. Rename to executeToolCalls/toolCallbacks to reduce the exported API surface. * ♻️ refactor: split pool.go into focused files by responsibility Extract the 510-line Pool god object into 4 files: - pool.go (~90 lines): struct, constructor, admin methods (SetFactory, Close) - pool_session.go (~170 lines): session lifecycle (Create, Resolve, Rotate, Archive, Get, List, History) - pool_chat.go (~155 lines): Chat streaming orchestration + streamEvents helper - pool_runner.go (~90 lines): getOrCreateRunner (runner lifecycle, liveness, model switching) No type or API changes — pure file split. All 77 existing tests pass. * ♻️ refactor: split preset.go into types and loader with tests Separate preset.go (250 lines) into: - preset.go (~65 lines): AgentPreset type, agentFrontmatter, PresetRegistry - preset_loader.go (~175 lines): filesystem discovery, YAML parsing, file loading Add preset_loader_test.go with 18 tests covering: - YAML frontmatter parsing (basic, tools, timeout, CRLF, errors) - File loading (valid, name fallback, missing description, invalid timeout) - Directory scanning (filters non-md, hidden, subdirs) - Multi-directory deduplication with priority ordering - PresetRegistry lookup/names/all * 🐛 fix: address review findings from Codex code review - WrapTool: propagate tool errors instead of swallowing them as text - NewRunner: defensively clone Tools map and ToolDefs slice for concurrency safety - NewReviewer: return (*Reviewer, error) to surface construction failures - getOrCreateRunner: re-check under lock before assigning runner to prevent duplicate runner creation race - Rename RunnerConfig.ToolDefs → ToolDefinitions for consistency - Remove unused AnnaHome field from LoadAgentPresetsConfig - Add preset loader tests for all 4 precedence tiers and path dedup * 🐛 fix: resolve empty telegram responses and add trace hook - Fix double user message: move mem.Ingest after mem.Assemble in pool_chat.go so GoRunner.Chat is the only place the user message is appended to the transcript - Fix silent error swallowing: streamAssistant now returns an error when StopReason=Error, so provider errors propagate to the user instead of showing "(empty response)" - Fix spurious stream errors: all three provider consumers (anthropic, openai, openai-response) now track terminal events and ignore benign sdkStream.Err() after successful completion - Merge consecutive same-role messages in Anthropic convertMessages to satisfy the alternating-role API requirement - Add trace hook plugin for LLM call and tool execution observability - Add Hooks section to admin plugins page - Populate HookMeta with session/agent/user context from GoRunner.Chat
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.
Summary
internal/agent/engine/→pkg/agent/so plugins no longer needinternal/importsRunnertype that encapsulates model, tools, system prompt, hooks — replaces repeatedEngine+LoopConfigboilerplateToolSetFromRegistry,ToolSetFromRegistryFiltered,WrapToolhelpers to eliminate duplicated closure-wrapping across consumersbuildProviderRegistry,buildToolRegistry,buildAgentPresets,buildHookSet)tools.Registry+ToolSetFromRegistryinstead of manual ToolSet, remove redundantreviewToolinterfaceExecuteToolCalls/ToolCallbacks— internal implementation details, not public APIpool.go(510 lines) into 4 focused files:pool.go,pool_session.go,pool_chat.go,pool_runner.gopreset.go(250 lines) intopreset.go(types/registry) andpreset_loader.go(filesystem/parsing) with 18 new testsTest plan
go build ./...passesmise run format— cleanmise run lint— 0 issuesmise run test— all tests pass with-raceinternal/agent/engineimports