Skip to content

Make A2UI dataModelUpdate.valueArray first-class#805

Merged
shanselman merged 2 commits into
openclaw:mainfrom
bkudiess:bkudiess-a2ui-canvas-parity
Jun 25, 2026
Merged

Make A2UI dataModelUpdate.valueArray first-class#805
shanselman merged 2 commits into
openclaw:mainfrom
bkudiess:bkudiess-a2ui-canvas-parity

Conversation

@bkudiess

@bkudiess bkudiess commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

The v0.8 dataModelUpdate.valueArray typed value was silently dropped by the native WinUI A2UI parser — DataModelEntry had no ValueArray field and ToJsonNode() returned null for it. Only valueString/valueNumber/valueBoolean/valueMap were handled.

This broke seeding an array into a surface's data model — most visibly a multi-select MultipleChoice bound to a path could never be pre-populated — even though valueArray is part of the v0.8 protocol (docs/a2ui/protocol.md §2.2 and docs/a2ui/data-and-actions.md).

What changed

  • Parser (A2UIProtocol.cs): add DataModelEntry.ValueArray; ToJsonNode() emits a real JsonArray; new ParseValueArray/ParseArrayElement handle value-typed object elements ({"valueString":"x"}), bare-primitive tolerance (["a",1,true]), nested maps/arrays, and preserve a JSON null (or value-less {}) as a stable index slot so position-sensitive consumers don't see shifted indices.
  • DoS guard (DataModelStore.cs): the existing 32-deep depth bound now recurses valueArray as well as valueMap, so deep nesting can't be smuggled in through an array.
  • Security (SecretRedactor.cs): the JsonArray redaction branch now checks each element path against the registered/denylist set before recursing (mirroring the object branch). Previously arrays only recursed, so a secret seeded into an array (e.g. an obscured TextField bound to /codes/0) could leak through canvas.a2ui.dump.
  • Docs (SKILL.md): correct the stale "arrays are not first-class" note.

Why it's safe

  • Purely additive on the wire; existing scalar/valueMap behavior is unchanged and regression-tested.
  • System.Text.Json TryGetValue<T> is strict (no cross-kind coercion), so the bare-primitive string→bool→number detection can't mis-tag a value.
  • Malformed "both map and array set" deterministically prefers the map; never throws.
  • The redaction change can only over-redact, never under-redact.

Tests

  • A2UIDataModelArrayTests.cs (new) — parser coverage: strings, mixed scalars, maps, nested arrays, bare primitives, empty, and null/value-less slots, plus valueMap/scalar regressions.
  • SecretRedactorTests.cs — array-redaction regressions: registered element, registered array parent, denylisted scalar inside an array.
  • A2UIDataModelStoreTests.cs (new) — store base-path landing + valueArray depth-guard rejection + valueMap depth regression.
  • A2UIControlMatrixTests.cs — end-to-end: MultipleChoice (multi) seeded via valueArray asserts both the snapshot array and the rendered ListView preselection (parser → store → renderer round-trip).

Validation

./build.ps1 (all projects), OpenClaw.Shared.Tests, OpenClaw.Tray.Tests, and the A2UI OpenClaw.Tray.UITests all pass (OPENCLAW_REPO_ROOT set per AGENTS.md).

Proof

I ran temporary proof-only harnesses locally to print the actual behavior, then removed those harnesses so they are not part of this PR.

Parser + redaction behavior

Input behavior exercised:

{"dataModelUpdate":{"surfaceId":"s","path":"/form","contents":[{"key":"picked","valueArray":[{"valueString":"red"},{"valueString":"blue"}]}]}}

Terminal output from the proof run:

valueArray path: /form
valueArray stored JSON: ["red","blue"]
registered secret paths: /codes/0
redacted dump JSON: {"codes":["[REDACTED]","5678"]}

This proves the parser stores a real JSON array at the scoped path and that a registered secret array element is redacted instead of leaking through dump/snapshot output.

WinUI rendered state behavior

The WinUI proof seeded a multi-select MultipleChoice with:

{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"picked","valueArray":[{"valueString":"g"},{"valueString":"b"}]}]}}

Terminal output from the proof run:

snapshot dataModel.picked JSON: ["g","b"]
rendered ListView selected tags: b,g

This proves the value flows through parser → data model store → surface snapshot → native WinUI renderer: the snapshot carries the JSON array and the rendered ListView has Green/Blue preselected.

Known limitations (pre-existing, out of scope)

  • Nested array/map breadth is bounded only by the 1 MiB line cap — identical to the existing valueMap behavior; a shared element-count budget would be a separate change.
  • BuildActionContext deep-clones a non-secret subtree without redacting nested secret-named descendants — pre-existing, applies to valueMap too, not introduced here.

@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 25, 2026, 4:08 PM ET / 20:08 UTC.

Summary
The PR adds A2UI valueArray parsing/storage, array depth guarding, array-element secret redaction, updated A2UI skill docs, and parser/store/redaction/WinUI tests.

Reproducibility: yes. Source inspection of current main shows docs require valueArray while DataModelEntry, ParseEntry, and ToJsonNode omit it, so a dataModelUpdate with only valueArray stores null instead of a JsonArray.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 9 files changed, +498/-9. The patch is focused on A2UI parser/storage/redaction/docs/tests but broad enough to warrant normal maintainer review.
  • Check rollup: 9 success, 6 skipped, 0 failing. The current GitHub check state supports merge-readiness review, though it does not replace maintainer approval.

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.

Mantis proof suggestion
A short visible WinUI proof would help maintainers inspect the path-bound MultipleChoice preselection behavior directly. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify that WinUI A2UI MultipleChoice bound to /picked is preselected when dataModelUpdate seeds /picked with valueArray ["g","b"].

Next step before merge

  • No automated repair is needed; with no concrete patch defects found, the remaining action is normal maintainer review or merge.

Security
Cleared: Cleared: the diff adds no dependencies, workflows, credential sources, or package-resolution changes, and the redaction update narrows an array-element leak path.

Review details

Best possible solution:

Land the focused protocol-parity fix after normal maintainer review, keeping valueArray handling in the existing A2UI parser/store/redaction path.

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

Yes. Source inspection of current main shows docs require valueArray while DataModelEntry, ParseEntry, and ToJsonNode omit it, so a dataModelUpdate with only valueArray stores null instead of a JsonArray.

Is this the best way to solve the issue?

Yes. Extending the existing typed-value parser, store depth guard, redactor, and focused A2UI tests is the narrow maintainable fix for the documented protocol behavior.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This fixes a documented A2UI parser/storage/redaction bug with limited blast radius to WinUI A2UI data-model updates.
  • 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. Sufficient (live_output): Sufficient: the PR body includes copied after-change live output showing parser/storage/redaction behavior and WinUI MultipleChoice preselection through the real A2UI path.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient: the PR body includes copied after-change live output showing parser/storage/redaction behavior and WinUI MultipleChoice preselection through the real A2UI path.
Evidence reviewed

What I checked:

Likely related people:

  • codemonkeychris: Authored the merged native WinUI A2UI renderer work that introduced the parser, data model store, secret redactor, and UI test surface touched by this PR. (role: feature introducer; confidence: high; commits: 9fa43f347703; files: src/OpenClaw.Tray.WinUI/A2UI/Protocol/A2UIProtocol.cs, src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs, src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs)
  • shanselman: Merged the native WinUI A2UI renderer PR and has adjacent test-safety history near this feature area. (role: reviewer and merger; confidence: medium; commits: 9fa43f347703, 394695cc3331; files: src/OpenClaw.Tray.WinUI/A2UI/Protocol/A2UIProtocol.cs, tests/OpenClaw.Tray.UITests/A2UIControlMatrixTests.cs)
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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jun 23, 2026
@bkudiess bkudiess changed the base branch from master to main June 23, 2026 19:29
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 23, 2026
@bkudiess bkudiess force-pushed the bkudiess-a2ui-canvas-parity branch from c224d26 to 12d2492 Compare June 23, 2026 19:37
@clawsweeper clawsweeper Bot removed merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jun 23, 2026
@bkudiess bkudiess force-pushed the bkudiess-a2ui-canvas-parity branch 4 times, most recently from c962797 to a171762 Compare June 25, 2026 18:44
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 25, 2026
@bkudiess bkudiess marked this pull request as ready for review June 25, 2026 20:04
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 25, 2026
00sadik-lab and others added 2 commits June 25, 2026 15:33
The v0.8 `dataModelUpdate.valueArray` typed value was silently dropped by the
WinUI parser: `DataModelEntry` had no `ValueArray` field and `ToJsonNode()`
returned null for it. Only `valueString/Number/Boolean/valueMap` were handled.
This broke seeding an array into a surface's data model (e.g. a multi-select
`MultipleChoice` bound to a path), even though `valueArray` is part of the v0.8
protocol (docs/a2ui/protocol.md §2.2 and data-and-actions.md).

Changes:
- Parser (A2UIProtocol.cs): add `DataModelEntry.ValueArray`; `ToJsonNode()`
  emits a `JsonArray`; new `ParseValueArray`/`ParseArrayElement` handle
  value-typed object elements, bare primitives (`["a",1,true]`), nested
  maps/arrays, and preserve JSON null as a stable index slot.
- DoS guard (DataModelStore.cs): the 32-deep depth bound now recurses
  `valueArray` as well as `valueMap`.
- Security (SecretRedactor.cs): the array branch now redacts registered/
  denylisted element paths instead of only recursing, so a secret seeded into
  an array (e.g. an obscured field bound to /codes/0) no longer leaks via
  canvas.a2ui.dump.
- Docs (SKILL.md): correct the stale "arrays are not first-class" note.

Tests: parser coverage for valueArray (strings, mixed scalars, maps, nested
arrays, bare primitives, empty, null slots) plus valueMap/scalar regressions;
secret-redaction regressions for secrets inside arrays; store-level base-path
landing and depth-guard rejection; and an end-to-end MultipleChoice surface
seeded via valueArray that asserts both the snapshot and the rendered
preselection.

Validation: build.ps1 (all projects), Shared.Tests, Tray.Tests, and A2UI
UITests all pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject non-canonical array pointer indices for data-model array access, notify descendant subscribers when container values are replaced, and redact registered secret descendants when action contexts or snapshots include parent paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@shanselman shanselman force-pushed the bkudiess-a2ui-canvas-parity branch from a171762 to 1ffb6bc Compare June 25, 2026 23:06
@shanselman shanselman merged commit ee8fc93 into openclaw:main Jun 25, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants