fix(liveobjects): reject live object references as map values#2246
fix(liveobjects): reject live object references as map values#2246sacOO7 wants to merge 1 commit into
Conversation
Passing an object obtained from the channel (an Instance or PathObject wrapper, or an internal LiveObject) as a value to LiveMap.set / Instance.set / PathObject.set, or as an entry value in LiveMap.create, was not caught by validation: the value fell through to the JSON encoding branch and failed at publish time with a confusing "TypeError: Converting circular structure to JSON" instead of a proper ErrorInfo. Per the objects spec, only the LiveMap.create() / LiveCounter.create() value types may be used to assign objects to map keys; references to existing objects are not an accepted value type (RTLMV4c, new RTLMV4c1). Reject them in LiveMap.validateKeyValue with an ErrorInfo (code 40013, statusCode 400), which covers all write paths - LiveMap.set, Instance.set, PathObject.set, BatchContext.set and LiveMap.create entry evaluation - since they share this validation. Co-Authored-By: Claude Fable 5 <[email protected]>
WalkthroughThis PR adds a validation guard to ChangesLiveMap Value Reference Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)test/realtime/liveobjects.test.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/realtime/liveobjects.test.js`:
- Around line 4090-4094: The new rejection tests for RTLMV4c1 only assert the
error message; update the calls to expectToThrowAsync used around map.set (the
three cases referencing map, entryInstance, entryPathObject) to also assert the
error object includes code: 40013 and statusCode: 400; specifically modify the
expectToThrowAsync invocations for these three assertions to verify both the
message ('Map value data type is unsupported') and that the thrown error has
code 40013 and statusCode 400 so the test enforces the PR's error-mapping
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 298cdd7c-589c-4679-bdb7-d8b545e4951d
📒 Files selected for processing (2)
src/plugins/liveobjects/livemap.tstest/realtime/liveobjects.test.js
| // RTLMV4c1 - references to existing objects (Instance or PathObject) are not valid | ||
| // map values; only LiveMap.create()/LiveCounter.create() value types can assign objects | ||
| await expectToThrowAsync(async () => map.set('key', map), 'Map value data type is unsupported'); | ||
| await expectToThrowAsync(async () => map.set('key', entryInstance), 'Map value data type is unsupported'); | ||
| await expectToThrowAsync(async () => map.set('key', entryPathObject), 'Map value data type is unsupported'); |
There was a problem hiding this comment.
Assert RTLMV4c1 error code/status in the new rejection checks.
These new cases currently verify only the message. Since this PR contract requires code: 40013 and statusCode: 400, add those assertions to prevent silent regressions in error mapping.
Suggested test tightening
- await expectToThrowAsync(async () => map.set('key', map), 'Map value data type is unsupported');
- await expectToThrowAsync(async () => map.set('key', entryInstance), 'Map value data type is unsupported');
- await expectToThrowAsync(async () => map.set('key', entryPathObject), 'Map value data type is unsupported');
+ await expectToThrowAsync(async () => map.set('key', map), 'Map value data type is unsupported', {
+ withCode: 40013,
+ withStatusCode: 400,
+ });
+ await expectToThrowAsync(async () => map.set('key', entryInstance), 'Map value data type is unsupported', {
+ withCode: 40013,
+ withStatusCode: 400,
+ });
+ await expectToThrowAsync(async () => map.set('key', entryPathObject), 'Map value data type is unsupported', {
+ withCode: 40013,
+ withStatusCode: 400,
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/realtime/liveobjects.test.js` around lines 4090 - 4094, The new
rejection tests for RTLMV4c1 only assert the error message; update the calls to
expectToThrowAsync used around map.set (the three cases referencing map,
entryInstance, entryPathObject) to also assert the error object includes code:
40013 and statusCode: 400; specifically modify the expectToThrowAsync
invocations for these three assertions to verify both the message ('Map value
data type is unsupported') and that the thrown error has code 40013 and
statusCode 400 so the test enforces the PR's error-mapping contract.
Problem
Passing an object obtained from the channel — an
InstanceorPathObjectwrapper, or an internalLiveObject— as a value to a map write was not caught by validation. A very natural call like:passed
LiveMap.validateKeyValue(which accepts any non-nullobject), fell through to the JSON encoding branch inprimitiveToObjectData({ json: value }), and then blew up at publish time inside wire encoding with:— a raw
TypeErrorrather than a properErrorInfo, with no hint about what the user did wrong. (For a non-circular object it would instead silently serialize internal SDK state onto the wire.)The TypeScript typings don't fully guard this either: the public
LiveMap/LiveCounterbranded types are inhabited by both thecreate()value types and the live objects in user type schemas, so this mistake can type-check.Why rejecting (rather than supporting) is correct
Per the objects spec, object-valued writes accept only the creation value types returned by
LiveMap.create()/LiveCounter.create(), which are evaluated into*_CREATEoperations at write time (RTLM20a3, RTLM20e7g, validated per RTLMV4c). Assigning a reference to an existing object was deliberately removed from the write API when the value-type design replaced the old RTLM20e5a behaviour. A new spec point making the rejection explicit (RTLMV4c1: live objects and theirPathObject/Instancewrappers must be rejected with40013) is being added in the specification repo alongside this PR.Fix
Add a check to
LiveMap.validateKeyValuerejectingLiveObject,DefaultPathObjectandDefaultInstanceinstances withErrorInfo(40013, 400)and an actionable message pointing the user atLiveMap.create()/LiveCounter.create().Because
validateKeyValueis the single validation chokepoint, this covers all write paths:LiveMap.set,Instance.set,PathObject.set,BatchContextwrites, and entry evaluation inLiveMap.create().Testing
Extended the existing
LiveMap.set throws on invalid inputscenario with three cases: passing the map's ownInstance, anotherInstance, and aPathObjectas values. Verified the new assertions fail before the fix (with theConverting circular structure to JSONerror above) and pass after it.🤖 Generated with Claude Code
Summary by CodeRabbit