Background
Two near-identical token parsers exist for the \${X} / \${X:-d} / \${X?msg} syntax:
internal/agent/custom.go::resolveTemplateToken — run-time, against the rendered variable map.internal/config/customengine.go::resolveEnvToken — config-load-time, strict mode.
They differ only in strictness and return signature. Bugs fixed in one are easy to miss in the other.
Independently, internal/config/CustomEngineConfig is now imported by both the config and agent packages. The agentkind leaf package (#PR24) already broke the factory dispatch cycle, but the config type itself still lives in internal/config, which means anything that needs the type today must depend on config.
Proposal
- Introduce
internal/customengine as a new leaf package that owns:
CustomEngineConfig, CustomLocalConfig, CustomHTTPConfig, CustomHTTPFile types (moved out of internal/config/schema.go).- A shared
ResolveToken(inner string, vars map[string]string, opts ResolveOpts) so both call sites use one parser.
- Re-export shims from
internal/config for one release to avoid a hard break for any external importer.
Why this is a follow-up, not a blocker
Both parsers behave correctly today and are covered by tests. The duplication is a maintenance hazard, not a defect.
Origin
Raised during the self code-review of feat/custom-engine-local (code-quality finding #1 and architecture finding #2).
Background
Two near-identical token parsers exist for the
\${X}/\${X:-d}/\${X?msg}syntax:internal/agent/custom.go::resolveTemplateToken— run-time, against the rendered variable map.internal/config/customengine.go::resolveEnvToken— config-load-time, strict mode.They differ only in strictness and return signature. Bugs fixed in one are easy to miss in the other.
Independently,
internal/config/CustomEngineConfigis now imported by both theconfigandagentpackages. The agentkind leaf package (#PR24) already broke the factory dispatch cycle, but the config type itself still lives ininternal/config, which means anything that needs the type today must depend onconfig.Proposal
internal/customengineas a new leaf package that owns:CustomEngineConfig,CustomLocalConfig,CustomHTTPConfig,CustomHTTPFiletypes (moved out ofinternal/config/schema.go).ResolveToken(inner string, vars map[string]string, opts ResolveOpts)so both call sites use one parser.internal/configfor one release to avoid a hard break for any external importer.Why this is a follow-up, not a blocker
Both parsers behave correctly today and are covered by tests. The duplication is a maintenance hazard, not a defect.
Origin
Raised during the self code-review of
feat/custom-engine-local(code-quality finding #1 and architecture finding #2).