Repository navigation
fix: copy repo-root fixtures into web-console test image - #1151
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
sakibsadmanshajib
force-pushed
the
fix/web-console-test-fixtures
branch
from
August 25, 2026 12:03
91a389e to
b2b7826
Compare
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
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.
Summary
Two
apps/web-consoleunit test files failed to load on every clean-tree run of the mandateddocker compose run --rm --build web-console npm run test:unitcommand, and a third test silently skipped, all for the same reason:deploy/docker/Dockerfile.web-consolebuilds with the repo root as its build context (context: ../../indocker-compose.yml) but onlyCOPYsapps/web-console/. Anything a test reads from outside that subtree does not exist in the image.tests/unit/control-plane-host.test.tsreads.env.exampleand the wholedeploy/tree (to cross-check every hostname underdeploy/againstdeploy/cloudflare/tunnel-ingress.json) at module load time. Missing.env.examplethrew synchronously, so the entire file failed to load.tests/unit/chat-coverage-lib.test.tsreadsdocs/proof/chat-interaction-coverage-2026-08-10/coverage.run.jsoninside the "is never below what any recorded live run enumerated" test, which reproduced the reported ENOENT.components/catalog/model-catalog-table.test.tsguards its last assertion withit.skipIf(constraintValues.length === 0), andconstraintValuescomes fromexistsSync(supabase/migrations). That directory is also outside the copied subtree, so the test silently skipped instead of failing loud. Same root-cause class as the two ENOENTs above, just surfaced as a quiet skip; the test file itself already documents this ("Skipped only where supabase/migrations is not on disk, which is the deploy/docker web-console image").Fix
Added three narrow
COPYlines toDockerfile.web-console, after the existingCOPY apps/web-console/ ./, landing each fixture at the same repo-root-relative path the tests already resolve against (/app/..., matchingWORKDIR /app/apps/web-console's/approot):.env.example(64K)deploy/(2.5M, needed whole since the test recursively scans it for hostnames)docs/proof/chat-interaction-coverage-2026-08-10/coverage.run.json(76K, the one file the test reads, not all ofdocs/proof/which is 17M)supabase/migrations/(652K)None of these paths are excluded by the repo's
.dockerignore. Total added context: about 3.2M, chosen to keep the image copy narrow rather than pulling in the rest of the repo (docs/proof/alone is 17M, of which the tests need one 76K file).Deliberately did not make either test skip when its fixture is missing, and did not touch the assertions themselves: this repo has an explicit rule that a loud failure is preferable to a quiet absence, and the fix here is to make the fixture actually present rather than to soften the check.
Investigation of the reported "1 failed" test
Re-running the mandated command against
origin/mainbefore this fix, three times, produced 0 failures beyond the two ENOENT load failures and the one skip described above; the module-level reads incontrol-plane-host.test.tsthrow at import time (whole-file load failure), whilechat-coverage-lib.test.ts's ENOENT throws lazily inside a singleit()body, so depending on how a given tool renders vitest's collect-error vs test-failure distinction, that single assertion can show up as either "file failed to load" or "1 failed test" in a summary. After the fix, all three (both ENOENTs and the skip) resolve together; no separate, unrelated failing test was found across repeated runs.Required-check fix:
deploy-demo-box.ymlpaths filterThe first push failed the required
Repo policy lints (tenant + audit)check, specificallynode .github/ci/lint-deploy-paths-filter.mjs. Read the real failure (gh run view --log-failed) rather than guessing: the lint is genuinely working as designed. It walks everyCOPY/ADDsource in everydeploy/docker/Dockerfile.*and requires a matching entry indeploy-demo-box.yml'son.push.paths, because that filter has twice before silently swallowed a real change (apps/web-consolevia #786,vendor/open-webuivia #971) with no failure at all, just a merge that never triggered a deploy. My three newCOPYsources (.env.example,deploy/, the onedocs/proof/...file) were exactly the next instance of that gap.supabase/migrations/was already covered by an existingsupabase/migrations/**entry, so the lint did not flag it.Fixed by adding three entries to the paths filter (with a comment matching this file's existing convention, referencing this PR):
.env.example,deploy/**,docs/proof/chat-interaction-coverage-2026-08-10/coverage.run.json.deploy/**is deliberately the whole tree, not narrowed to the pre-existingdeploy/docker/**/deploy/litellm/**entries, for the same reason the Dockerfile copies the whole tree (see next section). Verified locally:npm installat repo root, thennode .github/ci/lint-deploy-paths-filter.mjsreportsDeploy path-filter coverage OK: every COPY/ADD source across 15 Dockerfiles under deploy/docker/ is covered. Also re-ran the siblinglint-workflow-check-names.mjsin the same job to confirm it is unaffected.Security review of the
COPY deploy/ /app/deploy/scopeAnswering the three questions raised on this PR directly:
1. Does anything under
deploy/that now enters the image carry a credential, token, or internal hostname that should not be baked into a built image? No. Grepped the wholedeploy/tree for AWS-style keys, PEM private-key headers,sk-...style tokens, and literalpassword:/secret:values with a non-os.environright-hand side: zero hits. Readdeploy/litellm/config.yamlspecifically since it was named directly: everyapi_key:line in it isos.environ/OPENROUTER_API_KEY,os.environ/GROQ_API_KEY, or the literal"none"(for a route that takes no key); there is no hardcoded credential anywhere in that file. It does carry internal hostnames and routing/pricing commentary, which is operational detail rather than a secret, and no different from what already ships in the image today viadeploy/docker/**/deploy/litellm/**, both alreadyCOPYed by other Dockerfiles in this same directory before this PR.2. Is
hive-web-console:ci(the image this Dockerfile builds) genuinely CI/test-only, or does it also serve the console on the demo box? Confirmed CI/test-only, and distinct from what ships.docker-compose.ymldefines two separate services:web-console(imagehive-web-console:ci, this Dockerfile, gated behindprofiles: [dev], runs plainnext dev) andweb-console-prod(imagehive-web-console-prod:ci, a different file,Dockerfile.web-console.prod, sitting behind its own Caddy origin,caddy-console). The header comment on the paths-filter's existingapps/web-console/**entry says this explicitly: "console-hive.scubed.co is web-console-prod... built... by Dockerfile.web-console.prod."deploy-demo-box.ymlnever references theweb-consoleservice or the:citag at all, onlyapps/web-consoleas a source path (becauseweb-console-prodis built from that same source tree).docker-bake.hcl'sweb-consoletarget tags the imagehive-web-console:ciand is never pushed to any registry inci.ymlordeploy-demo-box.yml(grepped both; nodocker push/registry step references this tag). So the image this PR'sCOPY deploy/lands in never leaves the CI runner or a developer's machine, and never reaches the demo box.3. Would a narrower
COPYsatisfy the test? No, not without defeating the test's purpose, and this is deliberate, not laziness.control-plane-host.test.ts's "deploy configuration hostnames" suite exists specifically to recursively scan every text file under all ofdeploy/(excludingdeploy/cloudflare/itself, which is the registry it checks against, andnode_modules) for any*.scubed.cohostname not declared indeploy/cloudflare/tunnel-ingress.json. That is the whole point of the suite: catch a stray or retired hostname in any file underdeploy/, including ones that do not exist yet. Copying only today's known files would make the test blind to the next Caddyfile or compose fragment added later, which is exactly the silent-gap failure mode this repo's own rules warn against.supabase/migrations/is the same shape (whole directory, needed because the constraint-value scan unions across every migration file). The two single-file copies (.env.example, the onecoverage.run.json) are already as narrow as the tests need.Test plan
cd deploy/docker && docker compose run --rm --build web-console npm run test:unitrun to completion against the fixed image, with a clean/idle host:Test Files 60 passed (60),Tests 653 passed (653), 0 failed, 0 skipped..dockerignorereview that none of the four newly-copied paths are excluded from the build context.node .github/ci/lint-deploy-paths-filter.mjspasses locally after the paths-filter fix.deploy/for hardcoded secrets (AWS keys, PEM headers,sk-...tokens, literal password/secret values); none found.deploy/litellm/config.yamlconfirmed to useos.environ/...indirection for every API key.docker-compose.yml,docker-bake.hcl, and bothci.yml/deploy-demo-box.ymlthathive-web-console:ciis CI/dev-only, never pushed to a registry, and not the image serving the demo box (web-console-prod/Dockerfile.web-console.prodis).Note on later local reruns: after rebasing onto a newer
main, repeated non---buildand--buildruns on this same dev box intermittently showed 2-5 unrelated test failures (analytics-billing-page-wiring.test.tsx,members-page-rbac.test.tsx, others), each a different set each run, with no code in this diff touching those files. The box was running 20+ other containers from concurrent sessions at the time (docker psshowed multiplehiveverify-*,composerfix-*stacks, 4.2G swapped) and the same run's own "environment" setup phase ballooned from ~150s to ~360s between attempts, consistent with resource-contention flakiness inwaitFor()-based React tests rather than a regression from this PR's Dockerfile/workflow-only diff. GitHub's isolated CI runner ran theWeb console (type + unit + build)job clean (60/60) on the pre-fix commit of this same PR, which is the authoritative signal for this class of test.Buglog entry
{"date":"2026-08-25","error_message":"ENOENT open '/app/.env.example' and ENOENT open '/app/docs/proof/chat-interaction-coverage-2026-08-10/coverage.run.json' during `docker compose run --build web-console npm run test:unit`; separately, components/catalog/model-catalog-table.test.ts silently skips its last assertion via it.skipIf","root_cause":"Dockerfile.web-console uses the repo root as build context but only COPYs apps/web-console/, so any test reading a repo-root path (.env.example, deploy/, docs/proof/..., supabase/migrations/) gets ENOENT or a false existsSync inside the image, even though those files are present on disk in every other run context","fix":"Added narrow COPY lines for .env.example, deploy/, the one coverage.run.json fixture under docs/proof/, and supabase/migrations/ into Dockerfile.web-console, landing each at the same /app-relative path the tests already resolve against; added matching entries to deploy-demo-box.yml's push.paths filter so a change to any of these still triggers a demo-box deploy; left the tests themselves untouched so a genuinely stale/missing fixture still fails loudly","tags":["web-console","docker","test-fixtures","vitest","dockerfile","ci-paths-filter"]}