feat(core): engine registry and runtime selection (#414) - #433
Merged
Conversation
Replace hardcoded engine branching in execute_agent() with unified adapter registry lookup. Engine-specific retry logic (stall retry for React, supervisor retry for Plan) moved into the adapter shims so the runtime is engine-agnostic. - Refactor execute_agent() from ~180 lines of 3-branch conditional to ~80 lines using get_builtin_adapter()/get_external_adapter() - Move stall detection retry into BuiltinReactAdapter.run() - Move supervisor BLOCKED-retry and tactical recovery into BuiltinPlanAdapter.run() with graceful error handling - Add check_requirements() to engine_registry for requirement validation - Add engine field to EnvironmentConfig with VALID_ENGINES validation - Add cf engines list/check CLI commands - Wire workspace config engine default into work_start and batch_run - Add requirements() classmethod to builtin adapters
Contributor
WalkthroughAdds a pluggable engine registry and runtime dispatch for builtin/external adapters, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (engines check)
participant Registry as Engine Registry
participant Adapter as Adapter Class
participant Env as Environment
participant Console as Console
User->>CLI: cf engines check react
CLI->>Registry: check_requirements("react")
Registry->>Registry: validate engine name
Registry->>Adapter: _get_adapter_class("react")
Adapter-->>Registry: BuiltinReactAdapter class
Registry->>Adapter: BuiltinReactAdapter.requirements()
Adapter-->>Registry: {OPENAI_API_KEY: true}
Registry->>Env: verify env vars
Env-->>Registry: {OPENAI_API_KEY: true}
Registry-->>CLI: requirement status dict
CLI->>Console: render table with ✓/✗
Console-->>User: display results
sequenceDiagram
participant Runner as execute_agent()
participant Registry as Engine Registry
participant Builtin as Builtin Adapter
participant External as External Adapter
participant Workspace as Workspace/EventBus
participant Result as AgentResult
Runner->>Registry: get_builtin_adapter(engine) / get_external_adapter(engine)
alt engine is external
Registry-->>External: adapter instance
Runner->>External: adapter.run(on_event)
External-->>Result: AgentResult
else builtin engine
Registry-->>Builtin: adapter instance
Runner->>Builtin: adapter.run(stall_timeout_s, stall_action, on_event)
Builtin-->>Result: AgentResult
end
Runner->>Runner: map Result.status -> AgentStatus
Runner->>Workspace: emit events / create blockers if BLOCKED
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 |
- Fix engine resolution priority: CLI flag → env var → workspace config → default (env var was being bypassed when workspace config existed) - Support check_ready() classmethod on adapters for non-env-var requirement checks (e.g., binary presence for external engines) - Add test coverage for BuiltinPlanAdapter run/retry paths: supervisor unblock, tactical recovery, exception handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/runtime.py (1)
661-775:⚠️ Potential issue | 🔴 CriticalFail the run when adapter setup/execution throws.
Any exception raised while constructing or running the adapter currently skips straight to
finally. A missing external binary is the easy repro: the CLI has already created the run, then the task staysIN_PROGRESSwith aRUNNINGrun forever.Suggested fix
try: # Create event callback to emit workspace events and log def on_agent_event(event_type: str, data: dict) -> None: ... ... return state + except Exception as exc: + run_logger.error( + LogCategory.ERROR, + "Agent execution crashed", + {"engine": engine, "error": str(exc)[:500]}, + ) + fail_run(workspace, run.id, str(exc)[:500]) + raise finally: # Always close the output logger to ensure file is properly flushed output_logger.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/runtime.py` around lines 661 - 775, The try block around adapter setup and execution lacks an except handler so any exception (e.g., from get_external_adapter, VerificationWrapper.run, get_builtin_adapter, or adapter.run) falls through to finally and leaves the run IN_PROGRESS; add an except Exception as e: block after the try to log the error via run_logger.error (include exception details), call fail_run(workspace, run.id), construct and return an AgentState(status=AgentStatus.FAILED) (or set agent_status appropriately), then let the existing finally close output_logger; this ensures errors during adapter construction/execution properly mark the run as failed.
🤖 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/builtin.py`:
- Around line 105-116: The retry/failure branches in the adapter (handling
StallDetectedError and the supervisor recovery paths) currently only log via
logger and must also emit AgentEvent entries using the event API from
core/events.py so runtime and workspace logs observe state transitions; update
the StallDetectedError handler in this module to import the event emitter (e.g.,
emit_event or AgentEvent factory) from core/events.py and call it on each retry
(“stall_retry” with attempt number) and on final failure (“stall_failed” with
attempt count and error text), and apply the same pattern to the supervisor
recovery branches referenced around lines 203-262 so every retry, recovery
attempt, and terminal failure emits a corresponding AgentEvent while preserving
the existing logger calls and the existing return of AgentResult.
In `@codeframe/core/engine_registry.py`:
- Around line 167-171: check_requirements() currently treats every requirement
key as an env var; update it to detect when a req_method is providing subprocess
engine requirements and verify executables instead of only os.getenv.
Specifically, in check_requirements() (the loop that iterates reqs produced by
req_method()), branch based on the requirement type or known subprocess keys
(e.g., "claude-code", "opencode" or a marker returned by req_method) and use
shutil.which() (or equivalent) to test that the executable exists on PATH,
setting result[key] = bool(shutil.which(key)) for those cases; leave the
existing os.getenv() check for environment-variable requirements and ensure
result remains a dict[str, bool].
In `@codeframe/core/runtime.py`:
- Around line 675-701: The external-adapter event bridge (on_adapter_event) must
also forward AdapterEvent(type="output") payloads into the RunOutputLogger so
external engines produce visible stdout; update on_adapter_event (used by
wrapper.run / VerificationWrapper) to check if event.type == "output", extract
the text (event.data["line"] or similar) and write/append it into the run's
RunOutputLogger instance (e.g., obtain the existing run.output_logger or create
a RunOutputLogger for run.task_id/workspace) in addition to calling
on_agent_event(event.type, event.data).
- Around line 734-748: The code currently rebuilds AgentState from only
result.status which drops useful data (blocker, step_results, last error);
instead construct AgentState preserving those fields by passing the existing
fields from result into the AgentState constructor (e.g.,
AgentState(status=agent_status, blocker=result.blocker or
result.blocker_question, step_results=result.step_results,
last_error=result.last_error)) so callers can inspect state.blocker and
state.step_results as before; keep the existing blocker creation branch that
calls blockers.create(...) when result.status == "blocked".
In `@tests/core/test_engine_registry_extended.py`:
- Around line 1-8: The new test module test_engine_registry_extended.py is
missing the v2 marker; add module-level or per-test marking so CI treats it as a
v2-only test: either add pytestmark = pytest.mark.v2 at the top of the module or
decorate the test functions/classes with `@pytest.mark.v2`; update the module that
imports check_requirements and _get_adapter_class to include that marker so the
new engine-registry/runtime paths are skipped by non-v2 runs.
---
Outside diff comments:
In `@codeframe/core/runtime.py`:
- Around line 661-775: The try block around adapter setup and execution lacks an
except handler so any exception (e.g., from get_external_adapter,
VerificationWrapper.run, get_builtin_adapter, or adapter.run) falls through to
finally and leaves the run IN_PROGRESS; add an except Exception as e: block
after the try to log the error via run_logger.error (include exception details),
call fail_run(workspace, run.id), construct and return an
AgentState(status=AgentStatus.FAILED) (or set agent_status appropriately), then
let the existing finally close output_logger; this ensures errors during adapter
construction/execution properly mark the run as failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b412501-d8cf-4c8e-a5e6-be5aa0f0b8c6
📒 Files selected for processing (8)
codeframe/cli/app.pycodeframe/cli/engines_commands.pycodeframe/core/adapters/builtin.pycodeframe/core/config.pycodeframe/core/engine_registry.pycodeframe/core/runtime.pytests/core/test_engine_registry_extended.pytests/e2e/cli/test_engines_cli.py
Comment on lines
+105
to
+116
| except StallDetectedError as exc: | ||
| logger.warning( | ||
| "Stall detected (attempt %d): %s", | ||
| stall_attempt + 1, exc, | ||
| ) | ||
| if stall_attempt >= _MAX_STALL_RETRIES: | ||
| logger.error("Max stall retries exceeded, failing task") | ||
| return AgentResult( | ||
| status="failed", | ||
| error=f"Stall detected after {stall_attempt + 1} attempts: {exc}", | ||
| ) | ||
| logger.info("Retrying after stall (attempt %d)", stall_attempt + 2) |
Contributor
There was a problem hiding this comment.
Emit recovery events from these adapter retry paths.
Stall retries and supervisor recoveries now live here, but these branches only write to logger. Runtime never sees an AgentEvent for them, so the workspace event log/SSE stream misses the retry/intervention entirely.
As per coding guidelines, codeframe/core/**/*.py: All core modules must emit events for state transitions via core/events.py for audit and observability.
Also applies to: 203-262
🤖 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 105 - 116, The retry/failure
branches in the adapter (handling StallDetectedError and the supervisor recovery
paths) currently only log via logger and must also emit AgentEvent entries using
the event API from core/events.py so runtime and workspace logs observe state
transitions; update the StallDetectedError handler in this module to import the
event emitter (e.g., emit_event or AgentEvent factory) from core/events.py and
call it on each retry (“stall_retry” with attempt number) and on final failure
(“stall_failed” with attempt count and error text), and apply the same pattern
to the supervisor recovery branches referenced around lines 203-262 so every
retry, recovery attempt, and terminal failure emits a corresponding AgentEvent
while preserving the existing logger calls and the existing return of
AgentResult.
Comment on lines
+167
to
+171
| reqs = req_method() | ||
| result: dict[str, bool] = {} | ||
| for key in reqs: | ||
| # Check environment variables | ||
| result[key] = bool(os.getenv(key)) |
Contributor
There was a problem hiding this comment.
check_requirements() only knows how to check env vars.
This loop treats every requirement key as an environment variable. For subprocess engines, the missing prerequisite is often the executable itself, so cf engines check cannot correctly flag a missing claude-code/opencode binary and can report an engine as ready when launch will still fail.
🤖 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 167 - 171,
check_requirements() currently treats every requirement key as an env var;
update it to detect when a req_method is providing subprocess engine
requirements and verify executables instead of only os.getenv. Specifically, in
check_requirements() (the loop that iterates reqs produced by req_method()),
branch based on the requirement type or known subprocess keys (e.g.,
"claude-code", "opencode" or a marker returned by req_method) and use
shutil.which() (or equivalent) to test that the executable exists on PATH,
setting result[key] = bool(shutil.which(key)) for those cases; leave the
existing os.getenv() check for environment-variable requirements and ensure
result remains a dict[str, bool].
Comment on lines
+675
to
701
| # Bridge AgentEvent callbacks to workspace event system | ||
| def on_adapter_event(event: AdapterEvent) -> None: | ||
| on_agent_event(event.type, event.data) | ||
|
|
||
| # Get adapter via registry and run | ||
| if is_external_engine(engine): | ||
| from codeframe.core.engine_registry import get_external_adapter | ||
| from codeframe.core.context_packager import TaskContextPackager | ||
| from codeframe.core.adapters.verification_wrapper import VerificationWrapper | ||
| from codeframe.core.adapters.agent_adapter import AgentEvent | ||
|
|
||
| run_logger.info( | ||
| LogCategory.AGENT_ACTION, | ||
| f"Using external engine: {engine}", | ||
| {"engine": engine}, | ||
| ) | ||
|
|
||
| # Build rich context prompt for the external agent | ||
| packager = TaskContextPackager(workspace) | ||
| packaged = packager.build(run.task_id) | ||
|
|
||
| # Get the adapter and wrap with verification gates | ||
| adapter = get_external_adapter(engine) | ||
| wrapper = VerificationWrapper( | ||
| adapter, | ||
| workspace, | ||
| max_correction_rounds=3, | ||
| verbose=verbose, | ||
| adapter, workspace, max_correction_rounds=3, verbose=verbose, | ||
| ) | ||
|
|
||
| # Bridge AgentEvent callbacks to workspace event system | ||
| def on_adapter_event(event: AgentEvent) -> None: | ||
| on_agent_event(event.type, event.data) | ||
|
|
||
| result = wrapper.run( | ||
| run.task_id, | ||
| packaged.prompt, | ||
| workspace.repo_path, | ||
| run.task_id, packaged.prompt, workspace.repo_path, | ||
| on_event=on_adapter_event, | ||
| ) |
Contributor
There was a problem hiding this comment.
Write external-engine stdout into RunOutputLogger.
Builtin adapters receive output_logger directly, but external adapters only surface output through AgentEvent(type="output", data={"line": ...}). on_adapter_event() forwards those events into events.emit_for_workspace() only, so cf work follow has nothing to read for claude-code/opencode runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 675 - 701, The external-adapter event
bridge (on_adapter_event) must also forward AdapterEvent(type="output") payloads
into the RunOutputLogger so external engines produce visible stdout; update
on_adapter_event (used by wrapper.run / VerificationWrapper) to check if
event.type == "output", extract the text (event.data["line"] or similar) and
write/append it into the run's RunOutputLogger instance (e.g., obtain the
existing run.output_logger or create a RunOutputLogger for
run.task_id/workspace) in addition to calling on_agent_event(event.type,
event.data).
Comment on lines
+734
to
748
| # Map AgentResult to AgentState for rest of runtime | ||
| status_map = { | ||
| "completed": AgentStatus.COMPLETED, | ||
| "failed": AgentStatus.FAILED, | ||
| "blocked": AgentStatus.BLOCKED, | ||
| } | ||
| agent_status = status_map.get(result.status, AgentStatus.FAILED) | ||
| state = AgentState(status=agent_status) | ||
|
|
||
| # Create blocker if adapter reported one | ||
| if result.status == "blocked" and result.blocker_question: | ||
| from codeframe.core import blockers | ||
| blockers.create( | ||
| workspace, task_id=run.task_id, question=result.blocker_question, | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't return a status-only AgentState.
The caller still inspects state.blocker and state.step_results to print the blocker question and last error. Rebuilding AgentState from just result.status drops that data, so blocked/failed runs lose their most useful CLI feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 734 - 748, The code currently
rebuilds AgentState from only result.status which drops useful data (blocker,
step_results, last error); instead construct AgentState preserving those fields
by passing the existing fields from result into the AgentState constructor
(e.g., AgentState(status=agent_status, blocker=result.blocker or
result.blocker_question, step_results=result.step_results,
last_error=result.last_error)) so callers can inspect state.blocker and
state.step_results as before; keep the existing blocker creation branch that
calls blockers.create(...) when result.status == "blocked".
added 2 commits
March 9, 2026 20:11
Catch unhandled exceptions in execute_agent() and transition the run to FAILED instead of leaving it stuck IN_PROGRESS. Addresses CodeRabbit review finding about runs staying in limbo when adapter setup throws.
…rker - Set state.blocker when adapter reports a blocker question so the CLI can display the question (previously only created in workspace DB) - Add pytestmark = pytest.mark.v2 to test_engine_registry_extended.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codeframe/cli/app.py (1)
2033-2040: Consider extracting duplicated engine resolution logic.This engine resolution block (CLI → env var → workspace config → default) is duplicated verbatim in
batch_run(lines 2926-2933). The existingresolve_engine()inengine_registry.pyhandles CLI → env var → default but doesn't support workspace config fallback.Consider extending
resolve_engine()to accept an optional workspace path for config-based fallback, or extract a helper function in this module to avoid the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/cli/app.py` around lines 2033 - 2040, The duplicated engine resolution logic in app.py and batch_run should be consolidated: update resolve_engine (in engine_registry.py) to accept an optional workspace path parameter (e.g., resolve_engine(cli_value, workspace_path=None)) so it performs CLI → env var → workspace config → default resolution, using load_environment_config(path) when workspace_path is provided; then replace the duplicated blocks in app.py (the engine resolution code) and in batch_run with calls to the extended resolve_engine(...) to avoid duplication.
🤖 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/runtime.py`:
- Around line 744-766: The code creates a blocker via blockers.create(...) but
never captures its ID, then calls block_run(workspace, run.id, "") losing the
blocker linkage; update the logic to store the created blocker (e.g., blocker =
blockers.create(...)) and pass its identifier into block_run(workspace, run.id,
blocker.id or blocker_id) instead of the empty string, ensuring you use the same
creation call (blockers.create) and the block_run(...) function to maintain the
association.
---
Nitpick comments:
In `@codeframe/cli/app.py`:
- Around line 2033-2040: The duplicated engine resolution logic in app.py and
batch_run should be consolidated: update resolve_engine (in engine_registry.py)
to accept an optional workspace path parameter (e.g., resolve_engine(cli_value,
workspace_path=None)) so it performs CLI → env var → workspace config → default
resolution, using load_environment_config(path) when workspace_path is provided;
then replace the duplicated blocks in app.py (the engine resolution code) and in
batch_run with calls to the extended resolve_engine(...) to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7edc7c40-1aad-4770-9e8b-9e05cc43eaed
📒 Files selected for processing (4)
codeframe/cli/app.pycodeframe/core/engine_registry.pycodeframe/core/runtime.pytests/core/test_engine_registry_extended.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/test_engine_registry_extended.py
Comment on lines
+744
to
+766
| if result.status == "blocked" and result.blocker_question: | ||
| from codeframe.core import blockers | ||
| blockers.create( | ||
| workspace, task_id=run.task_id, question=result.blocker_question, | ||
| ) | ||
|
|
||
| state = agent.run(run.task_id) | ||
|
|
||
| # If agent is BLOCKED, try supervisor resolution | ||
| # (only for plan engine — ReactAgent handles retries internally) | ||
| if state.status == AgentStatus.BLOCKED: | ||
| from codeframe.core.conductor import get_supervisor | ||
|
|
||
| supervisor = get_supervisor(workspace) | ||
| if supervisor.try_resolve_blocked_task(run.task_id): | ||
| # Supervisor resolved the blocker - retry the agent | ||
| print("[Supervisor] Retrying task after auto-resolution...") | ||
|
|
||
| # Create a new agent instance and retry | ||
| agent = Agent( | ||
| workspace=workspace, | ||
| llm_provider=provider, | ||
| dry_run=dry_run, | ||
| on_event=on_agent_event, | ||
| debug=debug, | ||
| verbose=verbose, | ||
| fix_coordinator=fix_coordinator, | ||
| output_logger=output_logger, | ||
| event_publisher=event_publisher, | ||
| ) | ||
| state = agent.run(run.task_id) | ||
|
|
||
| # If agent FAILED, check if supervisor can help with common technical issues | ||
| # (only for plan engine — ReactAgent handles retries internally and | ||
| # doesn't populate the AgentState fields that supervisor inspection needs) | ||
| if state.status == AgentStatus.FAILED and engine == "plan": | ||
| from codeframe.core.conductor import get_supervisor, SUPERVISOR_TACTICAL_PATTERNS | ||
|
|
||
| if debug: | ||
| logger.debug("Agent FAILED - analyzing for supervisor intervention") | ||
| logger.debug("state.blocker: %s", state.blocker) | ||
| logger.debug( | ||
| "state.step_results count: %d", | ||
| len(state.step_results) if state.step_results else 0 | ||
| ) | ||
| logger.debug( | ||
| "state.gate_results count: %d", | ||
| len(state.gate_results) if state.gate_results else 0 | ||
| ) | ||
|
|
||
| # Extract error message from available sources | ||
| error_msg = "" | ||
| error_source = "none" | ||
| if state.blocker: | ||
| error_msg = state.blocker.reason or state.blocker.question or "" | ||
| error_source = "blocker" | ||
| elif state.step_results: | ||
| # Check last step result for error info | ||
| last_result = state.step_results[-1] | ||
| if debug: | ||
| error_preview = last_result.error[:200] if last_result.error else "None" | ||
| logger.debug( | ||
| "Last step result: status=%s, error=%s", | ||
| last_result.status, error_preview | ||
| ) | ||
| if hasattr(last_result, 'error') and last_result.error: | ||
| error_msg = last_result.error | ||
| error_source = "step_result.error" | ||
| elif hasattr(last_result, 'output') and last_result.output: | ||
| error_msg = last_result.output | ||
| error_source = "step_result.output" | ||
| elif state.gate_results: | ||
| # Check gate results for failure info | ||
| for gate in state.gate_results: | ||
| if debug: | ||
| logger.debug("Gate result: passed=%s", gate.passed) | ||
| if not gate.passed: | ||
| for check in gate.checks: | ||
| if debug: | ||
| output_preview = check.output[:100] if check.output else "None" | ||
| logger.debug( | ||
| " Check: %s status=%s output=%s", | ||
| check.name, check.status, output_preview | ||
| ) | ||
| if check.output: | ||
| error_msg = check.output | ||
| error_source = f"gate.{check.name}" | ||
| break | ||
|
|
||
| if debug: | ||
| logger.debug("Extracted error from: %s", error_source) | ||
| error_preview = error_msg[:300] if error_msg else "EMPTY" | ||
| logger.debug("Error message (first 300 chars): %s", error_preview) | ||
|
|
||
| error_msg_lower = error_msg.lower() | ||
| matched_patterns = [p for p in SUPERVISOR_TACTICAL_PATTERNS if p in error_msg_lower] | ||
| if debug: | ||
| logger.debug("Matched tactical patterns: %s", matched_patterns) | ||
|
|
||
| if error_msg and matched_patterns: | ||
| supervisor = get_supervisor(workspace) | ||
| resolution = supervisor._generate_tactical_resolution(error_msg) | ||
| logger.info( | ||
| "Supervisor detected recoverable error, providing guidance: %s...", | ||
| resolution[:100] | ||
| ) | ||
|
|
||
| # Create a blocker with the resolution for the agent's next run | ||
| from codeframe.core import blockers | ||
| blocker = blockers.create( | ||
| workspace, | ||
| task_id=run.task_id, | ||
| question=f"Technical error: {error_msg[:500]}", | ||
| ) | ||
| blockers.answer(workspace, blocker.id, resolution) | ||
| if debug: | ||
| logger.debug("Created blocker %s and answered with resolution", blocker.id[:8]) | ||
|
|
||
| # Retry the agent with the new context | ||
| logger.info("Supervisor retrying task with guidance...") | ||
| agent = Agent( | ||
| workspace=workspace, | ||
| llm_provider=provider, | ||
| dry_run=dry_run, | ||
| on_event=on_agent_event, | ||
| debug=debug, | ||
| verbose=verbose, | ||
| fix_coordinator=fix_coordinator, | ||
| output_logger=output_logger, | ||
| event_publisher=event_publisher, | ||
| ) | ||
| state = agent.run(run.task_id) | ||
| if debug: | ||
| logger.debug("Retry completed with status: %s", state.status) | ||
| elif debug: | ||
| logger.debug( | ||
| "No supervisor intervention - error_msg empty=%s, no pattern match=%s", | ||
| not error_msg, not matched_patterns | ||
| ) | ||
|
|
||
| # Log final status | ||
| if state.status == AgentStatus.COMPLETED: | ||
| run_logger.info(LogCategory.STATE_CHANGE, "Agent completed successfully") | ||
| elif state.status == AgentStatus.BLOCKED: | ||
| blocker_reason = state.blocker.question if state.blocker else "Unknown" | ||
| run_logger.warning(LogCategory.BLOCKER, f"Agent blocked: {blocker_reason[:200]}", { | ||
| "blocker_question": blocker_reason, | ||
| run_logger.warning(LogCategory.BLOCKER, f"Agent blocked: {result.blocker_question or 'Unknown'}", { | ||
| "blocker_question": result.blocker_question or "Unknown", | ||
| }) | ||
| elif state.status == AgentStatus.FAILED: | ||
| # Log detailed error information for diagnosis | ||
| error_info = {} | ||
| if state.step_results: | ||
| last_step = state.step_results[-1] | ||
| error_info["last_step_status"] = last_step.status.value if hasattr(last_step.status, 'value') else str(last_step.status) | ||
| error_info["last_step_error"] = last_step.error[:500] if last_step.error else None | ||
| if state.gate_results: | ||
| error_info["gate_failures"] = sum(1 for g in state.gate_results if not g.passed) | ||
| run_logger.error(LogCategory.ERROR, "Agent execution failed", error_info) | ||
| run_logger.error(LogCategory.ERROR, "Agent execution failed", { | ||
| "error": (result.error or "")[:500], | ||
| }) | ||
|
|
||
| # Update run status based on agent result | ||
| if state.status == AgentStatus.COMPLETED: | ||
| complete_run(workspace, run.id) | ||
| elif state.status == AgentStatus.BLOCKED: | ||
| # Get blocker ID from state if available | ||
| blocker_id = "" | ||
| if state.blocker and hasattr(state, "_blocker_id"): | ||
| blocker_id = state._blocker_id | ||
| block_run(workspace, run.id, blocker_id) | ||
| block_run(workspace, run.id, "") |
Contributor
There was a problem hiding this comment.
Blocker ID not passed to block_run().
The blocker is created on lines 746-748, but its ID isn't captured and passed to block_run() on line 766. The empty string "" loses the association between the run and the blocker.
♻️ Suggested fix
+blocker_id = ""
# Create blocker if adapter reported one
if result.status == "blocked" and result.blocker_question:
from codeframe.core import blockers
- blockers.create(
+ blocker = blockers.create(
workspace, task_id=run.task_id, question=result.blocker_question,
)
+ blocker_id = blocker.id
# ... later ...
elif state.status == AgentStatus.BLOCKED:
- block_run(workspace, run.id, "")
+ block_run(workspace, run.id, blocker_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 744 - 766, The code creates a blocker
via blockers.create(...) but never captures its ID, then calls
block_run(workspace, run.id, "") losing the blocker linkage; update the logic to
store the created blocker (e.g., blocker = blockers.create(...)) and pass its
identifier into block_run(workspace, run.id, blocker.id or blocker_id) instead
of the empty string, ensuring you use the same creation call (blockers.create)
and the block_run(...) function to maintain the association.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
codeframe/core/runtime.py (2)
743-749:⚠️ Potential issue | 🟡 MinorPass the created blocker ID into
block_run().The blocked branch now creates
blocker_obj, but Line 767 still callsblock_run(workspace, run.id, ""). That drops the run↔blocker association immediately.♻️ Proposed fix
- # Create blocker if adapter reported one and populate state for CLI + blocker_id = "" + + # Create blocker if adapter reported one and populate state for CLI if result.status == "blocked" and result.blocker_question: from codeframe.core import blockers as blockers_mod blocker_obj = blockers_mod.create( workspace, task_id=run.task_id, question=result.blocker_question, ) + blocker_id = blocker_obj.id state.blocker = blocker_obj @@ if state.status == AgentStatus.COMPLETED: complete_run(workspace, run.id) elif state.status == AgentStatus.BLOCKED: - block_run(workspace, run.id, "") + block_run(workspace, run.id, blocker_id) elif state.status == AgentStatus.FAILED: fail_run(workspace, run.id)Also applies to: 766-767
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/runtime.py` around lines 743 - 749, The blocked branch creates a blocker via blockers_mod.create and assigns it to state.blocker, but the subsequent call to block_run uses an empty string and loses the association; update the call to block_run to pass the created blocker ID (use blocker_obj.id or blocker_obj.get_id() as appropriate) instead of "" so the run (run.id) remains linked to the blocker; ensure you reference the created blocker_obj from the same block where blockers_mod.create and state.blocker are set.
675-701:⚠️ Potential issue | 🟠 MajorMirror external
outputevents intoRunOutputLogger.
output_loggeris only passed to builtin adapters. On the external path, Lines 676-677 forwardAgentEventobjects into workspace events only, soAgentEvent(type="output")never reaches the run log andcf work followstays empty for subprocess stdout/stderr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/runtime.py` around lines 675 - 701, The on_adapter_event callback currently only forwards AdapterEvent objects into workspace events so AgentEvent(type="output") from external adapters never reaches the run log; update on_adapter_event (the function defined above) to detect events with type "output" and mirror their payload into the run's output logger (the RunOutputLogger associated with the current run, e.g. run.output_logger or the RunOutputLogger API) in addition to emitting the workspace event so that wrapper.run / VerificationWrapper.run external adapter output is recorded and cf work follow shows subprocess stdout/stderr.
🤖 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/runtime.py`:
- Around line 758-769: The final state transition is dropping the failure
reason—when state.status is BLOCKED or FAILED you must pass the adapter
error/exception into the state-change call so emitted RUN_FAILED/RUN_BLOCKED
events include context; update the calls to block_run(workspace, run.id, ...)
and fail_run(workspace, run.id, ...) to pass a concise failure message (prefer
result.error if present else str(exc) or repr(exc), truncated to a safe length
similar to the run_logger.error usage) and ensure you reference state.status,
result.error, exc, run_logger.error, run.id, block_run and fail_run when making
the change.
---
Duplicate comments:
In `@codeframe/core/runtime.py`:
- Around line 743-749: The blocked branch creates a blocker via
blockers_mod.create and assigns it to state.blocker, but the subsequent call to
block_run uses an empty string and loses the association; update the call to
block_run to pass the created blocker ID (use blocker_obj.id or
blocker_obj.get_id() as appropriate) instead of "" so the run (run.id) remains
linked to the blocker; ensure you reference the created blocker_obj from the
same block where blockers_mod.create and state.blocker are set.
- Around line 675-701: The on_adapter_event callback currently only forwards
AdapterEvent objects into workspace events so AgentEvent(type="output") from
external adapters never reaches the run log; update on_adapter_event (the
function defined above) to detect events with type "output" and mirror their
payload into the run's output logger (the RunOutputLogger associated with the
current run, e.g. run.output_logger or the RunOutputLogger API) in addition to
emitting the workspace event so that wrapper.run / VerificationWrapper.run
external adapter output is recorded and cf work follow shows subprocess
stdout/stderr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6749af-411a-4da6-8677-ba493779554b
📒 Files selected for processing (2)
codeframe/core/runtime.pytests/core/test_engine_registry_extended.py
Comment on lines
758
to
769
| elif state.status == AgentStatus.FAILED: | ||
| # Log detailed error information for diagnosis | ||
| error_info = {} | ||
| if state.step_results: | ||
| last_step = state.step_results[-1] | ||
| error_info["last_step_status"] = last_step.status.value if hasattr(last_step.status, 'value') else str(last_step.status) | ||
| error_info["last_step_error"] = last_step.error[:500] if last_step.error else None | ||
| if state.gate_results: | ||
| error_info["gate_failures"] = sum(1 for g in state.gate_results if not g.passed) | ||
| run_logger.error(LogCategory.ERROR, "Agent execution failed", error_info) | ||
| run_logger.error(LogCategory.ERROR, "Agent execution failed", { | ||
| "error": (result.error or "")[:500], | ||
| }) | ||
|
|
||
| # Update run status based on agent result | ||
| if state.status == AgentStatus.COMPLETED: | ||
| complete_run(workspace, run.id) | ||
| elif state.status == AgentStatus.BLOCKED: | ||
| # Get blocker ID from state if available | ||
| blocker_id = "" | ||
| if state.blocker and hasattr(state, "_blocker_id"): | ||
| blocker_id = state._blocker_id | ||
| block_run(workspace, run.id, blocker_id) | ||
| block_run(workspace, run.id, "") | ||
| elif state.status == AgentStatus.FAILED: | ||
| fail_run(workspace, run.id) |
Contributor
There was a problem hiding this comment.
Include the failure reason in the final state transition.
result.error and exc are logged, but both failed paths call fail_run() without a reason, so the emitted RUN_FAILED event carries an empty string. Pass the adapter error / exception through so downstream event consumers keep the actual failure context.
♻️ Proposed fix
if state.status == AgentStatus.COMPLETED:
complete_run(workspace, run.id)
elif state.status == AgentStatus.BLOCKED:
block_run(workspace, run.id, "")
elif state.status == AgentStatus.FAILED:
- fail_run(workspace, run.id)
+ fail_run(workspace, run.id, (result.error or "")[:500])
@@
run_logger.error(LogCategory.ERROR, f"Unhandled error in execute_agent: {exc}", {})
try:
- fail_run(workspace, run.id)
+ fail_run(workspace, run.id, str(exc)[:500])
except Exception:
pass # Best-effort — don't mask the original errorAs per coding guidelines, codeframe/core/**/*.py: All core modules must emit events for state transitions via core/events.py for audit and observability.
Also applies to: 773-779
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/runtime.py` around lines 758 - 769, The final state transition
is dropping the failure reason—when state.status is BLOCKED or FAILED you must
pass the adapter error/exception into the state-change call so emitted
RUN_FAILED/RUN_BLOCKED events include context; update the calls to
block_run(workspace, run.id, ...) and fail_run(workspace, run.id, ...) to pass a
concise failure message (prefer result.error if present else str(exc) or
repr(exc), truncated to a safe length similar to the run_logger.error usage) and
ensure you reference state.status, result.error, exc, run_logger.error, run.id,
block_run and fail_run when making the change.
Code Review: feat(core) engine registry and runtime selection (#414)This is a clean refactor. The Bug: Blocker ID not passed to
|
This was referenced Mar 13, 2026
8 tasks
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
execute_agent()with unified adapter registry lookup viaget_builtin_adapter()/get_external_adapter()BuiltinReactAdapter, supervisor retry →BuiltinPlanAdaptercf engines listandcf engines check <name>CLI commands for engine discovery and requirement validationenginefield toEnvironmentConfigfor per-workspace default engine configurationwork_startandbatch_runcommandsKey Changes
codeframe/core/runtime.pyexecute_agent()from ~180 to ~80 lines — no per-engine branchingcodeframe/core/adapters/builtin.pycodeframe/core/engine_registry.pycheck_requirements()and_get_adapter_class()codeframe/core/config.pyenginefield toEnvironmentConfigwith validationcodeframe/cli/engines_commands.pycf engines list/checkCLI commandscodeframe/cli/app.pyTest plan
cf engines listdisplays all engines with requirement statuscf engines check reactvalidates requirements--engine reactand--engine planstill work unchangedruff checkpasses)Closes #414
Summary by CodeRabbit
New Features
Improvements
Tests