fix(quality): clear the dev SonarCloud reliability findings blocking the gate (#2604)

* fix(quality): clear the dev SonarCloud reliability findings blocking the gate

dev's SonarCloud quality gate has been failing on `new_reliability_rating = C`
(threshold A) across several runs, which fails the `Coverage and SonarCloud`
job and, through it, the fail-closed `ci` aggregate. Five findings drive it;
all five are analyzer artifacts against correct code, so each is resolved by
restating the same behavior in a shape the analyzer can follow rather than by
changing semantics.

typescript:S4822 (4x) — "promise inside a try". Four call sites share one
deliberate, commented pattern: a `try` containing only a promise-returning call
guards a SYNCHRONOUS throw from a non-conforming implementation, while the
rejection is handled just after the block. Sonar sees only the floating promise.
Each now builds the chain (including its `.catch`) inside the `try`:

- deps.ts `reconcileTaskWorkspacesAtStartup` and registerSw.ts
  `cleanupDevServiceWorkers`/`registerSw` swallow both failure modes by design,
  so attaching `.catch` at the call site is exactly equivalent — and drops a
  now-redundant `pending` local plus its `ReturnType<...>` annotation.
- opencodeRuntimeComposition.ts `startFacadeExecution` must NOT swallow the
  rejection: the awaiting request path maps it to 502/408 and settles the tool.
  It gets a no-op `.catch` tap instead, which does not consume the rejection for
  that path (it still observes it via `raceAbort`) and additionally suppresses an
  unhandled-rejection warning on the abort path, where the caller can stop
  awaiting before the facade settles.

javascript:S3403 (1x) — `options.policy === "production"` reported as always
false in verify-portable-runtime-signing.mjs. "production" is a real member of
PORTABLE_VERIFICATION_POLICIES; the comparison is reachable. The all-`undefined`
object literal in parseArgs pinned every property's inferred type to `undefined`,
so the analyzer read every later value comparison as impossible. parseArgs now
declares the three locals and assembles the object on return.

No behavior change intended. Verified locally: typecheck, lint, format:check,
keiko-ui install suite (59), scripts suite (1794) all green. The 6 failures in
codingAutonomyQaMatrix.test.ts under a path-filtered server run reproduce
identically with these changes stashed — pre-existing, filter-context only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(quality): cover verify-portable-runtime-signing argument parsing

Restating `parseArgs` in the previous commit put its lines into SonarCloud's
new-code window, where they were measured and found entirely uncovered: this
script had no test file at all, which drove the PR's new-code coverage to 39.1%
against an 80% threshold.

Exports `parseArgs` behind the `@internal` marker the repo already uses for
test-only seams (see deps.ts) and adds eight direct unit tests. A direct import
rather than a spawned subprocess is deliberate: subprocess-driven script tests
report 0% v8 coverage against the vitest parent, so they would not have moved
the measurement.

Cases: every supported policy returned verbatim (a regression pin for the
javascript:S3403 fix — the previous all-`undefined` options literal made
`options.policy === "production"` read as statically impossible), the optional
verification input, order independence, and all six fail-closed paths (missing
manifest, missing policy, unsupported policy, unsupported argument, flag without
a value, empty flag value). `fail()` ends the process, so `process.exit` is
trapped through a throw, matching release-script-lcov-mapping.test.mjs.

The signing script's argument validation was untested until now, independent of
the gate.

Verified locally: lint, format:check, check:knip, and the full scripts suite
(95 files, 1802 tests) green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(quality): parameterize the parseArgs fail-closed cases (S5976)

SonarCloud flagged three of the six fail-closed cases as structurally identical
(javascript:S5976). They differ only in the argv they feed, so they collapse into
one it.each table without losing a case or its label.

Verified: 8 tests still pass, eslint and prettier clean on the file.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: oscharko

packages/keiko-server/src/coding-runtime/opencodeRuntimeComposition.ts

@@ -1058,9 +1058,9 @@ function responseForToolResult(
 }
 
 // Start the facade call, containing only its SYNCHRONOUS throw (a facade that dies before
-// returning a promise). The thrown value is carried back to the caller — which surfaces it as an
-// operator diagnostic before failing the request — while the returned promise's rejection is
-// handled by the awaiting request path, so no promise ever lives inside a try (typescript:S4822).
+// returning a promise). The thrown value is carried back to the caller, which surfaces it as an
+// operator diagnostic before failing the request; the returned promise's rejection stays owned by
+// the awaiting request path, which maps it to a 502/408 and settles the tool.
 type FacadeStart =
   | { readonly ok: true; readonly work: Promise<CodingToolResult> }
   | { readonly ok: false; readonly error: unknown };
@@ -1073,10 +1073,13 @@ function startFacadeExecution(
   admission: AdmittedToolRequest,
 ): FacadeStart {
   try {
-    return {
-      ok: true,
-      work: facade.execute({ body, capability, headers, signal: admission.controller.signal }),
-    };
+    const work = facade.execute({ body, capability, headers, signal: admission.controller.signal });
+    // A no-op tap, NOT the error handler: attaching it does not consume the rejection for the
+    // awaiting request path, which still observes it through `raceAbort`. It keeps the promise from
+    // floating inside this `try` (typescript:S4822) and suppresses an unhandled-rejection warning on
+    // the abort path, where the caller can stop awaiting before the facade settles.
+    void work.catch(() => undefined);
+    return { ok: true, work };
   } catch (error) {
     return { ok: false, error };
   }

packages/keiko-server/src/deps.ts

@@ -1688,18 +1688,18 @@ export function reconcileTaskWorkspacesAtStartup(
   service: WorkspaceReconciliationService | undefined,
 ): void {
   if (service === undefined) return;
-  // `service.reconcile()` is typed as always returning a Promise, but the call itself (property
-  // lookup + invocation) can still throw synchronously for a non-conforming implementation (e.g. a
-  // test double). Isolate that from the Promise-rejection path below so each failure mode is
-  // handled exactly once.
-  let pending: ReturnType<WorkspaceReconciliationService["reconcile"]>;
+  // Construction must never fail because of reconciliation, so BOTH failure modes are swallowed
+  // here and each is still handled exactly once: `.catch` absorbs the Promise rejection, and the
+  // surrounding `try` absorbs a synchronous throw from the call itself (property lookup +
+  // invocation), which a non-conforming implementation (e.g. a test double) can still raise even
+  // though `reconcile()` is typed as always returning a Promise. Attaching `.catch` at the call
+  // site rather than after the `try` keeps the promise from ever floating inside it
+  // (typescript:S4822).
   try {
-    pending = service.reconcile();
+    void service.reconcile().catch(() => undefined);
   } catch {
-    // construction must never fail because of reconciliation.
     return;
   }
-  void pending.catch(() => undefined);
 }
 
 function seedInitialProject(

packages/keiko-ui/src/app/components/desktop/install/registerSw.ts

@@ -112,30 +112,30 @@ function deleteKeikoShellCaches(): void {
 }
 
 function cleanupDevServiceWorkers(sw: ServiceWorkerContainer): void {
-  // Wrap the synchronous call too: although the spec says `getRegistrations()` returns a
-  // Promise, a non-conforming runtime (or a test stub) could throw synchronously, and this
-  // helper's whole contract is that it never breaks the app. Isolate that guard from the
-  // already-`.catch`-handled Promise chain below so each failure mode is handled exactly once.
-  let pending: ReturnType<ServiceWorkerContainer["getRegistrations"]>;
+  // Silent failure by design: development cleanup must never break the app, so both failure modes
+  // are swallowed and each is still handled exactly once. `.catch` absorbs the Promise rejection,
+  // and the surrounding `try` absorbs a synchronous throw from the call itself, which a
+  // non-conforming runtime (or a test stub) could still raise even though the spec says
+  // `getRegistrations()` returns a Promise. Building the chain inside the `try` keeps the promise
+  // from ever floating in it (typescript:S4822).
   try {
-    pending = sw.getRegistrations();
+    void sw
+      .getRegistrations()
+      .then((registrations) =>
+        Promise.all(registrations.map((registration) => registration.unregister())),
+      )
+      .then(() => {
+        deleteKeikoShellCaches();
+        if (sw.controller === null) return;
+        const reloadKey = "keiko.dev.service-worker-cleanup-reloaded";
+        if (window.sessionStorage.getItem(reloadKey) === "true") return;
+        window.sessionStorage.setItem(reloadKey, "true");
+        window.location.reload();
+      })
+      .catch((_error: unknown) => undefined);
   } catch {
-    // Silent failure by design. Development cleanup must never break the app.
     return;
   }
-  void pending
-    .then((registrations) =>
-      Promise.all(registrations.map((registration) => registration.unregister())),
-    )
-    .then(() => {
-      deleteKeikoShellCaches();
-      if (sw.controller === null) return;
-      const reloadKey = "keiko.dev.service-worker-cleanup-reloaded";
-      if (window.sessionStorage.getItem(reloadKey) === "true") return;
-      window.sessionStorage.setItem(reloadKey, "true");
-      window.location.reload();
-    })
-    .catch((_error: unknown) => undefined);
 }
 
 export function registerSw(): void {
@@ -150,26 +150,22 @@ export function registerSw(): void {
     return;
   }
 
-  // Fire-and-forget. Wrap the synchronous call too: although the spec says `register()`
-  // returns a Promise, a non-conforming runtime (or a test stub) could throw synchronously,
-  // and the helper's whole contract is that it never breaks the page. Isolate that guard from
-  // the already-`.catch`-handled Promise chain below so each failure mode is handled exactly
-  // once. `.catch` swallows the Promise rejection. Using `unknown` for the rejection value —
-  // `register()` rejects with `DOMException` in spec but other runtimes may differ, and we
-  // never inspect the error in production code.
-  let pending: ReturnType<ServiceWorkerContainer["register"]>;
+  // Fire-and-forget, silent failure by design: static shell caching must never break app startup,
+  // so both failure modes are swallowed and each is still handled exactly once. `.catch` absorbs
+  // the Promise rejection, and the surrounding `try` absorbs a synchronous throw from the call
+  // itself, which a non-conforming runtime (or a test stub) could still raise even though the spec
+  // says `register()` returns a Promise. Building the chain inside the `try` keeps the promise from
+  // ever floating in it (typescript:S4822). Using `unknown` for the rejection value — `register()`
+  // rejects with `DOMException` in spec but other runtimes may differ, and we never inspect the
+  // error in production code.
   try {
-    pending = sw.register("/sw.js", { scope: "/" });
+    void sw
+      .register("/sw.js", { scope: "/" })
+      .then((registration) => {
+        handleRegisteredServiceWorker(sw, registration);
+      })
+      .catch((_error: unknown) => undefined);
   } catch {
-    // Silent failure by design. Static shell caching must never break app startup.
     return;
   }
-  void pending
-    .then((registration) => {
-      handleRegisteredServiceWorker(sw, registration);
-    })
-    .catch((_error: unknown) => {
-      // Silent failure by design. Static shell caching must never break app startup.
-      return undefined;
-    });
 }

scripts/__tests__/verify-portable-runtime-signing-args.test.mjs

@@ -0,0 +1,64 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+
+import { parseArgs } from "../verify-portable-runtime-signing.mjs";
+
+// `fail()` ends the process. Route it through a throw so the rejection paths stay assertable in
+// band, matching the convention in release-script-lcov-mapping.test.mjs.
+function trapExit() {
+  vi.spyOn(console, "error").mockImplementation(() => undefined);
+  return vi.spyOn(process, "exit").mockImplementation((code) => {
+    throw new Error(`process.exit(${String(code)})`);
+  });
+}
+
+afterEach(() => {
+  vi.restoreAllMocks();
+});
+
+describe("verify-portable-runtime-signing parseArgs", () => {
+  it("returns every supported policy verbatim, so a later value comparison stays reachable", () => {
+    // Regression pin for javascript:S3403: an all-`undefined` options literal used to pin each
+    // property's inferred type to `undefined`, which made `options.policy === "production"` read as
+    // statically impossible even though "production" is a real policy.
+    for (const policy of ["staging", "development", "pull-request", "production"]) {
+      expect(parseArgs(["--manifest", "/tmp/m.json", "--policy", policy])).toEqual({
+        manifest: "/tmp/m.json",
+        policy,
+        verificationInput: undefined,
+      });
+    }
+  });
+
+  it("accepts the optional verification input and is order-independent", () => {
+    expect(
+      parseArgs([
+        "--policy",
+        "production",
+        "--verification-input",
+        "/tmp/in.json",
+        "--manifest",
+        "/tmp/m.json",
+      ]),
+    ).toEqual({
+      manifest: "/tmp/m.json",
+      policy: "production",
+      verificationInput: "/tmp/in.json",
+    });
+  });
+
+  it.each([
+    ["a missing manifest", ["--policy", "production"]],
+    ["a missing policy", ["--manifest", "/tmp/m.json"]],
+    ["a policy outside the supported set", ["--manifest", "/tmp/m.json", "--policy", "prod-ish"]],
+    [
+      "an unsupported argument",
+      ["--manifest", "/tmp/m.json", "--policy", "production", "--sign-anyway"],
+    ],
+    ["a flag present without its value", ["--manifest"]],
+    ["a flag value that is an empty string", ["--policy", ""]],
+  ])("fails closed on %s", (_case, argv) => {
+    const exit = trapExit();
+    expect(() => parseArgs(argv)).toThrow("process.exit(1)");
+    expect(exit).toHaveBeenCalledWith(1);
+  });
+});

scripts/verify-portable-runtime-signing.mjs

@@ -22,35 +22,40 @@ function fail(message) {
   process.exit(1);
 }
 
-function parseArgs(argv) {
-  const options = { manifest: undefined, policy: undefined, verificationInput: undefined };
+// Declared-then-assigned rather than an all-`undefined` object literal: the literal pinned every
+// property's inferred type to `undefined`, which made later value comparisons (e.g.
+// `options.policy === "production"`) read as statically impossible (javascript:S3403).
+/** @internal Exported only for the deterministic argument-parsing tests. */
+export function parseArgs(argv) {
+  let manifest;
+  let policy;
+  let verificationInput;
   for (let index = 0; index < argv.length; index += 1) {
     const arg = argv[index];
     const value = argv[index + 1];
     if (arg === "--manifest") {
-      options.manifest = requiredValue(value, arg);
+      manifest = requiredValue(value, arg);
       index += 1;
       continue;
     }
     if (arg === "--policy") {
-      options.policy = requiredValue(value, arg);
+      policy = requiredValue(value, arg);
       index += 1;
       continue;
     }
     if (arg === "--verification-input") {
-      options.verificationInput = requiredValue(value, arg);
+      verificationInput = requiredValue(value, arg);
       index += 1;
       continue;
     }
     fail(`unsupported argument: ${arg}`);
   }
-  if (options.manifest === undefined) fail("pass --manifest <path>");
-  if (options.policy === undefined)
-    fail("pass --policy <staging|development|pull-request|production>");
-  if (!PORTABLE_VERIFICATION_POLICIES.includes(options.policy)) {
-    fail(`unsupported verification policy: ${options.policy}`);
+  if (manifest === undefined) fail("pass --manifest <path>");
+  if (policy === undefined) fail("pass --policy <staging|development|pull-request|production>");
+  if (!PORTABLE_VERIFICATION_POLICIES.includes(policy)) {
+    fail(`unsupported verification policy: ${policy}`);
   }
-  return options;
+  return { manifest, policy, verificationInput };
 }
 
 function requiredValue(value, arg) {