Skip to content

Make builds Vite+ tasks - #212

Open
Maximo-Guk wants to merge 1 commit into
mainfrom
maximo/vp-task-env
Open

Make builds Vite+ tasks#212
Maximo-Guk wants to merge 1 commit into
mainfrom
maximo/vp-task-env

Conversation

@Maximo-Guk

@Maximo-Guk Maximo-Guk commented Aug 14, 2026

Copy link
Copy Markdown
Member

A cached vp run executes each task with only (PATH, HOME, CI, NODE_OPTIONS, …) forwarded https://viteplus.dev/guide/cache#environment-variables, anything else is invisible to the command and absent from the cache fingerprint.

In order to passthrough env vars we've converted the "build" script in to a Vite+ task and it now passes through relevant env vars. In addition to this, all "deploy" scripts use the --no-cache option when building, so they wouldn't be suspectible to this issue to begin with.

I also added tests which should catch numerous cases such as if you forgot to make your deploy script --no-cache, if you introduced a new build-time env read that isn't passed through, a deploy invoking a builder directly instead of through its task and a configurator gatekeeper with no vite.config.ts

@github-actions github-actions Bot added workshop/frontend Changes to the Workshop frontend kernel Changes to the Workshop kernel delivery Changes to CI or release delivery labels Aug 14, 2026
@ask-bonk

This comment was marked as resolved.

@Maximo-Guk
Maximo-Guk marked this pull request as draft August 14, 2026 18:07
@github-actions github-actions Bot added the gatekeeper Changes to a gatekeeper integration label Aug 14, 2026
@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@ask-bonk

This comment was marked as resolved.

@Maximo-Guk Maximo-Guk changed the title Make the two env-dependent builds Vite+ tasks Make builds Vite+ tasks Aug 14, 2026
@github-actions github-actions Bot added configurator Changes to gatekeeper configurator UI backend/utils Changes to shared backend utilities mcp/shared Changes to shared MCP integration code router Changes to the public router labels Aug 14, 2026
@Maximo-Guk
Maximo-Guk force-pushed the maximo/vp-task-env branch 2 times, most recently from 6471b23 to c43d9c7 Compare August 14, 2026 23:01
@github-actions github-actions Bot removed configurator Changes to gatekeeper configurator UI backend/utils Changes to shared backend utilities mcp/shared Changes to shared MCP integration code router Changes to the public router labels Aug 14, 2026
@ask-bonk

This comment was marked as resolved.

Comment thread scripts/build-gatekeeper-configurator.test.js Outdated
@ask-bonk

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Aug 14, 2026

Copy link
Copy Markdown

All 13 configurator packages are correctly wired: each re-exports the shared config and reaches build:configurator through vp run in its deploy. The wiring tests pass.

I've completed a thorough independent review of this PR at the current HEAD c823af6.

Review Summary

This PR fixes a real, subtle correctness bug: a cached vp run executes tasks in a stripped environment, so env-dependent build scripts silently dropped their build-time flags — both at execution (invisible to the command) and in the cache fingerprint (a changed value replayed a stale artifact). Affected: workshop-frontend (VITE_*), workshop-backend (FORMAT_BLUEPRINTS_DIR), and 13 configurator gatekeepers (VITE_FRONTEND_ERROR_REPORTING). The fix converts these build scripts into Vite+ tasks with declared env (or cache: false where the flag names an out-of-workspace path), plus two new guard tests.

The prior blocking finding is resolved

The last review flagged that scripts/build-gatekeeper-configurator.test.js imported parseTasks from a non-existent ./vite-config-tasks.mjs, breaking the whole root test suite. At HEAD c823af6 that import is gone — the test now imports typescript6 (line 8) and uses locally-defined stripComments/taskDeclaration helpers. I confirmed no reference to vite-config-tasks.mjs or parseTasks remains anywhere in scripts/.

Verification performed

  • Both new test suites pass (7/7) run against the actual tree: deploy-scripts.test.js and env-passthrough.test.js.
  • build-gatekeeper-configurator.test.js env/wiring logic verifies by hand: the builder reads exactly VITE_FRONTEND_ERROR_REPORTING, the config declares exactly that, and all 13 configurator packages re-export the shared config and reach build:configurator via vp run in deploy.
  • withVitestTask merge correctly preserves backend's build (cache: false) alongside the injected test task; RunTasksConfig.tasks is Record<string, unknown>, so the non-VitestTask shape type-checks.
  • Command order preserved byte-for-byte — frontend ['tsc', 'tsc -p tsconfig.vite.json', 'NODE_ENV=production vite build'] and backend codegen + tsc, split into cache-separable entries.
  • build-release.mjs correctly switches to vp run -F @gadgets/workshop-frontend build (package name verified; run defaults cwd: ROOT; FRONTEND_DIR still used for collectAssets; VITE_CF_ACCESS_MODE flows through env: ['VITE_*']).
  • All 15 packages dropped only the build script; gatekeeper-cloudflare (no configurator) correctly keeps build: tsc; workshop-backend retains build:worker; mcp-shared/gatekeeper-scheduler retain env-free build scripts (their remaining README pnpm --filter build lines stay valid).
  • Docs (AGENTS.md, 5 READMEs, format-blueprints README, SKILL/SKELETON) are accurate and internally consistent.

One non-blocking note (unchanged from prior reviews): dependencies aren't installed in this read-only checkout, so I couldn't execute vp to confirm VITE_* glob support in env or the NODE_ENV=production env-prefix syntax. The PR body reports these measured working, and the affected tests I could run pass.

LGTM!

github run

A cached vp run executes each task with only (PATH, HOME, CI, NODE_OPTIONS, …) forwarded. Anything else is invisible to the command and absent from the cache fingerprint.
@Maximo-Guk
Maximo-Guk marked this pull request as ready for review August 14, 2026 23:42
@ask-bonk

ask-bonk Bot commented Aug 14, 2026

Copy link
Copy Markdown

The only remaining pnpm --filter ... build reference is gatekeeper-scheduler, which correctly still has a build script (pnpm run typecheck:app && vp run --cache build:app && tsc), so pnpm --filter can see it. Correct — no stale doc.

My review is complete. Let me summarize.

Review Summary

This PR fixes a real, subtle correctness bug: a cached vp run executes tasks/scripts in a stripped environment, so env-dependent build scripts silently dropped their build-time flags — both at execution (invisible to the command) and in the cache fingerprint (a changed value replays a stale artifact). Affected: workshop-frontend (VITE_*), workshop-backend (FORMAT_BLUEPRINTS_DIR), and 13 configurator gatekeepers (VITE_FRONTEND_ERROR_REPORTING). The fix converts these into Vite+ tasks with declared env (or cache: false where the flag names an out-of-workspace path), plus three new guard tests.

The prior blocking finding is resolved

The last posted review flagged that scripts/build-gatekeeper-configurator.test.js imported parseTasks from a non-existent ./vite-config-tasks.mjs, breaking the entire root test suite. At the current HEAD aa44243 (single squashed commit) that import is gone — the test imports typescript6 and uses locally-defined stripComments/taskDeclaration helpers. I confirmed no reference to vite-config-tasks.mjs or parseTasks remains anywhere in scripts/.

Verification performed against HEAD aa44243

  • env-passthrough.test.js and deploy-scripts.test.js pass (7/7) run against the actual tree.
  • The configurator env-declaration and wiring logic verifies by hand (I extracted and ran it): builder reads exactly VITE_FRONTEND_ERROR_REPORTING, the task declares exactly that, and all 13 configurator gatekeepers re-export the shared config and reach build:configurator via vp run --no-cache in deploy. build-gatekeeper-configurator.test.js's only failure here is the deps-free checkout missing typescript6 (an environment limitation, not a defect).
  • Command order preserved — frontend ['tsc', 'tsc -p tsconfig.vite.json', 'NODE_ENV=production vite build'] matches the old script; backend splits into build:format-blueprints (cache: false) + build (browser-runtime + tsc), preserving the original && chain's behavior.
  • build-release.mjs correctly switches to vp run -F @gadgets/workshop-frontend build — package name verified, run defaults cwd: ROOT, FRONTEND_DIR still used for collectAssets, VITE_CF_ACCESS_MODE flows through env: ['VITE_*'].
  • All packages dropped only the build script; gatekeeper-cloudflare (no configurator) keeps build: tsc; mcp-shared/gatekeeper-scheduler keep env-free build scripts, so their remaining pnpm --filter ... build README lines stay valid; gatekeeper-context/-scheduler nest vp run --cache build:app in build.
  • Docs (AGENTS.md, 5 READMEs, format-blueprints README, SKILL/SKELETON) are accurate and internally consistent; the stale "vite build still doesn't cache" claim is corrected.

The hand-written stripComments tokenizer in the configurator test is a nice touch — it correctly handles the trap where a glob like !**/src/generated/** contains /*, which a naive regex comment-stripper would misread and swallow the only env declaration.

One non-blocking note (unchanged from prior reviews, and unresolvable here): dependencies aren't installed in this read-only checkout, so I couldn't execute vp to confirm VITE_* glob support in env or the NODE_ENV=production env-prefix syntax. The PR body reports these measured working, and every test I could run passes.

No actionable issues found.

LGTM!

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delivery Changes to CI or release delivery gatekeeper Changes to a gatekeeper integration kernel Changes to the Workshop kernel workshop/frontend Changes to the Workshop frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant