fix(web_core): reject writes through a primitive value consistently - #2499
Conversation
`DataModel.set` walks to the container holding the last segment and then branches on whether it is a map or a list. When it was neither — the path ran through a primitive, or the root itself was one — both branches were skipped and the method returned normally, having written nothing. The write was reported as applied while the value never landed, so the client held state the agent believed it had set, with no error anywhere to explain the difference. Intermediate segments already threw for this; only the final container did not. It now throws `A2uiDataError`, which is what `set` does everywhere else it cannot honour a path, and matches how web_core answers the same write.
`DataModel.set` guarded the root with `if (!this.data) this.data = {}`,
which is a truthiness test, so the root was replaced whenever it held a
falsy value. Setting the root to `false`, `0` or `''` and then writing
any path silently discarded that root and started a fresh object.
A truthy primitive root took the other path and reached
`current[lastSegment] = value` on a number or string, which in a module
(strict mode) throws a raw `TypeError` — outside the `A2uiError`
hierarchy consumers catch, and not what the method's own documentation
promises.
The root is now replaced only when it is absent, and a primitive root
raises `A2uiDataError`, the same answer this method already gives for a
primitive at any other depth.
There was a problem hiding this comment.
Code Review
This pull request updates the DataModel implementation in both Dart and TypeScript to prevent writing paths through primitive values. Specifically, attempting to write a path when the containing value or the root itself is a primitive now throws an A2uiDataError instead of silently dropping the write or replacing the root. Unit tests have been added to verify this behavior. There are no review comments, so I have no feedback to provide.
|
Re-reviewed this change against a wider set of neighbouring cases, since it turns silence into errors in two implementations at once. Twelve cases, run through both, agreeing on all twelve: Still accepted, as before:
Now rejected, with the prior state left intact in every case: a write through a nested primitive, a primitive root, a falsy root ( The one I most wanted to rule out was a partially applied write — the walk creates intermediate containers as it goes, so an error at the last segment could in principle leave those behind. It cannot: containers are only created where nothing exists, and once a fresh container is created everything below it is empty, so no primitive can be met further down the same path. Confirmed by case 8 above — One adjacent case this PR deliberately does not address, found while checking the above. In Dart, a consumer that constructs the model with a strongly typed map gets a raw error rather than an DataModel(<String, String>{'a': 'x'}).set('/a', 2);
// _TypeError: type 'int' is not a subtype of type 'String' of 'value'It is the same shape as the |
…tive-writes # Conflicts: # dart/a2ui_core/lib/src/core/data_model.dart
|
Rebased onto main now that #2439 has landed, and the Dart half of this PR is no longer mine to make: #2439 fixes the same silent write in What is left here:
So this went from "fix two implementations" to "fix web_core, and add the regression tests for the Dart side that just landed". Happy to split the Dart tests into their own PR if that reads better. |
|
Hi @gspencergoog, I have pulled and verified the changes locally, and all tests pass as expected. Could you please review this PR? |
| expect(model.get('/'), isEmpty); | ||
| }); | ||
|
|
||
| test('rejects writes through a primitive value', () { |
There was a problem hiding this comment.
This is a Dart test, but the rest of the code changes are web_core changes. Did you mean to separate it like that?
There was a problem hiding this comment.
Fair question — it did not start that way, and the split is a leftover from a rebase.
The PR originally fixed the same bug in both: Dart's DataModel.set walked to the container for the last segment, found neither a map nor a list, and returned normally having written nothing, so the agent got no error and the value simply was not there. web_core had two variants of the same hole — a raw TypeError on a primitive root, and a root holding false, 0 or '' being silently discarded, because the guard was if (!this.data).
Then #2439 landed with the same Dart fix, at the same branch, raising the same A2uiDataError. I dropped my version of that hunk and kept its wording, which left what you are looking at: the web_core fix, plus the Dart tests, which now cover #2439's fix rather than my own. They assert the error type rather than the message, so they passed against that wording unmodified — and the case they pin, a write when the root itself is a primitive, has no test on main.
So the Dart test is deliberate but not original intent. Happy to split it into its own PR if you would rather review them separately; it is two tests and it moves cleanly.
There was a problem hiding this comment.
No, as long as it is adding to the Dart test corpus, and not a mistake, then this is fine to keep here.
|
Can you update the PR description with the actual cases addressed by this PR? |
|
It is there, but the table made it look like two separate changes when it is one branch, which is a fair thing to trip over. - if (!this.data) {
+ if (this.data === undefined || this.data === null) {
this.data = {};
+ } else if (typeof this.data !== 'object') {
+ throw new A2uiDataError(
+ `Cannot set path '${path}': the data model root is a primitive value.`,
+ path,
+ );
}The old guard was a truthiness test, so a primitive root took one of two paths depending on whether it was truthy:
Splitting the condition into "absent" and "primitive" handles both: only The Rewriting the description now: it still describes the PR as it was before the rebase, when it also carried the Dart fix that #2439 has since landed. |
|
Could you please identify the agent, model, and prompt used to build this PR? |
|
Claude Opus 5, through Claude Code in its ultracode mode. Not a single prompt — a series across one session: locating the divergence, measuring it, writing the fix, and reviewing the result. The factual claims were produced by running them rather than asserted: the 1,392-program differential behind #2498, the six-root table above, and the test counts. I reviewed and directed each step before it went up, and I am the one answering review here. |
Cool, thanks, I was curious which you were using. Mostly I was curious if you would respond, or a bot. :-) |
|
Hahaha, I was watching to see where this was going 😂 |
Description
Addresses the web_core half of #2498. Rewritten after rebasing: this PR originally fixed the same bug in Dart and TypeScript, and #2439 has since landed the Dart fix, so what is left is web_core plus the Dart tests for the fix that arrived without any.
What this PR changes
One branch in
DataModel.set, which is why the two rows below are a single diff hunk:The old guard was a truthiness test, so a primitive root behaved differently depending on whether it happened to be truthy:
set('/', 1),set('/a', 2)TypeError: Cannot create property 'a' on number '1'A2uiDataError, root preservedset('/', false),set('/a', 2){a: 2}A2uiDataError, root preservedA truthy primitive root fell through to
current[lastSegment] = valueon a number or a string; modules run in strict mode, so that throws a rawTypeError— outside theA2uiErrorhierarchy consumers catch, and not what the method's own doc comment promises:A falsy primitive root took the other path:
!this.datawas true, so a root holdingfalse,0or''— a value the caller put there — was silently replaced with a fresh object.Verified against the built code, all six primitive roots now behave the same way and keep their value:
Two Dart tests, covering the fix #2439 landed rather than one of mine. They assert the error type rather than its message, so they pass against that wording unmodified, and the case they pin — a write when the root itself is a primitive — had no test on main. Confirmed with @gspencergoog in review that keeping them here is fine.
What this PR no longer changes
The Dart fix in
DataModel.set. #2439 landed the same correction at the same branch, raising the sameA2uiDataError; I kept its wording and dropped mine.What is not addressed here
Three findings from #2498 are decisions rather than defects, and none is in this PR:
set(path, null)removes the key in Dart and stores the null in web_core — 137 of the 221 diverging programs. The v0.9 schema distinguishes the two intents by whethervalueis present, but Dart'sDataModelhas no way to express that, andtest/data_model_test.dartpins the removal deliberately. Fixing it means splitting the setter from a remover and havingMessageProcessorbranch on presence.__proto__is rejected by web_core and stored as an ordinary key by Dart.Tests
Two per language, all four failing on
mainbefore the respective fix:A2uiDataErrorrather thanTypeError, and each falsy root (false,0,'') surviving a rejected write.A2uiDataErrorwith the untouched value still readable.node --test "dist/**/*.test.js"inrenderers/web_core: 421 passing.eslint src/v0_9/state/reports 33 warnings, exactly as on main.dart testindart/a2ui_core: 324 passing;dart analyzeanddart formatclean.Compatibility
Both changes turn silence into an error, so a client that was quietly losing writes will now hear about them. No existing test in either package expected the old behaviour.
Pre-launch Checklist
One time:
For this PR: