Conversation
There was a problem hiding this comment.
All reported issues were addressed across 46 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Findings on 768c193. The 5s The iosSimulator coverage row in declarations.ts:1109 is a runtime-facts contract test, so nothing here exercises the actual runner route: daemon → runAppleRunnerCommand → pressLockButton → notify lockstate. The PR body says no live simulator run was done, so the one supported leaf is unproven — we don't know whether Not blocking, take or leave: Given the PR stays under the 700-line net production threshold (381 lines) and mostly adds rows to existing owners (SYSTEM_BUTTONS, the facts table, runner traits, registry descriptor), is the vacuous surface check in ScreenLock.swift:111 the only thing worth cutting, or is there another owner here that could absorb more? I did not run the Swift or TS tests in this review, and the Swift transition tests inject One CI check was reported and it's green, but it doesn't run the iOS runner route this PR changes, so it doesn't prove screen-lock works on a simulator. Two things need to happen before this is ready to merge: move the verification deadline so at least one read happens after dispatch (with a regression test for the shouldContinue-false-then-locked case), and provide the live simulator screen-lock run described above. |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Follow-up after 99e9c47 (verification window reset after dispatch) and bd45e6c (Lock Screen-specific AX surface):
The current implementation now checks both the SpringBoard lock-state notification and the Lock Screen-specific |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Follow-up on 2d1b117 and 272f8dc: the Lock Screen verifier now accepts either the date view or the Lock Screen-specific |
|
This PR is ready at f740401. Both blockers from the earlier review (#3001 (comment)) are fixed: the verification deadline now starts after dispatch returns, and a final lock-state read was added with a regression test. The one reported check is green, but the upstream Actions runs are still held at action_required, and no hosted job runs the iOS runner XCTest route that ScreenLock.swift and the Models.swift traits change, so CI says nothing about that route. No conflicts. The next step is for a maintainer to approve the held Actions runs. Not blocking, take or leave: the screen-lock text in website/docs/docs/commands.md, the help description and cliDetail in src/commands/system/index.ts, and the ScreenLockCommandResult doc in packages/contracts/src/navigation.ts still say success needs the Lock Screen date surface, while verifyLockScreenSurface now also accepts SBCoverSheetWindow, so one shared wording would keep them in step; the six fallow-ignore suppressions removed in src/client/client-types.ts (https://github.com/callstack/agent-device/blob/f740401/src/client/client-types.ts#L36) leave the comment above them stale and are unrelated to screen lock; and RunnerTests+ScreenLockTests.swift repeats the production date-view/cover-sheet predicate inline, and verifyLockScreenSurface builds its own SpringBoard XCUIApplication instead of using the existing On evidence: I did not run the Swift or TS tests, and the Swift passes on iOS 26.5 and 27.0 come from the author. I did not reproduce the live CLI run, so its details come from the author's comment. That run was on bd45e6c, before 2d1b117 widened the surface check, and only the focused XCTest was run after it. The live refusal was on Android, so the Apple physical device and macOS refusal path rests on the runtime.test.ts fact classification alone. I did not check whether SBCoverSheetWindow appears in the AX tree while the screen is unlocked, but the lockstate check still guards against a false success. testScreenLockStartsVerificationWindowAfterDispatchReturns only checks that the callback ran, so only code reading and the live timing pin where the deadline sits relative to dispatch. |
|
The code verdict for f740401 is unchanged: it is still clean. The PR now conflicts with main in |
Summary
screen-lockcommand through the typed Node client, CLI, MCP, daemon registry, runtime facts, and structured result schemaUNSUPPORTED_OPERATIONrefusals on other platform leavesImplementation
The Apple runner uses XCTest's simulator-only
pressLockButtonselector, the same audited route used by Appium WebDriverAgent, and independently verifiescom.apple.springboard.lockstatebefore checking the visible SpringBoard surface. This avoids Simulator.app menu/keyboard automation and does not conflate screen state with process mutexes, device claims, or runner leases.Reference: https://github.com/appium/WebDriverAgent/blob/master/WebDriverAgentLib/Categories/XCUIDevice%2BFBHelpers.m
Verification
pnpm typecheckpnpm check:packaged-runner-swift(58 packaged Swift files parsed)pnpm check:xctest-selection(all declared tests reachable by a configured lane)pnpm check:affected --run: formatting, lint, typecheck, layering, fallow gate, build, integration coverage, and 5,019 related tests reached green after classification fixes; the full rerun still hit the unrelated timing-sensitiveweb shutdown cleanup reaps the exact daemonassertion under load. The exact test passed standalone. GitHub's Apple/XCTest and device lanes remain authoritative.No npm package was published and no live device/simulator green is claimed locally.