Repository navigation
fix(recording): preserve explicit macOS session surfaces - #3217
Conversation
|
Fresh review of f90a483 against 2212eac: no confirmed blocking issues in the five-file diff. The canonical recorder now preserves explicitly supplied surface values, and the open script serializer/parser round-trip all four values while retaining omission. Tests exercise the actual SessionStore event/script boundary and reject malformed surface arguments. Published head/base and validation claims match the reviewed source and terminal check evidence. Live macOS replay remains unverified; this review does not count it as a pass. Named Claude and codex-rescue review tools are unavailable in this session; this is an independent Codex review. |
|
The code looks right at f90a483, but the Coverage check fails and live macOS evidence is still missing. The failure is caused by this diff: the new import of Please keep the surface check but stop adding modules to the closure. One way is The PR also changes replay on a device route: Not blocking, take or leave: No conflicts. Before merge, the new import in |
Live macOS replay checkHead: Each leg used a fresh session name with no active sessions. I ran Result: all three legs pass.
desktop
Replay: menubar
Replay: surface omitted
The script does not contain Side note: surface from a config fileI also set |
|
I pushed two commits to finish this PR:
I left the CI passes. It is still a draft, so please mark it ready when you agree. |
|
This PR is ready at c98897a. The earlier evidence gap is closed, and the checks are green: 15 checks, none failing, including the Coverage job that runs the eager-closure budgets. Commit 87a4b1a removes the runtime contracts import that overlapped that job. Not blocking: the replay surface parser in packages/ad-script/src/internal/open-script.ts (https://github.com/callstack/agent-device/blob/c98897a/packages/ad-script/src/internal/open-script.ts#L97) repeats the trim, lowercase and "Invalid surface" message logic from parseSessionSurface in packages/contracts/src/session-surface.ts. The Record<SessionSurface, true> typing keeps the value list in sync, but the normalization and message text can still drift. If the closure budget is raised, or the contracts parser gets a lighter entry, you could delete the local copy and import parseSessionSurface. Take it or leave it. Evidence limits: I did not run the eager-closure-budgets tests locally. That pass rests on the CI summary and a read of the import graph. The live macOS run was on f90a483, not c98897a. I judged the delta not to touch the device route, since it only swaps the parser and edits docs, but the new parser was not live-run. I did not reproduce the "all 30 tests failed without it" claim. There are no review threads on this head. No conflicts. The PR is still a draft, so you or a maintainer can mark it ready for merge. |
thymikee
added
the
ready-for-human
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ad-script/src/internal/open-script.ts">
<violation number="1" location="packages/ad-script/src/internal/open-script.ts:113">
P3: A script line ending in `--surface` (or `--surface` followed directly by another flag) passes `undefined` to `parseReplaySurface`, so the error reads `Invalid surface: undefined. Use app|frontmost-app|desktop|menubar.` — the message leaks the word `undefined` instead of saying the value is missing. Special-case the missing-value case with its own message before validating the value.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| if (normalized !== undefined && isReplaySurface(normalized)) return normalized; | ||
| throw new AppError( | ||
| 'INVALID_ARGS', | ||
| `Invalid surface: ${value}. Use ${Object.keys(REPLAY_SURFACES).join('|')}.`, |
There was a problem hiding this comment.
P3: A script line ending in --surface (or --surface followed directly by another flag) passes undefined to parseReplaySurface, so the error reads Invalid surface: undefined. Use app|frontmost-app|desktop|menubar. — the message leaks the word undefined instead of saying the value is missing. Special-case the missing-value case with its own message before validating the value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/ad-script/src/internal/open-script.ts, line 113:
<comment>A script line ending in `--surface` (or `--surface` followed directly by another flag) passes `undefined` to `parseReplaySurface`, so the error reads `Invalid surface: undefined. Use app|frontmost-app|desktop|menubar.` — the message leaks the word `undefined` instead of saying the value is missing. Special-case the missing-value case with its own message before validating the value.</comment>
<file context>
@@ -84,6 +94,26 @@ export function parseReplayOpenFlags(args: string[]): {
+ if (normalized !== undefined && isReplaySurface(normalized)) return normalized;
+ throw new AppError(
+ 'INVALID_ARGS',
+ `Invalid surface: ${value}. Use ${Object.keys(REPLAY_SURFACES).join('|')}.`,
+ );
+}
</file context>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Surface parsing duplicates the canonical contract, and public documentation and installed help need correction.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Preserves explicit macOS surfaces across action recording, journal events, and saved replay scripts while retaining omission semantics.
Changes:
- Records and serializes explicit
open --surfacevalues. - Validates replayed surface arguments.
- Adds regression coverage and documentation.
| File | Description |
|---|---|
website/docs/docs/commands.md |
Documents saved-script surface behavior. |
src/daemon/__tests__/session-store.test.ts |
Tests recording and round trips for all surfaces. |
packages/command-registry/src/flag-definitions-target.ts |
Enables surface recording. |
packages/ad-script/src/internal/open-script.ts |
Serializes and parses replay surfaces. |
packages/ad-script/src/internal/__tests__/open-script.test.ts |
Tests malformed surface rejection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const REPLAY_SURFACES: Record<SessionSurface, true> = { | ||
| app: true, | ||
| 'frontmost-app': true, | ||
| desktop: true, | ||
| menubar: true, | ||
| }; |
| usageDescription: 'macOS session surface for open (defaults to app)', | ||
| projectConfig: true, | ||
| recorded: false, | ||
| recorded: true, |
| - `open <app> --launch-console <path>` captures launch-time stdout/stderr for direct iOS simulator app launches. It is not valid for URL opens or | ||
| non-simulator targets. | ||
| - `open --platform macos --surface app|frontmost-app|desktop|menubar` selects the macOS session surface explicitly. `app` is the default when an app argument is provided. | ||
| - A surface given to `open`, as a flag or from a config file, is saved in `--save-script` scripts, so `replay` reopens the same surface. When no surface is given, the script has no `--surface`, and replay opens an app session. |
Summary
open --platform macos --surface desktoploses its surface in recorded actions and saved replay scripts, so replay can target a different surface. Preserve explicitly supplied values in the existing recorder, journal projection, and.adopen round trip; keep omission unchanged. Closes #3216.The recording-policy migration preserved the old allowlist's omission; macOS surface selection needs to survive recording.
Validation
Tested five-file commit
f90a483with an actual SessionStore event/script regression for all four surfaces and omission, plus malformed replay arguments. Without the recording fix, explicit journal values disappear; without the serializer fix, replay loses them. Both regressions fail before their fix and the complete focused family passes (42 tests).pnpm check:affected --runpassed on this exact commit: formatting, lint, types, layering, Fallow, build, and 5,170 related tests across 660 files. Live macOS replay and GitHub CI are pending; the shared installed daemon remains unchanged.