Conversation
Launch Safari through LaunchServices and surface child-process errors instead of converting them into missing browser reports. Verified exact candidate tree a7097c1: build, 712 unit tests, seeded browser self-test, real JSON smoke, and forced osascript failure. Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found across 1 file
Confidence score: 3/5
- In
scripts/browser-smoke.mjs, the end-of-run tab-closingosacan throw on anosascripterror, skipping the verdict block and returning exit code 1 even when the smoke run otherwise succeeds; make cleanup errors non-fatal so the verdict is always reported. - In
scripts/browser-smoke.mjs, sendingactivateimmediately afteropen -a Safarican hit Safari’s cold-start-600 application isn't runningerror, causing intermittent startup failures; wait for Safari to be running or retry the Apple event.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/browser-smoke.mjs">
<violation number="1" location="scripts/browser-smoke.mjs:244">
P2: The end-of-run tab-closing `osa` is now fatal: because `osa` delegates to `run` and rejects on any osascript error, a failure while closing tabs throws at top level, skips the verdict block, and exits 1 even when every tool passed — hiding the "reported X/Y ... PASS" summary. That cleanup is best-effort and is neither a launch nor a navigation failure, so it should not nullify the results. Wrap it in try/catch (or call it without await) so a cleanup error cannot mask the verdict.</violation>
<violation number="2" location="scripts/browser-smoke.mjs:278">
P2: `open -a Safari` returns before Safari finishes launching, and the code immediately sends the `activate` Apple event. osascript can transiently error with '-600 application isn't running' during a cold start; the old `osa` swallowed that error and the following `sleep(2000)` let Safari finish. `run`/`osa` now reject, so this genuine but transient race aborts the smoke run with an unhandled rejection instead of passing. Give Safari a short settle (or retry `activate`) between `open` and the Apple event so only real launch failures propagate.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| else resolve(); | ||
| }), | ||
| ); | ||
| const osa = (script) => run("osascript", ["-e", script]); |
There was a problem hiding this comment.
P2: The end-of-run tab-closing osa is now fatal: because osa delegates to run and rejects on any osascript error, a failure while closing tabs throws at top level, skips the verdict block, and exits 1 even when every tool passed — hiding the "reported X/Y ... PASS" summary. That cleanup is best-effort and is neither a launch nor a navigation failure, so it should not nullify the results. Wrap it in try/catch (or call it without await) so a cleanup error cannot mask the verdict.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/browser-smoke.mjs, line 244:
<comment>The end-of-run tab-closing `osa` is now fatal: because `osa` delegates to `run` and rejects on any osascript error, a failure while closing tabs throws at top level, skips the verdict block, and exits 1 even when every tool passed — hiding the "reported X/Y ... PASS" summary. That cleanup is best-effort and is neither a launch nor a navigation failure, so it should not nullify the results. Wrap it in try/catch (or call it without await) so a cleanup error cannot mask the verdict.</comment>
<file context>
@@ -233,10 +233,15 @@ const server = http.createServer(async (req, res) => {
+ else resolve();
+ }),
);
+const osa = (script) => run("osascript", ["-e", script]);
let opened = false;
</file context>
| // navigation, and the self-test would otherwise fail for lack of time. | ||
| // AppleScript does not reliably launch a cold Safari process. Launch it via | ||
| // LaunchServices first, and surface either launch or navigation failures. | ||
| await run("open", ["-a", "Safari"]); |
There was a problem hiding this comment.
P2: open -a Safari returns before Safari finishes launching, and the code immediately sends the activate Apple event. osascript can transiently error with '-600 application isn't running' during a cold start; the old osa swallowed that error and the following sleep(2000) let Safari finish. run/osa now reject, so this genuine but transient race aborts the smoke run with an unhandled rejection instead of passing. Give Safari a short settle (or retry activate) between open and the Apple event so only real launch failures propagate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/browser-smoke.mjs, line 278:
<comment>`open -a Safari` returns before Safari finishes launching, and the code immediately sends the `activate` Apple event. osascript can transiently error with '-600 application isn't running' during a cold start; the old `osa` swallowed that error and the following `sleep(2000)` let Safari finish. `run`/`osa` now reject, so this genuine but transient race aborts the smoke run with an unhandled rejection instead of passing. Give Safari a short settle (or retry `activate`) between `open` and the Apple event so only real launch failures propagate.</comment>
<file context>
@@ -268,8 +273,9 @@ console.log(
-// navigation, and the self-test would otherwise fail for lack of time.
+// AppleScript does not reliably launch a cold Safari process. Launch it via
+// LaunchServices first, and surface either launch or navigation failures.
+await run("open", ["-a", "Safari"]);
await osa('tell application "Safari" to activate');
await sleep(2000);
</file context>
Summary
test(safari): propagate smoke harness launch failures. This is logical change 1/11 in the dependency-ordered product-audit release stack.
Refs #969
What changed
b15e277bab25fd4ad1ce88363aedba70571063a9; validated tree:a7097c1115598fcf4a3ce5b841440271fdad7a79.Why
Make Safari smoke failures authoritative instead of passing after a browser launch failure.
Verification
bun run buildpassed on this exact candidate tree.npm test -- --maxWorkers=2passed on this exact candidate tree.Safari fault injection detected asset/console/error/rejection failures; real JSON smoke and forced launch-error propagation passed.
Final combined tree additionally passed 801 unit tests and all 293 Playwright tests with retries disabled, plus all 48 primary tool workflows at desktop and mobile. The exploratory Color Converter exact-HEX boundary remains a documented pre-existing defect; its runtime is unchanged by this stack.
Risk & rollback
Summary by cubic
Makes Safari smoke test launch failures propagate instead of passing silently. Previously, a failed browser launch (e.g., cold Safari not starting) was swallowed, and the test could report a missing-browser outcome rather than a failure. The harness now launches Safari through
open -a Safari(LaunchServices) before running AppleScript, and any child-process error rejects the test. This makes the smoke test authoritative for launch failures. Refs #969.Written for commit b15e277. Summary will update on new commits.