Skip to content

feat(backends): unified resolveSettings() in NativeToolEngine, remove static schema pre-registration#1029

Merged
aaight merged 1 commit intodevfrom
feature/unified-settings-resolution
Mar 24, 2026
Merged

feat(backends): unified resolveSettings() in NativeToolEngine, remove static schema pre-registration#1029
aaight merged 1 commit intodevfrom
feature/unified-settings-resolution

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 24, 2026

Summary

  • Add protected resolveSettings<S>() template method to NativeToolEngine base class that reads input.engineSettings ?? input.project.engineSettings and validates via the engine's schema identified by this.definition.id
  • Remove static import/re-export/pre-registration of ClaudeCodeSettingsSchema, CodexSettingsSchema, OpenCodeSettingsSchema from src/config/engineSettings.ts (lines 1-12 import block and lines 24-26 pre-registration)
  • Schema registration now happens exclusively through registerEngineWithSettings() in bootstrap.ts, which is called by all entry points before config parsing

Test plan

  • New unit tests added for resolveSettings() in NativeToolEngine.test.ts (5 cases)
  • All 354 test files, 6797 tests pass with zero failures
  • TypeScript type check passes with zero errors
  • Lint passes with zero errors
  • All entry points (router, worker, dashboard) already call registerBuiltInEngines() before config parsing — verified

Card: https://trello.com/c/69c1aaa53b8e204b0317a47d

🤖 Generated with Claude Code

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 24, 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 extraction of a reusable resolveSettings() template method into NativeToolEngine, and safe removal of static schema pre-registration that was already redundant with the bootstrap flow.

Verification

  • No broken consumers: The removed re-exports (ClaudeCodeSettingsSchema, CodexSettingsSchema, OpenCodeSettingsSchema and their type aliases) from src/config/engineSettings.ts are not imported from that path by any remaining file — all consumers import directly from their respective engine settings.ts modules.
  • Bootstrap coverage: All entry points (router/index.ts, worker-entry.ts, dashboard.ts) plus the agent registry call registerBuiltInEngines() before config parsing, so dynamic registration is guaranteed.
  • resolveSettings() correctness: The implementation correctly delegates to getEngineSettings() with this.definition.id as the key, handles the null/undefined case by falling back to {}, and the ?? operator correctly prefers input.engineSettings over input.project.engineSettings.
  • Tests are well-structured: 5 test cases cover the key scenarios (no settings, project-level, override precedence, missing engine key, correct engine id lookup). The resolveSettingsPublic wrapper pattern for exposing protected methods is a clean, common Vitest approach.
  • CI passes: All 7 checks green including lint, typecheck, unit tests, and integration tests.

Notes

The new resolveSettings() method is not yet used by any concrete engine (Claude Code, Codex, and OpenCode still use their own resolveXxxSettings() helper functions that also apply defaults). This is fine — it's an incremental step that lays groundwork for subclasses to migrate to the shared method later, and the existing per-engine resolve*Settings() functions do more than just parse (they apply defaults and return a Resolved*Settings type), so they can't be trivially replaced yet.

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

@aaight aaight merged commit 022771f into dev Mar 24, 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