Repository navigation
refactor(plugin): initialize workbench databases with Node SQLite - #1335
mldangelo-oai wants to merge 27 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…/sqlite-database-info-fixes
|
@codex review |
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed 6a707aa. I found the following issue and am leaving this PR unapproved.
[P2] Keep initialization and later operations on the same database — sdk/typescript/src/server/sqlite-store.ts:27
Calling runWorkbench directly here leaves this.options unset. If a caller changes CODEX_SECURITY_STATE_DIR (including through process.env), or changes cwd with a relative state directory, after initialize() but before the first list/insert, that operation opens a different database. Previously initialize() went through this.run() and cached the normalized absolute location. Cache that location during initialization and reuse it for subsequent operations, while keeping Python resolution lazy.
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # sdk/typescript/tests-ts/runtime.test.ts
# Conflicts: # sdk/typescript/tests-ts/api.test.ts
|
Fixed the database-location issue from this review. Initialization now captures the state directory and environment, so later operations keep the same database after directory or environment changes. The follow-up changes also preserve relative Python paths and the original executable-trust context. Interpreter validation remains lazy, and Bun’s Node selection uses the same captured trust context. Regression tests cover directory changes before and after the first Python operation, missing interpreters, and repository-local executable rejection. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c17cd094e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
The workbench stores scans and findings in SQLite. Initialize that database with Node's built-in SQLite so findings-store startup and
database-infocan work without Python. Other workbench operations continue to use Python and the same database.Changes
Reuse the shared migration history and legacy repairs, preserving saved results, complete diagnostics, and transaction errors. Keep existing directory links and permissions; create missing storage privately, including Windows access controls. The SDK resolves the state directory once and uses its trusted Node selection when running under Bun.
Initialization captures the state directory, environment, and original executable-trust context. Relative Python paths retain their original location if the caller changes directories. Python discovery stays lazy until a legacy command runs, then caches the validated executable; a missing explicit interpreter does not prevent Node initialization.
Testing
Checks on Node 22.13 and Node 24 upgraded every released schema through version 42, including editable scan names, to the same current schema, then repeated the upgrade without changing migration history. Workflow fixtures retained embedded-NUL diagnostics and large integer values. A forced storage failure preserved the original error and pre-upgrade data. Concurrent-access cases kept an up-to-date reader usable during a write and retried an upgrade after a held lock.
SDK fixtures initialize two stores concurrently with Python configured to a missing executable, then verify the shared database path. Additional fixtures check that Python operations see the same stored findings and that Bun ignores a repository-local Node shim.
With dependencies and the host's plugin/native bundle built, repeat the database scenarios using
node --experimental-strip-types --test tests/test_workbench_database.tsfromplugins/codex-security/mcp-app.Portable plugin checks, SDK build/types/format, build-plugin fixtures, and focused runtime/Deep Scan worker checks pass. The latest fix passes 35 findings-store tests, 93 focused runtime tests, and 11 executable-trust tests. Regressions cover state-location changes, changing directories before and after the first Python operation, an unavailable explicit interpreter, and rejection of an executable from the original repository. Full SDK-suite coverage is reported at the stack tip.
Risk and rollout
Depends on #1333. Node remains 22.13 or newer within supported major versions. Preserve Python database compatibility, foreign keys, concurrent readers, lock retries, path handling, and terminal-control escaping. No public flags are added. The recorded local tests do not establish behavior on every supported platform or packaging path.
Related PRs
Merge order: #1333 → #1335 → #1341 → #1342 → #1343 → #1397 → #1408
Public disclosure review