refactor(hooks): drop run_effect — deciders are pure inline logic#61
Merged
Conversation
run_effect baked in an in-process execution model: DirectExecutor and DBOS both run the effect in the loop's own process, so a bare closure is always reachable. A dispatch-to-pool durable runtime (Temporal) can't honor it — an activity's input must be serializable and a closure can't cross that boundary; the only "implementation" drags execution back in-process (local activity + co-located registry), defeating the runtime's own model. When the sole way a durable executor can satisfy a seam is to subvert its distribution, the seam encodes the wrong assumption. It was also unused: zero production callers (only tests). The real decider hooks (cycle-id stamping, search-budget veto, finish nudge) are pure, deterministic logic — read run_context/messages, return a decision — and run inline, replay-safe, like the existing on_pre_tool/on_before_finish callbacks. The one production hook that does durable I/O (per-chat usage recording) already dispatches a named activity directly, the one durability primitive every executor implements natively. So: remove run_effect from HookContext, the loop's _make_hook_context, and DirectExecutor; drop passthrough_effect. HookContext stays read-only (run_context + messages). Durable side effects, if ever needed, belong in an activity/step the consumer's hook dispatches — not a framework seam only in-process executors can honor.
There was a problem hiding this comment.
Pull request overview
This PR removes the run_effect seam from the agent lifecycle hook system so hook deciders remain pure, inline, and replay-safe by construction (with durable I/O expected to be dispatched via executor-native activities/steps instead of closures).
Changes:
- Remove
run_effect(and the inlinepassthrough_effectfallback) fromHookContextand stop wiring it inAgentLoop. - Drop
DirectExecutor.run_effectfrom the core executor implementation. - Update CPython and MicroPython tests to stop asserting side-effect dispatch via hooks and keep only deterministic decider behavior checks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
exoclaw/agent/hooks.py |
Removes run_effect from HookContext (both CPython/MP branches) and updates hook contract documentation to emphasize pure deciders. |
exoclaw/agent/loop.py |
Stops importing/wiring passthrough_effect and builds a simpler, read-only HookContext. |
exoclaw/executor.py |
Removes DirectExecutor.run_effect implementation. |
tests/test_hooks.py |
Removes passthrough_effect/run_effect tests and updates hook contract test descriptions accordingly. |
tests/micro/test_agent_loop_paths.py |
Updates MicroPython hook-path tests to only validate run_context stamping (no run_effect). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
323
to
332
| @@ -334,10 +332,6 @@ def _make_hook_context(self, event: str, **fields: Any) -> HookContext: | |||
| except Exception: | |||
Address Copilot on #61. HookContext is documented read-only, but _make_hook_context passed Conversation.run_context()'s dict through by reference — a decider mutating ctx.run_context would touch the conversation's bag and leak into later seams. Hand it a shallow copy (same posture as the per-dict messages copy). Test pins that the source bag is untouched.
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
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
run_effect(added in 0.30.0) is the wrong seam, and unused.Wrong seam: it baked in an in-process execution model.
DirectExecutorand DBOS both run the effect in the loop's own process, so a bare closure is always reachable. A dispatch-to-pool durable runtime — Temporal — can't honor it: an activity's input must be serializable and a Go/Python closure can't cross that boundary. The only "implementation" drags execution back in-process (local activity + a co-located thunk registry), which defeats Temporal's distribution model. When the sole way a durable executor can satisfy a seam is to subvert its own runtime, the seam encodes the wrong assumption.Unused: zero production callers — every
run_effect()call site was a test. The real decider hooks (cycle-id stamping, search-budget veto, finish-cycle nudge) are pure, deterministic logic — readrun_context/messages, return a decision — and run inline, replay-safe, exactly like the existingon_pre_tool/on_before_finishcallbacks. The one production hook that does durable I/O (per-chat token-usage recording in hey_lefty) already dispatches a named activity directly — the one durability primitive every executor implements natively.Changes
agent/hooks.py: removerun_effectfromHookContext(both dual-class branches); deletepassthrough_effect; docstring now frames a decider as pure inline logic, with durable I/O delegated to an activity/step the consumer's hook dispatches.agent/loop.py:_make_hook_contextno longer wiresrun_effect; drop thepassthrough_effectimport.executor.py: removeDirectExecutor.run_effect.test_passthrough_effect_runs_inline+test_loop_before_tool_decider_can_run_effect; the MP…stamps_and_runs_effecttest becomes…stamps_from_run_context(keeps the stamp assertion, drops the effect).HookContextstays read-only (run_context+messages) plus the decision return.Test plan
ruff check+ruff format --checkcleanty check exoclawclean (only the pre-existingloop.pyunused-ignore)Follow-ups (separate)
RunEffect/RunEffecterfrom the unpushed 0.30.x sync (deciders run inline).run_effectDBOS durability test (keep theon_before_finishone).