Repository navigation
fix(ios): serialize status bar normalization around simulator captures - #4136
Conversation
The screenshot status bar transaction read the simulator's current overrides as the state to restore to, applied its deterministic normalization, captured, and restored. Two captures that overlapped on one simulator broke that: the second read the first capture's normalization as its "previous" state, so its restore leg re-applied the normalized network and Wi-Fi overrides and left them on. The daemon serializes ordinary requests for a device, but the capture interface itself did not, so any caller outside that path hit it. `withNormalizedSimulatorStatusBar` now owns the whole normalize -> capture -> restore window and holds the simulator's keyed lock across it, so a capture can only ever snapshot the caller's own overrides, and a capture that throws still restores before the queued one starts.
|
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 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 5 new · 0 still open · 6 no longer raised 5 finding(s): 3 minor, 2 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (3)
🔵 Nit (2)
Reply under an inline comment with |
![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: 2 new findings on e6a411d. The verdict and the full list are in the summary comment.
Route a capture without --normalize-status-bar through the same per-simulator lock so it cannot take its screenshot of the temporary normalized bar another capture installed. Pass the request's abort signal to the lock and bound every simctl status_bar call so a wedged tool cannot hold a simulator's captures. Restore stays uncancellable: the normalization is shared device state. Release both capture gates in a finally, pin the propagated capture error, throw on an override value the fake cannot list, and rename the cleared-status bar case to what it asserts.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
An iOS simulator's runner capture renders the same screen a simctl capture does, but the interactor's runner backend called it directly, so it could copy an image of the temporary normalized overrides another capture had installed. It now takes the same per-simulator reservation without normalizing the bar, and re-enters the one the simctl fallback already holds. Take both capture reads at the top of the capture step, where the status bar owner still holds the state the PNG is taken with, and settle both captures through one helper so a rejection is never left unobserved.
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on b5fa69d. The verdict and the full list are in the summary comment.
One request's two capture legs both run inside the status bar window, and the runner leg takes that same window. Drive the real normalization around the real runner fallback with only the tools faked, and assert the order: snapshot, normalize, runner capture, restore. The runner leg re-entering that window is what keeps the nested reservation from opening a second one. Give the restore a longer deadline than setup: leaving the normalization on the simulator is the failure this owner exists to prevent, and an untimed wedged tool would hold the simulator forever. State in the doc which commands the request's abort signal does and does not reach.
There was a problem hiding this comment.
Apex Sentinel: 2 new findings on 6a8579c. The verdict and the full list are in the summary comment.
A runner capture nested in the status bar window its own request held was cancelling on that request's abort signal, so a normalized capture that fell back to the runner after an abort never took the image while the window still restored a bar it had normalized for nothing. The reservation now drops the signal once the caller already holds the simulator's window, and the restore comment states that its budget applies per command.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
06caa38 to
e84c775
Compare
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on e84c775. The verdict and the full list are in the summary comment.
A `settings wifi` or `settings airplane` write that landed while a normalized capture held the simulator's status bar was reverted by that capture's restore, which wrote back the bar it had snapshotted. Both writers now take the same per-simulator hold, so a setting lands after the restore instead of under it. A restore whose clear command fails now drops the known-cleared answer, because the simulator holds whatever the failed command left, and the next capture has to read the bar again. The runner paths read one named predicate each, and the kernel pins that heldKeyedLockKeys reports exactly the keys withKeyedLock would re-enter.
There was a problem hiding this comment.
All reported issues were addressed across 7 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: 5 new findings on 61a4a6a. The verdict and the full list are in the summary comment.
A restore whose clear command fails leaves the deterministic bar on the simulator. Reading the bar again would make the next capture adopt that normalization as the caller's state and hand it back on every later capture, so the answer that says the bar is cleared now survives the failure: the next capture skips the read and its own restore clears the leftover. The kernel test checks the pending hold before it is awaited, and the runner branch comment names the targets that take a status bar window.
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on b02d672. The verdict and the full list are in the summary comment.
`simctl status_bar` runs scoped to a device set, so one udid in two sets is two bars. The transaction and cache key now carry the resolved set path, so records of one simulator still share one window while simulators of different sets keep their own. The settings writes take the request's signal, so a write queued behind a capture window can still be cancelled, and the screenshot help states that an iOS simulator capture waits for the bar and reports cancellation.
There was a problem hiding this comment.
All reported issues were addressed across 5 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: 8 new findings on af11ed6. The verdict and the full list are in the summary comment.
The wifi and airplane writes now hold the per-simulator window every iOS capture queues behind, so a wedged `simctl status_bar` inside that hold would stop those captures. Each of the three commands keeps the same per-command budget the capture's own status bar commands use, asserted by a new case in the concurrency file. The settings cancellation case releases its capture gate before awaiting the write, so a regression reports its assertion instead of hanging, and the screenshot help says cancellation covers the wait for the bar.
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: 4 new findings on 97a1090. The verdict and the full list are in the summary comment.
Live device evidence at head
|
| Check | base f7879cf2d2 |
head 6d59c2b3e5 |
|---|---|---|
| Two overlapping normalized captures (3 runs) | leaked 3/3, bar left WiFi Mode: 3, WiFi Bars: 3, once DataNetworkType: 11 |
VERDICT: caller overrides restored every run |
Plain capture started mid-window (no --normalize-status-bar) |
captured the temporary bar, Time: 09:41 |
saw the caller's bar: WiFi Mode: 3, WiFi Bars: 2 |
settings wifi on started mid-window |
applied after 675 ms and was reverted — bar ended WiFi Mode: 2, WiFi Bars: 0 |
waited for the window (3.0–4.4 s) and survived — bar ended WiFi Mode: 3, capture's other overrides restored |
settings wifi on cancelled mid-window |
n/a (no hold) | rejected after 403 ms, VERDICT: cancelled write wrote nothing, caller bar intact |
| End state | — | simctl status_bar … clear, then list → Current Status Bar Overrides: empty |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Independent review: needs-changes.
Reviewed exactly merge-base f7879cf2d266ee1dd476d0342b2819bdeceb647b → head 6d59c2b3e5680fd7ea84ce09891b610c3fd1a923. Source remained read-only.
-
P2 — Canonicalize the simulator set before deriving its lock/cache key.
packages/platform-apple/src/core/screenshot-status-bar.ts:213-215includes the raw set-path spelling in the key.simulatorAddressForonly trims that path (packages/kernel/src/device-isolation.ts:7-11), and inventory preserves the spelling (packages/platform-apple/src/simulator-inventory.ts:79-91). With no operator-pinned set, two requests may therefore select the same UDID through/path/to/setand/path/to/set/.. Bothsimctl --setcommands reach the same directory and simulator, but the keys differ. Capture B can enter while A's deterministic overrides are installed, snapshot them as caller state, and restore them after A restores the real caller state: the original leak remains reachable. Omitting the set versus explicitly naming the default set is another instance. Resolve one canonical simulator-set identity, including the implicit default, at the address boundary and use it consistently for both the transaction and cleared-state cache. Add a same-directory/different-spelling overlap case alongside the existing genuinely-different-sets case. -
P2 — Invalidate the cleared-state cache before a settings override can mutate the bar.
packages/platform-apple/src/core/app-settings.ts:115-120(also airplane-on at134-155) invalidates only after the newly timed command succeeds. Concrete case: a prior normalized capture has recorded this simulator as known-cleared;settings wifi offapplies--wifiMode failedbutsimctlthen stalls and hits the new 10-second deadline. The lock releases, but the cache still says cleared. The next normalized capture skipsstatus_bar list, overwrites the setting, and restores withclear, permanently discarding the applied caller override. A timeout does not prove that the write had no effect. Invalidate inside the hold before attempting either override, so failed or timed-out writes force the next capture to read the actual state. Cover a tool double that applies the override and then throws the timeout; merely asserting thattimeoutMswas supplied does not cover this failure. -
P3 — Document the settings wait and timeout on the owning user surfaces.
packages/platform-apple/src/core/app-settings.ts:112-123/130-164changessettings wifiandsettings airplanefrom an immediate write to a queued operation with a 10-second execution budget. A caller issuing a setting during a long screenshot now waits for capture and restoration, or receives cancellation before any write. Only screenshot CLI detail was updated (packages/command-surface/src/capture/screenshot.ts:77);settings --help(packages/command-surface/src/capture/settings.ts:97-108) and the settings/screenshot sections ofwebsite/docs/docs/commands.mddo not explain this behavior. Add the wait, cancellation, and per-command timeout semantics to settings help and the user docs. Clarify that 10 seconds bounds execution after acquisition, not time spent queued. This is required by AGENTS.md's help-and-user-docs rule.
Requested adversarial/design checks:
- For identical address strings, the lock covers snapshot → normalization → capture/fallback → restoration;
finallyreleases it after capture failure. Distinct actual device sets with the same UDID have distinct keys. Physical-device and macOS runner routes bypass this lock. - The status-bar tool calls have 10-second setup/write and 30-second restore budgets. The simctl capture has its existing 20-second retry deadline; boot recovery and runner startup/transport also have their own budgets. Thus the ordinary hung-tool paths are bounded, but this is not one overall transaction or queue deadline. An uncancelled waiter waits through all preceding holders and does not receive a holder's failure; a queued caller with a signal rejects with its own abort reason. File/PNG processing is also inside the hold and has no overall transaction watchdog, so the per-command timeouts are not proof of a hard bound on every possible stall.
- The existing AsyncLocalStorage re-entrant lock avoids a runner fallback or nested settings write waiting on its own hold. A settings write deliberately made inside a normalized capture callback would still be overwritten by that callback's enclosing restore; I found no production capture leg that performs such a settings write, so I am not treating that hypothetical call graph as a regression.
- Including plain and runner captures is necessary to keep them from observing temporary normalization. Including wifi/airplane writes prevents restoration from undoing concurrent writes; bounding those commands is necessary once they can block screenshots. Keep these changes together; no new shared helper package or unrelated command-budget framework is needed. The diff adds 185 net production lines and 976 net test lines; the posted Size report is +982 B unpacked, below the mandatory size-review thresholds.
Thread 4435764303: follow-up, not an overlap-fix blocker. The failed-restore-clear path with real caller overrides is real: it can leave normalization behind and a later capture can adopt it. The base has the same clear → override restore sequence and swallowed restore error. Recovering the actual caller values requires retaining the original snapshot for recovery; pretending the bar was cleared only prevents adopting normalization while discarding those values. Track that separately with a caller-overrides regression case. The new explanatory comment at screenshot-status-bar.ts:177-181 should explicitly limit its recovery explanation to the already-known-cleared case.
Validation limits: static source/caller/test review and git diff --check only; no unit tests, device operations, or CI waiting. The new stateful concurrency tests exercise the real capture/status-bar boundary and visibly distinguish the base overlap behavior. The PR body reports identical-test base 8 failed / 2 passed versus head 10 passed, plus live base/head evidence; I did not independently reproduce those counts. Source approval is blocked by findings 1 and 2, independently of CI.
|
The change at 6d59c2b looks right to me, apart from the open invalidation-order thread in the first note below, which should be fixed before merge. CI is green (18 checks, none failing at this commit), and there are no conflicts. Not blocking, and fine to take or leave: (1) Evidence: the live run called the platform-apple owner directly, not through the daemon, so the request signal path was not run live. Optionally, run two overlapping On the open threads: the cubic thread on the invalidation order (#4136 (comment)) still stands and is the same as the first note. Four lower-priority threads from the other review still apply and are docs or naming nits. Two do not apply and can be resolved: the failed-clear recovery is the intended path (#4136 (comment)), and Nothing stops the merge. I suggest taking the invalidation reorder first, then replying on the open threads. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…aliases Resolve the simulator device set to one canonical path where the address is minted, and use that value for both the status bar lock key and the `--set` argument, so aliased or non-normalized spellings of one set cannot open two windows over one status bar. Naming the default device root now answers as the default set. Platform code reaches the canonical path through the host-kit file port, which is where host authority belongs. Drop the cached known-cleared answer before a `wifi` or `airplane` status bar command instead of after it: a write that hits its deadline may still have reached `simctl`, so no cached answer survives the attempt and the next capture reads the bar. Document that `settings wifi` and `settings airplane` wait for the simulator's screenshot window and bound their own write to 10 seconds, in `settings --help` and the command reference.
b999afd to
6f7b9ea
Compare
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on 6f7b9ea. The verdict and the full list are in the summary comment.
|
All three findings from #4136 (review) are addressed at head P2 — set-path aliases produced different lock keys for one simulator ( P2 — a timed-out settings write left a stale known-cleared answer ( P3 — the settings wait and timeout were undocumented. One line added to the owning Gate. Thread |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Round-2 review of 6f7b9ead18328ac4dd3c5a2ed0bba8d5a9b878eb.
The round-1 alias finding is resolved: canonical identity feeds both the simulator lock/cache address and --set. The wifi/airplane cache is now invalidated before each write, so timeout ambiguity cannot preserve a stale cleared answer. The settings help and commands page describe the same wait and 10-second write budget. The host-kit port is appropriately narrow for keeping filesystem ownership outside platform-apple, but its use raises the following issues.
Blocking findings:
- P1 — Explicit simulator set can lose its scope. In
packages/platform-apple/src/core/simctl.ts:31-44, ifrealpathSyncfails for either candidate,canonicalExistingPathreturns a merely normalized absolute path; the code still compares it to the process-home default and may returnundefined. ThenscopeSimctlArgsomits--set. An explicit caller-selected set (including a symlinked path) can silently become simctl's ambient default. Keep explicit paths scoped unless successful canonicalization proves they are the default, and avoid process-home authority where the request host differs. This also means intended symlink aliases do not canonicalize when their target does not yet exist; failure should preserve explicit scoping rather than erase it. - P1 — Nested settings writes can still be undone.
withSimulatorStatusBarHeldbypasses the lock when the async context already holds its key (screenshot-status-bar.ts:136-145). If wifi/airplane is invoked from a normalized capture's context, the setting write runs immediately and the outer capture's finally restores its older snapshot over it. The new serialization only protects callers outside that context; add a write-safe re-entry path or otherwise prevent restoring a snapshot over a later setting write. - P2 — A failed restore override leaves a false known-cleared cache. If the restore clear succeeds but re-applying the caller's overrides fails,
restoreSimulatorStatusBarOverridesexits before invalidating the cache (screenshot-status-bar.ts:184-194). If that key was previously known-cleared, the next capture skips reading and treats the leftover deterministic bar as cleared, losing the caller's overrides. Distinguish failed clear from failed re-apply and do not retain a cleared answer for the latter. - P2 — Cancellation after entering the status-bar transaction is swallowed. Setup errors are diagnosed and swallowed, and restore errors are also diagnosed and swallowed (
screenshot-status-bar.ts:162-170, 110-113). Once a normalized capture has entered, abort-triggered simctl failures can therefore return a screenshot success despite cancellation. Propagate cancellation from the owned signal while retaining best-effort restore.
The host-kit canonicalHostPathSync port itself is narrow and correctly avoids direct node:fs in platform-apple. However, canonicalization currently performs synchronous filesystem resolution twice per simctl argument build, including the unchanged default root on hot capture paths. This is a material event-loop cost, not an incorrect ownership boundary; cache the default identity and avoid unnecessary resolution. Missing/nonexistent paths cannot be resolved with realpath and should remain explicit.
Bot-thread verdicts (15 open threads; no replies posted):
screenshot-status-bar.ts:173— real defect, blocking: failed restore override can leave stale known-cleared state and lose caller overrides (P2 above).capture/screenshot.ts:74— not a defect: commands.md now documents the wait and cancellation behavior.app-settings.ts:107— not a defect: settings cliDetail and commands.md now document the settings wait.screenshot.ts:252— not a defect: naming nit only; predicate accurately selects simulator status-bar captures.app-settings.ts:130— real defect, blocking: re-entrant airplane/wifi write can be overwritten by the outer capture restore (P1 above).screenshot-status-bar.ts:61— not a defect: bounded command windows can delay queued work, but the capture includes its required operation; the suggested 90s framing sums separate per-step caps and does not establish an unbounded command. The queue wait is intentionally outside the 10s write budget.screenshot-status-bar.ts:223— not a defect: a failed snapshot already cannot safely restore unknown overrides; the timeout bounds that existing best-effort probe.screenshot.ts:261— not a defect: comment wording nit; the reservation covers the status bar regardless of backend path.commands.md:761— not a defect: timeout may leave the setting applied, but the command correctly invalidates cache before running and returns the command error; docs do not promise rollback.simctl.ts(cubic-dev-ai) — not a defect: it requests usinghostHomeDirectory(), which this code already does via host-kit.screenshot-status-bar.ts:96— real defect, blocking: cancellation from an in-progress setup/restore can be swallowed (P2 above).simctl.ts:22— real defect, blocking: process-home comparison can erase an explicit set scope; additionally fallback comparison after failed canonicalization is unsafe (P1 above).simctl.ts:32— not a defect: synchronous canonicalization overhead is real and merits caching, but is a performance concern without demonstrated correctness impact.capture/settings.ts:105— not a defect: code, CLI help and commands.md currently agree on 10 seconds; drift risk alone is not a present behavior mismatch.app-settings.ts:128— not a defect: a plain screenshot must wait while the status bar is being modified so it cannot capture an in-progress write; this is the reservation's intended serialization.
Verdict: needs changes.
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Round-2 independent review: needs-changes.
Reviewed 6d59c2b3e5680fd7ea84ce09891b610c3fd1a923 → 6f7b9ead18328ac4dd3c5a2ed0bba8d5a9b878eb, plus the affected callers and all 15 open bot threads returned by GraphQL reviewThreads (pagination complete). Source remained read-only.
P2 — Derive default-set identity from the executing host before removing an explicit --set. packages/platform-apple/src/core/simctl.ts:22-36 reads hostHomeDirectory() (process os.homedir()), then turns an explicit path matching that directory into undefined. However, daemon commands execute inside withCommandHost({ host, requestEnv }, ...) (packages/daemon/src/server/request-router.ts:222-224), and children receive that host's environment (packages/host-kit/src/internal/exec.ts:157-160). Those homes can already differ in the current architecture; this is not dependent on a future host feature. A source-executing probe with the bound host's HOME=/tmp/.../host-home and the explicit requested set /Users/thymikee/Library/Developer/CoreSimulator/Devices returned simctlListInventoryArgs(...) === ['list', '-j']: the caller's explicit set was discarded based on the unrelated process home. A host/provider whose unscoped default differs can therefore list or operate on a different set, or fail to find the requested UDID. In the reverse direction, the host's actual explicit default and its implicit default can retain different lock/cache keys. Resolve the default identity from the same host authority used to execute the command; when that authority cannot establish the default, retain the explicit set instead of guessing from the process home. Keep explicit canonical argv and implicit-default identity consistent without erasing the caller's scope based on another home. Add a differing-host-home regression case. This confirms the core of bot thread 4436131110; its extra realpath-failure scenario is not needed to establish the finding.
The three round-1 findings:
- Aliases: resolved for ordinary existing local sets, with the host-default defect above still blocking full closure. The real symlink and
/.probe produced the same canonical address and the same canonical--setargv.canonicalHostPathSyncbelongs in host-kit's existing file port: it is one narrow filesystem primitive, and Apple-specific default-set policy remains in platform-apple, as R13 requires. For a missing/unresolvable set, the catch retains an absolute normalized explicit path and does not create anything; simctl still owns the eventual success/failure. That fallback does not establish filesystem identity when realpath is unavailable, so the claim is not unconditional. For a stable, ordinary symlink to an existing set, sending the target path reaches the same directory; I found no demonstrated regression merely from deliberately using such a symlink. Erasing an explicit default based on the wrong home is the demonstrated behavior change. - Stale known-cleared cache: resolved. Wifi and both airplane branches invalidate inside
withSimulatorStatusBarHeld, before the command; airplane-off remembers cleared only after success. A separate capture cannot read the invalidated cache or half-applied write while that same key is held. The command settles before release, including timeout rejection. This removes the specific apply-then-timeout stale-cache failure. No production capture callback callssetIosSetting; the hypothetical re-entrant write from round 1 does not become a reachable regression here. - Wait/budget docs: resolved.
settings.ts:105,commands.md:761, and the screenshot bullet atcommands.md:1046describe the wait and cancellation correctly.STATUS_BAR_COMMAND_TIMEOUT_MSbounds the actual status-bar write after acquisition, not queue time or the whole request. Cancellation during the queue prevents the write. Timeout after acquisition does not promise rollback; the additional clarification suggested below is useful.
Hot-path assessment: for an explicit nondefault set, canonicalization performs two synchronous realpath operations each time it runs. A normal buildSimctlArgsForDevice canonicalizes at both simulatorAddressFor and scopeSimctlArgs, so it can perform four, with further calls for cache/lock keys. An implicit default returns before filesystem work. This is real repeated synchronous I/O, not a cached address; xcrun's timeout starts afterwards and does not bound a slow filesystem lookup. I have no measured local-filesystem latency regression to classify as another blocking finding. The bot's suggested lexical “outside home” shortcut would be incorrect for symlinks, and process-wide memoization would need to account for host identity and paths created/rebound later.
One-line verdicts for all 15 open bot threads (no thread replies posted):
- 4435811817 — failed restore / known-cleared: Not a defect as stated — a real-overrides snapshot is read only when the key is absent, and a successful restore-clear removes normalization before the override; the claimed simultaneous known-cleared state and leftover deterministic bar do not follow. Caller-snapshot recovery after failed restore remains the already-dispositioned, pre-existing follow-up.
- 4435811819 — missing user docs: Not a defect on this head — settings help plus
commands.md:761,1046now document both waits and queued cancellation. - 4435811821 — missing settings help: Not a defect on this head —
packages/command-surface/src/capture/settings.ts:105contains the requested wait and execution-budget explanation. - 4435811824 — predicate naming: Not a defect — a naming preference with no incorrect predicate or triggering behavior.
- 4435903514 — nested airplane restore: Not a production defect — independent requests queue; hold-only re-entry does not invoke normalization, and there is no production capture → settings call edge to trigger the hypothetical enclosing restore.
- 4435903518 — long total hold: Not a defect — command budgets deliberately bound steps rather than the transaction/queue; both plain screenshot waiting and post-wait settings budgets are documented, and rejecting overlap requires the shared hold.
- 4435903523 — snapshot timeout loses overrides: Real defect, nonblocking pre-existing recovery policy — a failed snapshot, including the newly bounded slow read, enters
canRestore:falseand later clears unknown caller overrides; the base already deliberately normalizes after read failure, and a longer timeout only moves that failure boundary. Track preservation/fail-closed behavior separately from the reviewed overlap fix. - 4435903524 — density-lock comment: Real defect, nonblocking P3 —
screenshot.ts:266-267attributes density coverage to the inner runner reservation, but its callback ends at runner capture; describe the outer fallback-flow hold instead (direct runner capture performs no density step). - 4436090997 — timeout outcome docs: Real defect, nonblocking P3 — the newly documented timeout should also tell callers that the bar may already have changed; only cancellation while queued guarantees no write.
- 4436091001 — direct os.homedir import: Not a defect on this head — this is the one outdated open thread; the direct
node:osuse was replaced by the host-kit port (the separate bound-host semantics issue is item 12). - 4436131105 — active cancellation swallowed: Not a defect as stated — status-bar setup/restore intentionally receive no request signal, and help expressly promises cancellation while waiting; throwing from a restore catch would jeopardize cleanup without fixing the asserted path.
- 4436131110 — wrong home authority: Real defect, blocking P2 — process home can differ from the bound executing host, so removing explicit
--setand deriving its default lock identity from it is unsafe; see the reproduced finding above. - 4436131112 — repeated synchronous realpath: Not an established defect — the repeated synchronous I/O is real for explicit sets, but not every command, and no material local latency regression is demonstrated; proposed permanent caching/lexical filtering would require additional correctness constraints.
- 4436131115 — literal timeout in help: Not a defect — the documented 10 seconds matches the owning constant; hypothetical future drift does not justify hiding an observable limit or importing platform implementation into command-surface.
- 4436131118 — plain capture waits on settings: Not a defect —
status_bar clear/overridechanges the visible bar a plain screenshot photographs, not merely its cache; sharing the reservation is deliberate.
Bot total: 4 real-defect threads: 1 blocking P2, 2 nonblocking P3 documentation issues, 1 nonblocking pre-existing snapshot-failure policy. The last is not a new round-2 finding.
Validation: static source/caller/test review, git diff --check (passed), and one standalone Node probe executing the source modules with workspace import resolution. The probe exercised a real symlink, /., a missing path, and a bound command host with a different HOME; it did not invoke simctl or any device. No package test suite, install/build, device operation, or CI wait was performed. Author-reported gate/device results were read as reports, not independently reproduced evidence. Approval remains blocked by the host-default P2.
There was a problem hiding this comment.
All reported issues were addressed across 9 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: 8 new findings on d4a6670. The verdict and the full list are in the summary comment.
Derive the default device root from the bound command host's environment instead of this process's home, so an explicit `--set` is only ever recognised as the default when the host that will execute `simctl` owns that root. When no host is bound, or that host names no absolute home, keep the caller's explicit set rather than guess from another home. The host-environment home lookup moves to the kernel owner of the environment type, where every caller already reaches it. Describe the density step as covered by the fallback flow's own hold, and tell settings callers that a timed-out write may already have changed the bar: only cancelling while the request is queued guarantees no write.
d4a6670 to
5ffe9ec
Compare
There was a problem hiding this comment.
All reported issues were addressed across 6 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: 4 new findings on 5ffe9ec. The verdict and the full list are in the summary comment.
|
Round 3 addresses review 5728727416 at new head Blocking P2 —
P3 — density-lock comment (bot 4435903524). P3 — timeout outcome docs (bot 4436090997). The settings help text and Other 13 bot threads — each replied with the reviewer's one-line verdict and resolved: 4435811817 not a defect as stated (caller-snapshot recovery stays the pre-existing follow-up); 4435811819 / 4435811821 not a defect on this head (docs landed); 4435811824 naming preference; 4435903514 not a production defect (no capture→settings edge); 4435903518 budgets bound steps deliberately; 4435903523 replied "follow-up, pre-existing on base" and resolved; 4436131105 cancellation while waiting is what help promises; 4436131112 repeated realpath real but not an established regression; 4436131115 literal matches the owning constant; 4436131118 sharing the reservation is deliberate; 4436091001 outdated — host-kit port in place. Gate. Live check on |
|
Coordinator check of round 3 (
No blocking findings. All findings from rounds 1–2 are resolved. |
|
I found no code defects at 5ffe9ec, and the findings from the earlier review are now fixed. Live proof is still missing: I did not run the new simctl and exec regression tests, so the fail-before claim is unverified. I also did not reproduce the live set-identity run. The overlapping-capture status bar run was owner-direct in the earlier round, not through the daemon. I did not verify that CoreSimulator resolves its default set from HOME rather than from the user record, so the default-root erase rests on that assumption. On the daemon route, host HOME equals the user's home, so behavior does not change there. CI is green, with 21 checks and none failing at this commit. There are no conflicts. Before merge, please answer the design question below. Not blocking, and you can take or leave it: Could the design be simpler? Key the status bar lock and the known-cleared cache by On the open inline threads: no P1 or P2 thread still applies. Four lower-priority threads still apply, and none of them blocks. Please resolve these, which no longer apply:
|
|
Implemented the maintainer's design simplification at
The first exact-head gate failed only in (1) Real daemon route, one session — production route evidence Device: host Seed setup: A Then another normalized Final LTE, cellular overrides, and carrier survived; Wi-Fi remained off after the capture. CoreSimulator chooses the bars count for the mode-only settings command, so the assertion checks Wi-Fi mode. Two earlier evidence-script attempts incorrectly pinned that bars count; their assertions failed, were corrected, and their logs were retained. (2) Two-capture overlap — isolated daemon harness evidence The daemon's per-device claim already prevents two sessions on one device, so overlapping capture transactions are reachable only through direct platform callers and this harness. No production claim checks or source code were weakened. The harness loaded the source modules at the named head and started two isolated embedded HTTP daemon runtimes in one process, each with a seeded session ( Only one caller-state read occurred while A was held. Both eventual Cleanup and artifacts: both live runs finished with Parent-relative round: production 9 files, +23/-97; tests 3 files, +6/-246; docs 0. Final PR: 12 files, production +263/-78, tests +1179/-135, docs +2/-0. At publication, GitHub shows the new head with 17 checks queued/running and 2 successful; coverage/integration remain GitHub-owned. This is not a merge-ready claim. |
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on f01442f. The verdict and the full list are in the summary comment.
|
Handled all six threads in one help/docs-only round at
All six threads have replies and are resolved. Validation: Parent-relative scope: 2 files, +4/-3 (help data: +2/-1; command reference: +2/-2); no behavior or test changes. Cold self-review completed; no new independent review. No live run was needed because capture routing and execution are unchanged. The separately labeled production/harness evidence remains attributed to |
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on be0846f. The verdict and the full list are in the summary comment.
|
The changes in be0846f look good. The help and docs now separate the 90 s request timeout, which includes the wait, from the 10 s write limit, and the missing blank line before the status-bar paragraph is fixed. This answers the simplicity challenge from the earlier review (#4136 (comment)). All 21 checks pass at this commit. I did not run the focused suites or the check:affected gate, so the pass counts are yours. I did not repeat the live daemon-route run either. It was taken at f01442f, and be0846f differs from it only in help and docs, so it covers this head. There are no conflicts. None of the earlier inline threads from the other review still applies. These two are fixed at this head: the blank line in settings.ts (#4136 (comment)) and the timeout wording in the help and commands.md (#4136 (comment)). These are benign by design or outside this change, and they are already resolved: the udid-only lock key (#4136 (comment)), the positional signal on setIosSetting (#4136 (comment)), the explicit xcrun timeouts (#4136 (comment)), abort handling after admission in the keyed lock (#4136 (comment)), cache sharing across copied sets, which was already the behavior on main (#4136 (comment)), the lock window over the container copy (#4136 (comment)), the help figures matching the constants (#4136 (comment)), and re-entry into withNormalizedSimulatorStatusBar, which has no production caller (#4136 (comment)). |
thymikee
added
the
ready-for-human
Summary
Fix SM-11: hold snapshot → normalize → capture → restore under one iOS simulator status-bar lock. Plain captures and Wi-Fi/airplane writes share the hold; writes retain their budget and invalidate the cleared-state cache before mutation.
The lock and cache use
platform:kind:udid. CoreSimulator creates a UUID per device; copied sets with colliding UDIDs may over-serialize, an accepted trade-off. Set-path identity changes remain reverted to main. Twelve files remain in the PR.Help/docs distinguish the request timeout (90 seconds for standalone CLI settings, including waiting) from the 10-second status-bar command limit. Screenshot reads/preparation allow 10 seconds per command; restoration allows 30 seconds per command.
Validation
Head
be0846f81c868339e01a1faff6047ffe1450acee; this round changes only help/docs fromf01442f6.f01442f6: worktree build onlocal/e2e-apex; normalized screenshot restores the seed, then concurrently requested Wi-Fi off executes and survives.f01442f6: two isolated embedded HTTP daemon runtimes with seeded sessions share the platform owner; both restore the seed.The daemon's per-device claim prevents two sessions on one device, so overlap is reachable only through direct platform callers and the harness. The capture route is unchanged; live evidence was not rerun. See live evidence and this review round. Coverage/integration remain GitHub-owned.