fix: config consistency refactor

[devthejo]

No description provided.

[revu-bot-dev]

Overview

This refactor centralizes runtime configuration and wires it into server startup. The direction is solid, but there are a few issues that could cause runtime failures and configuration inconsistencies.

File	Lines	Severity	Issue	Action
src/index.ts	6	CRITICAL	Importing .ts extension in ESM/Node runtime may fail	Use .js extension to match built output and existing pattern
src/core/utils/runtime-config.ts	1–3	CRITICAL	Imports use .ts extensions; inconsistent with repo’s .js imports	Switch to .js extensions for runtime imports and type-only imports
src/core/utils/runtime-config.ts	~79	IMPORTANT	LLM_PROVIDER env var not honored (docs claim it is)	Read and validate process.env.LLM_PROVIDER before app config
src/core/utils/runtime-config.ts	~155–161	IMPORTANT	REDIS_DB parsing inconsistent; may allow negatives/NaN	Reuse parseNumber and preserve undefined fallback
src/core/utils/runtime-config.ts	~189	IMPORTANT	Port parsing permits floats/out-of-range	Validate integer 0–65535, else fail back to default
src/index.ts	~13–16	CRITICAL	Missing early validation of required GitHub App secrets	Fail fast with clear error before initializing Probot
src/core/utils/runtime-config.ts	~214, ~238	IMPORTANT	Public functions lack docstrings (guideline 4)	Add concise JSDoc for exported functions

Additionally, please add tests for precedence (env > config > defaults), boolean parsing (including invalid values), and boundary checks (ports, Redis DB). This is critical code, so aim for high coverage per guidelines.

[revu-bot-dev]

An error occurred: 400 Invalid schema for response_format 'code_review': In context=(), 'additionalProperties' is required to be supplied and to be false.

Revu logs

[revu-bot-dev]

An error occurred: 400 Invalid schema for response_format 'code_review': In context=(), 'additionalProperties' is required to be supplied and to be false.

Revu logs

[revu-bot-dev]

An error occurred: 400 Invalid schema for response_format 'code_review': In context=('properties', 'comments', 'items'), 'required' is required to be supplied and to be an array including every key in properties. Missing 'start_line'.

Revu logs

[revu-bot-dev]

Overall PR Summary

This refactor centralizes environment/config handling into a RuntimeConfig layer and updates senders, discussion handling, rate limiting, and retry logic to consume it. The direction is solid and improves consistency and testability.

Key points:

• LLM provider/model, prompt cache, discussion limits, Redis, and retry behavior are now resolved in one place.
• config-loader is simplified to config.json + defaults, with env overrides moved to runtime-config.
• Critical paths (discussion handler, senders, rate limiting, retry) now depend on runtime config instead of ad‑hoc process.env reads.

Main issues to address:

• [IMPORTANT] getRuntimeConfigSync can silently ignore config.json if called before getRuntimeConfig, which is easy to do in synchronous modules and can lead to confusing behavior.
• [IMPORTANT] rate-limit.ts and retry.ts capture a runtime snapshot at module load; changes to env or config after first import (including tests that rely on _resetRuntimeConfigCacheForTests) will not be reflected.
• [IMPORTANT] build-discussion-prompt-segments still has its own DEFAULT_MAX_CHARS_PER_FILE/MAX_TOTAL_CHARS and a small bug where MAX_DIFF_CHARS uses input.maxTotalChars instead of the already-resolved MAX_TOTAL_CHARS, which can desync behavior from the runtime config.
• [MINOR] parseBooleanEnv logs a generic warning without the env var name, which can make debugging noisy configs harder.

Addressing these will make the new runtime config layer more robust and predictable.

[revu-bot-dev]

An error occurred: Error processing line comments: Unknown error

Revu logs

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[revu-bot-dev]

Overall PR Summary

This refactor centralizes environment/config handling into a RuntimeConfig layer and updates senders/handlers to consume it. The direction is solid and improves consistency and testability. Most changes are straightforward wiring, but there are a couple of behavioral shifts and one subtle testing/initialization concern worth addressing.

Key points:

• [IMPORTANT] getAppConfig no longer validates LLM_PROVIDER, but tests still expect a warning on invalid env; that behavior moved to runtime-config and is now untested.
• [IMPORTANT] rate-limit.ts and retry.ts capture runtime at module load via getRuntimeConfigSync, which can diverge from the async-initialized runtime (and from config.json) in some startup orders.
• [MINOR] build-discussion-prompt-segments now hardcodes DEFAULT_MAX_CHARS_PER_FILE and relies on the caller to pass runtime values; this is fine but could use a small comment to avoid future drift.

Addressing these will keep behavior consistent and avoid hard-to-debug config discrepancies.

[revu-bot-dev]

[IMPORTANT] getRuntimeConfigSync can diverge from async-initialized config

• Why it matters:

  • getRuntimeConfigSync builds a runtime config from DEFAULT_APP_CONFIG + env only when cachedRuntimeConfig is null.
  • rate-limit.ts and retry.ts call getRuntimeConfigSync() at module load, before getRuntimeConfig() is necessarily called.
  • If config.json provides overrides (e.g., repliesPerWindow, rateWindowSeconds, pRetryRetries, redis*), those will be ignored in any process where a sync consumer is imported first. This is subtle and environment/order dependent.

• Proposed change:

  • Make the divergence explicit and safer by either:
    1. Fail-fast when getRuntimeConfigSync is called before async initialization in non-test environments, or
    2. Ensure getRuntimeConfig() is always called during startup before any sync modules are imported (and document this contract clearly).

A fail-fast variant could look like:

export function getRuntimeConfigSync(): RuntimeConfig {
  if (!cachedRuntimeConfig) {
    const isTest =
      process.env.NODE_ENV === 'test' || process.env.VITEST_WORKER_ID != null
    if (!isTest) {
      throw new Error(
        'getRuntimeConfigSync() called before async initialization. Call getRuntimeConfig() at startup first.'
      )
    }
    const appConfig: RevuAppConfig = { ...DEFAULT_APP_CONFIG }
    cachedRuntimeConfig = buildRuntimeConfig(appConfig)
  }
  return cachedRuntimeConfig
}

This keeps the convenient test path while preventing silent config drift in production.

Suggested change

	export function getRuntimeConfigSync(): RuntimeConfig {
	if (!cachedRuntimeConfig) {
	// Fallback to building a runtime config from defaults + env only.
	// This avoids requiring an async initialization step in purely
	// synchronous modules. Config from config.json will not be
	// reflected in this rare path.
	const appConfig: RevuAppConfig = { ...DEFAULT_APP_CONFIG }
	cachedRuntimeConfig = buildRuntimeConfig(appConfig)
	}
	return cachedRuntimeConfig
	}
	export function getRuntimeConfigSync(): RuntimeConfig {
	if (!cachedRuntimeConfig) {
	const isTest =
	process.env.NODE_ENV === 'test' || process.env.VITEST_WORKER_ID != null
	if (!isTest) {
	throw new Error(
	'getRuntimeConfigSync() called before async initialization. Call getRuntimeConfig() at startup first.'
	)
	}
	// In tests, fall back to building a runtime config from defaults + env only.
	// Config from config.json will not be reflected in this path.
	const appConfig: RevuAppConfig = { ...DEFAULT_APP_CONFIG }
	cachedRuntimeConfig = buildRuntimeConfig(appConfig)
	}
	return cachedRuntimeConfig
	}

[devthejo]

Maybe we should do DI for config, WDYT ?

[revu-bot-dev]

DI for config would be a cleaner long-term direction, especially to avoid the global singleton / load-order issues.

Given the current codebase, I’d suggest an incremental step rather than full-blown DI everywhere:

• Keep runtime-config as the central builder, but:
  • Stop calling getRuntimeConfigSync() at module scope (e.g. in rate-limit.ts, retry.ts).
  • Instead, inject RuntimeConfig from the composition root (index.ts / webhook handler) into those utilities, or at least into the functions that need it.
• Once that’s in place, we can gradually move callers to accept a RuntimeConfig (or narrower typed subset) parameter, making the remaining global accessor mostly a legacy compatibility layer.

If you’re open to it, I can sketch how rate-limit.ts and retry.ts would look with a simple, constructor-style DI first.