refactor: unify environment variable structure and improve author resolution logic#359
refactor: unify environment variable structure and improve author resolution logic#359
Conversation
…olution logic - Replace `apiBaseUrl` references with `iamBaseUrl`, `cogitBaseUrl`, and `llmBaseUrl` for consistent base URL handling. - Simplify and centralize commit author detection logic with `resolveAuthor` method, including better fallback mechanisms. - Adjust OIDC URLs to derive dynamically from `iamBaseUrl`, removing redundant variables. - Update transport handlers, services, and configuration logic to align with refactored base URL structure. - Refine signing key CRUD operations and CLI commands for enhanced reliability.
…d adjust OIDC fallback URLs
|
Claude finished @leehpham's task in 2m 45s —— View job PR Review
SummaryScore: 4 / 5 This is a clean, well-reasoned refactor. Collapsing 6 env vars into 3 base URL vars and deriving OIDC paths programmatically is the right call — it eliminates an entire class of misconfiguration bugs where individual OIDC URLs could drift out of sync with the IAM base. What's good
Issues foundCorrectness — trailing slash normalization (3 inline comments) Testing — missing required-var coverage (1 inline comment on Nitpick — dual semantics on Non-issues
|
|
@hieuntg81 please go through the feedback from Claude Code first. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }) | ||
| export const getCurrentConfig = (): EnvironmentConfig => { | ||
| const iamBaseUrl = readRequiredEnv('BRV_IAM_BASE_URL') | ||
| const oidcBase = `${iamBaseUrl}/api/v1/oidc` |
There was a problem hiding this comment.
suggestion (correctness): If BRV_IAM_BASE_URL is set with a trailing slash (e.g. http://localhost:3000/), all derived OIDC URLs will contain a double slash (http://localhost:3000//api/v1/oidc/authorize). Consider normalizing the value:
| const oidcBase = `${iamBaseUrl}/api/v1/oidc` | |
| const oidcBase = `${iamBaseUrl.replace(/\/$/, '')}/api/v1/oidc` |
Same protection would be good for cogitBaseUrl and llmBaseUrl in feature-handlers.ts.
| const userService = new HttpUserService({apiBaseUrl: envConfig.apiBaseUrl}) | ||
| const teamService = new HttpTeamService({apiBaseUrl: envConfig.apiBaseUrl}) | ||
| const spaceService = new HttpSpaceService({apiBaseUrl: envConfig.apiBaseUrl}) | ||
| const iamApiV1 = `${envConfig.iamBaseUrl}/api/v1` |
There was a problem hiding this comment.
suggestion (correctness): Same trailing-slash hazard here. If iamBaseUrl ends with /, iamApiV1 becomes http://localhost:3000//api/v1. Strip before concatenating:
| const iamApiV1 = `${envConfig.iamBaseUrl}/api/v1` | |
| const iamApiV1 = `${envConfig.iamBaseUrl.replace(/\/$/, '')}/api/v1` |
| const contextFileReader = new FileContextFileReader() | ||
| const cogitPushService = new HttpCogitPushService({apiBaseUrl: envConfig.cogitApiBaseUrl}) | ||
| const cogitPullService = new HttpCogitPullService({apiBaseUrl: envConfig.cogitApiBaseUrl}) | ||
| const cogitApiV1 = `${envConfig.cogitBaseUrl}/api/v1` |
There was a problem hiding this comment.
suggestion (correctness): Same trailing-slash concern for cogitBaseUrl:
| const cogitApiV1 = `${envConfig.cogitBaseUrl}/api/v1` | |
| const cogitApiV1 = `${envConfig.cogitBaseUrl.replace(/\/$/, '')}/api/v1` |
| @@ -125,11 +123,11 @@ describe('Environment Configuration', () => { | |||
|
|
|||
| it('should throw when a required env var is missing', async () => { | |||
There was a problem hiding this comment.
suggestion (testing): Only BRV_IAM_BASE_URL is tested as a missing required var. The other four required vars (BRV_COGIT_BASE_URL, BRV_GIT_REMOTE_BASE_URL, BRV_LLM_BASE_URL, BRV_WEB_APP_URL) have no coverage for the "missing → throws" path. Consider parameterizing:
for (const key of ['BRV_COGIT_BASE_URL', 'BRV_GIT_REMOTE_BASE_URL', 'BRV_LLM_BASE_URL', 'BRV_WEB_APP_URL']) {
it(`should throw when ${key} is missing`, async () => {
delete process.env[key]
const {getCurrentConfig} = await import(`../../../src/server/config/environment.js?t=${Date.now()}`)
expect(() => getCurrentConfig()).to.throw(`Missing required environment variable: ${key}`)
})
}Not a blocker, but improves safety net for future env-var changes.
| } | ||
| } | ||
|
|
||
| export const getGitRemoteBaseUrl = (): string => |
There was a problem hiding this comment.
nitpick (consistency): getGitRemoteBaseUrl() silently falls back to 'https://byterover.dev' when the env var is absent, but getCurrentConfig() calls readRequiredEnv('BRV_GIT_REMOTE_BASE_URL') which throws on the same var. These two functions have divergent semantics for the same key. This predates the PR, but since the file was touched it's worth tracking — a caller using getGitRemoteBaseUrl() could silently get the production default while getCurrentConfig() would hard-fail in the same environment.
2 participants
Summary
authorize,token,issuer) are always derived from the IAM base, and API version paths (/api/v1) are baked into env vars instead of being appended at point of use.BRV_API_BASE_URL,BRV_AUTHORIZATION_URL,BRV_ISSUER_URL,BRV_TOKEN_URL?BRV_IAM_BASE_URLBRV_COGIT_API_BASE_URL?BRV_COGIT_BASE_URLBRV_LLM_API_BASE_URL?BRV_LLM_BASE_URLauthorizationUrl,issuerUrl,tokenUrl) are now derived fromiamBaseUrlinsidegetCurrentConfig()/api/v1) are appended at point of use infeature-handlers.tsEnvironmentConfigtype, all consumers, and testsType of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)N/A
Test plan
test/unit/config/environment.test.tsBRV_IAM_BASE_URLUser-visible changes
.env.exampleupdated ? developers must update their.env.development/.env.productionfiles:BRV_API_BASE_URL=http://localhost:3000/api/v1BRV_IAM_BASE_URL=http://localhost:3000BRV_AUTHORIZATION_URL=http://localhost:3000/api/v1/oidc/authorizeBRV_ISSUER_URL=http://localhost:3000/api/v1/oidcBRV_TOKEN_URL=http://localhost:3000/api/v1/oidc/tokenBRV_COGIT_API_BASE_URL=http://localhost:3001/api/v1BRV_COGIT_BASE_URL=http://localhost:3001BRV_LLM_API_BASE_URL=http://localhost:3002BRV_LLM_BASE_URL=http://localhost:3002Evidence
TypeScript type check and all environment config tests pass after changes.
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
.env.development/.env.productionfiles will get startup errors after pulling this change.Missing required environment variable: BRV_IAM_BASE_URL)..env.exampleserves as reference.