Skip to content

feat(codex): extend NativeToolEngine instead of implementing AgentEngine directly#1023

Merged
aaight merged 1 commit intodevfrom
feature/codex-extend-native-tool-engine
Mar 23, 2026
Merged

feat(codex): extend NativeToolEngine instead of implementing AgentEngine directly#1023
aaight merged 1 commit intodevfrom
feature/codex-extend-native-tool-engine

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 23, 2026

Summary

  • Migrates CodexEngine to extend NativeToolEngine instead of directly implementing AgentEngine
  • Implements the three required abstract methods: getAllowedEnvExact(), getExtraEnvVars(), and resolveEngineModel()
  • Updates afterExecute() to call super.afterExecute() first (for context file cleanup), then capture the refreshed Codex auth token
  • Switches env building in execute() from the standalone buildEnv() to this.buildEnv() from the base class
  • Removes the standalone filterProcessEnv wrapper from codex/env.ts (deprecated in favor of shared utilities)

Key decisions

  • The _adapterLifecycleActive flag is preserved (not removed) since it is still needed to distinguish between direct execute() calls and adapter-mediated lifecycle calls — the base class does not provide this flag
  • codex/env.ts still exports buildEnv since the unit tests import it directly; only the redundant filterProcessEnv wrapper is removed
  • afterExecute() correctly calls super.afterExecute() before token capture, ensuring context file cleanup always runs

Test plan

  • All 63 existing codex tests pass without any assertion changes
  • All 6749 unit tests pass
  • TypeScript type check passes with zero errors
  • Lint passes with zero errors

Trello card: https://trello.com/c/69c1aa652cbf7f327b9278ab

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

LGTM — clean migration of CodexEngine from direct AgentEngine implementation to extending NativeToolEngine. The refactoring is correct and follows the established pattern set by ClaudeCodeEngine.

Verified:

  • getAllowedEnvExact(), getExtraEnvVars(), and resolveEngineModel() exactly reproduce the previous behavior (env var allowlist, extra vars, and model resolution are identical)
  • afterExecute() correctly calls super.afterExecute() first for context cleanup, then does engine-specific token capture — same pattern as ClaudeCodeEngine
  • this.buildEnv() delegates to buildEngineEnv with the same parameters as the standalone buildEnv() did
  • _directCallCleanup path (non-adapter direct calls) is unchanged and still handles context cleanup and token capture correctly
  • The removed filterProcessEnv wrapper and ALLOWED_ENV_PREFIXES/BLOCKED_ENV_EXACT aliases have no remaining references
  • cleanupContextFiles import is retained because _directCallCleanup still uses it
  • The env.ts duplication is acceptable since only tests import its buildEnv directly

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit e05cefc into dev Mar 23, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants