Repository navigation
test(api): fail when api startup or bootstrap configuration read process.env - #3116
Conversation
…ess.env #3111 moved API startup into api-startup.mjs and the Job's settings into scripts/lib/bootstrap-configuration.mjs, both taking their environment as a parameter so the sign-in parity tests can run them in process. Open PRs that edit server.mjs must port their edits there, and a ported process.env read would make those in-process tests check the runner's environment instead. Add a conformance test that parses both files and fails on process.env (member, computed, destructured or via globalThis) or an import from node:process, with appended-read cases proving each form is caught. It runs in checks-baseline-2 beside the other source-boundary checks.
Review follow-up: the member rule flagged any property named process (such
as options.process) yet missed `const { process } = globalThis`. Treat
globalThis/global like process itself, catch import()/re-exports of
node:process, and pin that a plain property named process stays allowed.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:feature
|
Codex review: needs maintainer review before merge. What this changesAdds a source-boundary test that rejects ambient process-environment access in API startup and bootstrap configuration, and registers it in CI. Example: A future startup edit reads process.env.OCC_CONSOLE_THEME_DIR instead of the supplied env parameter.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it adds a useful, focused guard for an existing startup contract, with no concrete introduced defect found; the failing required CI check still needs classification. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherProduction entrypoints pass process.env into parameterized startup functions, while parity tests pass isolated environment objects; the new guard parses those function owners and reports forbidden ambient access. flowchart LR
A[Startup source files] --> B[Existing TypeScript parser]
B --> C[Environment boundary assertions]
C --> D[Checks and Conformance 2]
D --> E[CI result]
Technical reviewBest possible solution: Retain the focused source guard alongside the existing runtime parity coverage. Do we have a high-confidence way to reproduce the issue? This is preventive coverage, not a report of a current runtime defect; source inspection verifies the explicit-environment contract and the guard's intended regression inputs. Is this the best way to solve the issue? The AST-based check is proportional to the existing contract, reuses an established parser, and leaves real startup behavior covered by the existing parity tests. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c9cfa7a84dde. Provenance checked
TestingProof path: unit tests only. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 21, 2026, 4:39 AM ET / 08:39 UTC. |
Problem
#3111 moved API startup into
apps/controller/src/api-startup.mjs(startApi(env)). It also moved the initialization Job's settings intoscripts/lib/bootstrap-configuration.mjs. Both take their environment as a parameter, sosign-in-chart-parity.test.mjscan run them in process with its own environment. Open PRs that editserver.mjs(#3048, #2980, #2518 and #1619) have to move those edits intoapi-startup.mjs. If a ported edit readsprocess.env(for example a newOCC_CONSOLE_THEME_DIRread), the in-process tests check the test runner's environment instead of the one they pass, and nothing fails.Change
Add
tests/conformance/startup-environment-boundary.test.mjsand map it to checks-baseline-2, next to the other source-boundary checks. The test parses both files with the same parser asscripts/verify-repository-credentials-boundary.mjsand fails on any of these:process.envin member, computed or optional formprocessused as a value, such as a destructuring or aliasprocessreached throughglobalThisorglobalnode:processprocess.exitCode,process.once, and properties that are only namedprocessare still allowed. Comments that mentionprocess.envare ignored.For each file, the test also appends every forbidden form to the real source and checks that each one is caught. It also checks that the allowed forms pass.
Verification
node --test tests/conformance/startup-environment-boundary.test.mjspasses in about 0.1 s.configuration(process.env)instartApiprocess.env.OCC_CONSOLE_THEME_DIRspread into the settingsenv[name] ?? process.env[name]fallback inbootstrap-configuration.mjschecks-baseline-2andstaticlanes (no cache) passed on c826dfd.run-tests.mjs auditpasses. eslint and prettier are clean.Risks
Test-only. Its scope is limited to these two files. Their dependencies still read a few process-wide settings from
process.env(such asOCC_DATABASE_AUTH), as #3111 documents.