feat: Agent Adapter Architecture — delegate to frontier coding agents (#408) - #428
Conversation
Wraps ContextLoader to produce agent-ready prompts with gate requirements and execution instructions for the adapter architecture.
Provides shared subprocess infrastructure for all external coding agent adapters (Claude Code, OpenCode, Codex): binary detection, stdout streaming, exit code mapping, and blocker detection via classify_error_for_blocker.
…ing (#415) Wraps any AgentAdapter with verification gates (pytest, ruff, etc.) and a self-correction loop. After the inner adapter completes, gates run automatically. On failure, the adapter is re-invoked with error context for up to max_correction_rounds (default 3). This decouples the self-correction pattern from ReactAgent so it works with any execution engine.
Wrap existing ReactAgent and Agent classes behind the AgentAdapter protocol so they can be used through the unified engine registry without modifying the original classes.
Implement ClaudeCodeAdapter extending SubprocessAdapter to delegate task execution to the `claude` CLI with --print for non-interactive output. Supports optional --allowedTools for permission control. Includes 12 unit tests covering protocol conformance, command building, stdin piping, and execution success/failure paths.
…#414) Centralized registry that maps engine names to adapter factories, resolves engine selection (CLI flag > env var > default), and handles the split between builtin engines (need workspace + LLM provider) and external subprocess engines (just need binary on PATH).
- Add external engine code path in execute_agent() using adapter pipeline (TaskContextPackager → get_external_adapter → VerificationWrapper) - Skip ANTHROPIC_API_KEY check for external engines (they manage own auth) - Expand --engine flag to accept: react, plan, claude-code, opencode, built-in - Conditional API key validation in CLI work_start and batch_run commands
WalkthroughAdds an AgentAdapter abstraction and engine registry, external subprocess adapters (Claude Code, OpenCode), builtin adapter wrappers, a context packager, a verification wrapper with self-correction, CLI/runtime wiring for engine selection and conditional Anthropic auth, plus comprehensive adapter/registry/runtime tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI<br/>(work_start)
participant Runtime as Runtime<br/>(execute_agent)
participant Registry as Engine<br/>Registry
participant Packager as Context<br/>Packager
participant Adapter as Agent<br/>Adapter
participant External as External<br/>Agent
participant Wrapper as Verification<br/>Wrapper
participant Gates as Gates
User->>CLI: cf work start <id> --execute --engine claude-code
CLI->>Runtime: execute_agent(engine="claude-code")
Runtime->>Registry: is_external_engine("claude-code")
Registry-->>Runtime: true
Runtime->>Runtime: skip ANTHROPIC_API_KEY check
Runtime->>Packager: build(task_id, gate_names)
Packager-->>Runtime: PackagedContext(prompt, context)
Runtime->>Registry: get_adapter("claude-code")
Registry-->>Runtime: ClaudeCodeAdapter
Runtime->>Wrapper: VerificationWrapper(adapter)
Wrapper->>Adapter: run(task_id, prompt, workspace_path)
Adapter->>External: claude-code --print (stdin=prompt)
External-->>Adapter: stdout/stderr + exit_code
Adapter-->>Wrapper: AgentResult(status, output, modified_files)
alt status == "completed"
Wrapper->>Gates: run_gates(gate_names)
Gates-->>Wrapper: gate_result
alt gates pass
Wrapper-->>Runtime: AgentResult(completed)
else gates fail
Wrapper->>Adapter: run(...with error context)
Adapter->>External: claude-code --print
External-->>Adapter: AgentResult(...)
Wrapper-->>Runtime: AgentResult(final)
end
else
Wrapper-->>Runtime: AgentResult(as-is)
end
Runtime-->>CLI: final result
sequenceDiagram
actor User
participant CLI as CLI<br/>(work_start)
participant Runtime as Runtime<br/>(execute_agent)
participant Registry as Engine<br/>Registry
participant Packager as Context<br/>Packager
participant Adapter as Agent<br/>Adapter
participant Builtin as Built-in<br/>Agent
participant Wrapper as Verification<br/>Wrapper
participant Gates as Gates
User->>CLI: cf work start <id> --execute --engine react
CLI->>Runtime: execute_agent(engine="react")
Runtime->>Registry: is_external_engine("react")
Registry-->>Runtime: false
Runtime->>Runtime: require ANTHROPIC_API_KEY & provider
Runtime->>Packager: build(task_id)
Packager-->>Runtime: PackagedContext(prompt)
Runtime->>Registry: get_adapter("react", workspace, provider)
Registry-->>Runtime: BuiltinReactAdapter
Runtime->>Wrapper: VerificationWrapper(adapter)
Wrapper->>Adapter: run(task_id, prompt, workspace_path)
Adapter->>Builtin: ReactAgent.run(...)
Builtin-->>Adapter: AgentStatus + output
Adapter-->>Wrapper: AgentResult(status, output)
Wrapper->>Gates: run_gates(gate_names)
Gates-->>Wrapper: gate_result
alt gates fail
Wrapper->>Adapter: run(...with correction)
Adapter->>Builtin: ReactAgent.run(...)
Builtin-->>Adapter: AgentResult(...)
end
Wrapper-->>Runtime: AgentResult(final)
Runtime-->>CLI: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
- Drain stderr in background thread to prevent pipe buffer deadlock - Add configurable timeout (default 30min) with process kill on expiry - Add timeout test case
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
codeframe/core/adapters/verification_wrapper.py (1)
11-11: Unused import.
Workspaceis imported but only stored asself._workspaceand passed directly torun_gates(). The type annotation in__init__relies on it, but consider whether this import is necessary or if it should be aTYPE_CHECKINGimport for cleaner dependency management.Optional: Move to TYPE_CHECKING block
-from codeframe.core.workspace import Workspace +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from codeframe.core.workspace import Workspace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/adapters/verification_wrapper.py` at line 11, The import of Workspace is unused at runtime—it's only used for the __init__ type annotation, stored as self._workspace and forwarded to run_gates—so convert it to a TYPE_CHECKING-only import to avoid unnecessary runtime dependency: add "from typing import TYPE_CHECKING" and move "from codeframe.core.workspace import Workspace" into an if TYPE_CHECKING: block (or use a string forward reference in the __init__ signature) so __init__, self._workspace and run_gates keep the type information without importing Workspace at runtime.codeframe/core/adapters/builtin.py (1)
64-66: Minor duplication of_bridge_eventhelper.Both
BuiltinReactAdapter.runandBuiltinPlanAdapter.rundefine identical_bridge_eventclosures. Consider extracting to a module-level helper or base class method if more adapters are added.Also applies to: 142-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/adapters/builtin.py` around lines 64 - 66, Both BuiltinReactAdapter.run and BuiltinPlanAdapter.run define identical _bridge_event closures; extract that closure into a single reusable helper to remove duplication. Create a module-level function (e.g., bridge_event_helper) or add a base class method (e.g., BuiltinAdapter._bridge_event) that accepts on_event, event_type, and data and invokes on_event(AgentEvent(...)) if present, then replace the inline _bridge_event definitions in BuiltinReactAdapter.run and BuiltinPlanAdapter.run with calls to the shared helper.codeframe/core/adapters/claude_code.py (1)
41-62: Redundant method overrides (same as OpenCodeAdapter).Both
build_commandandget_stdinare identical to the base class implementation. Consider removing them or adding a comment explaining their presence for documentation purposes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/adapters/claude_code.py` around lines 41 - 62, The methods build_command and get_stdin in claude_code.py duplicate the base-class behavior (same as OpenCodeAdapter); either remove these redundant overrides from the ClaudeCodeAdapter so it inherits the base implementations, or if you need them for clarity, keep them but replace the method bodies with a short doc-comment explaining why they are intentionally identical to the base and reference the base methods (e.g., note they mirror the base adapter's build_command and get_stdin behavior) to prevent linter warnings and clarify intent.codeframe/core/adapters/subprocess_adapter.py (1)
169-174: Blocker question extraction is simplistic.
_extract_blocker_questionreturns only the last non-empty line, which may not always contain the most meaningful context. Consider whether subclasses should override this for agent-specific blocker parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/adapters/subprocess_adapter.py` around lines 169 - 174, The _extract_blocker_question method currently returns only the last non-empty line; change it to a more robust, overridable default by implementing a simple heuristic (prefer the last line containing a question mark, or the last line that begins with interrogative words like "Why", "How", "What", "When", "Where", "Who"); if none match, fall back to the last non-empty line, and update the docstring to state that subclasses may override _extract_blocker_question in agent-specific adapters (e.g., in subprocess_adapter.py) to provide custom parsing for different agent outputs.codeframe/core/adapters/opencode.py (1)
27-48: Redundant method overrides could be removed.Both
build_commandandget_stdinimplement identical behavior to the base classSubprocessAdapter. If retained only for documentation purposes, consider adding a comment explaining why, or remove them to reduce maintenance overhead.Option: Remove redundant overrides
`@property` def name(self) -> str: # noqa: D102 return "opencode" - - def build_command(self, prompt: str, workspace_path: Path) -> list[str]: - """Build opencode CLI command. - - Args: - prompt: The task prompt (sent via stdin, not in the command). - workspace_path: Workspace root (cwd is set by the base class). - - Returns: - Command list for subprocess.Popen. - """ - return [self._binary_path, *self._cli_args] - - def get_stdin(self, prompt: str) -> str | None: - """Send prompt via stdin. - - Args: - prompt: The task prompt to pipe into the opencode process. - - Returns: - The prompt string. - """ - return prompt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/adapters/opencode.py` around lines 27 - 48, The overrides build_command and get_stdin in opencode.py replicate SubprocessAdapter behavior and are redundant; either delete these methods from class to inherit the base implementations, or keep them but replace their bodies with a one-line comment explaining they intentionally mirror SubprocessAdapter for clarity (mentioning the base class name SubprocessAdapter) so future maintainers know they are deliberate; update/remove build_command and get_stdin accordingly.codeframe/core/runtime.py (1)
713-735: Consider enrichingAgentStatewith error details for failed external engine runs.The external engine result is mapped to a minimal
AgentState(status=agent_status). For failed runs, this loses the error message fromresult.error, which could be useful for downstream logging and supervisor analysis (see lines 844-933 which inspectstate.blocker,state.step_results, andstate.gate_results).This doesn't break functionality since the error is logged (line 733-735) and the supervisor retry logic at lines 830+ only applies to the
"plan"engine. However, for consistency with builtin engines which populate richer state, consider capturing the error:Optional enhancement
agent_status = status_map.get(result.status, AgentStatus.FAILED) - state = AgentState(status=agent_status) + state = AgentState(status=agent_status) + # Capture error for diagnostics if the external engine failed + if result.error: + # Store error in a blocker-like structure for consistency + from codeframe.core.agent import Blocker + state.blocker = Blocker(reason=result.error, question=None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/runtime.py` around lines 713 - 735, The AgentState created for external engine results only sets status and discards result.error; update the code that builds AgentState (the AgentState(...) call after mapping result.status to agent_status) to include the engine error information when result.status == "failed" (e.g., pass result.error into an error/message field on AgentState or set state.error = result.error after construction) so downstream inspections of state (AgentState, AgentStatus, result.error) can see the external error; keep existing blocker creation (result.blocker_question + blockers.create) and logging unchanged.
🤖 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/cli/app.py`:
- Around line 2051-2057: Validate the requested engine before checking
credentials: call the engine registry validation (e.g., get_engine or
is_valid_engine from codeframe.core.engine_registry) and raise/return the proper
invalid-engine error if it doesn't exist, then only call
is_external_engine(engine) and require_anthropic_api_key() for builtin engines;
apply the same change to the other occurrence (the block around lines 2972-2977)
so an unknown engine no longer falls through and triggers an ANTHROPIC_API_KEY
error.
In `@codeframe/core/engine_registry.py`:
- Around line 160-167: The public factory get_adapter currently lets unknown
engine names fall through to the builtin branch and surface a misleading
"requires workspace and llm_provider" error; update get_adapter to first
validate the engine is known (e.g., check whether is_external_engine(engine) is
true OR engine is in the set of builtin engine names) and if not raise a clear
ValueError like "Unknown engine '<name>'"; only after this validation proceed to
call get_external_adapter or (if builtin) verify workspace/llm_provider and call
get_builtin_adapter.
In `@tests/core/adapters/test_agent_adapter.py`:
- Around line 1-5: Add the v2 pytest marker to this test module: import pytest
if missing and set a module-level marker by adding "pytestmark = pytest.mark.v2"
near the top of the file (or alternatively decorate the test functions/classes
with `@pytest.mark.v2`); ensure the symbol pytestmark is defined and pytest is
imported so the v2 marker is applied to all tests in this module.
In `@tests/core/adapters/test_builtin.py`:
- Around line 1-14: Add the required pytest v2 marker and remove the unused
import: add a module-level pytestmark = pytest.mark.v2 (or decorate the test
class/functions) in the test file to satisfy the v2 requirement, and remove the
unused AgentResult import from the imports list (keep AgentAdapter, AgentEvent
if used); ensure references to BuiltinPlanAdapter, BuiltinReactAdapter,
_REACT_AGENT_CLS and _PLAN_AGENT_CLS remain unchanged.
In `@tests/core/adapters/test_claude_code.py`:
- Around line 1-10: This test module is missing the required v2 pytest marker;
add a module-level marker by defining pytestmark = pytest.mark.v2 at the top of
the file (after imports) or apply `@pytest.mark.v2` to the test functions in this
file that exercise v2 behavior; ensure you import pytest if not already imported
and place the marker near the top so the ClaudeCodeAdapter tests run under the
v2 marker.
In `@tests/core/adapters/test_opencode.py`:
- Around line 1-10: Add the module-level pytest v2 marker by defining pytestmark
= pytest.mark.v2 at the top of the test module (the file that imports pytest and
declares tests for OpenCodeAdapter); since pytest is already imported, add the
single module-level variable assignment (pytestmark) so the tests in this module
are marked with v2 for the OpenCodeAdapter-related tests.
In `@tests/core/adapters/test_subprocess_adapter.py`:
- Around line 1-9: The test module lacks the required v2 marker; add a
module-level marker by declaring pytestmark = pytest.mark.v2 at the top of the
test file (e.g., in tests/core/adapters/test_subprocess_adapter.py) or decorate
the test functions/classes with `@pytest.mark.v2` so the
SubprocessAdapter/AgentAdapter tests are recognized as v2 tests; ensure you
import pytest if not already present.
- Around line 194-197: Add a test that exercises SubprocessAdapter.run with an
empty stdin to catch the edge case where get_stdin("") returns "" but run skips
piping stdin; specifically, in tests/core/adapters/test_subprocess_adapter.py
add a case that patches subprocess.run (and shutil.which as already done), calls
adapter.run("", ...) and asserts the subprocess.run was invoked without an
input/stdin payload (e.g., input is None or no stdin kwarg), referencing
SubprocessAdapter.run and get_stdin to ensure the adapter does not incorrectly
attempt to pipe an empty string.
In `@tests/core/adapters/test_verification_wrapper.py`:
- Around line 1-10: Add the required v2 pytest marker and remove the unused
import: add a module-level marker (e.g., add "pytestmark = pytest.mark.v2" near
the top of tests/core/adapters/test_verification_wrapper.py) so the test is
tagged as v2, and remove the unused AgentResult import from the import list
(leave VerificationWrapper, AgentAdapter, and AgentEvent imports intact) to
eliminate the unused-import pipeline failure.
In `@tests/core/test_context_packager.py`:
- Around line 1-9: Add the required module-level pytest v2 marker to this test
module so it's categorized as v2: import pytest at the top (already present) and
add a module-level assignment pytestmark = pytest.mark.v2 near the top of the
file (above or immediately below the existing imports/docstring) for the tests
that exercise TaskContextPackager / PackagedContext / TaskContext.
In `@tests/core/test_engine_registry.py`:
- Around line 1-18: This test module lacks the v2 marker; add a module-level
marker by inserting "pytestmark = pytest.mark.v2" (using the existing import
pytest) near the top of the file (e.g., after the imports) so all tests in
tests/core/test_engine_registry.py are marked v2; no other code changes required
and this will satisfy the requirement for new v2 tests such as those exercising
get_adapter, get_builtin_adapter, get_external_adapter, is_external_engine,
resolve_engine, and the BUILTIN_ENGINES/EXTERNAL_ENGINES/VALID_ENGINES
constants.
In `@tests/core/test_runtime_adapters.py`:
- Around line 3-5: Remove the unused Path import from the test module's
top-level imports: update the import statement that currently reads "from
pathlib import Path" (or the combined line including Path) so that Path is no
longer imported and only keep the used imports (e.g., "import pytest" and "from
unittest.mock import MagicMock, patch"); this will resolve the Ruff
unused-import failure for Path.
- Around line 1-7: The test module exercises the v2 engine/adaptor flow but
lacks the required v2 marker; add a module-level marker by defining pytestmark =
pytest.mark.v2 (or decorate the test functions with `@pytest.mark.v2`) near the
top of the file so the suite is recognized as v2; ensure you import pytest if
not already present and place the pytestmark assignment at module scope
(referencing pytestmark and pytest.mark.v2).
---
Nitpick comments:
In `@codeframe/core/adapters/builtin.py`:
- Around line 64-66: Both BuiltinReactAdapter.run and BuiltinPlanAdapter.run
define identical _bridge_event closures; extract that closure into a single
reusable helper to remove duplication. Create a module-level function (e.g.,
bridge_event_helper) or add a base class method (e.g.,
BuiltinAdapter._bridge_event) that accepts on_event, event_type, and data and
invokes on_event(AgentEvent(...)) if present, then replace the inline
_bridge_event definitions in BuiltinReactAdapter.run and BuiltinPlanAdapter.run
with calls to the shared helper.
In `@codeframe/core/adapters/claude_code.py`:
- Around line 41-62: The methods build_command and get_stdin in claude_code.py
duplicate the base-class behavior (same as OpenCodeAdapter); either remove these
redundant overrides from the ClaudeCodeAdapter so it inherits the base
implementations, or if you need them for clarity, keep them but replace the
method bodies with a short doc-comment explaining why they are intentionally
identical to the base and reference the base methods (e.g., note they mirror the
base adapter's build_command and get_stdin behavior) to prevent linter warnings
and clarify intent.
In `@codeframe/core/adapters/opencode.py`:
- Around line 27-48: The overrides build_command and get_stdin in opencode.py
replicate SubprocessAdapter behavior and are redundant; either delete these
methods from class to inherit the base implementations, or keep them but replace
their bodies with a one-line comment explaining they intentionally mirror
SubprocessAdapter for clarity (mentioning the base class name SubprocessAdapter)
so future maintainers know they are deliberate; update/remove build_command and
get_stdin accordingly.
In `@codeframe/core/adapters/subprocess_adapter.py`:
- Around line 169-174: The _extract_blocker_question method currently returns
only the last non-empty line; change it to a more robust, overridable default by
implementing a simple heuristic (prefer the last line containing a question
mark, or the last line that begins with interrogative words like "Why", "How",
"What", "When", "Where", "Who"); if none match, fall back to the last non-empty
line, and update the docstring to state that subclasses may override
_extract_blocker_question in agent-specific adapters (e.g., in
subprocess_adapter.py) to provide custom parsing for different agent outputs.
In `@codeframe/core/adapters/verification_wrapper.py`:
- Line 11: The import of Workspace is unused at runtime—it's only used for the
__init__ type annotation, stored as self._workspace and forwarded to
run_gates—so convert it to a TYPE_CHECKING-only import to avoid unnecessary
runtime dependency: add "from typing import TYPE_CHECKING" and move "from
codeframe.core.workspace import Workspace" into an if TYPE_CHECKING: block (or
use a string forward reference in the __init__ signature) so __init__,
self._workspace and run_gates keep the type information without importing
Workspace at runtime.
In `@codeframe/core/runtime.py`:
- Around line 713-735: The AgentState created for external engine results only
sets status and discards result.error; update the code that builds AgentState
(the AgentState(...) call after mapping result.status to agent_status) to
include the engine error information when result.status == "failed" (e.g., pass
result.error into an error/message field on AgentState or set state.error =
result.error after construction) so downstream inspections of state (AgentState,
AgentStatus, result.error) can see the external error; keep existing blocker
creation (result.blocker_question + blockers.create) and logging unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64dbe3e0-b3a9-4a8a-b7ab-e5e3157755e5
📒 Files selected for processing (21)
codeframe/cli/app.pycodeframe/core/adapters/__init__.pycodeframe/core/adapters/agent_adapter.pycodeframe/core/adapters/builtin.pycodeframe/core/adapters/claude_code.pycodeframe/core/adapters/opencode.pycodeframe/core/adapters/subprocess_adapter.pycodeframe/core/adapters/verification_wrapper.pycodeframe/core/context_packager.pycodeframe/core/engine_registry.pycodeframe/core/runtime.pytests/core/adapters/__init__.pytests/core/adapters/test_agent_adapter.pytests/core/adapters/test_builtin.pytests/core/adapters/test_claude_code.pytests/core/adapters/test_opencode.pytests/core/adapters/test_subprocess_adapter.pytests/core/adapters/test_verification_wrapper.pytests/core/test_context_packager.pytests/core/test_engine_registry.pytests/core/test_runtime_adapters.py
| # Validate API key before creating run record (avoids dangling IN_PROGRESS state) | ||
| # External engines (claude-code, opencode) manage their own authentication | ||
| if execute: | ||
| from codeframe.cli.validators import require_anthropic_api_key | ||
| require_anthropic_api_key() | ||
| from codeframe.core.engine_registry import is_external_engine | ||
| if not is_external_engine(engine): | ||
| from codeframe.cli.validators import require_anthropic_api_key | ||
| require_anthropic_api_key() |
There was a problem hiding this comment.
Validate engine before deciding which credentials to require.
An unknown engine currently falls through is_external_engine() as “builtin”, so work start --engine typo and work batch run --engine typo can fail with an ANTHROPIC_API_KEY error instead of the real invalid-engine error whenever the key is unset. Resolve or validate the engine first, then branch on external vs builtin.
Suggested fix
- if execute:
- from codeframe.core.engine_registry import is_external_engine
- if not is_external_engine(engine):
+ if execute:
+ from codeframe.core.engine_registry import is_external_engine, resolve_engine
+ engine = resolve_engine(engine)
+ if not is_external_engine(engine):
from codeframe.cli.validators import require_anthropic_api_key
require_anthropic_api_key()- from codeframe.core.engine_registry import is_external_engine
- if not is_external_engine(engine):
+ from codeframe.core.engine_registry import is_external_engine, resolve_engine
+ engine = resolve_engine(engine)
+ if not is_external_engine(engine):
from codeframe.cli.validators import require_anthropic_api_key
require_anthropic_api_key()Also applies to: 2972-2977
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/cli/app.py` around lines 2051 - 2057, Validate the requested engine
before checking credentials: call the engine registry validation (e.g.,
get_engine or is_valid_engine from codeframe.core.engine_registry) and
raise/return the proper invalid-engine error if it doesn't exist, then only call
is_external_engine(engine) and require_anthropic_api_key() for builtin engines;
apply the same change to the other occurrence (the block around lines 2972-2977)
so an unknown engine no longer falls through and triggers an ANTHROPIC_API_KEY
error.
| if is_external_engine(engine): | ||
| return get_external_adapter(engine, **kwargs) | ||
| else: | ||
| if workspace is None or llm_provider is None: | ||
| raise ValueError( | ||
| f"Builtin engine '{engine}' requires workspace and llm_provider" | ||
| ) | ||
| return get_builtin_adapter(engine, workspace, llm_provider, **kwargs) |
There was a problem hiding this comment.
Reject unknown engines before the builtin-arg check.
get_adapter("nonexistent") currently falls into the builtin branch and can raise "requires workspace and llm_provider" instead of reporting an invalid engine. That makes the public factory lie about the actual failure mode.
Suggested fix
def get_adapter(
engine: str,
workspace: Any = None,
llm_provider: Any = None,
**kwargs: Any,
) -> AgentAdapter:
@@
- if is_external_engine(engine):
- return get_external_adapter(engine, **kwargs)
- else:
- if workspace is None or llm_provider is None:
- raise ValueError(
- f"Builtin engine '{engine}' requires workspace and llm_provider"
- )
- return get_builtin_adapter(engine, workspace, llm_provider, **kwargs)
+ if engine not in VALID_ENGINES:
+ raise ValueError(
+ f"Invalid engine '{engine}'. "
+ f"Must be one of: {', '.join(sorted(VALID_ENGINES))}"
+ )
+
+ resolved = "react" if engine == "built-in" else engine
+ if is_external_engine(resolved):
+ return get_external_adapter(resolved, **kwargs)
+
+ if workspace is None or llm_provider is None:
+ raise ValueError(
+ f"Builtin engine '{engine}' requires workspace and llm_provider"
+ )
+ return get_builtin_adapter(resolved, workspace, llm_provider, **kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/engine_registry.py` around lines 160 - 167, The public factory
get_adapter currently lets unknown engine names fall through to the builtin
branch and surface a misleading "requires workspace and llm_provider" error;
update get_adapter to first validate the engine is known (e.g., check whether
is_external_engine(engine) is true OR engine is in the set of builtin engine
names) and if not raise a clear ValueError like "Unknown engine '<name>'"; only
after this validation proceed to call get_external_adapter or (if builtin)
verify workspace/llm_provider and call get_builtin_adapter.
| """Tests for agent adapter protocol and data types.""" | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent, AgentResult |
There was a problem hiding this comment.
Add the required v2 marker to this test module.
This file covers new v2 adapter behavior but is missing the required pytest.mark.v2 marker.
Suggested fix
"""Tests for agent adapter protocol and data types."""
from pathlib import Path
+import pytest
+
from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent, AgentResult
+
+pytestmark = pytest.mark.v2As per coding guidelines "tests/**/*.py: Test files must use the @pytest.mark.v2 decorator or module-level pytestmark = pytest.mark.v2 for v2 functionality tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_agent_adapter.py` around lines 1 - 5, Add the v2
pytest marker to this test module: import pytest if missing and set a
module-level marker by adding "pytestmark = pytest.mark.v2" near the top of the
file (or alternatively decorate the test functions/classes with
`@pytest.mark.v2`); ensure the symbol pytestmark is defined and pytest is imported
so the v2 marker is applied to all tests in this module.
| """Tests for builtin adapter shims.""" | ||
|
|
||
| import pytest | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent, AgentResult | ||
| from codeframe.core.adapters.builtin import BuiltinPlanAdapter, BuiltinReactAdapter | ||
| from codeframe.core.agent import AgentState, AgentStatus | ||
|
|
||
| # Patch targets at the source modules, since builtin.py uses lazy imports. | ||
| _REACT_AGENT_CLS = "codeframe.core.react_agent.ReactAgent" | ||
| _PLAN_AGENT_CLS = "codeframe.core.agent.Agent" | ||
|
|
There was a problem hiding this comment.
Missing required @pytest.mark.v2 marker and unused import.
Two issues:
- Per coding guidelines, new v2 Python tests must include the v2 marker.
- Pipeline failure indicates
AgentResultis imported but not used in the test file.
Proposed fix
"""Tests for builtin adapter shims."""
import pytest
from pathlib import Path
from unittest.mock import MagicMock, patch
-from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent, AgentResult
+from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent
from codeframe.core.adapters.builtin import BuiltinPlanAdapter, BuiltinReactAdapter
from codeframe.core.agent import AgentState, AgentStatus
+pytestmark = pytest.mark.v2
+
# Patch targets at the source modules, since builtin.py uses lazy imports.
_REACT_AGENT_CLS = "codeframe.core.react_agent.ReactAgent"
_PLAN_AGENT_CLS = "codeframe.core.agent.Agent"As per coding guidelines: tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2
🧰 Tools
🪛 GitHub Actions: Test Suite (Unit + E2E)
[error] 7-7: Ruff check reported an unused import: 'codeframe.core.adapters.agent_adapter.AgentResult'. Remove unused import or use --fix to auto-fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_builtin.py` around lines 1 - 14, Add the required
pytest v2 marker and remove the unused import: add a module-level pytestmark =
pytest.mark.v2 (or decorate the test class/functions) in the test file to
satisfy the v2 requirement, and remove the unused AgentResult import from the
imports list (keep AgentAdapter, AgentEvent if used); ensure references to
BuiltinPlanAdapter, BuiltinReactAdapter, _REACT_AGENT_CLS and _PLAN_AGENT_CLS
remain unchanged.
| """Tests for Claude Code adapter.""" | ||
|
|
||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| from codeframe.core.adapters.agent_adapter import AgentAdapter | ||
| from codeframe.core.adapters.claude_code import ClaudeCodeAdapter | ||
|
|
There was a problem hiding this comment.
Missing required @pytest.mark.v2 marker for v2 functionality tests.
Per coding guidelines, new v2 Python tests must be marked with @pytest.mark.v2 decorator or module-level pytestmark = pytest.mark.v2. This file tests new v2 adapter functionality but lacks the required marker.
Proposed fix
"""Tests for Claude Code adapter."""
from pathlib import Path
from unittest.mock import MagicMock, patch
import pytest
from codeframe.core.adapters.agent_adapter import AgentAdapter
from codeframe.core.adapters.claude_code import ClaudeCodeAdapter
+pytestmark = pytest.mark.v2
+
class TestClaudeCodeAdapter:As per coding guidelines: tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_claude_code.py` around lines 1 - 10, This test
module is missing the required v2 pytest marker; add a module-level marker by
defining pytestmark = pytest.mark.v2 at the top of the file (after imports) or
apply `@pytest.mark.v2` to the test functions in this file that exercise v2
behavior; ensure you import pytest if not already imported and place the marker
near the top so the ClaudeCodeAdapter tests run under the v2 marker.
| """Tests for VerificationWrapper.""" | ||
|
|
||
| import pytest | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from codeframe.core.adapters.verification_wrapper import VerificationWrapper | ||
| from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent, AgentResult | ||
| from codeframe.core.gates import GateStatus | ||
|
|
There was a problem hiding this comment.
Missing required @pytest.mark.v2 marker and unused import.
Two issues:
- Per coding guidelines, new v2 Python tests must include the v2 marker.
- Pipeline failure indicates
AgentResultis imported but unused.
Proposed fix
"""Tests for VerificationWrapper."""
import pytest
from pathlib import Path
from unittest.mock import MagicMock, patch
from codeframe.core.adapters.verification_wrapper import VerificationWrapper
-from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent, AgentResult
+from codeframe.core.adapters.agent_adapter import AgentAdapter, AgentEvent
from codeframe.core.gates import GateStatus
+pytestmark = pytest.mark.v2
+As per coding guidelines: tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_verification_wrapper.py` around lines 1 - 10, Add
the required v2 pytest marker and remove the unused import: add a module-level
marker (e.g., add "pytestmark = pytest.mark.v2" near the top of
tests/core/adapters/test_verification_wrapper.py) so the test is tagged as v2,
and remove the unused AgentResult import from the import list (leave
VerificationWrapper, AgentAdapter, and AgentEvent imports intact) to eliminate
the unused-import pipeline failure.
| """Tests for TaskContextPackager.""" | ||
|
|
||
| import pytest | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from codeframe.core.context_packager import TaskContextPackager, PackagedContext | ||
| from codeframe.core.context import TaskContext | ||
|
|
There was a problem hiding this comment.
Missing required @pytest.mark.v2 marker.
Per coding guidelines, test files for v2 functionality must include the v2 marker. Add a module-level marker to ensure these tests are properly categorized.
Proposed fix
"""Tests for TaskContextPackager."""
import pytest
from pathlib import Path
from unittest.mock import MagicMock, patch
+pytestmark = pytest.mark.v2
+
from codeframe.core.context_packager import TaskContextPackager, PackagedContext
from codeframe.core.context import TaskContextAs per coding guidelines: "New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Tests for TaskContextPackager.""" | |
| import pytest | |
| from pathlib import Path | |
| from unittest.mock import MagicMock, patch | |
| from codeframe.core.context_packager import TaskContextPackager, PackagedContext | |
| from codeframe.core.context import TaskContext | |
| """Tests for TaskContextPackager.""" | |
| import pytest | |
| from pathlib import Path | |
| from unittest.mock import MagicMock, patch | |
| pytestmark = pytest.mark.v2 | |
| from codeframe.core.context_packager import TaskContextPackager, PackagedContext | |
| from codeframe.core.context import TaskContext |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_context_packager.py` around lines 1 - 9, Add the required
module-level pytest v2 marker to this test module so it's categorized as v2:
import pytest at the top (already present) and add a module-level assignment
pytestmark = pytest.mark.v2 near the top of the file (above or immediately below
the existing imports/docstring) for the tests that exercise TaskContextPackager
/ PackagedContext / TaskContext.
| """Tests for engine registry.""" | ||
|
|
||
| import os | ||
|
|
||
| import pytest | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from codeframe.core.adapters.agent_adapter import AgentAdapter | ||
| from codeframe.core.engine_registry import ( | ||
| BUILTIN_ENGINES, | ||
| EXTERNAL_ENGINES, | ||
| VALID_ENGINES, | ||
| get_adapter, | ||
| get_builtin_adapter, | ||
| get_external_adapter, | ||
| is_external_engine, | ||
| resolve_engine, | ||
| ) |
There was a problem hiding this comment.
Mark this module as a v2 test.
This is new v2 coverage but the file is missing the required pytest.mark.v2 marker, so marker-based runs can skip it.
Suggested fix
import os
import pytest
from unittest.mock import MagicMock, patch
+pytestmark = pytest.mark.v2
+
from codeframe.core.adapters.agent_adapter import AgentAdapter
from codeframe.core.engine_registry import (As per coding guidelines "tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Tests for engine registry.""" | |
| import os | |
| import pytest | |
| from unittest.mock import MagicMock, patch | |
| from codeframe.core.adapters.agent_adapter import AgentAdapter | |
| from codeframe.core.engine_registry import ( | |
| BUILTIN_ENGINES, | |
| EXTERNAL_ENGINES, | |
| VALID_ENGINES, | |
| get_adapter, | |
| get_builtin_adapter, | |
| get_external_adapter, | |
| is_external_engine, | |
| resolve_engine, | |
| ) | |
| """Tests for engine registry.""" | |
| import os | |
| import pytest | |
| from unittest.mock import MagicMock, patch | |
| pytestmark = pytest.mark.v2 | |
| from codeframe.core.adapters.agent_adapter import AgentAdapter | |
| from codeframe.core.engine_registry import ( | |
| BUILTIN_ENGINES, | |
| EXTERNAL_ENGINES, | |
| VALID_ENGINES, | |
| get_adapter, | |
| get_builtin_adapter, | |
| get_external_adapter, | |
| is_external_engine, | |
| resolve_engine, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_engine_registry.py` around lines 1 - 18, This test module
lacks the v2 marker; add a module-level marker by inserting "pytestmark =
pytest.mark.v2" (using the existing import pytest) near the top of the file
(e.g., after the imports) so all tests in tests/core/test_engine_registry.py are
marked v2; no other code changes required and this will satisfy the requirement
for new v2 tests such as those exercising get_adapter, get_builtin_adapter,
get_external_adapter, is_external_engine, resolve_engine, and the
BUILTIN_ENGINES/EXTERNAL_ENGINES/VALID_ENGINES constants.
| """Tests for runtime execute_agent with external engine adapters.""" | ||
|
|
||
| import pytest | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from codeframe.core.adapters.agent_adapter import AgentResult |
There was a problem hiding this comment.
Mark this runtime adapter suite as v2.
These tests exercise new v2 engine/adaptor flow, but the module is missing the required pytest.mark.v2 marker.
Suggested fix
"""Tests for runtime execute_agent with external engine adapters."""
import pytest
-from pathlib import Path
from unittest.mock import MagicMock, patch
from codeframe.core.adapters.agent_adapter import AgentResult
+pytestmark = pytest.mark.v2
+As per coding guidelines "tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2".
🧰 Tools
🪛 GitHub Actions: Test Suite (Unit + E2E)
[error] 4-4: Ruff check reported an unused import: 'pathlib.Path'. Remove unused import or use --fix to auto-fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_runtime_adapters.py` around lines 1 - 7, The test module
exercises the v2 engine/adaptor flow but lacks the required v2 marker; add a
module-level marker by defining pytestmark = pytest.mark.v2 (or decorate the
test functions with `@pytest.mark.v2`) near the top of the file so the suite is
recognized as v2; ensure you import pytest if not already present and place the
pytestmark assignment at module scope (referencing pytestmark and
pytest.mark.v2).
Code Review — Agent Adapter Architecture (#408)This is a well-structured implementation. The strangler-fig approach keeps existing engine paths untouched, the Bug:
|
| # | Severity | Issue |
|---|---|---|
| 1 | Bug | "built-in" falls through in runtime.execute_agent, causing runtime error |
| 2 | Medium | Subprocess timeout doesn't bound execution time while stdout is streaming |
| 3 | Minor | OpenCodeAdapter kwargs not forwarded |
| 4 | Minor | Missing pytestmark = pytest.mark.v2 on new test files |
| 5 | Nit | Missing type annotation on _format_gate_errors |
Items 1 and 2 should be fixed before merge. The rest can be addressed in follow-up.
Overall the architecture is clean — Protocol + strangler-fig + VerificationWrapper as a decorator is exactly the right shape for this problem. The test coverage for the new adapter layer is excellent.
|
test |
CodeFRAME v2 — Golden Path Contract (CLI-first)This document is the contract for CodeFRAME v2 development. Rule 0 (the only rule that matters):
This applies to both humans and agentic coding assistants. GoalsWhat "done" looks like (Enhanced MVP definition)CodeFRAME can run a complete end-to-end AI-driven development workflow from the CLI on a target repo:
No UI is required. Non-Goals (explicitly forbidden until Golden Path works)Do not build or refactor:
These may be revisited only after Golden Path is working and stable. Golden Path CLI Flow (the only flow that matters)0) Preconditions
1) Initialize a workspaceCommand:
Required behavior:
Artifacts:
2) AI-driven PRD generation and refinementCommands:
Required behavior for
3) Intelligent task generation with dependency analysisCommands:
Required behavior:
4) Batch task execution with orchestrationCommands:
Required behavior for batch execution:
5) Enhanced human-in-loop blocker resolutionCommands:
Required behavior:
6) Integrated Git workflow and PR managementCommands:
Required behavior:
7) Enhanced verification and quality gatesCommands:
Required behavior:
8) Integrated artifact and commit managementCommands:
Required behavior:
9) Comprehensive checkpointing and state managementCommands:
Required behavior:
State Machine (authoritative)Statuses:
Allowed transitions (comprehensive):
The CLI is the authority for transitions. PR Workflow Integration:
Implementation PrinciplesCore-first (no FastAPI in the core)
CLI-first (server optional)
Salvage safely
Keep it runnable
Acceptance Checklist (Enhanced MVP - must pass)Status: 🔄 Enhanced MVP Partially Complete 📊 Current Implementation StatusOverall Assessment: Enhanced MVP is ~60% complete with solid foundation but critical gaps remaining. ✅ Fully Implemented Phases:
|
ReactAgent Deep AnalysisDate: 2026-02-16 1. Test Run Summary
All 6 PRD requirements were implemented and functional. The CLI passes 60 tests and all commands work correctly (add, list, complete, delete, priority, search). 2. Architecture OverviewCore Files
Execution Flow3. What Worked Well3.1 LLM Dependency InferenceThe
This is a strong result — the dependencies were logically correct and enabled meaningful parallelism. 3.2 Code QualityThe generated code for the Task Tracker CLI was clean and well-structured:
3.3 Tool DesignThe 7-tool set is well-scoped:
3.4 Inline Lint FeedbackAfter every
This prevents lint errors from accumulating and gives the LLM immediate feedback. 3.5 Supervisor Auto-ResolutionThe
3.6 Loop Detection3-iteration identical tool-call signature detection prevents infinite loops. This worked in the test — no tasks got stuck in loops. 3.7 Conversation Compaction3-tier compaction prevents context overflow:
4. Issues Observed4.1 Verification Gate Failures on "Already Done" Tasks (HIGH)5 tasks blocked because verification gates kept failing after the features were already implemented by earlier tasks. The blocked tasks were:
Root cause: When parallel tasks create/modify the same files, later tasks in Group 4 try to add code that's already there. The edit_file search/replace fails because the code pattern has changed, and after 3 verification retries, it escalates to a blocker. Impact: 33% of tasks blocked unnecessarily. The project was fully functional despite these "failures." 4.2 Over-Granular Task Decomposition (MEDIUM)The LLM generated 15 tasks from a simple 20-line spec. Several tasks had overlapping scope:
Result: Tasks 10-13 are essentially verification/enhancement passes over work already completed by tasks 1-9. 4.3 Test Data Leakage (LOW)The "Test all features" task ran the CLI to create test tasks, but the data persisted in Root cause: The agent tests by running the CLI directly (via 4.4 Ruff Not in Target Project Dependencies (LOW)The verification gates try to run Current behavior: The agent's 4.5 Blocker Text Quality Could Be Better (LOW)All 5 blocked tasks had identical blocker text: "Verification keeps failing and automated fixes are not working." The blocker should include more specific context:
5. Future Optimization Opportunities5.1 Task Deduplication / No-Op Detection (HIGH PRIORITY)Problem: Tasks that describe features already implemented by earlier tasks waste iterations and create false blockers. Approaches:
Suggested implementation: Option 1 (pre-flight check) is lowest effort and highest impact. Add a 5.2 Smarter Task Generation Prompt (MEDIUM PRIORITY)Problem: The task generation prompt doesn't account for implicit features that frameworks provide (e.g., Click auto-generates Approach: Include framework-awareness in the task generation prompt: 5.3 Parallel Conflict Prevention (MEDIUM PRIORITY)Problem: Parallel tasks that modify the same files can conflict, causing search/replace failures. Approaches:
Suggested implementation: Option 2 (merge-on-conflict) — the 5.4 Gate Configuration Per-Project (MEDIUM PRIORITY)Problem: Verification gates assume ruff is available, but the target project may not have it. Approach: The Suggested implementation: Make 5.5 Adaptive Verification Retry Budget (LOW PRIORITY)Problem: All tasks get the same verification retry budget (5 retries). Simple tasks that fail verification after 3 retries are unlikely to succeed on retries 4-5. Approach: Reduce the retry budget when:
5.6 Better Blocker Context (LOW PRIORITY)Problem: Escalation blockers have generic text ("Verification keeps failing"). Approach: Include in the blocker:
5.7 Workspace-Aware Test Execution (LOW PRIORITY)Problem: The agent runs CLI commands that leave state behind (test data leakage). Approach: When the task is "test all features":
5.8 Context Compaction Improvements (LOW PRIORITY)The current 3-tier compaction is functional but could be improved:
5.9 Tool Result Compression (LOW PRIORITY)Problem: Approach:
5.10 Intent Preview for Complex Tasks (LOW PRIORITY)Current behavior: Tasks with Enhancement: For tasks with complexity >= 3, require the agent to output a structured plan (files to modify, approach) in its first response before executing any tools. This prevents the agent from diving into code before understanding the full scope. 6. Performance ProfileToken Usage (Estimated)
Timing
7. Comparison: Plan Engine vs React Engine
8. Key Takeaways
|
|
PART 1 of review - see body below for full text |
|
test body no hashes |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
codeframe/core/adapters/subprocess_adapter.py (1)
49-52: Consider usingOSErrorinstead ofEnvironmentError.
EnvironmentErrorhas been an alias forOSErrorsince Python 3.3. UsingOSErrordirectly is more idiomatic and consistent with the exception handling in therunmethod (line 148).Suggested change
- raise EnvironmentError( + raise OSError( f"'{binary}' not found on PATH. " f"Install it or ensure it is available in your environment." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/adapters/subprocess_adapter.py` around lines 49 - 52, Replace the use of the deprecated alias EnvironmentError with OSError in the subprocess adapter: change the exception raised where the code currently does raise EnvironmentError(...) to raise OSError(...) so it matches the idiomatic exception type used elsewhere (e.g., in the run method) and keeps exception handling consistent across functions in subprocess_adapter.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@codeframe/core/adapters/subprocess_adapter.py`:
- Around line 49-52: Replace the use of the deprecated alias EnvironmentError
with OSError in the subprocess adapter: change the exception raised where the
code currently does raise EnvironmentError(...) to raise OSError(...) so it
matches the idiomatic exception type used elsewhere (e.g., in the run method)
and keeps exception handling consistent across functions in
subprocess_adapter.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98dbe4f2-2b45-478e-af83-76715f625b30
📒 Files selected for processing (2)
codeframe/core/adapters/subprocess_adapter.pytests/core/adapters/test_subprocess_adapter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/adapters/test_subprocess_adapter.py
|
PLACEHOLDER - WILL UPDATE |
Code Review - PR 428: Agent Adapter ArchitectureThis is a well-structured implementation. Protocol over ABC, strangler fig pattern, lazy imports, and VerificationWrapper decoupling quality gates are all sound architectural choices consistent with CLAUDE.md. Bug: --engine built-in silently skips agent executioncodeframe/core/runtime.py - execute_agent validates engine against VALID_ENGINES (which includes built-in), then dispatches with if/elif branches for is_external_engine(), react, and plan. built-in matches none of these. A user passing --engine built-in passes validation but no agent ever runs. The run is created, no work happens, and the runtime proceeds to completion logic with no result. engine_registry.py has resolve_engine() that correctly normalizes built-in to react, but it is never called in execute_agent or app.py. Fix: call resolve_engine() early in the CLI/runtime path, or add engine == built-in to the elif engine == react branch. Potential deadlock in SubprocessAdapter.runcodeframe/core/adapters/subprocess_adapter.py - The read order streams stdout first, then reads ALL stderr after stdout finishes. This deadlocks if the subprocess fills the OS stderr pipe buffer (~64 KB) before stdout is consumed. With a verbose external agent this is realistic: the process blocks writing stderr while this code iterates stdout, so neither side makes progress. Fix: read stderr concurrently using a background thread. Redundant overrides in ClaudeCodeAdapter and OpenCodeAdapterBoth subclasses override build_command and get_stdin with implementations identical to SubprocessAdapter base class defaults. ~20 lines, no behavioral difference. Remove them or add a comment noting they are placeholders. Type safety in builtin.py_map_status and _map_state accept object and use type: ignore. Typing them as AgentStatus and AgentState removes the suppressions and clarifies intent. Minor: kwargs silently dropped for OpenCodeget_external_adapter accepts kwargs but ignores them for opencode. Add a comment noting the intentional drop. Test structure notetests/core/test_runtime_adapters.py - test_external_engine_skips_api_key_check uses manual patch start/stop instead of contextlib.ExitStack. Correct but hard to follow. Summary
Architecture is well-designed and coverage is thorough (127 new tests across 8 files). The built-in dispatch gap and subprocess deadlock are the two items worth addressing before merge. |
CodeFRAME v2 — Golden Path Contract (CLI-first)This document is the contract for CodeFRAME v2 development. Rule 0 (the only rule that matters):
This applies to both humans and agentic coding assistants. GoalsWhat "done" looks like (Enhanced MVP definition)CodeFRAME can run a complete end-to-end AI-driven development workflow from the CLI on a target repo:
No UI is required. Non-Goals (explicitly forbidden until Golden Path works)Do not build or refactor:
These may be revisited only after Golden Path is working and stable. Golden Path CLI Flow (the only flow that matters)0) Preconditions
1) Initialize a workspaceCommand:
Required behavior:
Artifacts:
2) AI-driven PRD generation and refinementCommands:
Required behavior for
3) Intelligent task generation with dependency analysisCommands:
Required behavior:
4) Batch task execution with orchestrationCommands:
Required behavior for batch execution:
5) Enhanced human-in-loop blocker resolutionCommands:
Required behavior:
6) Integrated Git workflow and PR managementCommands:
Required behavior:
7) Enhanced verification and quality gatesCommands:
Required behavior:
8) Integrated artifact and commit managementCommands:
Required behavior:
9) Comprehensive checkpointing and state managementCommands:
Required behavior:
State Machine (authoritative)Statuses:
Allowed transitions (comprehensive):
The CLI is the authority for transitions. PR Workflow Integration:
Implementation PrinciplesCore-first (no FastAPI in the core)
CLI-first (server optional)
Salvage safely
Keep it runnable
Acceptance Checklist (Enhanced MVP - must pass)Status: 🔄 Enhanced MVP Partially Complete 📊 Current Implementation StatusOverall Assessment: Enhanced MVP is ~60% complete with solid foundation but critical gaps remaining. ✅ Fully Implemented Phases:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/test_runtime_adapters.py (1)
50-115: Simplify test setup by removing duplicate patches and unused code.A few cleanup opportunities:
run_gatesis patched twice (line 61 and lines 100-105) - remove from line 61.mocksdict (lines 69-72) is populated but never used.os.environ.pop("ANTHROPIC_API_KEY", None)on line 66 is redundant sincepatch.dict(..., clear=True)already clears the environment.Suggested simplification
def test_external_engine_skips_api_key_check(self, mock_workspace, mock_run): """External engines should not require ANTHROPIC_API_KEY.""" from codeframe.core.runtime import execute_agent patches = _runtime_patches() + [ patch( "codeframe.core.runtime.get_external_adapter", create=True, ), patch("codeframe.core.context_packager.ContextLoader"), patch("codeframe.core.runtime.complete_run"), - patch("codeframe.core.adapters.verification_wrapper.run_gates"), ] with patch.dict("os.environ", {}, clear=True): - import os - os.environ.pop("ANTHROPIC_API_KEY", None) - # Apply all patches - mocks = {} for p in patches: - m = p.start() - mocks[p.attribute or ""] = m + p.start() try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_runtime_adapters.py` around lines 50 - 115, The test test_external_engine_skips_api_key_check contains redundant setup: remove the duplicate patch for run_gates from the initial patches list (the later with patch("codeframe.core.adapters.verification_wrapper.run_gates") inside the import block is the one used), drop the unused mocks dict population (the variable mocks is never referenced), and remove the redundant call to os.environ.pop("ANTHROPIC_API_KEY", None) since patch.dict(..., clear=True) already clears the environment; keep the single inner patch of run_gates that configures mock_gate_result and ensure patches are started/stopped as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/test_runtime_adapters.py`:
- Around line 50-115: The test test_external_engine_skips_api_key_check contains
redundant setup: remove the duplicate patch for run_gates from the initial
patches list (the later with
patch("codeframe.core.adapters.verification_wrapper.run_gates") inside the
import block is the one used), drop the unused mocks dict population (the
variable mocks is never referenced), and remove the redundant call to
os.environ.pop("ANTHROPIC_API_KEY", None) since patch.dict(..., clear=True)
already clears the environment; keep the single inner patch of run_gates that
configures mock_gate_result and ensure patches are started/stopped as currently
done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4f2cd95-b7f1-4f80-9884-c2fb13a6f9e2
📒 Files selected for processing (2)
tests/core/adapters/test_builtin.pytests/core/test_runtime_adapters.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/adapters/test_builtin.py
Code Review — Agent Adapter Architecture (#408)This is a well-structured PR that delivers the adapter architecture cleanly. The strangler-fig approach (existing react/plan paths untouched), Issues1.
|
1 participant
Summary
Implements #408: Agent Adapter Architecture — delegate to frontier coding agents.
Refactors CodeFrame's execution layer so frontier coding agents (Claude Code, OpenCode) are first-class execution engines. CodeFrame becomes the orchestrator while delegating actual code writing to specialized tools.
New architecture:
What was added:
typing.Protocolfor structural subtyping (any class withname+run()works)claude --printwith prompt via stdinopencode --non-interactivewith prompt via stdinexecute_agent(), conditional API key checks--engineflag now accepts:react,plan,claude-code,opencode,built-inWhat was NOT changed:
Acceptance Criteria
cf work start <id> --execute --engine claude-codedelegates to Claude Code CLIcf work start <id> --execute --engine opencodedelegates to OpenCodecf work start <id> --execute --engine built-inuses existing ReactAgentCODEFRAME_ENGINEworks now)Test Plan
Implementation Notes
AgentAdapterusestyping.Protocolfor structural subtyping (external adapters don't need to import CodeFrame)classify_error_for_blockerfromblocker_detection.pyCloses #408
Summary by CodeRabbit
New Features
Behavior Changes
Tests