fix: preserve snapshot digest through the client capture path — Phase 4#949
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Blocking this for the shipped CLI path and sequencing. The SDK/client side of the stack delta is right: But Please add a CLI shipped-path test for Also, this PR is stacked on #945 ( |
client.capture.snapshot() always ran the daemon data through
normalizeSnapshotResult, which expects the full `nodes` tree — so a non-default
responseLevel digest ({ nodeCount, refs }) collapsed to an empty snapshot, the
same gap #945 fixed for screenshot. Make it level-aware: when the effective
responseLevel is non-default, return the leveled payload verbatim. Default-level
behavior is unchanged. Adds a shipped-path client test asserting the digest
survives (nodeCount/refs preserved, no normalizer identifiers).
agent-device snapshot --level digest --json dropped nodeCount/refs: snapshot goes through the generic CLI path (runCliCommandWithOutput -> formatCliOutput), whose snapshot formatter serializes the default CaptureSnapshotResult shape and discards digest-only fields. Make runGenericClientBackedCommand level-aware: for a non-default responseLevel it emits the raw leveled payload verbatim (JSON / JSON text), bypassing the default-shape formatter. This generalizes to any generic-path command. Adds a snapshot CLI shipped-path test.
|
Fixed (pushed). Re sequencing: rebased this onto the updated #945 (which now has its CLI digest fix), so the stack is: #945 (screenshot client + CLI) → #949 (snapshot client + generic CLI). tsc/oxlint/fallow/layering clean; 1119 tests pass (the one blip was the known-flaky daemon-client mid-stream-abort test — green on re-run). |
thymikee
force-pushed
the
feat/phase4-snapshot-client-digest
branch
from
028d1d0 to
d11b0b2
Compare
thymikee
added
the
ready-for-human
|
Reviewed the follow-up for the earlier CLI-path blocker. The shipped path now preserves the digest: client.capture.snapshot({ responseLevel: "digest" }) bypasses the default snapshot normalizer, and runGenericClientBackedCommand() emits non-default response-level payloads from the raw command result instead of running them through the default-shape snapshot formatter. The added snapshot --level digest --json test covers the CLI path that was dropping nodeCount/refs. Checks are green. I do not see remaining actionable blockers in this PR. Sequencing note: this is still stacked on #945, so it should land after #945 or be retargeted once that base is in main. |
thymikee
merged commit 69ef3c3
into
feat/phase4-screenshot-digest-view
|
* feat: screenshot digest response-view — Phase 4
Add a `screenshot` entry to the Phase 4 RESPONSE_VIEWS registry. At
responseLevel `digest`, the view keeps the cheap result fields — the
captured `path` and the `artifacts` retrieval handle — and collapses the
token-heavy `overlayRefs` array (each carrying ref + label + three
geometry rects) to a total `overlayCount` plus the first 12 refs leveled
down to `{ ref, label }`. `default`/`full` return today's shape unchanged,
so unregistered and default-level responses stay byte-identical.
* fix: preserve the screenshot digest through the client capture path
The daemon screenshot view returns a leveled digest (path, overlayCount, leveled
overlayRefs, artifacts), but client.capture.screenshot() always ran the data
through readScreenshotResultData, which keeps only path + full-geometry overlay
refs and drops overlayCount/artifacts — so the digest never reached SDK/CLI
callers. Make the capture method level-aware: when the effective responseLevel
(request override or client config) is non-default, return the leveled payload
verbatim instead of normalizing it. Default-level behavior is unchanged.
Adds shipped-path client tests: capture.screenshot({ responseLevel: 'digest' })
returns the raw digest (overlayCount/artifacts preserved, no normalizer
identifiers); the default path still normalizes. Kept the return type as
CaptureScreenshotResult (the union variant cascaded into many default-path
consumers); the caller opted into the level, so the runtime value is leveled.
* fix: preserve the screenshot digest through the CLI command path
agent-device screenshot --level digest --json still dropped overlayCount and
artifacts: the CLI command rebuilt the default { path, overlayRefs } shape from
the (now leveled) client result. Make screenshotCommand level-aware — for a
non-default responseLevel it emits the leveled payload verbatim (JSON / JSON
text). Adds a CLI shipped-path test (--level digest --json preserves the digest;
the default level still emits the normalized shape). Factors the predicate into
a shared isNonDefaultResponseLevel in contracts.ts, reused by the client helper.
* fix: preserve snapshot digest through the client capture path — Phase 4 (#949)
* fix: preserve the snapshot digest through the client capture path
client.capture.snapshot() always ran the daemon data through
normalizeSnapshotResult, which expects the full `nodes` tree — so a non-default
responseLevel digest ({ nodeCount, refs }) collapsed to an empty snapshot, the
same gap #945 fixed for screenshot. Make it level-aware: when the effective
responseLevel is non-default, return the leveled payload verbatim. Default-level
behavior is unchanged. Adds a shipped-path client test asserting the digest
survives (nodeCount/refs preserved, no normalizer identifiers).
* fix: preserve leveled digests through the generic CLI path
agent-device snapshot --level digest --json dropped nodeCount/refs: snapshot
goes through the generic CLI path (runCliCommandWithOutput -> formatCliOutput),
whose snapshot formatter serializes the default CaptureSnapshotResult shape and
discards digest-only fields. Make runGenericClientBackedCommand level-aware: for
a non-default responseLevel it emits the raw leveled payload verbatim (JSON /
JSON text), bypassing the default-shape formatter. This generalizes to any
generic-path command. Adds a snapshot CLI shipped-path test.
1 participant
What
The follow-up I flagged on #945:
client.capture.snapshot()has the identical digest-stripping gap as screenshot did. It always runs the daemon data throughnormalizeSnapshotResult(which expects the fullnodestree), so a non-defaultresponseLeveldigest ({ nodeCount, refs }, from the merged #942) collapsed to an empty snapshot before reaching SDK/CLI callers.Same fix as #945's screenshot: the capture method is now level-aware — when the effective
responseLevel(request override or client config) is non-default, it returns the leveled payload verbatim instead of normalizing. Default-level behavior is byte-identical. Return type staysCaptureSnapshotResult(a union cascades into every.nodesconsumer; the caller opted into the level so the runtime value is the leveled shape).Adds a shipped-path client test:
capture.snapshot({ responseLevel: 'digest' })returns the raw digest (nodeCount/refs preserved, no normalizeridentifiers).Verification
tsc --noEmit0;oxfmt+oxlint --deny-warningsclean;fallow audit --base origin/mainclean;vitest run src/__tests__/client.test.tspass; Layering Guard empty.