Skip to content

feat(config): add managed configuration mode - #6717

Merged
houko merged 4 commits into
mainfrom
feat/managed-config
Aug 7, 2026
Merged

houko merged 4 commits into
mainfrom
feat/managed-config

Conversation

@houko

@houko houko commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Implements the core of #6695. Refs #6695 rather than Closes — the RFC's acceptance criteria are broader than this PR, and the gaps are listed at the bottom rather than left to be discovered.

The four design questions in the RFC are answered in the issue thread; this is those answers built.

The contract

Variable Effect Default
LIBREFANG_CONFIG_PATH Load config.toml from this exact path instead of $LIBREFANG_HOME/config.toml. unset
LIBREFANG_CONFIG_MODE managed locks the file. Anything else — unset, empty, a typo — is mutable. unset

They are independent, which is the RFC's first design question. Relocating a file is not a statement about who owns it: a Compose deployment may reasonably bind-mount the config directory outside LIBREFANG_HOME and still want to edit it from the dashboard. Inferring the lock from a custom path hands that operator a read-only UI they never asked for, with no way out short of moving the file back.

A typo resolves to mutable. Defaulting a misspelling to the locked mode would take the dashboard away from a deployment that never asked for it; defaulting to mutable preserves existing behaviour, which is the compatibility guarantee the RFC requires.

The mode is read from the process environment and never from the config file, so an API write cannot unlock the very file it is being refused access to.

Enforcement

guard_config_write() in routes/mod.rs is the single place that decides the status and body:

{ "ok": false, "error": "configuration is managed by the deployment",
  "code": "config_managed", "source": "/etc/librefang/config.toml" }

423 Locked, returned before the handler reads the existing file, so a refused write never opens, truncates, or rewrites anything — asserted by comparing the file byte-for-byte after a refused config/set.

Enforcement is in the handlers, not on the mount. A read-only mount is still worth having as defence in depth, but it cannot be the mechanism: an EACCES surfaces as a 500 with an errno, which tells an operator nothing about why, and it does not apply at all to a deployment that leaves the file writable while still expecting the manifest to win. That is the RFC's "filesystem read-only errors must never be the enforcement mechanism", and this is the concrete reason it is right.

Guarded: POST /api/config/set, the budget persistence path (PUT /api/budget, PUT /api/providers/{name}/budget, per-user budget routes), and the user persistence path (POST/PUT/DELETE /api/users, key rotation).

Provenance

GET /api/config/status, authenticated like every /api/* route:

{ "mode": "managed", "source": "/etc/librefang/config.toml", "writable": false,
  "checksum": "sha256:9f2b…", "modified_at": "2026-08-04T09:41:12+00:00" }

writable is what a client branches on — equivalent to mode == "mutable", exposed separately so the dashboard uses a boolean rather than string-matching a mode name. The checksum is over the file's raw bytes and carries no value from inside it.

This is the RFC's "the UI must consume server-provided capability metadata rather than infer managed mode from failed writes".

The bug the RFC did not mention

crates/librefang-kernel/src/config.rs writes the migrated config back to disk whenever it loads a file written against an older schema version. Against a read-only ConfigMap mount that std::fs::write fails — and because the failure was only a warn!, the migration re-ran silently on every boot, forever, with nothing but a repeating log line.

Managed mode now skips the write and logs one targeted warning naming the file and both schema versions. The in-memory config is migrated either way, so nothing is degraded at runtime; what the operator learns is that their manifest is a schema version behind.

I flagged this on the issue as worth adding to the acceptance criteria. It is the case that proves enforcement cannot live in the filesystem.

Verification

  • cargo clippy --workspace --all-targets -- -D warnings — zero warnings.
  • cargo test -p librefang-api --test config_managed_mode_test — 3 passed.
  • cargo test -p librefang-api --test config_routes_integration — 45 passed.
  • cargo test -p librefang-kernel --lib config — 113 passed.
  • openapi_spec_test, openapi_path_coverage_test, dead_route_audit_test — green; openapi.json, the four SDKs, and the openapi.sha256 baseline regenerated.

The managed-mode tests live in their own test binary. LIBREFANG_CONFIG_MODE is process-global and Rust runs a binary's tests on parallel threads, so the first version of these tests broke five unrelated config_set cases by setting the variable out from under them — a mutex only serializes the tests that take it. A separate file is a separate process, so the blast radius stops there. Keeping them in config_routes_integration.rs would reintroduce that, so please don't merge them back.

No new dependencies: sha2, chrono, and serde were already librefang-kernel deps.

Deliberately not in this PR

  • The dashboard badge and disabled write controls. The server-side metadata they need is what this PR adds; the UI work is a second, self-contained change and this diff is already across kernel, API, OpenAPI, SDKs, docs, and tests.
  • In-place reload / ConfigMap watching. Rollout-only, per the answer on the issue. In-place reload has to survive Kubernetes' atomic symlink swap without reading a half-written file, then guarantee an invalid file never partially replaces the last valid config — a second hard problem, and a checksum-annotation rollout covers restart-required fields too, which is the superset.
  • Field-level ownership overlays. Whole-config lock, per the answer on the issue. WRITABLE_EXACT_PATHS / WRITABLE_SECTION_PREFIXES is already a field-level model and already load-bearing for security (external_auth. is excluded because of the OIDC login does not require email_verified — allowed_domains can be impersonated #3703 impersonation vector); a second orthogonal axis produces a matrix where the interesting cases are the corners.

Known gaps, stated rather than buried

Coverage is not complete. The skills routes (routes/skills/mod.rs, six sites), the memory route, and the dashboard-credential change in server.rs also write config.toml and are not guarded here.

Auditing for this turned up something worth its own attention: those same sites do not take config_write_lock either, so they can already race a concurrent config/set in mutable mode. That is a pre-existing bug this PR surfaces rather than introduces, and adding the lock to seven call sites changes concurrency semantics on paths I have not otherwise touched — which is why it is not bundled here. Happy to do it as a follow-up, or to fold it in if you'd rather it land together.

Both gaps are documented in docs/operations/managed-config.md under "Known gaps", so an operator does not discover them by trusting the lock and being wrong.

houko added 2 commits August 4, 2026 21:23
LibreFang treats config.toml as application state: the dashboard writes to it, several API routes persist into it, and boot-time migration rewrites it in place.
That is wrong for a deployment whose configuration comes from a ConfigMap or a config-management system, where a dashboard write is either lost on the next rollout or silently drifts the running config away from the manifest.

LIBREFANG_CONFIG_PATH relocates the file and LIBREFANG_CONFIG_MODE=managed locks it.
The two are independent because relocating a file is not a statement about who owns it, and inferring the lock from a custom path would hand a read-only dashboard to an operator who only wanted the file elsewhere.
The mode is read from the process environment and never from the config file, so an API write cannot unlock the file it is being refused access to.
A typo resolves to mutable rather than managed, so a misspelling cannot take the dashboard away from a deployment that never asked for it.

Managed mode answers 423 Locked with a structured body from the handler, before it reads the existing file, so a refused write never opens or truncates anything.
Enforcement is in the handlers rather than on a read-only mount: an EACCES surfaces as a 500 with an errno and says nothing about why, and it does not apply at all when the deployment leaves the file writable but still expects the manifest to win.

GET /api/config/status reports mode, source, writability, a SHA-256 over the file bytes, and last-modified, so the dashboard can present managed settings as read-only from server metadata instead of attempting a save and reading the refusal back.

Boot-time schema migration no longer writes the migrated config back when the file is managed.
That write previously failed against a read-only mount with only a warn!, so the migration silently re-ran on every boot forever.
The in-memory config is migrated either way; the warning now tells the operator their manifest is a schema version behind.
@github-actions github-actions Bot added area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) area/sdk JavaScript and Python SDKs size/L 250-999 lines changed labels Aug 4, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Deploying librefang-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3e94a87
Status: ✅  Deploy successful!
Preview URL: https://33c79d1d.librefang-docs.pages.dev
Branch Preview URL: https://feat-managed-config.librefang-docs.pages.dev

View logs

@github-actions github-actions Bot added the ready-for-review PR is ready for maintainer review label Aug 4, 2026
claude added 2 commits August 4, 2026 12:45
The new /api/config/status doc comment was inserted above the existing
/// GET /api/config/schema line instead of below it, so utoipa attributed
the merged two-endpoint comment block to config_status and left
config_schema's OpenAPI summary empty. Move the schema doc comment back
to its own function and regenerate openapi.json.
CLAUDE.md requires breaking prose only at sentence boundaries, not at a
column width. The doc comments and docs/operations/managed-config.md
added by this PR hard-wrapped multi-sentence blocks at ~90-100 columns
instead. Reflowed the new comments/docs in routes/mod.rs, config.rs,
config/manage.rs, config_managed_mode_test.rs, and
docs/operations/managed-config.md; regenerated openapi.json and its
sha256 baseline since one of the affected doc comments feeds a
utoipa summary/description.

@houko houko left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Daily automated review pass — CLAUDE.md compliance check.

Clean PR overall: changelog fragment present and correctly attributed (changelog.d/added/6695-managed-config-mode.md), the new /api/config/status route is registered in both openapi.rs and config/mod.rs, the new doc comments and docs/operations/managed-config.md are one-sentence-per-line per the prose-wrapping rule, no config.toml field was added for the mode (correctly — the design note explains why the mode must not be settable through the file itself), and the new integration test's use of server::build_router directly (rather than a start_test_server() wrapper) matches the existing pattern used by ~25 other files in crates/librefang-api/tests/, not a deviation. CI is green across the board.

One finding worth a maintainer call rather than a mechanical fix. This PR's own docs/operations/managed-config.md "Known gaps" section discloses that the skills, memory, and dashboard-credential routes write config.toml without going through guard_config_write() (or config_write_lock). Confirmed the three sites: routes/memory.rs:1622 (PATCH /api/memory/config), server.rs:1178 (dashboard password / API-key-hash rotation), and routes/skills/mod.rs (upsert_mcp_server_entry / remove_mcp_server_entry, ~L748-801). All three live in this same crate (librefang-api) that the diff is already touching. Per CLAUDE.md's "fix what you found" rule, the bar for deferring to a follow-up is "would fixing it require touching a different crate or domain" — here the answer is no, so wiring the guard into these three sites arguably belongs in this PR rather than being documented as a known gap, unless there's a reason (e.g. the pre-existing missing-lock race needs its own fix first) that makes splitting it out the right call. Flagging for your judgment rather than pushing a fix myself, since closing it touches three more handlers' write paths and I'd rather not guess at behavior changes across all of them in one pass.


Generated by Claude Code

@houko houko left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Daily automated review pass, continued from the prior pass on this same head commit (3e94a87).

Re-verified: openapi.json / xtask/baselines/openapi.sha256 are in sync with the current tree (sha256sum matches the baseline), no new KernelConfig struct fields were added (the mode is intentionally env-only, matching the design note), no HashMap/HashSet was introduced on any LLM-prompt-adjacent path, the changelog fragment is correctly formatted and attributed, commit messages are conventional-commit with no AI attribution, and the new/touched prose (doc comments, docs/operations/managed-config.md) is sentence-per-line per the prose-wrapping rule. Scoped cargo check -p librefang-api -p librefang-kernel --lib was run locally against this head (full --workspace --lib fails in this environment only on an unrelated, pre-existing librefang-desktop / gdk-sys system-library gap, not on anything in this diff).

One new finding not covered by the earlier review, left as a single inline comment: the managed-mode guard added to the budget and user persistence paths has no integration test coverage of its own, only config/set does.


Generated by Claude Code

state: &Arc<AppState>,
new_budget: &librefang_types::config::BudgetConfig,
) -> Result<(), PersistBudgetError> {
if crate::routes::guard_config_write().is_some() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Separate from the already-flagged "Known gaps" coverage question: the new config_managed_mode_test.rs only exercises the 423 path through POST /api/config/set. This guard call (and the matching one in routes/users.rs:1184) is new behavior on PUT /api/budget, PUT /api/providers/{name}/budget, the per-user budget routes, and POST/PUT/DELETE /api/users / key rotation — none of which have a test asserting the 423 Locked response or that the underlying file is left untouched when managed mode is active.

Per CLAUDE.md's Integration Testing section, a changed route's behavior (mutable write vs. refused write is a real behavior change, not just an internal refactor) should get its own #[tokio::test] against TestServer/build_router, following the pattern already established for config/set in this same file. Flagging rather than authoring the tests myself since picking representative cases across ~6 guarded handlers is a judgment call on scope, not a mechanical addition.


Generated by Claude Code

@houko houko left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Read through the full diff, the RFC discussion referenced in the description, docs/operations/managed-config.md, and docs/operations/config-reload.md.
Ran cargo check -p librefang-kernel --lib, cargo check -p librefang-api --lib, cargo clippy -p librefang-api -p librefang-kernel --all-targets -- -D warnings (clean except one pre-existing nonminimal_bool lint in routes/config/system.rs:338 that is untouched by this diff and present on origin/main too — unrelated to this PR), cargo test -p librefang-api --test config_managed_mode_test (3 passed), and cargo test -p librefang-api --test config_routes_integration (45 passed, no regressions).

Confirmed this PR does not add any new KernelConfig struct field (the mode is intentionally env-only, matching the design note in the description), so the struct-field/Default/serde checklist and the build_reload_plan classification table don't apply here.
No HashMap introduced on anything prompt-reaching.
Changelog fragment is present, correctly formatted, and its (#6695, #6717) correctly suppresses the generated line.
SDK bindings (Go/JS/Python/Rust) and the OpenAPI spec/checksum are all updated consistently for the one new route.

Left two comments — both need a maintainer call rather than a mechanical fix, so no commits from me this pass.


Generated by Claude Code

///
/// `LIBREFANG_CONFIG_PATH` relocates the file and nothing more.
/// It is independent of [`config_mode`] on purpose: a Compose deployment may reasonably bind-mount the config directory somewhere outside `LIBREFANG_HOME` while still editing it from the dashboard, and inferring the lock from the path would hand that operator a read-only UI they never asked for.
pub fn default_config_path() -> PathBuf {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LIBREFANG_CONFIG_PATH only relocates the initial read; the write, reload, and status paths do not follow it, and I think that breaks the PR's own advertised scenario.

Traced the actual read/write targets: config_set and persist_budget/persist_users build their file path from state.kernel.home_dir().join("config.toml") (routes/config/manage.rs:1342, routes/budget.rs, routes/users.rs), and reload_config() uses self.home_dir_boot.join("config.toml") (kernel/config_reload_ops.rs:38).
None of those three call default_config_path() — only load_config(None) (here) and guard_config_write() / config_provenance() in routes/mod.rs do.

KernelConfig.home_dir is populated from KernelConfig::default().home_dir, which is librefang_home_dir() in librefang-types/src/config/types.rs — that function checks LIBREFANG_HOME only, it has no knowledge of LIBREFANG_CONFIG_PATH.
So an operator who sets only LIBREFANG_CONFIG_PATH=/etc/librefang/config.toml (exactly the Compose bind-mount example in the PR description and in docs/operations/managed-config.md) without also setting LIBREFANG_HOME=/etc/librefang, or hand-adding home_dir = "/etc/librefang" inside the relocated file itself, gets:

  • the file correctly loaded from /etc/librefang/config.toml at boot,
  • but config_set/budget/user writes and reload_config() reading/writing ~/.librefang/config.toml (or wherever LIBREFANG_HOME points) instead — a different file,
  • while GET /api/config/status and the 423 refusal body's source field (both via default_config_path()) keep reporting the original relocated path, so the operator has no visible signal that writes are landing somewhere else.

I don't think this is a one-line fix — it's a design choice between (a) deriving home_dir from the resolved config path's parent whenever LIBREFANG_CONFIG_PATH is set, (b) a boot-time WARN when home_dir and default_config_path().parent() diverge, or (c) documenting that LIBREFANG_CONFIG_PATH requires a matching home_dir/LIBREFANG_HOME and isn't just a drop-in path override.
Grepped the repo for LIBREFANG_CONFIG_PATH/CONFIG_PATH_ENV — it's referenced nowhere outside this function's four lines, so there's no test exercising the relocation case at all today.


Generated by Claude Code


/// The default deployment is unchanged: writable, mode `mutable`, and `config/set` still works.
/// This is the compatibility guarantee the RFC asks for, so it is asserted rather than assumed.
#[tokio::test(flavor = "multi_thread")]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The three tests here only exercise GET /api/config/status and POST /api/config/set under managed mode.
The same guard is also wired into 10 more call sites across routes/budget.rs (update_budget, update_provider_budget, update_user_budget, delete_user_budget) and routes/users.rs (create_user, update_user, delete_user, import_users, rotate_user_key, update_user_policy), and none of them are exercised under LIBREFANG_CONFIG_MODE=managed anywhere in the suite.

The wiring is small (each site is if guard_config_write().is_some() { return Err(...::Managed) } ahead of the existing config_write_lock), and I did trace all ten by hand and they look correct, so I'm not flagging this as a bug.
But it's exactly the kind of "un-registered new response branch" that CLAUDE.md's integration-testing section asks for a #[tokio::test] on, and a single case (e.g. PUT /api/budget under managed mode refused with 423 and the daily-budget file byte-for-byte unchanged, mirroring config_set_is_locked_in_managed_mode_and_leaves_the_file_untouched) would cover the PersistBudgetError::Managed / PersistError::Managed arms at least once outside hand-reading.
Leaving this for your call on whether the config_set case is representative enough given the shared guard_config_write() helper, or whether it's worth one more case per domain.


Generated by Claude Code

houko commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Found one process issue while re-checking this PR: commit 8f77cf3 ("fix(api): reattach doc comment to config_schema, not config_status") has its author identity set to Claude <noreply@anthropic.com>:

$ git log -1 --format='%an <%ae>' 8f77cf3d06cea6a7d34c6c58847423b37d5d8e3a
Claude <noreply@anthropic.com>

CLAUDE.md's git conventions section is explicit that commit messages/PR bodies/comments must carry no AI/Claude/Anthropic attribution, and the repo's own commit-msg hook is supposed to reject "a commit whose author identity (git var GIT_AUTHOR_IDENT) resolves to Claude / Anthropic even when the message itself is clean." This commit predates that check catching it (likely pushed through a path where core.hooksPath wasn't pointed at scripts/hooks/ yet, or via an API push that bypasses local hooks). The message content itself is fine — this is purely the author identity.

I'm not rewriting history here (this routine's hard rule is never force-push, even on a branch I opened), so flagging for a manual fix before merge: git commit --amend --author="Evan Hu <suzukaze.haduki@gmail.com>" (or squash it into an adjacent commit) and force-push, since a rebase is the only way to correct an already-pushed commit's authorship.


Generated by Claude Code

@houko houko left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Daily automated review pass. No mechanical fixes needed — changelog fragment, docs cross-references, openapi.json/SDK regeneration, and test coverage all check out against the PR's own claims (verified by building and running the new test locally). One inline comment left on a real, already-self-flagged gap for maintainer judgment on scope.


Generated by Claude Code

Stated plainly rather than left to be discovered:

- **Coverage is not yet complete.**
The skills routes (`crates/librefang-api/src/routes/skills/mod.rs`), the memory route, and the dashboard-credential change in `server.rs` write `config.toml` without going through the guard.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified this gap is real, not just theoretical: change_password in crates/librefang-api/src/server.rs (~line 1178-1233) reads config.toml, mutates dashboard_user/dashboard_pass_hash, and calls std::fs::write(&config_path, &toml_string) directly — no guard_config_write() call and no config_write_lock acquisition anywhere in the function. Same pattern confirmed in routes/skills/mod.rs (upsert_mcp_server_config / the sibling remove function around line 748-800).

This one seems worth prioritizing over the others: it's the credential-rotation path, so under managed mode an operator (or anyone who reaches this authenticated route) can still silently rewrite the file the deployment believes it owns, and the write will vanish on the next ConfigMap rollout with no warning — the opposite of the guarantee GET /api/config/status advertises. The skills/memory gaps are "another feature can still edit config" whereas this one is "the credential story managed mode is partly meant to protect can still mutate the locked file."

Given the PR description already offers to fold this in or do it as a follow-up: my read is change_password is small and self-contained enough (single guard check + the existing config_write_lock, mirroring the budget.rs/users.rs pattern already in this diff) that it could land in this PR without touching the concurrency-lock question for the other six sites, which does deserve its own PR given the "changes concurrency semantics on paths I have not otherwise touched" concern you raised. But that's a scope call for a maintainer, not something I'm changing unilaterally here.

(Automated review pass — verified via a fresh clone + cargo check -p librefang-api -p librefang-kernel --lib (clean) and cargo test -p librefang-api --test config_managed_mode_test (3 passed, matches the PR description). No mechanical issues found: changelog fragment, docs cross-reference, openapi.json/SDK regen, and auth-allowlist placement all check out.)


Generated by Claude Code

///
/// `LIBREFANG_CONFIG_PATH` relocates the file and nothing more.
/// It is independent of [`config_mode`] on purpose: a Compose deployment may reasonably bind-mount the config directory somewhere outside `LIBREFANG_HOME` while still editing it from the dashboard, and inferring the lock from the path would hand that operator a read-only UI they never asked for.
pub fn default_config_path() -> PathBuf {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LIBREFANG_CONFIG_PATH only changes where the initial boot load reads from — it does not change where the daemon writes or reloads from afterward, so relocation and reload/write silently diverge onto two different files.

default_config_path() is called from load_config(None) at boot, but every other place that touches config.toml builds the path independently from home_dir (which defaults to librefang_home() — unrelated to LIBREFANG_CONFIG_PATH — see KernelConfig::default() in crates/librefang-types/src/config/types.rs:6462):

  • reload_config — crates/librefang-kernel/src/kernel/config_reload_ops.rs:40: self.home_dir_boot.join("config.toml")
  • config_set — crates/librefang-api/src/routes/config/manage.rs:1342: state.kernel.home_dir().join("config.toml")
  • persist_budget — crates/librefang-api/src/routes/budget.rs:596: same pattern
  • persist_users (crates/librefang-api/src/routes/users.rs) and export_config (crates/librefang-api/src/routes/config/manage.rs:1045): same pattern

Concretely: set LIBREFANG_CONFIG_PATH=/etc/librefang/config.toml with LIBREFANG_HOME left at its default, and mutable mode. Boot reads /etc/librefang/config.toml, but POST /api/config/set (and the budget/users persist paths) will read-modify-write ~/.librefang/config.toml instead — a different file than the one the daemon booted from — and POST /api/config/reload re-reads ~/.librefang/config.toml too, not the relocated file. That contradicts the managed-config.md claim that reload "still works... it re-reads the file" (it re-reads the wrong file when relocated), and it means the doc's own headline scenario — "a Compose bind mount or a ConfigMap mounted outside LIBREFANG_HOME" — doesn't actually work end-to-end for anything past the first boot.

This is new: default_config_path() and home_dir-based path construction used to always agree (both reduced to librefang_home().join("config.toml")) before CONFIG_PATH_ENV was introduced here. Worth resolving before merge, or at minimum calling out as a known limitation in docs/operations/managed-config.md next to the other "Known gaps" — right now the doc states the relocation case as the supported scenario without the caveat.


Generated by Claude Code

"Config was migrated in memory but NOT written back: the file is deployment-managed (LIBREFANG_CONFIG_MODE=managed). \
This migration re-runs on every boot until the managed source is updated to the new schema."
);
} else if migrated && file_version < CONFIG_VERSION {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This managed-mode migration-skip branch (and config_mode() / default_config_path()'s LIBREFANG_CONFIG_PATH handling below) has no unit test in this file — coverage is only indirect, via the API-level tests in crates/librefang-api/tests/config_managed_mode_test.rs, which don't exercise load_config's migration path at all.

A direct test here (write a v1 config under LIBREFANG_CONFIG_MODE=managed, call load_config, assert the in-memory config is migrated but the on-disk bytes are untouched and the warning fires) would be the natural analog of the existing test_load_config_migrates_v1_api_section right above it. The catch is the same one config_managed_mode_test.rs calls out in its own module doc: LIBREFANG_CONFIG_MODE is process-global, and this crate's #[cfg(test)] mod tests runs many other tests in the same binary on parallel threads, so setting the env var here risks bleeding into unrelated tests (e.g. test_load_config_v2_skips_migration, test_strict_mode_*) the same way the API test's own comment says the first version of that file broke five unrelated cases. Given that documented hazard, I'd rather flag it than land a same-file env-mutating test speculatively — worth a deliberate call on serial-test / a dedicated binary / a mutex before adding it, not a five-minute follow-up.


Generated by Claude Code

@houko
houko merged commit 35d3ab5 into main Aug 7, 2026
42 checks passed
@houko
houko deleted the feat/managed-config branch August 7, 2026 05:07
houko added a commit that referenced this pull request Aug 9, 2026
… the documented gap list (#6737)

* fix(api): lock the provider config routes in managed mode

`set_provider_key`, `set_provider_url` and `set_default_provider` persisted into `config.toml` without passing through `guard_config_write`, so a managed deployment could still have `[default_model]`, `[provider_urls]` and `[provider_proxy_urls]` rewritten from the dashboard. Each now returns the documented 423 with `code: "config_managed"` before reading anything.

The guard sits in the handler rather than in `persist_default_model` / `upsert_provider_url` / `upsert_provider_proxy_url` because those are sync fns returning a boxed error, which the handlers map to a 500 or swallow into a `warn!` — either way losing the 423 and its structured body.

`set_provider_key` is refused in full, including its `secrets.env` write. Its config write is conditional on live daemon state the caller cannot observe, so guarding only the write would accept or refuse the identical request depending on timing, having already rewritten `secrets.env` in the refusing case. `set_default_provider` is refused before the OpenRouter / EveryAPI catalog refreshes so a refused request costs no outbound HTTP, and because its persist failure is only a `warn!` — without the guard it would answer 200 and hot-switch the live default against the manifest.

`PUT /api/providers/{name}/discovery` stays unguarded: its `atomic_write` targets a per-provider catalog fragment under `providers_dir`, not `config.toml`.

Also corrects the Known-gaps section of docs/operations/managed-config.md, which named `routes/memory.rs`, `routes/skills/mod.rs` and `server.rs` without their write sites. All three do write `config.toml` — via `std::fs::write` rather than `crate::atomic_write` — so the section keeps them but now names each route with the exact call that performs the write, adds the sidecar-channel and quick-init routes it had missed, records that the MCP routes only write the file under the default `mcp_runtime_store = "file"`, and drops the inaccurate claim that none of the gap routes take `config_write_lock` (the sidecar routes do).

Refs #6695, #6717

* docs(changelog): add fragment for the managed-mode provider route guard

* fix(api): rewrap new test comments to one sentence per line

CLAUDE.md's prose-wrapping rule requires new prose to break only at
sentence boundaries, not at a fixed column.
Four `//` comment blocks added by this PR in config_managed_mode_test.rs
still hard-wrapped mid-sentence across multiple lines while the new
`///` doc comments in the same PR already followed the rule.

* docs(managed-config): clarify mcp_runtime_store escape does not cover extension routes

The known-gaps table groups the extension install/uninstall routes with
the direct MCP-server routes under the same mcp_runtime_store qualifier,
but routes/skills/extensions.rs calls upsert_mcp_server_config and
remove_mcp_server_config unconditionally, with no mcp_runtime_store
check, so setting mcp_runtime_store = "db" does not stop
install_extension/uninstall_extension from writing config.toml.

---------

Co-authored-by: Claude <noreply@anthropic.com>
@houko houko mentioned this pull request Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) area/sdk JavaScript and Python SDKs ready-for-review PR is ready for maintainer review size/L 250-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants