Skip to content

Commit 569839f

Browse files
d-gubertclaude
andcommitted
refactor(apps): address review on the accessor-consolidation parity harness
- parityHarness: deep-snapshot recorded params at emit time so a later accessor mutation of a shared object cannot rewrite an already-recorded call; fix the stale migration-doc reference. - parityHarness test: assert the emitted params (id + APP_ID sentinel on lookup, mutated message + APP_ID sentinel on write), not just the ordered method names. - docs: give the AppResourceBridge fenced block an explicit language. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 7d52339 commit 569839f

3 files changed

Lines changed: 25 additions & 6 deletions

File tree

docs/proposals/apps-accessor-consolidation/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ manager / the app's own storage item"*. Rather than keeping a residual `accessor
258258
them, add one internal bridge so they ride the existing, permission-checked, appId-substituting
259259
`bridges:*` channel:
260260

261-
```
261+
```text
262262
AppResourceBridge (host-side, internal — not part of the app-facing definition surface)
263263
├── doProvideSetting(setting, appId) → ProxiedApp.getStorageItem() mutation (SettingsExtend semantics)
264264
├── doGetAppSetting(id, appId) → app.getStorageItem().settings[id] (SettingRead semantics)

packages/apps/base-runtime/src/lib/accessors/tests/helpers/parityHarness.test.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ describe('parityHarness', () => {
4444
});
4545

4646
it('pins the host-bound traffic a MOVE-style accessor emits (worked example: ModifyExtender.extendMessage + finish)', async () => {
47-
const { sender, methods } = createRecordingSender({
47+
const { sender, emitted } = createRecordingSender({
4848
'bridges:getMessageBridge:doGetById': { id: 'message-id', text: 'original' },
4949
});
5050

@@ -55,7 +55,23 @@ describe('parityHarness', () => {
5555
await extender.finish(messageExtender);
5656

5757
// This is the parity assertion a Phase 1-2 port must keep satisfying: the
58-
// exact ordered bridge traffic, normalized to the APP_ID sentinel.
59-
assert.deepStrictEqual(methods(), ['bridges:getMessageBridge:doGetById', 'bridges:getMessageBridge:doUpdate']);
58+
// exact ordered bridge traffic *and* its params, normalized to the APP_ID
59+
// sentinel.
60+
const calls = emitted();
61+
assert.deepStrictEqual(
62+
calls.map((c) => c.method),
63+
['bridges:getMessageBridge:doGetById', 'bridges:getMessageBridge:doUpdate'],
64+
);
65+
66+
// extendMessage looks the message up by id; the caller identity is the APP_ID sentinel.
67+
assert.deepStrictEqual(calls[0].params, ['message-id', 'APP_ID']);
68+
69+
// finish writes the mutated message back, again trailed by the APP_ID sentinel.
70+
// (editedAt is a fresh Date, so we assert the fields the accessor is responsible for.)
71+
const [updatedMessage, appId] = calls[1].params as [Record<string, unknown>, string];
72+
assert.strictEqual(appId, 'APP_ID');
73+
assert.strictEqual(updatedMessage.id, 'message-id');
74+
assert.deepStrictEqual(updatedMessage.editor, { id: 'user-id' });
75+
assert.deepStrictEqual(updatedMessage.customFields, { key: 'value' });
6076
});
6177
});

packages/apps/base-runtime/src/lib/accessors/tests/helpers/parityHarness.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type * as Messenger from '../../../messenger';
33
/**
44
* Transitional parity harness for the accessor-consolidation migration.
55
*
6-
* MOVE accessors (see docs/base-runtime-accessor-consolidation.md) are *live in
6+
* MOVE accessors (see docs/proposals/apps-accessor-consolidation/README.md) are *live in
77
* production today* via the host proxy path, so a subtly wrong port - a dropped
88
* default, an off-by-one on a cap, a renamed sort field - is an immediate
99
* observable regression. Ordinary ported unit tests only prove the new local
@@ -62,7 +62,10 @@ export function createRecordingSender(responses: CannedResponses = {}): Recordin
6262
const calls: RecordedCall[] = [];
6363

6464
const sender = ((requestDescriptor: { method: string; params?: unknown }) => {
65-
const params = Array.isArray(requestDescriptor.params) ? requestDescriptor.params : [];
65+
// Deep-snapshot the params at emit time (including nested objects/arrays) so a
66+
// later accessor mutation of a shared object cannot rewrite a call we already
67+
// recorded - the recorded sequence must reflect the wire traffic as it was sent.
68+
const params = structuredClone(Array.isArray(requestDescriptor.params) ? requestDescriptor.params : []);
6669
const call: RecordedCall = { method: requestDescriptor.method, params };
6770
calls.push(call);
6871

0 commit comments

Comments
 (0)