refactor(simulator): extract errors and device catalog (#708 1/4) by shaun0927 · Pull Request #738 · shaun0927/opensafari

shaun0927 · 2026-05-07T15:15:04Z

Implements #708 steps 1+2 (errors + device catalog). Behavior-preserving. Follow-ups: lifecycle, app-manager, ui-controller+facade.

Does not Close — multi-PR refactor.

What changed

src/simulator/errors.ts (new, 70 lines): all 7 simulator error classes extracted verbatim — BootTimeoutError, ShutdownTimeoutError, DeviceNotFoundError, DeviceNotBootedError, ScreenshotTimeoutError, AppNotInstalledError, AppLaunchError. Same names, same constructors, same instanceof semantics.
src/simulator/device-catalog.ts (new, 87 lines): pure discovery/catalog functions (listDevices, listRuntimes, getDevice, resolveDevice) extracted from SimulatorManager. Uses dependency injection for SimctlExecutor.
src/simulator/manager.ts (reduced from 560 → 357 lines, −203): imports catalog functions and delegates; re-exports all error classes as a compatibility shim so existing callers are unaffected.

Tests

tests/unit/simulator-errors.test.ts (new): 14 cases — instanceof, .name, .message content, and public fields for every error class; also verifies manager re-exports are identical classes.
tests/unit/device-catalog.test.ts (new): 9 cases — listDevices happy path, unavailable filtering, unknown runtime, multi-runtime; getDevice by UDID + unknown UDID; resolveDevice by UDID/preset/substring/fuzzy + DeviceNotFoundError shape.
All 4 existing simulator test suites pass unchanged.

Validation

npx tsc --noEmit: clean
npx jest (new tests): 20/20 passed
npx jest (existing simulator suites): 33/33 passed
npm run build: success
npm run lint -- --quiet: clean
git diff --check: clean

…1/4) Behavior-preserving extraction: - src/simulator/errors.ts: all simulator error classes (BootTimeoutError, ShutdownTimeoutError, DeviceNotFoundError, DeviceNotBootedError, ScreenshotTimeoutError, AppNotInstalledError, AppLaunchError) - src/simulator/device-catalog.ts: pure discovery functions (listDevices, listRuntimes, getDevice, resolveDevice) with SimctlExecutor DI - src/simulator/manager.ts: delegates to catalog module; re-exports error classes as compatibility shim for existing callers No behavior changes. instanceof semantics preserved via re-export. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

qodo-code-review · 2026-05-07T15:15:08Z

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

gemini-code-assist

Code Review

This pull request refactors the simulator management logic by extracting device cataloging and error definitions into dedicated modules, device-catalog.ts and errors.ts, and adding unit tests. The SimulatorManager now delegates device-related tasks to the new catalog and re-exports error classes for backward compatibility. Feedback focuses on improving the robustness of the simctl JSON parsing by handling optional keys and providing fallbacks for missing data. Additionally, a suggestion was made to harden the fuzzy matching logic to prevent incorrect matches when the input consists only of whitespace or separators.

gemini-code-assist · 2026-05-07T15:17:37Z

+  devices: Record<string, Array<{
+    udid: string;
+    name: string;
+    state: string;
+    isAvailable: boolean;
+  }>>;
+  runtimes: Array<{
+    identifier: string;
+    version: string;
+    isAvailable: boolean;
+    platform: string;
+  }>;


The SimctlListResult interface defines devices and runtimes as required fields. However, simctl list devices -j only returns the devices key, and simctl list runtimes -j only returns the runtimes key. Marking them as optional is more accurate and prevents potential runtime errors when these keys are missing from the JSON response.

Suggested change

devices: Record<string, Array<{

udid: string;

name: string;

state: string;

isAvailable: boolean;

}>>;

runtimes: Array<{

identifier: string;

version: string;

isAvailable: boolean;

platform: string;

}>;

devices?: Record<string, Array<{

udid: string;

name: string;

state: string;

isAvailable: boolean;

}>>;

runtimes?: Array<{

identifier: string;

version: string;

isAvailable: boolean;

platform: string;

}>;

gemini-code-assist · 2026-05-07T15:17:38Z

+  const result = await simctl.execJson<SimctlListResult>(['list', 'devices']);
+  const devices: SimulatorDevice[] = [];
+
+  for (const [runtimeId, deviceList] of Object.entries(result.devices)) {


Since simctl list devices -j does not return a runtimes key, and the devices key itself might be missing in some error states or different simctl versions, it's safer to use a fallback when accessing result.devices to avoid a crash during iteration.

Suggested change

for (const [runtimeId, deviceList] of Object.entries(result.devices)) {

for (const [runtimeId, deviceList] of Object.entries(result.devices ?? {})) {

gemini-code-assist · 2026-05-07T15:17:38Z

+
+export async function listRuntimes(simctl: SimctlExecutor): Promise<SimulatorRuntime[]> {
+  const result = await simctl.execJson<SimctlListResult>(['list', 'runtimes']);
+  return result.runtimes.filter(r => r.isAvailable);


Similar to listDevices, result.runtimes will be undefined if the JSON response from simctl only contains other keys (like devices). Adding a fallback prevents a runtime error when calling .filter().

Suggested change

return result.runtimes.filter(r => r.isAvailable);

return (result.runtimes ?? []).filter(r => r.isAvailable);

gemini-code-assist · 2026-05-07T15:17:38Z

+  const keywords = lower.split(/[\s-]+/);
+  const fuzzy = devices.find(d => {


The fuzzy matching logic can match all devices if the presetKey results in empty keywords (e.g., if it contains only whitespace or multiple separators). Since string.includes("") is always true, the current implementation would return the first available device for such inputs. Filtering out empty keywords and ensuring the keyword list is not empty improves the reliability of the resolver.

Suggested change

const keywords = lower.split(/[\s-]+/);

const fuzzy = devices.find(d => {

const keywords = lower.split(/[\s-]+/).filter(Boolean);

if (keywords.length === 0) throw new DeviceNotFoundError(presetKey, devices.map(d => d.name));

const fuzzy = devices.find(d => {

Address gemini-code-assist review on PR #738: - Treat `SimctlListResult.devices` and `runtimes` as optional. `simctl list devices -j` and `simctl list runtimes -j` only return the relevant key, so marking both required misrepresents the wire format and made `Object.entries` / `.filter` brittle to a missing key. - Add `?? {}` / `?? []` fallbacks at the call sites for the same reason. - In `resolveDevice`, filter empty fuzzy keywords. With whitespace- or separator-only input the previous split produced an array of `""` tokens, and `every(kw => name.includes(""))` returned `true` for every device — silently resolving to the first available simulator. Skipping the fuzzy step when no real keywords remain makes the resolver fall through to the explicit `DeviceNotFoundError`. Tests: existing 20 device-catalog/error suites still pass.

Moves boot, shutdown, erase, delete, clone from SimulatorManager into src/simulator/lifecycle.ts. SimulatorManager delegates to the new module; all public method shapes are unchanged. Behavior-preserving: same timeouts, same retry policy, same shutdown escalation (poll → retry → nuclear erase). Timeout errors surface explicitly — ShutdownTimeoutError is thrown after erase, not swallowed. Erase/delete require explicit caller intent (no implicit cleanup). Adds tests/unit/simulator-lifecycle.test.ts: boot happy path, boot timeout, shutdown happy path, shutdown timeout surfaces, delete explicitness, clone UDID extraction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…4/4) Extract screenshot, setAppearance/getAppearance/toggleAppearance, rotate, overrideStatusBar, and openUrl from SimulatorManager into src/simulator/ui-controller.ts with dependency-injected SimctlExecutor. SimulatorManager is now a thin facade: constructor wiring + one-liner delegations only — zero business logic remaining in manager.ts. Behavior strictly preserved: same simctl commands, same error semantics, same screenshot temp-file cleanup. Transient-screenshot retry (PR #658) lives in src/tools/app-screenshot-native.ts (MCP tool layer, unchanged). Tests: 97 existing tests pass; 30 new tests added in tests/unit/simulator-ui-controller.test.ts covering all extracted functions. manager.ts reduction across 4-PR series: PR1 (#738, errors+catalog): 560 → 452 lines (-108) PR2 (#740, lifecycle): 452 → 404 lines (-48) PR3 (#742, app-manager): 404 → 296 lines (-108) PR4 (this, ui-controller): 296 → 211 lines (-85) Total: 560 → 211 lines (-349 lines, -62%) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Address codex review on PR #740 (de3d645): The previous iteration of this PR re-threw `ShutdownTimeoutError` after the nuclear `erase` step. That broke the pre-refactor `SimulatorManager.shutdown` contract: it used to return on a successful erase, so MCP callers like `device_shutdown` (`src/tools/device-shutdown.ts`) treated a stalled-but- cleaned-up teardown as success. Re-throwing turned that into a user-visible hard failure even though the simulator was already wiped. Match pre-refactor behavior exactly: - After `simctl erase` succeeds, return. - If the erase itself fails, propagate the underlying `SimctlError` so callers can still distinguish "cleaned up" from "couldn't even erase". Tests: - Replace the `ShutdownTimeoutError` assertions with the best-effort-cleanup contract: `shutdown(...)` resolves after the nuclear erase, and propagates `SimctlError` when erase itself fails. The "issues simctl erase" test is reworded to no longer imply a throw. - Drop the now-redundant `ShutdownTimeoutError` import from the lifecycle module (the error class itself stays exported from `errors.ts` for callers that want to surface a timeout in a higher-level wrapper).

Behavior-preserving extraction of installApp/launchApp/terminateApp/ uninstallApp/activateApp/listRunningApps/resetApp from SimulatorManager into src/simulator/app-manager.ts. Same simctl commands, same error semantics (AppNotInstalledError, AppLaunchError), same retry/timeout policy. SimulatorManager facade delegates to the new module — all public method signatures unchanged. Adds tests/unit/simulator-app-manager.test.ts with 28 cases covering install/launch/terminate happy paths, DeviceNotBootedError, AppNotInstalledError, AppLaunchError, and resetApp step tracking. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…etection (#708 3/4) Replace greedy /\[.*\]$/ with non-greedy /\[[^\]]*\]$/ in listRunningApps so a label containing nested brackets is stripped correctly. Extract isAppNotInstalledError() helper that checks 'domain not found' and 'not installed' patterns; replace the four duplicated inline checks in launchApp, terminateApp, activateApp, and resetApp with a single call. Add unit tests covering both detection strings and the non-greedy regex. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…#708 3/4) Codex P2 follow-up: the previous non-greedy regex `/\[[^\]]*\]$/` only stripped the last bracketed suffix, so labels like `UIKitApplication:com.example.app[0x1][debug]` became `com.example.app[0x1]`, causing classifyMobileContext bundle-id comparisons to mismatch. Switch to `/(\[[^\]]*\])+$/` so one or more trailing bracket groups are removed without overshooting bracketless prefixes. Test added for the multi-suffix case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…4/4) Extract screenshot, setAppearance/getAppearance/toggleAppearance, rotate, overrideStatusBar, and openUrl from SimulatorManager into src/simulator/ui-controller.ts with dependency-injected SimctlExecutor. SimulatorManager is now a thin facade: constructor wiring + one-liner delegations only — zero business logic remaining in manager.ts. Behavior strictly preserved: same simctl commands, same error semantics, same screenshot temp-file cleanup. Transient-screenshot retry (PR #658) lives in src/tools/app-screenshot-native.ts (MCP tool layer, unchanged). Tests: 97 existing tests pass; 30 new tests added in tests/unit/simulator-ui-controller.test.ts covering all extracted functions. manager.ts reduction across 4-PR series: PR1 (#738, errors+catalog): 560 → 452 lines (-108) PR2 (#740, lifecycle): 452 → 404 lines (-48) PR3 (#742, app-manager): 404 → 296 lines (-108) PR4 (this, ui-controller): 296 → 211 lines (-85) Total: 560 → 211 lines (-349 lines, -62%) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…simctl.exec (#708 4/4) - Hoist `promisify(execFile)` to module top-level as `execFileAsync` instead of recreating it on every rotate() call. - Replace direct `xcrun simctl` execFile invocation in the simctl path with `deps.simctl.exec(['io', deviceId, 'setorientation', orientation], { timeout: 10000 })` for consistency with the rest of the module (xcrun prefixing + structured errors). - AppleScript fallback path retains `execFileAsync` (module-level) for osascript; simctl-first / AppleScript-fallback ordering and error semantics are fully preserved. - Update rotation-fallback.test.ts mocks to provide `{ stdout, stderr }` result object required by promisify(execFile) so SimctlExecutor.exec() resolves correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

shaun0927 · 2026-05-07T17:01:02Z

Pushed fix commit addressing previous review feedback. @codex review @gemini-code-assist review

gemini-code-assist · 2026-05-07T17:01:05Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

chatgpt-codex-connector · 2026-05-07T17:04:07Z

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…4/4) (#743) refactor(simulator): extract UI controller and finalize facade (#708 4/4)

refactor(simulator): extract lifecycle module (#708 2/4)

refactor(simulator): extract app manager module (#708 3/4)

…rmes/update-738

Resolve conflicts and integrate develop changes with the simulator refactor: - Preserve facade pattern in src/simulator/manager.ts; new state-cache and getDeviceState logic delegates to extracted modules. - Wire invalidateCache + getDeviceState through src/simulator/lifecycle.ts so polling loops observe transient ShuttingDown via SimulatorStateCache. - Extend logAuditEntry with optional status parameter to satisfy mcp-server call sites added on develop. - Take develop's mcp-server.ts signatures (returns MCP envelope shape). Conflicts resolved per plan in src/simulator/manager.ts.

Fixes 24 of the test failures introduced by the develop merge: - auth-tools-verification, issue-167-wire-verification: add httpHighRiskTools flag so tier-3 / high-risk auth tools register on the test HTTP transport. - mcp-server: add httpInsecure to the freshServer.start() call so the request reaches the JSON-RPC handler instead of returning 401. - http-high-risk-tools: spy fs.appendFileSync (matches the audit logger's current write path) instead of the deprecated fs.appendFile signature. - audit-logger: drop 'text' and 'value' from SENSITIVE_KEYS — the spec is to redact only credential-named keys; over-redacting blanks out legitimate type.text and select_option.value arguments. Same change applied in sibling PR #731. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86a1920d58

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-08T13:30:31Z

  'session',
-  // Free-form user input that flows through MCP tool calls (e.g.
-  // `type.text`, `select_option.value`) can carry passwords or OTPs, so
-  // redact these keys defensively even though they are not credentials
-  // by name.
-  'text',
-  'value',
 ];


Restore redaction for free-form text/value audit fields

Keep text and value in SENSITIVE_KEYS; removing them causes raw user-entered form content to be written to args_summary in audit logs. MCP tool payloads commonly carry credentials/OTPs under keys like type.text and select_option.value, so this change reintroduces a concrete secret-leak path even when other credential-named fields are redacted.

Useful? React with 👍 / 👎.

The audit logger now records expression length only (len=N) instead of the raw expression text. Update the assertion to match — same change already applied in sibling PR #731 (input refactor) and #732 (webkit refactor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Restore defensive redaction for free-form MCP text/value arguments and add regression coverage for the device-catalog hardening that earlier bot reviews requested. Constraint: PR #738 is a #708 simulator refactor but includes develop audit-hardening reconciliation. Rejected: leaving text/value visible in audit summaries | Codex flagged a concrete secret-leak path for type.text and select_option.value. Confidence: high Scope-risk: narrow Directive: Keep audit policy changes separate from simulator refactors unless they are conflict-resolution fixes with explicit regression tests. Tested: npx jest affected 12 suites --runInBand; npx tsc --noEmit; npm run lint -- --quiet; npm run build; git diff --check Not-tested: live macOS Simulator smoke.

shaun0927 · 2026-05-12T06:34:30Z

I reviewed this PR for intent, scope, and merge readiness.

Summary:

The PR is valid for Decompose SimulatorManager responsibilities into lifecycle, app, UI, and device catalog modules #708: it decomposes SimulatorManager into focused simulator modules while keeping the manager as a compatibility facade.
Backward compatibility is preserved through the manager re-exports for simulator errors / RotationResult, and the public facade continues delegating the existing lifecycle, app, UI, and catalog behavior.
Earlier bot feedback was addressed: the device-catalog parsing now tolerates missing devices / runtimes keys and avoids separator-only fuzzy matches.
I also confirmed Codex's P1 audit concern was real and fixed it in f66aa3fe: free-form MCP text and value arguments are redacted again, with regression coverage proving raw values do not reach audit logs.

Validation evidence:

Local: npx jest affected suites — 12 suites / 213 tests passed.
Local: npx tsc --noEmit passed.
Local: npm run lint -- --quiet passed.
Local: npm run build passed.
Local: git diff --check passed.
Final review gates: code review returned APPROVE with 0 issues; architecture review returned CLEAR.
GitHub CI at the final head passed: dependency-audit, lint, test, and build.

Residual risk:

No merge-blocking issue remains. The only remaining confidence gap is an optional live macOS Simulator smoke test, which is operational rather than a code/design blocker and is not required to merge this covered refactor.

Given the above, this PR has been improved to a merge-ready state and is safe to merge.

gemini-code-assist Bot reviewed May 7, 2026

View reviewed changes

This was referenced May 7, 2026

refactor(simulator): extract lifecycle module (#708 2/4) #740

Merged

refactor(simulator): extract app manager module (#708 3/4) #742

Merged

refactor(simulator): extract UI controller and finalize facade (#708 4/4) #743

Merged

shaun0927 and others added 2 commits May 8, 2026 01:27

shaun0927 and others added 6 commits May 8, 2026 01:49

shaun0927 and others added 6 commits May 8, 2026 14:11

refactor(simulator): extract UI controller and finalize facade (#708 …

8d9fee7

…4/4) (#743) refactor(simulator): extract UI controller and finalize facade (#708 4/4)

refactor(simulator): extract lifecycle module (#708 2/4) (#740)

f8b495f

refactor(simulator): extract lifecycle module (#708 2/4)

refactor(simulator): extract app manager module (#708 3/4) (#742)

006a125

refactor(simulator): extract app manager module (#708 3/4)

Merge remote-tracking branch 'origin/refactor/708b-lifecycle' into he…

88c62cc

…rmes/update-738

chatgpt-codex-connector Bot reviewed May 8, 2026

View reviewed changes

shaun0927 and others added 2 commits May 8, 2026 22:34

shaun0927 merged commit 08bc02f into develop May 12, 2026
4 checks passed

This was referenced May 12, 2026

Decompose SimulatorManager responsibilities into lifecycle, app, UI, and device catalog modules #708

Open

Roadmap: OpenSafari automation safety, speed, and maintainability hardening #713

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(simulator): extract errors and device catalog (#708 1/4)#738

refactor(simulator): extract errors and device catalog (#708 1/4)#738
shaun0927 merged 17 commits into
developfrom
refactor/708a-errors-catalog

shaun0927 commented May 7, 2026

Uh oh!

qodo-code-review Bot commented May 7, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Uh oh!

gemini-code-assist Bot May 7, 2026

Uh oh!

gemini-code-assist Bot May 7, 2026

Uh oh!

gemini-code-assist Bot May 7, 2026

Uh oh!

shaun0927 commented May 7, 2026

Uh oh!

gemini-code-assist Bot commented May 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 8, 2026

Uh oh!

shaun0927 commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	for (const [runtimeId, deviceList] of Object.entries(result.devices)) {
	for (const [runtimeId, deviceList] of Object.entries(result.devices ?? {})) {

	return result.runtimes.filter(r => r.isAvailable);
	return (result.runtimes ?? []).filter(r => r.isAvailable);

		const keywords = lower.split(/[\s-]+/);
		const fuzzy = devices.find(d => {

Conversation

shaun0927 commented May 7, 2026

What changed

Tests

Validation

Uh oh!

qodo-code-review Bot commented May 7, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

shaun0927 commented May 7, 2026

Uh oh!

gemini-code-assist Bot commented May 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 8, 2026

Choose a reason for hiding this comment

Uh oh!

shaun0927 commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants