feat(backends): add NativeToolEngine abstract base class for subprocess engines#1019
feat(backends): add NativeToolEngine abstract base class for subprocess engines#1019
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured abstract base class that correctly consolidates shared native-tool engine patterns. All CI checks pass, type checking is clean, and the 19 unit tests cover all shared methods well. No blocking issues.
Should Fix
-
src/backends/shared/NativeToolEngine.ts:5 — Doc claims the class provides "System/task prompt building via buildSystemPrompt / buildTaskPrompt" but the class does not actually expose or delegate to these functions — they remain standalone imports from
nativeToolPrompts.tsthat each engine calls directly in its ownexecute(). Consider removing this bullet (it describes a codebase pattern, not a class responsibility) or, if a future story will move prompt building into the base class, noting that explicitly. -
src/backends/shared/NativeToolEngine.ts:15 — Doc says subclasses must implement
executeTurn()but the actual abstract method isexecute()(line 108). This looks like a leftover from an earlier design where the base class would own the continuation loop and delegate single turns to subclasses. Please update to match the actual contract.
🕵️ claude-code · claude-opus-4-6 · run details
Remove the erroneous bullet about buildSystemPrompt/buildTaskPrompt (those functions are standalone imports used by each engine's own execute(), not a base class responsibility). Also correct the abstract method name from executeTurn() to execute() to match the actual contract. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — clean, well-designed abstract base class that correctly consolidates the shared subprocess engine patterns.
The NativeToolEngine accurately captures the common surface across Claude Code, Codex, and OpenCode engines:
buildEnv()delegates tobuildEngineEnvwith the same signature pattern used by the existing per-engine env builders (e.g.,codex/env.ts:buildEnv,claude-code/env.ts:buildClaudeEnv). TheallowedEnvExactparameter correctly expects only engine-specific vars sincebuildEngineEnvalready mergesSHARED_ALLOWED_ENV_EXACTinternally.afterExecute()implements the shared minimum (cleanupContextFiles), leaving space for subclasses to override and callsuper.afterExecute()for engine-specific cleanup (persisted sessions, token capture, etc.).resolveModel()→resolveEngineModel()delegation is a clean template-method pattern that satisfies theAgentEnginecontract without forcing subclasses to know about the interface method name.beforeExecuteandgetSettingsSchemaare correctly omitted from the base class since not all engines implement them (OpenCode has nobeforeExecute).
Interface conformance is correct — required AgentEngine members are covered (abstract definition, abstract execute, concrete supportsAgentType), and optional members (resolveModel, afterExecute) are implemented where the shared pattern applies.
Tests are thorough (19 tests covering all shared methods via StubEngine), mocks are clean, and CI is fully green.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
NativeToolEngineabstract base class insrc/backends/shared/NativeToolEngine.tsthat consolidates shared patterns from Claude Code, Codex, and OpenCode enginessrc/backends/shared/index.tsand re-exported fromsrc/backends/index.tstests/unit/backends/NativeToolEngine.test.tswith aStubEngine extends NativeToolEngineWhat was implemented
src/backends/shared/NativeToolEngine.tsAbstract base class implementing
AgentEnginewith:getAllowedEnvExact(),getExtraEnvVars(),resolveEngineModel(),execute()resolveModel(): delegates toresolveEngineModel()so theAgentEnginecontract is metsupportsAgentType(): returnstrue(all native-tool engines support all agent types), overridablebuildEnv(): callsbuildEngineEnv()with engine-specific allowlist and extra varsafterExecute(): callscleanupContextFiles()— engines with additional cleanup override and callsuper.afterExecute()src/backends/shared/index.tsNew barrel export for the shared module.
src/backends/index.tsRe-exports
NativeToolEnginefrom the shared barrel.Test plan
NativeToolEngine.test.tscovering all shared methods via aStubEnginestubNotes
AgentEngineinterface is unchangedTrello card: https://trello.com/c/cWjLnCjl/537-as-a-developer-i-want-a-nativetoolengine-abstract-base-class-so-that-shared-subprocess-engine-logic-is-consolidated
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details