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>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 4/5
- In
scripts/browser-smoke.mjs, Safari may not finish launching before the activation Apple event is sent, causing intermittent-600failures on cold starts; wait for Safari to launch or retry activation.
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: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
| // 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>
Tab cleanup is best-effort: an osascript error while closing the harness's tabs threw past the verdict and exited 1 with no PASS/FAILED summary. Warn instead, while launch and navigation failures stay fatal. Adds a Safari-free exit-code contract test that drives the harness through fake open/osascript executables.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
The fake is an extensionless file outside a type:module package, so Node loads it as CommonJS; its top-level await only worked because Node >= 22.12 re-parses such files as ESM. Wrap the body in an async IIFE so it also runs on Node 22.0-22.11 (--no-experimental-detect-module), keeping the exact open/osascript names the harness resolves on PATH.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 5/5
- In
src/contracts/browser-smoke-harness.test.js, the Node version threshold comment is inaccurate about when module syntax detection became enabled by default; update it to reflect v22.7.0 and v20.19.0.
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="src/contracts/browser-smoke-harness.test.js">
<violation number="1" location="src/contracts/browser-smoke-harness.test.js:21">
P3: The comment's version threshold is off: Node's module syntax detection (which re-parses extensionless files with top-level await as ESM) was enabled by default in v22.7.0 and v20.19.0 (nodejs.org/api/packages.html, "Syntax detection" version table), not v22.12.0. The IIFE is still the correct, robust fix (it is also needed for 22.0–22.6 and for runs with `--no-experimental-detect-module` on 22.7–22.11), but the stated reason is inaccurate and can mislead future maintainers reasoning about when the workaround is required.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const script = process.argv[process.argv.indexOf("-e") + 1] || ""; | ||
| const fail = (msg) => { process.stderr.write(msg + "\\n"); process.exit(1); }; | ||
| // Async IIFE, not top-level await: the file is extensionless, so Node loads it as | ||
| // CommonJS and only Node >= 22.12 re-parses it as ESM on a top-level await. |
There was a problem hiding this comment.
P3: The comment's version threshold is off: Node's module syntax detection (which re-parses extensionless files with top-level await as ESM) was enabled by default in v22.7.0 and v20.19.0 (nodejs.org/api/packages.html, "Syntax detection" version table), not v22.12.0. The IIFE is still the correct, robust fix (it is also needed for 22.0–22.6 and for runs with --no-experimental-detect-module on 22.7–22.11), but the stated reason is inaccurate and can mislead future maintainers reasoning about when the workaround is required.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contracts/browser-smoke-harness.test.js, line 21:
<comment>The comment's version threshold is off: Node's module syntax detection (which re-parses extensionless files with top-level await as ESM) was enabled by default in v22.7.0 and v20.19.0 (nodejs.org/api/packages.html, "Syntax detection" version table), not v22.12.0. The IIFE is still the correct, robust fix (it is also needed for 22.0–22.6 and for runs with `--no-experimental-detect-module` on 22.7–22.11), but the stated reason is inaccurate and can mislead future maintainers reasoning about when the workaround is required.</comment>
<file context>
@@ -17,6 +17,9 @@ const HARNESS = path.join(ROOT, "scripts/browser-smoke.mjs");
const script = process.argv[process.argv.indexOf("-e") + 1] || "";
const fail = (msg) => { process.stderr.write(msg + "\\n"); process.exit(1); };
+// Async IIFE, not top-level await: the file is extensionless, so Node loads it as
+// CommonJS and only Node >= 22.12 re-parses it as ESM on a top-level await.
+(async () => {
if (/close tab/.test(script)) {
</file context>
| // CommonJS and only Node >= 22.12 re-parses it as ESM on a top-level await. | |
| // CommonJS and only Node >= 22.7 re-parses it as ESM on a top-level await. |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
1 participant
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 failures authoritative instead of passing silently. A failed browser launch previously surfaced as a missing-browser outcome rather than a failure; the harness now launches Safari through
open -a Safariand propagates child-process errors, while tab-cleanup failures warn instead of aborting before the verdict. Adds a Safari-free exit-code contract test driven by fakeopen/osascriptexecutables that also runs on Node 22.0–22.11. Refs #969.Written for commit 1723273. Summary will update on new commits.