Config: fail closed invalid config loads by vincentkoc · Pull Request #39071 · openclaw/openclaw

vincentkoc · 2026-03-07T18:01:53Z

Summary

Problem: one invalid config key caused loadConfig() to swallow INVALID_CONFIG and return {}.
Why it matters: read-only CLI and service-management paths could proceed under fallback defaults, dropping intended security-sensitive settings.
What changed: loadConfig() now fails closed on invalid config; explicit best-effort reads are used only where diagnostics still need to inspect a broken config.
What did NOT change (scope boundary): this PR does not add partial schema acceptance for arbitrary unknown keys; invalid config still remains invalid until fixed.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Invalid config no longer degrades to an empty runtime config.
Read-only diagnostics (status, health, gateway inspection commands) still work, but only through an explicit best-effort config path.
Valid settings like tools.sessions.visibility are preserved for diagnostics even when an unrelated unknown key makes the config invalid.

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (No)
New/changed network calls? (No)
Command/tool execution surface changed? (No)
Data access scope changed? (No)
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS 15.6.1
Runtime/container: local pnpm workspace
Model/provider: N/A
Integration/channel (if any): CLI / gateway diagnostics
Relevant config (redacted): invalid skills.nano-banana-pro plus valid tools.sessions.visibility: "all"

Steps

Write an invalid openclaw.json with one unknown skill key and one valid setting.
Call loadConfig() and compare it with the explicit best-effort config read.
Run read-only CLI diagnostics and targeted tests.

Expected

loadConfig() fails on invalid config.
Diagnostic paths preserve valid settings instead of falling back to {} defaults.

Actual

Verified after the patch.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: reproduced issue config, confirmed loadConfig() throws, confirmed readBestEffortConfig() preserves tools.sessions.visibility: "all", ran pnpm build, pnpm check, and targeted Vitest coverage for config/status/health/gateway/daemon flows.
Edge cases checked: invalid config with unknown skill key, gateway status JSON diagnostics, daemon install/restart drift tests, health command output paths.
What you did not verify: full end-to-end daemon install on every platform.

Compatibility / Migration

Backward compatible? (Mostly)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly: revert the four commits in this PR.
Files/config to restore: config loader changes in src/config/io.ts and the explicit best-effort call sites.
Known bad symptoms reviewers should watch for: diagnostic commands unexpectedly failing on invalid config instead of surfacing best-effort output.

Risks and Mitigations

Risk: a read-only diagnostic path still relies on implicit loadConfig() and now fails on invalid config.
Mitigation: build + targeted tests cover the touched diagnostic and daemon CLI paths; local repro confirms best-effort routing preserves valid settings.

greptile-apps · 2026-03-07T18:09:22Z

Greptile Summary

This PR fixes a long-standing silent failure in loadConfig() where an INVALID_CONFIG error was swallowed and converted to an empty runtime config ({}), potentially causing security-sensitive settings to be silently dropped. loadConfig() now re-throws on invalid config, and a new readBestEffortConfig() async helper is introduced for diagnostic paths that still need to surface partial config output when the file is broken.

Key observations:

Core fix is correct — making loadConfig() fail closed on INVALID_CONFIG is the right call, and the updated compat test directly validates the new behavior.
Diagnostic call-sites are correctly routed — gateway-status, status.scan, status-all, health, register, and run-main are all read-only paths and appropriately switch to readBestEffortConfig().
TOCTOU in readBestEffortConfig() — when the snapshot is valid, the helper discards the already-parsed snapshot.config and calls loadConfig() a second time. If the config file is replaced between the two reads (e.g. a concurrent write), loadConfig() can throw INVALID_CONFIG, defeating the resilience the helper is meant to provide. Returning snapshot.config directly on the valid branch would eliminate the race.
Mutating paths use best-effort config — runDaemonInstall (install.ts), runDaemonRestart/resolveGatewayRestartPort (lifecycle.ts), and the token-drift check in runServiceRestart (lifecycle-core.ts) all switched to readBestEffortConfig(). These are write/mutating operations, not read-only diagnostics. If gateway port or token settings happen to be in the invalid section of the config, the daemon can be installed or restarted with wrong defaults silently — contrary to the PR's stated "fail closed" intent for non-diagnostic paths.
getAgentLocalStatuses default parameter — the loadConfig() default is retained solely for a fallback that no current call-site uses; making the parameter required and removing the now-dead import would be cleaner.

Confidence Score: 3/5

Safe to merge for diagnostic paths, but the use of best-effort config in daemon install/restart write paths warrants scrutiny before shipping.
The core fix (throwing on INVALID_CONFIG) is correct and well-tested. However, three mutating daemon lifecycle paths (runDaemonInstall, runDaemonRestart, runServiceRestart) now proceed with potentially partial config, which could silently misconfigure the daemon. Additionally, the TOCTOU double-read in readBestEffortConfig() can cause the helper to throw in the very scenario it is meant to handle gracefully.
src/config/io.ts (TOCTOU in readBestEffortConfig), src/cli/daemon-cli/install.ts, src/cli/daemon-cli/lifecycle.ts, and src/cli/daemon-cli/lifecycle-core.ts (mutating paths using best-effort config).

Comments Outside Diff (3)

src/config/io.ts, line 1379-1382 (link)

TOCTOU: double file read with a validity race window

When the snapshot says the config is valid, readBestEffortConfig() discards the already-parsed snapshot.config and calls loadConfig() again, reading the file a second time. If the file is replaced between readConfigFileSnapshot() and loadConfig() (e.g. a config write in another process), loadConfig() could now throw INVALID_CONFIG — the very error that readBestEffortConfig() is designed to suppress — making the best-effort path itself non-resilient.

Returning snapshot.config unconditionally would be consistent and eliminate the race:

Or more clearly:
```
export async function readBestEffortConfig(): Promise<OpenClawConfig> {
  const snapshot = await readConfigFileSnapshot();
  return snapshot.config;
}
```
If loadConfig() is intentionally called here to pick up in-process runtime overrides that snapshot.config doesn't carry, that dependency should be documented with a comment explaining why the double-read is necessary.
src/cli/daemon-cli/install.ts, line 29-35 (link)

Write path using best-effort config may silently drop security-sensitive settings

runDaemonInstall is a mutating operation, not a read-only diagnostic. By switching to readBestEffortConfig(), the install can proceed when the config is invalid, using whatever partial settings the snapshot could extract. If the gateway port or token configuration lives in the invalid portion of the file, resolveGatewayPort(cfg) will fall back to defaults, potentially installing the daemon on a different port or with a missing/incorrect token without surfacing any error to the operator.

The PR description states that best-effort reads are "used only where diagnostics still need to inspect a broken config," but runDaemonInstall, runDaemonRestart (lifecycle.ts), and runServiceRestart (lifecycle-core.ts) are all write/mutating paths. Failing with a clear error message on invalid config here aligns better with the "fail closed" intent and prevents silent mis-configuration.

Consider reverting these three call-sites to loadConfig() (which now correctly throws on invalid config) so the operator is forced to fix the config before mutating daemon state.
src/commands/status.agent-local.ts, line 36-38 (link)

Default parameter calls loadConfig() which now throws on invalid config

The previous implementation called loadConfig() unconditionally inside the function body; the new signature moves that call to a default parameter value. The runtime behavior for callers that omit cfg is identical — a throw on invalid config — but the new form is subtler to audit: the loadConfig import is retained solely to power a default that is bypassed by every internal call-site in this PR.

If the intent is that callers should always supply cfg (as all the updated call-sites now do), making the parameter required and removing the default would make that expectation explicit and eliminate the now-unused loadConfig import in the same file.

_{Last reviewed commit: 745bd17}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 609ea676ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

vincentkoc · 2026-03-07T18:47:08Z

@greptile review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 745bd17411

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eca5074ac7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e1bf67815

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-08T01:57:46Z

  }

-  const cfg = loadConfig();
+  const cfg = await readBestEffortConfig();


Fail daemon install when config read is unavailable

Switching this path to readBestEffortConfig() makes openclaw gateway install proceed when the config file cannot be read (for example EACCES on a root-owned config), because the snapshot fallback returns an empty config object instead of throwing. In that state install logic falls back to defaults and can produce a service install that is out of sync with the operator’s intended config/auth settings, whereas the previous loadConfig() path failed closed; install should still hard-fail on unreadable config and reserve best-effort behavior for read-only diagnostics.

Useful? React with 👍 / 👎.

* Config: fail closed invalid config loads * CLI: keep diagnostics on explicit best-effort config * Tests: cover invalid config best-effort diagnostics * Changelog: note invalid config fail-closed fix * Status: pass best-effort config through status-all gateway RPCs * CLI: pass config through gateway secret RPC * CLI: skip plugin loading from invalid config * Tests: align daemon token drift env precedence

* Config: fail closed invalid config loads * CLI: keep diagnostics on explicit best-effort config * Tests: cover invalid config best-effort diagnostics * Changelog: note invalid config fail-closed fix * Status: pass best-effort config through status-all gateway RPCs * CLI: pass config through gateway secret RPC * CLI: skip plugin loading from invalid config * Tests: align daemon token drift env precedence (cherry picked from commit 2c7fb54)

* Config: fail closed invalid config loads * CLI: keep diagnostics on explicit best-effort config * Tests: cover invalid config best-effort diagnostics * Changelog: note invalid config fail-closed fix * Status: pass best-effort config through status-all gateway RPCs * CLI: pass config through gateway secret RPC * CLI: skip plugin loading from invalid config * Tests: align daemon token drift env precedence

vincentkoc self-assigned this Mar 7, 2026

openclaw-barnacle Bot added cli CLI command changes commands Command implementations size: S maintainer Maintainer-authored PR labels Mar 7, 2026

vincentkoc marked this pull request as ready for review March 7, 2026 18:04

chatgpt-codex-connector Bot reviewed Mar 7, 2026

View reviewed changes

Comment thread src/commands/status-all.ts

chatgpt-codex-connector Bot reviewed Mar 7, 2026

View reviewed changes

Comment thread src/commands/status.scan.ts

vincentkoc force-pushed the vincentkoc-code/issue-28140-invalid-config-fail-closed branch from 745bd17 to 8b40b3c Compare March 7, 2026 23:51

openclaw-barnacle Bot added size: M and removed size: S labels Mar 7, 2026

vincentkoc force-pushed the vincentkoc-code/issue-28140-invalid-config-fail-closed branch from 8b40b3c to eca5074 Compare March 7, 2026 23:52

chatgpt-codex-connector Bot reviewed Mar 8, 2026

View reviewed changes

Comment thread src/cli/daemon-cli/lifecycle-core.ts Outdated

vincentkoc added 8 commits March 7, 2026 17:44

Config: fail closed invalid config loads

1110420

CLI: keep diagnostics on explicit best-effort config

03308a7

Tests: cover invalid config best-effort diagnostics

3ea96f6

Changelog: note invalid config fail-closed fix

fa7499d

Status: pass best-effort config through status-all gateway RPCs

7b09b29

CLI: pass config through gateway secret RPC

e07d7c7

CLI: skip plugin loading from invalid config

a17a677

Tests: align daemon token drift env precedence

2e1bf67

vincentkoc force-pushed the vincentkoc-code/issue-28140-invalid-config-fail-closed branch from eca5074 to 2e1bf67 Compare March 8, 2026 01:47

vincentkoc merged commit 2c7fb54 into main Mar 8, 2026
23 of 27 checks passed

vincentkoc deleted the vincentkoc-code/issue-28140-invalid-config-fail-closed branch March 8, 2026 01:48

chatgpt-codex-connector Bot reviewed Mar 8, 2026

View reviewed changes

hddevteam mentioned this pull request Mar 8, 2026

fix(msteams): pass teamId into inbound route resolution #39352

Closed

github-actions Bot mentioned this pull request Mar 8, 2026

上游更新: v2026.3.7 — 19 P0 + 122 P1 待合并 jiulingyun/openclaw-cn#486

Open

alexey-pelykh mentioned this pull request Mar 22, 2026

Cherry-pick issue #887: upstream CLI commands updates remoteclaw/remoteclaw#1791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Config: fail closed invalid config loads#39071

Config: fail closed invalid config loads#39071
vincentkoc merged 8 commits intomainfrom
vincentkoc-code/issue-28140-invalid-config-fail-closed

vincentkoc commented Mar 7, 2026

Uh oh!

greptile-apps Bot commented Mar 7, 2026 •

edited

Loading

Comments Outside Diff (3)

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

vincentkoc commented Mar 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Mar 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vincentkoc commented Mar 7, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Uh oh!

greptile-apps Bot commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Comments Outside Diff (3)

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

vincentkoc commented Mar 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps Bot commented Mar 7, 2026 •

edited

Loading