test(reindex): close the three remaining review gaps - #187
Merged
Conversation
…rd why the third is not closed
Two closed:
- The crash-injection test re-read the rolled-back index on the SAME
connection that ran the failed swap. db.ts's own header sets the stronger
bar — "a FRESH connection still read the original table" — because vec0
keeps four shadow tables and per-connection module state, so the handle that
failed is the one most likely to answer from memory a reopen would not
reproduce. It now closes the database, reopens, and asserts row count and
vector width from the new handle.
- check-doc-claims gained the doc-to-code direction for CLI options. A sibling
test already scans SOURCE files so no message can recommend a flag the CLI
rejects (it caught two real ones the day it was added), but nothing looked
the other way — an option table left listing a removed flag passed silently,
which is what happened to --vectors. 24 documented flags now resolve.
--discard-generation is added to the reindex option table, so the gate has
the new flag under it too.
One NOT closed, with the dead end recorded in the test file so the next attempt
does not repeat it: the CLI verdict (the incomplete banner and exit code 1) has
been unguarded since the pre-flight probe was added, because the existing test
stops AT the probe and never calls reindex(). Reaching the loop offline needs a
provider that answers the probe at the configured width and the corpus at the
wrong one. An in-process http.createServer CANNOT do it — run() uses
execFileSync, which blocks this process's event loop, so the stub never answers
the child and every request times out (measured: "Ollama embedding request timed
out"). A working version has to spawn the stub as its own process.
Verification, this session:
node scripts/run-tests-isolated.mjs exit=0
Test Files 154 passed (154) / Tests 2243 passed (2243); no "Errors" line
npm run typecheck exit=0
node scripts/check-doc-claims.mjs exit=0
24 documented CLI flags all resolve to registered options
…p, closed
Supersedes the previous commit's claim that this one could not be closed. It
could; the obstacle was real but it was not a dead end, and recording it instead
of solving it was the wrong call.
The verdict a shell script actually reads — the incomplete banner and
`process.exitCode = 1` — has been unguarded since the pre-flight probe was
added: the existing test stops AT the probe, exits before `withDatabase`, and
never calls `reindex()`. Nothing else spawns this CLI.
Reaching the loop offline needs a provider that answers twice, differently: at
the configured width for the single probe string, and at the wrong width for the
corpus. An in-process `http.createServer` cannot do it, and that part of the
earlier note was correct — `run()` uses `execFileSync`, which blocks this
process's event loop, so the stub never answers the child and every request
times out. The answer is simply that the stub does not belong in this process:
it is written to the test's tmpdir and `spawn`ed, so it has its own event loop
and keeps serving while this one is blocked.
Two anti-vacuity assertions guard the test itself, because the failure mode here
is a test that passes by never reaching the code: it asserts the probe-refusal
message is ABSENT and that "Reindexing" is present, so a run that stopped at the
probe fails rather than quietly proving nothing.
Verification, this session:
node scripts/run-tests-isolated.mjs tests/cli-reindex-vectors-guard.test.ts
exit=0 Tests 7 passed (7)
npm run typecheck
exit=0
Break-test: `if (incomplete) process.exitCode = 1` -> `if (false && ...)`,
dist rebuilt (this suite spawns dist/, so a source-only mutation is invisible
to it):
mutant run exit=1 -> KILLED
cli.ts restored byte-identical, dist rebuilt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test(reindex): pin the CLI incomplete-verdict contract — the third gap, closed
Supersedes the previous commit's claim that this one could not be closed. It
could; the obstacle was real but it was not a dead end, and recording it instead
of solving it was the wrong call.
The verdict a shell script actually reads — the incomplete banner and
process.exitCode = 1— has been unguarded since the pre-flight probe wasadded: the existing test stops AT the probe, exits before
withDatabase, andnever calls
reindex(). Nothing else spawns this CLI.Reaching the loop offline needs a provider that answers twice, differently: at
the configured width for the single probe string, and at the wrong width for the
corpus. An in-process
http.createServercannot do it, and that part of theearlier note was correct —
run()usesexecFileSync, which blocks thisprocess's event loop, so the stub never answers the child and every request
times out. The answer is simply that the stub does not belong in this process:
it is written to the test's tmpdir and
spawned, so it has its own event loopand keeps serving while this one is blocked.
Two anti-vacuity assertions guard the test itself, because the failure mode here
is a test that passes by never reaching the code: it asserts the probe-refusal
message is ABSENT and that "Reindexing" is present, so a run that stopped at the
probe fails rather than quietly proving nothing.
Verification, this session:
node scripts/run-tests-isolated.mjs tests/cli-reindex-vectors-guard.test.ts
exit=0 Tests 7 passed (7)
npm run typecheck
exit=0
Break-test:
if (incomplete) process.exitCode = 1->if (false && ...),dist rebuilt (this suite spawns dist/, so a source-only mutation is invisible
to it):
mutant run exit=1 -> KILLED
cli.ts restored byte-identical, dist rebuilt