refactor(config)!: make config sections orthogonal (#731)#747
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the application configuration so that each section is fully self-contained with no cross-section value inheritance. Previously, [llm] was a shared base that [llm.chat] and [llm.extraction] overrode; now [chat.llm] and [extraction.llm] are independent with their own defaults. The OCR TSV settings are also reorganized into a dedicated [extraction.ocr.tsv] sub-section. All deprecated key migration code, env var renames, and backward-compatibility shims (except cache_ttl_days) are removed.
Changes:
- Rearchitected config types:
LLMbase →Chat/ChatLLM/ExtractionLLMwith independent defaults;OCR.TSV/OCRTSVreplaces the flattsv boolandconfidence_thresholdfields inOCR - Removed all deprecated TOML key migrations, env var rename logic, and backward-compat shims (~769 lines)
- Updated all call sites (
main.go,app/types.go,app/model.go,app/chat.go), tests, documentation, and VHS tapes to use the new config paths and env vars
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
plans/orthogonal-config.md |
New design document explaining the orthogonal config architecture |
internal/config/config.go |
Core types rewrite: LLM → Chat/ChatLLM/ExtractionLLM/OCRTSV; removes migrateRenamedKeys, envRenames, migrateRenamedEnvVars, coalesce, ResolvedLLM, validateOverrideTimeout → validateTimeout |
internal/config/show.go |
Removes deprecatedPaths map and deprecated field annotation logic; isEmptyValue updated so non-nil pointer to zero is not considered empty |
internal/config/config_test.go |
Tests updated to new paths/env vars; adds findAll helper; removes ~400 lines of deprecated migration tests |
internal/config/show_test.go |
Tests updated to new section names; removes pipeline override/deprecated section tests |
internal/app/types.go |
llmConfig → chatConfig (with Enabled bool); SetLLM → SetChat; extractionConfig comment cleaned |
internal/app/model.go |
llmConfig *llmConfig → chatCfg chatConfig; extractionLLMClient no longer falls back to chat config |
internal/app/chat.go |
llmConfig → chatCfg reference; config hint message updated to new section names |
internal/app/extraction_test.go |
llmConfig → chatCfg in test setup; TestExtractionClient_FallsBackToChatConfig replaced with TestExtractionClient_NilWhenNoExtractionModel |
cmd/micasa/main.go |
Updated to call SetChat/SetExtraction with new config field accessors |
docs/tapes/llm-chat.tape |
Env var updated to MICASA_CHAT_LLM_MODEL |
docs/tapes/extraction.tape |
Env var updated to MICASA_EXTRACTION_LLM_MODEL |
docs/content/docs/reference/configuration.md |
Full reference table and section docs updated to new config paths |
docs/content/docs/guide/llm-chat.md |
[llm] → [chat.llm] references |
docs/content/blog/llm-plumbing.md |
Config examples and env vars updated |
cmd/micasa/main_test.go |
Config key updated to chat.llm.model |
.claude/codebase/types.md |
Config type map updated |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/config/config.go:444
- The
applyEnvOverridesfunction's comment (line 440-441) still mentions "The extra map supplies values migrated from deprecated env var names (checked when the canonical env var is unset)." Sinceextrais always passed asnilafter the refactor removed all deprecated env var migration, the comment is now misleading. Theextraparameter and its nil-guard (if val == "" && extra != nil) inwalkEnvFieldsare also now dead code — the function signature and nil check could be simplified by removing theextraparameter entirely.
// applyEnvOverrides walks the Config struct and applies environment variable
// overrides. Env var names are derived from the dotted TOML path via
// [EnvVarName]. The extra map supplies values migrated from deprecated env
// var names (checked when the canonical env var is unset).
func applyEnvOverrides(cfg *Config, extra map[string]string) error {
return walkEnvFields(reflect.ValueOf(cfg).Elem(), "", extra)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9fa589d to
405d063
Compare
405d063 to
80bae95
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/config/show.go:33
- The ShowConfig-related comments and naming still refer to “overrides” (e.g.
ShowConfigsays it’s “marking active overrides”, andsectionBlock.overrideis set bystrings.Contains(prefix, ".")). With the new orthogonal config (no inheritance), these are no longer overrides—just nested tables. Updating the comments and renamingoverrideto something likesubtable/nestedwould make the intent clearer, and avoids implying there’s still cross-section override behavior.
// ShowConfig writes the fully resolved configuration as valid TOML to w,
// annotating each field with its env var name and marking active overrides.
func (c Config) ShowConfig(w io.Writer) error {
internal/config/config.go:443
applyEnvOverridesstill takes anextramap and the comment says it supplies values migrated from deprecated env vars, butLoadFromPathalways callsapplyEnvOverrides(&cfg, nil)and nothing else passes a non-nil map. This leaves dead code (and a misleading comment) inwalkEnvFields(if val == "" && extra != nil { ... }). Consider removing theextraparameter entirely and updating the comment to reflect the current behavior (no deprecated env-var migration).
// applyEnvOverrides walks the Config struct and applies environment variable
// overrides. Env var names are derived from the dotted TOML path via
// [EnvVarName]. The extra map supplies values migrated from deprecated env
// var names (checked when the canonical env var is unset).
func applyEnvOverrides(cfg *Config, extra map[string]string) error {
return walkEnvFields(reflect.ValueOf(cfg).Elem(), "", extra)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rearchitect the config system so each section is self-contained with no cross-section value inheritance. Kill the shared [llm] base section and replace it with independent [chat.llm] and [extraction.llm] sections. Move OCR TSV settings into their own [extraction.ocr.tsv] sub-section. - Remove all deprecated key migration code, env var renames, and backward-compatibility shims - Remove deprecated MICASA_CURRENCY env var; only MICASA_LOCALE_CURRENCY is supported - Remove redundant llmExtraContext field; use chatCfg.ExtraContext - Remove dead collectValues/allValues code from ShowConfig - Combine env source and deprecation comments in config --dump output - Rename persisted settings DB key from llm.model to chat.llm.model - Update all docs, VHS tapes, error messages, and codebase maps - Add AGENTS.md rules for resisting configuration and orthogonality BREAKING CHANGE: The [llm] config section and all deprecated env vars (MICASA_LLM_*, MICASA_EXTRACTION_MODEL, MICASA_EXTRACTION_TIMEOUT, MICASA_CURRENCY) have been removed. Use [chat.llm] and [extraction.llm] sections with their corresponding MICASA_CHAT_LLM_* and MICASA_EXTRACTION_LLM_* env vars instead. The persisted model DB key changed from llm.model to chat.llm.model; existing model preferences will reset on first use. Closes #731 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a8160e2 to
26144a5
Compare
Rearchitect the config system so each section is self-contained with no
cross-section value inheritance. Kill the shared [llm] base section and
replace it with independent [chat.llm] and [extraction.llm] sections.
Move OCR TSV settings into their own [extraction.ocr.tsv] sub-section.
backward-compatibility shims
is supported
BREAKING CHANGE: The [llm] config section and all deprecated env vars
(MICASA_LLM_, MICASA_EXTRACTION_MODEL, MICASA_EXTRACTION_TIMEOUT,
MICASA_CURRENCY) have been removed. Use [chat.llm] and [extraction.llm]
sections with their corresponding MICASA_CHAT_LLM_ and
MICASA_EXTRACTION_LLM_* env vars instead. The persisted model DB key
changed from llm.model to chat.llm.model; existing model preferences
will reset on first use.
Closes #731
🤖 Generated with Claude Code