Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 40 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
f989dc2 to
7e440f1
Compare
thymikee
changed the title
|
Updated at 7e440f1 and rebased onto c8ad8fb; range-diff preserves both commits. Warming is best-effort and non-blocking. Stop uses a deadline race without cancelling the single cached compile; unavailable tooling is reported only for MP4 verification. The local affected gate passed (exactCommit=true, run-QoD9cv), including 17,147 unit tests. The full provider lane passed all 245 tests with unchanged assertions, and ten regression mutations were caught. Cold Air run: start 4.21s with compilation pending, compile 17.87s, early stop 16.92s; later/warm stops 1.09s/0.96s. Daemon and simulator are off, pgrep is empty, and isolated caches are removed. Size: 41 files, +592/-328 (net +264; production +61). CI is pending; the parent is handling watches. |
Apex SentinelVerdict: Since the last review: 2 new · 5 still open · 2 no longer raised 7 finding(s): 1 minor, 6 nit. Verdict: approve. 🟡 Minor (1)
🔵 Nit (6)
Reply under an inline comment with Earlier reviewsReview of
|
| Where | Finding |
|---|---|
src/platform-runtime-screen-recording-android-host.ts:112 |
🆕 Android recording stop on a macOS host fails when the background validator compile needs more than 10s |
🟡 Minor (3)
| Where | Finding |
|---|---|
src/platform-runtime-screen-recording-finalizer-host.ts:63 |
Overlay no longer verifies the published video after renaming the composite over the original |
packages/capture-kit/src/recording/video.ts:57 |
Failed validator preparation is never cleared unless a new prepare() runs, so stop reports the stale failure with a hint to run prepare |
src/platform-runtime-screen-recording-android-host.ts:112 |
🆕 Android transport probe now throws video_validator_not_prepared where it used to return playable: false |
🔵 Nit (3)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/swift-cache.ts:153 |
🆕 Validator compile can no longer be cancelled; swiftc is left unowned again |
packages/provider-limrun/src/recording-runtime.ts:62 |
🆕 Duplicated webm-extension check in startTransportScreenRecording and startLimrunRecording |
src/platform-runtime-apple-application-tools.ts:99 |
🆕 Background validator preparation is fired and swallowed twice |
Review of d8bc80e0 · 03.11.2026 16:44 UTC
Apex Sentinel
Verdict: request_changes
Since the last review: 8 new · 1 still open · 8 no longer raised
9 finding(s): 2 major, 3 minor, 4 nit. Verdict: request_changes.
Required fixes
- A failed validator preparation is cached forever, so the documented "retry record stop" can never succeed
- Android host probe now throws instead of reporting an unvalidated recording, changing the Android stop/recovery path
🟠 Major (2)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:57 |
🆕 A failed validator preparation is cached forever, so the documented "retry record stop" can never succeed |
src/platform-runtime-screen-recording-android-host.ts:110 |
🆕 Android host probe now throws instead of reporting an unvalidated recording, changing the Android stop/recovery path |
🟡 Minor (3)
| Where | Finding |
|---|---|
website/docs/docs/commands.md:1068 |
🆕 Docs still say the overlay budget covers a video check that runs after the overlay, and the removed post-overlay playability probe is undocumented |
packages/capture-kit/src/recording/video.ts:226 |
🆕 Behavior change: an unavailable validator used to make MP4s read as playable, now it hard-fails the stop |
src/platform-runtime-screen-recording-finalizer-host.ts:54 |
Post-overlay playability check was dropped: an unplayable published composite now exits 0 with no warning |
🔵 Nit (4)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:50 |
🆕 videoValidatorPreparations grows one entry per Swift cache root for the life of the process |
packages/capture-kit/src/recording/video.ts:75 |
🆕 Removed the now-dead buildSwiftToolEnv import path from video.ts's validator run while keeping the import |
packages/test-kit/src/recording-video.ts:12 |
🆕 prepareRecordingValidatorFixture reads the -o argument with an index expression that passes its own guard when -o is missing |
src/platform-runtime-apple-application-tools.ts:96 |
🆕 prepareRunner now charges dynamic module loading to startedAtMs |
Review of 776aab84 · 03.11.2026 15:37 UTC
Apex Sentinel
Verdict: request_changes
Since the last review: 7 new · 2 still open · 8 no longer raised
9 finding(s): 1 major, 3 minor, 5 nit. Verdict: request_changes.
Required fixes
- Android pullPlayable now throws instead of reporting playable:false when no validator was prepared in that process
🟠 Major (1)
| Where | Finding |
|---|---|
src/platform-runtime-screen-recording-android-host.ts:108 |
Android pullPlayable now throws instead of reporting playable:false when no validator was prepared in that process |
🟡 Minor (3)
| Where | Finding |
|---|---|
src/platform-runtime-screen-recording-finalizer-host.ts:73 |
🆕 Budget-exhausted post-overlay check reports the overlay as dropped when the burned video is what shipped |
packages/capture-kit/src/screen-recording-transport.ts:99 |
🆕 Background validator preparation is started before the unsupported-option check, so an invalid --fps still spawns swiftc |
src/platform-runtime-screen-recording-finalizer-host.ts:22 |
🆕 Validator preparation reads platform and cache env from the recording client's process, not the capture host's |
🔵 Nit (5)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/swift-cache.ts:137 |
🆕 Unenforced invariant: preparation and reading hash only the Swift source, while ensureSwiftExecutable compiles the written file path |
packages/capture-kit/src/recording/video.ts:226 |
🆕 Validator-unavailable errors from a failed run drop the stderr evidence |
packages/capture-kit/src/recording/video.ts:267 |
🆕 Trailing \b fix leaves a redundant regex alternative |
packages/platform-android/src/recording/runtime.ts:68 |
WebM output still requests MP4 validator preparation on three platforms |
src/platform-runtime-apple-application-tools.ts:96 |
🆕 prepareRunner records startedAtMs before an untracked validator launch, so the timing fields can attribute the wrong wall clock |
Review of 49b95be5 · 03.11.2026 14:44 UTC
Apex Sentinel
Verdict: request_changes
Since the last review: 5 new · 5 still open · 4 no longer raised
10 finding(s): 1 major, 2 minor, 7 nit. Verdict: request_changes.
Required fixes
- record stop on a non-macOS host now hard-fails MP4 finalization where it previously tolerated the missing Swift toolchain
🟠 Major (1)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:195 |
record stop on a non-macOS host now hard-fails MP4 finalization where it previously tolerated the missing Swift toolchain |
🟡 Minor (2)
| Where | Finding |
|---|---|
src/platform-runtime-screen-recording-android-host.ts:108 |
Android pulled-chunk playability now throws instead of reporting playable:false, breaking MP4 stops on hosts without the Swift validator |
packages/provider-limrun/src/recording-runtime.ts:58 |
🆕 Limrun now refuses any .webm output path although its own help says WebM needs no Swift validator |
🔵 Nit (7)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/overlay.ts:160 |
🆕 Inconsistent validator parameter: explicitly undefined in 14 test call sites while TypeScript infers it |
packages/capture-kit/src/recording/video.ts:57 |
Background validator preparation can never be retried once the in-process map holds a failure, for every caller except the finalizer |
packages/capture-kit/src/recording/video.ts:155 |
🆕 readPreparedVideoValidator returns a rejected promise when preparation previously failed and no cached executable exists |
packages/capture-kit/src/recording/video.ts:185 |
🆕 Duplicated video_validator_not_prepared error with divergent hints inside isPlayableVideo |
packages/capture-kit/src/recording/video.ts:264 |
Removed trailing \b from the validator-unavailable regex silently widens matching |
packages/capture-kit/src/screen-recording-transport.ts:102 |
Container detection logic for finalize.prepare is duplicated in three places with different strategies |
src/platform-runtime-screen-recording-finalizer-host.ts:26 |
🆕 void prepareVideoValidator(host) in finalizer prepare swallows the rejection, so the failed flag in the module-level map is the only retry signal |
Review of a5c82f55 · 03.11.2026 13:23 UTC
Apex Sentinel
Verdict: request_changes
Since the last review: 7 new · 2 still open · 6 no longer raised
9 finding(s): 2 major, 2 minor, 5 nit. Verdict: request_changes.
Required fixes
- record stop can never recover from a failed validator preparation in the same session (stale failed promise, wrong hint)
- record stop now fails hard on hosts without AVFoundation/swiftc, where the old code accepted the MP4
🟠 Major (2)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:57 |
record stop can never recover from a failed validator preparation in the same session (stale failed promise, wrong hint) |
packages/capture-kit/src/recording/video.ts:130 |
🆕 record stop now fails hard on hosts without AVFoundation/swiftc, where the old code accepted the MP4 |
🟡 Minor (2)
| Where | Finding |
|---|---|
packages/provider-limrun/src/recording-runtime.ts:58 |
🆕 Limrun .webm refusal added, but the WebM branch in startTransportScreenRecording is dead code for Limrun |
src/platform-runtime-screen-recording-android-host.ts:112 |
🆕 A stop that never saw a prepare (daemon-restart recovery) fails forever on a hint that says to wait for a background preparation that never starts |
🔵 Nit (5)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/overlay.ts:28 |
🆕 Stale comment still claims a cold host compiles inside the overlay budget |
packages/capture-kit/src/recording/swift-cache.ts:153 |
🆕 compileSwiftSourceText no longer accepts a cancellation signal; the stale doc comment describes a caller-less feature |
packages/capture-kit/src/recording/video.ts:256 |
🆕 isSwiftVideoValidatorUnavailable: trailing \b after a closing quote never matches |
packages/capture-kit/src/screen-recording-transport.ts:102 |
The .webm container is inferred from the extension in three places |
src/platform-runtime-screen-recording-host.ts:11 |
🆕 prepare's doc says it does not wait, but the host wrapper awaits a dynamic import |
Review of 7e440f19 · 03.11.2026 12:49 UTC
Apex Sentinel
Verdict: request_changes
8 finding(s): 2 major, 2 minor, 4 nit. Verdict: request_changes.
Required fixes
- isPlayableVideo now throws for a missing/unavailable validator on a best-effort poll path (Android pull retries, overlay verification)
🟠 Major (2)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:76 |
Failed validator preparation is cached forever per cache root: no retry ever re-prepares it |
packages/capture-kit/src/recording/video.ts:177 |
isPlayableVideo now throws for a missing/unavailable validator on a best-effort poll path (Android pull retries, overlay verification) |
🟡 Minor (2)
| Where | Finding |
|---|---|
packages/platform-android/src/recording/runtime.ts:65 |
MP4 toolchain requirement is enforced only at stop; record start still accepts MP4 on hosts that cannot finalize it |
src/platform-runtime-screen-recording-finalizer-host.ts:73 |
Post-overlay playability check now fails a successful export as 'unplayable' when the budget is spent |
🔵 Nit (4)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:52 |
Unbounded videoValidatorPreparations map keyed by caller-controlled cache root |
packages/capture-kit/src/screen-recording-transport.ts:102 |
Container sniffing by .webm filename suffix is duplicated in two packages |
packages/provider-limrun/src/recording-runtime.ts:62 |
startLimrunRecording gains a new user-facing refusal — an INVALID_ARGS error for a .webm output path — but neit… |
packages/test-kit/src/recording-video.ts:12 |
prepareRecordingValidatorFixture reads the -o operand by scanning the whole argument list |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 8 new findings on 7e440f1. The verdict and the full list are in the summary comment.
| const validatorPath = await waitForValidatorCompile( | ||
| host, | ||
| deadline?.remainingMs() ?? VALIDATOR_COMPILE_WAIT_MS, | ||
| const notPrepared = () => |
There was a problem hiding this comment.
No longer raised on a5c82f5.
🟠 Major · isPlayableVideo now throws for a missing/unavailable validator on a best-effort poll path (Android pull retries, overlay verification)
The new isPlayableVideo throws AppError(COMMAND_FAILED, reason: 'video_validator_not_prepared') when validator is undefined (video.ts:187) and throws video_validator_unavailable when the validator run reports a missing toolchain (video.ts:218-219). Its documented contract is a boolean answer — waitForPlayableVideo 'resolves whether it played' — and several unchanged callers poll it as a cheap check and handle false by retrying, not by aborting:
Suggested change
Keep isPlayableVideo total for these paths: return false when no validator is prepared and let the caller's own deadline/poll budget decide, or expose an explicit assertVideoValidatorReady for the finalizer and have the Android transport and overlay verification use the boolean form so their retry/overlayWarning handling still runs.
Details
pullPlayableChunk(packages/platform-android/src/recording/chunks.ts:240-251) callstransport.pullPlayableup to PLAYABLE_PULL_ATTEMPTS times, expecting{ playable: false }while a device-side recorder finishes writing itsmoov, and keeps the remote evidence on exhaustion (pinned by 'retries a pulled MP4 until its moov is playable and retains remote evidence on exhaustion' and 'does not remove native evidence when bounded playable pulls are exhausted'). The Android transport now answers withreadPreparedVideoValidator(host)(src/platform-runtime-screen-recording-android-host.ts:115-117), which returnsundefinedwhen no warm has run — every daemon-restart recovery/reattach stop, and any first stop after a warm that already failed (see the poisoned-cache finding) — so the firstpullPlayablerejects withvideo_validator_not_preparedinstead of returning{ playable: false }, the bounded retry never runs, and the pull fails on attempt 1.overlayTouches(src/platform-runtime-screen-recording-finalizer-host.ts:75) catches everything into anoverlayWarning, so a not-prepared/unavailable throw now silently discards a composited overlay as 'failed to overlay recording touches: The video validator is not ready...' — the message blames the overlay for a validator-state problem, and the raw recording is returned without any hint thatrecord stopshould be retried.
Predecessor behavior: it never threw for a missing validator (it compiled one) and it returned true when the toolchain was unavailable. Both escape routes are gone and the boolean-only callers were not updated.
Also raised here
Also raised at this location by javascript: Android pullPlayable now throws instead of reporting playable: false, bypassing the pull retry loop — pullPlayable computes playable: result.exitCode === 0 && (await isPlayableVideo(outputPath, readPreparedVideoValidator(host))). readPreparedVideoValidator returns undefined when this process never started a preparation for that cache root and no executable is published, and isPlayableVideo throws a video_validator_not_prepared AppError for an undefined validator. The predecessor never threw here for a cold cache: it compiled the validator lazily from host and returned a boolean, and the caller pullPlayableChunk (packages/platform-android/src/recording/chunks.ts:240-251) retries transport.pullPlayable for PLAYABLE_PULL_ATTEMPTS rounds and only then reports 'failed to retrieve playable Android recording'. …
persona: senior-developer, javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -65,6 +65,7 @@ async function startAndroidRecording(params: { | |||
| }) { | |||
There was a problem hiding this comment.
No longer raised on a5c82f5.
🟡 Minor · MP4 toolchain requirement is enforced only at stop; record start still accepts MP4 on hosts that cannot finalize it
The PR now documents that 'MP4 verification requires macOS with Xcode command line tools and AVFoundation; unavailable tooling returns video_validator_unavailable only at stop' (packages/command-surface/src/recording/index.ts cliDetail, website/docs/docs/commands.md:1068). On a non-Apple host — the common Android/Harmony setup on Linux CI — the background warm at record start (packages/platform-android/src/recording/runtime.ts:68, packages/platform-harmonyos/src/recording/runtime.ts:80) fails with TOOL_MISSING, the promise is swallowed by void ...prepare(...).catch(() => {}), and the user only learns after recording up to 180s of footage that stop can never produce a validated export. startAndroidRecording and startHarmonyRecording have all the inputs needed to classify this at start (the host env plus the container decided from input.outputPath), and they already refuse other unsupported combinations up front via assertScreenRecordingOptionsSupported. Reporting a non-retriable condition as early as possible is the convention this repository follows elsewhere, and the failure is strictly more expensive after capture.
Suggested change
Either surface the warm's typed unavailable outcome at record start for MP4 destinations (or record the outcome on the session and disclose it in the start result), or state in the MP4 requirement note that record start does not check it so users on Linux hosts know to expect a stop-time failure.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| (command, args) => { | ||
| if (command !== 'xcrun' || args[0] !== 'swiftc') { | ||
| throw new Error(`Unexpected validator preparation command: ${command}`); | ||
| } |
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🔵 Nit · prepareRecordingValidatorFixture reads the -o argument with an index expression that passes its own guard when -o is missing
The new exported helper derives the compiler's output path with args[args.indexOf('-o') + 1]. When -o is absent, indexOf returns -1, so this evaluates to args[0] — the string 'swiftc' — which is truthy and so slips past the if (!output) guard; the helper then writes an executable stub to ./swiftc in the harness cwd and prepareVideoValidator resolves against a path the cache never looks at. The caller (packages/capture-kit/src/recording/swift-cache.ts:213) builds the argument list as ['swiftc', ...sourcePaths, '-o', tempExecutablePath], so today it happens to work; nothing ties the fixture to that shape. Look up the index first and reject when it is -1.
| } | |
| const outputIndex = args.indexOf('-o'); | |
| const output = outputIndex === -1 ? undefined : args[outputIndex + 1]; | |
| if (!output) throw new Error('Swift compiler fixture received no executable output'); |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
Review follow-up at Accepted fixes:
Other finding dispositions:
Validation:
Exact-head affected-gate receipt and publication status follow below. Final exact-head validation:
Publication: pushed once with the explicit lease from |
There was a problem hiding this comment.
Apex Sentinel: 7 new findings and 1 moved to a new line on a5c82f5. The verdict and the full list are in the summary comment.
| @@ -109,7 +112,9 @@ export async function createAndroidScreenRecordingTransport( | |||
| }); | |||
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🟠 Major · Android host probe now throws instead of reporting an unvalidated recording, changing the Android stop/recovery path
probeArtifact (or its equivalent) used to be lenient: isPlayableVideo(outputPath, host) returned true when the Swift validator was unavailable, because the old catch mapped any isSwiftVideoValidatorError (TOOL_MISSING, or stderr mentioning a missing swiftc/AVFoundation module) to true — "we cannot verify, treat as playable". Now the call is isPlayableVideo(outputPath, readPreparedVideoValidator(host)), and readPreparedVideoValidator returns undefined (never prepared, no published executable) or a rejected promise (preparation failed). isPlayableVideo throws video_validator_not_prepared for undefined (video.ts:198-204) and video_validator_unavailable for a rejected preparation. So the probe's playable expression rejects instead of resolving true/false.
Suggested change
At the Android probe call site, decide what an absent/failed authority means (treat as not verified rather than throwing), or pass a deadline and catch the typed not-prepared/unavailable errors so the probe still resolves a boolean for the recovery read-back.
Details
Concrete case: macOS host, Android recording, no Xcode/AVFoundation (or a preparation that failed). The old code returned playable: true and the stop proceeded; the new code aborts the probe with a non-retriable video_validator_unavailable in the middle of the Android artifact probe / recovery read-back, where the probe's purpose is only to report whether a device-side file can still become an export. Nothing in this PR's tests covers an unprepared/failed validator reaching this Android call site (the Android host tests mock the video module's old signature).
Also raised here
Also raised at this location by javascript: isPlayableVideo now throws for an unprepared validator, which escapes pullPlayable's bounded retry loop on Android — src/platform-runtime-screen-recording-android-host.ts:115-117 passes readPreparedVideoValidator(host) into isPlayableVideo, which now throws video_validator_not_prepared when the argument is undefined (video.ts:198-204) instead of producing a boolean. pullPlayable is consumed in a bounded retry loop at packages/platform-android/src/recording/chunks.ts:240-250 (if (pulled.exitCode === 0 && pulled.playable) return; ... throw new Error('failed to retrieve playable Android recording')), which was written assuming a pull answers playable: false. …
persona: javascript, senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -240,7 +255,7 @@ export async function waitForPlayableVideo( | |||
|
|
|||
| function isSwiftVideoValidatorUnavailable(stderr: string, stdout: string): boolean { | |||
There was a problem hiding this comment.
No longer raised on 776aab8.
🔵 Nit · Removed trailing \b from the validator-unavailable regex silently widens matching
The diff deletes only the trailing word boundary in isSwiftVideoValidatorUnavailable (video.ts:266): ...swiftc?["'])\b became ...swiftc?["']). The change is invisible in the description and untested in that direction: the new tests cover no such module 'AVFoundationExtras' (rejected, but that alternative is anchored by the closing quote, so it is unaffected), while nothing covers the two swiftc alternatives. Dropping the trailing boundary is what makes the unavailable classification fire on any output with text appended after the quoted swiftc/swiftc? token, so those two alternatives now match a strictly larger set of compiler output — every output that gets classified as video_validator_unavailable is reported non-retriable and the recording is refused outright, and with the new code that classification also fires on the validator's own stderr (video.ts:232-233). Either restore the boundary, or add the case the relaxation was meant to catch so the intent is encoded.
Suggested change
Restore the trailing \b, or state which real message the relaxation handles and cover it with a test each side (matching and non-matching output).
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| host: HostEnvironment; | ||
| }): ScreenRecordingRuntimeHost { | ||
| const host = options.host; | ||
| const loadFinalizer = async () => { |
There was a problem hiding this comment.
No longer raised on 49b95be.
🆕 🔵 Nit · prepare's doc says it does not wait, but the host wrapper awaits a dynamic import
ScreenRecordingFinalizer.prepare is documented as 'Starts best-effort helper preparation without waiting for it', while createScreenRecordingRuntimeHost implements it as async (input) => (await loadFinalizer()).prepare(input). That await pulls in the whole finalizer module (and its capture-kit imports) before the promise settles, so the fire-and-forget callers added in this PR actually pay module-load cost inside their .catch(() => {}) chain, and prepare is not the non-blocking call the contract advertises. Start the import eagerly, or say in the contract that prepare may await module loading.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
At a5c82f5 two open readiness threads still stand, two regression tests would pass without the code they protect, and the Android and iOS device runs are missing. All other checks pass; the only one not passing is the external review bot status, which runs no repository code. There are no conflicts. The pending-preparation test never observes its callback being entered, so it passes even if start never warms the validator and cold stop has no prepared authority. Can the test record entry before recorder launch and prove start completes while preparation remains unresolved? Deleting the invocation and awaiting preparation must each fail for the intended reason. Test The runner-preparation test checks only the mocked runner result, so it passes when the preparation call is deleted and does not protect runner warming. Can it assert preparation entry and successful runner output, and cover production finalizer preparation with a rejected compiler outcome that preserves the typed failure without failing runner preparation? Removing the preparation call must fail the test. Test The changed Android pull consumes prepared authority, but the packet contains no Android device run; the iOS Air report lacks commands, a named device, and observable output. Please run Android and iOS with an isolated cold daemon state directory without test-process prewarming, and provide commands, source SHA, device, output or artifacts, and cleanup evidence. On Android, show capture starting while compilation is pending, adb pullPlayable and finalization producing a playable MP4, a typed early readiness timeout, a successful retry without a second compile, and daemon-restart manifest recovery with no published validator. On iOS, supply auditable evidence for the 7e440f1 Air run or repeat cold start, early stop, and warm stop on the final source, including touch telemetry and compositing. Show compilation beginning before or alongside capture, none beginning from stop, and a same-session retry succeeding after video_validator_not_prepared if readiness expires. Android pull Could the existing readiness owner and Swift executable store adopt an already published executable before replaying a failed attempt, preserving the diagnostic only when no executable exists? Could MP4 resource admission and restoration initiate the same best-effort preparation outside stop, with accurate pending, absent, and failed outcomes, while readers and overlays remain compile-free? The open threads on published executable recovery and restored recording preparation still stand; no lower-priority threads still apply. The following can be resolved: playability would mask typed failure; start failure conflicts with best-effort preparation; overlay warning is not a fatal stop failure; executable lifetime has no demonstrated memory failure; format checks have distinct responsibilities; Limrun documentation now describes refusal; compiler arguments use exact token matching; unavailable outcome is required; WebM branch remains live; overlay compilation uses a separate helper; cancellation remains supported; module matching already excludes AVFoundationExtras; capture delay does not await preparation. The regression findings follow from source inspection; no mutation experiment was run. The packet does not establish live validation for the other native and provider MP4 consumers. |
|
Recovery follow-up published at
The exact-commit affected gate passed on an unchanged retry after an unrelated temp-path assertion failure was reproduced on base. Expanded focused tests passed 107 cases; real host-only cross-process recovery passed. Independent read-only review found no issues. CI/device obligations and review approval remain. Full evidence and both gate receipts are in the local recovery handoff. No Apex thread was replied to or resolved. Validation detail: receipt run-mJP5jC passed with exactCommit=true and checkoutChanged=false: 11 checks passed, eight skipped, one deduplicated. The initial failed receipt run-c4g6p8 remains recorded. Head and unchanged a5c82f5 replayed the same cli-config-trust.test.ts:40 assertion failure when the temporary path contained run-123; ordinary setup passed 19 tests and the unchanged full gate rerun passed. No test assertion, timeout, selection, allowlist or source was changed to get that pass. The pre-existing test-path collision remains a separate risk. Real host scope: the same long-lived reader retained the failed preparation, another process published the real AVFoundation executable in the isolated cache, and the reader discovered/executed it with its compiler PATH unavailable. This proves helper recovery, not a new live recording or end-to-end CLI prepare run. Earlier live recording evidence remains at its original SHA. No daemon or device session was opened for this round. |
|
Integration timeout diagnosis at CI job: proxy/direct publication parity hit its 10,000 ms test timeout; 244 other provider tests passed. The log identifies no unfinished request phase or infrastructure error. Sequential four-worker comparison in an isolated checkout:
Temporary phase instrumentation measured parity at 3.44 s on base and 3.56 s on head, with millisecond teardown. A diagnostic full head run passed, but those probes changed the tree; that is not an unmodified validation pass. All probes were removed, and the checkout is clean. No timeout, retry, skip, pool/isolation, assertion, or gate selection was changed. The parity scenario issues no recording or runner-prepare command. Its two worlds and lifecycle script are byte-identical between base and head; first-use setup/open/snapshot imports and the unchanged post-open settle dominate the measured work. This evidence establishes neither a recording regression nor a host-contention cause. The changing local timeout locations also do not establish a common cause. Local Darwin/arm64, Node 26.11.1 and pnpm 11.25.0 differ from CI Ubuntu/x64, Node 24.13.1 and pnpm 11.17.0. The next diagnostic needs that matching toolchain plus setup/request/teardown and worker delay evidence when the timeout occurs. No matching Linux runtime was available locally. The independent Android smoke assertion investigation continues separately. The passing affected receipt remains attributed to its recorded selected checks; this failed full provider lane is not overridden by that receipt. The PR is not merge-ready. |
There was a problem hiding this comment.
Apex Sentinel: 4 new findings and 2 moved to a new line on 49b95be. The verdict and the full list are in the summary comment.
| @@ -59,6 +59,9 @@ async function startLimrunRecording( | |||
| params: Parameters<typeof createLimrunScreenRecordingOperations>[0], | |||
There was a problem hiding this comment.
No longer raised on 776aab8.
🆕 🟡 Minor · Limrun now refuses any .webm output path although its own help says WebM needs no Swift validator
startLimrunRecording refuses every path ending in .webm (case-insensitively) with INVALID_ARGS. Nothing in the diff explains this with a WebM-only Limrun limitation: the change's own documentation says the opposite — the new record cliDetail says "WebM uses container validation without Swift" and commands.md:1069 says "WebM validation needs no Swift helper" — and WebM finalization really does need no Swift (isPlayableVideo returns true on the WebM container sniff, video.ts:184). The new test only asserts the refusal, not its reason. So the change adds a hard refusal of an input class whose stated justification does not hold. If the intent was only to stop .webm being silently recorded as MP4 (which is what the code does today), that belongs in the refusal message and the docs, which currently say the opposite. Also note the check sits before params.getDeviceSession, so an unknown-device call about to fail with INVALID_ARGS now answers the container error instead.
Suggested change
Either keep WebM working for Limrun (its finalization needs no Swift helper, as the new docs state) or state the real Limrun-specific reason for the refusal in the error message and in the record help/commands.md, which currently claim WebM needs no Swift tooling.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -158,6 +159,7 @@ function temporarySiblingVideoPath(videoPath: string): string { | |||
|
|
|||
| export async function overlayRecordingTouches(params: { | |||
There was a problem hiding this comment.
No longer raised on 776aab8.
🆕 🔵 Nit · Inconsistent validator parameter: explicitly undefined in 14 test call sites while TypeScript infers it
The overlayRecordingTouches signature adds validator: VideoValidator | undefined as a required parameter (not optional), which forces every call site to pass it. In overlay.test.ts, 14 call sites add validator: undefined where the value is unused. If the parameter were optional (validator?: VideoValidator), existing callers would not need a mechanical change and the tests that exercise the validator path would stand out. The required-but-always-undefined pattern makes the diff larger and hides the two call sites that actually pass a real validator.
| export async function overlayRecordingTouches(params: { | |
| export async function overlayRecordingTouches(params: { | |
| host: HostEnvironment; | |
| validator?: VideoValidator; | |
| videoPath: string; |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const runTimeoutMs = Math.min( | ||
| VALIDATOR_RUN_TIMEOUT_MS, | ||
| deadline?.remainingMs() ?? VALIDATOR_RUN_TIMEOUT_MS, | ||
| const notPrepared = () => |
There was a problem hiding this comment.
No longer raised on 776aab8.
🆕 🔵 Nit · Duplicated video_validator_not_prepared error with divergent hints inside isPlayableVideo
notPrepared() (lines 185-194) builds the video_validator_not_prepared AppError used for the timeout, while lines 196-200 build a second, near-identical AppError with the same reason but a different message and hint for the !validator case. The two hints give the operator different recovery instructions for the same reason code, so which remedy the operator sees depends on which branch fired.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
Follow-up at Accepted platform findings:
Other current review dispositions:
Android smoke repair:
Focused validation: 61 platform/cache/finalizer/overlay tests; 62 final platform/settle/wait boundaries; 11 Node coverage/report contracts. Both Linux/Windows cases exercise missing Swift tooling without compilation; malformed MP4 and missing authority still refuse. Independent platform and final combined review found no issues; the authority union was assessed as the smallest coherent design. Isolated regression proof in this checkout applied only the reviewed video.test.ts patch to AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm exec vitest run --project unit-core packages/capture-kit/src/recording/video.test.ts -t 'recording validation uses container authority without Swift preparation'Exact affected gate passed using The old CI parity timeout and different local lifecycle timeouts remain transparently recorded in the earlier diagnosis. The earlier local provider result was 244/245; its lifecycle failure logs retain that attribution. The prior |
There was a problem hiding this comment.
Apex Sentinel: 6 new findings and 2 moved to a new line on 776aab8. The verdict and the full list are in the summary comment.
| @@ -99,6 +99,11 @@ export async function startTransportScreenRecording<Collectible>(params: { | |||
| const { host, device, owner, input, signal, transport } = params; | |||
There was a problem hiding this comment.
No longer raised on d8bc80e.
🆕 🟡 Minor · Background validator preparation is started before the unsupported-option check, so an invalid --fps still spawns swiftc
startTransportScreenRecording fires host.screenRecording.finalize.prepare(...) with .catch(() => {}) immediately (lines 102-106). assertScreenRecordingOptionsSupported runs just before it, but the Apple and HarmonyOS runtimes call finalize.prepare in their own start paths after their own validation, and prepareRunner in createAppleApplicationTools fires it unconditionally. Consequence: a start request that is later refused (for example the signal aborts at signal.throwIfAborted(), or a start that throws before capture) leaves a swiftc compile running to its own 120 s bound with nobody owning it — this is exactly the orphaned-compiler situation the removed signal/SharedValidatorCompile cancellation existed to prevent (the deleted test said "Nothing owns swiftc once its requests end"). With cancellation gone there is no path that stops a preparation started by a request that was abandoned.
Suggested change
Bound the background compile (pass a timeoutMs to prepareVideoValidator / compileSwiftSourceText) and state in the comment why an abandoned start is allowed to leave swiftc running, since the PR also removes the cancellation input that used to cover this.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -20,6 +23,9 @@ export function createScreenRecordingFinalizer( | |||
| host: HostEnvironment, | |||
There was a problem hiding this comment.
No longer raised on d8bc80e.
🆕 🟡 Minor · Validator preparation reads platform and cache env from the recording client's process, not the capture host's
startTransportScreenRecording, startAndroidRecording, startHarmonyRecording and both Apple start paths call host.screenRecording.finalize.prepare({ container }) (packages/capture-kit/src/screen-recording-transport.ts:102-106, packages/platform-android/src/recording/runtime.ts:68, packages/platform-harmonyos/src/recording/runtime.ts:80, packages/platform-apple/src/recording/runtime.ts:101 and 293), and createScreenRecordingFinalizer passes the host it was built with to prepareVideoValidator (src/platform-runtime-screen-recording-finalizer-host.ts:26-28). prepareVideoValidator/readPreparedVideoValidator then branch on process.platform and getSwiftCacheRoot(host.env) (video.ts:61-64, 82-85). So the container-vs-Swift choice and the cache root are properties of whichever process owns the PlatformRuntimeHost, while the recorded bytes may come from a remote transport (Limrun instances, the WebDriver/remote host). A remote or forwarded AGENT_DEVICE_SWIFT_CACHE_DIR therefore warms — and later reads authority from — a Swift cache that has nothing to do with the machine that will finalize that recording; the stop then reports video_validator_not_prepared even though a prepare ios-runner was run on the machine the hint names. The docs now tell users to run that command "on the recording host using the same Swift cache" (website/docs/docs/commands.md:1068), which the code cannot check.
Suggested change
Say explicitly which process the preparation targets (the daemon's host environment) in the prepare contract and in the video_validator_not_prepared hint, or key the preparation on the transport's host so a remote recording warms the cache its own stop will read.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| }): Promise<string> { | ||
| }>; | ||
|
|
||
| function swiftSourceTextPaths(params: SwiftSourceTextInput) { |
There was a problem hiding this comment.
No longer raised on d8bc80e.
🆕 🔵 Nit · Unenforced invariant: preparation and reading hash only the Swift source, while ensureSwiftExecutable compiles the written file path
swiftSourceTextPaths keys the executable on [SWIFT_CACHE_VERSION, platform, arch, source], so readCompiledSwiftSourceText resolves the same video-validator-<key> binary that compileSwiftSourceText publishes. ensureSwiftExecutable, however, runs xcrun swiftc <absoluteSourcePath> -o <temp> and the sibling compileSwiftSourceFile path hashes path.resolve(sourcePath) precisely because the absolute path is part of the produced binary. The read path is only correct while the cache root — hence the source file path — is stable for a given cache key. Two processes using different AGENT_DEVICE_SWIFT_CACHE_DIR values (test fixtures and the macos-world change now create per-run temp caches, and the PR's own first CI failure was "a temp-path regex collision") produce binaries with the same name and key but compiled from different paths; a process with cache root B can then read an executable published by a process with cache root A only if it is pointed at the same root, but nothing in the key documents the dependency. If compileSwiftSourceFile's treatment of paths is the correct one, the text path's key is missing that input.
Suggested change
Either include the resolved source path (or the cache root) in the source-text cache key, matching compileSwiftSourceFile, or add a comment at swiftSourceTextPaths stating that the source-text binary must not depend on its absolute path so the shorter key is intentional.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| ); | ||
| if (runTimeoutMs <= 0) return false; | ||
| const result = await runCmd(prepared.executablePath, [filePath], { | ||
| allowFailure: true, |
There was a problem hiding this comment.
No longer raised on d8bc80e.
🆕 🔵 Nit · Validator-unavailable errors from a failed run drop the stderr evidence
unavailableVideoValidator(cause?) accepts a cause, and the compile path uses it (throw unavailableVideoValidator(error)). The run path throws unavailableVideoValidator() with no cause, so when the published validator itself reports no such module 'AVFoundation' or unable to find utility "swiftc", the message that named the missing toolchain is discarded and the operator only sees the generic hint — the one case where the device-less reproduction in this PR's notes was possible is exactly the one that loses its evidence.
| allowFailure: true, | |
| if (isSwiftVideoValidatorUnavailable(result.stderr, result.stdout)) | |
| throw unavailableVideoValidator( | |
| new AppError('COMMAND_FAILED', result.stderr || result.stdout), | |
| ); |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -240,7 +266,7 @@ export async function waitForPlayableVideo( | |||
|
|
|||
| function isSwiftVideoValidatorUnavailable(stderr: string, stdout: string): boolean { | |||
There was a problem hiding this comment.
No longer raised on d8bc80e.
🆕 🔵 Nit · Trailing \b fix leaves a redundant regex alternative
Removing the trailing \b (which could never match after the closing quote, so no such module 'AVFoundation' at end of line went undetected) is the right fix, but it makes the third alternative xcrun: error: unable to find utility ["']swiftc?["'] dead: it is already a strict substring of the second alternative unable to find utility ["']swiftc?["'], so it can never match anything the second one does not. Drop it, or the next reader will assume the two differ and keep one for the wrong reason.
| function isSwiftVideoValidatorUnavailable(stderr: string, stdout: string): boolean { | |
| function isSwiftVideoValidatorUnavailable(stderr: string, stdout: string): boolean { | |
| const combined = `${stderr}\n${stdout}`; | |
| return /\b(no such module ['"]AVFoundation['"]|unable to find utility ["']swiftc?["'])/i.test( | |
| combined, | |
| ); | |
| } |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| }) { | ||
| const { host, transport, device, owner, input, signal } = params; | ||
| signal.throwIfAborted(); | ||
| void host.screenRecording.finalize.prepare({ container: 'mp4' }).catch(() => {}); |
There was a problem hiding this comment.
No longer raised on d8bc80e.
🔵 Nit · WebM output still requests MP4 validator preparation on three platforms
startTransportScreenRecording derives the container from the output path (input.outputPath.toLowerCase().endsWith('.webm') ? 'webm' : 'mp4') so a WebM recording does not start a Swift compile, but startAndroidRecording (packages/platform-android/src/recording/runtime.ts:68), startHarmonyRecording (packages/platform-harmonyos/src/recording/runtime.ts:80) and startAppleRunnerRecording/startAppleSimulatorRecording (packages/platform-apple/src/recording/runtime.ts:101, 293) hardcode { container: 'mp4' }. On a macOS host a .webm output path through one of those runtimes therefore spends a background swiftc compile on a validator that the stop will never consult, because readVideoContainerKind returns 'webm' and isPlayableVideo resolves it from the container alone (video.ts:186).
Suggested change
Derive the container from input.outputPath in these call sites too (or move the one call into a shared start helper), so a WebM destination never arms a Swift compile.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -92,9 +96,10 @@ export function createAppleApplicationTools(env: Readonly<EnvMap>): AppleApplica | |||
| await releaseIosRunnerOnClose(deviceId, options); | |||
There was a problem hiding this comment.
No longer raised on d8bc80e.
🆕 🔵 Nit · prepareRunner records startedAtMs before an untracked validator launch, so the timing fields can attribute the wrong wall clock
startedAtMs is now taken before void prepareRecording(...) and the two await load*() calls (lines 97-101). The runner diagnostic timing therefore includes time spent in work whose failure is deliberately swallowed and never reported. If the intent is that validator warm-up is invisible to the runner timings, capture startedAtMs after those two lines as before; if it is meant to be included, the prepare command metadata should say the reported phases include validator warm-up.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
At 776aab8 non-macOS container authority and published-executable recovery are fixed. Two regression tests still pass without the code they protect, the macOS restoration thread still applies, and the device runs are missing. Smoke Tests and Coverage were still running, and there are no conflicts. Could the simulator start test observe preparation entry before recorder launch and prove start resolves while preparation remains pending? The callback currently records no entry, so deleting the invocation still satisfies the assertions by source inspection. Both deleting and awaiting preparation must fail the test; keep production finalizer coverage separate from the port double. https://github.com/callstack/agent-device/blob/776aab8/packages/platform-apple/src/recording/runtime.test.ts#L638 Could the runner preparation test assert that preparation runs and the runner succeeds? Deleting the preparation call still passes by source inspection. Exercise the production finalizer with a compiler failure at its real seam too, and prove runner success and the retained typed validator failure. Deleting preparation must fail the regression test. https://github.com/callstack/agent-device/blob/776aab8/src/platform-runtime-apple-application-tools.test.ts#L119 Please provide an isolated cold daemon state run on macOS without harness prewarming for Android recording. Show capture during compilation, typed readiness expiry, successful same-session retry without another compile, playable adb pull and finalization, and daemon-restart manifest recovery without a published validator. Include commands, source SHA, device, backend and helper version, output or artifacts, and cleanup. Retain final-head hosted Linux MP4 evidence without Swift. The posted smoke covers a prewarmed normal recording, so it does not establish these paths. https://github.com/callstack/agent-device/blob/776aab8/src/platform-runtime-screen-recording-android-host.ts#L117 Please provide auditable evidence for the earlier iOS run with its SHA, or repeat it on the final source with an isolated cold daemon state. Include commands and device, compilation beginning before or alongside capture, no compilation initiated by stop, playable early and warm exports, touch telemetry and compositing, and cleanup. If readiness expires, show the typed error and successful same-session retry. The posted timing summary and prewarmed runner with hidden touches do not establish these states. https://github.com/callstack/agent-device/blob/776aab8/packages/platform-apple/src/recording/runtime.ts#L101 Could host-validator preparation be separated from Apple runner and device admission, then connected to recording admission and restored MP4 resources? Keep the authority union and executable store, prepare restored resources outside stop, and keep stop, readers, and overlays compile-free. This would allow recovery on an Android-only Mac without an iOS target and avoid compilation during stop. The other open threads no longer apply at this head and can be resolved. |
|
Validation and review evidence for Experimental work ran only in the test worker's tool-provided isolated checkout. The parent implementation checkout stayed read-only. Setup used The same command ran in sequence with the supplied two-file test patch on AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm exec vitest run --project unit-core src/platform-runtime-screen-recording-finalizer-host.test.ts packages/capture-kit/src/recording/video.test.ts -t 'a verified overlay is published without another check|an unavailable validator at execution'
Deletion audit used the retained suite, with the obsolete post-publication canary absent: AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm exec vitest run --project unit-core packages/capture-kit/src/recording/__tests__/overlay.test.tsBaseline: 12 passed. Mutating only
The prepared-validator forwarding assertion also failed. Reversing only this worker's mutation restored 12 passed, with clean unchanged head. These retained tests cover the boundary that can preserve the raw recording before atomic rename. The unchanged-head gate passed under the normal shared validation lock: AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm check:affected --run --head 6829cbf8892bd9161f0b77c958f5822312a56d78Receipt Latest review dispositions: three fixed, six rejected.
The independent size review at Prior evidence remains attributed to |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 2 new findings and 5 moved to a new line on d8bc80e. The verdict and the full list are in the summary comment.
| - iOS recording works on simulators and CoreDevice-backed physical devices. Physical devices capture runner screenshots, not a native video stream: no audio, dropped frames are expected, and the default is 15 FPS. `--fps <n>` (1-120) caps the rate; a busy app or long command yields fewer frames. | ||
| - `--quality medium|high` (default `medium`) sets export quality on Android and Limrun; Apple export always keeps the captured resolution. Numeric `5`-`7` map to `medium` and `8`-`10` to `high`. | ||
| - With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video checks that run before and after it; an export past the budget returns the raw video with `overlayWarning`. On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first `record stop` of a long recording is more likely to return the raw video. | ||
| - On macOS, MP4 `record start` warms the video validator in the background without delaying capture. `agent-device prepare ios-runner --platform ios` also warms it without waiting or failing on build errors. `record stop` waits within its finalization budget. If preparation is still pending, stop returns retriable `video_validator_not_prepared`; preparation continues, so retry stop in the same session. macOS MP4 verification requires Xcode command line tools and AVFoundation; unavailable tooling returns `video_validator_unavailable` only at stop. Other hosts validate the MP4 container without Swift. If preparation is missing or failed, install any missing tooling, then run `agent-device prepare ios-runner --platform ios` on the recording host using the same Swift cache. Preparation runs in the background; once it is ready, retry `record stop` in the original recording session. WebM validation needs no Swift helper. |
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🟡 Minor · Docs still say the overlay budget covers a video check that runs after the overlay, and the removed post-overlay playability probe is undocumented
src/platform-runtime-screen-recording-finalizer-host.ts no longer calls isPlayableVideo after overlayRecordingTouches (the if (!(await isPlayableVideo(...))) throw new Error('recording post-processing produced an unplayable video') block is deleted), and the rewritten finalizer test now asserts expect(video.isPlayable).not.toHaveBeenCalled() after the overlay. But website/docs/docs/commands.md:1068-1069 still says the fixed budget "also covers the video checks that run before and after it" — a check that no longer runs. The same section keeps the sentence "On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first record stop of a long recording is more likely to return the raw video", which the PR's own overlay.ts comment rewrite (validator preparation no longer comes out of the budget) makes stale, and which now duplicates the new validator-warming bullet directly above it.
| - On macOS, MP4 `record start` warms the video validator in the background without delaying capture. `agent-device prepare ios-runner --platform ios` also warms it without waiting or failing on build errors. `record stop` waits within its finalization budget. If preparation is still pending, stop returns retriable `video_validator_not_prepared`; preparation continues, so retry stop in the same session. macOS MP4 verification requires Xcode command line tools and AVFoundation; unavailable tooling returns `video_validator_unavailable` only at stop. Other hosts validate the MP4 container without Swift. If preparation is missing or failed, install any missing tooling, then run `agent-device prepare ios-runner --platform ios` on the recording host using the same Swift cache. Preparation runs in the background; once it is ready, retry `record stop` in the original recording session. WebM validation needs no Swift helper. | |
| - With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video check that runs on the composited output before it replaces the raw; an export past the budget returns the raw video with `overlayWarning`. | |
Suggested change
Update the record touch-overlay bullet to say the budget covers the video checks that run before the overlay (the composited output is verified before it is renamed over the raw), and drop or rewrite the one-time-tool-setup sentence now that validator preparation is warmed at start rather than paid at stop.
Details
Also raised here
Also raised at this location by maintainability: Updated commands.md still says the overlay budget covers video checks "after" the overlay, which this change removed — The rewritten bullet keeps "which also covers the video checks that run before and after it" (website/docs/docs/commands.md:1069), but this change deleted the post-overlay isPlayableVideo call from overlayTouches in src/platform-runtime-screen-recording-finalizer-host.ts and renamed its test to "a verified overlay is published without another check". AGENTS.md: "When behavior changes, update the owning help and user docs." Drop "and after" so the page does not promise a verification that no longer runs.
persona: javascript, maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| deadline?.remainingMs() ?? VALIDATOR_RUN_TIMEOUT_MS, | ||
| ); | ||
| if (runTimeoutMs <= 0) return false; | ||
| const result = await runCmd(prepared.executablePath, [filePath], { |
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🟡 Minor · Behavior change: an unavailable validator used to make MP4s read as playable, now it hard-fails the stop
The predecessor's isPlayableVideo ended with return isSwiftVideoValidatorUnavailable(result.stderr, result.stdout) for a non-zero exit, and its catch returned true for any TOOL_MISSING / missing-module error — i.e. "cannot verify, assume playable", so a macOS host without working Swift tools still completed stops. The new code inverts that: a non-zero validator run whose stderr matches the unavailable pattern throws unavailableVideoValidator (retriable: false, video.ts:236-240), a thrown TOOL_MISSING likewise (video.ts:232), and an unprepared authority throws video_validator_not_prepared (video.ts:198-204). waitForPlayableVideo does not catch these, so finalizeScreenRecording aborts with them instead of returning the recording.
Suggested change
Add a line to the record help/docs noting that a validator that cannot run now fails the stop instead of publishing the recording unverified, and add a finalizer test for the unavailable-at-execution error reaching the caller.
Details
Concrete case: a macOS host where AVFoundation is broken (e.g. a Swift cache whose validator binary exists but fails to load the module). Every MP4 record stop on that host now fails with non-retriable video_validator_unavailable and the recording is never published, where the previous build published it with an unverified-compliance note. The new command help mentions video_validator_unavailable, but no doc line or migration note says previously-succeeding stops on such hosts now fail, and no finalizer test covers video_validator_unavailable propagating through complete (only not_prepared is covered there).
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| // for another process's compile. Nothing owns `swiftc` once its requests end, so a daemon that | ||
| // exits would leave it running. | ||
| const videoValidatorCompiles = new Map<string, SharedValidatorCompile>(); | ||
| export type VideoValidator = PreparedVideoValidator | Promise<PreparedVideoValidator>; |
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🔵 Nit · videoValidatorPreparations grows one entry per Swift cache root for the life of the process
packages/capture-kit/src/recording/video.ts:50-54 keys videoValidatorPreparations by getSwiftCacheRoot(host.env) and never evicts: a failed entry is only marked failed and replaced, and a successful entry keeps its resolved promise plus the frozen env from buildSwiftToolEnv. The removed videoValidatorCompiles map deleted its entry once a compile settled. A daemon that serves sessions with different AGENT_DEVICE_SWIFT_CACHE_DIR values (per-session or per-test-harness caches) retains every one of them plus a rejected promise whose only handler is the internal flag setter.
Suggested change
Bound the map (delete a failed entry once it has been observed, or key/evict per session) so long-lived daemons do not retain every cache root, env and settled promise they have ever prepared.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| : preparing?.promise; | ||
| } | ||
|
|
||
| function preparedVideoValidator( |
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🔵 Nit · Removed the now-dead buildSwiftToolEnv import path from video.ts's validator run while keeping the import
packages/capture-kit/src/recording/video.ts still imports buildSwiftToolEnv (line 6) and uses it only inside preparedVideoValidator (line 79), while isPlayableVideo now runs the executable with prepared.env captured at preparation time. That is correct, but it means the env captured by a background preparation is reused for a validator run arbitrarily much later; a validator run no longer sees the caller host's env. Worth one comment on preparedVideoValidator saying the env is the preparation host's, not the recording host's, since the two differ whenever the daemon serves per-session envs.
Suggested change
Add a short comment on preparedVideoValidator naming that env is frozen from the preparation host's environment and reused by every later validation run, or rebuild the tool env per run from the recording host.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -92,9 +96,10 @@ export function createAppleApplicationTools(env: Readonly<EnvMap>): AppleApplica | |||
| await releaseIosRunnerOnClose(deviceId, options); | |||
There was a problem hiding this comment.
No longer raised on 9bf6505.
🆕 🔵 Nit · prepareRunner now charges dynamic module loading to startedAtMs
src/platform-runtime-apple-application-tools.ts:99-100 hoists const startedAtMs = Date.now(); above the await loadRetry() / await loadRunnerOperations() imports that used to precede it, so the first cold call folds @agent-device/host-kit/retry and the runner-operations module load into the wall-clock figure the prepare output reports. The prepare metadata this PR just edited spells out which timings are additive, so an inflated phase total is worth a deliberate call.
Suggested change
If startedAtMs is meant to bound runner work only, keep it after the module loads; if the intent is full prepare wall-clock, note that in the prepare command help so the timing fields stay comparable across versions.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
Validation/publication evidence for The original d8 Coverage job failed in Controlled synchronization proof used the same command and environment at d8 and e6: AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm exec vitest run --project unit-core packages/platform-apple/src/launch-confirmation.test.ts -t 'one Info.plist read hangs after the app list answered'A temporary partial
Retained deadline proof at e6 used the same two-worker command with Setup/install/build and full-repository format passed. Exact unchanged-head gate: AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm check:affected --run --head e6b8b205087372975bc4a319b45d20eaba0b84c0Receipt The independent read-only d8..e6 review was clear. This scope expansion changes only CI launch-test synchronization in one file (+12/-10); production is unchanged. Earlier review dispositions remain recorded, and the already-resolved Cubic invocation thread is unchanged. No Apex/Cubic threads were touched. Prior 776aab8 hosted integration/coverage, Apple/Android/Linux smoke, 245 provider tests, and live Android six scenarios plus bootstrap retain that earlier SHA attribution. The prior narrowed 33-test coverage diagnostic exited 1 on global thresholds and is not a full coverage pass. New-head full coverage, hosted CI/device lanes, and review approval remain pending. Logs, exact experimental patches, drafts, receipt and handoff are saved under |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Validated and published This round adds two parent-authored cleanups across two files (+4/-3); overall scope is 47 files (+841/-436). Production behavior is unchanged from e6. The parent supplied the cold review of these two files; I did not perform a new independent review. Earlier architecture and launch-test reviews remain attributed to their original runs. Nine Apex dispositionsThese are the parent's dispositions of the nine findings at d8, carried forward with the two accepted cleanups in 9bf:
Validation and attribution
Apex threads were left untouched. The already-resolved Cubic invocation thread was left untouched. No CI polling, watches, merge, labels, bot messages, or thread replies/resolutions were performed. Exact-head local receipt and skips
Full receipt, test reports, logs and drafts retained in own checkout under |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 1 new finding and 3 moved to a new line on 9bf6505. The verdict and the full list are in the summary comment.
| @@ -109,7 +112,9 @@ export async function createAndroidScreenRecordingTransport( | |||
| }); | |||
There was a problem hiding this comment.
No longer raised on f026759.
🆕 🟠 Major · Android recording stop on a macOS host fails when the background validator compile needs more than 10s
isPlayableVideo now bounds its wait for a pending validator by deadline?.remainingMs() ?? VALIDATOR_RUN_TIMEOUT_MS (10s, video.ts:213). The Android pull path passes no deadline at all, so a record stop that arrives while the background compile started at record start is still running waits at most 10s and then throws video_validator_not_prepared. This diff's own removed comment states a cold Swift module compile takes over 30s on a loaded runner, so on a macOS host with a cold Swift cache the first (short) Android recording reliably fails: pullPlayable rejects, pullPlayableChunk propagates that rejection out of its retry loop (chunks.ts:246-250) instead of its own 'failed to retrieve playable Android recording', and stop returns an error. The predecessor waited for the shared compile (up to the caller deadline, or 30s without one) and succeeded. record stop retries hit the same 10s bound until the compile happens to finish, so the failing window is the whole cold compile. Please give this call site an explicit deadline (or let the pull path wait the pull budget) instead of the 10s validator-run constant.
Suggested change
Either pass an explicit Deadline (the stop/pull budget) to isPlayableVideo from the Android pullPlayable transport, or keep a longer default wait for callers without a deadline, and add a unit test that a pending compile does not fail pullPlayable inside the pull budget.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| await releaseIosRunnerOnClose(deviceId, options); | ||
| }, | ||
| prepareRunner: async (device, input, signal) => { | ||
| const startedAtMs = Date.now(); |
There was a problem hiding this comment.
No longer raised on f026759.
🆕 🔵 Nit · Background validator preparation is fired and swallowed twice
prepareRunner wraps an already-best-effort call in a second silent catch: void prepareRecording({ container: 'mp4' }).catch(() => {}), while createScreenRecordingFinalizer.prepare itself never awaits and never rejects (if (input.container === 'mp4') void prepareVideoValidator(host);). Same double void + .catch(() => {}) pattern appears at every start site (transport, Android, Apple simulator, Apple runner, HarmonyOS). The contract in ScreenRecordingFinalizer already says preparation is best-effort, so the extra swallow hides where the failure is deliberately dropped and makes it look as if prepare can reject. Keep the swallow at exactly one owner (the contract implementation) and let callers just void ...prepare(...), or let prepare return the promise so callers can await it in tests without the void.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
At 9bf6505, the earlier review’s transport ordering, authority forwarding and error handling issues are fixed, but the preparation regressions and live evidence remain outstanding. The ownership design adds no new follow-up complexity, though the restoration issue remains. The Apple start test never observes the pending preparation callback. Source deletion analysis shows that removing the invocation leaves its assertions passing, so cold-start warming remains unprotected as at d8bc80e. Can the test observe preparation before recorder launch and prove start succeeds while it remains unresolved? Deleting or awaiting the invocation must fail for the intended reason. The runner preparation test does not assert that preparation is entered, so removing warming still passes the runner assertions. The finalizer test separately mocks the validator and does not prove compiler failure through composition. Can the test assert preparation entry and runner success, and can the production finalizer and video owner be tested with a rejected compiler seam to prove a retained typed validation failure without failing runner preparation? Removing warming must fail the regression. The Android pull route lacks an auditable cold macOS run through pull and finalization; the reported smoke prepares authority before capture. Please post the source SHA, exact commands, named device, android-helper backend and helperVersion, outputs, artifacts and cleanup. Show capture during pending compilation, typed early readiness expiry, successful same-session retry without a second compile, playable adb pull and finalization, and manifest recovery after daemon restart before publication. Retain attribution for hosted non-macOS MP4 validation without Swift. The earlier Air timing summary lacks an auditable transcript, and the posted iOS harness prepares the validator before capture and hides touches. Please supply its source SHA, named device, commands, outputs, artifacts and cleanup, or repeat on final production source with an isolated cold daemon state. Show preparation before or alongside capture, no compile initiated by stop, playable early and warm exports, and touch telemetry and compositing. If readiness expiry is induced, show its typed reason and successful same-session retry. The open threads on restoration, restoration admission, test diagnostics, overlay documentation, and Android readiness budget still stand. The other open threads no longer apply at this head and can be resolved. |
A background video-validator compile kept running after the daemon exited. process.exit() ended the supervisor while swiftc was still a live child, so the daemon left an orphaned compiler and a held cache lock. The shutdown path had no owner for the preparation because the module-level prepare/read API did not track the running promise. Give the preparation one owner per runtime host. createVideoValidatorPreparation() holds the promise and an AbortController, so close() can cancel it and await the settlement. The host closes that owner while it shuts down, before the Apple runner sweep. Late host imports close the owner the import produced, so a warm-up that starts after shutdown compiles nothing. Restore signal propagation from compileSwiftSourceText through ensureSwiftExecutable into the cache lock and runCmd, so cancellation terminates the compiler child and removes the lock and temp paths. Cross-process dedup stays with the filesystem cache lock. Stop no longer looks like a cancelled compile, and one host closing does not cancel another host's compiler.
|
Validation evidence for At Hosted coverage, device and startup validation remain pending at this head. Older b92 startup/hash evidence does not validate this production-changing head. |
There was a problem hiding this comment.
All reported issues were addressed across 21 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Validation evidence for Independent reviews cleared the final delta, including the prior cleanup finding. The real-lock test covers two in-flight owners and warm reuse. Baseline and restored proof runs passed both tests. Removing second-compiler abort produced the explicit bounded cleanup failure and the exact cache was removed. A shared-controller mutation was detected by the bounded compiler-count poll (expected 2, got 1) and the slow-test guard; this is timing-based detection, not an immediate non-timeout failure. All eight roots recorded in the final proof were absent. The implementation handoff also checked its recorded real-child roots and PIDs absent; this does not account for all historical experiments. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 2 new findings and 5 moved to a new line on f026759. The verdict and the full list are in the summary comment.
| } from '@agent-device/capture-kit/recording-video'; | ||
| import { loadAndroidMechanics } from './platform-runtime-android-mechanics.ts'; | ||
|
|
||
| const ANDROID_MANIFEST_NAME = 'agent-device-recording-active.json'; |
There was a problem hiding this comment.
🟡 Minor · Android/Harmony MP4 stops on a macOS daemon now depend on Apple validator preparation and lose the old graceful pass
createAndroidScreenRecordingTransport now hands the Apple validator owner to isPlayableVideo (src/platform-runtime-screen-recording-android-host.ts:16,114): isPlayableVideo(outputPath, validator.read()), with no deadline. The host it used to receive is gone, so an Android (or HarmonyOS, whose collected MP4 goes through the same finalizer) recording validated on a macOS daemon is validated by the AVFoundation executable chosen by createVideoValidatorPreparation for the daemon's Swift cache. Two behaviors the predecessor had are lost for that caller: (1) it compiled the validator on demand (old waitForValidatorCompile, bounded by 30 s or the caller's deadline), whereas now read() answers undefined when this daemon never prepared, so the Android transport's playable check throws video_validator_not_prepared and the stop fails; (2) a TOOL_MISSING / missing-AVFoundation host previously returned true from isPlayableVideo (old isSwiftVideoValidatorError → return true), so a machine without Xcode tooling still reported and kept its Android recording; now video.ts:264-270, 296-306 raise video_validator_unavailable with retriable: false, and in waitForPlayableVideo that throw escapes the polling loop and fails the whole stop. So an Android record stop on a macOS host that lacks AVFoundation, or whose Apple preparation never ran, now errors out on a perfectly good MP4 — and the hint it shows tells the operator to run agent-device prepare ios-runner --platform ios, an Apple-runner command, for an Android capture. The docs sentence "Other hosts validate the MP4 container without Swift" describes non-macOS hosts only, so it does not cover this daemon. Consider letting non-Apple transports request container-only authority ({ kind: 'container' }) instead of the shared AVFoundation owner, and returning the old graceful true where an unavailable validator previously degraded, or making video_validator_unavailable retriable.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| throw error; | ||
| } | ||
| if (!(await waitForPlayableVideo(outputPath, { host, deadline }))) { | ||
| if (!(await waitForPlayableVideo(outputPath, { validator: params.validator, deadline }))) { |
There was a problem hiding this comment.
🔵 Nit · Overlay discards a successfully exported composite when validator preparation is still pending
finalizeScreenRecording reads the preparation once (const prepared = validator.read()) and hands that same promise to waitForPlayableVideo both for the raw check and, through overlayTouches, to exportProcessedVideo. The predecessor verified the composited output with the host (waitForPlayableVideo(outputPath, { host, deadline })), which compiled the validator on demand, so the check always had an authority by the time the export finished.
| if (!(await waitForPlayableVideo(outputPath, { validator: params.validator, deadline }))) { | |
| if (!(await waitForPlayableVideo(outputPath, { validator: params.validator, deadline }))) { | |
| exportOverran = deadline.isExpired(); | |
| throw new AppError( | |
| 'COMMAND_FAILED', | |
| isVideoValidatorNotPreparedError(error) ? error.message : 'The composited video was not verified as playable', | |
| ); | |
| } |
Suggested change
Do not reuse the pending preparation promise as the authority for verifying the composited output. Either re-read validator.read() after the export (so an executable published during the export is used), or skip the composite verification with an explicit typed reason instead of deleting a composite the helper already produced.
Details
Now, after a long overlay export the deadline has little left, and isPlayableVideo races the still-pending preparation promise against deadline.remainingMs(). If preparation has not settled within what the overlay left of OVERLAY_BUDGET_MS, the race rejects with video_validator_not_prepared, exportProcessedVideo treats it as The composited video was not verified as playable, deletes the exported file in its finally, and the stop returns the raw video with an overlayWarning — even though the helper exported the composite successfully. The same happens when read() answered undefined (nothing prepared and nothing published): the raw-video check in finalizeScreenRecording throws out of complete, so showTouches stops can only ever keep the raw video.
The verified composite is thrown away for a reason unrelated to the export, and the user-visible fix (retry stop) re-runs the whole export.
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| @@ -177,9 +190,16 @@ async function ensureSwiftExecutable(params: { | |||
| const executableDir = path.dirname(params.executablePath); | |||
There was a problem hiding this comment.
🔵 Nit · ensureSwiftExecutable now waits for a canceled compile instead of releasing the Swift cache lock
swift-cache.ts replaced withProcessLock with a manual acquireSwiftCacheLock + try/finally-style release, and the release now happens after the compile task settles. Under the old withProcessLock, acquire and task were separate, so canceling a compile (AbortSignal, or the timeout in the new test at swift-cache.test.ts:175) released the lock while swiftc was still being killed. Now a canceled or timed-out compile holds ${executablePath}.lock until runCmd's SIGINT grace (graceMs: 2_000 per the new assertion) plus process teardown have passed, and only then gives the lock back. createVideoValidatorPreparation.close() and compileSwiftSourceText's own timeoutMs both take that path, so a second owner waiting on that cache must now wait the full kill-grace on top of timeoutMs (default 120_000) before it can compile — and the lock-acquire wait inside it uses the same timeoutMs, so it can time out with process_lock_timeout where it previously succeeded. The video-validator owner deliberately cancels its compile on shutdown (video.ts close), which is exactly the case that leaves the lock held for seconds.
Suggested change
Release the cache lock as soon as the compile stops waiting (before awaiting the killed compiler's exit), or document and bound the extra lock hold time a canceled compile now imposes on the next owner.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| } | ||
|
|
||
| export async function waitForStableFile( | ||
| filePath: string, |
There was a problem hiding this comment.
🆕 🔵 Nit · isUnavailableVideoValidatorError reuses arbitrary compile stderr as the reason an MP4 is not playable
The new check accepts any AppError whose details.stderr happens to contain the AVFoundation/swiftc phrase (isSwiftVideoValidatorUnavailable(String(error.details?.stderr ?? ''), …)) and rethrows it through unavailableVideoValidator, whose spread ...(cause instanceof AppError ? cause.details : {}) copies that untrusted stderr/stdout straight into the user-facing failure details (the new video.test.ts case asserts details.stderr is the whole message). The same classifier is then applied to the validator run's output at video.ts:296 to decide whether the video is playable. A command failure or a wrapper error that echoes a compiler log containing no such module 'AVFoundation' — e.g. a cached stderr from a different source file, or a build log with several modules — is reported as video_validator_unavailable, retriable: false and permanently refuses the recording until the toolchain is re-prepared, even though the container itself was fine.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const validatorPath = await waitForValidatorCompile( | ||
| host, | ||
| deadline?.remainingMs() ?? VALIDATOR_COMPILE_WAIT_MS, | ||
| const notPrepared = () => |
There was a problem hiding this comment.
🔵 Nit · A stop whose preparation never started returns retriable video_validator_not_prepared, but nothing on the stop path ever starts it — retries cannot recover
isPlayableVideo has two different video_validator_not_prepared errors. The deadline one (video.ts:254-263) says "Preparation continues in the background. Retry record stop in this session once the validator is ready." — true only when a preparation is actually in flight. The no-validator one (video.ts:264-270) is raised when VideoValidatorPreparation.read() answered undefined, which by its own contract (video.ts:105-117) means this host never prepared anything and no executable is published in the cache. createVideoValidatorPreparation.prepare() is only ever reached from a recording start (createScreenRecordingFinalizer.prepare is called from startTransportScreenRecording, startAndroidRecording, startAppleSimulatorRecording, startAppleRunnerRecording, startHarmonyRecording, prepareRunner). So a recording recovered by a restarted daemon — the documented recovery path where record stop finishes a recording a previous daemon left — has a host whose validator promise was never created, read() returns undefined, and every retry of record stop re-raises the same retriable error forever: no stop-path code calls prepare(), so the background preparation the error tells the user to wait for does not exist. Sequence: macOS daemon starts an MP4 recording, is killed mid-compile (its unpublished output and lock are reclaimed but nothing was published), the new daemon's record stop on the recovered recording fails with video_validator_not_prepared / retriable: true, and record stop retried N times produces the identical error; only an unrelated Apple command (prepare ios-runner --platform ios) or a fresh record start clears it. Either start preparation from finalizeScreenRecording when read() answers undefined (bounded by the finalization deadline, as before), or mark that outcome non-retriable and name the command that actually starts it, as the sibling error already does.
Details
persona: senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const { host, device, owner, input, signal, transport } = params; | ||
| assertScreenRecordingOptionsSupported(input, params.support, params.unsupported); | ||
| signal.throwIfAborted(); | ||
| void host.screenRecording.finalize |
There was a problem hiding this comment.
🔵 Nit · Container kind is inferred from the output path in two places that will drift
input.outputPath.toLowerCase().endsWith('.webm') ? 'webm' : 'mp4' is added in screen-recording-transport.ts:102-106, and provider-limrun/recording-runtime.ts:62 adds the same input.outputPath.toLowerCase().endsWith('.webm') test as a rejection condition. hasVideoContainer already owns container naming in capture-kit/recording-video. Keep one helper (e.g. recordingContainerFor(outputPath)) next to the container reader so a third consumer — or a change to what counts as WebM — cannot make the transport warm the validator for a container the backend refuses.
Suggested change
Extract the path-to-container decision into one shared helper and use it from the transport warm-up and the Limrun argument check.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| const host = options.host; | ||
| // One preparation owner for this host, created when a recording first asks for it. Compilation | ||
| // across runtimes and processes stays with the Swift cache lock, not a second lock database. | ||
| let validator: Promise<VideoValidatorPreparation> | undefined; |
There was a problem hiding this comment.
🆕 🔵 Nit · Validator preparation selects container vs AVFoundation from process.platform at call time, not from the host it was built with
createVideoValidatorPreparation(host) captures a HostEnvironment (its env selects the Swift cache root) but chooses container-vs-AVFoundation authority from the live process.platform on every prepare()/read() call, while swiftSourceTextPaths hashes process.platform and process.arch into the cache key. createScreenRecordingRuntimeHost builds exactly one owner from the host under construction and keeps it for the host's lifetime, so the authority a stop uses depends on process.platform at the moment of the call rather than on the host the owner was created for.
Suggested change
Decide the validation kind once, from the host the preparation was created for (e.g. keep a platform captured at construction alongside host), and use that same value for both prepare() and read() and for the cache key, so the authority read back always matches the authority prepared.
Details
A host object captured for a different platform (the tests in this PR exercise exactly this by redefining process.platform, and platform-runtime-operation-host-validator-shutdown.test.ts does so for the whole file) therefore answers prepare() with an AVFoundation compile while read() returns container authority — or vice versa — and the compile that prepare() started is never read back. Nothing in the type or the code ties the chosen kind to the host the owner was constructed with, so the invariant holds only while process.platform never changes for that host.
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
The review of f026759 still finds gaps in warming coverage, shutdown diagnostics, and live recording evidence. The bounded launch assertion, non-macOS documentation, owner isolation, and compiler fixture issues from the earlier review are fixed; the warming regressions remain. The pending-preparation test never observes entry into preparation, so deleting the start invocation still satisfies its assertions. A future loss of warming could make cold macOS MP4 stops fail while the test passes. Please assert preparation entry before recorder launch and that capture start resolves while preparation remains pending; verify that deleting or awaiting the invocation fails for the intended reason. The runner test also never asserts entry into its throwing preparation callback, so deleting warming still passes. The requested compiler-failure composition remains absent: the finalizer test substitutes a preparation stub, and the new deadline test covers pending or successful compilation. Please assert entry and runner success, exercise runner warming through the production finalizer and video owner with failure at runCmd, and assert the retained typed stop error. Verify that deleting warming makes the regression fail. The changed Android pull path lacks auditable cold macOS recording evidence. The live harness prepares before recording, so it cannot establish the pending compilation, pull readiness deadline, or restart behavior. Please post a final-production-SHA run with an isolated cold daemon and storage, exact commands, named Android device, android-helper backend and helperVersion, outputs, artifacts, and cleanup. Show capture during compilation, typed readiness expiry, successful same-session retry without another compile, playable adb pull and finalization, and manifest recovery after daemon restart before publication. Retain separately attributed hosted non-macOS MP4 and no-Swift evidence. The iOS capture change also lacks an auditable cold, early, and warm recording transcript with touch compositing. The earlier Air report has timings only, and the simulator harness prepares first and hides touches. Please provide the complete earlier transcript with its source SHA or repeat on final production source with an isolated cold daemon and storage, named device, exact commands, outputs, artifacts, and cleanup. Show preparation before or alongside capture, no compile initiated by stop, playable early and warm exports, touch telemetry and composite, unfinished-compile shutdown cleanup, and reuse of the published executable. If readiness expires, show its typed reason and a successful same-session retry. Lifecycle finalization can throw an AggregateError when cleanup fails, but daemon close calls it directly after platform shutdown. The platform shutdown normalizer therefore does not handle these errors, and daemon diagnostics lose the nested typed validator, temporary-file, and lock failures. The new tests insert a platform-owner wrapper that production does not use. Please preserve sanitized typed cleanup details at the lifecycle boundary and expose them through actual daemon close diagnostics, with coverage of that route. Could the lifecycle own the typed sanitized cleanup failure and use existing daemon normalization, while keeping the per-runtime preparation owner and filesystem lock? Please replace the artificial owner-wrapper tests with actual daemon shutdown coverage; the restored-recording admission question remains open. The threads on cold restoration, pull readiness, cleanup bounds, and restored preparation still stand. All 21 checks pass at this head; the earlier startup regression is not yet shown to be fixed. No conflicts are known. Before merge, please fix the warming tests and lifecycle diagnostics, address the remaining restoration, readiness, and cleanup threads, and provide the cold Android and iOS evidence. |
|
Validation at Close-stall regression: focused baseline 3 passed; restoring the unbounded re-await mutant produced 1 failure and 2 skips at the explicit three-second status assertion, before Vitest's ten-second timeout; restored code passed all 3. The mutant's cache root was absent after runner fixture cleanup, not the stalled scenario's own cleanup. The test invokes the real second close and withholds its result; it does not prove arbitrary internal owner stalls. Independent static reviews of the cleanup and final bound found no issues. Prior CI and approval at |
|
At b1150a8, the fixture guard, diagnostic matching, format validation and overlay documentation address parts of the earlier review, but the findings below remain. CI reports 20 passing checks. No conflicts are known. Before merge, repair warming coverage and shutdown diagnostics, resolve restoration readiness and cleanup bounds, and provide the cold device evidence below. The pending callback test does not observe preparation, so deleting its start call leaves the assertions passing. Lost background warming could regress cold MP4 stop unnoticed. Assert preparation enters before recorder launch and capture resolves while it remains pending; verify that deleting or awaiting preparation makes the test fail. The throwing callback test does not observe warming, so deleting it still passes. Assert callback entry, then exercise runner warming through the actual finalizer and owner with a runCmd failure and assert the typed stop error. Could the lifecycle boundary normalize sanitized typed nested failures when runner and validator cleanup both fail? AggregateError currently escapes the platform shutdown normalizer, and the test uses a platform owner that production does not, so daemon diagnostics can lose validator, temporary-file and lock details. Cover actual daemon close diagnostics. The Android pull path still needs an auditable final-production cold macOS daemon run. Please record the device, helper backend and version, commands, outputs, artifacts and cleanup; show capture during compilation, typed expiry, same-session retry without a new compile, playable pull and finalization, and restart recovery. Keep non-macOS MP4 and no-Swift evidence separately attributed. The iOS recording path still needs an attributable cold, early and warm transcript: the available simulator harness prepares first and hides touches. Please show capture alongside preparation, no stop compile, playable early and warm exports, touch composite and telemetry, shutdown cleanup and published executable reuse; show typed expiry and retry if reached. The cleanup bound thread still applies, along with 12 lower-priority threads. The new stalled-close test settles the real close first, so it does not cover a preparation that never settles. |
Summary
MP4 validator preparation runs in the background without delaying capture. Stop uses cached or started preparation without canceling it at its deadline. macOS uses AVFoundation; other hosts validate the container. Missing authority returns typed errors; recover with
agent-device prepare ios-runner --platform ioson the recording host using the same Swift cache, then retry stop in the original session.Shutdown cancels and awaits owned unfinished compilation and cleanup; published executables remain reusable. A second same-cache owner can continue after the first closes. Cleanup failures remain typed. Non-macOS touch visualization returns raw video and telemetry with
overlayWarning. Launch waits are bounded; Android smoke uses settled Back and confirmed fill. Follow-up to #4150.Scope: lifecycle, recording, tests and docs; 56 files, +2,136/-427 against
c8ad8fb1257787ce0973cf74271cb9a4fafb51d9. The finalf026759..b1150a8change is test-only: bounded close-stall cleanup and regression coverage; no runtime behavior changes.Validation
At
b1150a8715055777d9f9788c521f4374cd313c19,pnpm check:affected --run --head b1150a8715055777d9f9788c521f4374cd313c19passed (exact commit, unchanged checkout): 13 passed, 8 skipped, 1 deduplicated; 1,230 related and 17,188 unit tests passed, 1 unit skipped. See the evidence comment for the regression proof and limits.Hosted coverage and device checks remain pending at this head. The startup CI increase at
7ffdid not reproduce in bounded macOS base/head/base comparisons; its cause remains unproven.