refactor: better configuration - #2854
Conversation
- Introduced `guard-no-legacy-config.mjs` to prevent direct usage of `process.env` in core packages, ensuring all environment variables are declared in `app-config`. - Added a new script `sync-config-docs.ts` to automatically generate and update the declared configuration documentation from `appConfigSchema`. - Updated `run-guards.ts` to include the new guards for configuration management. - Enhanced the `agent-friction-report.mjs` with a new pattern to track environment variable sprawl.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Code Review Summary
PR #2854 centralizes environment-backed configuration in app-config, adds generated configuration documentation, and tightens guard coverage across core consumers. The overall direction is sound: a declared schema, explicit precedence layers, shared resolvers, and executable guards reduce duplicated parsing and make configuration discoverable. The new tests around layer merging and origin resolution are helpful, and the legacy-env guard passes in this checkout.
Key Findings
🟡 MEDIUM
- The committed generated configuration table is stale, so the new documentation guard fails on a clean checkout.
- The legacy-env guard has broad directory and namespace exemptions that allow undeclared configuration reads to bypass the intended default-deny policy.
- The generated worker shell rebuilds origin configuration from
process.env, bypassing higher-precedencedefineAppConfig()values. - Webhook self-dispatch now uses the generic resolver and can ignore the inbound request host when an ambient configured URL exists.
The PR is standard risk: it changes shared core configuration and deployment behavior, but not authentication or payment logic.
🧪 Browser testing: Will run after this review (PR touches UI code)
| order it is consulted; app configuration wins over any of them. Fields with | ||
| no alias are settable only in code. | ||
|
|
||
| | Field | Environment aliases | Type | Default | Description | |
There was a problem hiding this comment.
🟡 Regenerate the declared configuration table
pnpm guard:config-docs currently fails because the checked-in generated table does not exactly match scripts/sync-config-docs.ts output (the committed table is padded while the generator emits unpadded rows). Since this guard is registered in the guard suite, the branch cannot pass from a clean checkout; regenerate the block and commit the result.
Additional Info
Found by 4 of 4 review agents; confirmed by running pnpm guard:config-docs.
| "server/credential-provider.ts", | ||
| "server/request-context.ts", | ||
| // Build/deploy tooling composes env for a child, rather than reading config. | ||
| "deploy/", |
There was a problem hiding this comment.
🟡 Limit legacy-config guard directory exemptions
Exempting every file under deploy/, vite/, cli/, and scripts/ lets future runtime configuration consumers in those directories read process.env without declaring an app-config field. Limit the exemption to the specific child-environment producer modules or relevant lines so CLI and script consumers remain covered by the default-deny guard.
Additional Info
Found by 1 of 4 review agents; overlaps the guard's documented resolver-only policy.
| "LAMBDA_TASK_ROOT", | ||
| ]); | ||
|
|
||
| const PLATFORM_PREFIXES = ["AWS_", "npm_", "GITHUB_", "VITEST", "NETLIFY_"]; |
There was a problem hiding this comment.
🟡 Restrict platform namespace exemptions
Exempting every AWS_*, GITHUB_*, NETLIFY_*, npm_*, and VITEST* variable allows undeclared application settings—and credentials such as GITHUB_TOKEN—to bypass the guard. Use an explicit allowlist of genuinely host-owned keys rather than whole namespaces.
Additional Info
Found by 1 of 4 review agents; confirmed from the guard implementation.
| // app.url / workspace.* in app-config (worker bundles a string copy; it | ||
| // can't import them). Impersonal values only — this ships into the | ||
| // CDN-cached shell. | ||
| const env = globalThis.process?.env || {}; |
There was a problem hiding this comment.
🟡 Honor explicit app configuration in worker shell output
The generated worker origin projection reconstructs values directly from globalThis.process.env, while the normal SSR projection uses getAppConfig() and honors higher-precedence defineAppConfig() values. An app that sets app.url or workspace.* in code while environment aliases differ can therefore send conflicting server and browser origins on worker deployments; pass the resolved app-config values into this generated path or otherwise preserve the same precedence ladder.
Additional Info
Found by 2 of 4 review agents; confirmed by the differing resolution paths.
| `http://localhost:${process.env.PORT || 3000}`, | ||
| ); | ||
| } | ||
| return resolveSelfDispatchBaseUrl(event); |
There was a problem hiding this comment.
🟡 Preserve request-aware webhook self-dispatch fallback
Delegating resolveBaseUrl(event) to resolveSelfDispatchBaseUrl(event) changes webhook behavior when an ambient configured URL exists: the shared resolver can prefer that URL before considering the inbound request host. The targeted webhook test fails by dispatching to the configured local origin instead of the fixture host; preserve the webhook-specific request-aware fallback or derive and pass the request base URL explicitly.
Additional Info
Found by 1 of 4 review agents; targeted webhook test failure was reported and the resolver behavior is confirmed in surrounding code.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
guard-no-legacy-config.mjsto prevent direct usage ofprocess.envin core packages, ensuring all environment variables are declared inapp-config.sync-config-docs.tsto automatically generate and update the declared configuration documentation fromappConfigSchema.run-guards.tsto include the new guards for configuration management.agent-friction-report.mjswith a new pattern to track environment variable sprawl.