Skip to content

fix(config): restrict config paths to own properties#99846

Merged
vincentkoc merged 1 commit into
mainfrom
fix/config-path-own-properties
Jul 4, 2026
Merged

fix(config): restrict config paths to own properties#99846
vincentkoc merged 1 commit into
mainfrom
fix/config-path-own-properties

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Related: #99148

What Problem This Solves

Fixes an issue where config path operations could treat inherited object properties as persisted config. Reads could expose inherited values, unsets could report missing prototype-named keys as deleted, and sets could traverse inherited branches or invoke inherited descriptors instead of creating owned config data.

Why This Change Was Made

Config path reads, writes, and removals now operate on own properties only. Set operations define canonical enumerable, writable, configurable own properties, so inherited data, setters, getters, and non-writable descriptors cannot intercept config writes.

This completes the invariant identified by @zenglingbiao in #99148. The original fork disabled maintainer edits, so this replacement preserves the contributor as commit author while covering the full shared helper surface.

User Impact

Config show/unset behavior is accurate for prototype-named keys, and config mutations cannot follow, alter, or invoke inherited prototype state.

Evidence

  • Before: inherited values were readable, inherited leaves could be reported as deleted, inherited object parents could be traversed, and inherited setters/non-writable properties could intercept or reject writes.
  • After: reads and removals require owned segments; setters create owned properties without invoking inherited descriptors.
  • node scripts/run-vitest.mjs src/config/config-paths.test.ts src/config/config-misc.test.ts src/config/runtime-overrides.test.ts - 106 tests passed.
  • .agents/skills/autoreview/scripts/autoreview --mode local --stream-engine-output - clean, no accepted/actionable findings, 0.94 confidence.
  • Testbox tbx_01kwnxv1admc2fnvekh4gxm5k6: pnpm check:changed passed for core and core tests, including typecheck, lint, import-cycle, and guard lanes.
  • Signed prepared head: a3bf04584d44dc840e8186b926038e4e98a31321, based on landed main 03fafe2364948dc58e63e7d28ebb36b432c23b4d.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Jul 4, 2026
@vincentkoc vincentkoc self-assigned this Jul 4, 2026
@vincentkoc

vincentkoc commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Land-ready proof for prepared head a3bf04584d44dc840e8186b926038e4e98a31321:

  • Rebased on landed main 03fafe2364948dc58e63e7d28ebb36b432c23b4d; the later LINE-only 02b529a8ccf main drift has no path overlap.
  • Focused regression: node scripts/run-vitest.mjs src/config/config-paths.test.ts src/config/config-misc.test.ts src/config/runtime-overrides.test.ts - 106/106 passed.
  • The final patch covers inherited data properties, setters, getters, and non-writable descriptors at both parent and leaf write positions.
  • Testbox tbx_01kwnxv1admc2fnvekh4gxm5k6: pnpm check:changed passed for core and core tests, including typecheck, lint, import-cycle, and guard lanes.
  • Hosted exact-head CI passed with zero failures: https://github.com/openclaw/openclaw/actions/runs/28698089567
  • oxfmt, oxlint, and git diff --check passed.
  • Fresh autoreview: clean, no accepted/actionable findings; confidence 0.94.
  • Review artifacts validated with recommendation READY FOR /prepare-pr and zero findings.
  • scripts/pr prepare-run 99846 passed with hosted exact-head gates.
  • Commit authorship remains @zenglingbiao; this replacement is required because fix(config): use Object.hasOwn instead of in operator in unsetConfigValueAtPath #99148 disabled maintainer edits and the complete invariant spans get, set, and unset.

No known proof gaps remain for the changed surface.

@vincentkoc
vincentkoc marked this pull request as ready for review July 4, 2026 06:24
@clawsweeper

clawsweeper Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 2:40 AM ET / 06:40 UTC.

Summary
The branch changes shared config path get, set, and unset helpers to traverse own properties only and adds regression coverage for inherited prototype-named keys.

PR surface: Source +4, Tests +44. Total +48 across 2 files.

Reproducibility: yes. Source inspection of current main shows get, set, and unset can traverse inherited properties; I did not run tests in this read-only review.

Review metrics: 1 noteworthy metric.

  • Config path helpers hardened: 3 changed: get, set, unset. The maintainer replacement intentionally fixes the full shared helper invariant rather than only the unset leaf check.

Root-cause cluster
Relationship: canonical
Canonical: #99846
Summary: This PR is the canonical maintainer-owned replacement for inherited-property config path traversal; earlier PRs cover only the unset helper or an adjacent redaction helper.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

  • [P2] No repair lane is needed; this maintainer-labeled PR has no actionable findings and only needs normal maintainer landing plus related-PR reconciliation.

Security
Cleared: The diff narrows prototype-chain traversal in config helpers and does not change dependencies, workflows, credentials, or package resolution.

Review details

Best possible solution:

Land this broader shared-helper own-property fix after exact-head gates, then close or reconcile the narrower predecessor PRs against the landed commit.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection of current main shows get, set, and unset can traverse inherited properties; I did not run tests in this read-only review.

Is this the best way to solve the issue?

Yes. Fixing the shared helper for get, set, and unset is narrower and more maintainable than caller-side checks or the unset-only predecessor.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against cafbd745c1b1.

Label changes

Label justifications:

  • P2: This is a bounded config-helper correctness and hardening fix with direct current-main source evidence and limited blast radius.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor real-proof gate does not apply to this maintainer-labeled replacement PR, though the body and comment include focused validation and Crabbox proof references.
Evidence reviewed

PR surface:

Source +4, Tests +44. Total +48 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 11 7 +4
Tests 1 44 0 +44
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 55 7 +48

What I checked:

  • Repository policy applied: Full root AGENTS.md was read; no scoped AGENTS.md exists under src/config, so root config-sensitive PR review guidance applies. (AGENTS.md:1, cafbd745c1b1)
  • Current main bug path: Current main still reads intermediate path segments with cursor[key], checks unset leaves with leafKey in cursor, and reads values without an own-property guard. (src/config/config-paths.ts:63, cafbd745c1b1)
  • User-facing caller: The /config unset mutation path delegates to unsetConfigValueAtPath; its false return drives the no-value-found user response. (src/auto-reply/reply/config-mutations.ts:41, cafbd745c1b1)
  • Sibling own-property pattern: Adjacent config write path existence/deletion helpers already guard object paths with Object.hasOwn-style checks, supporting the shared-helper fix shape. (src/config/io.write-prepare.ts:799, cafbd745c1b1)
  • PR diff coverage: The PR changes setConfigValueAtPath, unsetConfigValueAtPath, and getConfigValueAtPath to require own properties during traversal, with tests for inherited leaf and parent branches. (src/config/config-paths.ts:40, e526054a6c31)
  • Latest release still has old behavior: The latest release tag v2026.6.11 still contains the old leafKey in cursor check, so this repair is not already shipped. (src/config/config-paths.ts:63, e085fa1a3ffd)

Likely related people:

  • steipete: The original /config chat config update introduced src/config/config-paths.ts and the chat config mutation surface that consumes it. (role: introduced behavior and adjacent owner; confidence: high; commits: 8b579c91a555, 08e020881d6e; files: src/config/config-paths.ts, src/auto-reply/reply/config-mutations.ts, src/auto-reply/reply/commands-config.ts)
  • Dallin Romney: Current blame for the helper lines points to the refactor/extraction commit that carried the existing config path behavior into the current files. (role: recent refactor author; confidence: medium; commits: 13652f6654da; files: src/config/config-paths.ts, src/auto-reply/reply/config-mutations.ts)
  • vincentkoc: Recent merged work touched the /config command authorization surface that consumes these helpers, and this PR coordinates the maintainer-owned replacement path. (role: recent adjacent contributor; confidence: medium; commits: 08aa57a3de37; files: src/auto-reply/reply/commands-config.ts, src/auto-reply/reply/command-gates.ts)
  • zenglingbiao: Authored this fix commit and a merged adjacent own-property config redaction fix using the same JavaScript invariant. (role: current fix author and recent adjacent contributor; confidence: high; commits: e526054a6c31, 459b93e3ddaf; files: src/config/config-paths.ts, src/config/config-paths.test.ts, src/config/redact-snapshot.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 4, 2026
@vincentkoc
vincentkoc force-pushed the fix/config-path-own-properties branch from e526054 to a3bf045 Compare July 4, 2026 06:43
@vincentkoc
vincentkoc merged commit 2981425 into main Jul 4, 2026
103 checks passed
@vincentkoc
vincentkoc deleted the fix/config-path-own-properties branch July 4, 2026 06:52
@vincentkoc

vincentkoc commented Jul 4, 2026

Copy link
Copy Markdown
Member Author

Merged via squash.

This was referenced Jul 4, 2026
Closed
Closed
@github-actions github-actions Bot mentioned this pull request Jul 4, 2026
Open
This was referenced Jul 4, 2026
Closed
Closed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 5, 2026
@vincentkoc @zenglingbiao
fix(config): restrict config paths to own properties (openclaw#99846)
4d2fc9c 
Co-authored-by: zenglingbiao <[email protected]>
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @zenglingbiao