Repository navigation
fix(config): save config changes into the nested $include file that owns them - #116108
Conversation
|
Codex review: blocked before merge. Reviewed September 6, 2026, 11:47 PM ET / September 7, 2026, 03:47 UTC. ClawSweeper reviewWhat this changesRoutes configuration edits and Doctor repairs into the nested include file that owns them, preserves neighboring include pointers, and reports refused writes accurately. Merge readiness⛔ Blocked before merge - 2 items remain Keep open: nested include write-through remains absent from current main and the latest release. The prior ownership findings are resolved, and the updated evidence addresses the previous validation gap. No new actionable patch defect was found. Priority: P2 Review scores
Verification
How this fits togetherOpenClaw combines the root configuration and included fragments into one configuration snapshot. Configuration commands and Doctor use that snapshot to decide which authored file can safely receive a change. flowchart TD
A[Root configuration and fragments] --> B[Snapshot with include ownership]
C[Configuration edit or Doctor repair] --> D[Resolve changed paths]
B --> D
D --> E{One eligible owner?}
E -->|Yes| F[Recheck graph and write fragment]
E -->|No| G[Guarded root write or clear refusal]
F --> H[Reload and report saved result]
Before merge
Agent review detailsSecurityNone. PR surfaceSource +388, Tests +1700, Docs +27, Other -1. Total +2114 across 33 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Keep one shared ownership resolver for configuration writes and Doctor, preserving authored fragments and refusing ambiguous or concurrently changed ownership. Do we have a high-confidence way to reproduce the issue? Not applicable as an approved capability extension: main and v2026.9.2 explicitly limit writes to top-level includes, while the captured CLI evidence demonstrates the nested limitation and successful write-through afterward. Is this the best way to solve the issue? Yes. Sharing ownership resolution between the writer and Doctor extends the existing persistence path without adding configuration switches or a competing multi-file writer. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6e8fd6c41f6a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (32 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
914126e to
1c9a750
Compare
…wns them Config writes only routed into an $include file when the directive sat at a top-level key. A directive nested deeper, such as a per-agent agents.entries.<id> include, fell through to the root write, which refused the write rather than flatten the included content. - Resolve the deepest authored include that solely owns every changed path, using the include provenance already recorded while reading the snapshot. - Require every enclosing include to be a sole owner, so a merged or sibling-overridden ancestor still declines write-through. - Keep array-entry includes, root-level changes, and changes spanning an include boundary on the existing root write path. - Retain the parsed-directive fallback for snapshots without provenance. Verified: new nested write-through and out-of-boundary cases in src/config/mutate.test.ts fail without this change; 17 boundary-resolution cases in src/config/include-write-boundary.test.ts; runtime-config shard 214 files / 2967 tests pass; pnpm build, pnpm lint, tsgo:core, tsgo:ui and check:architecture pass.
…ough Read-time roster migration adds keys to the resolved source config that the authored include file does not carry, so the pre-write equality check rejected every nested include write with "included config changed since last load". - Compare the authored include content against sourceConfigBeforeMigrations when the snapshot carries it, falling back to sourceConfig. - Add a regression case covering a migration-added key at the boundary. Verified: openclaw config set against a real nested include layout now writes the included file and leaves the root $include directive in place.
1c9a750 to
fdc6df8
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
- src/config/mutate.ts: keep upstream's persistCanonicalAgentRoster root-write guard in front of the changed-path include check; pre-migration source comparison reads the boundary path. - src/commands/doctor-config-flow.ts: keep upstream's persistCanonicalAgentRoster result field beside includeBoundaryWrite; singleTopLevelIncludeWrite removed. - src/commands/doctor-config-flow.test.ts: upstream mock without the "issues" preflight mode; includeProvenance passthrough kept; Doctor wiring tests use a retired tuning knob inside an include as the repair vehicle. - resolveIncludeWriteBoundary declines every boundary beneath a root-level $include (documented read-only topology). - configWriteTargetsIncludeBoundary declines when persistCanonicalAgentRoster is set or the candidate no longer carries the boundary value, matching the writer; Doctor passes the flag. - Root writer include-flatten refusals are typed (createConfigIncludeOwnershipError, carrying the owned config path and the authored $include target); the Doctor write runner records them as configWriteRefusal "include-ownership", names the included file to repair by hand, and keeps the queued repair panels unprinted instead of surfacing a raw error. - docs/gateway/configuration.md, configuration-reference.md, opentelemetry.md: Doctor refuses a run that mixes a root-owned repair with an include-owned repair. - extensions/file-transfer/src/cli.test.ts: provenance fixture carries hasArrayAncestor. - docs/cli/plugins.md: nested write-through wording for plugins commands.
…ter and Doctor eligibility - `resolveIncludeOwnedWriteCandidate` is the one decision consumed by `tryWriteIncludeOwnedConfigMutation` and `configWriteTargetsIncludeBoundary`; `getIncludeMutationBoundary` inlined - keyed config-path reads and writes use the existing `config-paths.ts` helpers; `readConfigPathValue`, `writeConfigPathValue`, `hasConfigPathValue` and their tests removed - an in-memory candidate carrying an own `undefined` at the boundary now takes the root writer instead of serializing `undefined` into the include file; authored JSON5 configs behave as before - top-level parsed-directive fallback for snapshots without recorded provenance kept (openclaw#114251) - docs: root includes read-only means every section of a config whose root object authors `$include` Refs openclaw#116107
Keep array ancestry optional for existing SDK callers, reuse include detection and overlap ownership logic, and cover Doctor fragment writeback with unchanged ancestor files. The broader nested-write PR remains held for ownership review and its positive production LOC delta.
Merge current main while preserving the contributor's commits, then reject fragment files shared by other logical owners and revalidate the captured include graph before persistence, after writing, and after reload. Preserve main's authority guards and roster projection order. Keep hasArrayAncestor optional for existing snapshot callers. The feature-class production growth is maintainer-approved. Independent review, formatting, and the scoped whitespace check passed. Runtime validation is pending the exact-head hosted gate after local Testbox synchronization failed before tests started. Co-authored-by: Sasan Sotoodehfar <sasan1200@gmail.com>
|
@clawsweeper re-review The maintainer continuation at 2d85efc resolves the two recorded ownership findings: canonical targets shared by another logical subtree are refused, and captured include targets and bytes are revalidated around persistence and after reload. Conditional rollback preserves concurrent edits. Mainline conflicts are resolved while retaining Sasan's original commit ancestry and current-main authority and roster-projection behavior. The feature-class production growth is approved by the maintainer; |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Change only the intended plugin leaf so the regression fixtures reach the include ownership boundary without introducing unrelated root changes. The pre-repair implementation now fails all five intended ownership cases, while the unchanged delegation control passes. The candidate passes all 100 focused config and Doctor tests on Testbox. Co-authored-by: Sasan Sotoodehfar <sasan1200@gmail.com>
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
The compatibility and stored-data review items are addressed in the body for 80a755a. Existing formats and canonical keys are retained; There is no SQLite schema, persisted config key, or new migration transform. The existing Doctor migration is persisted into its owning fragment; the real preparation/write fixture proves canonical |
|
Merged via squash.
|
Closes #116107
What Problem This Solves
Fixes an issue where users split configuration into nested
$includefragments and config commands ordoctor --fixcannot save changes to the owning file. The old writer recognized only top-level include ownership and correctly refused to flatten nested fragments into the root.Why This Change Was Made
Config writes and Doctor share one resolver that selects the deepest sole include owner. It accounts for every logical owner of the canonical target, including filesystem aliases and merged includes, so updating one subtree cannot silently alter another. The writer revalidates captured include targets and contents around persistence and after reload, with conditional rollback if an intermediate file changes. Current-main authority checks and roster projection order are preserved.
Doctor omits root wizard metadata for an include-only repair and records refused persistence accurately. Root-owned keyed-agent sibling additions preserve existing include pointers. The exported
ConfigIncludeOwnership.hasArrayAncestorremains optional, with absence meaning false.The maintainer approved this as a feature-class change on September 6, including positive production growth. No new config option, dependency, schema, protocol, or multi-file write mechanism is introduced. The mainline conflict was resolved with a merge so Sasan's original commits and authorship remain intact, as required by the native landing ancestry gate.
Compatibility review: existing authored config formats and canonical keys are retained.
hasArrayAncestoris optional snapshot metadata, so existing callers that omit it remain valid. Existing sole-owner top-level writes remain covered by the focused suite. Shared physical targets now intentionally refuse writes that could alter another logical owner; operators can edit those files directly or give each subtree its own fragment.Stored-data review: this adds no SQLite schema, persisted config key, or new migration transform. Existing Doctor migrations now persist their already-canonical result in the owning fragment. The nested
sandbox.perSessiontosandbox.scopefixture proves that upgrade repair with root and intermediate bytes preserved; the companion fixture proves mixed-owner repairs are refused without changing authored files. The maintainer's September 6 feature approval covers this persistence capability.User Impact
Eligible nested fragments receive their own changes while root and ancestor files retain their authored bytes. Root includes, array and merged ownership, sibling overrides, shared physical targets, external write targets, and mutations spanning multiple owners remain refused. Concurrent include-graph changes cannot be reported as a successful write to an inactive fragment.
Thanks to @sasan1200 (Sasan Sotoodehfar) for the original implementation.
Evidence
git diff --checkand focused formatting passed.src/config/mutate.test.ts,src/config/include-write-boundary.test.ts, andsrc/commands/doctor-config-flow.include-refusal.test.tspassed on Testbox. This includes the real Doctor write-stage repair with root and intermediate files byte-preserved.2d85efcc8e6f7b0cba4d66fec21a0a2cdd219bb2and applied the exact test-only follow-up now committed as80a755a50d998e71fa59896d799a9f172dd53344; its test-file SHA-256 matches the committed file. No other source files differ. The successful command exit/timing record owns this proof; stopping the leased runner can mark the linked Actions workflow cancelled.pnpm check:changed --timed -- <all 33 PR paths>passed on the same Testbox candidate, including core and test types, lint, baseline guards, and import cycles. The complete proof driver exited 0..80a755a50d998e71fa59896d799a9f172dd53344, after one unchanged-target rerun of a Gateway mock-binding failure outside the fragment worker. The earlier candidate's three fixture failures were corrected by the reviewed test-only follow-up. No assertions or gates were weakened.Existing full CLI fragment proof
The synthetic
pnpm openclaw doctor --fix --non-interactivefixture was previously run against pre-fix commitd4eb1353071c0bc8e2d81c799b72da28d80c8129and contributor continuation67d1c02da5351095c68e55521bb31a850fca0038:agents.entries.main.sandbox.perSessionwas not migrated; root and intermediate files were unchanged.sandbox.scope; root and intermediate files were byte-identical.src/commands/doctor-config-flow.include-refusal.test.ts, including its refusal sibling. Its refreshed result is included in the Testbox proof above.