Repository navigation
fix(startup): honor documented env loading #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
diegosouzapw
merged 4 commits into
cheaper-inference:main
from
kfiramar:fix-startup-env-key-loading
Mar 14, 2026
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
88cc53a
fix(startup): honor documented env loading
kfiramar da39e14
fix(startup): fail closed on key inspection errors
kfiramar e3a2bd7
fix(startup): ignore blank data dir override
kfiramar b19e6a8
fix(startup): pass env through env-file lookup
kfiramar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import fs from "node:fs"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import Database from "better-sqlite3"; | ||
|
|
||
| import { bootstrapEnv } from "../../scripts/bootstrap-env.mjs"; | ||
|
|
||
| function withTempEnv(fn) { | ||
| const originalCwd = process.cwd(); | ||
| const originalEnv = { ...process.env }; | ||
| const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-bootstrap-test-")); | ||
| const tempCwd = path.join(tempRoot, "cwd"); | ||
| const tempHome = path.join(tempRoot, "home"); | ||
|
|
||
| fs.mkdirSync(tempCwd, { recursive: true }); | ||
| fs.mkdirSync(tempHome, { recursive: true }); | ||
|
|
||
| delete process.env.DATA_DIR; | ||
| delete process.env.XDG_CONFIG_HOME; | ||
| delete process.env.APPDATA; | ||
| delete process.env.JWT_SECRET; | ||
| delete process.env.STORAGE_ENCRYPTION_KEY; | ||
| delete process.env.STORAGE_ENCRYPTION_KEY_VERSION; | ||
| delete process.env.API_KEY_SECRET; | ||
| delete process.env.INITIAL_PASSWORD; | ||
| process.env.HOME = tempHome; | ||
| process.chdir(tempCwd); | ||
|
|
||
| try { | ||
| fn({ tempRoot, tempCwd, tempHome, dataDir: path.join(tempHome, ".omniroute") }); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| for (const key of Object.keys(process.env)) { | ||
| if (!(key in originalEnv)) delete process.env[key]; | ||
| } | ||
| for (const [key, value] of Object.entries(originalEnv)) { | ||
| process.env[key] = value; | ||
| } | ||
| fs.rmSync(tempRoot, { recursive: true, force: true }); | ||
| } | ||
| } | ||
|
|
||
| test("bootstrapEnv prefers ~/.omniroute/.env over server.env", () => { | ||
| withTempEnv(({ dataDir }) => { | ||
| fs.mkdirSync(dataDir, { recursive: true }); | ||
| fs.writeFileSync( | ||
| path.join(dataDir, ".env"), | ||
| "STORAGE_ENCRYPTION_KEY=from-dot-env\nJWT_SECRET=jwt-from-dot-env\n", | ||
| "utf8" | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(dataDir, "server.env"), | ||
| "STORAGE_ENCRYPTION_KEY=from-server-env\nJWT_SECRET=jwt-from-server-env\n", | ||
| "utf8" | ||
| ); | ||
|
|
||
| const env = bootstrapEnv({ quiet: true }); | ||
|
|
||
| assert.equal(env.STORAGE_ENCRYPTION_KEY, "from-dot-env"); | ||
| assert.equal(env.JWT_SECRET, "jwt-from-dot-env"); | ||
| }); | ||
| }); | ||
|
|
||
| test("bootstrapEnv refuses to generate a new key over encrypted data", () => { | ||
| withTempEnv(({ dataDir }) => { | ||
| fs.mkdirSync(dataDir, { recursive: true }); | ||
| const db = new Database(path.join(dataDir, "storage.sqlite")); | ||
| try { | ||
| db.exec(` | ||
| CREATE TABLE provider_connections ( | ||
| id TEXT PRIMARY KEY, | ||
| access_token TEXT, | ||
| refresh_token TEXT, | ||
| api_key TEXT, | ||
| id_token TEXT | ||
| ); | ||
| `); | ||
| db.prepare("INSERT INTO provider_connections (id, access_token) VALUES (?, ?)") | ||
| .run("conn-1", "enc:v1:deadbeef:feedface:cafebabe"); | ||
| } finally { | ||
| db.close(); | ||
| } | ||
|
|
||
| assert.throws( | ||
| () => bootstrapEnv({ quiet: true }), | ||
| /Refusing to auto-generate STORAGE_ENCRYPTION_KEY/ | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| test("bootstrapEnv fails closed when existing database cannot be inspected", () => { | ||
| withTempEnv(({ dataDir }) => { | ||
| fs.mkdirSync(path.join(dataDir, "storage.sqlite"), { recursive: true }); | ||
|
|
||
| assert.throws( | ||
| () => bootstrapEnv({ quiet: true }), | ||
| /Unable to inspect existing database/ | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| test("bootstrapEnv ignores blank dataDirOverride values", () => { | ||
| withTempEnv(({ dataDir }) => { | ||
| fs.mkdirSync(dataDir, { recursive: true }); | ||
| fs.writeFileSync(path.join(dataDir, ".env"), "JWT_SECRET=jwt-from-dot-env\n", "utf8"); | ||
|
|
||
| const env = bootstrapEnv({ dataDirOverride: " ", quiet: true }); | ||
|
|
||
| assert.equal(env.JWT_SECRET, "jwt-from-dot-env"); | ||
| }); | ||
| }); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These helper functions (
resolveDataDir,getPreferredEnvFilePath,hasEncryptedCredentials) are duplicated fromscripts/bootstrap-env.mjs. While I understand this is due to the CJS/ESM module differences between Electron's main process and the scripts, this duplication creates a maintenance burden. If the logic changes in one place, it must be manually updated in the other.To avoid this, consider extracting these shared functions into a separate CommonJS file (e.g.,
scripts/env-helpers.cjs). This new file could berequire'd here and also imported into the ESMbootstrap-env.mjsmodule. This would centralize the logic and improve maintainability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping this PR scoped to the startup/key-safety fix. I agree the duplication is a maintenance cost, but extracting a shared helper across the Electron CJS path and the bootstrap ESM path would broaden the runtime/module-loading surface for this PR. I would prefer to handle that as a separate follow-up refactor once the startup regression is merged.