Conversation
Contributor
WalkthroughThis PR consolidates bootstrap/env data into ChangesConfiguration System Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/util/TestServerConfig.ts (1)
5-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove throwing stubs that override working base class methods.
TestServerConfigconstructor passes real values tosuper()andServerConfigimplements all these methods with working logic. The throwing overrides forturnstileSiteKey(),jwtAudience(),jwtIssuer(),turnIntervalMs(),gameCreationRate(),numWorkers(),workerIndex(),workerPath(),workerPort(),workerPortByIndex(), andenv()defeat the refactor. Any test that calls these will throw at runtime, even though the parent class provides correct implementations.Additionally,
getRandomPublicGameModifiers()andsupportsCompactMapForTeams()are not on the baseServerConfigclass—they areTestServerConfig-only helpers. RemovesupportsCompactMapForTeams()since it is defined as a private method onMapPlaylist, not on the config. KeepgetRandomPublicGameModifiers()only if tests depend on it; otherwise remove it as well.🛠️ Suggested fix
export class TestServerConfig extends ServerConfig { constructor() { super({ gameEnv: GameEnv.Dev, numWorkers: 1, turnstileSiteKey: "", jwtAudience: "localhost", }); } - turnstileSiteKey(): string { - throw new Error("Method not implemented."); - } - jwtAudience(): string { - throw new Error("Method not implemented."); - } - jwtIssuer(): string { - throw new Error("Method not implemented."); - } - turnIntervalMs(): number { - throw new Error("Method not implemented."); - } - gameCreationRate(): number { - throw new Error("Method not implemented."); - } - async lobbyMaxPlayers(): Promise<number> { - throw new Error("Method not implemented."); - } - numWorkers(): number { - throw new Error("Method not implemented."); - } - workerIndex(gameID: GameID): number { - throw new Error("Method not implemented."); - } - workerPath(gameID: GameID): string { - throw new Error("Method not implemented."); - } - workerPort(gameID: GameID): number { - throw new Error("Method not implemented."); - } - workerPortByIndex(workerID: number): number { - throw new Error("Method not implemented."); - } - env(): GameEnv { - throw new Error("Method not implemented."); - } getRandomPublicGameModifiers(): PublicGameModifiers { return { isCompact: false, isRandomSpawn: false, isCrowded: false, isHardNations: false, isAlliancesDisabled: false, }; } - async supportsCompactMapForTeams(): Promise<boolean> { - throw new Error("Method not implemented."); - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/util/TestServerConfig.ts` around lines 5 - 62, Remove the stub overrides that throw in TestServerConfig so the class inherits the working implementations from ServerConfig: delete the methods turnstileSiteKey(), jwtAudience(), jwtIssuer(), turnIntervalMs(), gameCreationRate(), numWorkers(), workerIndex(), workerPath(), workerPort(), workerPortByIndex(), and env(); keep the constructor as-is (it already calls super with real values). Also remove supportsCompactMapForTeams() entirely (it belongs to MapPlaylist), and either keep or remove getRandomPublicGameModifiers() based on whether tests rely on that helper—if no tests use it, remove it too.tests/server/Archive.test.ts (1)
3-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMock
ServerEnvto prevent test from depending on environment variables.The test mocks
getServerConfigbutsrc/server/Archive.tscallsServerEnv.apiKey(),ServerEnv.gitCommit(),ServerEnv.subdomain(), andServerEnv.domain()directly. SinceServerEnvis not mocked, the test will use real environment variables (process.env.API_KEY,process.env.GIT_COMMIT, etc.). Add a mock forServerEnvso the test does not depend on the test environment's actual env vars.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/Archive.test.ts` around lines 3 - 11, The test currently mocks getServerConfig but not ServerEnv, and src/server/Archive.ts calls ServerEnv.apiKey(), ServerEnv.gitCommit(), ServerEnv.subdomain(), and ServerEnv.domain(), causing tests to read real env vars; add a vi.mock(...) for ServerEnv that exports apiKey, gitCommit, subdomain, and domain functions returning fixed test values (e.g., "test-key", "DEV", "test", "test") so Archive.test.ts does not depend on process.env and the mocked getServerConfig remains consistent with ServerEnv.
🧹 Nitpick comments (10)
tests/util/TestConfig.ts (1)
101-122: 💤 Low value
UseRealAttackLogicreaches intoConfig.prototypeto undoTestConfig's override — consider composition instead.
UseRealAttackLogic extends TestConfigonly to bypassTestConfig.attackLogicand fall back toConfig.attackLogic. That is a fragile inheritance pattern (calls break if the chain is reorganized). A small typed flag, or holding arealConfig: Configand delegating, would be easier to follow withoutprototype.call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/util/TestConfig.ts` around lines 101 - 122, The class UseRealAttackLogic currently subclasses TestConfig just to call Config.prototype.attackLogic via TestConfig.prototype override, which is fragile; change UseRealAttackLogic to use composition/delegation instead: remove inheritance from TestConfig, add a realConfig: Config instance (or a boolean flag on TestConfig) and implement attackLogic to delegate to realConfig.attackLogic(gm, attackTroops, attacker, defender, tileToConquer) (or check the flag and call super.attackLogic when false), referencing the existing UseRealAttackLogic.attackLogic, TestConfig, and Config.attackLogic symbols so the override no longer needs to reach into Config.prototype.src/core/configuration/Config.ts (4)
1-3: ⚖️ Poor tradeoffNew runtime dependency added inside
src/core/.This file pulls in
zodand usesfetchfor network I/O. Per the repo guideline,src/core/**/*.ts"must contain deterministic game simulation with pure TypeScript and no external dependencies".colord/josewere already present, so this PR is not the source of the drift, but the newServerConfig(withfetch+zodparsing of remote JWKS) is server infrastructure and would fit better undersrc/server/whileConfig(the gameplay class) stays insrc/core/. Worth at least keeping on the radar as a follow-up split.As per coding guidelines: "The
src/core/directory must contain deterministic game simulation with pure TypeScript and no external dependencies".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/Config.ts` around lines 1 - 3, This file introduces runtime/networking and a new dependency into src/core by adding ServerConfig that uses fetch and zod to parse remote JWKS; move the server-specific parts out of the deterministic core module by splitting ServerConfig (and any JWKS-fetching/Zod parsing logic) into a new file under src/server (e.g., ServerConfig.ts) and keep only pure gameplay-related definitions (e.g., the original Config class) in src/core/Config.ts; update imports/usages to reference the new ServerConfig location and ensure no fetch or zod imports remain in any file under src/core/.
108-108: ⚡ Quick win
publicKeyfield type does not allowundefined, but it can be undefined.
private publicKey: JWK;is never initialized, yetjwkPublicKey()reads it before assignment. UnderstrictPropertyInitialization/strictthis is a type lie — the truthy check on line 133 only works because the value isundefinedat runtime.Make the type honest:
🛠️ Suggested fix
- private publicKey: JWK; + private publicKey: JWK | undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/Config.ts` at line 108, The class field publicKey is declared as type JWK but can be undefined at runtime; update its declaration in Config to allow undefined (e.g., change type to JWK | undefined or mark it optional) and ensure jwkPublicKey() and any other consumers handle the undefined case explicitly (retain the existing truthy check or throw/return appropriately). Specifically modify the publicKey field in class Config and review the jwkPublicKey() method to rely on the corrected type rather than assuming initialization.
84-94: 💤 Low valueJWKS schema only accepts EdDSA — verify this matches your issuer.
The schema rejects RS256 and ES256 keys. Typical OAuth issuers (Cloudflare, Auth0, Okta) publish JWKS with RS256 (RSA) or ES256 (ECDSA P-256), not EdDSA. If you use an external issuer, this schema will fail. If your issuer is yours and only publishes EdDSA, this is fine. Otherwise, widen the schema to accept RS256 and ES256, and select the matching key instead of always taking
keys[0].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/Config.ts` around lines 84 - 94, The JwksSchema currently only accepts EdDSA/Ed25519 keys which will reject common issuer JWKS; update the schema (JwksSchema) to allow multiple key types/algorithms (e.g., RS256 with kty "RSA" and alg "RS256", ES256 with kty "EC"/crv "P-256"/alg "ES256", and EdDSA as before), and change any code that blindly uses keys[0] to instead search the parsed keys array for a key matching the token's "alg" or the expected kid (e.g., find by kid or alg in the JWKS result) and use that selected key for verification. Ensure the updated schema still requires at least one key and preserves type-safe parsing via the existing zod declaration (JwksSchema).
35-46: 💤 Low value
parseGameEnvsignature acceptsundefinedbut throws — make it consistent.The function signature is
parseGameEnv(value: string | undefined): GameEnv, but anyundefinedvalue falls through to the default case and throws an error. This is inconsistent with how other code in the codebase handlesGAME_ENV:
RenderHtml.ts:26usesprocess.env.GAME_ENV ?? "dev"Logger.ts:63usesprocess.env.GAME_ENV ?? "prod"Env.ts:62returnsgetEnv("GAME_ENV") ?? "dev"The function is also called directly in
ServerEnv.ts:5andConfigLoader.ts:63without providing a default, which means they will throw ifGAME_ENVis undefined.Choose one approach:
- Remove
undefinedfrom the parameter type and require callers to provide a default (e.g.,process.env.GAME_ENV ?? "dev"), or- Handle
undefinedexplicitly in the switch ascase undefined: return GameEnv.Dev;to match the common default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/Config.ts` around lines 35 - 46, The parseGameEnv function currently accepts undefined but throws; update parseGameEnv to explicitly handle undefined by adding a case undefined (or a guard) that returns GameEnv.Dev so callers like ServerEnv and ConfigLoader that pass through process.env.GAME_ENV without a default won’t throw; keep the signature as (value: string | undefined): GameEnv and ensure the default error remains for truly unsupported strings.src/client/Main.ts (1)
893-893: 💤 Low valueSimplify the type annotation.
The
Awaited<>wrapper is unnecessary sincegetServerConfig()now returnsServerConfigsynchronously (not a Promise).Awaited<T>for a non-Promise type just returnsT, so this works but is verbose.♻️ Simplify to remove Awaited wrapper
- config: Awaited<ReturnType<typeof getServerConfig>>, + config: ReturnType<typeof getServerConfig>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/Main.ts` at line 893, The type annotation for the parameter `config` in Main.ts is unnecessarily wrapped in `Awaited<ReturnType<typeof getServerConfig>>`; since `getServerConfig()` now returns `ServerConfig` synchronously, remove the `Awaited<>` and simplify the type to `ReturnType<typeof getServerConfig>` or directly to `ServerConfig` (update the signature where `config: Awaited<ReturnType<typeof getServerConfig>>` appears, keeping the parameter name `config` and references to `getServerConfig` intact).src/core/configuration/ConfigLoader.ts (3)
22-29: 💤 Low value
getGameLogicConfigno longer needs to beasync.The body just returns
new Config(...)synchronously, so theasyncandPromise<Config>only force every caller into needlessawaits. If you'd rather keep the public signature stable (callers likeGameRunner.tsalreadyawaitit), that's fine, but a sync version is more honest about the contract.♻️ Drop the async
-export async function getGameLogicConfig( +export function getGameLogicConfig( serverConfig: ServerConfig, gameConfig: GameConfig, userSettings: UserSettings | null, isReplay: boolean = false, -): Promise<Config> { +): Config { return new Config(serverConfig, gameConfig, userSettings, isReplay); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/ConfigLoader.ts` around lines 22 - 29, getGameLogicConfig is declared async but constructs and returns a Config synchronously; change its signature to export function getGameLogicConfig(...): Config (remove async and Promise<Config>) and have it return new Config(...) directly; then update any callers (e.g., places like GameRunner.ts that currently `await getGameLogicConfig(...)`) to call it synchronously (remove the unnecessary `await`) or keep awaiting only if you choose to preserve the Promise contract for compatibility.
46-53: ⚡ Quick winTell the operator which bootstrap field is missing.
A single
"Missing BOOTSTRAP_CONFIG"for any of four fields makes diagnosing a misconfigured deployment harder than it needs to be. Logging the offending key(s) keeps it cheap and a lot friendlier in production.📝 Name the missing field
- if ( - bc.gameEnv === undefined || - bc.numWorkers === undefined || - bc.turnstileSiteKey === undefined || - bc.jwtAudience === undefined - ) { - throw new Error("Missing BOOTSTRAP_CONFIG"); - } + const missing = ( + ["gameEnv", "numWorkers", "turnstileSiteKey", "jwtAudience"] as const + ).filter((key) => bc[key] === undefined); + if (missing.length > 0) { + throw new Error( + `Missing BOOTSTRAP_CONFIG fields: ${missing.join(", ")}`, + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/ConfigLoader.ts` around lines 46 - 53, The current ConfigLoader.ts check throws a generic Error("Missing BOOTSTRAP_CONFIG") when any of bc.gameEnv, bc.numWorkers, bc.turnstileSiteKey, or bc.jwtAudience are undefined; change this to compute which specific keys are missing from the bc object (e.g., check each of bc.gameEnv, bc.numWorkers, bc.turnstileSiteKey, bc.jwtAudience), build a descriptive message listing those missing keys, and throw or log an error containing that message (update the throw in the ConfigLoader validation block so it includes the names of the offending fields).
35-40: 💤 Low valueBrowser fallback to
process.envis fragile.When
windowexists butBOOTSTRAP_CONFIGisundefined(e.g., a misconfigured deployment, or the script tag failing to run), this falls through tobuildFromProcessEnv(), which dereferencesprocess.env. Whether that works depends entirely on whether the bundler statically replacesprocess.env.*— if not, it throwsReferenceError: process is not definedand you lose the chance to surface a clearer message about the bootstrap script. A small guard makes the failure mode obvious.🛡️ Guard the browser fallback
- if (typeof window !== "undefined" && window.BOOTSTRAP_CONFIG !== undefined) { - cachedServerConfig = buildFromBootstrap(window.BOOTSTRAP_CONFIG); - } else { - cachedServerConfig = buildFromProcessEnv(); - } + if (typeof window !== "undefined") { + if (window.BOOTSTRAP_CONFIG === undefined) { + throw new Error( + "BOOTSTRAP_CONFIG missing on window; bootstrap script may have failed to load", + ); + } + cachedServerConfig = buildFromBootstrap(window.BOOTSTRAP_CONFIG); + } else { + cachedServerConfig = buildFromProcessEnv(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/configuration/ConfigLoader.ts` around lines 35 - 40, The current logic in ConfigLoader returns buildFromProcessEnv() whenever window exists but window.BOOTSTRAP_CONFIG is undefined, which can raise ReferenceError: process is not defined in browsers; update the branch around cachedServerConfig so that if typeof window !== "undefined" and window.BOOTSTRAP_CONFIG === undefined you do NOT call buildFromProcessEnv() directly — instead detect whether process is available (typeof process !== "undefined" && process.env) before calling buildFromProcessEnv(), and if not available produce a clear error or fallback (e.g., throw a descriptive Error or return a safe default) so callers of cachedServerConfig, buildFromBootstrap and buildFromProcessEnv see an explicit message when the bootstrap script is missing.src/server/Worker.ts (1)
38-38: 💤 Low valueModule-load-time
getServerConfig()can crash imports.Calling
getServerConfig()at the top level means any test or tool that transitively importssrc/server/Worker.tswill throw immediately whenNUM_WORKERS/TURNSTILE_SITE_KEY/JWT_AUDIENCEaren't set, since the new loader throws on missing env. It's safer to read it lazily insidestartWorker()(and passconfigdown) so the module is import-safe.♻️ Lazy initialization sketch
-const config = getServerConfig(); - -const workerId = parseInt(process.env.WORKER_ID ?? "0"); -const log = logger.child({ comp: `w_${workerId}` }); -const playlist = new MapPlaylist(); +const workerId = parseInt(process.env.WORKER_ID ?? "0"); +const log = logger.child({ comp: `w_${workerId}` }); +const playlist = new MapPlaylist(); export async function startWorker() { + const config = getServerConfig(); log.info(`Worker starting...`);(The
startMatchmakingPollinghelper would needconfigpassed in or read locally.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/Worker.ts` at line 38, getServerConfig() is being called at module load time (const config = getServerConfig()), which can throw when env vars like NUM_WORKERS / TURNSTILE_SITE_KEY / JWT_AUDIENCE are missing and break any import; move the call into startWorker() so config is read lazily and pass that config object into any helpers that need it (e.g., startMatchmakingPolling and other functions currently reading the top-level config), updating their signatures to accept the config parameter and ensuring all usages use the passed-in config rather than the removed module-level const.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/configuration/Config.ts`:
- Around line 132-145: The jwkPublicKey function must check HTTP errors and
avoid a concurrent-fetch race: first, before calling response.json(), verify
response.ok and throw a descriptive error including response.status/text so
non-JSON error pages are handled; second, introduce an in-flight promise (e.g.,
this.jwkPublicKeyPromise or similar) and assign the Promise returned by the
fetch/parse flow to it at the start so concurrent callers await the same
Promise; inside that Promise, parse JSON with JwksSchema.safeParse (use
z.prettifyError on parse failures), set this.publicKey on success and clear the
in-flight promise on both success and failure to avoid stuck state. Ensure
subsequent calls return this.publicKey immediately if present or await the
in-flight promise if set.
In `@src/core/configuration/Env.ts`:
- Around line 89-100: The new Env getters (NUM_WORKERS, TURNSTILE_SITE_KEY,
JWT_AUDIENCE, ALLOWED_FLARES) lack unit tests; add tests for the Env class that
verify each getter returns the expected value when the corresponding environment
variable is set, returns the default/undefined behavior when unset, and respects
precedence if multiple env sources are supported (e.g., process.env vs. a config
file loader used by getEnv). Create tests that set and restore process.env
around assertions, cover successful reads and fallback cases for each getter,
and add asserts for precedence and cleanup so these behaviors are enforced
before merging.
In `@src/core/worker/Worker.worker.ts`:
- Line 142: Add a unit test that covers worker initialization path which
constructs a ServerConfig from message.serverConfigValues and ensures those
values are forwarded to createGameRunner: mock or instantiate Worker.worker (or
the initialization entry function used in Worker.worker.ts), send/emit the init
message containing serverConfigValues, assert that new ServerConfig(...) is
constructed with those values (or that createGameRunner was called with an
equivalent ServerConfig instance), and verify createGameRunner receives the
expected config; reference ServerConfig, message.serverConfigValues,
createGameRunner, and the worker initialization logic in Worker.worker.ts when
writing the test.
In `@src/server/GamePreviewRoute.ts`:
- Around line 24-25: The host fallback builds an invalid "." when
req.get("host") is missing and ServerEnv.subdomain()/ServerEnv.domain() are
empty; update the host computation (the const host assignment) to use a safe
fallback like "localhost" if the combined
`${ServerEnv.subdomain()}.${ServerEnv.domain()}` is empty or equals "." (or if
req.get("host") is falsy), and make the same change where the host is computed
again (the other occurrence around the GamePreviewRoute file). Ensure you
normalize/trust the final host value before using it to build origins/URLs.
In `@src/server/Master.ts`:
- Line 17: The module-level call to getServerConfig() (assigned to config) runs
at import time and can throw before startMaster() runs; move the call into
startMaster() (or ensure getServerConfig() returns safe defaults) so config is
lazily initialized. Update Master.ts to remove the top-level const config =
getServerConfig() and instead call getServerConfig() inside the startMaster()
function (or guard with explicit defaults) and use that returned config for any
downstream calls; apply the same pattern in Worker.ts, OtelResource.ts, and
Archive.ts where getServerConfig() is currently called at module scope.
In `@src/server/RenderHtml.ts`:
- Around line 27-29: Replace the silent fallbacks in RenderHtml.ts for
NUM_WORKERS, TURNSTILE_SITE_KEY, and JWT_AUDIENCE with explicit validation that
throws at render time: add helpers like requireEnv(name) and requireIntEnv(name)
and use them to obtain numWorkers, turnstileSiteKey, and jwtAudience (instead of
parseInt(... ?? "") / ?? ""), so invalid or missing values raise descriptive
errors during render; this prevents downstream NaN/null values that break
ServerConfig.workerIndex (simpleHash(gameID) % numWorkers) and avoids empty
jwtAudience/turnstileSiteKey causing cryptic runtime failures.
In `@src/server/ServerEnv.ts`:
- Around line 29-31: The ServerEnv.apiKey() currently returns an empty string
when Env.API_KEY is unset which causes silent auth failures; change
ServerEnv.apiKey() to fail-fast by throwing a descriptive error (e.g., "API_KEY
is required") when Env.API_KEY is null/undefined/empty instead of returning "";
update the implementation in the ServerEnv.apiKey method to check Env.API_KEY
and throw if missing so callers receive a clear runtime exception immediately.
In `@src/server/WorkerMetrics.ts`:
- Around line 19-21: The code currently only conditionally adds the OTEL auth
header via ServerEnv.otelEnabled() but still always initializes the metrics
exporter, causing noisy/invalid export attempts; update WorkerMetrics.ts to wrap
the exporter setup (creation of headers, OTLPMetricExporter, MeterProvider,
PeriodicExportingMetricReader and the call to
MetricsAPI.global.setMeterProvider) in an if (ServerEnv.otelEnabled()) guard so
the exporter and readers are only created when otelEnabled() is true, and keep
the headers assignment inside that same guard; reference the symbols
ServerEnv.otelEnabled(), headers, OTLPMetricExporter, MeterProvider,
PeriodicExportingMetricReader, and MetricsAPI.global.setMeterProvider to find
and modify the code.
In `@tests/core/configuration/ConfigLoader.test.ts`:
- Around line 17-25: The beforeEach currently assigns original.* values back
into process.env which turns undefined into the string "undefined"; update the
teardown in the beforeEach that restores process.env.GAME_ENV,
process.env.NUM_WORKERS, process.env.TURNSTILE_SITE_KEY, and
process.env.JWT_AUDIENCE so that for each env var you delete process.env.VAR
when the corresponding original.VAR is undefined, otherwise reassign the
original value; keep the window.BOOTSTRAP_CONFIG reset and call
clearCachedServerConfig() as-is so nothing else changes.
---
Outside diff comments:
In `@tests/server/Archive.test.ts`:
- Around line 3-11: The test currently mocks getServerConfig but not ServerEnv,
and src/server/Archive.ts calls ServerEnv.apiKey(), ServerEnv.gitCommit(),
ServerEnv.subdomain(), and ServerEnv.domain(), causing tests to read real env
vars; add a vi.mock(...) for ServerEnv that exports apiKey, gitCommit,
subdomain, and domain functions returning fixed test values (e.g., "test-key",
"DEV", "test", "test") so Archive.test.ts does not depend on process.env and the
mocked getServerConfig remains consistent with ServerEnv.
In `@tests/util/TestServerConfig.ts`:
- Around line 5-62: Remove the stub overrides that throw in TestServerConfig so
the class inherits the working implementations from ServerConfig: delete the
methods turnstileSiteKey(), jwtAudience(), jwtIssuer(), turnIntervalMs(),
gameCreationRate(), numWorkers(), workerIndex(), workerPath(), workerPort(),
workerPortByIndex(), and env(); keep the constructor as-is (it already calls
super with real values). Also remove supportsCompactMapForTeams() entirely (it
belongs to MapPlaylist), and either keep or remove
getRandomPublicGameModifiers() based on whether tests rely on that helper—if no
tests use it, remove it too.
---
Nitpick comments:
In `@src/client/Main.ts`:
- Line 893: The type annotation for the parameter `config` in Main.ts is
unnecessarily wrapped in `Awaited<ReturnType<typeof getServerConfig>>`; since
`getServerConfig()` now returns `ServerConfig` synchronously, remove the
`Awaited<>` and simplify the type to `ReturnType<typeof getServerConfig>` or
directly to `ServerConfig` (update the signature where `config:
Awaited<ReturnType<typeof getServerConfig>>` appears, keeping the parameter name
`config` and references to `getServerConfig` intact).
In `@src/core/configuration/Config.ts`:
- Around line 1-3: This file introduces runtime/networking and a new dependency
into src/core by adding ServerConfig that uses fetch and zod to parse remote
JWKS; move the server-specific parts out of the deterministic core module by
splitting ServerConfig (and any JWKS-fetching/Zod parsing logic) into a new file
under src/server (e.g., ServerConfig.ts) and keep only pure gameplay-related
definitions (e.g., the original Config class) in src/core/Config.ts; update
imports/usages to reference the new ServerConfig location and ensure no fetch or
zod imports remain in any file under src/core/.
- Line 108: The class field publicKey is declared as type JWK but can be
undefined at runtime; update its declaration in Config to allow undefined (e.g.,
change type to JWK | undefined or mark it optional) and ensure jwkPublicKey()
and any other consumers handle the undefined case explicitly (retain the
existing truthy check or throw/return appropriately). Specifically modify the
publicKey field in class Config and review the jwkPublicKey() method to rely on
the corrected type rather than assuming initialization.
- Around line 84-94: The JwksSchema currently only accepts EdDSA/Ed25519 keys
which will reject common issuer JWKS; update the schema (JwksSchema) to allow
multiple key types/algorithms (e.g., RS256 with kty "RSA" and alg "RS256", ES256
with kty "EC"/crv "P-256"/alg "ES256", and EdDSA as before), and change any code
that blindly uses keys[0] to instead search the parsed keys array for a key
matching the token's "alg" or the expected kid (e.g., find by kid or alg in the
JWKS result) and use that selected key for verification. Ensure the updated
schema still requires at least one key and preserves type-safe parsing via the
existing zod declaration (JwksSchema).
- Around line 35-46: The parseGameEnv function currently accepts undefined but
throws; update parseGameEnv to explicitly handle undefined by adding a case
undefined (or a guard) that returns GameEnv.Dev so callers like ServerEnv and
ConfigLoader that pass through process.env.GAME_ENV without a default won’t
throw; keep the signature as (value: string | undefined): GameEnv and ensure the
default error remains for truly unsupported strings.
In `@src/core/configuration/ConfigLoader.ts`:
- Around line 22-29: getGameLogicConfig is declared async but constructs and
returns a Config synchronously; change its signature to export function
getGameLogicConfig(...): Config (remove async and Promise<Config>) and have it
return new Config(...) directly; then update any callers (e.g., places like
GameRunner.ts that currently `await getGameLogicConfig(...)`) to call it
synchronously (remove the unnecessary `await`) or keep awaiting only if you
choose to preserve the Promise contract for compatibility.
- Around line 46-53: The current ConfigLoader.ts check throws a generic
Error("Missing BOOTSTRAP_CONFIG") when any of bc.gameEnv, bc.numWorkers,
bc.turnstileSiteKey, or bc.jwtAudience are undefined; change this to compute
which specific keys are missing from the bc object (e.g., check each of
bc.gameEnv, bc.numWorkers, bc.turnstileSiteKey, bc.jwtAudience), build a
descriptive message listing those missing keys, and throw or log an error
containing that message (update the throw in the ConfigLoader validation block
so it includes the names of the offending fields).
- Around line 35-40: The current logic in ConfigLoader returns
buildFromProcessEnv() whenever window exists but window.BOOTSTRAP_CONFIG is
undefined, which can raise ReferenceError: process is not defined in browsers;
update the branch around cachedServerConfig so that if typeof window !==
"undefined" and window.BOOTSTRAP_CONFIG === undefined you do NOT call
buildFromProcessEnv() directly — instead detect whether process is available
(typeof process !== "undefined" && process.env) before calling
buildFromProcessEnv(), and if not available produce a clear error or fallback
(e.g., throw a descriptive Error or return a safe default) so callers of
cachedServerConfig, buildFromBootstrap and buildFromProcessEnv see an explicit
message when the bootstrap script is missing.
In `@src/server/Worker.ts`:
- Line 38: getServerConfig() is being called at module load time (const config =
getServerConfig()), which can throw when env vars like NUM_WORKERS /
TURNSTILE_SITE_KEY / JWT_AUDIENCE are missing and break any import; move the
call into startWorker() so config is read lazily and pass that config object
into any helpers that need it (e.g., startMatchmakingPolling and other functions
currently reading the top-level config), updating their signatures to accept the
config parameter and ensuring all usages use the passed-in config rather than
the removed module-level const.
In `@tests/util/TestConfig.ts`:
- Around line 101-122: The class UseRealAttackLogic currently subclasses
TestConfig just to call Config.prototype.attackLogic via TestConfig.prototype
override, which is fragile; change UseRealAttackLogic to use
composition/delegation instead: remove inheritance from TestConfig, add a
realConfig: Config instance (or a boolean flag on TestConfig) and implement
attackLogic to delegate to realConfig.attackLogic(gm, attackTroops, attacker,
defender, tileToConquer) (or check the flag and call super.attackLogic when
false), referencing the existing UseRealAttackLogic.attackLogic, TestConfig, and
Config.attackLogic symbols so the override no longer needs to reach into
Config.prototype.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69138720-89fb-4b25-bbfe-31e427e473fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
index.htmlpackage.jsonsrc/client/AccountModal.tssrc/client/ClientGameRunner.tssrc/client/GameModeSelector.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LobbySocket.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/components/CopyButton.tssrc/core/AssetUrls.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/core/worker/WorkerMessages.tssrc/server/Archive.tssrc/server/GamePreviewRoute.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/OtelResource.tssrc/server/RenderHtml.tssrc/server/ServerEnv.tssrc/server/Turnstile.tssrc/server/Worker.tssrc/server/WorkerMetrics.tssrc/server/jwt.tstests/client/clan/ClanModalTestUtils.tstests/core/configuration/ConfigLoader.test.tstests/core/game/TrainStation.test.tstests/server/Archive.test.tstests/server/GameLifecycle.test.tstests/server/KickPlayerAuthorization.test.tstests/util/TestConfig.tstests/util/TestServerConfig.tsvite.config.ts
💤 Files with no reviewable changes (4)
- src/core/configuration/ProdConfig.ts
- src/core/configuration/PreprodConfig.ts
- src/core/configuration/DevConfig.ts
- src/core/configuration/DefaultConfig.ts
Comment on lines
+132
to
+145
| async jwkPublicKey(): Promise<JWK> { | ||
| if (this.publicKey) return this.publicKey; | ||
| const jwksUrl = this.jwtIssuer() + "/.well-known/jwks.json"; | ||
| console.log(`Fetching JWKS from ${jwksUrl}`); | ||
| const response = await fetch(jwksUrl); | ||
| const result = JwksSchema.safeParse(await response.json()); | ||
| if (!result.success) { | ||
| const error = z.prettifyError(result.error); | ||
| console.error("Error parsing JWKS", error); | ||
| throw new Error("Invalid JWKS"); | ||
| } | ||
| this.publicKey = result.data.keys[0]; | ||
| return this.publicKey; | ||
| } |
Contributor
There was a problem hiding this comment.
jwkPublicKey() has two real problems: no HTTP error check and a concurrent-fetch race.
await response.json()runs even if the JWKS request returned 4xx/5xx. The bad body may passsafeParsefailure with a confusing error, orresponse.json()itself may throw on an HTML error page. Checkresponse.okfirst.- Two concurrent callers will each
fetchand parse the same JWKS, since the cache is only set afterawait. Cache the in-flightPromise, not the resolved value.
🛠️ Suggested fix
-export class ServerConfig {
- private publicKey: JWK;
+export class ServerConfig {
+ private publicKey: JWK | undefined;
+ private publicKeyPromise: Promise<JWK> | undefined;
@@
- async jwkPublicKey(): Promise<JWK> {
- if (this.publicKey) return this.publicKey;
- const jwksUrl = this.jwtIssuer() + "/.well-known/jwks.json";
- console.log(`Fetching JWKS from ${jwksUrl}`);
- const response = await fetch(jwksUrl);
- const result = JwksSchema.safeParse(await response.json());
- if (!result.success) {
- const error = z.prettifyError(result.error);
- console.error("Error parsing JWKS", error);
- throw new Error("Invalid JWKS");
- }
- this.publicKey = result.data.keys[0];
- return this.publicKey;
- }
+ async jwkPublicKey(): Promise<JWK> {
+ if (this.publicKey) return this.publicKey;
+ this.publicKeyPromise ??= (async () => {
+ const jwksUrl = this.jwtIssuer() + "/.well-known/jwks.json";
+ console.log(`Fetching JWKS from ${jwksUrl}`);
+ const response = await fetch(jwksUrl);
+ if (!response.ok) {
+ throw new Error(
+ `JWKS fetch failed: ${response.status} ${response.statusText}`,
+ );
+ }
+ const result = JwksSchema.safeParse(await response.json());
+ if (!result.success) {
+ console.error("Error parsing JWKS", z.prettifyError(result.error));
+ throw new Error("Invalid JWKS");
+ }
+ this.publicKey = result.data.keys[0];
+ return this.publicKey;
+ })().catch((err) => {
+ this.publicKeyPromise = undefined; // allow retry on next call
+ throw err;
+ });
+ return this.publicKeyPromise;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/configuration/Config.ts` around lines 132 - 145, The jwkPublicKey
function must check HTTP errors and avoid a concurrent-fetch race: first, before
calling response.json(), verify response.ok and throw a descriptive error
including response.status/text so non-JSON error pages are handled; second,
introduce an in-flight promise (e.g., this.jwkPublicKeyPromise or similar) and
assign the Promise returned by the fetch/parse flow to it at the start so
concurrent callers await the same Promise; inside that Promise, parse JSON with
JwksSchema.safeParse (use z.prettifyError on parse failures), set this.publicKey
on success and clear the in-flight promise on both success and failure to avoid
stuck state. Ensure subsequent calls return this.publicKey immediately if
present or await the in-flight promise if set.
Comment on lines
+89
to
+100
| get NUM_WORKERS() { | ||
| return getEnv("NUM_WORKERS"); | ||
| }, | ||
| get TURNSTILE_SITE_KEY() { | ||
| return getEnv("TURNSTILE_SITE_KEY"); | ||
| }, | ||
| get JWT_AUDIENCE() { | ||
| return getEnv("JWT_AUDIENCE"); | ||
| }, | ||
| get ALLOWED_FLARES() { | ||
| return getEnv("ALLOWED_FLARES"); | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add tests for the new Env getters before merge.
I don’t see accompanying tests for this src/core change. Please add coverage for the new getters (including unset/default behavior and env-source precedence).
As per coding guidelines: src/core/**/*.ts: “All changes to src/core/ must include tests”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/configuration/Env.ts` around lines 89 - 100, The new Env getters
(NUM_WORKERS, TURNSTILE_SITE_KEY, JWT_AUDIENCE, ALLOWED_FLARES) lack unit tests;
add tests for the Env class that verify each getter returns the expected value
when the corresponding environment variable is set, returns the
default/undefined behavior when unset, and respects precedence if multiple env
sources are supported (e.g., process.env vs. a config file loader used by
getEnv). Create tests that set and restore process.env around assertions, cover
successful reads and fallback cases for each getter, and add asserts for
precedence and cleanup so these behaviors are enforced before merging.
| // CDN base. Workers have no `window`, so AssetUrls falls back to this. | ||
| globalThis.__CDN_BASE__ = message.cdnBase; | ||
| gameRunner = createGameRunner( | ||
| new ServerConfig(message.serverConfigValues), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tests covering Worker.worker initialization and ServerConfig construction
rg -n --type=ts -C3 'Worker\.worker|WorkerClient.*init|ServerConfig.*serverConfigValues' tests/Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
#!/bin/bash
# First, find test files structure
echo "=== Finding test files related to worker ==="
fd -e spec.ts -e test.ts -e .test.ts | head -20
echo ""
echo "=== Looking for any worker-related test files ==="
fd -i worker | grep -i test
echo ""
echo "=== Check src/core test structure ==="
find src/core -name "*.test.ts" -o -name "*.spec.ts" | head -20
echo ""
echo "=== Broader search for ServerConfig or game runner tests ==="
rg -l "ServerConfig|createGameRunner" --type=ts | grep -i testRepository: openfrontio/OpenFrontIO
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Check actual test directory structure and worker tests
echo "=== List test directory structure ==="
ls -la tests/ 2>/dev/null || echo "tests/ not found"
echo ""
echo "=== Search for worker tests with broader patterns ==="
rg "Worker" --type=ts tests/ 2>/dev/null | head -20 || echo "No matches in tests/"
echo ""
echo "=== Check if there are test files in src/core ==="
find src/core -type f -name "*.test.ts" -o -name "*.spec.ts" -o -name "__tests__" -type dRepository: openfrontio/OpenFrontIO
Length of output: 6011
🏁 Script executed:
#!/bin/bash
# Look at the actual Worker.worker.ts file to understand the change
echo "=== Reading Worker.worker.ts around line 142 ==="
wc -l src/core/worker/Worker.worker.ts
head -150 src/core/worker/Worker.worker.ts | tail -20Repository: openfrontio/OpenFrontIO
Length of output: 836
🏁 Script executed:
#!/bin/bash
echo "=== Contents of tests/core directory ==="
find tests/core -type f -name "*.ts" | sort
echo ""
echo "=== Check TestServerConfig.ts ==="
cat -n tests/util/TestServerConfig.ts | head -50
echo ""
echo "=== Search for any tests using createGameRunner ==="
rg "createGameRunner" --type=ts tests/
echo ""
echo "=== Search for initialization tests with ServerConfig ==="
rg "ServerConfig.*message|message.*ServerConfig" --type=ts tests/Repository: openfrontio/OpenFrontIO
Length of output: 3262
🏁 Script executed:
#!/bin/bash
echo "=== Search for WorkerClient or worker message tests ==="
rg "WorkerClient|MainThreadMessage|InitMessage" --type=ts tests/
echo ""
echo "=== Check if GameLifecycle tests cover worker init ==="
head -100 tests/server/GameLifecycle.test.ts | grep -A5 -B5 -i "worker\|init"
echo ""
echo "=== Search for any test that calls createGameRunner ==="
rg "createGameRunner" --type=ts src/ | head -5Repository: openfrontio/OpenFrontIO
Length of output: 1077
🏁 Script executed:
#!/bin/bash
echo "=== Verify Worker.worker.ts imports and ServerConfig usage ==="
cat -n src/core/worker/Worker.worker.ts | grep -A2 -B2 "ServerConfig\|createGameRunner" | head -30
echo ""
echo "=== Check git history to see if this is new ==="
git log --oneline -n 1 -- src/core/worker/Worker.worker.ts 2>/dev/null || echo "Git info not available"
echo ""
echo "=== Check if there's a test directory specifically for worker ==="
ls -la tests/core/ | grep -i workerRepository: openfrontio/OpenFrontIO
Length of output: 1127
🏁 Script executed:
#!/bin/bash
echo "=== Check ServerConfig constructor to understand what it expects ==="
rg "class ServerConfig|constructor.*serverConfig" --type=ts -A10 src/core/configuration/Config.ts | head -30
echo ""
echo "=== Check what serverConfigValues contains in messages ==="
rg "serverConfigValues" --type=ts src/ | head -10
echo ""
echo "=== Check if InitMessage type validates serverConfigValues ==="
rg "InitMessage|serverConfigValues" --type=ts src/core/worker/ | head -20Repository: openfrontio/OpenFrontIO
Length of output: 1083
🏁 Script executed:
#!/bin/bash
echo "=== Check WorkerClient to see how it constructs the init message ==="
cat -n src/core/worker/WorkerClient.ts | grep -A20 "serverConfigValues"
echo ""
echo "=== Look for any game start integration tests ==="
rg "createGame|startGame" --type=ts tests/ | grep -i "test" | head -10
echo ""
echo "=== Check ConfigLoader test to see test pattern ==="
head -50 tests/core/configuration/ConfigLoader.test.ts | grep -A5 "describe\|it("Repository: openfrontio/OpenFrontIO
Length of output: 2299
Add test coverage for worker initialization with ServerConfig.
The coding guidelines require that all changes to src/core/ include tests. This change constructs ServerConfig from message.serverConfigValues during worker initialization, but no test coverage exists for this path. Add a test that validates the worker correctly receives the init message and passes the server config values to createGameRunner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/worker/Worker.worker.ts` at line 142, Add a unit test that covers
worker initialization path which constructs a ServerConfig from
message.serverConfigValues and ensures those values are forwarded to
createGameRunner: mock or instantiate Worker.worker (or the initialization entry
function used in Worker.worker.ts), send/emit the init message containing
serverConfigValues, assert that new ServerConfig(...) is constructed with those
values (or that createGameRunner was called with an equivalent ServerConfig
instance), and verify createGameRunner receives the expected config; reference
ServerConfig, message.serverConfigValues, createGameRunner, and the worker
initialization logic in Worker.worker.ts when writing the test.
Comment on lines
+24
to
+25
| const host = | ||
| req.get("host") ?? `${ServerEnv.subdomain()}.${ServerEnv.domain()}`; |
Contributor
There was a problem hiding this comment.
Harden host fallback to avoid invalid origin URLs.
If host is missing and env domain/subdomain are empty, this builds "." as host. Use a safe fallback like localhost.
Suggested fix
- const host =
- req.get("host") ?? `${ServerEnv.subdomain()}.${ServerEnv.domain()}`;
+ const envHost = [ServerEnv.subdomain(), ServerEnv.domain()]
+ .filter(Boolean)
+ .join(".");
+ const host = req.get("host") ?? envHost || "localhost";Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/GamePreviewRoute.ts` around lines 24 - 25, The host fallback
builds an invalid "." when req.get("host") is missing and
ServerEnv.subdomain()/ServerEnv.domain() are empty; update the host computation
(the const host assignment) to use a safe fallback like "localhost" if the
combined `${ServerEnv.subdomain()}.${ServerEnv.domain()}` is empty or equals "."
(or if req.get("host") is falsy), and make the same change where the host is
computed again (the other occurrence around the GamePreviewRoute file). Ensure
you normalize/trust the final host value before using it to build origins/URLs.
Comment on lines
+27
to
+29
| numWorkers: JSON.stringify(parseInt(process.env.NUM_WORKERS ?? "", 10)), | ||
| turnstileSiteKey: JSON.stringify(process.env.TURNSTILE_SITE_KEY ?? ""), | ||
| jwtAudience: JSON.stringify(process.env.JWT_AUDIENCE ?? ""), |
Contributor
There was a problem hiding this comment.
Silent fallbacks for missing env vars produce confusing downstream failures.
parseInt(process.env.NUM_WORKERS ?? "", 10)returnsNaNwhenNUM_WORKERSis unset;JSON.stringify(NaN)is"null", sowindow.BOOTSTRAP_CONFIG.numWorkersends upnull. LaterServerConfig.workerIndexdoessimpleHash(gameID) % null, which isNaN, and the worker URL becomeswNaN.turnstileSiteKey ?? ""andjwtAudience ?? ""quietly produce broken values: an emptyjwtAudiencemakesjwtIssuer()returnhttps://api., causing a cryptic JWKS fetch failure much later.
Fail fast at render time (or in the loader) when these are missing/invalid.
🛠️ Suggested fix
- numWorkers: JSON.stringify(parseInt(process.env.NUM_WORKERS ?? "", 10)),
- turnstileSiteKey: JSON.stringify(process.env.TURNSTILE_SITE_KEY ?? ""),
- jwtAudience: JSON.stringify(process.env.JWT_AUDIENCE ?? ""),
+ numWorkers: JSON.stringify(requireIntEnv("NUM_WORKERS")),
+ turnstileSiteKey: JSON.stringify(requireEnv("TURNSTILE_SITE_KEY")),
+ jwtAudience: JSON.stringify(requireEnv("JWT_AUDIENCE")),…with small helpers like:
function requireEnv(name: string): string {
const v = process.env[name];
if (!v) throw new Error(`Missing required env var: ${name}`);
return v;
}
function requireIntEnv(name: string): number {
const n = parseInt(requireEnv(name), 10);
if (!Number.isInteger(n) || n <= 0) {
throw new Error(`Invalid integer env var ${name}=${process.env[name]}`);
}
return n;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/RenderHtml.ts` around lines 27 - 29, Replace the silent fallbacks
in RenderHtml.ts for NUM_WORKERS, TURNSTILE_SITE_KEY, and JWT_AUDIENCE with
explicit validation that throws at render time: add helpers like
requireEnv(name) and requireIntEnv(name) and use them to obtain numWorkers,
turnstileSiteKey, and jwtAudience (instead of parseInt(... ?? "") / ?? ""), so
invalid or missing values raise descriptive errors during render; this prevents
downstream NaN/null values that break ServerConfig.workerIndex
(simpleHash(gameID) % numWorkers) and avoids empty jwtAudience/turnstileSiteKey
causing cryptic runtime failures.
Comment on lines
+29
to
+31
| static apiKey(): string { | ||
| return Env.API_KEY ?? ""; | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast when API_KEY is missing.
Returning "" hides bad config and causes silent auth failures across multiple server fetch paths. Throw here instead of returning an empty key.
Suggested fix
static apiKey(): string {
- return Env.API_KEY ?? "";
+ const key = Env.API_KEY;
+ if (!key) {
+ throw new Error("API_KEY not set");
+ }
+ return key;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static apiKey(): string { | |
| return Env.API_KEY ?? ""; | |
| } | |
| static apiKey(): string { | |
| const key = Env.API_KEY; | |
| if (!key) { | |
| throw new Error("API_KEY not set"); | |
| } | |
| return key; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/ServerEnv.ts` around lines 29 - 31, The ServerEnv.apiKey()
currently returns an empty string when Env.API_KEY is unset which causes silent
auth failures; change ServerEnv.apiKey() to fail-fast by throwing a descriptive
error (e.g., "API_KEY is required") when Env.API_KEY is null/undefined/empty
instead of returning ""; update the implementation in the ServerEnv.apiKey
method to check Env.API_KEY and throw if missing so callers receive a clear
runtime exception immediately.
Comment on lines
+19
to
21
| if (ServerEnv.otelEnabled()) { | ||
| headers["Authorization"] = "Basic " + ServerEnv.otelAuthHeader(); | ||
| } |
Contributor
There was a problem hiding this comment.
Skip exporter setup when OTEL is disabled.
Right now OTEL “disabled” only removes auth header, but metrics exporter still starts. Gate exporter initialization with ServerEnv.otelEnabled() to avoid invalid/noisy export attempts.
Suggested fix
export function initWorkerMetrics(gameManager: GameManager): void {
+ if (!ServerEnv.otelEnabled()) {
+ return;
+ }
+
// Create resource with worker information
const resource = getOtelResource();
// Configure auth headers
const headers: Record<string, string> = {};
- if (ServerEnv.otelEnabled()) {
- headers["Authorization"] = "Basic " + ServerEnv.otelAuthHeader();
- }
+ headers["Authorization"] = "Basic " + ServerEnv.otelAuthHeader();Also applies to: 25-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/WorkerMetrics.ts` around lines 19 - 21, The code currently only
conditionally adds the OTEL auth header via ServerEnv.otelEnabled() but still
always initializes the metrics exporter, causing noisy/invalid export attempts;
update WorkerMetrics.ts to wrap the exporter setup (creation of headers,
OTLPMetricExporter, MeterProvider, PeriodicExportingMetricReader and the call to
MetricsAPI.global.setMeterProvider) in an if (ServerEnv.otelEnabled()) guard so
the exporter and readers are only created when otelEnabled() is true, and keep
the headers assignment inside that same guard; reference the symbols
ServerEnv.otelEnabled(), headers, OTLPMetricExporter, MeterProvider,
PeriodicExportingMetricReader, and MetricsAPI.global.setMeterProvider to find
and modify the code.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/client/ClientEnv.ts (1)
74-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle JWKS fetch failures and de-duplicate concurrent loads.
response.json()still runs on 4xx/5xx, and concurrent callers still issue duplicate fetches because the cache is filled only afterawait. Checkresponse.okfirst and cache the in-flight promise, not just the final key.Suggested fix
export class ClientEnv { private static values: ClientEnvValues | null = null; private static publicKey: JWK | null = null; + private static publicKeyPromise: Promise<JWK> | null = null; @@ static reset(): void { ClientEnv.values = null; ClientEnv.publicKey = null; + ClientEnv.publicKeyPromise = null; } @@ static async jwkPublicKey(): Promise<JWK> { if (ClientEnv.publicKey) return ClientEnv.publicKey; - const jwksUrl = ClientEnv.jwtIssuer() + "/.well-known/jwks.json"; - console.log(`Fetching JWKS from ${jwksUrl}`); - const response = await fetch(jwksUrl); - const result = JwksSchema.safeParse(await response.json()); - if (!result.success) { - const error = z.prettifyError(result.error); - console.error("Error parsing JWKS", error); - throw new Error("Invalid JWKS"); - } - ClientEnv.publicKey = result.data.keys[0]; - return ClientEnv.publicKey; + ClientEnv.publicKeyPromise ??= (async () => { + const jwksUrl = ClientEnv.jwtIssuer() + "/.well-known/jwks.json"; + console.log(`Fetching JWKS from ${jwksUrl}`); + const response = await fetch(jwksUrl); + if (!response.ok) { + throw new Error( + `JWKS fetch failed: ${response.status} ${response.statusText}`, + ); + } + const result = JwksSchema.safeParse(await response.json()); + if (!result.success) { + const error = z.prettifyError(result.error); + console.error("Error parsing JWKS", error); + throw new Error("Invalid JWKS"); + } + ClientEnv.publicKey = result.data.keys[0]; + ClientEnv.publicKeyPromise = null; + return ClientEnv.publicKey; + })().catch((error) => { + ClientEnv.publicKeyPromise = null; + throw error; + }); + return ClientEnv.publicKeyPromise; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/ClientEnv.ts` around lines 74 - 86, The jwkPublicKey function must handle HTTP errors and deduplicate concurrent loads: introduce a cached in-flight promise (e.g. ClientEnv.publicKeyPromise) and assign it before starting the fetch so concurrent callers await the same promise; after fetch, check response.ok and throw an error including response.status/text (do not rely on response.json on 4xx/5xx), then parse and validate the JSON with JwksSchema, set ClientEnv.publicKey from result.data.keys[0] when successful and clear ClientEnv.publicKeyPromise on failure so subsequent calls can retry; keep using ClientEnv.publicKey for fast cache hits.src/server/ServerEnv.ts (1)
125-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
API_KEYis missing.Returning
""hides bad config and causes silent auth failures across multiple server fetch paths. Throw here instead of returning an empty key.Suggested fix
static apiKey(): string { - return Env.API_KEY ?? ""; + const key = Env.API_KEY; + if (!key) { + throw new Error("API_KEY not set"); + } + return key; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/ServerEnv.ts` around lines 125 - 127, The static method apiKey() on Env currently returns an empty string when Env.API_KEY is unset, which hides configuration errors; change Env.apiKey() to throw a clear Error (including guidance like "missing API_KEY") when Env.API_KEY is null/undefined instead of returning "", so callers immediately fail-fast and surface the misconfiguration.
🧹 Nitpick comments (1)
tests/core/configuration/ConfigLoader.test.ts (1)
12-27: ⚡ Quick winCover
workerPath()andturnIntervalMs()here too.Those are the two accessors this refactor threads through most heavily, but this rewritten suite does not assert either of them. A broken bootstrap contract could still pass this test and then fail in
JoinLobbyModal,Main,Matchmaking, orLocalServer.Suggested additions
test("reads from window.BOOTSTRAP_CONFIG without fetching", () => { window.BOOTSTRAP_CONFIG = { gameEnv: "staging", numWorkers: 4, + turnIntervalMs: 250, turnstileSiteKey: "test-key", jwtAudience: "openfront.dev", instanceId: "TEST_ID", }; const fetchSpy = vi.spyOn(globalThis, "fetch"); expect(ClientEnv.env()).toBe(GameEnv.Preprod); expect(ClientEnv.numWorkers()).toBe(4); + expect(ClientEnv.turnIntervalMs()).toBe(250); + expect(ClientEnv.workerPath("ABCD1234")).toMatch(/^w\d+$/); expect(ClientEnv.turnstileSiteKey()).toBe("test-key"); expect(ClientEnv.jwtAudience()).toBe("openfront.dev"); expect(ClientEnv.instanceId()).toBe("TEST_ID"); expect(fetchSpy).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/configuration/ConfigLoader.test.ts` around lines 12 - 27, Add assertions for ClientEnv.workerPath() and ClientEnv.turnIntervalMs() in this test: include workerPath and turnIntervalMs in the window.BOOTSTRAP_CONFIG setup (e.g. "workerPath": "/worker.js", "turnIntervalMs": 500) and then assert expect(ClientEnv.workerPath()).toBe(...) and expect(ClientEnv.turnIntervalMs()).toBe(...). Locate the test block in ConfigLoader.test.ts where window.BOOTSTRAP_CONFIG is defined and add these two properties and corresponding expectations so the refactor's threaded values are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/ClientEnv.ts`:
- Around line 27-45: The bootstrap validation currently allows invalid
numWorkers (0, negatives, non-integers) which breaks workerIndex() sharding;
update the check in ClientEnv (the block that sets ClientEnv.values using
parseGameEnv) to validate bc.numWorkers is a positive integer (e.g.,
Number.isInteger(bc.numWorkers) && bc.numWorkers > 0) and throw a descriptive
error if not, and apply the same validation in the other bootstrap-check site
around lines 94-99 where numWorkers is used; ensure you reference and protect
any code paths that call workerIndex() so they only receive a validated positive
integer.
In `@src/core/configuration/Config.ts`:
- Around line 2-3: Config.ts currently contains runtime/browser auth/bootstrap
logic (window bootstrap typing and JWKS validation) that breaks the
deterministic core rule; extract all runtime auth and bootstrap code (the window
bootstrap typing declarations and any JWKS validation functions/validators) out
of src/core and into platform-specific modules (e.g., client bootstrap and
server auth modules), leaving Config.ts with only pure TypeScript game
configuration types (e.g., AssetManifest and plain config interfaces) and no
browser or auth dependencies; then update any call sites to import the new
client/server implementations instead of pulling runtime logic from Config.ts.
- Around line 164-168: The falloutDefenseModifier uses floating-point math
(falloutRatio between 0 and 1) which violates deterministic-core rules; change
it to use integer fixed-point math by accepting a scaled integer ratio (e.g.,
falloutRatioPermille or falloutRatioBasisPoints) and compute the modifier with
only integer ops, e.g., scale constants (5000 for 5.000) and compute 5000 -
(falloutRatioPermille * 2000) / 1000 (or similar scaling) so no floats or
Math.exp/Math.pow are used; update all callers to pass the scaled integer ratio
and apply the same fixed-point refactor pattern to the other float-using blocks
referenced (lines ~280-301, ~593-700, ~816-858) to preserve determinism.
In `@src/server/ServerEnv.ts`:
- Around line 103-124: The current accessors domain(), subdomain(),
otelEndpoint(), otelAuthHeader(), and gitCommit() silently return "" when env
vars are unset; update them to mirror the behavior of numWorkers() and
adminToken() by throwing a clear error in non-development environments when
required values are missing, while keeping optional ones (used by otelEnabled())
returning empty/undefined; specifically, for domain() and subdomain() (and
gitCommit() if considered required) check ServerEnv.gameEnv !== GameEnv.Dev and
throw a descriptive Error if Env.DOMAIN/Env.SUBDOMAIN/Env.GIT_COMMIT is falsy,
while otelEndpoint() and otelAuthHeader() can remain permissive because
otelEnabled() already gates instrumentation.
In `@tests/client/clan/ClanModalTestUtils.ts`:
- Around line 131-134: configLoaderMockFactory currently returns ClientEnv as a
callable vi.fn which lacks the methods used in code (workerPath, env,
instanceId, etc.), causing tests to throw; update configLoaderMockFactory so it
returns ClientEnv as an object with those methods (e.g., ClientEnv: {
workerPath: vi.fn(() => ...), env: vi.fn(() => ...), instanceId: vi.fn(() =>
...), /* other used members */ }) so tests can call ClientEnv.workerPath(),
ClientEnv.env(), ClientEnv.instanceId(), etc.; add any other missing members
referenced by the code under test and ensure default return values match
expected types.
---
Duplicate comments:
In `@src/client/ClientEnv.ts`:
- Around line 74-86: The jwkPublicKey function must handle HTTP errors and
deduplicate concurrent loads: introduce a cached in-flight promise (e.g.
ClientEnv.publicKeyPromise) and assign it before starting the fetch so
concurrent callers await the same promise; after fetch, check response.ok and
throw an error including response.status/text (do not rely on response.json on
4xx/5xx), then parse and validate the JSON with JwksSchema, set
ClientEnv.publicKey from result.data.keys[0] when successful and clear
ClientEnv.publicKeyPromise on failure so subsequent calls can retry; keep using
ClientEnv.publicKey for fast cache hits.
In `@src/server/ServerEnv.ts`:
- Around line 125-127: The static method apiKey() on Env currently returns an
empty string when Env.API_KEY is unset, which hides configuration errors; change
Env.apiKey() to throw a clear Error (including guidance like "missing API_KEY")
when Env.API_KEY is null/undefined instead of returning "", so callers
immediately fail-fast and surface the misconfiguration.
---
Nitpick comments:
In `@tests/core/configuration/ConfigLoader.test.ts`:
- Around line 12-27: Add assertions for ClientEnv.workerPath() and
ClientEnv.turnIntervalMs() in this test: include workerPath and turnIntervalMs
in the window.BOOTSTRAP_CONFIG setup (e.g. "workerPath": "/worker.js",
"turnIntervalMs": 500) and then assert expect(ClientEnv.workerPath()).toBe(...)
and expect(ClientEnv.turnIntervalMs()).toBe(...). Locate the test block in
ConfigLoader.test.ts where window.BOOTSTRAP_CONFIG is defined and add these two
properties and corresponding expectations so the refactor's threaded values are
validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15459b40-4a1e-40ab-a6a3-3b4df0e09c88
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
index.htmlpackage.jsonsrc/client/AccountModal.tssrc/client/ClientEnv.tssrc/client/ClientGameRunner.tssrc/client/GameModeSelector.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LobbySocket.tssrc/client/LocalServer.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/Transport.tssrc/client/components/CopyButton.tssrc/client/graphics/layers/MultiTabModal.tssrc/core/AssetUrls.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.tssrc/core/worker/Worker.worker.tssrc/server/Archive.tssrc/server/GameManager.tssrc/server/GamePreviewRoute.tssrc/server/GameServer.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/MasterLobbyService.tssrc/server/OtelResource.tssrc/server/RenderHtml.tssrc/server/ServerEnv.tssrc/server/Turnstile.tssrc/server/Worker.tssrc/server/WorkerMetrics.tssrc/server/jwt.tstests/client/clan/ClanModalTestUtils.tstests/core/configuration/ConfigLoader.test.tstests/core/game/TrainStation.test.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/server/Archive.test.tstests/server/GameLifecycle.test.tstests/server/KickPlayerAuthorization.test.tstests/server/MasterLobbyServiceHealth.test.tstests/util/Setup.tstests/util/TestConfig.tstests/util/TestServerConfig.tsvite.config.ts
💤 Files with no reviewable changes (7)
- src/core/configuration/ProdConfig.ts
- tests/pathfinding/utils.ts
- src/core/configuration/PreprodConfig.ts
- src/client/ClientGameRunner.ts
- src/core/configuration/DevConfig.ts
- tests/util/TestServerConfig.ts
- src/core/configuration/DefaultConfig.ts
✅ Files skipped from review due to trivial changes (4)
- src/core/worker/Worker.worker.ts
- src/server/MapPlaylist.ts
- src/core/configuration/Env.ts
- src/server/OtelResource.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/client/AccountModal.ts
- index.html
- src/client/LobbySocket.ts
- src/server/Archive.ts
- src/server/RenderHtml.ts
- vite.config.ts
- src/server/WorkerMetrics.ts
- src/core/configuration/ConfigLoader.ts
- tests/util/TestConfig.ts
- package.json
- src/server/Logger.ts
- src/core/AssetUrls.ts
Comment on lines
+27
to
+45
| if ( | ||
| !bc || | ||
| bc.gameEnv === undefined || | ||
| bc.numWorkers === undefined || | ||
| bc.turnstileSiteKey === undefined || | ||
| bc.jwtAudience === undefined | ||
| ) { | ||
| throw new Error("Missing BOOTSTRAP_CONFIG"); | ||
| } | ||
| if (bc.instanceId === undefined) { | ||
| throw new Error("Missing BOOTSTRAP_CONFIG"); | ||
| } | ||
| ClientEnv.values = { | ||
| gameEnv: parseGameEnv(bc.gameEnv), | ||
| numWorkers: bc.numWorkers, | ||
| turnstileSiteKey: bc.turnstileSiteKey, | ||
| jwtAudience: bc.jwtAudience, | ||
| instanceId: bc.instanceId, | ||
| }; |
Contributor
There was a problem hiding this comment.
Validate numWorkers before using it for sharding.
Right now 0, negatives, or non-integers will flow into workerIndex() and produce NaN or wrong worker paths. That breaks create/join routing for the whole client. Reject bad bootstrap values up front.
Suggested fix
if (
!bc ||
bc.gameEnv === undefined ||
bc.numWorkers === undefined ||
bc.turnstileSiteKey === undefined ||
bc.jwtAudience === undefined
) {
throw new Error("Missing BOOTSTRAP_CONFIG");
}
+ if (!Number.isInteger(bc.numWorkers) || bc.numWorkers < 1) {
+ throw new Error(`Invalid BOOTSTRAP_CONFIG.numWorkers: ${bc.numWorkers}`);
+ }
if (bc.instanceId === undefined) {
throw new Error("Missing BOOTSTRAP_CONFIG");
}Also applies to: 94-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/ClientEnv.ts` around lines 27 - 45, The bootstrap validation
currently allows invalid numWorkers (0, negatives, non-integers) which breaks
workerIndex() sharding; update the check in ClientEnv (the block that sets
ClientEnv.values using parseGameEnv) to validate bc.numWorkers is a positive
integer (e.g., Number.isInteger(bc.numWorkers) && bc.numWorkers > 0) and throw a
descriptive error if not, and apply the same validation in the other
bootstrap-check site around lines 94-99 where numWorkers is used; ensure you
reference and protect any code paths that call workerIndex() so they only
receive a validated positive integer.
Comment on lines
+2
to
+3
| import { z } from "zod"; | ||
| import { AssetManifest } from "../AssetUrls"; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Keep runtime auth/bootstrap code out of src/core.
This refactor moves window bootstrap typing and JWKS validation into the deterministic core package. That makes src/core depend on browser/auth plumbing instead of pure simulation code. Please move this env/auth logic to client/server-specific modules and keep src/core focused on game rules only.
As per coding guidelines, "src/core/ directory must contain deterministic game simulation with pure TypeScript and no external dependencies."
Also applies to: 29-42, 99-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/configuration/Config.ts` around lines 2 - 3, Config.ts currently
contains runtime/browser auth/bootstrap logic (window bootstrap typing and JWKS
validation) that breaks the deterministic core rule; extract all runtime auth
and bootstrap code (the window bootstrap typing declarations and any JWKS
validation functions/validators) out of src/core and into platform-specific
modules (e.g., client bootstrap and server auth modules), leaving Config.ts with
only pure TypeScript game configuration types (e.g., AssetManifest and plain
config interfaces) and no browser or auth dependencies; then update any call
sites to import the new client/server implementations instead of pulling runtime
logic from Config.ts.
Comment on lines
+164
to
+168
| falloutDefenseModifier(falloutRatio: number): number { | ||
| // falloutRatio is between 0 and 1 | ||
| // So defense modifier is between [5, 2.5] | ||
| return 5 - falloutRatio * 2; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Do not move float-based simulation formulas into src/core.
These combat/economy paths now use decimal constants plus Math.exp, Math.pow, and similar float math inside the deterministic core. That is exactly the kind of logic that can drift between runtimes and desync a lockstep simulation. Please keep these formulas in integer/fixed-point form before merging.
As per coding guidelines, "Ensure deterministic behavior in src/core/ by using seeded PRNG and avoiding floating-point math."
Also applies to: 280-301, 593-700, 816-858
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/configuration/Config.ts` around lines 164 - 168, The
falloutDefenseModifier uses floating-point math (falloutRatio between 0 and 1)
which violates deterministic-core rules; change it to use integer fixed-point
math by accepting a scaled integer ratio (e.g., falloutRatioPermille or
falloutRatioBasisPoints) and compute the modifier with only integer ops, e.g.,
scale constants (5000 for 5.000) and compute 5000 - (falloutRatioPermille *
2000) / 1000 (or similar scaling) so no floats or Math.exp/Math.pow are used;
update all callers to pass the scaled integer ratio and apply the same
fixed-point refactor pattern to the other float-using blocks referenced (lines
~280-301, ~593-700, ~816-858) to preserve determinism.
Comment on lines
+103
to
+124
| static domain(): string { | ||
| return Env.DOMAIN ?? ""; | ||
| } | ||
| static subdomain(): string { | ||
| return Env.SUBDOMAIN ?? ""; | ||
| } | ||
| static otelEnabled(): boolean { | ||
| return ( | ||
| ServerEnv.gameEnv !== GameEnv.Dev && | ||
| Boolean(ServerEnv.otelEndpoint()) && | ||
| Boolean(ServerEnv.otelAuthHeader()) | ||
| ); | ||
| } | ||
| static otelEndpoint(): string { | ||
| return Env.OTEL_EXPORTER_OTLP_ENDPOINT ?? ""; | ||
| } | ||
| static otelAuthHeader(): string { | ||
| return Env.OTEL_AUTH_HEADER ?? ""; | ||
| } | ||
| static gitCommit(): string { | ||
| return Env.GIT_COMMIT ?? ""; | ||
| } |
Contributor
There was a problem hiding this comment.
Empty string fallbacks may hide missing configuration.
Methods like domain(), subdomain(), otelEndpoint(), otelAuthHeader(), and gitCommit() return "" when env vars are unset. While some of these are truly optional (e.g., otelEndpoint is checked by otelEnabled()), others like domain and subdomain might be required in production. Returning "" can cause silent failures or incorrect behavior downstream.
Consider whether these values should throw in production environments when missing, similar to numWorkers() and adminToken().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/ServerEnv.ts` around lines 103 - 124, The current accessors
domain(), subdomain(), otelEndpoint(), otelAuthHeader(), and gitCommit()
silently return "" when env vars are unset; update them to mirror the behavior
of numWorkers() and adminToken() by throwing a clear error in non-development
environments when required values are missing, while keeping optional ones (used
by otelEnabled()) returning empty/undefined; specifically, for domain() and
subdomain() (and gitCommit() if considered required) check ServerEnv.gameEnv !==
GameEnv.Dev and throw a descriptive Error if
Env.DOMAIN/Env.SUBDOMAIN/Env.GIT_COMMIT is falsy, while otelEndpoint() and
otelAuthHeader() can remain permissive because otelEnabled() already gates
instrumentation.
Comment on lines
131
to
134
| export function configLoaderMockFactory() { | ||
| return { | ||
| getRuntimeClientServerConfig: vi.fn(() => ({})), | ||
| ClientEnv: vi.fn(() => ({})), | ||
| }; |
Contributor
There was a problem hiding this comment.
Mock ClientEnv with methods, not as a callable.
This helper now returns ClientEnv as vi.fn(() => ({})), but the client code in this PR uses ClientEnv.workerPath(), ClientEnv.env(), ClientEnv.instanceId(), etc. Any test hitting those paths will throw because the mock has no matching members.
Suggested fix
export function configLoaderMockFactory() {
return {
- ClientEnv: vi.fn(() => ({})),
+ ClientEnv: {
+ env: vi.fn(),
+ numWorkers: vi.fn(),
+ turnstileSiteKey: vi.fn(),
+ jwtAudience: vi.fn(),
+ instanceId: vi.fn(),
+ turnIntervalMs: vi.fn(),
+ workerPath: vi.fn(() => "w0"),
+ reset: vi.fn(),
+ },
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function configLoaderMockFactory() { | |
| return { | |
| getRuntimeClientServerConfig: vi.fn(() => ({})), | |
| ClientEnv: vi.fn(() => ({})), | |
| }; | |
| export function configLoaderMockFactory() { | |
| return { | |
| ClientEnv: { | |
| env: vi.fn(), | |
| numWorkers: vi.fn(), | |
| turnstileSiteKey: vi.fn(), | |
| jwtAudience: vi.fn(), | |
| instanceId: vi.fn(), | |
| turnIntervalMs: vi.fn(), | |
| workerPath: vi.fn(() => "w0"), | |
| reset: vi.fn(), | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/client/clan/ClanModalTestUtils.ts` around lines 131 - 134,
configLoaderMockFactory currently returns ClientEnv as a callable vi.fn which
lacks the methods used in code (workerPath, env, instanceId, etc.), causing
tests to throw; update configLoaderMockFactory so it returns ClientEnv as an
object with those methods (e.g., ClientEnv: { workerPath: vi.fn(() => ...), env:
vi.fn(() => ...), instanceId: vi.fn(() => ...), /* other used members */ }) so
tests can call ClientEnv.workerPath(), ClientEnv.env(), ClientEnv.instanceId(),
etc.; add any other missing members referenced by the code under test and ensure
default return values match expected types.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/GameRunner.ts (1)
32-80:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd tests for this
src/corebehavior change before mergeThis PR changes core runtime setup in
createGameRunner(Line 38), but nosrc/coretests are included in the provided changes. That violates the repo rule for core changes and leaves this refactor unguarded.As per coding guidelines: "All changes to
src/core/must include tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/GameRunner.ts` around lines 32 - 80, The change to core runtime setup in createGameRunner (which now constructs GameRunner with new Executor and calls gr.init()) lacks accompanying tests; add unit/integration tests under src/core that exercise createGameRunner to validate the new initialization behavior: ensure createGameRunner returns a GameRunner, that Executor is wired with the provided gameID and clientID, that gr.init() side-effects occur (e.g., expected initial Game state or callbacks), and that createGame, createNationsForGame, loadGameMap and PlayerInfo mapping are invoked/produce correct results for a sample GameStartInfo; use test helpers/mocks for GameMapLoader and the callBack to assert proper interactions and outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/configuration/Theme.ts`:
- Around line 6-31: Add a type-level contract test that locks the Theme
interface shape: create a test file under tests/core (e.g.,
theme.contract.test.ts or a .d.ts type-only test) that imports Theme (and the
referenced types Colord, PlayerView, GameMap, TileRef) and declares a value
typed as Theme (or a helper that asserts assignability) to ensure all
methods/properties (teamColor, territoryColor, structureColors, borderColor,
defendedBorderColors, focusedBorderColor, terrainColor, backgroundColor,
falloutColor, font, textColor, selfColor, allyColor, neutralColor, enemyColor,
spawnHighlightColor, spawnHighlightSelfColor, spawnHighlightTeamColor,
spawnHighlightEnemyColor) exist with the expected signatures; run this under the
project’s TypeScript test harness so missing or mismatched members cause a
compile/test failure.
- Line 1: The Theme interface currently depends on Colord from the external
colord package; change the core contract to use a core-owned color type (e.g.,
export type CoreColor = string | {r:number,g:number,b:number}) and update Theme
to return/accept that type instead of Colord, removing the import of Colord from
Theme.ts; keep Colord usage and conversions confined to implementation/adaptor
files (e.g., PastelTheme.ts, Colors.ts, ColorAllocator.ts) outside src/core by
mapping between CoreColor and Colord at those boundaries, and add unit tests for
PastelTheme to assert deterministic outputs against expected CoreColor values.
---
Outside diff comments:
In `@src/core/GameRunner.ts`:
- Around line 32-80: The change to core runtime setup in createGameRunner (which
now constructs GameRunner with new Executor and calls gr.init()) lacks
accompanying tests; add unit/integration tests under src/core that exercise
createGameRunner to validate the new initialization behavior: ensure
createGameRunner returns a GameRunner, that Executor is wired with the provided
gameID and clientID, that gr.init() side-effects occur (e.g., expected initial
Game state or callbacks), and that createGame, createNationsForGame, loadGameMap
and PlayerInfo mapping are invoked/produce correct results for a sample
GameStartInfo; use test helpers/mocks for GameMapLoader and the callBack to
assert proper interactions and outputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a3fda3b-0bd7-4388-bac5-a3c5816a872b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
index.htmlpackage.jsonsrc/client/AccountModal.tssrc/client/ClientEnv.tssrc/client/ClientGameRunner.tssrc/client/GameModeSelector.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LobbySocket.tssrc/client/LocalServer.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/Transport.tssrc/client/components/CopyButton.tssrc/client/graphics/AnimatedSpriteLoader.tssrc/client/graphics/SpriteLoader.tssrc/client/graphics/fx/SpriteFx.tssrc/client/graphics/layers/FxLayer.tssrc/client/graphics/layers/MultiTabModal.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/TerrainLayer.tssrc/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitLayer.tssrc/core/AssetUrls.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PastelTheme.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.tssrc/core/configuration/Theme.tssrc/core/worker/Worker.worker.tssrc/server/Archive.tssrc/server/GameManager.tssrc/server/GamePreviewRoute.tssrc/server/GameServer.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/MasterLobbyService.tssrc/server/OtelResource.tssrc/server/RenderHtml.tssrc/server/ServerEnv.tssrc/server/Turnstile.tssrc/server/Worker.tssrc/server/WorkerMetrics.tssrc/server/jwt.tstests/client/clan/ClanModalTestUtils.tstests/core/configuration/ConfigLoader.test.tstests/core/game/TrainStation.test.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/server/Archive.test.tstests/server/GameLifecycle.test.tstests/server/KickPlayerAuthorization.test.tstests/server/MasterLobbyServiceHealth.test.tstests/util/Setup.tstests/util/TestConfig.tstests/util/TestServerConfig.tsvite.config.ts
💤 Files with no reviewable changes (7)
- src/core/configuration/ProdConfig.ts
- src/core/configuration/DefaultConfig.ts
- src/core/configuration/DevConfig.ts
- src/core/configuration/PreprodConfig.ts
- tests/util/TestServerConfig.ts
- src/core/configuration/ConfigLoader.ts
- tests/pathfinding/utils.ts
✅ Files skipped from review due to trivial changes (10)
- src/client/graphics/layers/TerrainLayer.ts
- src/client/graphics/fx/SpriteFx.ts
- src/core/configuration/PastelTheme.ts
- src/client/graphics/layers/NameLayer.ts
- src/client/graphics/layers/UILayer.ts
- src/core/worker/Worker.worker.ts
- src/client/graphics/SpriteLoader.ts
- src/client/graphics/layers/UnitLayer.ts
- tests/server/Archive.test.ts
- tests/server/GameLifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (41)
- src/server/MapPlaylist.ts
- tests/server/MasterLobbyServiceHealth.test.ts
- src/server/RenderHtml.ts
- src/server/Turnstile.ts
- tests/client/clan/ClanModalTestUtils.ts
- tests/core/pathfinding/_fixtures.ts
- src/client/components/CopyButton.ts
- src/client/LocalServer.ts
- src/client/AccountModal.ts
- vite.config.ts
- src/server/WorkerMetrics.ts
- src/client/GameModeSelector.ts
- package.json
- src/server/Archive.ts
- src/core/AssetUrls.ts
- src/server/GameManager.ts
- index.html
- tests/util/Setup.ts
- src/client/graphics/layers/MultiTabModal.ts
- src/client/Matchmaking.ts
- src/client/Main.ts
- tests/util/TestConfig.ts
- src/client/LobbySocket.ts
- src/server/MasterLobbyService.ts
- src/server/GameServer.ts
- src/server/Master.ts
- src/server/jwt.ts
- src/core/configuration/Env.ts
- src/server/OtelResource.ts
- src/server/GamePreviewRoute.ts
- src/client/ClientEnv.ts
- src/server/Logger.ts
- src/client/HostLobbyModal.ts
- tests/core/configuration/ConfigLoader.test.ts
- src/client/JoinLobbyModal.ts
- src/server/Worker.ts
- src/client/Transport.ts
- tests/core/game/TrainStation.test.ts
- src/server/ServerEnv.ts
- tests/server/KickPlayerAuthorization.test.ts
- src/core/configuration/Config.ts
| @@ -0,0 +1,31 @@ | |||
| import { Colord } from "colord"; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify external package imports inside src/core (non-relative/non-absolute aliases excluded).
rg -nP '^import\s+.*from\s+"(?!\.{1,2}/|src/|@/)[^"]+"' src/core
# Verify direct usage of colord types in src/core.
rg -nP '\bfrom\s+"colord"|\bColord\b' src/coreRepository: openfrontio/OpenFrontIO
Length of output: 8570
🏁 Script executed:
# Find test files related to Theme
find . -type f -name "*[Tt]heme*.test.*" -o -name "*[Tt]heme*.spec.*" 2>/dev/null | head -20
# Check if there are any tests in src/core that follow a pattern
find src/core -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | head -20
# Look at the structure of Theme.ts to see full context
wc -l src/core/configuration/Theme.tsRepository: openfrontio/OpenFrontIO
Length of output: 102
🏁 Script executed:
# Check git status to see if Theme.ts is newly added in this PR
git status src/core/configuration/Theme.ts 2>/dev/null || echo "Not in git status"
git log --oneline -1 src/core/configuration/Theme.ts 2>/dev/null || echo "Cannot access git log"Repository: openfrontio/OpenFrontIO
Length of output: 158
🏁 Script executed:
# Read the full Theme.ts file
cat -n src/core/configuration/Theme.tsRepository: openfrontio/OpenFrontIO
Length of output: 1410
🏁 Script executed:
# Search for test files in the entire codebase
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | grep -i theme
# Check if any configuration files have tests
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | grep -i config
# List all test files in src/core
find src/core -type f \( -name "*.test.ts" -o -name "*.spec.ts" \)Repository: openfrontio/OpenFrontIO
Length of output: 115
🏁 Script executed:
# Search for any test files that might test Theme
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs grep -l "Theme" 2>/dev/null
# Check if Theme has tests specifically
ls -la tests/core/configuration/ 2>/dev/null | head -20
# Look at ConfigLoader.test.ts to understand testing pattern
head -50 tests/core/configuration/ConfigLoader.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 1205
🏁 Script executed:
# Check if PastelTheme.ts (which also uses Colord) has tests
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs grep -l "PastelTheme" 2>/dev/null
# Check git log to see if Theme.ts was added in recent commits
git log --oneline -20 -- src/core/configuration/Theme.tsRepository: openfrontio/OpenFrontIO
Length of output: 92
🏁 Script executed:
# Find implementations of Theme interface
rg "implements Theme|class.*extends.*implements" src/core -A 2
# Check which files import Theme
rg "import.*Theme" src/core
# Look for usage patterns to understand the architecture
rg "Theme" src/core --type ts -l | head -10Repository: openfrontio/OpenFrontIO
Length of output: 879
🏁 Script executed:
# Check when Theme.ts was first introduced (full git log)
git log --all --oneline -- src/core/configuration/Theme.ts
# Check git diff to see what changed in recent commits involving Theme
git log --all -p --max-count=5 -- src/core/configuration/Theme.ts | head -100
# Verify the current branch/PR to see what files are being modified
git show --stat HEAD | head -50Repository: openfrontio/OpenFrontIO
Length of output: 4496
🏁 Script executed:
# Check if the guideline is applied to existing files - do other colord-using src/core files have tests?
find tests -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "colord\|Colord\|Colors\|ColorAllocator\|PastelTheme" 2>/dev/null
# Check what test coverage exists for src/core files that use zod (also external dep)
find tests -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "WorkerSchemas\|StatsSchemas" 2>/dev/null
# Get an overview of what files are in tests/core
find tests/core -type f | sortRepository: openfrontio/OpenFrontIO
Length of output: 1389
🏁 Script executed:
# Check what Colors.test.ts actually tests
head -100 tests/Colors.test.ts
# See if there are any tests for the color system beyond Colors.test.ts
find tests -name "*.test.ts" | xargs grep -l "ColorAllocator\|PastelTheme" 2>/dev/null
# Check if PastelTheme has any tests
rg "PastelTheme|Theme" tests/ --type tsRepository: openfrontio/OpenFrontIO
Length of output: 3317
Move color type abstraction into core domain
The Theme interface depends on Colord from the external colord package, but src/core should use only core-owned primitives. This file is part of a larger pattern—multiple src/core files (PastelTheme.ts, Colors.ts, ColorAllocator.ts) already import colord—but the guideline states that "src/core/ must contain deterministic game simulation with pure TypeScript and no external dependencies."
Consider creating a core color type (e.g., a simple string type like type CoreColor = string or a structured type like type RgbColor = { r: number; g: number; b: number }) for all Theme method returns. Map between core types and Colord only in implementation files that live outside src/core or in adapter layers. This keeps the core contract independent of external libraries while implementations can still use colord for color math.
Additionally, add tests for Theme implementations (like PastelTheme) to ensure color values remain deterministic and match expected outputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/configuration/Theme.ts` at line 1, The Theme interface currently
depends on Colord from the external colord package; change the core contract to
use a core-owned color type (e.g., export type CoreColor = string |
{r:number,g:number,b:number}) and update Theme to return/accept that type
instead of Colord, removing the import of Colord from Theme.ts; keep Colord
usage and conversions confined to implementation/adaptor files (e.g.,
PastelTheme.ts, Colors.ts, ColorAllocator.ts) outside src/core by mapping
between CoreColor and Colord at those boundaries, and add unit tests for
PastelTheme to assert deterministic outputs against expected CoreColor values.
Comment on lines
+6
to
+31
| export interface Theme { | ||
| teamColor(team: Team): Colord; | ||
| // Don't call directly, use PlayerView | ||
| territoryColor(playerInfo: PlayerView): Colord; | ||
| // Don't call directly, use PlayerView | ||
| structureColors(territoryColor: Colord): { light: Colord; dark: Colord }; | ||
| // Don't call directly, use PlayerView | ||
| borderColor(territoryColor: Colord): Colord; | ||
| // Don't call directly, use PlayerView | ||
| defendedBorderColors(territoryColor: Colord): { light: Colord; dark: Colord }; | ||
| focusedBorderColor(): Colord; | ||
| terrainColor(gm: GameMap, tile: TileRef): Colord; | ||
| backgroundColor(): Colord; | ||
| falloutColor(): Colord; | ||
| font(): string; | ||
| textColor(playerInfo: PlayerView): string; | ||
| // unit color for alternate view | ||
| selfColor(): Colord; | ||
| allyColor(): Colord; | ||
| neutralColor(): Colord; | ||
| enemyColor(): Colord; | ||
| spawnHighlightColor(): Colord; | ||
| spawnHighlightSelfColor(): Colord; | ||
| spawnHighlightTeamColor(): Colord; | ||
| spawnHighlightEnemyColor(): Colord; | ||
| } |
Contributor
There was a problem hiding this comment.
Add tests for this new core contract
This introduces a new public src/core interface without accompanying tests. Please add at least a type-level/contract test to lock the expected Theme shape.
As per coding guidelines: "All changes to src/core/ must include tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/configuration/Theme.ts` around lines 6 - 31, Add a type-level
contract test that locks the Theme interface shape: create a test file under
tests/core (e.g., theme.contract.test.ts or a .d.ts type-only test) that imports
Theme (and the referenced types Colord, PlayerView, GameMap, TileRef) and
declares a value typed as Theme (or a helper that asserts assignability) to
ensure all methods/properties (teamColor, territoryColor, structureColors,
borderColor, defendedBorderColors, focusedBorderColor, terrainColor,
backgroundColor, falloutColor, font, textColor, selfColor, allyColor,
neutralColor, enemyColor, spawnHighlightColor, spawnHighlightSelfColor,
spawnHighlightTeamColor, spawnHighlightEnemyColor) exist with the expected
signatures; run this under the project’s TypeScript test harness so missing or
mismatched members cause a compile/test failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 142-143: Add a validation block in deploy.sh to check that
NUM_WORKERS and TURNSTILE_SITE_KEY are set before proceeding: locate the
existing GHCR_USERNAME check example and insert a similar conditional that tests
[ -z "$NUM_WORKERS" ] || [ -z "$TURNSTILE_SITE_KEY" ] and prints a clear error
and exits non‑zero if either is empty so the deploy fails fast when repository
variables are missing.
In `@deploy.sh`:
- Around line 69-70: Add a validation that ensures DOMAIN is set before deriving
JWT_AUDIENCE: check if the DOMAIN environment variable is empty/null and if so
print a descriptive error and exit with non-zero status (same pattern used for
other required vars), and only then set JWT_AUDIENCE="$DOMAIN"; reference the
JWT_AUDIENCE and DOMAIN variables and follow the existing validation pattern
used elsewhere in the script.
- Around line 144-146: The deployment script writes NUM_WORKERS and
TURNSTILE_SITE_KEY into the remote env unvalidated, which can cause runtime
failures in ServerEnv.ts; add pre-write validation in deploy.sh to check that
NUM_WORKERS and TURNSTILE_SITE_KEY are non-empty (and optionally that
NUM_WORKERS is a positive integer) and if not, exit with a clear error message
(e.g., "Missing required env: NUM_WORKERS" or "Missing required env:
TURNSTILE_SITE_KEY") before proceeding to write the remote environment file.
In `@src/server/jwt.ts`:
- Line 73: The fetch call that concatenates ServerEnv.jwtIssuer() + "/users/@me"
can produce double slashes if jwtIssuer() has a trailing slash; replace the
string concatenation with safe URL joining using the URL constructor (e.g.,
create a new URL for the "/users/@me" path with ServerEnv.jwtIssuer() as the
base) and use that URL when calling fetch to ensure correct path joining; update
the expression at the fetch site in src/server/jwt.ts (the line building the
request URL) to use new URL(...).toString() (or equivalent) instead of simple
string concatenation with ServerEnv.jwtIssuer().
- Around line 35-37: ServerEnv.jwkPublicKey() returns a raw JWK but jwtVerify
expects a KeyLike; update the code in jwt.ts to import importJWK from "jose",
call const jwk = await ServerEnv.jwkPublicKey(), convert it with await
importJWK(jwk, "EdDSA") and pass that KeyLike to jwtVerify (keep issuer from
ServerEnv.jwtIssuer() and audience from ServerEnv.jwtAudience() and set
algorithms to ["EdDSA"]) so jwtVerify(token, key, { algorithms: ["EdDSA"],
issuer, audience }) receives the correct key type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6d56cf6-6ca7-4af2-9d46-2c1ce8cdceae
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (69)
.github/workflows/deploy.ymldeploy.shindex.htmlpackage.jsonsrc/client/AccountModal.tssrc/client/ClientEnv.tssrc/client/ClientGameRunner.tssrc/client/GameModeSelector.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LobbySocket.tssrc/client/LocalServer.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/Transport.tssrc/client/components/CopyButton.tssrc/client/graphics/AnimatedSpriteLoader.tssrc/client/graphics/SpriteLoader.tssrc/client/graphics/fx/SpriteFx.tssrc/client/graphics/layers/FxLayer.tssrc/client/graphics/layers/MultiTabModal.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/TerrainLayer.tssrc/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitLayer.tssrc/core/AssetUrls.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PastelTheme.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.tssrc/core/configuration/Theme.tssrc/core/worker/Worker.worker.tssrc/server/Archive.tssrc/server/GameManager.tssrc/server/GamePreviewRoute.tssrc/server/GameServer.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/MasterLobbyService.tssrc/server/OtelResource.tssrc/server/RenderHtml.tssrc/server/ServerEnv.tssrc/server/Turnstile.tssrc/server/Worker.tssrc/server/WorkerMetrics.tssrc/server/jwt.tstests/client/clan/ClanModalTestUtils.tstests/core/configuration/ConfigLoader.test.tstests/core/game/TrainStation.test.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/server/Archive.test.tstests/server/GameLifecycle.test.tstests/server/KickPlayerAuthorization.test.tstests/server/MasterLobbyServiceHealth.test.tstests/util/Setup.tstests/util/TestConfig.tstests/util/TestServerConfig.tsvite.config.ts
💤 Files with no reviewable changes (7)
- src/core/configuration/ProdConfig.ts
- src/core/configuration/DevConfig.ts
- src/core/configuration/PreprodConfig.ts
- tests/pathfinding/utils.ts
- tests/util/TestServerConfig.ts
- src/core/configuration/DefaultConfig.ts
- src/core/configuration/ConfigLoader.ts
✅ Files skipped from review due to trivial changes (16)
- src/client/graphics/layers/TerritoryLayer.ts
- src/client/graphics/AnimatedSpriteLoader.ts
- src/client/graphics/SpriteLoader.ts
- src/client/graphics/layers/NameLayer.ts
- tests/server/Archive.test.ts
- src/client/graphics/layers/UILayer.ts
- src/server/MapPlaylist.ts
- src/core/configuration/PastelTheme.ts
- src/client/graphics/layers/StructureIconsLayer.ts
- src/client/GameModeSelector.ts
- vite.config.ts
- src/client/graphics/layers/UnitLayer.ts
- src/core/worker/Worker.worker.ts
- src/client/graphics/layers/TerrainLayer.ts
- src/client/graphics/layers/FxLayer.ts
- src/client/graphics/layers/StructureLayer.ts
🚧 Files skipped from review as they are similar to previous changes (42)
- tests/client/clan/ClanModalTestUtils.ts
- src/server/RenderHtml.ts
- tests/server/GameLifecycle.test.ts
- tests/server/MasterLobbyServiceHealth.test.ts
- src/client/graphics/fx/SpriteFx.ts
- src/client/graphics/layers/StructureDrawingUtils.ts
- src/core/configuration/Theme.ts
- src/server/Logger.ts
- src/client/graphics/layers/MultiTabModal.ts
- src/server/WorkerMetrics.ts
- package.json
- src/server/OtelResource.ts
- src/client/Transport.ts
- src/client/LobbySocket.ts
- src/core/GameRunner.ts
- src/server/Turnstile.ts
- src/core/configuration/Env.ts
- src/server/Archive.ts
- src/client/JoinLobbyModal.ts
- src/core/AssetUrls.ts
- index.html
- src/client/HostLobbyModal.ts
- tests/server/KickPlayerAuthorization.test.ts
- tests/util/TestConfig.ts
- src/server/GameManager.ts
- src/server/GameServer.ts
- src/client/components/CopyButton.ts
- src/client/Main.ts
- src/server/Master.ts
- src/server/Worker.ts
- src/client/Matchmaking.ts
- tests/util/Setup.ts
- src/server/MasterLobbyService.ts
- src/client/ClientGameRunner.ts
- tests/core/game/TrainStation.test.ts
- tests/core/configuration/ConfigLoader.test.ts
- src/server/ServerEnv.ts
- src/client/ClientEnv.ts
- src/client/AccountModal.ts
- tests/core/pathfinding/_fixtures.ts
- src/server/GamePreviewRoute.ts
- src/core/configuration/Config.ts
Comment on lines
+142
to
+143
| NUM_WORKERS: ${{ vars.NUM_WORKERS }} | ||
| TURNSTILE_SITE_KEY: ${{ vars.TURNSTILE_SITE_KEY }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if repository variables are configured
gh api repos/openfrontio/OpenFrontIO/actions/variables --jq '.variables[] | select(.name == "NUM_WORKERS" or .name == "TURNSTILE_SITE_KEY") | {name: .name, value: .value}'Repository: openfrontio/OpenFrontIO
Length of output: 301
🏁 Script executed:
git ls-files '.github/workflows/*.yml' | head -20Repository: openfrontio/OpenFrontIO
Length of output: 251
🏁 Script executed:
cat -n .github/workflows/deploy.yml | head -150Repository: openfrontio/OpenFrontIO
Length of output: 7189
🏁 Script executed:
# Find the deploy.sh script
fd -t f -i 'deploy\.sh'Repository: openfrontio/OpenFrontIO
Length of output: 93
🏁 Script executed:
cat -n deploy.shRepository: openfrontio/OpenFrontIO
Length of output: 6158
🏁 Script executed:
cat -n build-deploy.shRepository: openfrontio/OpenFrontIO
Length of output: 3070
Add validation for NUM_WORKERS and TURNSTILE_SITE_KEY in deploy.sh.
These variables are passed to the deployment script but are not validated before use. If vars.NUM_WORKERS or vars.TURNSTILE_SITE_KEY are not defined in repository settings, empty strings will be passed to the remote server, which may cause the application to fail at runtime even though the deployment appears to succeed.
Add validation checks similar to the existing GHCR_USERNAME check (line 58-60 in deploy.sh):
if [ -z "$NUM_WORKERS" ] || [ -z "$TURNSTILE_SITE_KEY" ]; then
echo "Error: NUM_WORKERS or TURNSTILE_SITE_KEY not defined in repository variables or environment"
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy.yml around lines 142 - 143, Add a validation block
in deploy.sh to check that NUM_WORKERS and TURNSTILE_SITE_KEY are set before
proceeding: locate the existing GHCR_USERNAME check example and insert a similar
conditional that tests [ -z "$NUM_WORKERS" ] || [ -z "$TURNSTILE_SITE_KEY" ] and
prints a clear error and exits non‑zero if either is empty so the deploy fails
fast when repository variables are missing.
Comment on lines
+69
to
+70
| # jwtAudience always matched DOMAIN in the old per-env configs. | ||
| JWT_AUDIENCE="$DOMAIN" |
Contributor
There was a problem hiding this comment.
Validate DOMAIN before deriving JWT_AUDIENCE.
JWT_AUDIENCE is derived from $DOMAIN, but DOMAIN is not validated before use. If DOMAIN is missing from the environment files, JWT_AUDIENCE will be empty, causing runtime failures when the deployed application starts.
Following the existing pattern at lines 58-61 and 87-90, add a validation check for required variables.
🛡️ Suggested validation check
+# Validate DOMAIN is set before deriving JWT_AUDIENCE
+if [ -z "$DOMAIN" ]; then
+ echo "Error: DOMAIN not defined in .env file or environment"
+ exit 1
+fi
+
# jwtAudience always matched DOMAIN in the old per-env configs.
JWT_AUDIENCE="$DOMAIN"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # jwtAudience always matched DOMAIN in the old per-env configs. | |
| JWT_AUDIENCE="$DOMAIN" | |
| # Validate DOMAIN is set before deriving JWT_AUDIENCE | |
| if [ -z "$DOMAIN" ]; then | |
| echo "Error: DOMAIN not defined in .env file or environment" | |
| exit 1 | |
| fi | |
| # jwtAudience always matched DOMAIN in the old per-env configs. | |
| JWT_AUDIENCE="$DOMAIN" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy.sh` around lines 69 - 70, Add a validation that ensures DOMAIN is set
before deriving JWT_AUDIENCE: check if the DOMAIN environment variable is
empty/null and if so print a descriptive error and exit with non-zero status
(same pattern used for other required vars), and only then set
JWT_AUDIENCE="$DOMAIN"; reference the JWT_AUDIENCE and DOMAIN variables and
follow the existing validation pattern used elsewhere in the script.
Comment on lines
+144
to
+146
| JWT_AUDIENCE=$JWT_AUDIENCE | ||
| NUM_WORKERS=$NUM_WORKERS | ||
| TURNSTILE_SITE_KEY=$TURNSTILE_SITE_KEY |
Contributor
There was a problem hiding this comment.
Validate required variables before writing to remote environment.
NUM_WORKERS and TURNSTILE_SITE_KEY are written to the remote environment file without validation. According to the AI summary, these are required by ServerEnv.ts. If these variables are missing or empty, the deployment will succeed but the application will fail at runtime.
Add validation checks before line 133 to fail fast with a clear error message.
🛡️ Suggested validation checks
print_header "EXECUTING UPDATE SCRIPT ON SERVER"
+# Validate required environment variables
+if [ -z "$JWT_AUDIENCE" ]; then
+ echo "Error: JWT_AUDIENCE not defined"
+ exit 1
+fi
+
+if [ -z "$NUM_WORKERS" ]; then
+ echo "Error: NUM_WORKERS not defined in environment"
+ exit 1
+fi
+
+if [ -z "$TURNSTILE_SITE_KEY" ]; then
+ echo "Error: TURNSTILE_SITE_KEY not defined in environment"
+ exit 1
+fi
+
ssh -i $SSH_KEY $REMOTE_USER@$SERVER_HOST "chmod +x $REMOTE_UPDATE_SCRIPT && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy.sh` around lines 144 - 146, The deployment script writes NUM_WORKERS
and TURNSTILE_SITE_KEY into the remote env unvalidated, which can cause runtime
failures in ServerEnv.ts; add pre-write validation in deploy.sh to check that
NUM_WORKERS and TURNSTILE_SITE_KEY are non-empty (and optionally that
NUM_WORKERS is a positive integer) and if not, exit with a clear error message
(e.g., "Missing required env: NUM_WORKERS" or "Missing required env:
TURNSTILE_SITE_KEY") before proceeding to write the remote environment file.
Comment on lines
+35
to
+37
| const issuer = ServerEnv.jwtIssuer(); | ||
| const audience = ServerEnv.jwtAudience(); | ||
| const key = await ServerEnv.jwkPublicKey(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check ServerEnv method signatures and return types
# Look for ServerEnv class/module definition
ast-grep --pattern $'export class ServerEnv {
$$$
}'
# Also check for individual method definitions
rg -nP --type=ts -A5 'jwtIssuer|jwtAudience|jwkPublicKey' src/server/ServerEnv.tsRepository: openfrontio/OpenFrontIO
Length of output: 8237
🏁 Script executed:
cat -n src/server/jwt.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 2120
Convert JWK to KeyLike before passing to jwtVerify.
jwtVerify() from jose expects a KeyLike parameter, but ServerEnv.jwkPublicKey() returns a raw JWK object. Import and use importJWK() to convert:
import { jwtVerify, importJWK } from "jose";
const issuer = ServerEnv.jwtIssuer();
const audience = ServerEnv.jwtAudience();
const jwk = await ServerEnv.jwkPublicKey();
const key = await importJWK(jwk, "EdDSA");
const { payload } = await jwtVerify(token, key, {
algorithms: ["EdDSA"],
issuer,
audience,
});Without this conversion, the code will fail at runtime with a type error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/jwt.ts` around lines 35 - 37, ServerEnv.jwkPublicKey() returns a
raw JWK but jwtVerify expects a KeyLike; update the code in jwt.ts to import
importJWK from "jose", call const jwk = await ServerEnv.jwkPublicKey(), convert
it with await importJWK(jwk, "EdDSA") and pass that KeyLike to jwtVerify (keep
issuer from ServerEnv.jwtIssuer() and audience from ServerEnv.jwtAudience() and
set algorithms to ["EdDSA"]) so jwtVerify(token, key, { algorithms: ["EdDSA"],
issuer, audience }) receives the correct key type.
| try { | ||
| // Get the user object | ||
| const response = await fetch(config.jwtIssuer() + "/users/@me", { | ||
| const response = await fetch(ServerEnv.jwtIssuer() + "/users/@me", { |
Contributor
There was a problem hiding this comment.
Use proper URL joining to avoid potential double slashes.
Direct string concatenation could create https://example.com//users/@me if jwtIssuer() includes a trailing slash. Use the URL constructor for safer path joining.
🔗 Proposed fix using URL constructor
- const response = await fetch(ServerEnv.jwtIssuer() + "/users/@me", {
+ const url = new URL("/users/@me", ServerEnv.jwtIssuer());
+ const response = await fetch(url.toString(), {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const response = await fetch(ServerEnv.jwtIssuer() + "/users/@me", { | |
| const url = new URL("/users/@me", ServerEnv.jwtIssuer()); | |
| const response = await fetch(url.toString(), { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/jwt.ts` at line 73, The fetch call that concatenates
ServerEnv.jwtIssuer() + "/users/@me" can produce double slashes if jwtIssuer()
has a trailing slash; replace the string concatenation with safe URL joining
using the URL constructor (e.g., create a new URL for the "/users/@me" path with
ServerEnv.jwtIssuer() as the base) and use that URL when calling fetch to ensure
correct path joining; update the expression at the fetch site in
src/server/jwt.ts (the line building the request URL) to use new
URL(...).toString() (or equivalent) instead of simple string concatenation with
ServerEnv.jwtIssuer().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME