Merge reviewed contributor PR #3244 into backlog batch

Source-PR: https://github.com/affaan-m/ECC/pull/3244
Source-Head: 5ffe239ee5

Local integration checkpoint; aggregate review and hosted acceptance pending.

scripts/lib/platform-launch.js

@@ -9,10 +9,11 @@
  * branch that silently no-op'd on Windows/Linux). This helper:
  *
  *   1. Dispatches `open` / `cmd /c start` / `xdg-open` based on process.platform
- *   2. Wires the child's 'error' event so ENOENT / EACCES propagate to the caller
- *      instead of being swallowed by detached spawns
- *   3. Returns a structured { opened, reason } result so CLI consumers can
- *      surface the truth (browser did/did not open) instead of a lying true/false
+ *   2. Handles the child's 'error' event so a missing launcher does not cause
+ *      an unhandled error after a detached spawn
+ *   3. Returns a structured { opened, reason } result for the launch request.
+ *      Later asynchronous errors cannot change the returned result; success
+ *      does not prove a browser opened.
  *
  * The signature is intentionally small (single function, no class) so callers
  * can import without picking up the rest of scripts/lib.
@@ -39,15 +40,15 @@ function openerCommandFor(platform, url) {
 /**
  * Open a URL in the user's default browser, dispatching per-platform.
  *
- * Always returns a structured result so callers can:
- *   - show a clear error to the agent (no silent failures)
- *   - keep JSON CLI output truthful when browsers cannot launch
+ * Returns the synchronous launch-request result. Asynchronous child errors
+ * are handled, but are not an acknowledgment that a browser opened.
  *
  * @param {string} url
  * @param {NodeJS.Platform} [platform] - injectable for tests; defaults to process.platform
+ * @param {typeof spawn} [spawnProcess] - injectable process launcher for tests
  * @returns {{ opened: boolean, reason: string }}
  */
-function openBrowser(url, platform = process.platform) {
+function openBrowser(url, platform = process.platform, spawnProcess = spawn) {
   if (typeof url !== 'string' || url.length === 0) {
     return { opened: false, reason: 'invalid-url' };
   }
@@ -55,7 +56,7 @@ function openBrowser(url, platform = process.platform) {
   const [cmd, args] = openerCommandFor(platform, url);
   let child;
   try {
-    child = spawn(cmd, args, {
+    child = spawnProcess(cmd, args, {
       detached: true,
       stdio: 'ignore',
     });

tests/lib/platform-launch.test.js

@@ -21,28 +21,65 @@ test('openerCommandFor: unknown falls through to xdg-open', () => {
 });
 
 test('openBrowser: invalid url returns invalid-url without spawning', () => {
-  const r1 = openBrowser('');
-  assert.equal(r1.opened, false);
-  assert.equal(r1.reason, 'invalid-url');
-  const r2 = openBrowser(null);
-  assert.equal(r2.opened, false);
-  assert.equal(r2.reason, 'invalid-url');
+  let calls = 0;
+  const launch = () => { calls += 1; };
+  assert.deepEqual(openBrowser('', 'linux', launch), { opened: false, reason: 'invalid-url' });
+  assert.deepEqual(openBrowser(null, 'linux', launch), { opened: false, reason: 'invalid-url' });
+  assert.equal(calls, 0);
 });
 
-test('openBrowser: returns structured { opened, reason }', () => {
-  // Use a platform + URL that's syntactically valid. We can't easily assert
-  // whether the browser actually opens in CI, but the structure must match.
-  const r = openBrowser('http://localhost:0', 'linux');
-  assert.equal(typeof r.opened, 'boolean');
-  assert.equal(typeof r.reason, 'string');
-  assert.ok(r.reason.length > 0);
+test('openBrowser: reports synchronous launcher failures', () => {
+  const withCode = () => { throw Object.assign(new Error('missing launcher'), { code: 'ENOENT' }); };
+  const withoutCode = () => { throw new Error('launcher failed'); };
+  assert.deepEqual(openBrowser('http://localhost:0', 'linux', withCode), {
+    opened: false, reason: 'spawn-threw:ENOENT',
+  });
+  assert.deepEqual(openBrowser('http://localhost:0', 'linux', withoutCode), {
+    opened: false, reason: 'spawn-threw:unknown',
+  });
 });
 
-test('openBrowser: uses xdg-open on linux', () => {
-  // Spy by stubbing spawn via require cache (not possible without mocking module).
-  // Smoke-test: just ensure the function is callable.
-  const r = openBrowser('http://localhost:0', 'linux');
-  // Either opened=true (xdg-open exists on runner) or opened=false with reason
-  assert.ok(['spawned', 'child-error:ENOENT', 'child-error:EACCES', 'spawn-threw:ENOENT'].includes(r.reason)
-      || r.opened === true || r.opened === false);
+test('openBrowser: installs an error listener and detaches the launcher', () => {
+  const handlers = new Map();
+  let unrefCalls = 0;
+  let launchCalls = 0;
+  const child = {
+    on(event, listener) {
+      handlers.set(event, listener);
+    },
+    unref() {
+      assert.equal(typeof handlers.get('error'), 'function', 'listen before detaching');
+      unrefCalls += 1;
+    },
+  };
+
+  const result = openBrowser('http://localhost:0', 'linux', (command, args, options) => {
+    launchCalls += 1;
+    assert.equal(command, 'xdg-open');
+    assert.deepEqual(args, ['http://localhost:0']);
+    assert.deepEqual(options, { detached: true, stdio: 'ignore' });
+    return child;
+  });
+
+  assert.deepEqual(result, { opened: true, reason: 'spawned' });
+  assert.equal(launchCalls, 1);
+  assert.equal(unrefCalls, 1);
+  assert.equal(typeof handlers.get('error'), 'function');
+  assert.doesNotThrow(() => handlers.get('error')({ code: 'ENOENT' }));
+});
+
+test('openBrowser: a detach failure does not escape after the listener is installed', () => {
+  let listener;
+  const result = openBrowser('http://localhost:0', 'linux', () => ({
+    on(event, callback) {
+      assert.equal(event, 'error');
+      listener = callback;
+    },
+    unref() {
+      assert.equal(typeof listener, 'function');
+      throw new Error('cannot detach');
+    },
+  }));
+  assert.deepEqual(result, { opened: true, reason: 'spawned' });
+  assert.doesNotThrow(() => listener({ code: 'EACCES' }));
 });