feat(core): workspace lifecycle hooks system - #438
Merged
Conversation
Implement configurable shell hooks at 5 workspace lifecycle points:
after_init, before_task, after_task_success, after_task_failure, before_remove.
- HooksConfig dataclass integrated into EnvironmentConfig (YAML-backed)
- Jinja2 template rendering for {{task_id}}, {{task_title}}, {{task_status}}, {{workspace_path}}
- Subprocess execution with configurable timeout (default 60s)
- before_* hooks abort on failure (HookAbortError), after_* hooks log warnings
- Wired into runtime.execute_agent() and CLI init command
- CLI sub-app: cf hooks show/run/set/clear
- HOOK_EXECUTED and HOOK_FAILED event types
- 24 unit tests covering config, rendering, execution, timeouts, and abort behavior
Closes #392, #393, #394, #395, #396, #397
Contributor
WalkthroughAdds a workspace hooks system: config schema (HooksConfig), a hooks engine (rendering, subprocess execution, timeouts, results), CLI commands to manage/run hooks and register a Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI (app.py)
participant Config as EnvConfig
participant Hooks as Hooks Engine (hooks.py)
participant Subproc as Subprocess
participant Runtime as Runtime (runtime.py)
User->>CLI: cf init
CLI->>Config: load environment config
Config-->>CLI: EnvironmentConfig (with hooks)
CLI->>Hooks: execute_hook("after_init", abort_on_failure=False)
Hooks->>Hooks: render_hook_command(template, ctx)
Hooks->>Subproc: execute rendered command (with timeout)
Subproc-->>Hooks: HookResult (success/failure, stdout, stderr)
Hooks-->>CLI: HookResult
CLI->>User: display hook status
User->>CLI: submit task
CLI->>Runtime: execute_agent(task)
Runtime->>Config: load environment config
Config-->>Runtime: EnvironmentConfig (with hooks)
Runtime->>Hooks: execute_hook("before_task", abort_on_failure=True)
Hooks->>Subproc: execute rendered command
alt before_task succeeds
Subproc-->>Runtime: success
Runtime->>Runtime: run task
else before_task fails
Subproc-->>Runtime: failure (HookAbortError)
Runtime->>User: emit HOOK_FAILED, abort run
end
alt task succeeds
Runtime->>Hooks: execute_hook("after_task_success", abort_on_failure=False)
else task fails
Runtime->>Hooks: execute_hook("after_task_failure", abort_on_failure=False)
end
Hooks->>Subproc: execute post-task command
Subproc-->>Hooks: HookResult
Hooks-->>Runtime: result (non-blocking)
Runtime->>User: emit HOOK_EXECUTED or HOOK_FAILED
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
📝 Coding Plan
Comment |
|
Review: feat(core): workspace lifecycle hooks system Clean implementation overall. The design is well-aligned with CodeFrame architecture. A few things worth addressing, roughly in priority order. Security: shell=True with user-controlled template variables File: codeframe/core/hooks.py in run_hook() Hook commands are rendered with Jinja2, then passed to subprocess.run with shell=True. The task_title field comes from task records that users write. A task titled with shell metacharacters (e.g. Fix bug; rm -rf /) would result in that command executing silently when the hook fires. In a developer CLI tool this is lower-severity than in a multi-tenant server, but it is worth either escaping template variable values with shlex.quote() before interpolation, or documenting the risk prominently. At minimum, add a note in the docstring so users understand the trust model. Inconsistency: hooks_set vs hooks_clear config handling File: codeframe/cli/hooks_commands.py hooks_set does load_environment_config(path) or get_default_environment_config() - it creates config if missing. hooks_clear does load_environment_config(path) with an error exit if missing. This asymmetry is confusing: hooks set works outside a workspace, hooks clear does not. Since clearing a nonexistent config is a no-op, both should probably require existing config (or both should tolerate missing config). The former seems more correct. No hook for BLOCKED status The after-task hooks cover COMPLETED and FAILED but not BLOCKED. Teams that use blockers (e.g., to notify Slack or update a ticket tracker) have no hook point for that transition. Not a blocker for this PR since it matches the spec in issue 392, but worth a follow-up issue. Minor: hook_ctx mutated in-place File: codeframe/core/runtime.py HookContext is a plain mutable dataclass, and task_status is patched directly after creation before the after-task hooks run. Works fine since hook_ctx is function-local, but reads as surprising. Small readability improvement, not a bug. What is working well
The shell=True escaping concern is the only thing worth addressing before merge. The rest are polish items. |
![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: 7
🤖 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 153-171: The after_init hook invocation (using
load_environment_config, HookContext and execute_hook) is currently run
unconditionally in CLI and should be moved into the
first-time-init/workspace-creation flow so it only executes on fresh
initialization (preserve idempotency); relocate this block into the code path
that creates the workspace (or into the workspace-creation layer) rather than
the top-level init CLI path, and after running execute_hook emit the appropriate
audit event constants HOOK_EXECUTED or HOOK_FAILED (instead of only printing) so
the hook execution is recorded by the audit system.
In `@codeframe/cli/hooks_commands.py`:
- Around line 44-45: The code uses workspace_path or Path.cwd() directly when
calling load_environment_config, which causes handlers to operate in
subdirectories; fix by resolving the true workspace root before any read/write:
compute workspace_root = resolve_workspace_root(workspace_path or Path.cwd())
(or call the existing workspace root resolver used elsewhere) and pass
workspace_root to load_environment_config and any config write operations; apply
this change in hooks_show, hooks_run, hooks_clear and the hooks set/clear
handlers so all uses of load_environment_config and config writes use the
resolved workspace_root instead of the raw workspace_path/Path.cwd().
In `@codeframe/core/config.py`:
- Around line 84-97: Add validation to HooksConfig to reject non-positive
timeouts at config load time: implement a __post_init__ method on the
HooksConfig dataclass that checks if hook_timeout is an int > 0 and raises a
ValueError (or ConfigError if the project has one) with a clear message when
hook_timeout <= 0, so invalid YAML values are surfaced immediately when
HooksConfig is instantiated.
In `@codeframe/core/hooks.py`:
- Around line 154-169: The current execute_hook flow calls run_hook(hook_name,
...) but doesn't guard against exceptions from starting or running the hook, so
non-blocking hooks can still crash the caller; wrap the run_hook call in a
try/except that catches all exceptions, and on exception either raise
HookAbortError(hook_name, err) if abort_on_failure is true or log a warning via
logger.warning including hook_name and the exception details (and set a
failure-like result or return None as the existing API expects); update
references to run_hook, hook_name, abort_on_failure, HookAbortError and
logger.warning in this block so unexpected exceptions are treated the same as
non-zero exits for non-blocking hooks.
In `@codeframe/core/runtime.py`:
- Around line 795-813: The after_task hooks are being executed before the
run/task final state is persisted; update the logic so execute_hook(...) for
"after_task_success" / "after_task_failure" is invoked only after the run's
final state is saved (i.e., after calls that persist completion such as
complete_run() or fail_run()), and then emit events via
events.emit_for_workspace; locate the current block around execute_hook,
AgentStatus checks, and events.emit_for_workspace and move or defer that block
to the post-persistence/cleanup path so hooks and HOOK_EXECUTED/HOOK_FAILED are
observed after RUN_COMPLETED/RUN_FAILED.
In `@tests/core/test_hooks.py`:
- Around line 235-242: The test test_uses_hook_timeout_from_config currently
uses a broad pytest.raises(Exception); change it to assert the specific abort
type by importing HookAbortError from codeframe.core.hooks and using
pytest.raises(HookAbortError) around the execute_hook call so the test verifies
the timeout path raises HookAbortError (reference:
test_uses_hook_timeout_from_config, execute_hook, HookAbortError).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7238d72-c836-4e53-bb60-308488582f7f
📒 Files selected for processing (7)
codeframe/cli/app.pycodeframe/cli/hooks_commands.pycodeframe/core/config.pycodeframe/core/events.pycodeframe/core/hooks.pycodeframe/core/runtime.pytests/core/test_hooks.py
Comment on lines
+153
to
+171
| # Execute after_init hook (non-blocking) | ||
| from codeframe.core.config import load_environment_config | ||
| from codeframe.core.hooks import HookContext, execute_hook | ||
| env_config = load_environment_config(repo_path) | ||
| if env_config: | ||
| hook_ctx = HookContext( | ||
| task_id="", task_title="", task_status="init", | ||
| workspace_path=str(repo_path), | ||
| ) | ||
| hook_result = execute_hook( | ||
| "after_init", env_config, repo_path, hook_ctx, | ||
| abort_on_failure=False, | ||
| ) | ||
| if hook_result: | ||
| if hook_result.success: | ||
| console.print(f" Hook after_init: [green]OK[/green] ({hook_result.duration_ms}ms)") | ||
| else: | ||
| console.print(f" Hook after_init: [yellow]failed[/yellow] ({hook_result.stderr[:100]})") | ||
|
|
Contributor
There was a problem hiding this comment.
after_init is wired at the wrong lifecycle point.
This block runs on both fresh init and “already initialized” flows, so a configured after_init hook will rerun every time someone calls codeframe init, breaking the command’s idempotency guarantee. Because it lives entirely in the CLI layer, this path also bypasses the new HOOK_EXECUTED / HOOK_FAILED audit events. Move it behind the first-time-init path (or into the workspace-creation layer) and emit the hook event there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/cli/app.py` around lines 153 - 171, The after_init hook invocation
(using load_environment_config, HookContext and execute_hook) is currently run
unconditionally in CLI and should be moved into the
first-time-init/workspace-creation flow so it only executes on fresh
initialization (preserve idempotency); relocate this block into the code path
that creates the workspace (or into the workspace-creation layer) rather than
the top-level init CLI path, and after running execute_hook emit the appropriate
audit event constants HOOK_EXECUTED or HOOK_FAILED (instead of only printing) so
the hook execution is recorded by the audit system.
Comment on lines
+84
to
+97
| @dataclass | ||
| class HooksConfig: | ||
| """Workspace lifecycle hooks configuration. | ||
|
|
||
| Hooks are shell commands executed at specific lifecycle points. | ||
| Template variables (e.g., {{task_id}}) are rendered via Jinja2. | ||
| """ | ||
|
|
||
| after_init: Optional[str] = None | ||
| before_task: Optional[str] = None | ||
| after_task_success: Optional[str] = None | ||
| after_task_failure: Optional[str] = None | ||
| before_remove: Optional[str] = None | ||
| hook_timeout: int = 60 |
Contributor
There was a problem hiding this comment.
Validate hook_timeout when loading config.
HooksConfig currently accepts 0 or negative values, so a bad YAML value is only discovered later when hooks run. This should be rejected up front with the rest of the environment config.
🛠️ Suggested fix
`@dataclass`
class HooksConfig:
@@
after_task_failure: Optional[str] = None
before_remove: Optional[str] = None
hook_timeout: int = 60
+
+ def __post_init__(self) -> None:
+ if self.hook_timeout <= 0:
+ raise ValueError("hooks.hook_timeout must be > 0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/config.py` around lines 84 - 97, Add validation to HooksConfig
to reject non-positive timeouts at config load time: implement a __post_init__
method on the HooksConfig dataclass that checks if hook_timeout is an int > 0
and raises a ValueError (or ConfigError if the project has one) with a clear
message when hook_timeout <= 0, so invalid YAML values are surfaced immediately
when HooksConfig is instantiated.
- Shell-escape template variables via shlex.quote() to prevent injection - Wrap non-blocking hook execution in try/except for robustness - Move after_task hooks after state persistence (complete_run/fail_run) - Only run after_init hook on fresh workspace init (not re-init) - Resolve workspace root in CLI hooks commands via get_workspace - Use HookAbortError in timeout test assertion (not generic Exception) - Add shell injection prevention test
Follow-up Review Reviewed the updated commit (da65de5). Tracking status on items from my first pass. --- ### Primary concern resolved: shell=True injection shlex.quote() is now applied to all four template variables in render_hook_command(), and the docstring explicitly documents the trust model. The test_shell_escapes_values test covers the injection case. One note on that test: the assertion is technically correct but reads awkwardly. With shlex.quote, the payload will always contain a single-quote (the wrapping), so this passes vacuously on the second condition. A tighter form like asserting the raw payload substrings are not executable would be easier to follow, but it is not blocking. --- ### Still open: hooks_set / hooks_clear asymmetry Not addressed. hooks_set creates a default config when none exists; hooks_clear exits 1 if config is missing. The inconsistency is confusing. cf hooks clear before_task in a fresh workspace fails with an error, while cf hooks set before_task succeeds. Both should probably require an existing workspace config (clearing a nonexistent hook is a no-op, so the error is misleading). Easy fix, not blocking. --- ### CodeRabbit points worth flagging HooksConfig.hook_timeout validation: No guard against hook_timeout <= 0. A user who sets hook_timeout: 0 or -1 in YAML would get surprising behavior (zero timeout immediately expires; negative timeout behavior is Python-version-dependent). A one-line post_init guard is worthwhile before merge. After-task hooks timing: CodeRabbit flagged that after-task hooks fire before final state is persisted. Looking at the diff, complete_run()/fail_run() are called first, then after-task hooks run. The ordering is intentional and correct. CodeRabbit concern does not apply here. Exception handling in execute_hook: The try/except Exception around run_hook handles non-blocking paths correctly; the blocking path re-raises via bare raise. This is fine as-is. --- ### Summary The primary security concern from my first review is resolved. The only pre-merge fix I would recommend is a hook_timeout <= 0 validation in HooksConfig.post_init. The hooks_set/hooks_clear asymmetry is worth a follow-up issue. Everything else is in good shape. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
codeframe/cli/app.py (1)
153-171:⚠️ Potential issue | 🟠 MajorEmit audit events for
after_inithook outcomes.This path only prints the result. Without
HOOK_EXECUTED/HOOK_FAILED, init-time hook runs never appear in the workspace event log, even though the runtime hook paths are audited.🛠️ Suggested fix
if env_config and not already_existed: hook_ctx = HookContext( task_id="", task_title="", task_status="init", workspace_path=str(repo_path), ) hook_result = execute_hook( "after_init", env_config, repo_path, hook_ctx, abort_on_failure=False, ) if hook_result: + event_type = ( + EventType.HOOK_EXECUTED + if hook_result.success + else EventType.HOOK_FAILED + ) + emit_for_workspace( + workspace, + event_type, + {"hook": "after_init", "success": hook_result.success}, + print_event=False, + ) if hook_result.success: console.print(f" Hook after_init: [green]OK[/green] ({hook_result.duration_ms}ms)") else: console.print(f" Hook after_init: [yellow]failed[/yellow] ({hook_result.stderr[:100]})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/cli/app.py` around lines 153 - 171, The after_init hook result is only printed and not recorded; update the block that calls load_environment_config, creates HookContext and calls execute_hook("after_init", ...) so that when hook_result is returned you also emit an audit event: on success emit HOOK_EXECUTED with fields (hook_name="after_init", workspace_path=str(repo_path), task_status=hook_ctx.task_status, duration_ms=hook_result.duration_ms) and on failure emit HOOK_FAILED with (hook_name="after_init", workspace_path=str(repo_path), task_status=hook_ctx.task_status, duration_ms=hook_result.duration_ms, error=hook_result.stderr[:100]); use the existing audit/emitter API in the repo/workspace layer (call it where other runtime hook paths emit events) and keep console.prints intact.
🤖 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/hooks_commands.py`:
- Around line 44-50: Currently the code swallows all exceptions around
get_workspace(path); change the try/except to only catch the specific "workspace
not found" exception (import the actual exception class from
codeframe.core.workspace, e.g., WorkspaceNotFoundError or the module's
equivalent) and let any other exceptions propagate (or re-raise them) so
DB/schema errors aren't hidden; update the blocks that call get_workspace (the
handlers that use workspace_path, get_workspace, and ws.repo_path — the four
occurrences used by the "hooks set" and "hooks clear" flows) to use this
narrowed except pattern consistently.
- Around line 121-131: The command currently prints hook failures but still
exits 0; update the handler that iterates hook results (the block using
result.success, result.timed_out, result.stdout, result.stderr and the hook_name
variable) to cause a non-zero exit when any hook fails: either set a local
failure flag (e.g., exit_code = 1) when result.success is False and after
processing all hooks call sys.exit(exit_code), or directly call sys.exit(1)
immediately after printing the failure message; remember to import sys if not
already imported. Ensure the change uses the existing result.success and
result.timed_out checks so timed-out failures also produce a non-zero exit.
In `@codeframe/core/hooks.py`:
- Around line 163-172: The except block for run_hook should raise HookAbortError
(with the original exception chained) when abort_on_failure is True instead of
re-raising the raw exception; update the except in the function that calls
run_hook to import/use HookAbortError and raise HookAbortError(...) from exc so
callers (e.g., before_task in runtime.py) can catch the expected type, while
keeping the non-blocking path returning HookResult as currently implemented.
In `@codeframe/core/runtime.py`:
- Around line 803-820: The current after-task hook dispatch (using execute_hook
with "after_task_success" / "after_task_failure") only runs in the normal result
path and is skipped when the run raises an exception; modify the control flow so
the same hook dispatch logic is invoked for failure cases from both the normal
return path and any exception path: ensure that when state.status ==
AgentStatus.FAILED (or when the exception handler sets state.status/
hook_ctx.task_status), you call execute_hook("after_task_failure", env_config,
workspace.repo_path, hook_ctx, abort_on_failure=False) and emit the matching
events.emit_for_workspace(workspace, events.EventType.HOOK_EXECUTED or
HOOK_FAILED, {"hook": "after_task_failure", "success": hook_result.success})
from the exception handling/finally block (reusing execute_hook, after_hook,
hook_ctx, env_config, workspace, state symbols) so failures raised by the
adapter still trigger the after_task_failure hook and its audit event.
- Around line 661-691: The pre-hook block (load_environment_config, execute_hook
and HookAbortError handling) is currently outside the main try/finally in
execute_agent, so exceptions other than HookAbortError can bypass fail_run() and
the output_logger cleanup; move the entire block that builds HookContext and
calls load_environment_config/execute_hook (and its HookAbortError catch that
calls fail_run and returns AgentState) into the main try that wraps the run
execution so that any exception will still hit the finally where output_logger
is closed and fail_run() is guaranteed to be called; keep the same
HookAbortError-specific handling logic (emitting HOOK_FAILED and returning
AgentState) but ensure the initial env_config load/execute_hook invocation lives
inside execute_agent’s primary try/finally.
---
Duplicate comments:
In `@codeframe/cli/app.py`:
- Around line 153-171: The after_init hook result is only printed and not
recorded; update the block that calls load_environment_config, creates
HookContext and calls execute_hook("after_init", ...) so that when hook_result
is returned you also emit an audit event: on success emit HOOK_EXECUTED with
fields (hook_name="after_init", workspace_path=str(repo_path),
task_status=hook_ctx.task_status, duration_ms=hook_result.duration_ms) and on
failure emit HOOK_FAILED with (hook_name="after_init",
workspace_path=str(repo_path), task_status=hook_ctx.task_status,
duration_ms=hook_result.duration_ms, error=hook_result.stderr[:100]); use the
existing audit/emitter API in the repo/workspace layer (call it where other
runtime hook paths emit events) and keep console.prints intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9c2dc3f-ff8c-41af-ab3e-8456230540f0
📒 Files selected for processing (5)
codeframe/cli/app.pycodeframe/cli/hooks_commands.pycodeframe/core/hooks.pycodeframe/core/runtime.pytests/core/test_hooks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/test_hooks.py
Comment on lines
+44
to
+50
| from codeframe.core.workspace import get_workspace | ||
| path = workspace_path or Path.cwd() | ||
| try: | ||
| ws = get_workspace(path) | ||
| path = ws.repo_path | ||
| except Exception: | ||
| pass |
Contributor
There was a problem hiding this comment.
Only treat “workspace not found” as a fallback case.
These except Exception: pass blocks also swallow real get_workspace() failures such as schema/DB errors. In hooks set and hooks clear, that can silently fall back to the raw path and read/write a different .codeframe/config.yaml instead of surfacing the broken workspace.
🛠️ Suggested fix
- except Exception:
+ except FileNotFoundError:
passApply the same narrowing to all four handlers.
Also applies to: 95-101, 157-161, 191-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/cli/hooks_commands.py` around lines 44 - 50, Currently the code
swallows all exceptions around get_workspace(path); change the try/except to
only catch the specific "workspace not found" exception (import the actual
exception class from codeframe.core.workspace, e.g., WorkspaceNotFoundError or
the module's equivalent) and let any other exceptions propagate (or re-raise
them) so DB/schema errors aren't hidden; update the blocks that call
get_workspace (the handlers that use workspace_path, get_workspace, and
ws.repo_path — the four occurrences used by the "hooks set" and "hooks clear"
flows) to use this narrowed except pattern consistently.
Comment on lines
+163
to
+172
| try: | ||
| result = run_hook(hook_name, command, workspace_path, ctx, config.hooks.hook_timeout) | ||
| except Exception as exc: | ||
| if abort_on_failure: | ||
| raise | ||
| logger.warning("Hook '%s' raised unexpected error (non-blocking): %s", hook_name, exc) | ||
| return HookResult( | ||
| hook_name=hook_name, command=command, success=False, | ||
| stdout="", stderr=str(exc), duration_ms=0, timed_out=False, | ||
| ) |
Contributor
There was a problem hiding this comment.
Raise HookAbortError for unexpected blocking-hook failures.
When abort_on_failure=True, Line 167 re-raises raw exceptions from rendering or process startup. That breaks this function’s documented contract and bypasses callers that only handle HookAbortError, like the before_task path in codeframe/core/runtime.py.
🛠️ Suggested fix
try:
result = run_hook(hook_name, command, workspace_path, ctx, config.hooks.hook_timeout)
except Exception as exc:
- if abort_on_failure:
- raise
- logger.warning("Hook '%s' raised unexpected error (non-blocking): %s", hook_name, exc)
- return HookResult(
- hook_name=hook_name, command=command, success=False,
- stdout="", stderr=str(exc), duration_ms=0, timed_out=False,
- )
+ result = HookResult(
+ hook_name=hook_name,
+ command=command,
+ success=False,
+ stdout="",
+ stderr=f"{type(exc).__name__}: {exc}",
+ duration_ms=0,
+ timed_out=False,
+ )
+ if abort_on_failure:
+ raise HookAbortError(hook_name, result) from exc
+ logger.warning("Hook '%s' raised unexpected error (non-blocking): %s", hook_name, exc)
+ return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/hooks.py` around lines 163 - 172, The except block for
run_hook should raise HookAbortError (with the original exception chained) when
abort_on_failure is True instead of re-raising the raw exception; update the
except in the function that calls run_hook to import/use HookAbortError and
raise HookAbortError(...) from exc so callers (e.g., before_task in runtime.py)
can catch the expected type, while keeping the non-blocking path returning
HookResult as currently implemented.
- Move before_task hook inside main try block (prevents orphaned runs) - Fire after_task_failure hook in unhandled exception handler - Exit non-zero from 'cf hooks run' on hook failure - Narrow exception handling in hooks CLI to (FileNotFoundError, ValueError)
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
codeframe/core/runtime.py (2)
833-839:⚠️ Potential issue | 🟠 MajorUnhandled-exception hook path is missing hook outcome events and isolation.
On Line 833,
after_task_failureis invoked but its outcome is not emitted (HOOK_EXECUTED/HOOK_FAILED), and an unexpected hook error here can override the original failure path.Suggested fix
# Fire after_task_failure hook even on unhandled exceptions if env_config and hook_ctx: hook_ctx.task_status = "failed" - execute_hook( - "after_task_failure", env_config, workspace.repo_path, hook_ctx, - abort_on_failure=False, - ) + try: + hook_result = execute_hook( + "after_task_failure", env_config, workspace.repo_path, hook_ctx, + abort_on_failure=False, + ) + if hook_result: + evt = ( + events.EventType.HOOK_EXECUTED + if hook_result.success + else events.EventType.HOOK_FAILED + ) + events.emit_for_workspace( + workspace, evt, {"hook": "after_task_failure", "success": hook_result.success} + ) + except Exception as hook_exc: + events.emit_for_workspace( + workspace, + events.EventType.HOOK_FAILED, + {"hook": "after_task_failure", "error": str(hook_exc)}, + )As per coding guidelines, “All core modules must emit events for state transitions via
core/events.pyfor audit and observability”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/runtime.py` around lines 833 - 839, The after_task_failure hook call in the code does not emit outcome events and may let hook errors override the original failure; update the block around execute_hook("after_task_failure", ...) to run the hook inside its own try/except so any exception is caught and does not re-raise, and emit the proper events via the core/events.py API (emit_event or equivalent) for success (HOOK_EXECUTED) and failure (HOOK_FAILED) including relevant context from hook_ctx, env_config and workspace.repo_path; ensure hook errors are logged but do not change the original task failure path.
661-674:⚠️ Potential issue | 🔴 CriticalMove pre-hook config/context initialization inside the main
try.On Line 661, config/context loading still runs before the outer
try; if it raises,output_logger.close()(Line 844) and run failure handling are skipped.Suggested fix
- # Load hook config (before main try block so it's available everywhere) - from codeframe.core.config import load_environment_config - from codeframe.core.hooks import HookAbortError, HookContext, execute_hook - env_config = load_environment_config(workspace.repo_path) - hook_ctx: HookContext | None = None - if env_config: - task_record = tasks.get(workspace, run.task_id) - hook_ctx = HookContext( - task_id=run.task_id, - task_title=task_record.title if task_record else "", - task_status="in_progress", - workspace_path=str(workspace.repo_path), - ) + from codeframe.core.config import load_environment_config + from codeframe.core.hooks import HookAbortError, HookContext, execute_hook try: + env_config = load_environment_config(workspace.repo_path) + hook_ctx: HookContext | None = None + if env_config: + task_record = tasks.get(workspace, run.task_id) + hook_ctx = HookContext( + task_id=run.task_id, + task_title=task_record.title if task_record else "", + task_status="in_progress", + workspace_path=str(workspace.repo_path), + )🤖 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 - 674, The environment config and hook context creation (use of load_environment_config, HookContext, HookAbortError, execute_hook, and assignment to hook_ctx/env_config) must be moved inside the main try block so any exceptions trigger the existing failure handling and output_logger.close(); keep a pre-declaration hook_ctx: HookContext | None = None before the try for scope, then inside the try perform the imports (if needed), call env_config = load_environment_config(workspace.repo_path) and construct HookContext (task_id=run.task_id, task_title=..., task_status="in_progress", workspace_path=str(workspace.repo_path)) only when env_config is truthy; remove the current pre-try block so errors are caught by the outer try.codeframe/cli/hooks_commands.py (1)
45-51:⚠️ Potential issue | 🟠 MajorResolve workspace root from subdirectories and only fallback on “not found”.
At Line 45 / Line 96 / Line 159 / Line 193, resolving only
workspace_path or Path.cwd()means running fromrepo/subdircan load/saverepo/subdir/.codeframe/config.yamlinstead of the workspace root. Also, catchingValueErrorstill hides real workspace-data errors and should not silently fall back.Suggested fix
from pathlib import Path from typing import Optional @@ VALID_HOOK_NAMES = [ @@ ] + + +def _resolve_workspace_root(workspace_path: Optional[Path]) -> Path: + from codeframe.core.workspace import get_workspace + + current = (workspace_path or Path.cwd()).resolve() + for candidate in (current, *current.parents): + try: + return get_workspace(candidate).repo_path + except FileNotFoundError: + continue + return current @@ - from codeframe.core.workspace import get_workspace - path = workspace_path or Path.cwd() - try: - ws = get_workspace(path) - path = ws.repo_path - except (FileNotFoundError, ValueError): - pass # Workspace not initialized; fall back to raw path + path = _resolve_workspace_root(workspace_path) @@ - from codeframe.core.workspace import get_workspace - path = workspace_path or Path.cwd() - try: - ws = get_workspace(path) - path = ws.repo_path - except (FileNotFoundError, ValueError): - pass # Workspace not initialized; fall back to raw path + path = _resolve_workspace_root(workspace_path) @@ - from codeframe.core.workspace import get_workspace - path = workspace_path or Path.cwd() - try: - ws = get_workspace(path) - path = ws.repo_path - except (FileNotFoundError, ValueError): - pass # Workspace not initialized; fall back to raw path + path = _resolve_workspace_root(workspace_path) @@ - from codeframe.core.workspace import get_workspace - path = workspace_path or Path.cwd() - try: - ws = get_workspace(path) - path = ws.repo_path - except (FileNotFoundError, ValueError): - pass # Workspace not initialized; fall back to raw path + path = _resolve_workspace_root(workspace_path)Also applies to: 96-103, 159-166, 193-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/cli/hooks_commands.py` around lines 45 - 51, The code currently sets path = workspace_path or Path.cwd() and calls get_workspace(path) while catching both FileNotFoundError and ValueError, which lets runs from subdirectories load a subdir config and silences real workspace errors; change it so you determine the workspace root by calling get_workspace(start_path) where start_path is workspace_path or Path.cwd(), and only fall back to the original path when get_workspace raises FileNotFoundError (do not catch ValueError); then set path = ws.repo_path when get_workspace succeeds and call load_environment_config(path); apply this same fix at the other occurrences that call get_workspace/load_environment_config (the blocks around lines referencing get_workspace at the other three sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@codeframe/cli/hooks_commands.py`:
- Around line 45-51: The code currently sets path = workspace_path or Path.cwd()
and calls get_workspace(path) while catching both FileNotFoundError and
ValueError, which lets runs from subdirectories load a subdir config and
silences real workspace errors; change it so you determine the workspace root by
calling get_workspace(start_path) where start_path is workspace_path or
Path.cwd(), and only fall back to the original path when get_workspace raises
FileNotFoundError (do not catch ValueError); then set path = ws.repo_path when
get_workspace succeeds and call load_environment_config(path); apply this same
fix at the other occurrences that call get_workspace/load_environment_config
(the blocks around lines referencing get_workspace at the other three sites).
In `@codeframe/core/runtime.py`:
- Around line 833-839: The after_task_failure hook call in the code does not
emit outcome events and may let hook errors override the original failure;
update the block around execute_hook("after_task_failure", ...) to run the hook
inside its own try/except so any exception is caught and does not re-raise, and
emit the proper events via the core/events.py API (emit_event or equivalent) for
success (HOOK_EXECUTED) and failure (HOOK_FAILED) including relevant context
from hook_ctx, env_config and workspace.repo_path; ensure hook errors are logged
but do not change the original task failure path.
- Around line 661-674: The environment config and hook context creation (use of
load_environment_config, HookContext, HookAbortError, execute_hook, and
assignment to hook_ctx/env_config) must be moved inside the main try block so
any exceptions trigger the existing failure handling and output_logger.close();
keep a pre-declaration hook_ctx: HookContext | None = None before the try for
scope, then inside the try perform the imports (if needed), call env_config =
load_environment_config(workspace.repo_path) and construct HookContext
(task_id=run.task_id, task_title=..., task_status="in_progress",
workspace_path=str(workspace.repo_path)) only when env_config is truthy; remove
the current pre-try block so errors are caught by the outer try.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31df1231-af81-43bc-8c63-3e2f05b04f80
📒 Files selected for processing (2)
codeframe/cli/hooks_commands.pycodeframe/core/runtime.py
|
Third-pass review (latest commit: 20ce21a) Tracking open items from previous reviews and assessing the new changes to hook_timeout <= 0 validation -- still not addressed. A user who sets def __post_init__(self) -> None:
if self.hook_timeout <= 0:
raise ValueError(f"hook_timeout must be > 0, got {self.hook_timeout}")hooks_set / hooks_clear asymmetry -- still not addressed. New in 20ce21a execute_hook exception path: bare In except Exception as exc:
if abort_on_failure:
dummy_result = HookResult(hook_name=hook_name, command=command,
success=False, stdout="", stderr=str(exc), duration_ms=0, timed_out=False)
raise HookAbortError(hook_name, dummy_result) from excafter_task_failure in exception path: hook fires but event is not emitted In Summary
The |
This was referenced Mar 13, 2026
5 tasks
4 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
Implements #392: Workspace Lifecycle Hooks System (+ sub-issues #393-#397)
Adds configurable shell hooks at 5 workspace lifecycle points, inspired by the Symphony spec. Teams can inject custom behavior (dependency installation, branch management, cleanup) without hardcoding it into CodeFrame's core.
.codeframe/config.yaml){{task_id}},{{task_title}},{{task_status}},{{workspace_path}}HookAbortError), after_ hooks log warnings* (non-blocking)execute_agent()cf initcf hooks show/run/set/clearfor hook managementAcceptance Criteria
.codeframe/config.yaml)before_*points abort the operationafter_*points are logged but non-blockingTest Plan
Implementation Notes
hooks_commands.py(followingengines_commands.pypattern) rather than adding toapp.pyconductor.pyspawnscf work startas subprocess, so hooks inruntime.execute_agent()fire automatically — no conductor changes neededcf workspace removecommand exists yet; the hook point is defined for future usefrom_dict()handles missinghookskey gracefully (defaults to all None)Closes #392, #393, #394, #395, #396, #397
Summary by CodeRabbit