fix: stop the nightly crying wolf, and prove the deny is actually armed

Three from the final whole-diff review.

The worst was self-inflicted and would have shipped as a recurring 4am alarm.
WEBJS_REQUIRE_NETWORK did double duty: it selected the live files AND promoted
every upstream-trouble skip into a failure. The nightly always sets it, and the
runners only select those files under it, so the transport-level skip that
#1219 built was unreachable everywhere automated. A single jspm 503 or DNS blip
would have redded the job and filed a bug issue, which is exactly the cry-wolf
outcome the workflow's own header argues against. The two concerns are separate
variables now: WEBJS_REQUIRE_NETWORK selects and lifts the deny,
WEBJS_FAIL_ON_SKIP promotes, and the nightly sets only the first. A skip
becomes a warning annotation instead, so a permanently skipping test is still
visible without waking anyone for an outage.

The live pin test had a wider version of the same trap. Its skip fired only on
a non-zero exit, but a hiccup on the bundle GET leaves pin exiting 0 with the
entry pinned and no hash, so the integrity assertion hard-failed on an outage
the exit code had already forgiven. It skips there too now.

withJspmDouble threw from a finally, which REPLACES an in-flight error. A
refused request usually travels with the assertion it broke, so that discarded
the message explaining what went wrong, in the case where it matters most. The
restore stays in the finally; the report moved to the success path.

And the guard file claimed to assert both enforcement mechanisms while never
checking the deny was armed in a running process. Its other tests exercise a
pure function and grep runner sources, so an inverted self-install would leave
them all green while the required job went back to reaching jspm. It now spawns
a preloaded child and asserts both branches of the switch.

Author: vivek7405

.github/workflows/vendor-cdn.yml

@@ -22,10 +22,18 @@ name: Vendor CDN contract (nightly)
 # outage; it would just be a red somebody is told to ignore, which is how a
 # real failure gets ignored too.
 #
-# WEBJS_REQUIRE_NETWORK does double duty. It selects the live files, and it
-# turns their upstream-trouble SKIP into a FAILURE. Without that second half a
-# permanently skipping test is indistinguishable from a passing one, which is
-# the exact way live coverage rots into decoration.
+# WEBJS_REQUIRE_NETWORK selects the live files and lifts the test-run deny. It
+# does NOT promote their upstream-trouble skip into a failure, and that
+# separation is deliberate: it briefly did both, and since this job always sets
+# it, the transport-level skip was unreachable wherever the tests actually run.
+# A single jspm 503 or DNS blip at 04:20 UTC would then have redded the job and
+# filed the issue below, which is precisely the cry-wolf failure the paragraph
+# above argues against.
+#
+# A permanently skipping test still must not pass for a healthy one, so the run
+# is scanned for skips and annotates a warning instead. That is visible in the
+# run summary without waking anyone for an outage. `WEBJS_FAIL_ON_SKIP=1`
+# promotes a skip to a failure when you want to force the question by hand.
 #
 # There is deliberately no `pull_request` trigger, so this can never become a
 # required check and can never block a merge.
@@ -61,12 +69,30 @@ jobs:
           cache: npm
       - run: npm ci
       - name: Run the live CDN tests
+        id: live
         env:
           WEBJS_REQUIRE_NETWORK: '1'
         run: |
+          set -o pipefail
           node --test \
             packages/server/test/vendor/jspm-cdn.live.test.js \
-            test/vendor-cli/vendor-pin.live.test.mjs
+            test/vendor-cli/vendor-pin.live.test.mjs 2>&1 | tee live.log
+
+      - name: Warn if a live check only skipped
+        # Runs even when the step above failed, so a partial skip is still
+        # reported. A skip means jspm could not answer, which is upstream's
+        # problem, not a regression; it is surfaced rather than escalated.
+        if: always()
+        run: |
+          set -euo pipefail
+          skipped=$(grep -c '^# SKIP\|^ℹ skipped' live.log 2>/dev/null || true)
+          if grep -q 'SKIP ' live.log 2>/dev/null; then
+            echo "::warning title=Live jspm check skipped::jspm.io could not answer at least one check. \
+          Not a regression, but if this repeats for days the live coverage has stopped running. \
+          Re-run with WEBJS_FAIL_ON_SKIP=1 to force it to fail instead."
+            grep 'SKIP ' live.log || true
+          fi
+          echo "skip markers: ${skipped}"
 
       - name: Report a failure on the tracking issue
         if: failure()

packages/server/test/vendor/jspm-cdn.live.test.js

@@ -13,15 +13,26 @@
  * whole job is to talk to jspm, and a double can only ever return what this
  * repo already believes about the API. So these two assertions stay real, and
  * `.github/workflows/vendor-cdn.yml` runs them nightly with
- * `WEBJS_REQUIRE_NETWORK=1`, which ALSO turns a skip into a failure. Without
- * that, a permanently skipping test is indistinguishable from a passing one.
+ * `WEBJS_REQUIRE_NETWORK=1`, and surfaces any skip as a warning annotation, so
+ * a permanently skipping test is visible rather than indistinguishable from a
+ * passing one.
  *
  * Upstream trouble skips rather than reds, judged at the transport: a throw, a
  * 5xx, or a 429 is jspm having a bad moment. A 4xx does not skip, because by
  * then a ground-truth call has just succeeded against the same fixture, so
  * upstream is demonstrably healthy and a 4xx means OUR request is malformed.
  * That distinction is #1219's, and it is the reason this file can be run
  * nightly without becoming a source of false alarms.
+ *
+ * That only holds if the skip is REACHABLE where the file runs. It briefly was
+ * not: `WEBJS_REQUIRE_NETWORK` both selected these files and promoted every
+ * skip to a failure, and the nightly always sets it, so the transport
+ * distinction had no effect anywhere automated and a single 503 at 04:20 UTC
+ * would have filed a bug issue. The two concerns are separate variables now.
+ * `WEBJS_REQUIRE_NETWORK` selects the files and lifts the deny;
+ * `WEBJS_FAIL_ON_SKIP` promotes a skip, and the nightly does NOT set it. The
+ * nightly instead reports skips as a warning annotation, so a permanently
+ * skipping test is visible without waking anyone for an outage.
  */
 import { test } from 'node:test';
 import assert from 'node:assert/strict';
@@ -37,18 +48,22 @@ const GENERATE_BODY = (install) => JSON.stringify({
  * Build a loud skip for one fixture.
  *
  * Loud on purpose: a silent skip is how a real regression hides, so the reason
- * and the fixture are always named. Under `WEBJS_REQUIRE_NETWORK` the skip
- * becomes a FAILURE instead, which is what makes the nightly job able to tell
- * "jspm changed under us" from "everything is fine". A normal run is
- * unaffected, since the runners exclude this file entirely.
+ * and the fixture are always named, and the nightly turns any skip into a
+ * warning annotation.
+ *
+ * `WEBJS_FAIL_ON_SKIP` promotes it to a failure. Deliberately NOT the same
+ * variable that selects this file, and deliberately not set by the nightly:
+ * upstream being down is not a regression, and a job that reds on it is a job
+ * whose reds get ignored. Set it by hand when you want to know that the check
+ * genuinely ran.
  *
  * @param {import('node:test').TestContext} t
  * @param {string} fixture
  */
 function skipper(t, fixture) {
   return (reason) => {
     const first = String(reason).split('\n')[0];
-    if (process.env.WEBJS_REQUIRE_NETWORK) {
+    if (process.env.WEBJS_FAIL_ON_SKIP) {
       assert.fail(`live jspm check could not run (${fixture}): ${first}`);
     }
     console.warn(`[jspm-cdn.live] SKIP ${fixture} (${first})`);

test/fixtures/deny-live-hosts.mjs

@@ -77,6 +77,15 @@ export function denyLiveHosts(realFetch, onDenied) {
   };
 }
 
+/**
+ * Set on `globalThis` when the self-install below has run, so a guard can
+ * prove the preload actually took effect in a process rather than only that a
+ * runner's source mentions it. An inverted or dropped self-install is
+ * otherwise invisible: every unit test of `denyLiveHosts` keeps passing while
+ * the required job goes back to reaching jspm.
+ */
+export const DENY_INSTALLED_FLAG = '__webjsDenyLiveHostsInstalled';
+
 if (!process.env.WEBJS_REQUIRE_NETWORK) {
   /** @type {Set<string>} */
   const seen = new Set();
@@ -92,4 +101,5 @@ if (!process.env.WEBJS_REQUIRE_NETWORK) {
     seen.add(key);
     process.stderr.write(`[deny-live-hosts] refused ${key}\n`);
   }));
+  /** @type {any} */ (globalThis)[DENY_INSTALLED_FLAG] = true;
 }

test/fixtures/jspm-double.mjs

@@ -230,16 +230,23 @@ export async function withJspmDouble(opts, body) {
   const original = globalThis.fetch;
   globalThis.fetch = /** @type {any} */ (double);
   clearVendorCache();
+  let result;
   try {
-    return await body(double);
+    result = await body(double);
   } finally {
     globalThis.fetch = original;
     clearVendorCache();
-    if (double.unexpected.length) {
-      throw new Error(
-        `the jspm double was asked for ${double.unexpected.length} request(s) it does not serve:\n  ` +
-        `${double.unexpected.join('\n  ')}`,
-      );
-    }
   }
+  // Deliberately OUTSIDE the finally. Throwing from a finally REPLACES an
+  // in-flight error, and a refused request usually travels with the assertion
+  // it broke, so raising it there would discard the message that explains
+  // what actually went wrong. Restoring is what the finally is for; reporting
+  // happens only on the success path, where nothing is being displaced.
+  if (double.unexpected.length) {
+    throw new Error(
+      `the jspm double was asked for ${double.unexpected.length} request(s) it does not serve:\n  ` +
+      `${double.unexpected.join('\n  ')}`,
+    );
+  }
+  return result;
 }

test/repo-health/live-cdn-callers.test.mjs

@@ -31,7 +31,10 @@ import { readdirSync, readFileSync } from 'node:fs';
 import { join, resolve, dirname, sep } from 'node:path';
 import { fileURLToPath } from 'node:url';
 
-import { denyLiveHosts, DENIED_HOSTS } from '../fixtures/deny-live-hosts.mjs';
+import { spawnSync } from 'node:child_process';
+import { pathToFileURL } from 'node:url';
+
+import { denyLiveHosts, DENIED_HOSTS, DENY_INSTALLED_FLAG } from '../fixtures/deny-live-hosts.mjs';
 
 const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..');
 
@@ -144,6 +147,37 @@ test('both runners install the deny and skip live files, unless the network is r
     "the preload must come AFTER bun's `test` subcommand");
 });
 
+test('the preload actually arms the deny in a real process', () => {
+  // Everything else here checks `denyLiveHosts` as a pure function or greps a
+  // runner's source, and neither notices if the self-install at the bottom of
+  // the fixture is inverted or deleted: the unit tests stay green while the
+  // required job goes back to reaching jspm. That is the same class of defect
+  // three earlier review rounds found in this guard, so prove the install by
+  // running it.
+  //
+  // Spawned rather than asserted on this process, so the check does not depend
+  // on how THIS file was launched, and so both branches of the env switch can
+  // be exercised in one test.
+  const fixture = pathToFileURL(join(ROOT, 'test/fixtures/deny-live-hosts.mjs')).href;
+  const probe = 'const r = await fetch("https://api.jspm.io/generate", { method: "POST" });'
+    + `console.log(JSON.stringify({ status: r.status, armed: Boolean(globalThis[${JSON.stringify(DENY_INSTALLED_FLAG)}]) }));`;
+
+  const run = (env) => {
+    const r = spawnSync(process.execPath, ['--import', fixture, '--input-type=module', '-e', probe],
+      { encoding: 'utf8', env: { ...process.env, ...env }, timeout: 30_000 });
+    return JSON.parse((r.stdout || '{}').trim() || '{}');
+  };
+
+  const denied = run({ WEBJS_REQUIRE_NETWORK: '' });
+  assert.equal(denied.armed, true, 'the preload must install itself by default');
+  assert.equal(denied.status, 503, 'a jspm call in a preloaded process must be denied, not sent');
+
+  // The opt-out has to actually opt out, or the nightly could never reach the
+  // real CDN. Asserting the flag rather than a live status keeps this offline.
+  const allowed = run({ WEBJS_REQUIRE_NETWORK: '1' });
+  assert.notEqual(allowed.armed, true, 'WEBJS_REQUIRE_NETWORK must lift the deny');
+});
+
 test('every allowlisted live caller is a *.live.test.* file that exists', () => {
   for (const entry of LIVE_CALLERS) {
     assert.ok(entry.file.includes(LIVE_MARKER),

test/vendor-cli/vendor-pin.live.test.mjs

@@ -54,16 +54,19 @@ test('pin resolves picocolors against the real CDN and hashes the bytes', async
       `import pico from 'picocolors';\nexport default () => pico.green('ok');`);
 
     const { code, stdout, stderr } = await runCli(['vendor', 'pin'], dir);
-    if (code !== 0) {
-      // A failed pin here is upstream trouble far more often than a regression,
-      // and the CLI already names the reason. Under WEBJS_REQUIRE_NETWORK the
-      // nightly wants to know, so fail loudly there instead of skipping.
-      const why = `exit ${code}: ${(stderr || stdout).split('\n').filter(Boolean).slice(-1)[0] || 'no output'}`;
-      if (process.env.WEBJS_REQUIRE_NETWORK) {
+    // Upstream trouble is not a regression. `WEBJS_FAIL_ON_SKIP` promotes it,
+    // and is deliberately NOT the variable that selects this file nor one the
+    // nightly sets, so a jspm outage does not red a scheduled run.
+    const skip = (why) => {
+      if (process.env.WEBJS_FAIL_ON_SKIP) {
         assert.fail(`live \`webjs vendor pin\` could not run (${why})`);
       }
       console.warn(`[vendor-pin.live] SKIP live pin (${why})`);
       t.skip('jspm.io was not in a state that can answer a pin');
+    };
+
+    if (code !== 0) {
+      skip(`exit ${code}: ${(stderr || stdout).split('\n').filter(Boolean).slice(-1)[0] || 'no output'}`);
       return;
     }
 
@@ -74,6 +77,15 @@ test('pin resolves picocolors against the real CDN and hashes the bytes', async
     // The offline double mints this tail, so its absence is what proves this
     // run really went to the network rather than picking up a stray preload.
     assert.doesNotMatch(url, /\/double\.js$/, 'this test must NOT be running against the double');
+
+    // A hiccup on the BUNDLE GET is the wider version of the same trap: pin
+    // exits 0 with the entry pinned and no hash, and the CLI says so, so
+    // asserting the hash outright would hard-fail on an outage the exit code
+    // already forgave.
+    if (!parsed.integrity || !parsed.integrity[url]) {
+      skip('jspm.io resolved the package but would not serve its bundle to hash');
+      return;
+    }
     assert.match(parsed.integrity[url], /^sha384-/,
       'the bundle behind the resolved url must have been fetched and hashed');
   } finally {