Repository navigation
feat(snapshot)!: make a node's ref optional and mint it per producer - #3547
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Why this exists as its own PRThe coordinator flagged #3515 at 1,031 gross lines (
Content is unchanged by the split: against the same merge-base, the old head
|
Review round 2: the minting mode keyed on the channel, not the producerA cold review flagged that Verdict: the axis was wrong, and it is fixed ( What the check actually looked atThe provenance table in this very file states the rule the check broke: "One channel is fed by several producers with different guarantees… Logic that assumes presentation, scope, or geometry guarantees must key on the producer, never on the channel alone." The mode is now a property of the producer ( The three things the review asked to verify1. Which adapters publish The other providers were the plausible candidates and none of them is on this channel:
And the pair is not writable by a foreign adapter: 2. Are they reachable from the product surface? 3. What does The tests
Those table cases cannot by themselves catch the old code, because every pair the table admits today agrees between the two axes. The case that discriminates is Why fix a defect with no live victimBecause the correction is the type, not a guard: a wrong axis plus a partial parameter is precisely the shape that makes the next web adapter's refs disappear, and the fix is that parameter's type plus one lookup list. It also means whoever adds a web adapter never has to know to look here — |
|
I reviewed ad579e0. The change looks right, but I would like one more test and an answer to a design question before merge. I ran no tests myself. I read the code, and I judged the mutation claim (the channel check fails exactly one test) from that reading. The rect-implies-ref rule relies on the producer in #3515, which is not in this diff. I also did not check whether the published SDK typings expose Production wiring is not pinned yet. Simplicity question: could the rect-requiring resolution in Not blocking, and you can take or leave these: make the CI is green, with 19 checks and none failing at ad579e0. There are no conflicts. The next step before merge is the design answer and the Both earlier P3 threads are fixed at this head and can be resolved: the |
ad579e0 to
12f1280
Compare
thymikee
changed the title
Review round 3: the minting mode is pinned end to end, and two answersHead Required: the production wiring is pinned
Mutation-checked: deleting the argument fails the agent-browser case alone ( Design question: the addressable-node type — measured, and not in this PRI tried it rather than reasoning about it. Two findings, both against doing it as asked:
So the design is right, but the seam that has to carry it is the pipeline, not the resolver, and that is a second decision: rows that require a rect ( Answered: yes, the published typings expose it, so this is source-breaking
How the repo records one: there is no changeset or CHANGELOG — Take-it-or-leave-it, taken where it paid
Both earlier P3 threads are already resolved at this head. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
12f1280 to
7a8f1ca
Compare
Correction to my round-3 commentCubic flagged ( The defect is real but it is one layer in, and the fix stays. The comment now says that ( Head is now |
7a8f1ca to
5f14d3c
Compare
|
This PR is ready at 5f14d3c. The earlier ref-address doc and test gaps from ad579e0 are fixed, and the optional ref contract now reads correctly in the code I checked. Not blocking, and fine to take or leave: the five-line comment on The three cubic-dev-ai P3 threads no longer apply and can be resolved: the comment now describes the in-memory own-property contract (#3547 (comment)), the I ran no tests. I judged that the Checks at 5f14d3c are still queued or in progress, and none has failed. Typecheck & Package and Integration Tests cover the changed contract, so they need to finish green before merge. No code change is needed from this review. |
thymikee
added
the
ready-for-human
`SnapshotNode.ref` is declared as always present, which a capture whose producer resolves actions against refs of its own cannot honor: a node the producer issued no ref for has one missing, and a tree carrying that node would be published with `ref` present. The consumers that offer a node as an address take the field at face value today — an overlay label, a pinned ref, a settle tail row, a competitor listed in a touch-point refusal, a replay target's recorded ref evidence — so the first capture that publishes such a node ships `@undefined` to an agent through all of them. `ref` is now optional and `attachRefs` takes the capture's minting mode. Dense producers (Apple, Android, HarmonyOS, Linux) number their trees exactly as before; a producer that owns its ref namespace (`backend-issued`, which agent-browser does — #2283) keeps its own refs and mints nothing beside them. `hasSnapshotRef` and `snapshotRefBodies` are the one way to read the absence, and every surface that offers a node as an address now reads it instead of assuming one: overlay labels, the printed snapshot line, settle tails, MCP ref pins, the touch-point competitor disclosure, keyboard occlusion, `find`/`get` reads, and the record- and replay-time target classifiers, whose ref evidence a ref-less node cannot contribute. No producer emits a ref-less node in this commit, so behavior is unchanged for every backend — proof, not assertion: `git diff --name-only origin/main...HEAD | grep -c '^packages/platform-'` is 0, so no backend's capture path is touched here. The contract and its consumers land first so the capture that needs it lands on top.
`refAddress` is the surface that decides whether a `find` result row prints an address, so it is where a maintainer asks "when is this absent". Naming only the web case would invite reading the field as a web quirk: dense producers mint a ref for every node through the same `attachRefs` call and reach this helper with one present every time. The rule the code actually encodes is "read the field, never infer a mode". The `kind` JSDoc contrasted itself with `ref` as "required like `ref`", which was the old contract; `ref` is optional now and for a reason, while `kind` is optional only as a fixture concession. The two absences mean different things, so the sentence that compared them names the difference.
…t the platform snapshotRefMinting asked whether a capture came from the web channel, but a channel is not a producer: the provenance table itself says several producers can feed one channel with different guarantees, and only agent-browser resolves actions against refs of its own. Any second adapter that lands on the web channel — a webdriver- or CDP-backed one — mints no refs and needs dense positional numbering, which the channel check would have taken away silently, making half its tree unaddressable. The mode now asks the owning question of the producer, and the parameter is the whole provenance pair rather than a partial one so no caller can hand over a channel and leave it to guess who acquired the tree. Behavior is unchanged for every producer that exists today; the table is what the next producer has to reason about.
…er producer The web ref-collision fix (#2283) rests on one argument at snapshot-state.ts:83, and no test reached it: deleting it returned every tree to dense numbering with the whole suite green, minting a ref onto web text its backend cannot resolve. One case per side of the mode now: an agent-browser capture publishes only the refs it issued (non-dense, byte-identical) and an Android capture stays densely numbered so every node remains addressable. Mutation-checked: removing the argument fails the first case alone.
…to a ref-less match toDaemonFindData rebuilt the response with an unconditional `ref: match.ref`, so a match whose capture issued no ref left an own-property whose value was not a ref on the response data. The wire form never showed it (JSON.stringify drops an undefined value), but the data object is what the CLI formatter, the MCP ref-pin merge and a Node consumer read, and an absent address is the one thing the optional field exists to express. The builder now omits the key instead.
The touch-point competitor disclosure and the MCP ref-pin merge each restated the filter over their own shape. Typed over the field rather than over SnapshotNode, both call it. Also states on the ref field why the geometry-requiring paths print it bare: a ref-less node carries no rect, so no acting path can reach one.
5f14d3c to
e67b761
Compare
|
Took the non-blocking items at
On your two verification notes, to make the record honest: Both edits are comment-only against |
|
Thanks for the update. The earlier feedback is addressed at e67b761: the three cubic-dev-ai doc threads are fixed at head, and the code has no blocking problems. CI is green, with 19 checks reported and none failing. There are no conflicts. Not blocking, so take it or leave it: src/core/interactors/web.ts:61 (https://github.com/callstack/agent-device/blob/e67b761/src/core/interactors/web.ts#L61) stamps producer 'agent-browser' on every snapshot, whatever WebProvider it wraps. snapshotRefMinting keys on that producer, so a second web adapter would get 'backend-issued' and lose refs on nodes it left unaddressed, which is the channel-keyed outcome the PR body says it avoids. The emulated provider in test/integration/provider-scenarios/web-world.ts hit this and needed refs e1 to e6 added. The kernel test that claims a second producer keeps dense numbering uses a cast pair that never reaches this route. Should WebSnapshotResult carry the provider's own producer, so the producer-keyed decision holds at that route? If not, please tone down the PR body claim and the kernel test comment to say the guarantee depends on web.ts stamping the producer correctly. The three cubic-dev-ai P3 threads are fixed at head, so you can resolve them: selector-read.ts doc (#3547 (comment)), kernel snapshot.ts:415 (#3547 (comment)), and daemonRefAddress JSDoc (#3547 (comment)). I did not run tsc or the tests locally, so the code check rests on green CI. The four acting-path prints of node.ref stay safe only because a rect-bearing node always has a ref. That fact is documented, not typed, and #3576 tracks typing it. Nothing else stands in the way of a maintainer merge decision. |
|
1 participant
Summary
SnapshotNode.refwas declared always present, which was already a lie the moment a capture's producer resolved actions against refs of its own: a node the producer issued no ref for has one missing, and every surface that prints a ref printed@undefinedfor it.refbecomes optional andattachRefstakes the capture's minting mode:snapshotRefMintingkeys on the capture's producer, not its platform channel, and takes the whole provenance pair rather than a partial one. A channel is not a producer — the provenance table's own rule is that several producers can feed one channel with different guarantees, and onlyagent-browserresolves actions against refs of its own. A second adapter that lands on thewebchannel mints no refs, and a channel-keyed check would have moved it tobackend-issuedand silently stripped the refs its whole tree depends on (see the review-round comment below).hasSnapshotRefandsnapshotRefBodiesare the one way to read the absence, and every surface that offers a node as an address now reads it: overlay labels, the printed snapshot line, settle tails, MCP ref pins, the touch-point competitor disclosure, keyboard occlusion,find/getreads, and the record- and replay-time target classifiers, whose ref evidence a ref-less node cannot contribute. No producer emits a ref-less node in this PR, so behavior is unchanged for every backend; the contract and its consumers land first so the capture that needs it (#3515) lands on top. 27 files, 582 gross lines.Breaking for TypeScript consumers, source-level.
SnapshotNode.refis in the shipped declarations (dist/src/sdk-contracts.d.ts, re-exported astype SnapshotNodefromdist/src/index.d.ts, and reached throughSnapshotState.nodes/CaptureSnapshotResult.nodes/agent-device/contracts). Code that reads it as always present —const r: string = node.ref, aSnapshotNodepassed whererefis required,node.ref.length— typechecks today and fails here;examples/sdk/client-session.ts:68is that exact case, now guarded withtarget?.ref !== undefinedbeforepress({ ref: target.ref }). The repo has no changeset and no CHANGELOG (release.yml:203generates notes from the commits since the last tag), so the record is the!in the subject —feat(snapshot)!: …— the waydbf113a55 refactor(platform-android)!: … (#3023)carried one. Runtime behavior for every existing backend is unchanged; the version bump stays with the release owner.Review round 2: the minting mode keys on the producer, not the channel
A cold review asked what
snapshotRefMinting'sprovenance?.backend === 'web'does to a non-agent-browserweb adapter (a webdriver- or Playwright-backed one) that publishesbackend: 'web': it would flip tobackend-issuedand every node it publishes would lose the ref its actions resolve against. The axis was wrong and is fixed; the silent-stripping half has no live victim. Full evidence in the review-round comment.xctestalone has four.REF_NAMESPACE_OWNERSnow names the producers that own their ref namespace, and the parameter is the whole pair (SnapshotCaptureProvenance) instead ofPartial<SnapshotProvenance>, so a caller cannot hand over a channel and leave this to guess who acquired the tree. That is pinned with@ts-expect-error, which failspnpm typecheckif the parameter widens back.src/core/interactors/web.tsis the only publisher ofbackend: 'web'and it stampsproducer: 'agent-browser'.provider-webdriveris typedExtract<SnapshotResult['backend'], 'android' | 'xctest'>,provider-limrunpublishes'xctest'/'limrun-ios-tree',provider-testmuhas no snapshot producer, and there is no Playwright/Puppeteer/webdriverio dependency in the workspace.{ backend: 'web', producer: <other> }is already a compile error (snapshot-provenance.test.ts).main's behavior: dense numbering. What changes is that the next adapter on the web channel gets dense numbering by default instead of having its refs stripped.agent-browserpairs assertdense), typedsatisfies readonly SnapshotProvenance[]so a producer added to the table arrives with a case, plus a case pinning that a second web producer staysdense. Mutation-checked: reverting to the channel check fails exactly that one test.Validation
Tested at
e67b761af, rebased onorigin/main(stack: #3515 → #3533 restacked on this head).pnpm check:affected --run --base origin/mainon that commit under the machine-wide gate lock:all runnable checks passed;pnpm vitest runover kernel/selectors/capture-kit/replay-port/interaction/mcp/platform-web green;provider-integrationgreen. Kernel tests cover both minting modes, the per-producer decision, and the addressability helpers. 28 files, 586 gross lines.Unchanged-behavior proof:
git diff --name-only origin/main...HEAD | grep -c '^packages/platform-'→0, so no backend's capture path changes; the twotest/integration/provider-scenarios/files are the emulated page the contract is asserted against. "Every address surface reads the absence" is a completeness claim, so it carries its grep.grep -rn '@${.*.ref}' src packages --exclude-dir=dist-typesleaves three families that print without a guard, and each is closed by a fact rather than by luck: the **acting** sites (find.ts:200and:426,target-visibility-stages.ts:45,resolution-disclosure.ts:87) take a node from a row declaringrequireRect: true, and a ref-less node carries norect— the invariant now stated on thereffield, and proven end to end by #3515's router case whereclick text=refuses withselector_not_found; the **ref-lookup** sites (ref-target-resolution.ts:124and:130,selector-read.ts:172) receive the ref as input; the two **collapsed-leaf warnings** (capture/runtime/snapshot.ts:381,quality-warnings.ts:101) require alabel, and the web projection publishes ref-less text onvalue. The remaining prints (snapshot-lines.ts:66,settle-output.ts:111) are guarded, and the settle tail filters onhasSnapshotRef`.That acting-site safety is a producer fact stated in the
refJSDoc, not yet a type fact: the pipeline rows declaringrequireRect: truestill carry a plainSnapshotNode. Typing them asSnapshotNode & { ref: string }needs a narrowing entry at the pipeline boundary rather than at the resolver, and I measured that separately rather than bundling it here — tracked as #3576, which records the attempt's error census (28 across 5 files) and why the guards relocate inward.Risk: this widens a type every backend reads, so a consumer merged into
mainbefore this lands could assume a non-optionalref; typecheck catches it. The rebase ontomainafter #3482 landed is the evidence: its own new keyboard-chrome assertion (packages/capture-kit/src/snapshot-chrome.test.ts) readnode.refas required and went red atTS2345, fixed here by the file's existing guard idiom — assert the ref is there, then use it.