Skip to content

refactor(config): born-valid options, layered decode, drop mirrors - #576

Merged
16bit-ykiko merged 1 commit into
mainfrom
refactor/config-born-valid
Aug 3, 2026
Merged

16bit-ykiko merged 1 commit into
mainfrom
refactor/config-born-valid

Conversation

@16bit-ykiko

Copy link
Copy Markdown
Member

What

Unifies the config system around one set of structs and one decode mechanism, then simplifies everything that existed to work around the old split.

One struct set. Feature options structs (CodeCompletionOptions, InlayHintsOptions) now double as their clice.toml / initializationOptions sections: every field is defaulted<T> with its default as the field initializer. The all-optional mirror structs (InlayHintsConfig, CodeCompletionConfig) are deleted, along with all three hand-written field-by-field overlay blocks (apply_defaults, the compiler copy sites, inspect's --config parser). Adding a new option key is now a one-line change.

Born-valid Config. A default-constructed Config is fully valid — no two-phase init, with_defaults() is gone. apply_defaults() shrinks to finalize(): derived paths, ${workspace} substitution, canonicalization, rule glob compilation, and validation (an explicit zero worker count / memory budget is warned and reset — it was previously a silent auto sentinel).

Layering is the decode. Config sources are decoded sequentially onto one object (clice.toml, then initializationOptions); the decoder only touches named fields and nested sections merge per field. This is the mechanism the server already used for the initializationOptions overlay — the refactor removes everything that duplicated it.

Workers carry the whole Config. QueryParams/BuildParams lose their per-feature forwarded option fields (inlay_options, completion_options) and carry one Config; features read their own section. No per-feature forwarding field is ever added again.

Also: the dead project.max_active_file knob is removed (it had no reader; unknown keys in existing configs surface as a warning, not an error), and the tiny document_link.h / inactive_regions.h headers are folded into feature.h.

Compatibility

No key renames — existing clice.toml files and initializationOptions payloads behave identically, except max_active_file (now an unknown-key warning) and explicit zero worker counts (now warned and reset instead of silently corrected).

Tests

New unit tests pin the load-bearing semantics: section deep-merge across TOML/JSON layers (including a field set in both — later source wins), JSON null on an option is a decode error, typos inside feature sections still warn, the full born-valid default surface, and zero-value validation. All four suites pass on RelWithDebInfo plus unit tests under Debug (ASan + assertions); the snap suite passes with zero snapshot diffs, confirming the wire behavior is unchanged.

Feature options structs double as their config sections via defaulted<>
fields, so the field initializers are the single source of every default:

- delete InlayHintsConfig/CodeCompletionConfig mirrors and all three
  hand-written field-by-field overlay blocks (apply_defaults, the
  compiler.cpp copy sites, inspect's parse_completion_config)
- a default-constructed Config is fully valid; with_defaults() is gone
- apply_defaults() shrinks to finalize(): derived paths, ${workspace}
  substitution, canonicalization, rule compilation, and validation of
  explicit zero worker/memory values (warned and reset to defaults)
- clean the 0=auto sentinels: worker counts, memory limit, tracker
  intervals and project scalars carry real initializer defaults
- workers receive the whole Config on interactive requests instead of
  per-feature forwarded option fields (inlay_options/completion_options
  deleted); features read their own section
- drop the dead project.max_active_file knob (no reader anywhere)
- fold document_link.h and inactive_regions.h into feature.h
- pin the layering semantics in config unit tests: sections deep-merge
  per field across TOML/JSON layers, JSON null is a decode error,
  feature-section typos still warn, full born-valid default surface
@coderabbitai

coderabbitai Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Configuration and feature refactor

Layer / File(s) Summary
Configuration defaults and finalization
src/server/state/config.*, src/server/state/workspace.h, src/server/transport/master_server.cpp, src/server/transport/lsp_client.cpp, tests/unit/server/config_tests.cpp, docs/...
Configuration fields now have concrete defaults. Config::finalize validates and derives values after overlays. The obsolete max_active_file option and with_defaults APIs were removed.
Feature option and scanning contracts
src/feature/feature.h, src/feature/inactive_regions.cpp, src/index/preamble_state.h, src/driver/inspect.cc, tests/unit/feature/inactive_region_tests.cpp
Feature options use defaulted fields. DocumentLink and InactiveScan are declared in feature.h, and inactive-region API consumers use the consolidated header.
Workspace configuration propagation
src/server/protocol/worker.h, src/server/compiler/compiler.cpp, src/server/worker/*.cpp, src/server/compiler/indexer.cpp, src/server/service/query.cpp
Query and build requests carry complete workspace configuration. Workers and indexing services read validated configuration values directly.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.84% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the configuration refactor, including born-valid options, layered decoding, and removal of mirror structures.
Description check ✅ Passed The description directly explains the configuration refactor, compatibility changes, worker updates, removed settings, and added tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/config-born-valid

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/feature/feature.h`:
- Around line 310-318: Update inactive_regions() directive traversal in
inactive_regions.cpp to process only directives within the requested
[resume_offset, end_offset) scan interval before updating stack or serializing
regions. Ensure pending regions are closed at end_offset, and add coverage for
both bounded scans and resumed scans so directives outside the interval do not
affect results.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 26e7162a-01ad-418b-85df-40832a4497a2

📥 Commits

Reviewing files that changed from the base of the PR and between 55313da and af01b4f.

📒 Files selected for processing (26)
  • docs/clice.toml
  • docs/en/dev/contribution.md
  • docs/en/guide/configuration.md
  • docs/zh/dev/contribution.md
  • docs/zh/guide/configuration.md
  • src/driver/inspect.cc
  • src/feature/document_link.h
  • src/feature/feature.h
  • src/feature/inactive_regions.cpp
  • src/feature/inactive_regions.h
  • src/index/preamble_state.h
  • src/server/compiler/compiler.cpp
  • src/server/compiler/indexer.cpp
  • src/server/protocol/worker.h
  • src/server/service/query.cpp
  • src/server/state/config.cpp
  • src/server/state/config.h
  • src/server/state/workspace.h
  • src/server/transport/lsp_client.cpp
  • src/server/transport/master_server.cpp
  • src/server/worker/stateful_worker.cpp
  • src/server/worker/stateless_worker.cpp
  • tests/unit/feature/inactive_region_tests.cpp
  • tests/unit/index/tu_index_tests.cpp
  • tests/unit/server/config_tests.cpp
  • tests/unit/server/query_overlay_tests.cpp
💤 Files with no reviewable changes (7)
  • src/feature/document_link.h
  • src/feature/inactive_regions.h
  • docs/zh/guide/configuration.md
  • tests/unit/server/query_overlay_tests.cpp
  • tests/unit/index/tu_index_tests.cpp
  • docs/clice.toml
  • docs/en/guide/configuration.md

Comment thread src/feature/feature.h
@16bit-ykiko
16bit-ykiko merged commit 6c90090 into main Aug 3, 2026
51 of 53 checks passed
@16bit-ykiko
16bit-ykiko deleted the refactor/config-born-valid branch August 8, 2026 13:42
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.

1 participant