Repository navigation
Conversation
Use direct, identifier-quoted table_xinfo PRAGMA statements so stored tables or views named pragma_table_xinfo cannot break database inspection. Cover both shadowing cases with real SQLite fixtures and preserved selected rows. Keep the synthetic JADX cleanup-failure child alive after stdin EOF until verified teardown kills it, avoiding natural-exit races in ownership scans. Include the stop result in teardown assertion failures. Validation: 21 focused SQLite tests, all eight check:ci tasks, runtime build, and real fixture EOF/lifetime probes passed. Local owned-worker and public SQLite tests remain blocked by this host's procps/PID namespace mismatch; those existing tests passed in the original GitHub CI run.
Keep possible shadow tables unknown when their virtual module cannot be resolved, and reject row selection through CLI and MCP. Read schema text directly as bytes without hex expansion or native sort records, preserving exact encodings and valid large definitions. Add real SQLite regressions for unknown modules, malformed text, UTF-16, large SQL, and long identifiers.
Declare unavailable function-body ranges and absent native API/value-flow evidence explicitly. The package fixture must satisfy the stricter function dossier contract now on main. Reproduced the CI rejection against the PR merge commit, then verified the same managed CLI workflow passes with the completed fixture.
Resolve the overlapping managed-native fixture fixes by retaining main's shared, validated function dossier builder. All SQLite changes are preserved. The merged tree passes 36 focused SQLite/MCP tests, all 8 static tasks, the runtime build, and the real managed CLI package verification.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Accept stable empty or fully zeroed rollback journal headers left by clean TRUNCATE/PERSIST commits, while refusing active, malformed, symlinked or changing journals. Bind each member read to the original file-set observation and report disappearance or replacement as changed evidence. Separate schema metadata preflight accounting from the final reply budget so names are not charged twice. Add real SQLite, filesystem-race and CLI/MCP regressions. Verified 49 focused tests, all static checks, runtime build and generated docs.
Encode REAL negative zero as the explicit string -0 alongside the existing infinity representations, preventing JSON serialization from silently changing the observed value to positive zero. Update the output contract and documentation, add a real SQLite regression that distinguishes REAL -0, REAL 0 and INTEGER 0, and extend CLI/MCP acceptance fixtures across rollback journal modes.
|
Review of 878b873: the offline DB/WAL acquisition, quoted table selection, read-only engine configuration, typed integers/REAL/blob/text representation and CLI/MCP parity are well covered. I independently ran the inspection/snapshot/real CLI+stdio MCP subset: 35 tests passed. P2 — retain the cleanup owner when private snapshot removal fails. In Please retain failed roots in a supervised collection and provide an awaited retry/close path through the service/server lifecycle, with a failure-then-success regression. Reporting the residual path is useful, but does not substitute for retaining ownership. Design follow-up: the hard 1000-row input ceiling ( |
|
TL;DR: This works well on Node 24.18.0. Schema, lossless rows, WAL-only rows and the row bounds all check out. The source files stay untouched. On Node 22.23.1, the tool says it needs Node 24.15+, as the docs say. But 31 of the 63 new tests then fail on Node 22, instead of skipping. I used Node 24.18.0, which is this repo's Tests.
CI runs only the Real checks. These ran on Node 24.18.0, through the CLI and over stdio MCP. The databases are ones I built with Python's
No One small UX note. On Node 22 the tool still shows in I didn't test encrypted databases, any real app databases, or macOS and Windows. Test setup
🤖 Generated with Claude Code (Opus 5.5) 🦊 |
Retain failed snapshot cleanup owners for awaited retry and CLI/MCP shutdown. Accept row samples bounded by actual byte and worker budgets. Gate native-engine tests on DatabaseSync.limits and cover unsupported runtimes explicitly. Merge current upstream main and resolve documentation/fixture conflicts. Validation: build and check:fast pass; focused Node 24 tests 51 passed, 1 capability skip; Node 22 tests 30 passed, 18 capability skips. Local worker tests are blocked by the environment's ps/process-identity visibility and remain for CI.
Retain failed SQLite worker supervisors before attempting snapshot removal. Retry process cleanup before releasing the database/WAL runtime root, and keep failed MCP server instances reachable after SDK shutdown or stdin EOF. Add regressions with a real owned child and the actual SDK stdio transport, including failure followed by successful cleanup and completed-owner release. Validation: 62 focused tests passed on Node 24.18.0 (one unsupported-runtime test skipped), 18 cleanup/shutdown tests passed on Node 22.23.1; build, typecheck, lint, formatting and diff checks passed.
|
TL;DR: Fixed: on Node 22.23.1 the new tests now skip instead of failing. Fixed: a failed snapshot cleanup is kept and retried, and the 1,000-row cap is gone. Still open: a Node 22 user learns about the Node 24.15 need only when the tool runs. This follows my earlier comment. I tested the new head
The counts differ by head. The new set has 74 tests: the earlier 63, plus 4 in the new cleanup file, 4 in changed files and 3 shutdown tests in Fixed from my earlier comment.
P2 from the review. Fixed. A failed cleanup is now kept, retried before the next call, and retried again at shutdown. Details are in the fold below. Two things I saw that are not fixes.
The "local worker tests". The commit says they are blocked by process visibility. Here they were not. On Linux they ran and passed on Node 24.18.0 and 26.11.1. That is 15 of 15 in the provider file and 4 of 4 cleanup tests. The real CLI and MCP cases passed too. No test skips for process reasons. Node 22 skips them only for the native limit. So I could not reproduce the block here. I could not match the commit counts. The middle head's message says 51 and 30. The new head's says 62 on Node 24 and 18 on Node 22. Neither says which files it ran. Not run
Thanks, Y0UW1SH, for the quick fixes and the real stdio shutdown tests. P2 cleanup check, step by stepI made the private snapshot copy hard to delete. A read-only subfolder inside my own folder did it. The first MCP call reported I then shut the server down with the block still on. It exited with code 1 and logged "Provider cleanup failed during server shutdown". It did not print the folder path. With the block lifted first, shutdown exited 0 and removed the folder. On the old head, a second call worked and only the first copy stayed, even after I lifted the block and closed the server. The CLI reports the same Commands# Node 22 is the system node; 24 and 26 are under ~/.local/opt/node24 and node26
env -i HOME=<fake> PATH=<node bin>:/usr/bin:/bin TMPDIR=/tmp \
node node_modules/vitest/vitest.mjs run <files> --reporter=verbose --reporter=json
rea inspect-sqlite-database <db> [--table T] [--row-limit N] --json
# MCP: tools/call inspect_sqlite_database over stdio (rea mcp)
npm run check:fastThe test files: I built the fixtures with Python's
Every source file kept the same SHA-256 after every run. To block cleanup, a small script waited for the snapshot folder, then added Raw outputTest setup
🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
…atibility Merge upstream fixes for failing browser, firmware, EVM and shared-skill regressions. Preserve failed SQLite source and reply descriptors through cleanup retries, including original permission and cancellation diagnostics. Read fixture insertion row IDs as exact BigInts and exercise SQLite on Node 22 and 26.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…anup Retain primary CLI inspection errors alongside shutdown failures, and keep reply-read cancellation typed when descriptor cleanup is incomplete. Add four real-filesystem CLI and worker boundary regressions.
Merge current main while preserving the reviewed SQLite cleanup fixes. Normalize the SVG context condition to the repository formatter and assert selected paths in decoded CLI errors so Windows backslashes remain exact.
Merge upstream's decoded Windows path assertions and cleanup updates. Share the SQLite snapshot fixture, unavailable-worker setup, and private root removal assertion while preserving all failure injections and cases.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
TL;DR: Still open: at the command line, a Node 22 user learns about the Node 24.15 need only when the tool runs. MCP clients can see it earlier. Fixed: the 3 Node 26 test failures are gone, and a failed cleanup no longer hides the original error. Edit, 11 October, 03:23 UTC: I retested at d63d1b1, and the results are here. This follows my retest at a9ce588. I tested the new head eed34a6. Its base is 60fbb2f on main. My open item: not fixed for the command line.
A one-line row in What changed since a9ce588Only the PR's own code is listed. I compared the PR diff against its base at each head.
Other callers of the shared file pass no scope and behave as before. The PR now touches 42 files with 4,684 added lines. At a9ce588 it was 39 files and 4,085 lines. ResultsI ran the CI job's 8 test files on each Node. Each run has 91 tests. At a9ce588 the same 7 files had 77. The new file adds 14.
The 33 skips on Node 22 are the tests that need native limits. So the new Node 22 job checks cleanup and shutdown, not inspection. The 2 skips on Node 24 and 26 are the tests that expect no native limits. The 3 Node 26 failures were in the test setup, and they pass now. Row checks, Node 24 and 26 (identical output)
Forced cleanup failure (Node 24.18.0). I made the private snapshot folder hard to delete, inside my own /tmp folder.
Not run
Thanks, Y0UW1SH, for the descriptor cleanup work and the BigInt fix. Odd things I saw
Commands# Node 22 is the system node; 24 and 26 are under ~/.local/opt/node24 and node26
git archive eed34a6e1 | tar -x -C head; npm ci; npm run build; npm run test:prepare
node scripts/generate-completion-ledger.mjs # CI runs this too
env -i HOME=<fake> PATH=<node bin>:/usr/bin:/bin TMPDIR=<tmp> \
node node_modules/vitest/vitest.mjs run \
src/application/sqlite/SqliteDatabaseService.test.ts \
tests/acceptance/sqliteDatabaseCli.test.ts \
tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts \
tests/boundary/sqlite/sqliteDatabaseDescriptorCleanup.test.ts \
tests/boundary/sqlite/sqliteDatabaseInspection.test.ts \
tests/boundary/sqlite/sqliteDatabaseProvider.test.ts \
tests/boundary/sqlite/sqliteDatabaseSnapshot.test.ts \
tests/boundary/mcp/serverLifecycleMcp.test.ts \
--reporter=verbose --reporter=json
rea inspect-sqlite-database <db> [--table T] [--row-limit N] --json
rea capabilities --json; rea doctor --json
# MCP over stdio: tools/list, tools/call binary_session, tools/call inspect_sqlite_databaseFixtures came from Python's
To block cleanup, a small script waited for the snapshot folder. Then it added Raw outputTest setup🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
Declare inert Electron imports in preload fixtures so current main can prove API provenance. Preserve option and source-coordinate assertions. Use the configured catalog startup budget instead of the narrower per-test override under coverage contention.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Retain upstream Electron imports, matching source ranges, and the catalog timeout. Include SQLite runtime compatibility in the new required CI gate.
Isolate compatibility checks, exercise minimum and rolling runtimes, share test selection, and guard required CI lanes. Merge current main and format its new reference-source files.
|
TL;DR: Fixed: on Node 22.23.1, This follows my retest at eed34a6. I tested d63d1b1. It adds two commits. The first, 7caf4db, adds a SQLite row to capabilities. The second, d63d1b1, fixes test fixtures for Electron. The base is still 60fbb2f. I did not rerun eed34a6, so that column comes from my earlier comment.
What the command line says now. On Node 22.23.1,
The new file has 8 tests of the version check, and the CLI acceptance file has 1 new test. All 9 pass on all three Nodes. The version check gives false for 24.14.9 and true for 24.15.0. Cleanup checks, Node 24.18.0 (Node 26.11.1 for the first row). The results match my earlier comment.
Still not run
Thanks, Y0UW1SH, for the capabilities row. Odd things I saw
Commands# Node 22 is the system node; 24 and 26 are under ~/.local/opt/node24 and node26
git archive d63d1b15c | tar -x -C head; npm ci; npm run build; npm run test:prepare
node scripts/generate-mcp-tool-catalog.mjs # and the skill-metadata, product-catalog
node scripts/generate-completion-ledger.mjs # and completion-ledger, as the CI job does
rea capabilities --json; rea providers --json; rea doctor --json; rea capabilities
rea inspect-sqlite-database <db> [--table T] [--row-limit N] --json
env -i HOME=<fake> PATH=<node bin>:/usr/bin:/bin TMPDIR=<tmp> \
node node_modules/vitest/vitest.mjs run \
src/application/sqlite/SqliteDatabaseCapability.test.ts \
src/application/sqlite/SqliteDatabaseService.test.ts \
tests/acceptance/sqliteDatabaseCli.test.ts \
tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts \
tests/boundary/sqlite/sqliteDatabaseDescriptorCleanup.test.ts \
tests/boundary/sqlite/sqliteDatabaseInspection.test.ts \
tests/boundary/sqlite/sqliteDatabaseProvider.test.ts \
tests/boundary/sqlite/sqliteDatabaseSnapshot.test.ts \
tests/boundary/mcp/serverLifecycleMcp.test.ts
# MCP over stdio: tools/list, tools/call binary_session, tools/call inspect_sqlite_databaseThe fixtures are the same as in my earlier comment. I built them with Python's Raw outputTest setup
🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
dandgabr
left a comment
dandgabr
left a comment
There was a problem hiding this comment.
P2 — Bound aggregate SQLite inspection resources. Each inspection can accept up to 256 MiB of database/WAL input (src/sqlite/SqliteDatabaseSnapshot.ts:196-215), while the provider admits concurrent inspections without a provider-wide cap (src/sqlite/SqliteDatabaseProvider.ts:72-88; src/artifacts/ArtifactResourceScope.ts:48-73). Concurrent near-limit requests can multiply buffered memory, worker processes, and temporary disk usage until the host or REA process is exhausted. Please cap concurrent inspections or enforce an aggregate resource budget across active snapshots and workers.
morluto
added
enhancement
Summary
Add standalone SQLite schema and selected-row inspection through
inspect_sqlite_databaseandrea inspect-sqlite-database. Preserve original database/WAL identities and lossless integer, blob, and text values.Requested enhancement; no linked issue.
Problem and expected behavior
Callers can inspect a quiescent SQLite database or application-created backup directly, without opening a native analysis target:
Equivalent MCP arguments:
{"path":"/artifacts/application.sqlite","table":"settings","row_limit":20}.Review and CI corrections in this update:
rea capabilitiesbefore a database is selected. CLI and MCP share the runtime policy and upgrade note; Node 22 and Node 24.11–24.14 remain supported for other REA features.Change and scope
Copy a checked DB/WAL file set into a private runtime root and inspect it in an owned, cancellable SQLite worker. Inventory tables, columns, indexes, views, triggers, and stored virtual declarations; optionally read bounded rows from one exact ordinary table. Preserve failed resource owners until release is confirmed.
Contract and boundary impact
DatabaseSync.limitssupport.rea capabilitiesexposes runtime availability and remediation before inspection; unsupported runtimes still run the inventory successfully.Evidence and regression coverage
Executed real SQLite fixtures cover WAL commits, quoted identifiers, integer boundaries, blob and text encodings, nonfinite values, corrupt input, native resource limits, and journal handling. Filesystem cases cover replacement and changes across acquisition.
Fourteen descriptor cleanup regressions cover DB/WAL/journal/reply release, retry admission, sticky invalidated-handle failures, EACCES/EPERM, caller cancellation, and CLI shutdown diagnostics. The four latest regressions reproduced the diagnostic failures before their fixes; the permanent reply regression uses the production owned-worker launcher.
Representative selected values:
{"columns":["id","payload"],"values":[[{"type":"integer","value":"1"},{"type":"blob","hex":"01"}]],"returned_rows":1,"truncated":true}Update hardening
f998fcb1and correct its two formatting failures without changing their behavior.Validation of the tree published as
2d96bf77:npm run test:sqlite: 81 passed, 2 skipped, 17 worker-dependent failures due to this host's existing procps/PID namespace mismatch. Real worker coverage remains with CI; the previous GitHub run passed the Node 22 and Node 26 SQLite lanes.These changes reduce avoidable integration drift and detect runtime regressions. They do not guarantee compatibility with arbitrary future changes to
main.Earlier validation
On local Node 24.19.0:
npm run check:ci: all eight tasks and architecture guards passed.npm run test:prepareandnpm run docs:check: passed.node scripts/verify-test-discovery.mjs: all 800 test files discovered exactly once.git diff --check: passed.Full CI on the discovery fix exposed Electron fixtures missing the explicit imports required by current main and a catalog test with a narrower timeout than the configured suite budget. The follow-up declares inert Electron imports, preserves source-range assertions, and restores that catalog case's configured budget.
Fresh GitHub Actions CI passed on corrected head
d63d1b15ca475548f408333be0268146c5ee475b: all 33 jobs, including the final CI required gate.Compatibility, safety, and release
Additive feature; no migration. Inspection accepts clean offline snapshots and inactive journals. A stable file set does not establish an atomic live backup. Arbitrary caller SQL, extensions, repair, encryption, and deleted-record recovery remain unsupported. Native heap/RSS is not fully capped; the worker and transport output budgets are separate.
Local owned-worker acceptance runs are blocked by this host's PID/procfs mismatch (
procps: fatal library error, lookup self). The earlier GitHub Actions run ond63d1b15passed the real CLI/MCP, process, and platform checks; validation for the hardening update is reported separately above.Review checklist