feat(web,worker): Environment overrides #597
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request consolidates three standalone packages ( Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application Startup
participant ES as entrypoint.sh
participant CO as loadConfig
participant OR as resolveEnvironmentVariableOverridesFromConfig
participant DB as Database
participant RD as Redis
App->>ES: Start container
Note over ES: CONFIG_PATH set?
alt CONFIG_PATH provided
ES->>CO: Load config file
CO-->>ES: SourcebotConfig
ES->>OR: Resolve overrides
Note over OR: Process env/googleCloudSecret tokens
OR-->>ES: {KEY=VALUE...}
ES->>ES: Apply overrides to process.env
end
Note over ES: Construct DATABASE_URL
alt DATABASE_URL not set
ES->>ES: Build from<br/>DATABASE_HOST,<br/>DATABASE_USER,<br/>DATABASE_PASS,<br/>DATABASE_NAME
end
Note over ES: Setup embedded services
alt REDIS_URL not set
ES->>RD: Enable embedded Redis
end
ES->>DB: Run migrations
ES->>ES: Complete initialization
sequenceDiagram
participant Code as Application Code
participant Shared as `@sourcebot/shared`
participant Crypto as crypto.ts
participant GCP as Google Cloud Secret Manager
Code->>Shared: import { getTokenFromConfig }
Code->>Code: Call getTokenFromConfig(token)
alt token.env
Code->>Crypto: Resolve env var
Crypto->>Crypto: Check process.env[key]
alt Found
Crypto-->>Code: Return value
else Not found
Crypto-->>Code: Throw Error
end
else token.googleCloudSecret
Code->>Crypto: Request GCP secret
Crypto->>GCP: Access secret
alt Success
GCP-->>Crypto: Secret payload
Crypto-->>Code: Return value
else Failure
Crypto-->>Code: Throw Error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai review |
This comment has been minimized.
This comment has been minimized.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
packages/shared/tools/resolveEnvOverrides.ts (1)
25-25: Fix incomplete string escaping for shell output.The escaping logic only handles double quotes but not backslashes. Values containing backslashes (e.g., Windows paths, literal
\n,\t) will produce malformed shell output.Apply this fix to escape both backslashes and quotes:
for (const [key, value] of Object.entries(overrides)) { - const escapedValue = value.replace(/"/g, '\\"'); + const escapedValue = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); originalConsoleLog(`${key}="${escapedValue}"`); }Note: Backslashes must be escaped first to avoid double-escaping.
packages/schemas/src/v3/index.schema.ts (1)
392-502: Duplicate: Same NEXT_PUBLIC_ constraint needed here.This root-level
environmentOverridesproperty has the same missing constraint as the definition (see comment on lines 138-248). The fix should be applied in both locations or the schema generation should be updated to ensure consistency.packages/shared/src/env.server.ts (1)
135-135: CONFIG_PATH requirement conflicts with runtimeEnv logic.This line marks
CONFIG_PATHas required, but lines 50-52 handle the case where it's undefined. See the earlier comment on lines 48-61 for the full analysis.
🧹 Nitpick comments (1)
entrypoint.sh (1)
38-52: Consider POSIX-compliant alternatives for colored output.The embedding flag logic is correct. However,
echo -eis not POSIX-compliant (as flagged by shellcheck). If strict POSIX compliance is required, consider usingprintfinstead:printf '\e[34m[Info] DATABASE_URL is not set. Using embeded database.\e[0m\n'Note: There's also a typo "embeded" (should be "embedded") in the log messages on lines 39 and 47.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (107)
.env.development(1 hunks)Dockerfile(0 hunks)Makefile(0 hunks)docs/snippets/schemas/v3/index.schema.mdx(13 hunks)entrypoint.sh(3 hunks)package.json(2 hunks)packages/backend/package.json(1 hunks)packages/backend/src/azuredevops.ts(2 hunks)packages/backend/src/bitbucket.ts(2 hunks)packages/backend/src/configManager.ts(1 hunks)packages/backend/src/connectionManager.ts(1 hunks)packages/backend/src/constants.ts(1 hunks)packages/backend/src/ee/githubAppManager.ts(2 hunks)packages/backend/src/ee/repoPermissionSyncer.ts(1 hunks)packages/backend/src/ee/syncSearchContexts.ts(1 hunks)packages/backend/src/ee/userPermissionSyncer.ts(1 hunks)packages/backend/src/env.ts(0 hunks)packages/backend/src/gerrit.ts(4 hunks)packages/backend/src/git.ts(1 hunks)packages/backend/src/gitea.ts(1 hunks)packages/backend/src/github.ts(1 hunks)packages/backend/src/gitlab.ts(1 hunks)packages/backend/src/index.ts(2 hunks)packages/backend/src/instrument.ts(1 hunks)packages/backend/src/posthog.ts(2 hunks)packages/backend/src/promClient.ts(1 hunks)packages/backend/src/repoCompileUtils.ts(1 hunks)packages/backend/src/repoIndexManager.ts(1 hunks)packages/backend/src/utils.ts(1 hunks)packages/backend/src/zoekt.ts(1 hunks)packages/crypto/.gitignore(0 hunks)packages/crypto/package.json(0 hunks)packages/crypto/src/environment.ts(0 hunks)packages/crypto/src/tokenUtils.ts(0 hunks)packages/crypto/tsconfig.json(0 hunks)packages/db/package.json(0 hunks)packages/db/tools/scriptRunner.ts(2 hunks)packages/db/tools/scripts/inject-audit-data.ts(4 hunks)packages/db/tools/scripts/inject-repo-data.ts(2 hunks)packages/db/tools/scripts/migrate-duplicate-connections.ts(3 hunks)packages/db/tools/scripts/test-repo-query-perf.ts(1 hunks)packages/db/tools/utils.ts(1 hunks)packages/error/package.json(0 hunks)packages/error/src/index.ts(0 hunks)packages/error/tsconfig.json(0 hunks)packages/logger/.gitignore(0 hunks)packages/logger/package.json(0 hunks)packages/logger/src/env.ts(0 hunks)packages/logger/tsconfig.json(0 hunks)packages/schemas/src/v3/index.schema.ts(13 hunks)packages/schemas/src/v3/index.type.ts(2 hunks)packages/shared/package.json(1 hunks)packages/shared/src/crypto.ts(5 hunks)packages/shared/src/db.ts(1 hunks)packages/shared/src/entitlements.ts(2 hunks)packages/shared/src/env.client.ts(1 hunks)packages/shared/src/env.server.ts(3 hunks)packages/shared/src/env.ts(0 hunks)packages/shared/src/index.client.ts(1 hunks)packages/shared/src/index.server.ts(2 hunks)packages/shared/src/logger.ts(2 hunks)packages/shared/src/types.ts(1 hunks)packages/shared/tools/resolveEnvOverrides.ts(1 hunks)packages/web/next.config.mjs(1 hunks)packages/web/package.json(0 hunks)packages/web/sentry.server.config.ts(1 hunks)packages/web/src/actions.ts(1 hunks)packages/web/src/app/[domain]/agents/page.tsx(1 hunks)packages/web/src/app/[domain]/chat/page.tsx(1 hunks)packages/web/src/app/[domain]/components/navigationMenu/index.tsx(1 hunks)packages/web/src/app/[domain]/components/settingsDropdown.tsx(1 hunks)packages/web/src/app/[domain]/layout.tsx(1 hunks)packages/web/src/app/[domain]/repos/[id]/page.tsx(1 hunks)packages/web/src/app/[domain]/settings/connections/[id]/page.tsx(1 hunks)packages/web/src/app/[domain]/settings/layout.tsx(1 hunks)packages/web/src/app/[domain]/settings/license/page.tsx(1 hunks)packages/web/src/app/[domain]/upgrade/page.tsx(1 hunks)packages/web/src/app/api/(server)/chat/route.ts(1 hunks)packages/web/src/app/api/(server)/ee/audit/route.ts(1 hunks)packages/web/src/app/api/(server)/ee/user/route.ts(1 hunks)packages/web/src/app/api/(server)/ee/users/route.ts(1 hunks)packages/web/src/app/api/(server)/health/route.ts(1 hunks)packages/web/src/app/api/(server)/stripe/route.ts(1 hunks)packages/web/src/app/api/(server)/version/route.ts(1 hunks)packages/web/src/app/api/(server)/webhook/route.ts(1 hunks)packages/web/src/app/components/authSecurityNotice.tsx(1 hunks)packages/web/src/app/components/organizationAccessSettings.tsx(1 hunks)packages/web/src/app/layout.tsx(1 hunks)packages/web/src/app/onboard/page.tsx(1 hunks)packages/web/src/app/posthogProvider.tsx(1 hunks)packages/web/src/app/signup/page.tsx(1 hunks)packages/web/src/auth.ts(1 hunks)packages/web/src/ee/features/audit/actions.ts(1 hunks)packages/web/src/ee/features/audit/auditService.ts(1 hunks)packages/web/src/ee/features/audit/factory.ts(1 hunks)packages/web/src/ee/features/billing/actions.ts(1 hunks)packages/web/src/ee/features/billing/stripe.ts(1 hunks)packages/web/src/ee/features/sso/sso.ts(2 hunks)packages/web/src/features/agents/review-agent/app.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generateDiffReviewPrompt.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts(1 hunks)packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts(1 hunks)packages/web/src/features/chat/actions.ts(3 hunks)packages/web/src/features/chat/agent.ts(2 hunks)
⛔ Files not processed due to max files limit (15)
- packages/web/src/features/chat/components/chatThread/answerCard.tsx
- packages/web/src/features/chat/utils.test.ts
- packages/web/src/features/fileTree/actions.ts
- packages/web/src/features/search/zoektClient.ts
- packages/web/src/hooks/useCaptureEvent.ts
- packages/web/src/initialize.ts
- packages/web/src/lib/authUtils.ts
- packages/web/src/lib/types.ts
- packages/web/src/middleware.ts
- packages/web/src/prisma.ts
- packages/web/src/withAuthV2.test.ts
- packages/web/src/withAuthV2.ts
- packages/web/tsconfig.json
- schemas/v3/index.json
- supervisord.conf
💤 Files with no reviewable changes (18)
- packages/logger/.gitignore
- packages/error/src/index.ts
- packages/crypto/tsconfig.json
- packages/logger/tsconfig.json
- packages/logger/src/env.ts
- packages/crypto/.gitignore
- packages/db/package.json
- packages/shared/src/env.ts
- packages/crypto/src/tokenUtils.ts
- packages/web/package.json
- packages/error/package.json
- packages/crypto/src/environment.ts
- packages/backend/src/env.ts
- Makefile
- packages/logger/package.json
- Dockerfile
- packages/crypto/package.json
- packages/error/tsconfig.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/web/src/features/agents/review-agent/nodes/generateDiffReviewPrompt.tspackages/web/src/features/agents/review-agent/nodes/generatePrReview.tspackages/web/src/app/[domain]/settings/license/page.tsxpackages/web/src/app/[domain]/repos/[id]/page.tsxpackages/web/src/features/agents/review-agent/nodes/fetchFileContent.tspackages/web/src/app/api/(server)/ee/audit/route.tspackages/web/src/app/onboard/page.tsxpackages/backend/src/zoekt.tspackages/db/tools/scriptRunner.tspackages/backend/src/azuredevops.tspackages/web/src/features/chat/agent.tspackages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.tspackages/web/src/features/agents/review-agent/nodes/githubPrParser.tspackages/backend/src/ee/userPermissionSyncer.tspackages/backend/src/repoCompileUtils.tspackages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.tspackages/web/src/app/[domain]/upgrade/page.tsxpackages/web/src/app/components/organizationAccessSettings.tsxpackages/web/src/auth.tspackages/db/tools/utils.tspackages/schemas/src/v3/index.type.tspackages/web/src/app/[domain]/settings/layout.tsxpackages/web/src/app/posthogProvider.tsxpackages/backend/src/constants.tspackages/shared/src/crypto.tspackages/backend/src/github.tspackages/shared/src/types.tspackages/web/src/actions.tspackages/shared/src/db.tspackages/web/src/ee/features/audit/actions.tspackages/web/src/app/api/(server)/stripe/route.tspackages/web/src/app/components/authSecurityNotice.tsxpackages/backend/src/index.tspackages/web/src/ee/features/sso/sso.tspackages/backend/src/connectionManager.tspackages/web/src/app/[domain]/components/navigationMenu/index.tsxpackages/backend/src/repoIndexManager.tspackages/web/src/app/api/(server)/ee/user/route.tspackages/backend/src/ee/githubAppManager.tspackages/backend/src/promClient.tspackages/web/src/app/api/(server)/ee/users/route.tspackages/backend/src/ee/syncSearchContexts.tspackages/backend/src/gerrit.tsentrypoint.shpackages/web/src/ee/features/audit/factory.tspackages/db/tools/scripts/migrate-duplicate-connections.tspackages/backend/src/posthog.tspackages/web/next.config.mjspackages/db/tools/scripts/inject-repo-data.tspackages/shared/src/entitlements.tspackages/web/src/ee/features/billing/stripe.tspackages/web/src/app/[domain]/agents/page.tsxpackages/web/src/app/[domain]/layout.tsxpackages/web/src/app/api/(server)/version/route.tspackages/web/src/app/[domain]/chat/page.tsxpackages/shared/tools/resolveEnvOverrides.tspackages/web/src/features/agents/review-agent/app.tspackages/shared/src/index.client.tspackages/web/src/app/[domain]/settings/connections/[id]/page.tsxpackages/web/src/app/layout.tsxpackages/web/src/app/api/(server)/webhook/route.tspackages/shared/src/env.client.tspackages/shared/src/logger.tspackages/backend/src/ee/repoPermissionSyncer.tspackages/web/src/app/signup/page.tsxpackages/web/sentry.server.config.tspackages/backend/src/bitbucket.tspackages/schemas/src/v3/index.schema.tspackages/backend/package.jsonpackages/db/tools/scripts/inject-audit-data.tspackages/web/src/app/api/(server)/health/route.tspackages/web/src/app/[domain]/components/settingsDropdown.tsxpackages/backend/src/utils.tspackage.jsonpackages/backend/src/instrument.tspackages/web/src/ee/features/billing/actions.tspackages/web/src/ee/features/audit/auditService.tspackages/backend/src/configManager.tspackages/web/src/app/api/(server)/chat/route.tspackages/shared/package.jsonpackages/shared/src/index.server.tspackages/web/src/features/chat/actions.tspackages/backend/src/gitlab.tsdocs/snippets/schemas/v3/index.schema.mdxpackages/shared/src/env.server.tspackages/backend/src/gitea.tspackages/db/tools/scripts/test-repo-query-perf.tspackages/backend/src/git.ts
🪛 GitHub Check: CodeQL
packages/shared/src/crypto.ts
[failure] 29-29: Use of password hash with insufficient computational effort
Password from an access to apiKey is hashed insecurely.
Password from an access to apiKey is hashed insecurely.
Password from an access to apiKeyPayload is hashed insecurely.
Password from a call to get is hashed insecurely.
Password from an access to apiKey is hashed insecurely.
Password from an access to apiKey is hashed insecurely.
Password from an access to apiKey is hashed insecurely.
Password from a call to get is hashed insecurely.
Password from an access to apiKeyString is hashed insecurely.
Password from an access to apiKeyString is hashed insecurely.
packages/shared/tools/resolveEnvOverrides.ts
[failure] 25-25: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🪛 Shellcheck (0.11.0)
entrypoint.sh
[warning] 13-13: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 23-23: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 39-39: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 47-47: In POSIX sh, echo flags are undefined.
(SC3037)
🔇 Additional comments (107)
.env.development (1)
87-87: LGTM!Setting
NODE_ENV=developmentin the development environment file is appropriate and aligns with standard practices.packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts (2)
5-5: LGTM! Import path updated correctly.The import path change from
@sourcebot/loggerto@sourcebot/sharedaligns with the PR's objective to consolidate standalone packages into a shared module.
7-35: LGTM! Function implementation is solid.The
fetchFileContentfunction has appropriate error handling, schema validation, and logging. No changes were made to the implementation logic, which remains correct.packages/db/tools/scriptRunner.ts (1)
27-45: LGTM! Clean logging migration.The replacement of logger calls with console output is straightforward and maintains the appropriate logging levels (console.error for errors, console.log for informational messages). The script behavior remains unchanged.
packages/db/tools/scripts/test-repo-query-perf.ts (1)
23-23: LGTM! Consistent logging migration.The logging change maintains the same informational output while aligning with the broader migration away from the logger package.
packages/db/tools/scripts/inject-audit-data.ts (1)
17-138: LGTM! Thorough logging migration.All logger calls have been consistently replaced with console output, maintaining appropriate logging levels throughout the script. The audit data generation logic and error handling remain unchanged.
packages/db/tools/utils.ts (1)
6-12: LGTM! Simple and consistent change.The logging migration in utility functions is straightforward, maintaining the same abort behavior and user messaging.
packages/db/tools/scripts/inject-repo-data.ts (1)
35-59: LGTM! Clean logging update.The logging changes appropriately report the repo creation progress without affecting the data injection logic.
packages/db/tools/scripts/migrate-duplicate-connections.ts (1)
18-50: LGTM! Consistent logging migration.The logging replacements maintain appropriate visibility into the migration process (duplicate count, individual migrations, and final summary) without altering the migration logic.
packages/web/src/features/chat/actions.ts (2)
4-4: LGTM! Import refactoring aligns with package consolidation.The migration of
envandgetTokenFromConfigimports to@sourcebot/sharedis consistent with the PR's objective to consolidate utilities into a shared package. These imports are used extensively throughout the file for environment variable access and token resolution.Also applies to: 23-23
192-192: LGTM! Correctly removes unused parameter.The
orgparameter was not referenced within the callback body, and the called functionupdateChatNameperforms its own org membership verification. Removing the unused parameter improves code clarity.packages/web/src/app/[domain]/settings/connections/[id]/page.tsx (1)
7-7: LGTM! Import refactoring looks correct.The import path update from
@/env.mjsto@sourcebot/sharedaligns with the package consolidation described in the PR summary. The single usage ofenv.CONFIG_PATHon line 35 should work seamlessly with the new import source.packages/web/next.config.mjs (1)
10-10: Verified:@t3-oss/env-nextjsremoval is safe.The package is not imported anywhere in the
packages/webdirectory. The only@t3-ossreference remaining is@t3-oss/env-corein the transpile configuration, which is correct. This change aligns with the refactor centralizing environment utilities to@sourcebot/shared.packages/web/src/app/[domain]/components/navigationMenu/index.tsx (1)
9-9: The import pattern is correct. The@sourcebot/sharedpackage is explicitly structured with separate index files (index.server.tsandindex.client.ts) that export the appropriateenvfor their context. The default export resolves toindex.server.js, makingimport { env } from "@sourcebot/shared"the correct and intended pattern for server-side code. This approach is already established across 60+ files in the codebase with no issues.packages/shared/src/entitlements.ts (2)
93-94: The review comment is incorrect — getPlan is server-only by design and cannot be called from client code.The package exports are configured so that
getPlanis only available via the server-only entry point (@sourcebot/shared), not the client-safe export (@sourcebot/shared/client). This means:
actions.tshas'use server'and safely callsgetPlanin server contextlicense/page.tsxis an async server component that safely callsgetPlanauthUtils.tsfunctions are imported only by server codeThe mixed environment access (using
clientEnv.NEXT_PUBLIC_*alongsideenv.SOURCEBOT_EE_LICENSE_KEY) is safe becausegetPlancan only execute in a server context where all environment variables are accessible. TypeScript and the bundler prevent importinggetPlaninto client code.Likely an incorrect or invalid review comment.
3-7: Imports are appropriate; verify call sites confirm server-only usage.The import consolidation correctly brings dependencies into the shared package and properly separates client/server environments. While the file does import both
env(server) andclientEnv(client), verification confirms all exported functions—getPlan(),hasEntitlement(), andgetEntitlements()—are called exclusively from server contexts: Server Components (packages/web/src/app/layout.tsx), server actions (packages/web/src/actions.ts), and backend code (packages/backend/src/ee/syncSearchContexts.ts).The mixed imports are intentional:
getPlan()checksNEXT_PUBLIC_SOURCEBOT_CLOUD_ENVIRONMENTfirst (cloud detection), then falls back to server-only license key validation. The early-return pattern prevents simultaneous env access, and the package's export structure ("."→index.server.js,"./client"→index.client.js) confirms these functions belong in the server export.packages/web/sentry.server.config.ts (1)
6-6: LGTM: Import path migration.The import path migration from
@sourcebot/loggerto@sourcebot/sharedis consistent with the PR's consolidation objective. The logger functionality remains unchanged.packages/web/src/app/[domain]/upgrade/page.tsx (1)
11-11: LGTM: Environment import centralization.The migration of
envimport from@/env.mjsto@sourcebot/sharedaligns with the PR's objective to centralize environment configuration. Usage at line 66 remains unchanged.packages/web/src/app/signup/page.tsx (1)
5-5: LGTM: Logger import consolidation.Migrating
createLoggerfrom@sourcebot/loggerto@sourcebot/sharedis consistent with the package consolidation effort. Functionality preserved.packages/web/src/app/components/authSecurityNotice.tsx (1)
4-4: LGTM: Client-side environment import.The migration to
@sourcebot/shared/clientis appropriate for this client component. The/clientsubpath ensures only client-safe environment variables are accessible, which is correct for accessingNEXT_PUBLIC_*variables.packages/backend/src/git.ts (1)
1-2: LGTM: Import consolidation and explicit dependency.The migration of
envto@sourcebot/sharedand the explicit import ofexistsSyncfromnode:fsimprove code clarity and align with the centralization effort.package.json (2)
7-8: LGTM: Explicit flags improve readability.Replacing shorthand flags (
-A) with explicit flags (--all --topological) improves script readability and maintainability.
21-21: Build:deps scope is correct and sufficient.The narrowed scope verification confirms that
{@sourcebot/schemas, @sourcebot/db, @sourcebot/shared}includes all necessary foundational dependencies:
@sourcebot/schemasand@sourcebot/dbare foundational (no internal dependencies)@sourcebot/shareddepends only on packages in scope (db, schemas)- Downstream packages (
@sourcebot/backend,@sourcebot/web) depend only on packages in the scope, so the narrowed build will not break their builds@sourcebot/mcphas no internal dependenciesThe change does not introduce any broken dependency chains.
packages/backend/src/github.ts (1)
3-6: LGTM: Comprehensive import consolidation.All utility imports (
getTokenFromConfig,createLogger,env,hasEntitlement) are now sourced from@sourcebot/shared, which aligns with the PR's package consolidation objective. The functionality remains unchanged.packages/backend/package.json (1)
54-54: Zod 3.25.x contains confirmed breaking changes—verify compatibility or revert the upgrade.Zod 3.25.x introduced breaking changes from 3.24.x, including TypeScript/compatibility issues related to Zod 4 being published under subpaths. The upgrade from
3.24.3to3.25.74crosses this breaking change boundary. Since this appears incidental to the PR's main objective, either:
- Verify the codebase passes tests with 3.25.74 and confirm no TypeScript regressions
- Revert to
^3.24.3if the upgrade was unintendedpackages/backend/src/gitea.ts (1)
1-10: LGTM - Import consolidation looks correct.The imports have been successfully refactored to use the centralized
@sourcebot/sharedpackage for utilities likegetTokenFromConfig,createLogger, andenv. The addition ofHttpResponsefromgitea-js(line 7) is properly utilized in thepaginatefunction (line 241) for accessing response headers.packages/backend/src/gitlab.ts (1)
1-9: LGTM - Import consolidation is consistent.The import restructuring to use
@sourcebot/sharedfor core utilities (getTokenFromConfig,createLogger,env) is consistent with the broader refactoring effort across the codebase.packages/backend/src/posthog.ts (1)
1-2: LGTM - Correct environment variable separation.The changes correctly separate public client-side environment variables (
clientEnvforNEXT_PUBLIC_*variables) from server-side environment variables (envforSOURCEBOT_INSTALL_ID). This aligns with Next.js best practices for environment variable handling.packages/shared/src/crypto.ts (2)
28-29: Static analysis note:hashSecretis for API keys, not passwords.The CodeQL warning about "insufficient computational effort" is a false positive in this context. The
hashSecretfunction (line 29) uses HMAC-SHA256 to hash API keys for storage/comparison purposes (seegenerateApiKeyat line 32), not user passwords.For API key hashing, HMAC-SHA256 is appropriate since API keys should already be high-entropy random values. However, the function name
hashSecretis somewhat misleading—consider renaming tohashApiKeyin a future refactor to avoid confusion.
81-107: Google Cloud Secret Manager implementation is sound; address error message sanitization.The
getTokenFromConfigfunction correctly usesSecretManagerServiceClient()without parameters, which properly leverages GCP's Application Default Credentials (ADC) chain—a best practice that handles authentication viaGOOGLE_APPLICATION_CREDENTIALS, workload identity, or metadata servers depending on the deployment environment.One concern: The error message at line 104 exposes the secret resource name (
token.googleCloudSecret) in the error string. Consider sanitizing error messages to avoid leaking infrastructure details:throw new Error(`Failed to access Google Cloud secret: ${error instanceof Error ? error.message : String(error)}`);Ensure your deployment environment has proper GCP authentication configured (either
GOOGLE_APPLICATION_CREDENTIALSenv var set or workload identity enabled for the service account).packages/backend/src/utils.ts (1)
5-5: LGTM - Import path updated correctly.The import source change from
@sourcebot/cryptoto@sourcebot/sharedis consistent with the package consolidation effort across this PR.packages/backend/src/promClient.ts (1)
4-4: LGTM! Logger import successfully migrated to shared package.This change aligns with the PR's consolidation effort to centralize logging utilities in
@sourcebot/shared.packages/shared/src/types.ts (1)
46-47: LGTM! Schema centralization improves code organization.Moving
tenancyModeSchemato the types file centralizes shared schema definitions and improves maintainability.packages/web/src/auth.ts (1)
7-7: LGTM! Environment import successfully migrated to shared package.This change consolidates environment utilities in
@sourcebot/shared, maintaining proper server-side context.packages/web/src/app/[domain]/settings/layout.tsx (1)
13-13: LGTM! Client-side environment import properly migrated.This change correctly uses
@sourcebot/shared/clientfor client-side environment access, maintaining proper client/server separation.packages/backend/src/configManager.ts (1)
2-2: LGTM! Logger import successfully consolidated.This change aligns with the PR's effort to centralize logging utilities in
@sourcebot/shared.packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.ts (1)
3-3: LGTM! Logger import successfully migrated.This change consolidates logging utilities in
@sourcebot/shared, maintaining consistency across the codebase.packages/web/src/app/api/(server)/ee/user/route.ts (1)
7-7: LGTM! Logger import successfully consolidated.This change aligns with the PR's consolidation of logging utilities into
@sourcebot/shared.packages/backend/src/repoCompileUtils.ts (1)
13-13: LGTM! Logger import successfully migrated.This change consolidates logging utilities in
@sourcebot/shared, completing the systematic refactor across the codebase.packages/web/src/app/api/(server)/chat/route.ts (1)
13-13: LGTM! Import path updated correctly.The logger import has been successfully migrated to
@sourcebot/sharedas part of the package consolidation effort. Usage remains unchanged.packages/web/src/features/agents/review-agent/nodes/githubPrParser.ts (1)
5-5: LGTM! Import path updated correctly.The logger import has been successfully migrated to
@sourcebot/shared. No functional changes.packages/web/src/features/agents/review-agent/nodes/generateDiffReviewPrompt.ts (1)
3-3: LGTM! Import path updated correctly.The logger import has been successfully migrated to
@sourcebot/shared. No functional changes.packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts (1)
5-5: LGTM! Import path updated correctly.The logger import has been successfully migrated to
@sourcebot/shared. No functional changes.packages/web/src/ee/features/audit/actions.ts (1)
8-8: LGTM! Import path updated correctly.The logger import has been successfully migrated to
@sourcebot/shared. No functional changes.packages/web/src/app/posthogProvider.tsx (1)
7-7: LGTM! Environment import updated correctly.The environment import has been properly migrated to
@sourcebot/shared/client, which is the correct path for client-side components. No functional changes.packages/backend/src/ee/syncSearchContexts.ts (1)
2-2: LGTM! Import path updated correctly.The logger import has been successfully migrated to
@sourcebot/shared. No functional changes.packages/web/src/ee/features/billing/stripe.ts (1)
2-2: LGTM! Environment import updated correctly.The environment import has been properly migrated to
@sourcebot/shared, which is the correct path for server-only modules. No functional changes.packages/web/src/ee/features/billing/actions.ts (1)
10-10: LGTM! Import consolidation for environment and logging utilities.The migration of
envandcreateLoggerimports to@sourcebot/sharedaligns with the PR's objective to centralize environment configuration and supports the new environment variable override functionality.Also applies to: 15-15
packages/web/src/app/onboard/page.tsx (1)
20-20: LGTM! Environment import centralized.The
envimport has been successfully migrated to@sourcebot/shared, maintaining consistency with the codebase-wide refactoring.packages/backend/src/connectionManager.ts (1)
3-3: LGTM! Clean import consolidation.The migration of
createLogger,loadConfig, andenvto@sourcebot/sharedcentralizes utility access and supports the environment override feature. CombiningloadConfigandenvin a single import statement is a nice touch.Also applies to: 5-5
packages/web/src/app/[domain]/chat/page.tsx (1)
12-12: LGTM! Environment import updated.The
envimport path has been correctly updated to@sourcebot/shared.packages/web/src/ee/features/audit/auditService.ts (1)
4-4: LGTM! Logger import centralized.The
createLoggerimport has been migrated to@sourcebot/sharedconsistently with other files in this PR.packages/web/src/app/api/(server)/ee/audit/route.ts (1)
9-9: LGTM! Environment import migrated.The
envimport has been successfully moved to@sourcebot/shared.packages/web/src/app/api/(server)/webhook/route.ts (1)
7-7: LGTM! Utility imports centralized.Both
envandcreateLoggerhave been migrated to@sourcebot/shared, maintaining consistency with the broader refactoring effort.Also applies to: 12-12
packages/backend/src/constants.ts (1)
2-2: LGTM! Environment import centralized.The
envimport has been correctly migrated from the local module to@sourcebot/shared, completing the backend's transition to centralized environment utilities.packages/web/src/app/api/(server)/health/route.ts (1)
3-3: LGTM - Logger import migrated to shared module.The import path update aligns with the repository-wide consolidation to
@sourcebot/shared. The logger functionality remains unchanged.packages/web/src/app/layout.tsx (1)
9-9: LGTM - Environment import centralized.The migration to
@sourcebot/sharedfor environment variables is consistent with the PR's objective to consolidate environment configuration.packages/web/src/app/[domain]/agents/page.tsx (1)
4-4: LGTM - Environment import consolidated.The import path update is part of the broader refactor to centralize environment utilities in
@sourcebot/shared.packages/web/src/app/components/organizationAccessSettings.tsx (1)
9-9: LGTM - Environment import migrated to shared module.This change aligns with the PR's consolidation strategy for environment configuration.
packages/web/src/app/[domain]/settings/license/page.tsx (1)
7-7: LGTM - Client environment import path is appropriate.Using
@sourcebot/shared/clientfor accessingNEXT_PUBLIC_*environment variables is the correct approach, ensuring these variables are available in the client bundle.packages/web/src/app/[domain]/repos/[id]/page.tsx (1)
7-7: LGTM - Environment import consolidated.The migration to
@sourcebot/sharedmaintains consistency across the codebase for environment configuration access.packages/web/src/app/[domain]/components/settingsDropdown.tsx (1)
35-35: LGTM - Client environment import is correctly used.Since this is a client component (
'use client'), importing from@sourcebot/shared/clientis the appropriate pattern for accessing client-side environment variables.packages/backend/src/zoekt.ts (1)
2-2: LGTM - Logger import consolidated to shared module.This change is part of the broader effort to centralize logging utilities in
@sourcebot/shared, maintaining consistency across the backend.packages/web/src/app/[domain]/layout.tsx (1)
19-19: LGTM: Clean import consolidation.The migration of
envfrom the local@/env.mjsto the centralized@sourcebot/sharedpackage aligns with the PR's objective to consolidate environment utilities.packages/backend/src/bitbucket.ts (1)
5-5: LGTM: Successful consolidation of utilities.Both
createLoggerandgetTokenFromConfighave been properly migrated from their standalone packages to@sourcebot/shared, consistent with the PR's consolidation objectives.Also applies to: 14-14
packages/web/src/app/api/(server)/stripe/route.ts (1)
7-8: LGTM: Import paths updated correctly.The migration of
envandcreateLoggerto@sourcebot/sharedis correct for this server-side route handler, with no impact on the webhook processing logic.packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (1)
3-3: LGTM: Import consolidation successful.The centralization of
envandcreateLoggerimports maintains the existing agent behavior while improving module organization.Also applies to: 5-5
packages/web/src/app/api/(server)/ee/users/route.ts (1)
7-7: LGTM: Logger import updated correctly.The migration of
createLoggerto the shared package is correct and maintains all existing logging functionality.packages/web/src/app/api/(server)/version/route.ts (1)
1-1: LGTM: Correct use of client environment path.The import from
@sourcebot/shared/clientis appropriate here since the route accessesNEXT_PUBLIC_SOURCEBOT_VERSION, which is a client-side environment variable. This demonstrates proper separation between client and server environment contexts.packages/web/src/ee/features/sso/sso.ts (1)
2-2: LGTM: SSO utilities properly centralized.The migration of
envandcreateLoggerto@sourcebot/sharedis consistent with the codebase-wide consolidation, with no impact on SSO provider configuration or authentication flows.Also applies to: 14-14
packages/shared/src/index.client.ts (1)
1-4: LGTM: Client export surface established.The addition of the
envexport fromenv.client.jsproperly establishes the client-side entry point for the shared package, supporting the environment consolidation strategy seen across the codebase.packages/web/src/ee/features/audit/factory.ts (1)
5-5: LGTM! Clean import migration.The migration from local env module to the centralized
@sourcebot/sharedpackage is correct and consistent with the PR objectives.packages/web/src/features/chat/agent.ts (3)
1-2: LGTM! Proper client/server env separation.The introduction of separate
envandclientEnvimports correctly separates server-side and client-side environment variables, following Next.js best practices.
6-6: LGTM! Logger import consolidated.The migration of
createLoggerfrom@sourcebot/loggerto@sourcebot/sharedaligns with the centralization effort.
144-144: LGTM! Correct use of client env for public variables.Using
clientEnv.NEXT_PUBLIC_SOURCEBOT_CLOUD_ENVIRONMENTis appropriate since this is a public environment variable that should be accessible on the client side.packages/web/src/features/agents/review-agent/app.ts (2)
5-5: LGTM! Import migration completed.The env import migration to
@sourcebot/sharedis consistent with the broader refactoring effort.
9-9: LGTM! Logger import consolidated.The migration of
createLoggerto the shared package is correct.packages/backend/src/repoIndexManager.ts (1)
3-4: LGTM! Effective import consolidation.The consolidation of logging, environment, and type imports into
@sourcebot/sharedreduces dependencies and aligns with the centralization effort. The migration is complete and correct.packages/backend/src/ee/repoPermissionSyncer.ts (1)
3-4: LGTM! Clean import consolidation.The migration consolidates
createLogger,env, andhasEntitlementimports into the centralized@sourcebot/sharedpackage, maintaining consistency across the codebase.packages/backend/src/index.ts (2)
4-5: LGTM! Import consolidation completed.The migration consolidates multiple utilities into
@sourcebot/shared, including the newgetDBConnectionStringfunction that supports the environment overrides feature.
31-37: LGTM! Explicit datasource configuration enables environment overrides.The explicit configuration of
PrismaClientwithgetDBConnectionString()is a key change that enables the environment overrides feature described in the PR. This allows theDATABASE_URLto be resolved fromconfig.jsonusing token references or direct overrides.packages/backend/src/ee/githubAppManager.ts (2)
2-6: LGTM! Import consolidation completed.The migration consolidates token, logger, and environment utilities into
@sourcebot/shared, maintaining consistency with the broader refactoring effort.
47-47: LGTM! Cleaner configuration loading.The removal of the non-null assertion (
!) is appropriate. TypeScript will enforce type safety ifCONFIG_PATHis required by theloadConfigfunction signature.packages/web/src/actions.ts (3)
4-4: LGTM! Environment import migrated.The env import migration to
@sourcebot/sharedis consistent with the centralization effort.
12-12: LGTM! Crypto utilities consolidated.The migration of
generateApiKey,getTokenFromConfig, andhashSecretfrom@sourcebot/cryptoto@sourcebot/sharedcentralizes cryptographic utilities effectively.
14-14: LGTM! Logger import consolidated.The migration of
createLoggerto the shared package completes the consolidation.packages/backend/src/azuredevops.ts (2)
2-2: LGTM: Import consolidation to shared package.The migration of
createLoggerandgetTokenFromConfigto@sourcebot/sharedaligns with the PR's objective to consolidate standalone packages into a unified shared module.Also applies to: 9-9
38-38: LGTM: Simplified error handling.Replacing custom error types with a plain
Erroris appropriate here while still maintaining Sentry reporting.packages/backend/src/ee/userPermissionSyncer.ts (1)
3-4: LGTM: Import consolidation to shared package.The migration of
createLogger,env, andhasEntitlementto@sourcebot/sharedis consistent with the PR's consolidation goals.packages/shared/src/logger.ts (2)
5-5: LGTM: Aligned with server-side environment utilities.The import change to
./env.server.jsproperly aligns the logger with the server-side environment configuration module.
19-19: LGTM: Cleanup of unused import.Removing
prettyPrintfrom the destructuring is appropriate as it was not used anywhere in the logger implementation.packages/backend/src/instrument.ts (1)
2-3: LGTM: Import consolidation to shared package.The migration to
@sourcebot/sharedand@sourcebot/shared/clientis consistent with the PR's package consolidation efforts.packages/shared/package.json (2)
9-10: LGTM: Added tooling for environment override resolution.The new
tool:resolve-env-overridesscript and updatedpostinstallsupport the environment overrides feature introduced in this PR.
13-24: LGTM: Dependencies aligned with package consolidation.The dependency changes properly support:
- Logger functionality moved from
@sourcebot/logger(winston, logtail)- Token resolution from external secret managers (GCP Secret Manager)
- Removal of now-consolidated
@sourcebot/cryptoand@sourcebot/loggerpackagespackages/schemas/src/v3/index.type.ts (1)
39-39: LGTM: Well-structured type definition for environment overrides.The new
EnvironmentOverridesinterface properly supports multiple value types (token, string, number, boolean) and follows the same token resolution pattern used elsewhere in the config (connections, auth providers, language models).Also applies to: 151-191
packages/shared/src/env.client.ts (1)
1-27: LGTM! Clean client-side environment configuration.The use of @t3-oss/env-core for client-side environment validation is well-structured, with appropriate defaults and optional fields.
packages/shared/src/index.server.ts (1)
19-49: LGTM! Exports align with the consolidation goals.The new server-side exports properly surface environment, logging, cryptography, and database utilities from the centralized shared package.
packages/backend/src/gerrit.ts (3)
35-62: LGTM! Error handling centralized as intended.The removal of local try/catch aligns with the PR's goal to centralize error handling. Errors from
fetchWithRetrywill now propagate naturally to calling code.
64-104: LGTM! Simplified error handling.The switch from BackendException to generic Error is consistent with the broader refactoring. The error message remains descriptive with endpoint and status information.
106-152: LGTM! Minor formatting improvement.The vertical layout of the special projects array improves readability with no functional change.
entrypoint.sh (3)
3-6: LGTM! Proper shell error handling.Setting
set -eensures the script exits on command failures, andset +aprevents unintended variable exports.
29-36: LGTM! DATABASE_URL construction logic is sound.The conditional construction from individual variables provides flexibility while respecting explicit DATABASE_URL settings.
202-202: LGTM! Explicit DATABASE_URL for migrations.Setting
DATABASE_URLexplicitly for the migration command ensures the resolved value is used, regardless of whether it was constructed or provided.packages/schemas/src/v3/index.schema.ts (1)
548-549: LGTM! Description placement improved.Moving token descriptions to the field level (after the
anyOf) improves schema readability without changing validation behavior.Also applies to: 756-757, 958-959, 1225-1226, 1393-1394
docs/snippets/schemas/v3/index.schema.mdx (2)
1-1: Remember: This is an auto-generated file.Any schema changes should be made in the source TypeScript schema file (likely
packages/schemas/src/v3/index.schema.ts) and then regenerated, not edited directly here.
139-249: The review comment references incorrect procedures and unverifiable requirements.The file
docs/snippets/schemas/v3/index.schema.mdxis auto-generated and marked "DO NOT MODIFY MANUALLY." Changes must be made to the source schema file atschemas/v3/index.json, not the generated.mdxdocumentation file. The generation script (packages/schemas/tools/generate.ts) derives all outputs from the JSON schema sources.Additionally:
- No TODO comments or PR objectives about blocking
NEXT_PUBLIC_environment variables exist anywhere in the codebase.- The pattern
^[a-zA-Z0-9_-]+$appears consistently across other schema properties (e.g.,connections), suggesting the hyphen support is an intentional design choice, not an oversight.- The proposed diff structure (using
patternPropertiesinside anotconstraint) is non-standard JSON Schema and unlikely to function as intended.Without documented requirements or PR context substantiating the
NEXT_PUBLIC_blocking request, this review is based on unverifiable assumptions.Likely an incorrect or invalid review comment.
packages/shared/src/env.server.ts (1)
198-218: Good additions for operational control.The new environment variables provide valuable operational control:
- Database configuration flexibility (lines 124-132)
- Redis configuration (lines 205-207)
- Logging configuration (lines 216-218)
- Operational timeouts and retry settings (lines 211-214)
The required
SOURCEBOT_ENCRYPTION_KEY(line 198) properly enforces security requirements.
| export const resolveEnvironmentVariableOverridesFromConfig = async (config: SourcebotConfig): Promise<Record<string, string>> => { | ||
| if (!config.environmentOverrides) { | ||
| return {}; | ||
| } | ||
|
|
||
| const resolved: Record<string, string> = {}; | ||
|
|
||
| const start = performance.now(); | ||
|
|
||
| for (const [key, override] of Object.entries(config.environmentOverrides)) { | ||
| switch (override.type) { | ||
| case 'token': | ||
| resolved[key] = await getTokenFromConfig(override.value); | ||
| break; | ||
| case 'boolean': | ||
| resolved[key] = override.value ? 'true' : 'false'; | ||
| break; | ||
| case 'number': | ||
| resolved[key] = override.value.toString(); | ||
| break; | ||
| case 'string': | ||
| resolved[key] = override.value; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const end = performance.now(); | ||
| console.debug(`resolved environment variable overrides in ${end - start}ms`); | ||
|
|
||
| return resolved; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use structured logger and add error handling.
Two improvements needed:
-
Line 43: Replace
console.debugwith the structured logger from./logger.js. This breaks the logging consistency and won't respectSOURCEBOT_LOG_LEVELor structured logging settings. -
Lines 28, 34, 37: Missing error handling for token resolution and type conversions. If
getTokenFromConfigfails or values are invalid, the function should provide clear error messages.
Apply this diff:
+import { logger } from "./logger.js";
+
export const resolveEnvironmentVariableOverridesFromConfig = async (config: SourcebotConfig): Promise<Record<string, string>> => {
if (!config.environmentOverrides) {
return {};
}
const resolved: Record<string, string> = {};
-
const start = performance.now();
for (const [key, override] of Object.entries(config.environmentOverrides)) {
- switch (override.type) {
- case 'token':
- resolved[key] = await getTokenFromConfig(override.value);
- break;
- case 'boolean':
- resolved[key] = override.value ? 'true' : 'false';
- break;
- case 'number':
- resolved[key] = override.value.toString();
- break;
- case 'string':
- resolved[key] = override.value;
- break;
+ try {
+ switch (override.type) {
+ case 'token':
+ resolved[key] = await getTokenFromConfig(override.value);
+ break;
+ case 'boolean':
+ resolved[key] = override.value ? 'true' : 'false';
+ break;
+ case 'number':
+ resolved[key] = override.value.toString();
+ break;
+ case 'string':
+ resolved[key] = override.value;
+ break;
+ }
+ } catch (error) {
+ logger.error({ key, error }, `Failed to resolve environment variable override for ${key}`);
+ throw new Error(`Failed to resolve environment variable override for ${key}: ${error instanceof Error ? error.message : String(error)}`);
}
}
const end = performance.now();
- console.debug(`resolved environment variable overrides in ${end - start}ms`);
+ logger.debug(`resolved environment variable overrides in ${end - start}ms`);
return resolved;
}🧩 Analysis chain
Security concern: Sensitive environment variables can be overridden.
The resolveEnvironmentVariableOverridesFromConfig function, combined with the EnvironmentOverrides schema, allows overriding nearly any environment variable through the config file. This includes sensitive variables like:
AUTH_SECRET(line 70)SOURCEBOT_ENCRYPTION_KEY(line 198)- API keys for various services (lines 153-179)
- Database credentials (lines 124-132)
If an attacker gains write access to config.json or can inject environment variables that resolve tokens, they could compromise the entire system. Consider:
- Expanding the blocklist in the schema to prevent overriding security-critical variables
- Adding runtime validation to reject overrides for sensitive keys
- Documenting the security implications of environment overrides clearly
🏁 Script executed:
#!/bin/bash
# Find all security-sensitive environment variable definitions
rg -n "AUTH_|_SECRET|_KEY|_PASSWORD" packages/shared/src/env.server.tsLength of output: 2795
---
🏁 Script executed:
```shell
#!/bin/bash
# Find EnvironmentOverrides schema definition
rg -n "EnvironmentOverrides" packages/shared/src/ -A 10 -B 2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any blocklist or validation of override keys
rg -n "environmentOverrides|override.*key|restricted|blocked|allow" packages/shared/src/env.server.ts -B 2 -A 2Length of output: 407
🏁 Script executed:
#!/bin/bash
# Search in config-related files for override restrictions
fd -e ts -e js packages/shared/src/ | xargs rg -l "EnvironmentOverrides\|environmentOverrides" | head -20Length of output: 385
🏁 Script executed:
#!/bin/bash
# Search all ts/js files for EnvironmentOverrides type definition
fd -type f -name "*.ts" -o -name "*.js" | xargs rg "EnvironmentOverrides\s*[=:{]" -B 2 -A 5Length of output: 730
🏁 Script executed:
#!/bin/bash
# Look for CONFIG_PATH references and any blocklist
rg -n "CONFIG_PATH" packages/shared/src/ -B 3 -A 3Length of output: 985
🏁 Script executed:
#!/bin/bash
# Search for config schema definitions (may be in config or schema files)
fd -type f \( -name "*config*.ts" -o -name "*schema*.ts" \) packages/ | head -20Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check if there's validation in resolveEnvironmentVariableOverridesFromConfig or surrounding code
cat packages/shared/src/env.server.ts | grep -n "resolveEnvironmentVariableOverridesFromConfig" -A 50 | head -60Length of output: 2086
🏁 Script executed:
#!/bin/bash
# Get full EnvironmentOverrides schema definition
cat packages/schemas/src/v3/index.type.ts | grep -n "EnvironmentOverrides" -A 20Length of output: 1631
🏁 Script executed:
#!/bin/bash
# Search for the actual JSON schema file (v3 schema)
find packages/schemas -name "*.json" | xargs grep -l "EnvironmentOverrides" 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any validation or blocklist logic around environmentOverrides
rg -n "environmentOverrides" packages/ -B 5 -A 5 --type ts --type json | head -80Length of output: 3161
🏁 Script executed:
#!/bin/bash
# Get the full blocklist from the schema
cat packages/schemas/src/v3/index.schema.ts | grep -n "environmentOverrides" -A 100 | head -150Length of output: 3495
🏁 Script executed:
#!/bin/bash
# Search for CONFIG_PATH in the schema blocklist
rg -n "CONFIG_PATH" packages/schemas/src/v3/index.schema.ts -B 10 -A 10Length of output: 1262
Security concern: Sensitive environment variables can be overridden via config file.
The schema blocks only CONFIG_PATH from override (lines 145-147 in packages/schemas/src/v3/index.schema.ts), leaving all other variables unprotected. The resolveEnvironmentVariableOverridesFromConfig function performs no runtime validation and directly assigns any key from the config's environmentOverrides to the resolved environment variables.
This allows overriding sensitive variables including:
AUTH_SECRET(line 70)SOURCEBOT_ENCRYPTION_KEY(line 198)- API keys:
ANTHROPIC_API_KEY,OPENAI_API_KEY,AZURE_API_KEY, etc. (lines 153-169) - Database credentials:
DATABASE_PASSWORD(line 130)
An attacker with write access to config.json could compromise the entire system. Consider:
- Expanding the blocklist to prevent overriding all security-critical variables
- Adding runtime validation to reject overrides for sensitive keys
- Documenting the security implications clearly
| // Database variables | ||
| // Either DATABASE_URL or DATABASE_HOST, DATABASE_USERNAME, DATABASE_PASSWORD, and DATABASE_NAME must be set. | ||
| // @see: shared/src/db.ts for more details. | ||
| DATABASE_URL: z.string().url().optional(), | ||
| DATABASE_HOST: z.string().optional(), | ||
| DATABASE_USERNAME: z.string().optional(), | ||
| DATABASE_PASSWORD: z.string().optional(), | ||
| DATABASE_NAME: z.string().optional(), | ||
| DATABASE_ARGS: z.string().optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify database validation logic enforces the "either...or" constraint.
The comment states "Either DATABASE_URL or DATABASE_HOST, DATABASE_USERNAME, DATABASE_PASSWORD, and DATABASE_NAME must be set," but the schema marks all as optional. This constraint isn't enforced at the schema level.
Confirm the validation exists in packages/shared/src/db.ts:
🏁 Script executed:
#!/bin/bash
# Check for database connection string validation logic
ast-grep --pattern $'function getDBConnectionString($_) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Read the db.ts file to see validation logic
cat packages/shared/src/db.tsLength of output: 542
🏁 Script executed:
#!/bin/bash
# Search for environment variable validation using refine/superRefine patterns
rg -n -A5 -B5 "DATABASE_(URL|HOST|USERNAME|PASSWORD|NAME)" packages/shared/src/Length of output: 2362
🏁 Script executed:
#!/bin/bash
# Look for any validation of database connection logic
rg -n "refine\|superRefine\|parse" packages/shared/src/env.server.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find where env is created/parsed to check for refine validation
rg -n "parse\|create\|parse(process" packages/shared/src/env.server.ts -A10 -B2Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for where getDBConnectionString is called to see error handling
rg -n "getDBConnectionString" packages/ -A3 -B3Length of output: 2430
🏁 Script executed:
#!/bin/bash
# Look at entire env.server.ts file structure around the schema definition
cat packages/shared/src/env.server.ts | head -200Length of output: 7863
🏁 Script executed:
#!/bin/bash
# Check if any refine() is applied to the env schema after line 132
rg -n "\.refine\|\.superRefine" packages/shared/src/env.server.ts -A5 -B5Length of output: 0
Add schema-level validation for database configuration.
The comment promises "Either DATABASE_URL or DATABASE_HOST, DATABASE_USERNAME, DATABASE_PASSWORD, and DATABASE_NAME must be set," but the schema marks all as optional without enforcing this constraint. While runtime logic in db.ts handles the validation, Zod should enforce this at parse time using .refine() or .superRefine() to prevent invalid configurations from passing validation. Currently, invalid combinations could pass schema validation and return undefined from getDBConnectionString(), which is then used directly without null-checking.
Add a refine check to the schema to ensure at least one valid configuration option is present.
🤖 Prompt for AI Agents
In packages/shared/src/env.server.ts around lines 124-132, the Zod schema
currently marks DATABASE_URL, DATABASE_HOST, DATABASE_USERNAME,
DATABASE_PASSWORD, and DATABASE_NAME as optional but does not enforce the
comment's requirement that either DATABASE_URL must be provided or the set
{DATABASE_HOST, DATABASE_USERNAME, DATABASE_PASSWORD, DATABASE_NAME} must all be
provided; add a .refine() or .superRefine() to the schema that returns true only
if DATABASE_URL is set (and valid) OR all four host/username/password/name are
non-empty strings, and provide a clear error message like "Either DATABASE_URL
or DATABASE_HOST, DATABASE_USERNAME, DATABASE_PASSWORD and DATABASE_NAME must be
set" so invalid envs fail schema parsing.
brendan-kellam
force-pushed
the
bkellam/environment_overrides
branch
from
e5bc471 to
169e709
Compare
The following adds support for
environmentOverridesinconfig.json, allowing for the ability to override environment variables in the config. Overrides support typesstring,boolean,number, andtoken.tokenhas the same shape as other places in the config (e.g., connections, auth providers, language models etc.).The main useful behaviour of this feature is the ability to configure environment variables with external secret management systems (e.g., GCP secret manage). For example:
{ "$schema": "./schemas/v3/index.json", "connections": { "sourcebot": { "type": "github", "repos": [ "sourcebot-dev/sourcebot" ] } }, "environmentOverrides": { "DATABASE_URL": { "type": "token", "value": { "googleCloudSecret": "projects/<id>/secrets/postgres-connection-string/versions/latest" } }, "REDIS_URL": { "type": "token", "value": { "googleCloudSecret": "projects/<id>/secrets/redis-connection-string/versions/latest" } } } }Additionally, folded the
@sourcebot/crypto@sourcebot/loggerand@sourcebot/errorpackages into the shared package.Summary by CodeRabbit
Release Notes
New Features
Chores