fix(core): reconcile duplicate AgentAdapter protocols - #430
Conversation
…location PR #408 already introduced AgentAdapter at codeframe/core/adapters/agent_adapter.py with run()-based interface used by 14 modules. PR #409 created a duplicate at codeframe/core/agent_adapter.py with execute()-based interface used by nothing. This reconciles them: - Merge new types (AgentContext, AdapterTokenUsage, AgentResultStatus) into canonical - Add token_usage/duration_ms to AgentResult, message/timestamp to AgentEvent - Remove duplicate codeframe/core/agent_adapter.py - Update tests to import from canonical location - Export new types from codeframe.core.adapters.__init__
WalkthroughThis PR relocates agent adapter type definitions from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Review: fix(core): reconcile duplicate AgentAdapter protocols This is a clean and necessary fix for the conflict from the merge ordering of #408 and #409. The approach is correct — preserve the canonical run()-based interface and fold in the new types. Overall looks good with a few things worth noting. What works well
Issue: AgentResultStatus enum is stranded (moderate) AgentResultStatus is added and exported, but AgentResult.status is still typed as Literal['completed', 'failed', 'blocked']. Two problems:
Options to fix:
Issue: Test file split is undocumented tests/core/test_agent_adapter.py now tests AgentContext, AdapterTokenUsage, AgentResultStatus, while tests/core/adapters/test_agent_adapter.py covers AgentResult, AgentEvent, and protocol compliance. This is a reasonable split, but a brief comment at the top of the former explaining the division would prevent future confusion. Minor observations
Summary The duplicate removal and type merges are correct. The main item to address is the AgentResultStatus/Literal inconsistency — either align them or defer the enum export until it is actually wired in. Right now it is exported but effectively unusable with AgentResult.status without a type error. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/test_agent_adapter.py (1)
20-175: Cover the newcodeframe.core.adaptersre-exports too.These tests only import from
codeframe.core.adapters.agent_adapter, so they never exercise the package surface added incodeframe/core/adapters/__init__.py. A smallfrom codeframe.core.adapters import AdapterTokenUsage, AgentContext, AgentResultStatussmoke test would catch broken re-exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_agent_adapter.py` around lines 20 - 175, Add a tiny smoke test that imports the re-exported symbols from the package-level module to exercise codeframe.core.adapters; specifically add a test (e.g., in the existing TestAgentAdapter suite) that does "from codeframe.core.adapters import AdapterTokenUsage, AgentContext, AgentResultStatus" and then asserts simple properties (e.g., instantiate AdapterTokenUsage and AgentContext and check AgentResultStatus.COMPLETED.value) so the package-level re-exports in __init__.py are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/core/adapters/agent_adapter.py`:
- Around line 17-23: AgentResult currently can't represent the new timeout
terminal state: update the AgentResult.status field to include the "timeout"
value by typing it to the AgentResultStatus enum (replace the current union of
string literals `"completed" | "failed" | "blocked"` with `AgentResultStatus`)
so the added AgentResultStatus.TIMEOUT is accepted and the enum remains
canonical; adjust any related type annotations or validations in the AgentResult
class/constructor (references: AgentResultStatus, AgentResult.status) to use the
enum rather than a restricted string union.
---
Nitpick comments:
In `@tests/core/test_agent_adapter.py`:
- Around line 20-175: Add a tiny smoke test that imports the re-exported symbols
from the package-level module to exercise codeframe.core.adapters; specifically
add a test (e.g., in the existing TestAgentAdapter suite) that does "from
codeframe.core.adapters import AdapterTokenUsage, AgentContext,
AgentResultStatus" and then asserts simple properties (e.g., instantiate
AdapterTokenUsage and AgentContext and check AgentResultStatus.COMPLETED.value)
so the package-level re-exports in __init__.py are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 101d12a5-32eb-4e14-b8d4-b12691ba772e
📒 Files selected for processing (4)
codeframe/core/adapters/__init__.pycodeframe/core/adapters/agent_adapter.pycodeframe/core/agent_adapter.pytests/core/test_agent_adapter.py
💤 Files with no reviewable changes (1)
- codeframe/core/agent_adapter.py
| class AgentResultStatus(str, Enum): | ||
| """Terminal status from an agent execution.""" | ||
|
|
||
| COMPLETED = "completed" | ||
| FAILED = "failed" | ||
| BLOCKED = "blocked" | ||
| TIMEOUT = "timeout" |
There was a problem hiding this comment.
AgentResult still cannot represent the new timeout terminal state.
Line 23 adds AgentResultStatus.TIMEOUT, but Line 63 still constrains AgentResult.status to "completed", "failed", and "blocked". A timed-out adapter result now has to either violate the declared contract or be downgraded to another state. Please widen the field to include "timeout"; if the enum is meant to be canonical, typing the field from AgentResultStatus would keep these definitions from drifting again.
🔧 Minimal fix
`@dataclass`
class AgentResult:
"""Result from an agent adapter execution."""
- status: Literal["completed", "failed", "blocked"]
+ status: Literal["completed", "failed", "blocked", "timeout"]Also applies to: 59-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/adapters/agent_adapter.py` around lines 17 - 23, AgentResult
currently can't represent the new timeout terminal state: update the
AgentResult.status field to include the "timeout" value by typing it to the
AgentResultStatus enum (replace the current union of string literals
`"completed" | "failed" | "blocked"` with `AgentResultStatus`) so the added
AgentResultStatus.TIMEOUT is accepted and the enum remains canonical; adjust any
related type annotations or validations in the AgentResult class/constructor
(references: AgentResultStatus, AgentResult.status) to use the enum rather than
a restricted string union.
1 participant
Summary
Fixes a conflict introduced when #409 was merged — PR #408 had already introduced
AgentAdapteratcodeframe/core/adapters/agent_adapter.py(used by 14 modules), but #409 created a duplicate atcodeframe/core/agent_adapter.pywith an incompatible interface.This PR reconciles them by:
AgentContext,AdapterTokenUsage,AgentResultStatus) into the canonicalcodeframe/core/adapters/agent_adapter.pyAgentResultgainstoken_usage/duration_ms,AgentEventgainsmessage/timestampcodeframe/core/agent_adapter.pycodeframe.core.adapters.__init__Why this happened
Issue #409 was written before PR #408 was merged. Both defined
AgentAdapterbut with different interfaces (execute()vsrun()). The canonicalrun()interface is already wired into runtime, registry, and all adapter implementations.Test Plan
Closes #409
Summary by CodeRabbit