Skip to content

refactor: unify environment variable structure and improve author resolution logic #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hieuntg81 wants to merge 3 commits into from
Closed

refactor: unify environment variable structure and improve author resolution logic #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa6353b
refactor: unify environment variable structure and improve author res…
hieuntg81 Apr 11, 2026
85756d4
refactor: update auth config tests to reflect `iamBaseUrl` changes an…
hieuntg81 Apr 11, 2026
9e9d0af
Merge branch 'main' into refactor/env-var-consolidation-2
leehpham Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,8 @@
# Copy this file to .env.development (for dev) or .env.production (for production)
# and fill in the values for your environment.

BRV_API_BASE_URL=http://localhost:3000/api/v1
BRV_AUTHORIZATION_URL=http://localhost:3000/api/v1/oidc/authorize
BRV_COGIT_API_BASE_URL=http://localhost:3001/api/v1
BRV_IAM_BASE_URL=http://localhost:3000
BRV_COGIT_BASE_URL=http://localhost:3001
BRV_GIT_REMOTE_BASE_URL=http://localhost:8080
BRV_ISSUER_URL=http://localhost:3000/api/v1/oidc
BRV_LLM_API_BASE_URL=http://localhost:3002
BRV_TOKEN_URL=http://localhost:3000/api/v1/oidc/token
BRV_LLM_BASE_URL=http://localhost:3002
BRV_WEB_APP_URL=http://localhost:8080
43 changes: 27 additions & 16 deletions src/server/config/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,22 @@ export const ENVIRONMENT: Environment = isEnvironment(envValue) ? envValue : 'de

/**
* Environment-specific configuration.
*
* Base URL vars (BRV_IAM_BASE_URL, BRV_COGIT_BASE_URL, BRV_LLM_BASE_URL)
* store only the root domain (e.g., http://localhost:8080).
* API version paths (/api/v1, /api/v3) are appended at the point of use.
*
* OIDC URLs are derived from iamBaseUrl; no separate env vars needed.
*/
type EnvironmentConfig = {
apiBaseUrl: string
authorizationUrl: string
clientId: string
cogitApiBaseUrl: string
cogitBaseUrl: string
gitRemoteBaseUrl: string
hubRegistryUrl: string
iamBaseUrl: string
issuerUrl: string
llmApiBaseUrl: string
llmBaseUrl: string
scopes: string[]
tokenUrl: string
webAppUrl: string
Expand All @@ -51,19 +57,24 @@ const readRequiredEnv = (name: string): string => {
return value
}

export const getCurrentConfig = (): EnvironmentConfig => ({
apiBaseUrl: readRequiredEnv('BRV_API_BASE_URL'),
authorizationUrl: readRequiredEnv('BRV_AUTHORIZATION_URL'),
clientId: DEFAULTS.clientId,
cogitApiBaseUrl: readRequiredEnv('BRV_COGIT_API_BASE_URL'),
gitRemoteBaseUrl: readRequiredEnv('BRV_GIT_REMOTE_BASE_URL'),
hubRegistryUrl: DEFAULTS.hubRegistryUrl,
issuerUrl: readRequiredEnv('BRV_ISSUER_URL'),
llmApiBaseUrl: readRequiredEnv('BRV_LLM_API_BASE_URL'),
scopes: [...DEFAULTS.scopes[ENVIRONMENT]],
tokenUrl: readRequiredEnv('BRV_TOKEN_URL'),
webAppUrl: readRequiredEnv('BRV_WEB_APP_URL'),
})
export const getCurrentConfig = (): EnvironmentConfig => {
const iamBaseUrl = readRequiredEnv('BRV_IAM_BASE_URL')
const oidcBase = `${iamBaseUrl}/api/v1/oidc`
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.


return {
authorizationUrl: `${oidcBase}/authorize`,
clientId: DEFAULTS.clientId,
cogitBaseUrl: readRequiredEnv('BRV_COGIT_BASE_URL'),
gitRemoteBaseUrl: readRequiredEnv('BRV_GIT_REMOTE_BASE_URL'),
hubRegistryUrl: DEFAULTS.hubRegistryUrl,
iamBaseUrl,
issuerUrl: oidcBase,
llmBaseUrl: readRequiredEnv('BRV_LLM_BASE_URL'),
scopes: [...DEFAULTS.scopes[ENVIRONMENT]],
tokenUrl: `${oidcBase}/token`,
webAppUrl: readRequiredEnv('BRV_WEB_APP_URL'),
}
}

export const getGitRemoteBaseUrl = (): string =>
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

process.env.BRV_GIT_REMOTE_BASE_URL ?? 'https://byterover.dev'
Expand Down
2 changes: 1 addition & 1 deletion src/server/infra/daemon/agent-process.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ async function start(): Promise<void> {

const envConfig = getCurrentConfig()
const agentConfig = {
apiBaseUrl: envConfig.llmApiBaseUrl,
apiBaseUrl: envConfig.llmBaseUrl,
fileSystem: {allowedPaths: ['.', ...sharedAllowedPaths], workingDirectory: projectPath},
llm: {
maxIterations: 10,
Expand Down
12 changes: 7 additions & 5 deletions src/server/infra/process/feature-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,10 @@ export async function setupFeatureHandlers({
const envConfig = getCurrentConfig()
const tokenStore = createTokenStore()
const projectConfigStore = new ProjectConfigStore()
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`
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (correctness): Same trailing-slash hazard here. If iamBaseUrl ends with /, iamApiV1 becomes http://localhost:3000//api/v1. Strip before concatenating:

Suggested change
const iamApiV1 = `${envConfig.iamBaseUrl}/api/v1`
const iamApiV1 = `${envConfig.iamBaseUrl.replace(/\/$/, '')}/api/v1`

const userService = new HttpUserService({apiBaseUrl: iamApiV1})
const teamService = new HttpTeamService({apiBaseUrl: iamApiV1})
const spaceService = new HttpSpaceService({apiBaseUrl: iamApiV1})

// Auth handler requires async OIDC discovery
const discoveryService = new OidcDiscoveryService()
Expand Down Expand Up @@ -145,8 +146,9 @@ export async function setupFeatureHandlers({
const contextTreeWriterService = new FileContextTreeWriterService({snapshotService: contextTreeSnapshotService})
const contextTreeMerger = new FileContextTreeMerger({snapshotService: contextTreeSnapshotService})
const contextFileReader = new FileContextFileReader()
const cogitPushService = new HttpCogitPushService({apiBaseUrl: envConfig.cogitApiBaseUrl})
const cogitPullService = new HttpCogitPullService({apiBaseUrl: envConfig.cogitApiBaseUrl})
const cogitApiV1 = `${envConfig.cogitBaseUrl}/api/v1`
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (correctness): Same trailing-slash concern for cogitBaseUrl:

Suggested change
const cogitApiV1 = `${envConfig.cogitBaseUrl}/api/v1`
const cogitApiV1 = `${envConfig.cogitBaseUrl.replace(/\/$/, '')}/api/v1`

const cogitPushService = new HttpCogitPushService({apiBaseUrl: cogitApiV1})
const cogitPullService = new HttpCogitPullService({apiBaseUrl: cogitApiV1})

// ConnectorManager factory — creates per-project instances since constructor binds to projectRoot
const fileService = new FsFileService()
Expand Down
2 changes: 1 addition & 1 deletion src/server/infra/transport/handlers/config-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class ConfigHandler {
this.transport.onRequest<void, ConfigGetEnvironmentResponse>(ConfigEvents.GET_ENVIRONMENT, () => {
const config = getCurrentConfig()
return {
apiBaseUrl: config.apiBaseUrl,
iamBaseUrl: config.iamBaseUrl,
isDevelopment: isDevelopment(),
webAppUrl: config.webAppUrl,
}
Expand Down
2 changes: 1 addition & 1 deletion src/shared/transport/events/config-events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export const ConfigEvents = {
} as const

export interface ConfigGetEnvironmentResponse {
apiBaseUrl: string
iamBaseUrl: string
isDevelopment: boolean
webAppUrl: string
}
Expand Down
15 changes: 6 additions & 9 deletions test/unit/config/auth.config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,10 @@ describe('Auth Configuration', () => {
let consoleWarnStub: sinon.SinonStub

const ENV_VARS = {
BRV_API_BASE_URL: 'https://api.test',
BRV_AUTHORIZATION_URL: 'https://auth.test/authorize',
BRV_COGIT_API_BASE_URL: 'https://cogit.test',
BRV_COGIT_BASE_URL: 'https://cogit.test',
BRV_GIT_REMOTE_BASE_URL: 'https://cogit-git.test',
BRV_ISSUER_URL: 'https://issuer.test',
BRV_LLM_API_BASE_URL: 'https://llm.test',
BRV_TOKEN_URL: 'https://auth.test/token',
BRV_IAM_BASE_URL: 'https://iam.test',
BRV_LLM_BASE_URL: 'https://llm.test',
BRV_WEB_APP_URL: 'https://app.test',
}

Expand Down Expand Up @@ -109,8 +106,8 @@ describe('Auth Configuration', () => {
it('should fallback to env var URLs when discovery fails', async () => {
const config = await getAuthConfig(discoveryService)

expect(config.authorizationUrl).to.equal('https://auth.test/authorize')
expect(config.tokenUrl).to.equal('https://auth.test/token')
expect(config.authorizationUrl).to.equal('https://iam.test/api/v1/oidc/authorize')
expect(config.tokenUrl).to.equal('https://iam.test/api/v1/oidc/token')
})

it('should still use environment-specific clientId and scopes in fallback', async () => {
Expand All @@ -123,7 +120,7 @@ describe('Auth Configuration', () => {
})

it('should throw on network errors', async () => {
discoveryService.discover = stub().rejects(new Error('getaddrinfo ENOTFOUND issuer.test'))
discoveryService.discover = stub().rejects(new Error('getaddrinfo ENOTFOUND iam.test'))

try {
await getAuthConfig(discoveryService)
Expand Down
26 changes: 12 additions & 14 deletions test/unit/config/environment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@ import {expect} from 'chai'

describe('Environment Configuration', () => {
const ENV_VARS = {
BRV_API_BASE_URL: 'https://api.test',
BRV_AUTHORIZATION_URL: 'https://auth.test/authorize',
BRV_COGIT_API_BASE_URL: 'https://cogit.test',
BRV_COGIT_BASE_URL: 'https://cogit.test',
BRV_GIT_REMOTE_BASE_URL: 'https://cogit-git.test',
BRV_ISSUER_URL: 'https://issuer.test',
BRV_LLM_API_BASE_URL: 'https://llm.test',
BRV_TOKEN_URL: 'https://auth.test/token',
BRV_IAM_BASE_URL: 'https://iam.test',
BRV_LLM_BASE_URL: 'https://llm.test',
BRV_WEB_APP_URL: 'https://app.test',
}

Expand Down Expand Up @@ -78,12 +75,13 @@ describe('Environment Configuration', () => {
const {getCurrentConfig} = await import(`../../../src/server/config/environment.js?t=${Date.now()}`)
const config = getCurrentConfig()

expect(config.apiBaseUrl).to.equal('https://api.test')
expect(config.authorizationUrl).to.equal('https://auth.test/authorize')
expect(config.cogitApiBaseUrl).to.equal('https://cogit.test')
expect(config.issuerUrl).to.equal('https://issuer.test')
expect(config.llmApiBaseUrl).to.equal('https://llm.test')
expect(config.tokenUrl).to.equal('https://auth.test/token')
expect(config.iamBaseUrl).to.equal('https://iam.test')
expect(config.authorizationUrl).to.equal('https://iam.test/api/v1/oidc/authorize')
expect(config.cogitBaseUrl).to.equal('https://cogit.test')
expect(config.gitRemoteBaseUrl).to.equal('https://cogit-git.test')
expect(config.issuerUrl).to.equal('https://iam.test/api/v1/oidc')
expect(config.llmBaseUrl).to.equal('https://llm.test')
expect(config.tokenUrl).to.equal('https://iam.test/api/v1/oidc/token')
expect(config.webAppUrl).to.equal('https://app.test')
})

Expand Down Expand Up @@ -125,11 +123,11 @@ describe('Environment Configuration', () => {

it('should throw when a required env var is missing', async () => {
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

delete process.env.BRV_ENV
delete process.env.BRV_API_BASE_URL
delete process.env.BRV_IAM_BASE_URL

const {getCurrentConfig} = await import(`../../../src/server/config/environment.js?t=${Date.now()}`)

expect(() => getCurrentConfig()).to.throw('Missing required environment variable: BRV_API_BASE_URL')
expect(() => getCurrentConfig()).to.throw('Missing required environment variable: BRV_IAM_BASE_URL')
})
})
})
Loading