feat(backends): co-locate engine settings schemas with engines#838
feat(backends): co-locate engine settings schemas with engines#838
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that co-locates engine settings schemas with their engines and replaces a hardcoded lookup object with a dynamic registry. The change is well-structured, backward-compatible, CI is green, and the architecture direction (engines own their configuration) is sound.
Architecture & Design
The move from a static Record to a dynamic Map registry is the right pattern for an extensible engine system. The backward-compatibility re-exports from engineSettings.ts are thoughtful and prevent breaking downstream consumers.
Circular import note: The re-exports in engineSettings.ts create a circular dependency with backends/codex/settings.ts and backends/opencode/settings.ts (each re-exports from the other). This works correctly in ESM because both sides export standalone declarations that don't depend on each other at initialization time — the live bindings resolve before any code calls these functions. TypeScript and the test suite confirm this. However, if this backward-compat layer is ever removed, the circularity goes away cleanly.
Initialization ordering: The EngineSettingsSchema validation now depends on registerBuiltInEngines() having been called first. All current call sites (agent registry, API routers, tests) correctly ensure this. Worth keeping in mind for future consumers.
Minor Observations (non-blocking)
getEngineSettingsSchema()is exported fromengineSettings.tsbut has zero call sites — it's dead code. If it's intended for future use by third-party engine plugins, a brief comment noting that would help. Otherwise it could be removed.- The inline
import('./types.js').AgentEnginetype annotation inregisterEngineWithSettingsis unconventional — a top-levelimport type { AgentEngine } from './types.js'would be more idiomatic. Not a problem, just a style note. - The
engine-contract.test.tsalready tests optional interface methods (resolveModel,beforeExecute,afterExecute) but doesn't covergetSettingsSchema. Adding a similar check would complete the contract surface.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
CodexSettingsSchemadefinition fromsrc/config/engineSettings.tstosrc/backends/codex/settings.tsOpenCodeSettingsSchemadefinition fromsrc/config/engineSettings.tstosrc/backends/opencode/settings.tsENGINE_SETTINGS_SCHEMASmap with a dynamicMapregistry inengineSettings.tsgetSettingsSchema?()method to theAgentEngineinterface intypes.tsCodexEngineandOpenCodeEngineimplementgetSettingsSchema()returning their local schemabootstrap.tsregisters each engine's settings schema viaregisterEngineSettingsSchema()duringregisterBuiltInEngines()CodexSettingsSchema/OpenCodeSettingsSchemaand their types fromengineSettings.tsfor backward compatibilityschema.test.tsandprojects.test.tsto callregisterBuiltInEngines()inbeforeAllTest plan
npm run typecheckpasses (zero errors)npm run lintpasses (zero errors)tests/unit/config/schema.test.ts,tests/unit/api/routers/projects.test.ts)Trello card: https://trello.com/c/SiKIeCHO/353-as-a-developer-i-want-engine-settings-co-located-with-engines-so-that-each-engine-owns-its-configuration-schema
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details