refactor: share screenshot capture and artifact persistence

[ThomasK33]

Closes #64.

Summary

Live host screenshot RPCs and CLI offline replay screenshots now both flow through a single captureScreenshotResult(...) helper in src/screenshot/capture.ts, mirroring the snapshot capture refactor in #69.

The shared seam owns:

• temp-file allocation under the session artifacts/ directory
• renderer backend.screenshot(...) invocation (with optional showCursor threaded through)
• the union of the previous live and offline runtime invariants (sessionId, profileName, pngSizeBytes, requested vs returned artifactPath, sha256)
• allowlisted public ScreenshotResult construction
• Zod validation through ScreenshotResultSchema before any rename or manifest append side effect, plus a post-parse sha256 invariant
• the final screenshot-<seq>-<profile>.png rename
• artifact manifest entry creation with the same metadata fields as before
• temp-file cleanup on failure (preserves the previous catch/cleanup/rethrow behavior)

parseScreenshotResult(...) is also exported from the shared module and used by the CLI to validate live RPC responses; the offline path returns a typed ScreenshotResult directly from the helper. The helper accepts a narrow Pick<RendererBackend, 'screenshot'> and returns CapturedScreenshotResult = ScreenshotResult & { sha256: string }, so callers see the runtime guarantee at the type level.

What did NOT change

• ScreenshotResultSchema, ScreenshotParamsSchema, and the artifact manifest schema
• screenshot artifact filename format and artifacts/ layout
• live RPC JSON envelope and screenshot --json output
• renderer behavior or visual fidelity
• snapshot capture, record export, or wait/retention behavior

sha256 is now an enforced invariant on every successful screenshot result. The real ghostty-web and libghostty-vt backends already always emit it; only test fakes that did not exercise the capture path omitted it.

CONTEXT.md

Already updated during the design grilling with three new domain terms (Screenshot Result, Screenshot Artifact, Screenshot Capture) and the corresponding relationships. No ADR was added; the refactor is internal and reversible.

Validation

• npm run verify — green: format:check + lint + typecheck + 1153 tests + build + smoke:install
• npm run test -- test/unit/screenshot/capture.test.ts test/unit/commands/screenshot.test.ts — 27 tests pass (15 in screenshot/capture.test.ts, 12 in commands/screenshot.test.ts)
• npm run test -- test/integration/host-renderer-rpc.test.ts — 8/8 pass (live screenshot integration)

Dogfood proof

dogfood/issue-64-share-screenshot-capture/ contains a reproducible commands.sh plus committed before/ (parent commit b2d5068, generated in a temporary git worktree) and after/ (this branch) directories with:

• 4 screenshot scenarios per side: live × {default-cursor, --show-cursor} and offline × {default-cursor, --show-cursor}
• their JSON envelopes, PNG files, artifact manifests, transcripts, and SHA-256 summaries

Verified parity:

Comparison	Result
sha256-summary.txt	identical
All 4 result envelopes (excluding ULID sessionId and the absolute artifactPath)	identical fields
Artifact manifest entries (excluding ULID id/sessionId/createdAt)	identical metadata
All 4 PNG files (cmp -s)	byte-identical

See dogfood/issue-64-share-screenshot-capture/README.md for the reviewer reproduction snippet.

Follow-up issues filed during review

• Rollback the screenshot rename when manifest append fails #79 — rename rollback when manifest append fails (preserves the explicit "out of scope" decision from this refactor)
• Extract a generic parseValidatedResult<T> helper for capture modules #80 — generic parseValidatedResult<T> helper for snapshot+screenshot+future capture modules
• Consolidate createFakeBackend factories into a shared test helper #81 — shared test/helpers/fakeBackend.ts to consolidate the four createFakeBackend factories

---

📋 Implementation Plan

Issue #64: Share screenshot capture and artifact persistence

Context

Issue: coder/agent-tty#64 — "Share screenshot capture and artifact persistence across live and offline replay".

The blocker, issue #60, is already closed and shipped as PR #69. That snapshot refactor created src/snapshot/capture.ts, which is the direct precedent for this work.

Current duplicated screenshot paths verified during grilling:

• src/host/hostMain.ts live screenshot RPC handler resolves the profile and renderer, allocates a temporary PNG path, calls backend.screenshot(...), validates several invariants, renames the temp PNG to its final artifact filename, appends a screenshot manifest entry, and returns a hand-built ScreenshotResult.
• src/cli/commands/screenshot.ts offline replay fallback does almost the same temp-file, renderer, rename, manifest, and result-construction work inside runOfflineScreenshot(...).
• The two paths currently differ in their runtime checks: the live path checks artifactPath === temporaryOutputPath; the offline path checks sha256 !== undefined.

The agreed design is to deepen screenshots the same way snapshots were deepened: one shared rendered-artifact capture seam owns all screenshot artifact persistence and public result construction.

Goals

• Use one shared implementation for live host screenshot RPCs and offline replay screenshots.
• Preserve public behavior exactly: filenames, final PNG paths, manifest metadata, sha256, cursor metadata, renderer metadata, render-profile hashes, and CLI/RPC JSON shapes.
• Preserve current cleanup-on-failure behavior for temporary screenshot files.
• Keep the change minimal and surgical; do not alter renderer fidelity, screenshot schemas, or unrelated artifact behavior.

Non-goals

• No changes to ScreenshotResultSchema, ScreenshotParamsSchema, or artifact manifest schema.
• No changes to screenshot filename format or artifact directory layout.
• No renderer behavior or visual-fidelity changes.
• No cleanup rollback for already-renamed final PNGs if manifest append fails; that is explicitly out of scope.
• No changes to snapshot capture semantics, including its optional expectedSessionId API.
• No record-export, wait, or retention refactors.
• No ADR; this is an internal, reversible refactor whose intent is obvious from the code and issue context.

Design decisions from grilling

1. Shared seam: implement an end-to-end capture helper, not just a persistence helper.
2. Backend input: pass the existing RendererBackend to the helper, not a custom callback.
3. Profile input: pass profileName: string; profile resolution remains at the caller-specific layer.
4. Return value: return a Zod-validated ScreenshotResult.
5. Invariants: enforce the union of current live/offline checks:
   • result.sessionId === expectedSessionId
   • result.profileName === profileName
   • result.pngSizeBytes > 0
   • result.artifactPath === temporaryOutputPath
   • result.sha256 !== undefined
6. Expected session: expectedSessionId is required.
7. Cleanup: preserve today's catch { rm(tmp, { force: true }).catch(() => undefined); throw error; } behavior.
8. Module: add src/screenshot/capture.ts.
9. CLI parsing: move parseScreenshotResult into the shared module; the CLI parses live RPC responses inline and uses offline replay's already-validated typed result directly.
10. Host handler: keep host-specific profile/renderer/replay setup in src/host/hostMain.ts, then return captureScreenshotResult(...) directly.
11. Tests: create test/unit/screenshot/capture.test.ts as the main unit surface and trim duplicated result-construction assertions from caller tests.
12. Dogfood: add a minimal side-by-side proof bundle under dogfood/ that demonstrates live and offline screenshot parity before/after the refactor.
13. Context docs: CONTEXT.md has already been updated during grilling with Screenshot Result, Screenshot Artifact, and Screenshot Capture; no ADR is needed.

Implementation plan

Phase 0 — Verify strict invariant assumptions

Before changing code, inspect the real renderer backends and existing test fakes that implement RendererBackend.screenshot(...).

Confirm that successful screenshot results always:

• return the same artifactPath that was passed into backend.screenshot(...); and
• include sha256.

If a real renderer backend violates either assumption, stop and reassess the union-invariant decision instead of silently weakening or working around it. If only test fakes violate the assumptions, update the fakes to match the real backend contract.

Quality gate after Phase 0:

• Record which files/backends were checked in the implementation notes or PR body.

Phase 1 — Add shared screenshot capture module

Create src/screenshot/capture.ts.

Exports:

export interface CaptureScreenshotResultOptions {
  backend: RendererBackend;
  sessionDir: string;
  profileName: string;
  expectedSessionId: string;
  showCursor?: boolean;
}

export function parseScreenshotResult(
  rawResult: unknown,
  message?: string,
): ScreenshotResult;

export async function captureScreenshotResult(
  options: CaptureScreenshotResultOptions,
): Promise<ScreenshotResult>;

Implementation details:

• Import rename and rm from node:fs/promises and ulid for .tmp-screenshot-<ulid>.png.
• Use existing helpers only:
  • ensureArtifactsDir, artifactPath, screenshotFilename
  • appendArtifact, createArtifactEntry
  • ScreenshotResultSchema
  • makeCliError(ERROR_CODES.PROTOCOL_ERROR, ...)
  • invariant(...)
• parseScreenshotResult(...) should mirror parseSnapshotResult(...) style from src/snapshot/capture.ts.
• captureScreenshotResult(...) should:
  1. assert non-empty sessionDir, profileName, and expectedSessionId;
  2. await ensureArtifactsDir(sessionDir);
  3. allocate temporaryOutputPath = artifactPath(sessionDir, `.tmp-screenshot-${ulid()}.png`);
  4. call backend.screenshot(temporaryOutputPath, showCursor === undefined ? undefined : { showCursor });
  5. assert the five agreed invariants;
  6. capture const sha256 = result.sha256; invariant(sha256 !== undefined, 'renderer screenshot must produce sha256') so later manifest/result construction is not forced to treat the hash as optional;
  7. compute filename = screenshotFilename(result.capturedAtSeq, result.profileName);
  8. compute finalArtifactPath = artifactPath(sessionDir, filename);
  9. build an allowlisted public result with artifactPath: finalArtifactPath and exactly the fields currently returned;
  10. validate that public result with parseScreenshotResult(publicResult, 'Screenshot result validation failed.') before any rename or manifest append side effect;
  11. await rename(temporaryOutputPath, finalArtifactPath);
  12. append a screenshot artifact entry using the parsed public result and the same metadata as today:
      • kind: 'screenshot'
      • filename
      • sessionId
      • capturedAtSeq
      • sha256
      • metadata: profileName, cols, rows, pngSizeBytes, cursorVisible, rendererBackend, pixelWidth, pixelHeight, renderProfileHash
  13. return the parsed public result.
• Do not construct the public result by spreading the renderer result. Build the same allowlisted 13 fields explicitly so unexpected backend fields do not leak into the strict public schema.
• Preserve rename(temporaryOutputPath, finalArtifactPath) semantics exactly. Do not add pre-existence checks, copy/unlink fallback, collision protection, deduplication, or manifest-entry dedupe logic as part of this refactor.
• Wrap the renderer/validation/persistence sequence in try/catch; on failure remove only temporaryOutputPath with force and rethrow.
• Use the generic shared validation message ('Screenshot result validation failed.') for helper-side validation failures. This is an intentional weird-path message change from the old offline-only parse message in exchange for one shared validated seam.
• Do not add persistScreenshotArtifact(...) unless implementation reveals a real caller; the current agreed surface should stay minimal.

Quality gate after Phase 1:

• Run targeted unit tests once the new unit file exists; before tests exist, run the confirmed repo script npm run typecheck only if useful, otherwise continue to Phase 2 and validate after callers are wired.

Phase 2 — Wire live host RPC to the shared module

Modify src/host/hostMain.ts.

• Remove only imports that are actually unused after extraction and confirmed by typecheck/lint. Likely screenshot-specific candidates are rename, rm, ulid, appendArtifact, createArtifactEntry, ensureArtifactsDir, artifactPath, and screenshotFilename, but keep any import still used elsewhere in hostMain.ts.
• Add import for captureScreenshotResult from ../screenshot/capture.js.
• Keep the current host-specific logic inside the screenshot RPC handler:
  • parse params as ScreenshotParams
  • resolve requestedProfileName ?? DEFAULT_RENDER_PROFILE_NAME with the existing error mapping
  • resolve host renderer name
  • load replay input
  • get backend from rendererManager.getBackend(rendererName, profile, replayInput)
• Replace the temp-file/render/manifest/hand-built-result block with:

return captureScreenshotResult({
  backend,
  sessionDir: sessDir,
  profileName: profile.name,
  expectedSessionId: sessionId,
  ...(showCursor === undefined ? {} : { showCursor }),
});

Quality gate after Phase 2:

• Typecheck if cheap; otherwise defer to Phase 4 tests. The main risk is import cleanup and exact RendererBackend type compatibility.

Phase 3 — Wire offline replay screenshot path to the shared module

Modify src/cli/commands/screenshot.ts.

• Remove private parseScreenshotResult(...) and import it from ../../screenshot/capture.js.
• Import captureScreenshotResult from the same module.
• Remove only imports that are actually unused after extraction and confirmed by typecheck/lint. Likely offline-path candidates are rename, rm, ulid, appendArtifact, createArtifactEntry, ensureArtifactsDir, artifactPath, screenshotFilename, ScreenshotResultSchema, and invariant.
• Keep resolveScreenshotRequest(...), formatScreenshotLines(...), session lookup, live/offline routing, and HOST_UNREACHABLE fallback behavior unchanged.
• Replace runOfflineScreenshot(...) body with a thin wrapper:

return withOfflineReplayRenderer(
  { sessionDir: sessionDirectory, profileName: profile, rendererName },
  async ({ backend, manifest }) =>
    captureScreenshotResult({
      backend,
      sessionDir: sessionDirectory,
      profileName: profile,
      expectedSessionId: manifest.sessionId,
      ...(showCursor === undefined ? {} : { showCursor }),
    }),
);

• Restructure runScreenshotCommand(...) so each branch produces a typed ScreenshotResult:
  • live RPC success: parseScreenshotResult(rawResult, 'Unexpected response from host')
  • live RPC HOST_UNREACHABLE fallback: await runOfflineScreenshot(...)
  • non-running manifest: await runOfflineScreenshot(...)
• Remove rawResult: unknown and invalidResultMessage branch-state variables.
• Preserve the legacy live-RPC gate comment about manifest.status === 'running' unchanged unless import/formatting forces movement.

Quality gate after Phase 3:

• Run targeted screenshot command tests, then fix failures before moving on:

npm run test -- test/unit/commands/screenshot.test.ts

Use mise run test -- ... if this repo's current script accepts passthrough in the implementation environment; otherwise use the npm script form.

Phase 4 — Add and adjust tests

Add test/unit/screenshot/capture.test.ts modeled after test/unit/snapshot/capture.test.ts but with a fake RendererBackend.

Primary coverage:

• Success path:
  • creates a temp path under the session artifacts directory
  • calls backend.screenshot(...) with that temp path
  • passes showCursor through when provided and omits options when undefined
  • renames to screenshot-<seq>-<profile>.png
  • appends the manifest entry with the exact existing metadata fields
  • returns a ScreenshotResult whose artifactPath is the final path
• Invariant failures:
  • session ID mismatch
  • profile name mismatch
  • non-positive PNG size
  • renderer returns a different artifactPath than requested
  • missing sha256
• Validation failure:
  • malformed public result is rejected through ScreenshotResultSchema and surfaced as PROTOCOL_ERROR before rename or manifest append;
  • temp file is removed;
  • no final PNG is written;
  • no manifest entry is appended.
• Cleanup-on-failure:
  • renderer throws: temp file removed or absent
  • rename throws: temp file removed
  • manifest append throws after rename: temp path absent and final PNG remains, documenting the intentionally preserved G1 orphan behavior
• Failure injection mechanics should mirror existing test/unit/snapshot/capture.test.ts patterns and prefer real-fs-induced failures where practical. Avoid brittle ESM mocks unless the existing test style already uses them.

Adjust existing tests:

• test/unit/commands/screenshot.test.ts: keep routing, fallback, request construction, output formatting, and CLI error behavior; remove duplicated assertions about manifest entry internals and result construction now owned by test/unit/screenshot/capture.test.ts.
• Host renderer RPC screenshot integration tests: keep at least one seam-level test that proves live RPC returns a valid screenshot result, writes a PNG, and creates a manifest entry. Trim only duplicated invariant-by-invariant expectations if present.
• E2E artifact-manifest tests: do not change expectations unless the refactor accidentally changes output; if output changes, treat it as a bug to fix rather than updating snapshots.

Quality gates after Phase 4:

npm run test -- test/unit/screenshot/capture.test.ts
npm run test -- test/unit/commands/screenshot.test.ts

Also run the existing host renderer RPC screenshot integration test and relevant e2e artifact-manifest test after locating their exact paths/names.

Phase 5 — Dogfood proof bundle

Create a fresh minimal dogfood proof under dogfood/issue-64-share-screenshot-capture/.

Bundle/script requirements:

• commands.sh with reproducible commands and an isolated absolute AGENT_TTY_HOME.
• Prefer a temporary git worktree or equivalent isolated checkout of main to generate the pre-refactor baseline, then run the same script against the refactor branch.
• Generate before/ and after/ directories, or main/ and refactor/ directories, containing:
  • live/running screenshot JSON envelope without --show-cursor
  • live/running screenshot JSON envelope with --show-cursor
  • offline/destroyed screenshot JSON envelope without --show-cursor
  • offline/destroyed screenshot JSON envelope with --show-cursor
  • corresponding PNG files
  • relevant artifact manifest snippets
• README.md should list the compared fields and conclusion.
• Commit generated binary artifacts only if that matches the repo's existing dogfood/ convention for comparable proof bundles. If not, commit the script/README and include generated artifact paths, hashes, and attached screenshots/video in the PR or handoff notes.

Dogfood method:

1. Use a fixture such as test/fixtures/apps/color-grid or hello-prompt; prefer the fixture already used by screenshot/e2e tests.
2. Run the same command sequence before and after the refactor.
3. Compare public JSON fields, manifest metadata fields, and PNG sha256 values.
4. Default profile only (reference-dark), because profile variance is not part of this refactor.
5. Include both default cursor behavior and --show-cursor, because cursor metadata is an explicit preservation criterion.
6. Capture reviewer-verifiable screenshots and a short recording of the dogfood flow if feasible; if not feasible for this CLI-only workflow, preserve the generated PNGs, manifests, hashes, and command transcript as the proof bundle.

If exact before/after branch comparison is impractical inside the implementation workspace, produce the after bundle plus a README section explaining the limitation and include automated test evidence that verifies unchanged fields. Do not silently skip dogfooding.

Quality gate after Phase 5:

• Run bash dogfood/issue-64-share-screenshot-capture/commands.sh from a clean repo state with isolated AGENT_TTY_HOME.
• Inspect generated screenshots and manifests.
• Record the compared hashes/fields in the bundle README or PR body.

Phase 6 — Documentation finalization

CONTEXT.md has already been updated during the grilling session with:

• Screenshot Result
• Screenshot Artifact
• Screenshot Capture

and relationships tying screenshot capture, result, and artifact together.

Implementation should:

• Keep that documentation update unless implementation discoveries invalidate the definitions.
• Avoid creating an ADR.
• Mention in the PR/body summary that the context glossary was updated as part of the design grilling.

Quality gate after Phase 6:

npm run format:check

or mise run format-check if preferred/available.

Validation plan

Run narrow checks first, then broader checks if touched areas warrant it. The repo scripts were verified in package.json/mise.toml: npm run test, npm run test:integration, npm run test:e2e, npm run typecheck, npm run lint, npm run format:check, npm run verify, and mise run ci exist.

Targeted checks:

npm run test -- test/unit/screenshot/capture.test.ts
npm run test -- test/unit/commands/screenshot.test.ts

After locating the exact existing screenshot host/RPC and e2e artifact-manifest test files, run them with the confirmed scripts, for example:

npm run test -- <exact-screenshot-host-or-integration-test-file>
npm run test -- <exact-screenshot-e2e-or-artifact-manifest-test-file>

Use npm run test:integration / npm run test:e2e for broader serial layer checks when the exact target file is unclear or when the touched behavior is broad enough to justify the full layer.

Repo quality checks:

npm run typecheck
npm run lint
npm run format:check

Preferred final check before handoff if time permits:

mise run ci

Fallback if mise is unavailable:

npm run verify

Acceptance criteria

• Live host screenshot RPCs and offline replay screenshots call the same shared capture implementation.
• The shared implementation owns temp-file handling, renderer screenshot invocation, final artifact naming, allowlisted result construction, Zod validation before persistence side effects, manifest metadata creation, and temp cleanup on failure.
• Screenshot filenames and final PNG paths remain unchanged.
• Artifact manifest entries preserve kind, filename, sessionId, capturedAtSeq, sha256, profileName, terminal dimensions, PNG size, cursor visibility, renderer backend, pixel dimensions, and render-profile hash behavior.
• sha256 remains present for successful screenshots.
• showCursor behavior is unchanged.
• Live RPC response JSON and CLI screenshot --json output remain schema-compatible and behavior-compatible.
• Temporary files are removed when renderer capture or persistence fails, matching today's behavior.
• Existing screenshot command unit tests, host renderer RPC screenshot tests, and e2e artifact-manifest checks pass.
• A reviewer-facing dogfood bundle demonstrates live and offline screenshot parity before/after the refactor.
• No ADR is added.

Risks and mitigations

• Risk: stricter union invariants expose fake test backends that omit sha256 or return a stale artifactPath.
  • Mitigation: update test fakes to match the real RendererBackend contract; if a real backend omits sha256, stop and reassess the invariant instead of weakening silently.
• Risk: centralizing code accidentally changes optional metadata ordering or omission behavior in manifest entries.
  • Mitigation: copy current manifest metadata construction exactly, add direct unit coverage, and verify with e2e/dogfood artifacts.
• Risk: CLI branch restructuring changes error messages.
  • Mitigation: preserve live RPC parse message (Unexpected response from host) and use a clear shared-module validation message for local capture failures; keep existing command tests for error envelopes.
• Risk: manifest append failure leaves a final PNG orphan.
  • Mitigation: document this as intentional behavior preservation in unit tests; defer rollback cleanup to a separate issue if desired.
• Risk: dogfood before/after comparison is hard inside a single isolated workspace.
  • Mitigation: script the commands so a reviewer can reproduce; if actual before artifacts cannot be generated locally, explain that limitation and rely on automated parity checks plus after artifacts.

Handoff notes

• The implementer should inspect the exact current test names before editing tests; do not assume host screenshot tests' paths.
• Keep edits surgical: shared module, two callers, focused tests, dogfood bundle, and existing CONTEXT.md update only.
• Prefer existing assertion helper invariant(...) and existing storage helpers; do not write manifest-like files with ad hoc fs.writeFile() logic.
• Use an isolated absolute AGENT_TTY_HOME for all dogfood and CLI validation.

Advisor review incorporated

Advisor review was requested after the initial plan draft. The plan now incorporates the review by:

• adding Phase 0 to verify real renderer/test-fake assumptions for sha256 and requested artifactPath before implementation;
• validating the allowlisted public ScreenshotResult before rename(...) or manifest append;
• forbidding renderer-result spreading into the public result;
• making sha256 narrowing explicit before manifest/result construction;
• preserving raw rename(temp, final) semantics exactly;
• documenting the generic helper-side validation message tradeoff;
• softening dogfood artifact-commit requirements while keeping reproducible proof and reviewer-verifiable evidence;
• confirming repo validation scripts from package.json and mise.toml; and
• making import cleanup typecheck/lint-driven rather than speculative.

---

Generated with mux • Model: anthropic:claude-opus-4-7 • Thinking: max

[ThomasK33]

/coder-agents-review

[coder-agents-review]

Clean, well-executed deduplication refactor. The shared captureScreenshotResult helper eliminates ~170 lines of duplicated logic across live host and offline replay paths, unifies the invariant sets (adding sha256 enforcement to live, artifactPath to offline), and introduces pre-side-effect Zod validation that neither old path had. The 10 new unit tests cover the success path, all five invariants, schema validation gating, and three distinct failure-cleanup scenarios with real filesystem assertions. Dogfood proof demonstrates byte-identical PNGs and field-identical JSON envelopes before and after.

As Pariston put it: "I tried to build a case against this and could not. The problem is correctly understood. The solution is proportional to the duplication."

3 P3, 2 P4, 3 Nit, 3 Note. All are test coverage gaps and style observations; no correctness or behavioral issues found. Netero's first pass was clean. 11 of 17 panel reviewers returned no findings.

---

test/unit/commands/screenshot.test.ts:366

P4 [DEREM-7] The implementation plan (Phase 4) explicitly called for removing duplicated assertions about manifest-entry internals and result construction, now owned by test/unit/screenshot/capture.test.ts. This file was not modified and the PR body does not mention the deviation. The tests work (vitest module mocks intercept all imports regardless of caller), but they now assert on transitive dependencies of captureScreenshotResult as if they were direct dependencies of the command module. Keeping them is conservative; the deviation from the plan should be acknowledged.

(Mafu-san P4, Bisky Note)

🤖

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

Re: review body P4 [DEREM-7] about test/unit/commands/screenshot.test.ts:366:

Done in dc38272. Removed the duplicated mocks.rename / mocks.createArtifactEntry / mocks.appendArtifact assertions from the offline-replay tests in test/unit/commands/screenshot.test.ts. Those manifest-entry internals are now exclusively asserted in test/unit/screenshot/capture.test.ts (the new dedicated capture-helper unit suite). The screenshot command tests now check only routing, fallback, request construction, output formatting, and the surfaced JSON envelope.

This brings Phase 4 of the implementation plan into compliance — the deviation you flagged was an oversight, not a deliberate tradeoff.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

All 9 fixed findings from R1 verified against dc38272. Every fix addresses the root cause. The showCursor: false test now guards the ternary, precondition invariants have explicit coverage, the "G1 orphan" jargon is gone, the typed intermediate catches field-name typos at compile time, and the narrowed backend type prevents lifecycle method creep. Well done.

DEREM-10 and DEREM-11 were author-accepted without tickets. These need a human decision: file tickets or explicitly accept the gaps.

As Meruem put it: "I tried to build a case against this change and could not. The problem is correctly understood. The solution is proportional."

1 P3, 4 Nit, 1 Note new this round. The P3 is the test comment claiming tracking that doesn't exist (DEREM-12, from Netero). The nits are style consistency items. 8 of 12 panel reviewers returned no new findings.

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

All 15 non-deferred findings addressed across 3 rounds. Every fix targets the cited root cause. The two deferrals have concrete tickets (#79, #80, #81). No corrections were silently dropped.

The shared captureScreenshotResult helper is clean, well-typed, and thoroughly tested. The Pick<RendererBackend, 'screenshot'> narrowing prevents lifecycle creep. The CapturedScreenshotResult intersection type propagates the sha256 guarantee without casts. The post-parse invariant defends against future schema regression. The dogfood bundle provides reproducible before/after evidence.

As Hisoka put it, after pulling every thread: "♠"

1 Nit, 1 Note this round. 8 of 8 panel reviewers returned no blocking findings.

---

test/unit/commands/screenshot.test.ts:494

Note [DEREM-20] The PR description's validation section says "22 tests pass (10 new in screenshot/capture.test.ts)" but HEAD has 23 tests (11 in capture.test.ts + 12 in screenshot.test.ts). The showCursor: false test added for DEREM-1 incremented the count. CI is green, so this is cosmetic.

(Mafu-san)

🤖

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

Re: review body Note [DEREM-20] about the PR description test count:

Updated the PR body to reflect the current test counts after DEREM-1 (showCursor: false) and DEREM-19 (it.each split):

• 27 unit tests across the two screenshot test files (15 in screenshot/capture.test.ts + 12 in commands/screenshot.test.ts)
• 8/8 in host-renderer-rpc.test.ts
• 1153 total via npm run verify

The PR description and the validation summary now match HEAD.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

R4: DEREM-19 fixed in 10833df (invariant cases migrated to it.each, 15 independent tests). DEREM-20 fixed (PR description updated to 27 unit tests). Netero and all 3 panel reviewers confirm a clean state. Zero open findings across 20 total (17 fixed, 2 deferred with tickets, 1 dropped).

Nothing left to address.

🤖 This review was automatically generated with Coder Agents.