fix: keep a live jspm outage from redding the required CI job - #1297
Conversation
6544020 to
f78a9d2
Compare
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff. The split-by-filename idea holds up and the offline double is the right shape, but three of the four things this branch claims about itself are not true of what shipped, and one of them is the guard that is supposed to stop the whole class from coming back.
The one that matters most is the repo-health guard. I ran its own logic against the pre-PR tree: it flags neither of the two files this PR converts. vendor.test.js had a live fetch('https://api.jspm.io/generate') and four unwrapped vendor entry points, and the file-level controlsFetch exemption waved all of it through because the file also happened to contain withMockedFetch. So the guard has an empty counterfactual, and a future test that drops withJspmDouble inside that same file goes back to live and stays green. That needs per-call scoping, not a file-level flag.
The fetchIntegrity timeout is real, but downloadBundle twenty lines above it is the same unguarded shape on the same CLI run, so --download keeps exactly the behaviour the comment says was removed. Both the source comment and the docs page overstate it.
And the framework-dev claim that no required check depends on a third party is not true yet: the app-boot tests still resolve vendors live on their cold first request, transitively, where neither the guard nor the filename rule can see them. Worth stating the residual exposure rather than a rule the tree does not satisfy.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta round over 8c735e0e, the commit that rewrote the guard. Three holes in the rewrite itself, which is the right place to have found them.
The one worth the round: the masker had no regex-literal awareness, and this suite is full of patterns like /rel=["']modulepreload["']/. Those quote characters read as string delimiters and desync the mask for the rest of the file, so every live call below such a line disappears from the scan. Eighteen files carry the shape, including the app-boot tests. Injecting a live fetch just after the regex on blog-smoke.test.js:115 left the guard green; the identical line above it redded. So the guard could report clean because it had gone blind, which is the same failure the commit under review was written to fix, one level down.
Also: the host lookup used a fixed character window that crossed statements, so fetch(localUrl) near an assertion naming a jspm url read as live; and the allowlist's live flag was read as merely falsy, so an entry missing the key skipped the *.live.test.* requirement and still took a whole-file exemption.
All three fixed in 2e810f65, each with the counterfactual that reproduces it. The lesson from the previous round was that this guard's own tests were the thing going unchecked, so the masker now has its own.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta round over 2e810f65, the commit that added regex awareness to the scan's masker. It introduced new blindness on a different set of files: regexCanStartHere() returns true after <, so the / in a closing tag inside a nested html`...` template opens a pseudo-regex that swallows the template's closing backtick and flips literal parity for the rest of the file. Thirteen files newly invisible, including render-client.test.js and the directive and slot tests. Reproduced: a live fetch appended to render-client.test.js is invisible at 2e810f65 and caught at its parent.
That is three rounds, three different blind spots, each fix opening the next. I am not writing a fourth heuristic. Deciding whether a / starts a regex means lexing JavaScript, and a hand-rolled lexer facing nested template literals full of markup will keep being wrong. The scan also cannot see the callers that matter most here: the app-boot tests reach jspm through resolveVendorImports with no fetch( anywhere in their own source.
So the scan is deleted and the runners preload a runtime deny instead (34be87a9). It answers 503 for jspm.io and registry.npmjs.org unless WEBJS_REQUIRE_NETWORK is set. No parsing, no blind spots, and it covers the transitive callers. A test that depends on a third party now fails on every run rather than only during an outage. Net 215 lines lighter.
The division counterfactual you flagged as non-discriminating went with it, which was the right call on the merits either way: it was asserting a heuristic that no longer exists. The detached JSDoc block went with the file too.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta round over 34be87a9, the commit that swapped the static scan for a runtime deny. Three findings, one of them a real break I had already caught by running the matrix, and two doc claims the swap invalidated.
The break: the deny flag went BEFORE bun's subcommand, and bun --preload X test <file> stops treating test as the subcommand and resolves it as the package.json script of that name, which here is the whole Node suite. Every matrix file spawned that, hit the 120s per-file timeout, and failed. Measured 0 pass / 23 fail before, 23 pass / 0 fail after. Worth noting the wiring guard did NOT catch it, since it only asserted the runner source mentions the fixture, which is equally true of the broken argv; it asserts the order now, and reverting the order reds it.
The two doc claims: the paragraph listing which required tests reach jspm predated the deny and still said they do (under the deny those calls get a 503 without leaving the process, and the point worth making is that they pass anyway because the resolve fails open); and both the fixture header and framework-dev claimed the deny has no blind spots while the very next bullet described one. Scoped to the test process now, with the spawned-child case called out.
All three fixed in 96e1103e.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta round over 96e1103e. Two stale claims, both copies of things corrected elsewhere in this branch, and no functional defects.
The guard test's header still said the deny covers every caller, which is the absolute the previous commit rewrote in the fixture and in framework-dev, and it was sitting in the very file that commit edited. Its own allowlist contradicts it: the second entry is a spawn-based caller the deny does not reach. And the e2e preload comment still said Node and Bun each ignore the other's flag, which the same branch had already measured and corrected in framework-dev, so the code comment and the doc that describes it disagreed.
Both scoped in e454972c. I swept for other copies of each phrase; the two that remain are the correctly-qualified ones.
Worth recording from this round's verification, since it is the strongest evidence the design is doing what it claims: a recording preload chained ahead of deny-live-hosts.mjs across the whole node suite, 3907 tests, logged ZERO external hosts outside the denied set. That is the narrowed no-blind-spots claim confirmed empirically rather than argued.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Delta round over e454972c, the doc-scoping commit. Clean, nothing to fix.
That is the first round in this cycle that produced no changes, so the branch gets one more read of the WHOLE diff rather than of a fragment, which is the first time anything has looked at the finished shape end to end. The four delta rounds were each scoped to one commit by design, and the piece under review was replaced wholesale partway through, so a full pass is worth having before this comes out of draft.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff, the first read of the finished shape rather than of one commit. The mechanism holds up under exercise, and three things do not.
The one worth the round 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 #1219 built was unreachable everywhere automated. One jspm 503 or DNS blip would have redded the job and filed a bug issue, which is precisely the cry-wolf outcome the workflow's own header argues against. Two variables now: one selects, one promotes, and the nightly sets only the first; a skip becomes a warning annotation. Measured with jspm unreachable on the nightly path: 3 skipped, 0 failed. With WEBJS_FAIL_ON_SKIP=1: 3 failed.
The live pin test carried a wider version of the same trap, since a hiccup on the bundle GET leaves pin exiting 0 with no hash, so the integrity assertion hard-failed on an outage the exit code had already forgiven.
Two smaller ones: withJspmDouble threw from a finally, which replaces an in-flight error and so discarded the assertion that actually failed; and the guard file claimed to assert both enforcement mechanisms while never checking the deny was armed in a running process, which is the same guard-is-the-broken-part shape as the earlier rounds. It spawns a preloaded child now and asserts both branches; inverting the self-install reds it.
All three fixed in 4cd07cef.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Check over ba41e983. Two, both in the machinery rather than the change.
The skip counter announced a jspm skip for runs where the tests never ran. Dropping 2>/dev/null turned a missing live.log into an empty string rather than 0, and the step is if: always(), so a failed checkout or a cancellation reported an outage. Verified across all three cases now: missing file gives no warning, a skipping run gives 3, a passing run gives 0.
And the reason I committed with the temp-file probe was wrong. Bun ignores unrecognised flags, so the Node-only --input-type was silently dropped and the old -e form ran fine there. The change is still right, for the honest reason that an argv meaning the same thing on both runtimes beats one that happens to be inert on one of them. Reasoning from the Node side is exactly what framework-dev warns about, and I did it in the commit that cites it.
Both fixed in 8bbd4cd1.
Calling the cycle here. Seven rounds, twenty-five findings, all fixed and none deferred. The chain has used its budget, and what the last three rounds found were defects in the guard and the workflow rather than in the change under test, which is the signal to stop reviewing and let the owner decide.
8bbd4cd to
a486b12
Compare
The vendor suites resolve against the live jspm CDN, so a jspm outage reds a pull request that has nothing to do with vendoring. This is the fixture that replaces the network. It models jspm rather than merely answering it. vendor.js's fallback ladder keys off the exact failure semantics: a 5xx or 429 is transient and retries per package, a 4xx is permanent and probes per install so the resolvable ones survive, and an unresolvable install fails the WHOLE batch. A double that answered every call with a 200 would leave all of that untested while looking green. Refusals are recorded rather than thrown, because every fetch caller in vendor.js swallows a throw and would degrade to "resolved nothing" instead of failing. The install-string parse moves into a shared module so the e2e stub and the double cannot drift apart on the subpath case.
Every runCli spawn now carries the preload, so the child answers its own jspm calls. Verified with the network namespace cut: all ten pass under unshare -rn, where six of them hard-failed before. The flag is picked from the runtime rather than hardcoded, because the spawn uses process.execPath and under bun test that is the bun binary, which ignores --import and NODE_OPTIONS both. Node gets a file:// URL because the spawn sets cwd to the temp app, so a relative path would resolve against the wrong place. The armed marker is asserted inside runCli rather than in one test, since the CLI's own output is identical whether the preload is there or not. Unplugging the flag reds all ten. The /double.js tail assertion is the second half of that proof, because the existing ga.jspm.io prefix check is equally true of the real CDN.
Thirteen tests move off the live CDN, two lose a gate they never needed (an empty install list short-circuits before any call, and the audit test installs its own throwing fetch), and two that were never gated at all stop reaching registry.npmjs.org. The whole vendor surface now runs with the network namespace cut: 170 tests, 0 failures. Three become stronger rather than merely offline. The cache test could only compare two results against a live CDN, which stays true even if a second round trip fired; it counts calls now. The partial-success test pins the exact permanent-failure trace (unified 401, two probes, the survivor served from the probe cache). And pinAll's flattened-transitive path (#446) gains its first coverage at any layer, because every existing pinAll test resolves picocolors, which has no dependencies for a live CDN to hoist. The live parity test moves to jspm-cdn.live.test.js, where it keeps its transport-level skip and gains a companion that re-anchors the premise the fallback ladder rests on: jspm fails the WHOLE batch, permanently, when one install is unresolvable. A double cannot vouch for that, since the double is built from the same belief.
A *.live.test.* file is the only place allowed to reach a third party, and run-node-tests.js and run-bun-tests.js both drop those unless WEBJS_REQUIRE_NETWORK is set. Measured: the node runner selects 364 files normally and 366 with the variable, the bun matrix 0 live and 2. This is what the NETWORK_OK gate could never be. That gate was opt-OUT, so CI, which never set it, always ran live; it was convention rather than a rule the runner could enforce; and it missed two registry.npmjs.org callers entirely. Leaving the parity test gated in place would not have fixed anything either, since after #1219 it still reds on a 4xx, and a WAF 403 or a moved route is exactly the shape #1149 hit. ci.yml is untouched on purpose. Eleven jobs share its trigger block and #1135 and #1257 are already editing it, so the filter belongs in the runners. The vendor suite rejoins the Bun matrix, which the exclusion had blocked as network-bound. All five files pass, and that is worth having: the double is a globalThis.fetch swap, which is where the two runtimes are most likely to diverge. A live pin test keeps one real run of the CLI command, so what a user actually types is still exercised against the real CDN nightly. The preload comment is corrected while here. Node rejects --preload outright, but Bun does accept --import as an alias, so the two are not symmetric the way the first draft claimed. Selecting per runtime is still right, since it keeps this from depending on Bun continuing to accept a Node spelling, and Bun does ignore NODE_OPTIONS entirely, which is the part that rules out an env var.
…nest The nightly runs the live files with WEBJS_REQUIRE_NETWORK, which both selects them and turns their upstream-trouble skip into a failure. That second half is the point: a permanently skipping test is indistinguishable from a passing one, which is how live coverage rots into decoration. It has no pull_request trigger, so it can never become a required check. A failure opens or comments on one fixed-title tracking issue rather than creating a new one per night, because GitHub notifies only the workflow file's last committer about a failed scheduled run, and a nightly nobody watches is a placebo. The double gets its own contract test, in the spirit of the one #1229's fixture carries. A double that answers the wrong shape makes the vendor tests pass for the wrong reason, which is strictly worse than the live dependency it replaced. It pins the url shape derivePinParts has to be able to read, the whole-batch 401, the recorded refusal, and the live generateCalls getter (which caught a real bug in the double while this was being written: Object.assign copies a getter's value, so every round-trip count read zero). The policy guard asserts the PROPERTY rather than a spelling. Counting occurrences of some marker constant would certify nothing, since a new test can call fetch without it, and would red on a rename. So it looks for a third-party host inside a fetch call and for the vendor entry points that reach one internally, exempting any file that installs a fetch it owns. Verified: dropping a live fetch into an ordinary test file reds it.
fetchIntegrity was the only outbound call in vendor.js with no AbortSignal, while jspmCall, fetchNpmJson, and fetchLiveIntegrity all carry one. Default mode pinAll runs it once per resolved URL, and a CLI run has no ambient deadline, so a CDN that accepted the connection and then stalled held the pin open indefinitely with nothing to interrupt it. It now uses the same INTEGRITY_FETCH_TIMEOUT_MS its sibling already had, which moves up beside the other outbound timeouts rather than sitting a thousand lines below its caller. The test asserts the SIGNAL rather than a wall clock: waiting out ten real seconds would make the test the slow thing it is complaining about, and shortening the timeout would mean growing a test-only knob in shipped code. Counterfactual run: dropping the signal argument reds it. A second candidate was investigated and deliberately NOT changed. clearVendorCache does not reset lastLiveResolveFailed, which reads like an omission in the documented start-clean primitive. It is not observable: resolveVendorImports is the flag's only reader and resets it on entry, and the pinned short-circuit ahead of that returns ok:true outright, so a value left behind by an earlier pinAll can never reach anything. The first version of this commit changed it anyway, with a test that passed just as happily once the fix was removed. A comment now says why the line is absent, so the next reader does not re-derive the same wrong conclusion.
The rule is now carried by a filename that both test runners enforce, so it needs one place that says what the filename means, why a nightly job exists to stop a permanent skip from hiding, and why the workflow deliberately has no pull_request trigger. Written down beside the #1228 note, which is the other half of the same problem. Also corrects the preload claim in that #1228 note while here. It says Node and Bun each ignore the other's flag spelling, which is only half true: node --preload is a hard bad-option error, but bun --import works today as an alias. The part that actually rules out an env var is that Bun ignores NODE_OPTIONS, so that is what the note says now. The no-build docs page said the LIVE hashing is bounded and fail-open, which was true and incomplete: pin-time hashing had no timeout at all until this branch. It now describes both.
…inating Four things this branch claimed about itself were not true of what shipped. The guard was the serious one. It was file-level, so a single withMockedFetch anywhere in a file exempted every live call in it. Measured against the pre-PR tree it reported ZERO offenders for vendor.test.js, the file carrying a live fetch(api.jspm.io) and four unwrapped vendor entry points, which is precisely the regression it exists to prevent. Its counterfactual passed only because the throwaway offender I tested with had no mock at all, a shape nobody writes. The scan is per call now, and lives in test/fixtures/live-caller-scan.mjs so it can be run against inline fixtures rather than only against today's tree. A call is exempt inside a withJspmDouble or withMockedFetch body, or behind an explicit `// live-cdn-ok: <reason>` marker at the site. Eleven such markers go in, each naming why that call provably returns before dialling; wrapping those in a double to satisfy a checker would have been churn that teaches the wrong thing. Against the pre-PR tree the rewritten scan now reports 24 offenders, starting with the live fetch. It has one blind spot and the header says so: it cannot see a spawned child, so vendor-cli.test.mjs reports zero both before and after. What covers that file is the armed-marker assertion inside its own runCli. downloadBundle was the other real one. It sits twenty lines above the call this branch bounded and is the same unguarded shape on the same CLI run, so `webjs vendor pin --download` kept exactly the behaviour the comment said had been removed. Both are bounded now and the test covers both modes. The two doc claims are corrected. The no-build page described only default-mode pinning, and framework-dev said no required check may depend on a third party, which is not true of the tree: the app-boot tests still resolve vendors live on cold boot. They fail OPEN, which is the property that actually matters, so that is what it says now. Measured with jspm and registry.npmjs.org forced to 503: the only failures across npm test are three that fail identically on a clean origin/main baseline.
Three holes in the guard the previous commit rewrote, all found by reviewing that commit rather than the branch. The masker had no regex-literal awareness, and this suite is full of patterns like /rel=["']modulepreload["']/ whose quote characters it read as string delimiters. That desyncs the mask for the REST OF THE FILE, so every live call below such a line vanished from the scan. Eighteen test files carry the shape, including the app-boot tests. Demonstrated end to end: injecting a live fetch after the regex on blog-smoke.test.js:115 left the guard fully green, while the identical line above it redded. So the scan could report clean because it had gone blind, which is the same class of hole the previous commit was written to close. Telling a regex from a division needs the preceding token, so the usual heuristic goes in, with character classes and escapes handled and division still reading as division. The host lookup used a 200-character raw window, which crosses statements. A `fetch(localUrl)` followed two lines later by an assertion naming a jspm url read as a live call, and so did a comment mentioning one. It reads the call's actual argument list now, via the same paren matcher the guard ranges use. And the allowlist's `live` flag was read as merely falsy, so an entry added without the key skipped the *.live.test.* requirement while still collecting a whole-file exemption. It must be an explicit boolean now. Each of the three ships with the counterfactual that reproduces it, since the previous round's lesson was that this guard's own tests were the thing not being checked.
Three review rounds found three different ways the static scan went blind, and each fix opened a new hole. A file-level exemption excused every live call in any file containing one withMockedFetch. Adding regex-literal awareness then desynced the mask on nested html templates, because the slash in a closing tag read as a regex opener and swallowed the template's backtick, blinding thirteen more files. Measured: 30 files blind before that fix, 15 after, 13 of them new. The fourth heuristic would not have been right either. Deciding whether a slash opens a regex means lexing JavaScript, and a hand-rolled lexer facing nested template literals full of markup is going to keep being wrong. A scan also cannot see the callers that matter most: the app-boot tests reach jspm through resolveVendorImports with no fetch and no vendor entry point in their source. So the runners preload a deny instead. It answers 503 for jspm.io and registry.npmjs.org unless WEBJS_REQUIRE_NETWORK is set, needs no parsing, has no blind spots, and covers the transitive callers. A test that depends on a third party now fails on every run rather than only during an outage, so it surfaces the day it is written. 503 rather than a throw because every fetch caller in vendor.js catches: a rejection would be swallowed, while 503 is the shape those sites already classify as transient. Verified: npm test with the deny installed is 3915 tests and the same five failures that fail identically on a clean origin/main baseline. The deny prints what it refuses, which is how the app-boot tests were confirmed to reach jspm. The eleven live-cdn-ok markers stay. They are accurate notes about why those calls return before dialling, and they cost nothing now that no scanner reads them.
… claims The deny flag went before the subcommand, and `bun --preload X test <file>` stops treating `test` as bun's subcommand: it resolves it as the package.json script of that name, which here is the whole Node suite. Every matrix file then spawned that, hit the 120s per-file timeout, and failed. Measured before the fix: 0 pass, 23 genuine fail. After: 23 pass, 0 fail. The wiring guard did not catch it, because it only asserted the runner source mentions the fixture, which is true of the broken argv too. It asserts the order now, with the failure mode written down, since a flag-order mistake here surfaces as a timeout that looks nothing like its cause. Two doc claims are corrected. The framework-dev paragraph describing which required tests reach jspm predates the deny and said they still do; under the deny those calls get a 503 without leaving the process, and the point worth making is that they pass anyway because the resolve fails open. And both the fixture header and framework-dev claimed the deny has no blind spots while the next bullet described one: a spawned child starts with its own globalThis, so the claim is scoped to the test process now.
Two stale copies of claims corrected elsewhere in this branch. The guard test's header still said the deny "covers every caller", the same absolute the previous commit rewrote in the fixture and in framework-dev, and in the very file that commit edited. It is contradicted by the spawned-child bullet a few lines below in framework-dev and by the guard's own allowlist, whose second entry is a spawn-based caller. And the e2e preload comment still said Node and Bun each ignore the other's flag. Measured on bun 1.3.14 and node v26.1.0: node --preload is a hard bad option error, but bun --import loads the module as an alias. framework-dev's #1228 paragraph documents that comment and already carried the correction, so the two disagreed. Independent confirmation of the narrowed claim, from the review: a recording preload chained ahead of deny-live-hosts over the whole node suite, 3907 tests, logged zero external hosts outside the denied set.
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.
The eleven live-cdn-ok comments were written for the static scanner, which reads them as a per-site exemption. That scanner is gone, so nothing reads them and the prefix implies a mechanism that no longer exists. The reasons are accurate and worth keeping, so only the prefix goes.
…n markers
Six from the fix-check, one of them badly wrong.
The armed-in-process guard I added last commit ran its probe twice, and the
second run lifted the deny. The probe fetches, so that was a real POST to
api.jspm.io on every npm test, from the one file whose entire job is to stop a
required check reaching a third party, and invisible to everything else because
the static scan that would have caught it was deleted earlier in this branch.
The two branches run different probes now: the first fetches, because the point
is that the deny answers it in-process; the second only reads the install flag.
Verified with the network namespace cut.
The same probe swallowed every child failure into `{}`, which satisfies the
`notEqual(armed, true)` assertion, so that half passed unconditionally whenever
the probe had not run at all. It asserts the exit status and an explicit ok
sentinel now. It also used `--input-type`, which is Node-only, while spawning
process.execPath, which is bun under the matrix; the probe is a temp file and
the preload flag is runtime-selected, so it runs on both.
The nightly's skip counter matched neither thing the run emits. Rather than
guess again at which reporter node --test picks, which depends on the version
and on whether stdout is a TTY, it counts the `] SKIP ` markers the two live
files print themselves. Measured: 3 on a skipping run, 0 on a passing one.
And three more copies of the promotion claim the previous commit invalidated,
in framework-dev, the node runner, and the live pin test's own header.
Two from the second fix-check. Dropping `2>/dev/null` from the skip counter turned a missing live.log into an empty string rather than 0, and an inequality against "0" is then true. The step is `if: always()`, so it announced a jspm skip precisely in the runs that never produced a log: a failed checkout, a failed npm ci, a cancellation. It now returns early when the file is absent and defaults the count otherwise. Verified across all three cases: missing file gives no warning, a skipping run gives 3, a passing run gives 0. And the reason committed with the temp-file probe was wrong. Bun ignores flags it does not recognise, so the Node-only --input-type was silently dropped and the old -e form ran fine there; nothing was broken. Moving to a file is still right, because an argv that means the same thing on both runtimes beats one that happens to be inert on one of them, and that is the honest reason. It is the same reason-from-the-Node-side mistake framework-dev warns about, made in the commit that cites it.
Two from the second fix-check. Dropping `2>/dev/null` from the skip counter turned a missing live.log into an empty string rather than 0, and an inequality against "0" is then true. The step is `if: always()`, so it announced a jspm skip precisely in the runs that never produced a log: a failed checkout, a failed npm ci, a cancellation. It now returns early when the file is absent and defaults the count otherwise. Verified across all three cases: missing file gives no warning, a skipping run gives 3, a passing run gives 0. And the reason committed with the temp-file probe was wrong. Bun ignores flags it does not recognise, so the Node-only --input-type was silently dropped and the old -e form ran fine there; nothing was broken. Moving to a file is still right, because an argv that means the same thing on both runtimes beats one that happens to be inert on one of them, and that is the honest reason. It is the same reason-from-the-Node-side mistake framework-dev warns about, made in the commit that cites it.
… allows A full comparison against importmap-rails, which sets no timeout anywhere and inherits Ruby's 60s Net::HTTP open and read defaults on every call it makes. The npm registry calls now take 60s. They are reached only by audit, outdated, and update, all CLI commands, and importmap-rails makes these same two requests with the same 60s effective bound, so 10s was a plain divergence. The generate call takes a per-caller budget instead of a constant, because it is the one call reached from BOTH sides. importmap-rails only ever resolves from the CLI, since its importmap is a static config file, so its flat 60s has no request path to slow down. Ours does: an unpinned app resolves on its cold first request. A pin passes 60s; a live resolve keeps 10s, so one stalled CDN cannot hold a request open for a minute. Two internal fallback call sites inside jspmGenerate were nearly missed, which would have dropped a pin back to 10s the moment the unified call failed and the per-install ladder took over. All five now thread the budget, and a grep for a call that omits it returns zero. The remaining 10s, on the warmup live-integrity pass, is deliberate and has no Rails counterpart: it gates readiness and is separately capped by a 15s total budget across every URL.
2ef2227 to
1c54ec8
Compare
1 participant
Closes #1150
A jspm outage should not be able to red a pull request that has nothing to do with vendoring, and today it can: the required
Unit + integration (node --test)job resolves vendors against the live CDN. PR #1149, a five-file documentation change, failed on the#448gitignore-healing test during a jspm 401 window and passed on a re-run of the identical commit.What changed
The test surface is split by FILENAME, so the runners can enforce the rule instead of leaving it to discipline.
test/fixtures/deny-live-hosts.mjs, which answers 503 forapi.jspm.io,ga.jspm.io, andregistry.npmjs.orgunlessWEBJS_REQUIRE_NETWORK=1. That covers every caller, including the app-boot tests that reach jspm transitively with nofetch(anywhere in their own source.*.live.test.{js,mjs}is the only shape allowed to touch a third party, and both runners drop those files unless the same variable is set. Measured: the node runner selects 364 files normally and 366 with it.test/fixtures/jspm-double.mjs, a new offline double that models jspm rather than merely answering it. A 5xx or 429 is transient and retries per package, a 4xx probes per install, and an unresolvable install fails the WHOLE batch, which is the premisejspmGenerate's fallback ladder is built on..github/workflows/vendor-cdn.ymlruns the live files nightly with that variable set. It does NOT promote their upstream-trouble skip into a failure, because a jspm outage is not a regression and a job that reds on one is a job whose reds get ignored.WEBJS_FAIL_ON_SKIP=1does that, by hand; the nightly surfaces a skip as a warning annotation instead. Measured with jspm unreachable on the nightly path: 3 skipped, 0 failed. With the promotion on: 3 failed.The old
NETWORK_OK/WEBJS_SKIP_NETWORK_TESTSgate is deleted. It was opt-OUT, so CI (which never set it) always ran live; it was convention rather than something a runner could check; and it missed tworegistry.npmjs.orgcallers entirely.Leaving the one live parity test gated in place would not have fixed the problem. After #1219 it skips on a throw, a 5xx, or a 429, but still reds on a 4xx, and a WAF 403 or a moved route is exactly the shape #1149 hit.
.github/workflows/ci.ymlis deliberately untouched. Eleven jobs share its trigger block and #1135 and #1257 are already editing it.Coverage is stronger, not just offline
jspmGenerate: second call ... hits in-process cachecould only compare two results against a live CDN, which stays true even if a second round trip fired. It counts calls now.pinAll: warns by name when some installs failpins the exact permanent-failure trace: unified 401, two probes, then the survivor served from the probe cache.pinAll's flattened-transitive path (dogfood: importmap per-package resolution yields inconsistent dep graph #446) gains its first coverage at any layer. Every existingpinAlltest resolves picocolors, which has no dependencies for a live CDN to hoist.packages/server/test/vendor/jspm-cdn.live.test.jsadds a test that re-anchors the whole-batch-401 premise against the real API, which a double cannot vouch for since the double is built from that same belief.vendorexclusion had blocked as network-bound. All five files pass. That is worth having: the double is aglobalThis.fetchswap, the kind of thing the two runtimes are most likely to disagree about.One product fix
fetchIntegrityanddownloadBundle(packages/server/src/vendor.js) were the two outbound calls in the file with noAbortSignal, whilejspmCall,fetchNpmJson, andfetchLiveIntegrityall carry one.pinAllruns one of them once per resolved URL depending on mode, and a CLI run has no ambient deadline, so a CDN that accepted the connection and then stalled held the pin open indefinitely in either mode. Both are bounded now, and the test covers both.A second candidate was investigated and deliberately NOT changed:
clearVendorCachedoes not resetlastLiveResolveFailed, which reads like an omission. It is unobservable, becauseresolveVendorImportsis the flag's only reader and resets it on entry, and the pinned short-circuit above that returnsok: trueoutright. The first attempt changed it anyway with a test that passed just as happily once the fix was removed. A comment now records why the line is absent.Test plan
npm testnormally: 3914 tests, 7 pre-existing failures (see below), live files absent from the run.npm testwithapi.jspm.io,ga.jspm.io, andregistry.npmjs.orgall forced to 503: only the 3 pre-existing failures below. (unshare -rnis not the right instrument for the whole suite, since it leaves loopback down and the server tests need it; it is used for the vendor-scoped runs, which bind nothing.)WEBJS_REQUIRE_NETWORK=1 node --test <the two live files>against real jspm: 3 pass.webjs check: no new violations.Counterfactuals, each run:
runCli: all ten CLI tests red on the armed-marker assertion.AbortSignalfromfetchIntegrity: the timeout test reds.AbortSignalfromdownloadBundlealone: the--downloadhalf of the timeout test reds.vendor.test.js, starting with the livefetch('https://api.jspm.io/generate'). The first version of that guard reported zero, which is what the review caught.Object.assigna getter instead of defining it: caught a real bug in the double during development, where every round-trip count silently read zero.Pre-existing failures, proven not mine.
differential-elision(3) and the twotest/bun/listener*files fail identically on a cleanorigin/mainworktree with linked deps, a freshly builtpackages/core/dist, and a migrated blog DB.test/integration/blog-http.test.mjsfailed only until the blog DB was migrated, which the CI unit job does first.Doc surfaces
framework-dev.md: a section stating the policy once, beside the fix(server): elision-OFF boot intermittently 404s a module, leaving components inert #1228 note that covers the other half of the same problem. Also corrects that note's claim that Node and Bun each ignore the other's preload flag; measured,node --preloadis a hard error butbun --importworks as an alias, and the part that actually rules out an env var is that Bun ignoresNODE_OPTIONS.website/app/docs/no-build/page.ts: it said the LIVE hashing is bounded and fail-open, which was true and incomplete, since pin-time hashing had no timeout at all.AGENTS.md, the skill at.agents/skills/webjs/, the scaffold templates, the MCP server, the editor plugins,README.md: no public or agent-facing API surface moves. The one shipped change is an internal timeout on a call that was already documented as fail-open.Review
Six rounds, seventeen findings, all fixed. Every round is on the PR with its threads.
Round 1 (4).
downloadBundlewas the same unguardedfetchtwenty lines above the one I had bounded. Two doc claims overstated. And the live-caller guard was file-level, so onewithMockedFetchanywhere in a file exempted every live call in it: against the pre-PR tree it reported zero offenders for the file carrying the livefetch(api.jspm.io).Rounds 2 and 3 (6) were almost entirely that guard. Adding regex-literal awareness fixed one blind spot and opened another, since the
/in a closing tag inside a nestedhtml`...`template read as a regex opener and swallowed the backtick. 30 files blind before, 15 after, 13 of them new.So the guard was deleted and replaced with a runtime deny. Three rounds, three blind spots, each fix opening the next; deciding whether
/starts a regex means lexing JavaScript, and a source scan also cannot see the transitive app-boot callers. Net 215 lines lighter, and a round independently confirmed it: a recording preload chained ahead of the deny across all 3907 tests logged zero external hosts outside the denied set.Round 4 (3) caught the redesign's own break: the Bun preload flag went before the subcommand, so
bun --preload X testran thepackage.jsonscript of that name and every matrix file timed out. 0/23 before, 23/0 after. Round 5 (2) was two stale claims. Round 6, the final whole-diff pass (3), caught the worst design error in the change:WEBJS_REQUIRE_NETWORKboth selected the live files and promoted their skip to a failure, and since the nightly always sets it, one routine jspm 503 would have redded the job and filed a bug issue every time. Split into two variables.The recurring lesson, stated because it drove the design: in five of the six rounds the defect was in the guard, not the code it guards.