Skip to content

test(sqlite): fix two SQLite boundary tests on native Windows - #2824

Closed
pablodiazjorge wants to merge 1 commit into
morluto:mainfrom
pablodiazjorge:test/windows-sqlite-boundary
Closed

pablodiazjorge wants to merge 1 commit into
morluto:mainfrom
pablodiazjorge:test/windows-sqlite-boundary

Conversation

@pablodiazjorge

Copy link
Copy Markdown
Contributor

Summary

Two SQLite boundary tests fail on native Windows because of assumptions in the tests themselves; the provider behaves correctly. No issue is linked: I found them while verifying inspect_sqlite_database (#2271) on Windows, which CI does not cover (sqlite-runtime.yml runs on Ubuntu only). The fix touches only these two test files, like #2312. The same verification found a real misclassification of EBUSY on Windows, reported separately in #2822.

Problem and expected behavior

Smallest trigger, on Windows with Node 24.18.0: npx vitest run tests/boundary/sqlite/sqliteDatabaseProvider.test.ts tests/boundary/sqlite/sqliteDatabaseDescriptorCleanup.test.ts gives 2 failures on 117bd23dd. Expected: both tests reach their assertions and pass.

Test Failure on Windows Cause
sqliteDatabaseProvider › cancels an acquired SQLite worker and removes its private snapshot ProviderAdapterError ("Provider process fixture exited before readiness") instead of AnalysisCancelledError The test builds the provider with an empty environment ({}). Without SystemRoot, Node aborts at startup on Windows (Assertion failed: ncrypto::CSPRNG(nullptr, 0), exit 134), so the fixture never prints ready
sqliteDatabaseDescriptorCleanup › retains reply-read cancellation and retries the failed reply descriptor close top-level execution_failure: "execution_failure" instead of "cancelled" The test keeps the reply descriptor open on purpose. Windows cannot remove the private root while a file inside it is open (Delete owned object by handle failed […] (Win32 145), directory not empty), so the provider reports the root cleanup failure and keeps the cancellation as diagnostics.previous_error. POSIX unlinks open files, so the root is removed there

Change and scope

  • sqliteDatabaseProvider.test.ts: the cancellation test passes process.env, like createProvider in the same file.
  • sqliteDatabaseDescriptorCleanup.test.ts: on Windows, the reply-read test expects the root cleanup error with the original cancellation as its previous error (same execution_failure, resource and captured output as before). Elsewhere the expectation is unchanged. The shutdown retry assertions that follow are unchanged and pass on both platforms.

No product code changes.

Contribution scope: not applicable (test-only fix within existing boundaries).

Contract and boundary impact

  • Semantic owner and earliest changed stage: test files only.
  • Contribution scope fit and linked maintainer decision: not applicable.
  • CLI and MCP/tool-catalog contract: none.
  • Provider, bridge, target-format, or platform compatibility: none; the tests now run on native Windows.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Evidence and regression coverage

  • Tests added or updated: the two tests above.
  • Base reproduction or other evidence: on 117bd23dd the SQLite test files give 2 failed, 89 passed, 9 skipped on Windows; the errors are in the table.
  • Remaining proof gaps: macOS not run.

Validation performed

Windows 11 x64 (10.0.26300), non-elevated, Node 24.18.0, with the Windows native addon built from the same commit.

  • The 8 SQLite test files (git ls-files | grep sqlite | grep test.ts) on 117bd23dd: 2 failed, 89 passed, 9 skipped.
  • The same files with this change: 91 passed, 9 skipped.
  • Linux (WSL Ubuntu, Node 24.18.0), same files with this change: 98 passed, 2 skipped.
  • npx oxfmt --check on the two files: passed.
  • npm run check:fast: typecheck passed; lint reports two max-lines-per-function errors that are already on main (ghidraLauncher.test.ts and GhidraFunctionValues.test.ts, from fix(ghidra): warn when callee/caller reads stop at a truncated function body #2360), none in the changed files.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: native Windows for these suites; no provider involved.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; tests only.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T18:56:59.704200Z e9a946b New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@greptile-apps

greptile-apps Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low impact] The test-only changes appear safe to merge.

Summary

Updates two SQLite boundary tests for native Windows without changing product code.

  • SQLite boundary tests now account for Windows worker startup and file cleanup.

Reviews (5) · Last reviewed commit: "test(sqlite): fix two SQLite boundary te..." · Reviewed by Greptile

@morluto morluto added area: evidence Evidence, artifacts, and reconstruction area: tests Test suite and verification P3: low Low priority, nice to have labels Oct 13, 2026
@morluto
morluto force-pushed the test/windows-sqlite-boundary branch from 2fc8a8a to 88f447c Compare October 13, 2026 18:29
@morluto
morluto force-pushed the test/windows-sqlite-boundary branch from 88f447c to e9a946b Compare October 13, 2026 18:54
@morluto morluto added the enhancement New feature or request label Oct 14, 2026
@pablodiazjorge

Copy link
Copy Markdown
Contributor Author

Closing, since these changes landed on main in 5b7fe94: the descriptor cleanup test as proposed, and the provider test with only SystemRoot on Windows. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: evidence Evidence, artifacts, and reconstruction area: tests Test suite and verification enhancement New feature or request P3: low Low priority, nice to have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants