fix: wire config.build.engine into pipeline and connection#1604
Conversation
Greptile SummaryThis PR fixes a silent engine-override gap where
Confidence Score: 5/5Safe to merge — changes are narrowly scoped to wiring an existing config value into call sites that were previously ignoring it, with no new code paths or data mutations. All three engine-selection sites now use the same priority chain; loadConfig() is cached so per-call overhead in connection.ts is negligible; moving loadConfig() earlier in setupPipeline() is safe because openDb/initSchema have no dependency on ctx.config; the ?? vs || change is type-safe given the TypeScript union type on engine. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI / programmatic call] --> B{opts.engine set?}
B -- yes --> E[use opts.engine]
B -- no --> C[loadConfig / ctx.config]
C --> D{config.build.engine set?}
D -- yes --> E
D -- no --> F[fallback: 'auto']
E --> G{engine !== 'wasm'?}
F --> G
G -- yes --> H{isNativeAvailable?}
H -- yes --> I[open NativeDatabase / rusqlite]
H -- no --> J[open better-sqlite3]
G -- no --> J
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[CLI / programmatic call] --> B{opts.engine set?}
B -- yes --> E[use opts.engine]
B -- no --> C[loadConfig / ctx.config]
C --> D{config.build.engine set?}
D -- yes --> E
D -- no --> F[fallback: 'auto']
E --> G{engine !== 'wasm'?}
F --> G
G -- yes --> H{isNativeAvailable?}
H -- yes --> I[open NativeDatabase / rusqlite]
H -- no --> J[open better-sqlite3]
G -- no --> J
Reviews (4): Last reviewed commit: "fix: add engine override param to openRe..." | Re-trigger Greptile |
| const engine = process.env.CODEGRAPH_ENGINE || 'auto'; | ||
| // config.build.engine covers both CODEGRAPH_ENGINE env (via applyEnvOverrides) | ||
| // and the .codegraphrc.json config-file path. | ||
| const engine = loadConfig().build.engine ?? 'auto'; |
There was a problem hiding this comment.
openReadonlyWithNative cannot accept a per-call engine override
openRepo() accepts opts.engine so callers can force 'wasm' for a specific operation, but openReadonlyWithNative() has no such escape hatch and always reads loadConfig().build.engine. Any future caller that needs --engine wasm for a query path (e.g. a benchmark or test harness) has no way to opt out of the native path through this function. Since this is a query-only surface the risk is low today, but the asymmetry may cause confusion if callers expect parity with openRepo().
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in f7606b6. Added an optional opts: { engine?: 'native' | 'wasm' | 'auto' } parameter to openReadonlyWithNative() (default {}), mirroring the openRepo() escape hatch. The priority chain is now opts.engine ?? loadConfig().build.engine ?? 'auto' — identical to openRepo(). All existing callers are unaffected (they pass no opts). TypeScript compiles cleanly; the change is purely additive.
| // so this covers both the env-var path and the .codegraphrc.json config-file path. | ||
| // This ensures --engine wasm and benchmark workers bypass the native path. | ||
| const engine = opts.engine || process.env.CODEGRAPH_ENGINE || 'auto'; | ||
| const engine = opts.engine ?? loadConfig().build.engine ?? 'auto'; |
There was a problem hiding this comment.
loadConfig() uses process.cwd(), not the DB's directory
Both openRepo() and openReadonlyWithNative() call loadConfig() without a rootDir argument, so they resolve .codegraphrc.json from process.cwd(). When a caller passes a customDbPath pointing to a different repository (e.g. codegraph query --db /other/project/.codegraph/graph.db), the engine setting from that project's config file is never consulted — only the CWD config is used. This was equally true of the previous process.env.CODEGRAPH_ENGINE approach (a process-wide variable), so it is not a regression, but it is worth documenting as a known gap if multi-repo support is planned.
There was a problem hiding this comment.
Not fixed in this PR — this is a pre-existing limitation that predates this change (the previous process.env.CODEGRAPH_ENGINE approach was equally process-wide). Tracked as #1605 for follow-up when multi-repo / --db hardening is planned.
Codegraph Impact Analysis4 functions changed → 42 callers affected across 32 files
|
0ca86b9 to
8436601
Compare
|
Addressed Greptile P2 findings:
|
Summary
openRepo()andopenReadonlyWithNative()inconnection.tspreviously readprocess.env.CODEGRAPH_ENGINEdirectly, silently ignoring anyenginevalue set via.codegraphrc.json. They now readloadConfig().build.engine, which already incorporates both the env-var path (viaapplyEnvOverrides) and the config-file path.initializeEngine()andsetupPipeline()inpipeline.tsusedctx.opts.engine || 'auto', skippingconfig.build.engine. The priority chain is nowctx.opts.engine ?? ctx.config.build.engine ?? 'auto'. Config is loaded first insetupPipeline()sobuild.engineis available before the native-availability check.fn-impact.ts(unnecessary parentheses, carried in from thefix/issue-1587merge).Test plan
npx tsc --noEmit)npm run lint)npx vitest run tests/graph/classifiers tests/unit/roles.test.ts)Closes #1596