Repository navigation
fix: Drivers CI Compile Gate + Audit Defect Fixes - #167
Merged
Merged
Conversation
Adds .github/workflows/drivers.yml (Android + iOS compile-only build, never previously executed) and fixes ~15 confirmed defects from a repo-wide comment-quality audit: Android tapPercent divided a 0-1 fraction by 100 (every percent tap landed near the top-left corner), iOS fabricated success for five no-op actions, log write-streams were never ended (truncated logs), a recording could be lost on a failing rename, driver recovery could replay mutating actions, and more.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a compile-only Android/iOS drivers workflow and fixes cross-platform driver contracts, device-node reliability, CLI PATH probing, cloud timeout parsing, test-path discovery, and supporting documentation and change records. ChangesDrivers CI and change planning
Device-node runtime reliability
CLI and cloud contracts
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
fab/changes/260730-zga4-drivers-ci-gate-audit-defects/plan.md (1)
457-457: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the Swift isolation requirement explicit before marking R5 complete.
The task still records a Swift 6 concurrency risk for
defaultScreenshotQualitybeing read fromnonisolatedmethods while declared in an@MainActorclass. Preferprivate nonisolated static let, or verify the exact project toolchain accepts the current declaration in the new workflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fab/changes/260730-zga4-drivers-ci-gate-audit-defects/plan.md` at line 457, Update the defaultScreenshotQuality declaration in GrpcDriverServer.swift to explicitly mark the static constant nonisolated, or verify and document that the project’s Swift 6 toolchain accepts the current isolation. Prefer the private nonisolated static let declaration so the nonisolated getScreenshot and getRawScreenshot methods can access it safely before marking R5 complete.packages/device-node/src/device/IOSRecordingProvider.ts (1)
199-235: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRename-over-delete fix looks correct.
Not deleting the original before the atomic same-directory rename correctly closes the "recording destroyed on rename failure" defect; verified against the new tests.
One minor note: if
fsp.renamethrows (line 234), the compressed file produced by ffmpeg is left behind with no cleanup — low risk given same-directory renames rarely fail, but a stray file could accumulate on repeated failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/device-node/src/device/IOSRecordingProvider.ts` around lines 199 - 235, Update _compressVideo to clean up compressedFilePath when fsp.rename fails, while preserving the original file and propagating the rename error. Scope cleanup to the rename failure path and avoid removing the compressed file after a successful rename.docs/memory/device-node/log-capture.md (1)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOuter-catch trigger list omits the success-path
finalize()failing.This says
finalizeQuietlyruns in the outercatch"when_waitForExitthrows," but inAndroidLogcatProvider.stopLogCapture/IOSLogProvider.stopLogCapturethe success-pathfinalize()call (described two lines above as "the success path's call") sits in the same try block. If thatfinalize()rejects (a write/flush error), it also lands in this outer catch, not just a_waitForExitrejection. Worth naming both triggers so a reader doesn't conclude the redundantfinalizeQuietlyonly guards against_waitForExit.✏️ Proposed wording
- (an early return), and each provider's outer `catch` when `_waitForExit` throws. Those paths still + (an early return), and each provider's outer `catch` when `_waitForExit` throws or the success + path's own `finalize` call throws. Those paths still🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/memory/device-node/log-capture.md` around lines 61 - 70, Update the documentation describing the outer catch paths for AndroidLogcatProvider.stopLogCapture and IOSLogProvider.stopLogCapture to state that finalizeQuietly also handles failures when the success-path finalize call rejects, in addition to _waitForExit throwing. Keep the distinction between throwing finalize and swallowing finalizeQuietly unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drivers/ios/finalrun-ios-test/Managers/XCViewHierarchyManager.swift`:
- Around line 64-69: Clamp the requested FPS in the gRPC streaming handler
before calculating frameDelayNs, using the same 1...60 bounds as
XCViewHierarchyManager’s startStreaming flow. Update the handler in
GrpcDriverServer around the frame-delay calculation so zero or out-of-range
requests are normalized before division, while preserving valid requested FPS
values.
In `@fab/changes/260730-zga4-drivers-ci-gate-audit-defects/plan.md`:
- Line 470: Document the retained public killDriver APIs in DeviceRuntime,
DeviceAgent, and the Device.killDriver() facade to explicitly state that they
close the driver channel rather than terminate the process. Keep the existing
signatures and behavior unchanged, and update the acceptance claim to reflect
that all three declarations are documented.
- Around line 464-465: Extend logCaptureProviders.test.ts with regression tests
covering AndroidLogcatProvider and IOSLogProvider when SIGINT delivery or
_waitForExit fails, verifying stopLogCapture still finalizes the log stream and
cleans up its registry entry. Exercise each provider error path and assert the
output is finalized without requiring a second stop.
In `@packages/device-node/src/device/ios/test/IOSSimulator.test.ts`:
- Around line 599-600: Remove the duplicate block-scoped declarations causing
TypeScript compilation failures: in
packages/device-node/src/device/ios/test/IOSSimulator.test.ts lines 599-600,
keep only one tapCalls declaration within the IOSSimulator mutating-action test;
in packages/device-node/src/device/test/Device.test.ts lines 341-342, keep only
one notifications declaration.
In `@packages/device-node/src/grpc/GrpcDriverClient.ts`:
- Around line 345-349: Update launchApp() to stop configuring retries for the
mutating launch request: remove or set its maxRetries to the non-retrying value
used by _unaryCall(). Preserve retry behavior for read-only and explicitly
replayable operations, while ensuring a launch is issued only once before driver
recovery.
---
Nitpick comments:
In `@docs/memory/device-node/log-capture.md`:
- Around line 61-70: Update the documentation describing the outer catch paths
for AndroidLogcatProvider.stopLogCapture and IOSLogProvider.stopLogCapture to
state that finalizeQuietly also handles failures when the success-path finalize
call rejects, in addition to _waitForExit throwing. Keep the distinction between
throwing finalize and swallowing finalizeQuietly unchanged.
In `@fab/changes/260730-zga4-drivers-ci-gate-audit-defects/plan.md`:
- Line 457: Update the defaultScreenshotQuality declaration in
GrpcDriverServer.swift to explicitly mark the static constant nonisolated, or
verify and document that the project’s Swift 6 toolchain accepts the current
isolation. Prefer the private nonisolated static let declaration so the
nonisolated getScreenshot and getRawScreenshot methods can access it safely
before marking R5 complete.
In `@packages/device-node/src/device/IOSRecordingProvider.ts`:
- Around line 199-235: Update _compressVideo to clean up compressedFilePath when
fsp.rename fails, while preserving the original file and propagating the rename
error. Scope cleanup to the rename failure path and avoid removing the
compressed file after a successful rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 525f4c3e-7e84-4f4b-adaa-4c65ae017235
📒 Files selected for processing (48)
.github/workflows/drivers.ymldocs/memory/ci/index.mddocs/memory/ci/pr-quality-gate.mddocs/memory/cli/host-preflight.mddocs/memory/cli/index.mddocs/memory/cloud-core/index.mddocs/memory/cloud-core/submit-pipeline.mddocs/memory/device-node/android-ios-mirror.mddocs/memory/device-node/index.mddocs/memory/device-node/log-capture.mddocs/memory/drivers/grpc-contract.mddocs/memory/drivers/index.mddocs/memory/drivers/log.mddocs/memory/index.mddrivers/android/app/src/androidTest/java/app/finalrun/android/TestUtils.ktdrivers/android/app/src/androidTest/java/app/finalrun/android/data/hierarchy/AccessibilityStreamer.ktdrivers/ios/finalrun-ios-test/GrpcDriverServer.swiftdrivers/ios/finalrun-ios-test/Managers/XCViewHierarchyManager.swiftfab/changes/260730-zga4-drivers-ci-gate-audit-defects/.history.jsonlfab/changes/260730-zga4-drivers-ci-gate-audit-defects/.status.yamlfab/changes/260730-zga4-drivers-ci-gate-audit-defects/intake.mdfab/changes/260730-zga4-drivers-ci-gate-audit-defects/plan.mdfab/changes/archive/2026/07/260730-ggeg-drivers-ci-gate-audit-defects/.history.jsonlfab/changes/archive/2026/07/260730-ggeg-drivers-ci-gate-audit-defects/.status.yamlfab/changes/archive/index.mdfab/project/config.yamlpackages/cli/src/hostPreflight.tspackages/cli/src/test/hostPreflight.test.tspackages/cloud-core/src/submit.tspackages/cloud-core/src/test/submit.test.tspackages/cloud-core/src/timeoutEnv.tspackages/cloud-core/src/upload.tspackages/common/src/interfaces/DeviceAgent.tspackages/device-node/src/device/AndroidLogcatProvider.tspackages/device-node/src/device/Device.tspackages/device-node/src/device/IOSLogProvider.tspackages/device-node/src/device/IOSRecordingProvider.tspackages/device-node/src/device/android/AndroidDevice.tspackages/device-node/src/device/ios/IOSSimulator.tspackages/device-node/src/device/ios/test/IOSSimulator.test.tspackages/device-node/src/device/logWriteStream.tspackages/device-node/src/device/shared/CommonDriverActions.tspackages/device-node/src/device/shared/DeviceRuntime.tspackages/device-node/src/device/test/Device.test.tspackages/device-node/src/device/test/IOSRecordingProvider.test.tspackages/device-node/src/device/test/logCaptureProviders.test.tspackages/device-node/src/grpc/GrpcDriverClient.tsproto/finalrun/driver.proto
Clamp the gRPC StartStreaming fps into 1...60 before it is used as a divisor in GrpcDriverServer.startStreaming. An unclamped request.fps of 0 made `1_000_000_000 / fps` an integer division by zero and a negative value made `UInt64(...)` a negative-to-unsigned conversion — both trap in Swift and take the XCUITest runner, and the whole driver, down with it. R6 clamped the legacy timer path in XCViewHierarchyManager; this is the same guard on the live gRPC path. Correct _unaryCall's retry doc in GrpcDriverClient: it claimed every maxRetries:2 opt-in is read-only, but launchApp opts in and a launch is mutating. The retry is pre-existing and load-bearing for iOS driver recovery, so it is documented as the one exception rather than silently narrowed (a behaviour change, out of scope). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WZELnpYMz46gN9meS2j4GC
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WZELnpYMz46gN9meS2j4GC
droid-ash
marked this pull request as ready for review
July 30, 2026 16:42
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Meta
zga4generated by fab-kit v2.16.8
Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr
Summary
A repo-wide comment-quality audit of all 234 code files surfaced ~20 confirmed runtime defects and the fact that
drivers/(Kotlin + Swift) has no CI verification of any kind — no workflow builds either native driver, andtest_pathscouldn't even match a native test file. This PR adds.github/workflows/drivers.yml(Android + iOS compile gate) and fixes 15 of those defects, split across the native drivers, the gRPC contract, device-node log/recording/recovery handling, cli host preflight, and cloud-core's timeout parser.Changes
build-for-testing; compile-only, additive toci.yml)fab/project/config.yaml—test_pathsnow matches native Kotlin/Swift test filestapPercent— was dividing a 0–1 fraction by 100 (wrong for every input)tapPercent—0.0(a valid left/top-edge coordinate) was rejectedqualitydefault corrected from 10 to 5, matching Android/proto/TS client0; now floating point.end()ed and flushed instead of just unpipedrmthat could strand a failed rename_disconnectionCallbackis now actually invoked on the disconnection pathcheckCommandOnPathnow useswhereon Windows instead of alwayswhichupload.ts's timeout parser now sharessubmit.ts'sNumber.isIntegercheck (wasNumber.isFinite, silently accepting1.5)success = truefor five no-op actionskillDriverrenamed tocloseDriverChannel/ documented (no behaviour change)driver.proto—reserved 4;replaces a prose comment about the removed fieldAccessibilityStreamer— deleted a false "5-second bounds stability" guarantee (no behaviour change)AccessibilityStreamer— documented the three divergent hierarchy traversals (no behaviour change)Verification & risk — please read before reviewing
1. The five native fixes (B1–B4, B14 / plan R4–R8, Kotlin + Swift) are compile-verified only. There is no Kotlin/Swift unit-test harness in this repo —
drivers/android/app/src/androidTest/is the driver (a single@Teststarts the gRPC server and blocks forever, zero assertions), and iOS has no test target at all. This change does not create one..github/workflows/drivers.ymlhas never run before this PR — it was built in an environment with no JDK, no Android SDK, and no Xcode. This PR is the first real execution of that workflow, so its ownandroidandiosjobs are the first verification of both the new gate itself and the native fixes it gates. If either job fails, treat that as the gate doing its job, not as a surprise.2. Most consequential fix, user-visible: Android
tapPercentcomputed(xPercent * screenWidth) / 100, but the TypeScript client always sends 0–1 fractions, never 0–100. A centre tap (0.5, 0.5) on a 1080×1920 screen landed at(5, 9)— the top-left corner — for every input. It's now(540, 960), matching iOS, which was already correct.3. One behaviour change has a live blast radius. B11 makes iOS stop returning
success = truefor five actions it never performed (copyText,pasteText,hideKeyboard,getAppList,setLocation). Of those five, onlyhideKeyboardis reachable from TypeScript via the driver today (IOSSimulator.ts→CommonDriverActions.hideKeyboard()→ the gRPCHideKeyboardRPC). A planner step emittingPLANNER_ACTION_HIDE_KEYBOARDon iOS now returnssuccess:falsewhere it previously returnedsuccess:true— intended (this is the defect B11 fixes), but worth knowing before you see it in a run report.Also worth knowing:
packages/device-node/src/device/logWriteStream.ts'sopen()has no persistenterrorlistener on the write stream — pre-existing in kind, judged outside this change's B5 scope.drivers.ymlcarries notimeout-minuteson either job. This matters more forios, which runs onmacos-latestat GitHub's 10× per-minute billing multiplier.Test/lint status: 539 tests passing (up from a 517 baseline before this change), lint unchanged at 42 warnings / 0 errors. Review passed after one rework cycle; 36/36 acceptance items verified for TypeScript-side behaviour and source-verified (not test-verified) for the native fixes.
Summary by CodeRabbit