morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #2824] [CLOSED] test(sqlite): fix two SQLite boundary tests on native Windows #5419

New issue

Closed

opened 2026-10-31 01:06:29 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:06:29 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2824
Author: @pablodiazjorge
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: test/windows-sqlite-boundary

📝 Commits (1)

e9a946b test(sqlite): fix two SQLite boundary tests on native Windows

📊 Changes

2 files changed (+17 additions, -4 deletions)

View changed files

📝 tests/boundary/sqlite/sqliteDatabaseDescriptorCleanup.test.ts (+16 -3)
📝 tests/boundary/sqlite/sqliteDatabaseProvider.test.ts (+1 -1)

📄 Description

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 #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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2824 **Author:** [@pablodiazjorge](https://github.com/pablodiazjorge) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `test/windows-sqlite-boundary` --- ### 📝 Commits (1) - [`e9a946b`](https://github.com/morluto/rea/commit/e9a946be47cc55282908e6598781c1a3dc55cb0d) test(sqlite): fix two SQLite boundary tests on native Windows ### 📊 Changes **2 files changed** (+17 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `tests/boundary/sqlite/sqliteDatabaseDescriptorCleanup.test.ts` (+16 -3) 📝 `tests/boundary/sqlite/sqliteDatabaseProvider.test.ts` (+1 -1) </details> ### 📄 Description ## 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 #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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:06:29 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5419

No description provided.

Rows
Columns