Skip to content

feat(config): normalize enable flags and add extraction.ocr subtable - #735

Merged
cpcloud merged 4 commits into
mainfrom
refactor/normalize-enable-flags
Mar 10, 2026
Merged

feat(config): normalize enable flags and add extraction.ocr subtable#735
cpcloud merged 4 commits into
mainfrom
refactor/normalize-enable-flags

Conversation

@cpcloud

@cpcloud cpcloud commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Rename extraction.enabled to extraction.enable with TOML key migration and env var deprecation (MICASA_EXTRACTION_ENABLED -> MICASA_EXTRACTION_ENABLE)
  • Add [extraction.ocr] subtable with enable (bool) and confidence_threshold (int) fields for independent OCR control
  • Wire OCR config through DefaultExtractors — OCR extractors conditionally included based on extraction.ocr.enable, low-confidence words filtered by confidence_threshold
  • Remove text_timeout from the config surface — pdftotext is fast, the 30s safety net stays as an internal DefaultTextTimeout constant
  • Fix FormatDuration to produce clean notation for whole minutes/hours (5m not 5m0s)
  • Fix docs that incorrectly described llm.timeout as a 5s quick-op timeout (it is a 5m HTTP response timeout)

closes #729

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR normalizes extraction-related config enable flags, adds a dedicated OCR subtable for independent OCR control, and wires OCR settings into the extraction pipeline, while also cleaning up duration formatting and documentation around timeouts.

Changes:

  • Rename extraction.enabledextraction.enable with TOML/env deprecation + migration; add [extraction.ocr] (enable, confidence_threshold) and validate threshold range.
  • Update extraction pipeline defaults/constructors to conditionally include OCR extractors and optionally filter OCR TSV output by confidence.
  • Remove extraction.text_timeout from the config surface and improve FormatDuration output for whole minutes/hours; update docs/tests accordingly.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/extract/pipeline_test.go Updates pipeline tests for the expanded DefaultExtractors signature.
internal/extract/pipeline.go Updates default extractor fallback to include new OCR-enabled/threshold parameters.
internal/extract/ocr_test.go Adds unit tests for TSV confidence filtering behavior.
internal/extract/ocr_progress_test.go Updates progress extraction tests for the expanded DefaultExtractors signature.
internal/extract/ocr_coverage_test.go Updates coverage tests for the expanded DefaultExtractors signature.
internal/extract/ocr.go Adds filterTSVByConfidence helper to drop low-confidence OCR rows.
internal/extract/extractor_test.go Extends tests for OCR enable/disable and confidence passthrough.
internal/extract/extractor.go Extends DefaultExtractors, adds OCR confidence threshold handling in OCR extractors.
internal/config/show_test.go Updates expectations for FormatDuration, enable key, and removal of text_timeout.
internal/config/show.go Adds extraction.enabled deprecation mapping and improves FormatDuration formatting.
internal/config/config_test.go Updates extraction enable env/key tests; adds migration tests and OCR config tests/validation coverage.
internal/config/config.go Introduces Extraction.Enable, keeps deprecated Enabled, adds Extraction.OCR subtable, removes TextTimeout, adds OCR validation, adds TOML/env migration for renamed keys.
docs/content/docs/reference/configuration.md Updates configuration reference for new keys, removed text_timeout, and corrects llm.timeout semantics.
cmd/micasa/main.go Wires OCR enable/threshold into DefaultExtractors; stops exposing text_timeout.
.claude/codebase/types.md Updates codebase type docs to reflect new config shape.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/config/config.go
Comment on lines +931 to +935
"extraction.enabled is deprecated -- use extraction.enable instead",
)
}
cfg.Extraction.Enabled = nil // never propagate the deprecated field

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

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

extraction.enabled is being cleared in migrateRenamedKeys, but the deprecated Enabled field can be repopulated by applyEnvOverrides when MICASA_EXTRACTION_ENABLED is set. That leaves both Extraction.Enable and Extraction.Enabled populated, which can cause duplicated/confusing output in ShowConfig (both enable and deprecated enabled keys showing as set). Consider clearing cfg.Extraction.Enabled after env overrides as well (and/or explicitly skipping env application for deprecated fields) so the deprecated field never propagates in the resolved config.

Copilot uses AI. Check for mistakes.
cpcloud and others added 4 commits March 9, 2026 18:43
Rename extraction.enabled -> extraction.enable and add extraction.ocr
subtable with enable and confidence_threshold fields. Add TOML key and
env var deprecation entries so existing configs continue to work with
warnings.

- Rename Extraction.Enabled to Extraction.Enable (TOML: enable)
- Keep deprecated Enabled field for backward compat migration
- Add OCR struct with Enable and ConfidenceThreshold
- Add filterTSVByConfidence to drop low-confidence OCR words
- DefaultExtractors now accepts ocrEnabled and confidenceThreshold
- Wire OCR config through cmd/main.go to extractors
- Add migration tests, OCR config tests, confidence filter tests
- Update docs and example TOML

closes #729

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pdftotext is fast enough that a user-configurable timeout adds no value.
The 30s safety net remains as an internal default in the extract package
(DefaultTextTimeout) but is no longer exposed as a config field, env var,
or documented option.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
FormatDuration now handles whole-minute and whole-hour multiples
(e.g. "5m" instead of "5m0s", "2h" instead of "2h0m0s"). Also
corrects docs that incorrectly described llm.timeout as a 5s
quick-op timeout — it's actually a 5m HTTP response timeout.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
applyEnvOverrides repopulates Extraction.Enabled from
MICASA_EXTRACTION_ENABLED after migrateRenamedKeys already cleared it.
Clear it again post-env-overrides so both fields never coexist in the
resolved config.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 9, 2026 22:44
@cpcloud
cpcloud force-pushed the refactor/normalize-enable-flags branch from 6b5244c to eb4f3f3 Compare March 9, 2026 22:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/config/show.go
Comment on lines 29 to 33
var deprecatedPaths = map[string]string{
"documents.cache_ttl_days": "documents.cache_ttl",
"extraction.enabled": "extraction.enable",
"extraction.model": "llm.extraction.model",
"extraction.thinking": "llm.extraction.thinking",

Copilot AI Mar 9, 2026

Copy link

Choose a reason for hiding this comment

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

deprecatedPaths includes extraction.enabled, but the loader migrates and then always clears cfg.Extraction.Enabled (both in migrateRenamedKeys and again after env overrides). As a result, ShowConfig can never emit the deprecated enabled = ... line with a DEPRECATED annotation, making this entry effectively dead. Either preserve the deprecated field for display (consistent with other deprecated keys like extraction.model) or remove this deprecatedPaths entry to avoid misleading future readers.

Copilot uses AI. Check for mistakes.
@codecov

codecov Bot commented Mar 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.60870% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.48%. Comparing base (b3e0106) to head (eb4f3f3).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/extract/extractor.go 52.94% 6 Missing and 2 partials ⚠️
cmd/micasa/main.go 0.00% 3 Missing ⚠️
internal/config/config.go 91.89% 2 Missing and 1 partial ⚠️
internal/extract/ocr.go 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
internal/config/show.go 81.69% <100.00%> (+0.32%) ⬆️
internal/extract/pipeline.go 95.89% <100.00%> (ø)
internal/extract/ocr.go 84.04% <90.00%> (+0.45%) ⬆️
cmd/micasa/main.go 3.33% <0.00%> (-0.03%) ⬇️
internal/config/config.go 89.42% <91.89%> (+2.76%) ⬆️
internal/extract/extractor.go 92.98% <52.94%> (-7.02%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cpcloud
cpcloud merged commit 7b9d7e5 into main Mar 10, 2026
29 checks passed
@cpcloud
cpcloud deleted the refactor/normalize-enable-flags branch March 10, 2026 08:10
cpcloud added a commit that referenced this pull request Mar 19, 2026
…735)

## Summary

- Rename `extraction.enabled` to `extraction.enable` with TOML key
migration and env var deprecation (`MICASA_EXTRACTION_ENABLED` ->
`MICASA_EXTRACTION_ENABLE`)
- Add `[extraction.ocr]` subtable with `enable` (bool) and
`confidence_threshold` (int) fields for independent OCR control
- Wire OCR config through `DefaultExtractors` — OCR extractors
conditionally included based on `extraction.ocr.enable`, low-confidence
words filtered by `confidence_threshold`
- Remove `text_timeout` from the config surface — pdftotext is fast, the
30s safety net stays as an internal `DefaultTextTimeout` constant
- Fix `FormatDuration` to produce clean notation for whole minutes/hours
(`5m` not `5m0s`)
- Fix docs that incorrectly described `llm.timeout` as a 5s quick-op
timeout (it is a 5m HTTP response timeout)

closes #729

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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.

refactor(config): normalize boolean enable flags to 'enable' with subtables

2 participants