Conversation
…ntEngine interface
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — well-designed lifecycle hooks that cleanly separate engine-specific setup/cleanup from the adapter orchestration layer. The interface extension is backward-compatible (optional methods), all engines are updated consistently, and all 571 backend tests pass.
Architecture & Design
The pattern is sound: beforeExecute/afterExecute in the AgentEngine interface give each engine a clean injection point for pre/post work, while the adapter's nested try/finally ensures afterExecute always runs even on failure. The backward compatibility story for direct execute() callers (e.g., tests) is well handled — ClaudeCode removes internal cleanup entirely, Codex uses _adapterLifecycleActive to avoid double-cleanup, and LLMist simply doesn't implement the hooks.
Should Fix
- src/backends/opencode/index.ts:812 (
afterExecute) — Unlike ClaudeCode (which fully moved cleanup toafterExecute) and Codex (which uses_adapterLifecycleActiveto skip internal cleanup when the adapter lifecycle is active), OpenCode'sexecute()still callscleanupContextFilesin itsfinallyblock (line 883-885 of the full file) andafterExecutecalls it again unconditionally. This results in duplicate cleanup when called through the adapter. WhilecleanupContextFilesis idempotent and this won't cause failures, it's an inconsistency with the other engines. Consider either (a) removing theif (hasOffloadedContext)block fromexecute()'s finally, or (b) adding the_adapterLifecycleActiveguard pattern from Codex.
🕵️ claude-code · claude-opus-4-6 · run details
See PR description
🕵️ claude-code · claude-sonnet-4-6 · run details