Skip to content

feat(apple): add verified simulator screen lock - #3001

Open
csark0812 wants to merge 11 commits into
callstack:mainfrom
csark0812:chris/agent/screen-lock
Open

csark0812 wants to merge 11 commits into
callstack:mainfrom
csark0812:chris/agent/screen-lock

Conversation

@csark0812

@csark0812 csark0812 commented Sep 27, 2026 •

Copy link
Copy Markdown

Summary

  • add the public screen-lock command through the typed Node client, CLI, MCP, daemon registry, runtime facts, and structured result schema
  • admit only iOS/iPadOS Simulators and return structured UNSUPPORTED_OPERATION refusals on other platform leaves
  • make the transition idempotent and report success only after SpringBoard lock state and a visible non-empty SpringBoard surface agree
  • add deterministic Swift transition tests for already-locked, delayed/booting, unavailable HID, timeout, and unverified-surface cases

Implementation

The Apple runner uses XCTest's simulator-only pressLockButton selector, the same audited route used by Appium WebDriverAgent, and independently verifies com.apple.springboard.lockstate before 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 typecheck
  • focused TypeScript suite: 139 tests passed
  • focused web/Linux runtime and coverage contracts passed
  • pnpm 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-sensitive web shutdown cleanup reaps the exact daemon assertion 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.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 46 files

Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

Comment thread website/docs/docs/commands.md Outdated
Comment thread apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift Outdated
Comment thread src/daemon/system-button-runtime.ts
Comment thread packages/platform-apple/src/runtime.test.ts
Comment thread packages/platform-apple/src/navigation/runtime.ts Outdated
Comment thread packages/contracts/src/system-button-runtime.ts
@thymikee

Copy link
Copy Markdown
Member

Findings on 768c193.

The 5s deadline in RunnerTests+ScreenLock.swift:14 is computed before dispatch(), and the loop checks shouldContinue() before each read but never does a final read after the loop ends. commands.md already notes the sibling action-button press takes about 5s inside XCUITest, so if XCUIDevice.perform(pressLockButton) takes close to that, the first shouldContinue() check after dispatch can already be false, and no read ever happens to catch the lock. A lock that actually succeeded would then be reported as COMMAND_FAILED with "SpringBoard did not report a locked screen", leaving the Lock Screen up while the agent thinks the command failed. The rule should be: at least one lock-state read must happen after dispatch returns, and every poll must fit inside the verification window — start the deadline after dispatch, or add a final readState after the loop, with a Swift test where shouldContinue is false immediately after dispatch and the read still reports locked and asserts ok.

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 notify_get_state on com.apple.springboard.lockstate from inside the XCTest runner process actually sees the simulator's SpringBoard lockstate, how long pressLockButton takes, or whether the Lock Screen visibly appears. Please run the CLI against a booted iOS Simulator with an app session open and screen unlocked (agent-device screen-lock --platform ios --device <udid> --json) and paste the ok response with state 'locked' and its wall time, a post-lock screenshot showing the Lock Screen, a second call returning ok with no new dispatch in runner.log to show the idempotent path, the runner.log timing for pressLockButton (this bears directly on the deadline issue above), and one refusal on a physical device or macOS target showing UNSUPPORTED_OPERATION with details.reason.

Not blocking, take or leave: verifyLockScreenSurface in RunnerTests+ScreenLock.swift:111 only checks that SpringBoard exists with a non-empty frame, which is true on the Home Screen too, so this check can't actually fail and its "Lock Screen surface was not visible" text and the commands.md claim of two agreeing conditions describe verification the code doesn't do — either drop the check (and its test and docs clause) and rely on lockstate alone, or make it Lock-Screen-specific; and appleScreenLockFact in runtime.ts:279 reimplements kernel isHandheldAppleSimulator (packages/kernel/src/device.ts:120) instead of calling it the way settings-leaf.ts and system/runtime.ts already do.

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 shouldContinue, so they can't show where the production deadline actually sits relative to dispatch. I also did not confirm that notify_get_state from the XCTest runner process reflects the simulator's SpringBoard state, did not measure pressLockButton latency, and did not check the WebDriverAgent reference — only a live run resolves these, which is why the deadline finding is rated likely rather than confirmed.

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@csark0812

Copy link
Copy Markdown
Author

Follow-up after 99e9c47 (verification window reset after dispatch) and bd45e6c (Lock Screen-specific AX surface):

  • Regression: the iOS XCTest build-for-testing succeeded; focused XCTest run passed 4/4, including testScreenLockStartsVerificationWindowAfterDispatchReturns, testScreenLockReportsSuccessOnlyForLockScreenSpecificSurface, final-read-after-deadline, and idempotent-already-locked.
  • Live transition: iPhone 18 Pro Simulator, iOS 27.0, UDID 7A277ED9-BE4E-4CB5-B6B8-534B1CB5DBAD, with com.callstack.agentdevicelab open and the app visibly foregrounded beforehand. screen-lock returned {success:true,data:{action:"screen-lock",state:"locked",message:"Screen locked"}} in 1.65s. A direct simctl screenshot immediately afterward showed the Lock Screen (captured at /private/tmp/pr3001-locked-screen.png).
  • Runner timing: runner.log recorded Pressing lock button at XCTest t=7.37s and command completion 1.552s after acceptance; the wall time was 1.65s. The verification deadline is now reset immediately after that dispatch returns.
  • Idempotence: a second call returned the same success in 0.16s. The runner log has two screenLock command accept/complete pairs but exactly one Pressing lock button entry, so the second call did not dispatch.
  • Unsupported leaf: against the booted Android API 35 emulator, the command returned UNSUPPORTED_OPERATION with details.reason: unsupported-platform-leaf.

The current implementation now checks both the SpringBoard lock-state notification and the Lock Screen-specific lockscreen-date-view AX element; the focused live XCTest passed. On ownership, I do not see a helpful larger extraction: the command contract, platform admission fact, and Apple runner effect are distinct owners already, while the lock transition policy is isolated in its focused runner helper. These changes preserve that boundary.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@csark0812

Copy link
Copy Markdown
Author

Follow-up on 2d1b117 and 272f8dc: the Lock Screen verifier now accepts either the date view or the Lock Screen-specific SBCoverSheetWindow, while still requiring the simulator lock state. The integration test restores/unlocks the Simulator in deferred cleanup. Focused XCTest passed on iOS 26.5, and pnpm check:affected --run now passes all runnable checks locally (including format, lint, typecheck, layering, Fallow, build, integration-node, affected Vitest, command-doc coverage, XCTest-selection, and packaged-runner Swift parsing). The workflow runs on upstream still show action_required with no jobs; the API requires repository-admin permission to approve them, so hosted checks remain unstarted from this account.

@thymikee

Copy link
Copy Markdown
Member

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 springboard lazy var.

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.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 29, 2026
@thymikee

Copy link
Copy Markdown
Member

The code verdict for f740401 is unchanged: it is still clean. The PR now conflicts with main in src/mcp/command-output-schemas.ts, after the recent MCP output-schema moves landed. I removed the ready-for-human label until the conflict is resolved. Please rebase on main and resolve it.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 29, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants