S10-e: browser deps off the core default path, and G-DIET tightens - #298
Merged
Conversation
…andatory The five modules that hold a value import of the browser driver each make the whole module graph unloadable the moment the package is absent — so an optional rung becomes a crashed process rather than a missing capability. Route all five, plus the TUI probe's second require, through one lazy seam that resolves from either wigolo's own tree or the data directory, and memoizes both answers. Type imports stay literal: they are erased, and keeping them is what lets the callers stay fully typed against a package that may not be on disk.
…es it Taking the driver off the default install path is only half a change; without the other half the size win is paid for in a capability the product still advertises. Warmup now installs the driver into the data directory when none resolves, and drives the browser-binary install from that same copy — resolving the CLI from a different root than the runtime resolves the module from is how an install lands beside a revision nothing will load. The binary stays installBrowser's job: the driver's own postinstall is suppressed so this cannot fetch a second copy of what the next step fetches.
npm installs optionalDependencies but not OPTIONAL PEER dependencies, so the peer/optional pair is the one manifest lever that actually removes bytes from `npm i -g wigolo`. Measured on the shipped G-DIET protocol, darwin-arm64: 698 MiB / 388 packages -> 681 MiB / 386, and the per-package diff moves exactly playwright and playwright-core with nothing added. It stays a devDependency so this repo's own suites and the studio e2e specs are unaffected, and the pinned peer range is the same revision the acquirer installs.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…dules The seam memoized the resolved module but not the in-flight resolution, so the three probes doctor runs through Promise.all each started their own import — and under a mocked module graph the first got the mock while its siblings got the real package, which is a difference the caller can see. Memoize the promise, as the two existing optional-dep loaders already do. The sync path also gets its own memo rather than sharing one: the two mechanisms agree by construction in production, so the shared memo bought nothing and cost an ordering hazard where one early on-disk probe pinned the answer for both. The dynamic specifier is spelled inline: routed through a const it is not statically analysable, which resolved the real package straight past a mock and would be dropped outright by a bundler.
…atch Both numbers are measured on the gate's own protocol minutes apart: 698 MiB with the driver on the default install path, 681 without, and the per-package diff between the two trees is exactly playwright and playwright-core. So 698 is not a model of the regression, it is the regression. 690 is the midpoint of that window. The habitual 'clean build + 3%' would have produced 701 — above the very regression the gate exists for — so an assertion pins that arithmetic rather than leaving the next reader to rediscover it. The dropped-gate composition is re-derived because this slice moved one of its inputs; the failure window narrows from 56 MiB to 45, so the decision to drop it is better supported than when it was taken.
…lidate Forcing absence through the test override answers before the memo is ever consulted, so the memo was never populated and deleting the invalidation outright left the assertion green — it red 0 of 1 under that mutation. Absence is now produced by the real resolution failing, via a module-graph mock that throws on first evaluation and succeeds on its second, with the attempt count as the outside signal: it separates 'served from the memo' from 're-resolved and happened to agree'. Both halves now red under their own mutation.
Observed in CI, not predicted: green on ubuntu and macOS, red on BOTH Windows runners with 'could not run the package manager: spawn EINVAL'. On Windows npm is a batch script, and since the fix for CVE-2024-27980 Node refuses to spawn one without a shell. A shell is not the fix — it joins arguments instead of passing a vector, and --prefix here is a path under the user's home directory, so it would break on the first space. npm runs as a JS entrypoint under the Node binary instead, exactly as installBrowser already runs the driver's own CLI. Also re-derives G-DIET from the RUNNER's 685 rather than a laptop's 681: the gate runs on one machine class, and the laptop-derived 690 left a clean build 5 MiB of room on the only machine that measures it. 693 restores the balance (8 MiB headroom, 9 MiB under the regression).
… both readings Two runs of this branch on the same clean-machine arm measured 685 then 683. The anchor stays the worse of the two — a threshold set from the better reading is a threshold that reds the next time the runner has a bad day.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slice S10-e per
internal-docs/superpowers/specs/2026-08-10-s10-diet-tiering-spec.md§5. Basebc0ccf4b.What comes off the default path
playwrightmoves fromdependenciesto an optional peer dependency. npm installsoptionalDependenciesbut not optional peers, so this is the one manifest lever that actually removes bytes fromnpm i -g wigolo— verified against a control fixture before relying on it.Measured on the shipped G-DIET protocol (
npm install --omit=dev --ignore-scripts --no-workspacesinto an empty dir), darwin-arm64, minutes apart:bc0ccf4bPer-package diff: exactly
playwright+playwright-coreremoved, nothing added.How a browser user still gets one
browser-pool,playwright-tier,browser-probe,browser-acquire,studio/session-browser) plus the TUI probe's secondrequirenow route through one lazy seam,src/fetch/browser-driver.ts, resolving from wigolo's own tree or the data directory. Type imports stay literal — they are erased, and they keep the callers fully typed against a package that may not be on disk.wigolo warmup --browser— and the existing lazyBrowserAcquireron first escalation, which drives exactly that — acquires the driver into~/.wigolo/browser-driver, then installs the binary from that same copy.devDependency, so this repo's suites and the studio e2e specs are unaffected.Proven end to end on a real clean global install (clean-machine-smoke, macOS):
Installing browser engine (chromium)... playwright installed→Browser: ok, with the package absent from the dependency tree. Also verified locally against the live registry: an empty prefix goes from unresolvable to resolvable in one call.The new G-DIET threshold, and why it is not a round number
≤ 693 MiB, down from 720.
The gate runs on exactly one machine class (clean-machine-smoke, macos-latest, node 22), and that runner measures 685 where this laptop measures 681. The threshold is anchored to the runner:
dependencies, worth exactly 17 MiB — lands at 702 and reds with 9 MiB of margin.G_TOTAL_DESKTOP_DROPPEDis re-derived because this slice moved one of its inputs; the composed failure window narrows 56 → 44 MiB, so A34's decision to drop that gate is better supported than when it was taken.Two bugs found by the probes rather than by review
doctorprobes three engines throughPromise.all, so three callers arrive before any finishes awaiting; with only a value memo each started its own import, and under a mocked graph the first got the mock while its siblings got the real package. Memoizing the promise (as the two existing optional-dep loaders already do) fixed five doctor assertions that were reading the wrong browser.Windows
The first push was green on ubuntu/macOS and red on both Windows runners:
spawn EINVAL, because npm is a batch script there and Node refuses to spawn one without a shell. A shell is not the fix (it joins arguments, and--prefixis a home-directory path). npm now runs as a JS entrypoint under the Node binary, exactly asinstallBrowseralready runs the driver's CLI. Note the degrade behaved correctly throughout — warmup named the cause and carried on to the models.Verification
gate:studiogreen, tests-debt holds at 412 (6 new debt errors fixed, not ratcheted).tsc --noEmitandnpm run lint -w apps/studioboth clean.git diff -- src/confirmed empty.