fix: close the remaining 22 findings from tonight's full-codebase review ledger - #203
Merged
Merged
Conversation
…onfig setters that lied about what they stored
**R7-a was only half fixed.** `openHookDb`'s writers got a 2s busy_timeout
cap in an earlier commit, but guard-check.js, session-start.js and
pre-edit-recall.js each need a `readOnly` handle — which `openHookDb`
cannot express — so all three open `MemeshDatabase` directly and inherited
the 30s cap meant for long-lived writers. Their own hooks.json budgets are
5s and 10s, so a contended lock still ended in the harness killing the
hook. All three now apply the same busy_timeout pragma.
pre-edit-recall.js has a second defect on top: it runs a guard query and
then, on the same connection, a recall query. `loadActiveGuards` swallows
the guard query's own failure by design (a guard-matching problem must
never crash this hook), so a lock still held after the busy_timeout wait
came back as "no guards matched" rather than as a visible error — and the
recall query right after it is NOT swallowed, paying the same wait again
before the hook finally gives up. Measured: ~4.4s against a 5s budget,
leaving almost nothing for the git subprocess calls and query work still
to run. A single probe before either pass now discovers a contended
connection once and gives up once.
**`status` never opened the database (M-01).** Every line it prints —
capabilities, install channel, the update check — comes from somewhere
other than the graph, so a corrupt or unreadable database file was
invisible to it: `status` printed a healthy-looking report while `doctor`
diagnosed the same file as broken. It now opens through the same
`withDatabase` every other command already uses.
**Two `config set` booleans silently stored the opposite of what was
typed (M-14).** The coercion only recognises `'true'`/`'1'` as true;
`yes`, `on`, `True` all became `false` — and the success line echoed the
raw value the user typed, not what was actually stored, so `Set
autoCapture = yes` looked like it worked. Both keys now reject a spelling
the coercion cannot read, matching every other enum-like key in this
file.
Verification:
- lint 0, typecheck 0, build 0, check-doc-claims 0, verification-audit 0
(one new baseline entry: an exitCode `.toBe(0)` in the new config tests
matched C1's emptiness heuristic; not an emptiness assertion, triaged)
- new: hook-time-budgets +3 read-only cases (per-case threshold tight
enough to separate a single busy_timeout wait from pre-edit-recall.js's
former double wait — 8s would have passed either way); error-envelopes
+3; bad-input-and-broken-db +1
- regression: tests/hooks 343 passed; pre-edit-recall/guard-check/
session-start(-unwritable) 51 passed; doctor-honest-pass 7 passed
- break-tests, each restored and rebuilt:
revert guard-check.js's pragma -> spawnSync ETIMEDOUT at 25s
revert pre-edit-recall.js's probe -> 4.4s exceeds the 4s bound
remove the autoCapture/transcriptMining
validators -> 2 of 3 new tests fail
remove `status`'s withDatabase probe -> 1 of 9 fails
…uld have seen, a latent recall window, and a demo tour that could annex someone's memory
**`memesh doctor` warned about its own translated docs on every real
install (M-08).** `README locale parity` treated README.md's mere presence
as "this install ships docs, so missing locale READMEs are a problem" —
but npm always includes README.md regardless of `files`, while the locale
READMEs are dev-only and never shipped. Every real install saw a WARN
naming translation files it was never supposed to have. Now: zero locale
READMEs present is read the same as README.md itself being absent —
packaged, skip silently — and only genuine drift (at least one locale
present, another missing or stale) still warns.
**Install ID named a path nobody's install actually uses (M-09).** The row
hardcoded `~/.memesh/install.json` while every sibling path in this file
resolves through `memeshDir()`, which a `MEMESH_DIR` override changes.
Now resolved the same way.
**A fresh install failed the one check it hadn't had time to pass yet
(M-10).** "No update check yet" is expected for an install a few minutes
old — the same 24h grace period the hook-wiring check already gives new
hooks. `isFreshInstall()` reads the install record's own `created_at`
(ISO-8601 — deliberately not `hoursSince`, which parses SQLite's
different timestamp shape and would silently read every fresh install as
"unknown age").
**A project's own candidate window had no ORDER BY (M-19).** Latent — the
largest real project measured is 177, under the 400-row cap — but above
it, `SELECT DISTINCT`'s dedup returns ascending by id: the OLDEST 400,
not the newest, with no way for ranking to ever see what fell outside
that window. The sibling "recent across all projects" query already had
the fix; this one didn't.
**A demo entity's own error-classifier comment claimed the opposite of
what it does (M-21).** Docstring-only: it described "database disk image
is malformed" as one of the messages a benign FTS delete mismatch
produces, immediately above a classifier that (correctly) refuses to
treat that message as benign. Corrected to match the code.
**Three backfills read their work list before the lock that acts on it
(M-22).** All three ran `SELECT ... FROM entities` outside `db.transaction()`,
then wrote inside it. A concurrent process — a real scenario here, since
CLI/MCP/HTTP all open the same file — could insert a row in the gap; that
row would never appear in the captured list, and the marker set at the
end of a successful pass records the backfill as permanently done anyway.
`runOnceMigration` exists for exactly this: read-and-commit under one
lock. All three now read their work list, and re-check the marker, inside
the transaction they act in.
**The demo tour could wire a real memory into its graph (M-16).** DEMO_
DATA's names (`auth-decision`, `db-choice`) are plausible names a real
memory could carry. `seedDemo` already refused to touch or duplicate a
colliding row — but `createRelation` resolves both endpoints by name
alone, with no notion of who created them, so a colliding real entity
still got a demo edge the moment any OTHER demo entity in the same run
needed inserting. Relations are now only wired between names confirmed
to actually be part of the demo tour (metadata.demo = true), whether
seeded this run or a prior one.
[Verified-By: npm run lint] exit=0
[Verified-By: npm run typecheck] exit=0
[Verified-By: npm run build] "All 6 smoke tests passed", exit=0
[Verified-By: node scripts/check-doc-claims.mjs] "Every documented claim matches the code.", exit=0
[Verified-By: node scripts/audit/verification-audit.mjs] "Every hit is triaged; every detector saw a non-empty candidate set.", exit=0 (5 baseline entries re-keyed: doctor.ts x4, demo.ts x1, each verified byte-identical before re-keying)
[Verified-By: node scripts/run-tests-isolated.mjs] "Test Files 185 passed (185) / Tests 2540 passed (2540)", exit=0
[Verified-By: npx vitest run tests/core/doctor.test.ts tests/core/briefing.test.ts tests/core/demo.test.ts] all passed (96 + 6 + 8 = 110 tests), exit=0
[Verified-By: break-tests, each restored and rebuilt after confirming red]:
remove the anyLocalePresent guard -> 1 of 96 doctor tests fails
hardcode ~/.memesh in the Install ID row -> 1 of 96 doctor tests fails
neutralise isFreshInstall() -> 1 of 96 doctor tests fails
drop ORDER BY from the project query -> 1 of 6 briefing tests fails
drop the demoNames filter on relations -> 1 of 8 demo tests fails
[Verified-By: M-22 note] three backfills have no dedicated new test — the defect is only observable under real cross-process write timing, which would need flaky multi-process machinery disproportionate to the fix; correctness rests on the same transactional-atomicity guarantee runOnceMigration already relies on, and the unchanged regression suite (185/185, 2540/2540 above) confirms no behavioural regression for the normal single-writer case.
… that could not fail, and two schemas that let a mistake through unremarked
**`install-hooks` wrote a second file and said nothing (M-11).** The
command reports the settings.json path and its backup path; the marker
`installHooks()` writes at the same time (~/.memesh/install-hooks.json —
what `doctor` reads to confirm hooks are wired) was never mentioned.
Reported now, gated on the same condition the write itself uses
(`added > 0 || skipped > 0`, not dry-run) — not on the backup path,
which is null on a fresh install with nothing pre-existing to back up.
**MCP export/import's namespace field didn't publish the constraint the
runtime enforces (M-12).** `remember`/`recall` declared `enum:
['personal', 'team', 'global']`; export/import only mentioned the three
values in the description's prose. A client had no machine-readable way
to know an arbitrary string would be rejected until it tried.
**`POST /v1/config` reported success for a mistyped key it silently
discarded (M-13).** `ConfigBody.strip()` dropped any unrecognized key
before validation ran, so `{sesionLimit: 77}` changed nothing and still
answered `200 {success: true}`. Every current dashboard caller posts a
small, purpose-built object (`{llm}`, `{autoUpdate: next}`) — never a
superset read back from GET — so nothing legitimate relied on an unknown
key being tolerated. `.strict()` now answers the same 400
`validation.bad-body` every other malformed POST on this route already
does.
**MCP `forget` could not report failure (M-17).** `ok(forget(...))`
wrapped every outcome in `isError: false` regardless of what happened —
a typo'd observation, or a name that doesn't exist, came back
indistinguishable from an actual removal to a caller checking `isError`
without parsing the JSON body. The CLI's `forget` command has always
exited 1 for exactly this. MCP now reports the same two cases (`archived
=== false`, `observation_removed === false`) as `isError: true`, with the
same messages the CLI already uses.
R20-e/R20-h/R20-k (from the same ledger) were checked against the current
tree and are already resolved by earlier work this session — R20-e's dead
fields (`toolPreferences`/`avgSessionMinutes`) no longer exist anywhere
in the codebase; R20-h's "no test observes a relation surviving import"
is now covered by two tests added alongside the R3-b fix (an existing-
target relation confirmed via SQL JOIN, and a target-appears-later
relation); R20-k's unconditional-initializer gap has its own dedicated
test file (`citation-compliance-numerator.test.ts`) from tonight's
citation-contract work, predating this review. No action needed.
[Verified-By: npm run lint] exit=0
[Verified-By: npm run typecheck] exit=0
[Verified-By: npm run build] "All 6 smoke tests passed", exit=0
[Verified-By: node scripts/check-doc-claims.mjs] "Every documented claim matches the code.", exit=0
[Verified-By: node scripts/audit/verification-audit.mjs] "Every hit is triaged; every detector saw a non-empty candidate set.", exit=0 (no new hits — none of these touched a C-class pattern)
[Verified-By: node scripts/run-tests-isolated.mjs] "Test Files 185 passed (185) / Tests 2544 passed (2544)", exit=0
[Verified-By: node scripts/run-tests-isolated.mjs tests/core/install-hooks.test.ts tests/core/schema-export.test.ts tests/tools.test.ts tests/transports/http.test.ts tests/transports/forget-selector-safety.test.ts]: all passed (17 + 14 + 43 + 65 + 9 = 148 tests), exit=0
[Verified-By: break-tests, each restored and rebuilt after confirming red]:
remove the Marker console.log in install-hooks -> 1 of 17 fails
drop the enum from export/import's namespace field -> 1 of 14 fails
ConfigBody back to .strip() -> 1 of 78 fails
forget handler back to bare ok(forget(...)) -> 2 of 43 fail
resolveFileCommits() ran `git ls-files --error-unmatch` to check whether
the argument was tracked — but that command only checks git's index,
never the filesystem, so it fails identically for "this file exists but
isn't committed" and "no such file". Both landed on `file_not_tracked`,
whose message ("File is not tracked by git") reads as confirmation that
a real file exists and just needs a commit — sending a caller who
mistyped a path looking in the wrong place.
A new `file_not_found` abstention is checked first (`existsSync`, before
the git call), with its own message ("No such file.") and its own exit
code: 1, matching the caller-mistake convention `pin`/`forget` already
use for "the thing named does not exist" — every other `why` abstention
is about a real file `why` cannot fully explain, so exit 0 for those is
unchanged.
[Verified-By: npm run lint] exit=0
[Verified-By: npm run typecheck] exit=0
[Verified-By: npm run build] "All 6 smoke tests passed", exit=0
[Verified-By: node scripts/check-doc-claims.mjs] "Every documented claim matches the code.", exit=0
[Verified-By: node scripts/audit/verification-audit.mjs] "Every hit is triaged; every detector saw a non-empty candidate set.", exit=0 (5 baseline entries re-keyed in why.ts/cli.ts, each verified byte-identical before re-keying)
[Verified-By: node scripts/run-tests-isolated.mjs] "Test Files 185 passed (185) / Tests 2546 passed (2546)", exit=0
[Verified-By: node scripts/run-tests-isolated.mjs tests/core/why.test.ts tests/cli/why.test.ts] "Test Files 2 passed (2) / Tests 20 passed (20)", exit=0
[Verified-By: break-test, restored and rebuilt after confirming red]: remove the existsSync check -> 2 of 20 why tests fail (both new ones)
… say why, and an import that grew without bound (M-05/M-06/M-18)
**`--obs " "` stored a memory with nothing in it (M-05).** Dogfooded on
the real v4.7.1 release: the CLI's `remember` calls `remember()` directly,
never through Zod, so nothing rejected a whitespace-only observation —
`"observations": [" "]` landed in the graph. Fixed at both points:
`RememberSchema` gets a per-element `.refine()` (protects MCP/HTTP), and
the CLI gets its own check mirroring `forget`'s existing `nonEmpty`
pattern (protects the path that bypasses the schema). `ExportResultSchema`
is deliberately NOT touched — it is the one schema this file documents as
tolerant by design, restoring a bundle that may predate this validation.
**A zero-hit couldn't say whether semantic search even ran (M-06).**
"No results found." was identical whether the install had no embedder
configured (there might be something related this pass could not see) or
a fully working one searched everything and still found nothing (this
really searched everything) — two very different confidence levels with
one indistinguishable line. The plain-text path now reads
`retrieval.mode`/`degraded` (already computed, already in the `--json`
envelope) the same way the sibling semantic-only-results message already
does.
**`import --merge append` duplicated observations without bound (M-18).**
Dogfooded: re-running the same import twice duplicated every observation
a second time, with no cap on further re-runs — the ordinary shape of
restoring the same backup twice, or two bundles sharing an entity.
`createEntity` has no dedupe of its own (correct for `remember`, where
re-asserting a fact may be deliberate) — import now filters each entity's
incoming observations against what it already has before calling it.
**`import --merge overwrite` printed the identical line for a destructive
replace and a harmless first-time create (M-18).** Both incremented the
same `imported` counter. A new `overwritten` field (subset of `imported`)
tracks entities that already existed and had their data repladed —
carried through the CLI's own message, the type, and the documented
response shape.
[Verified-By: npm run lint] exit=0
[Verified-By: npm run typecheck] exit=0
[Verified-By: npm run build] "All 6 smoke tests passed", exit=0
[Verified-By: node scripts/check-doc-claims.mjs] "Every documented claim matches the code.", exit=0 (ImportResult's new `overwritten` field kept in sync with its documented response shape)
[Verified-By: node scripts/audit/verification-audit.mjs] "Every hit is triaged; every detector saw a non-empty candidate set.", exit=0 (7 baseline entries re-keyed, 2 new SAFE-GUARDED entries added — both verified against the sibling validator/type that makes the guarded case real, not hidden)
[Verified-By: node scripts/run-tests-isolated.mjs] "Test Files 185 passed (185) / Tests 2557 passed (2557)", exit=0
[Verified-By: break-tests, each restored and rebuilt after confirming red]:
remove the CLI --obs check + the Zod refine (both) -> 3 of 53 fail
revert the zero-hit retrieval-mode message -> 1 of 5 fails
remove the observation dedupe + the overwritten count (both) -> 4 of 53 fail
…I runners guard-check.js and pre-edit-recall.js both failed PR #203's CI on macOS (Node 22 and Node 24) against the 4000ms flat ceiling this test asserted per hook — guard-check.js measured 4516ms, pre-edit-recall.js 4378ms, on a script that does exactly one busy_timeout wait either way. All six Ubuntu legs passed; the ceiling was sized from local timing (~2.2s single wait, ~4.4s double) that never accounted for macOS GitHub Actions process-spawn overhead, which alone ran ~2.4-2.5s on top of the 2s busy_timeout wait. Replaced the flat ceiling with a same-script self-difference: each case now runs the hook once unlocked (the process-spawn floor) and once against an exclusive lock, and asserts the DIFFERENCE stays under 1.5x HOOK_BUSY_TIMEOUT_MS. Both spawns pay the runner's spawn noise about equally, so subtracting cancels it — the assertion is on the lock wait itself, not on wall-clock plus whatever the runner happened to be doing. Also fixed a real bug in the new design before it shipped: the first draft ran the floor and contended spawns with the same file_path for pre-edit-recall.js, and that hook throttles repeat recalls of one file per session (session-recalled-files.json, keyed on memeshDir — which the two spawns deliberately share). The floor run's throttle write silently made the contended run skip its recall query, collapsing it to a single query regardless of which code was under test — a false negative that would have passed unfixed code. Caught by the break-test below, not by inspection. Fixed by giving each spawn its own file_path via a `makeInput(tag)` function instead of a fixed input object. [Verified-By: node scripts/run-tests-isolated.mjs tests/hooks/hook-time-budgets.test.ts] Test Files 1 passed (1), Tests 6 passed (6) [Verified-By: break-test] reverted pre-edit-recall.js's probe fix -> 'pre-edit-recall.js' case failed: "paid 4202ms above its own 64ms unlocked floor (contended run took 4266ms)" (expected < 3000ms); restored -> green again [Verified-By: break-test] reverted guard-check.js's busy_timeout pragma -> 'guard-check.js' case failed with spawnSync ETIMEDOUT at 25051ms (inherited the 30s default); restored -> green again [Verified-By: npm run typecheck] exit 0 [Verified-By: node scripts/run-tests-isolated.mjs] Test Files 185 passed (185), Tests 2557 passed (2557), no Errors line [Verified-By: npm run verify:release] exit 0, all 8 verification-audit detectors new=0, check-doc-claims all pass
Commit 3e8c9d0 replaced a flat 4000ms ceiling with a same-script floor-subtracted one (contended minus unlocked), capped at 1.5x HOOK_BUSY_TIMEOUT_MS (3000ms) — and that failed too, on the SAME macOS CI legs, for all three scripts at once: guard-check.js 3013ms, session-start.js 3169ms, pre-edit-recall.js 3092ms. All three currently carry their correct fix; nothing regressed between the two pushes. What the second failure actually shows: a contended busy_timeout wait costs more than its nominal 2000ms even after subtracting a script's own process-spawn floor — apparently ~50% more under real lock contention on a loaded macOS runner (retry granularity, WAL/journal state the unlocked floor run never touches) — and that premium is itself runner-dependent, so a second guessed multiplier was just a different flat ceiling with extra steps. The three same-run measurements clustered within ~5% of each other (3013/3169/3092), which is the actual finding: whatever "one wait" costs right now, on this runner, under this load, different scripts pay almost exactly the same premium for it. So stop guessing that number and read it off a live reference instead. guard-check.js can only structurally ever pay one wait (one query, no swallow-and-continue), so it is measured fresh in the same test run and used as the "what does 1x cost right now" baseline; pre-edit-recall.js's own paid-wait is compared against THAT measurement (< 1.6x), never against a constant written down in this file. guard-check.js's and session-start.js's own cases keep a generous flat 15000ms ceiling — the regression they guard against (pragma never applied, inherits the 30s meant for long-lived writers) is off by an order of magnitude either way, so it never needed fine calibration in the first place. [Verified-By: node scripts/run-tests-isolated.mjs tests/hooks/hook-time-budgets.test.ts] Test Files 1 passed (1), Tests 6 passed (6); repeated 3 more times back to back, all green (4564ms/4568ms/4559ms for the pre-edit-recall.js case) [Verified-By: break-test] reverted pre-edit-recall.js's probe fix -> failed: "pre-edit-recall.js paid 4291ms vs guard-check.js's own 2182ms single-wait reference ... expected 4291 to be less than 3491.2"; restored -> green again [Verified-By: break-test] reverted guard-check.js's busy_timeout pragma -> BOTH guard-check.js's own case AND the pre-edit-recall.js case (which uses it as a reference) failed with spawnSync ETIMEDOUT at 25s, correctly cascading; restored -> green again [Verified-By: npm run typecheck] exit 0 [Verified-By: node scripts/run-tests-isolated.mjs] Test Files 185 passed (185), Tests 2557 passed (2557), no Errors line [Verified-By: npm run verify:release] exit 0, all 8 verification-audit detectors new=0, check-doc-claims all pass
2 tasks
… one
The previous fix widened this file's rmSync retry window from 5/100ms to
10/200ms on the theory that Windows was lagging on releasing a handle after
the child process exited. It failed again, identically, on the next CI run —
which disconfirms the theory rather than under-tuning it: a 2s window that
does not help means the holder is not lagging, it is still running.
Reading session-start.js settles it. Every runHook() reaches
runPostBannerUpdateTasks() -> spawnFreshUpdateCheck(), which spawns a
detached, unref'd `node dist/transports/cli/cli.js status` against the test's
HOME to refresh the update-check cache. That spawn is deliberately
fire-and-forget so session start never blocks on a slow npm lookup, and
nothing waits for it. On a loaded Windows runner it can still be mid-flight
during afterEach, holding a handle under <home>/.memesh. No retry budget
fixes a process that has not finished.
The one failing case being the only test that calls runHook() twice matches
this exactly, and matches nothing about a timing distribution.
This test asserts that the SessionStart hook writes the citation contract
into the installed scope. It does not assert that Windows can rmdir a temp
directory, so a cleanup failure should not fail it — a leaked temp dir on a
CI runner is harmless. ENOTEMPTY/EBUSY/EPERM are tolerated; every other error
still throws. The stale comment asserting the widened window was sufficient
is replaced with the mechanism.
Local: node scripts/run-tests-isolated.mjs tests/hooks/citation-rule-self-heal.test.ts
exit=0, Test Files 1 passed (1), Tests 6 passed (6)
npm run typecheck exit=0
…s-only
The previous commit read this as a Windows handle-release lag and tolerated it
in one file. Half right. The same run then failed the Release Verification Gate
on ubuntu, in a different file — tests/hooks/session-start-unwritable.test.ts,
ENOTEMPTY on rmdir '/tmp/memesh-unwritable-yQYt8W/.memesh'. Same error, same
.memesh subdirectory, no Windows involved.
Mechanism, observed rather than reasoned about. Ran the hook against a scratch
HOME and watched it:
- the hook process exits
- `pgrep -f "cli.js status"` still shows a live child (PID 37163)
- ~1s later `update-check.4.7.1.json` appears in <home>/.memesh
That child is spawnFreshUpdateCheck (scripts/hooks/session-start.js:393):
detached, unref'd, deliberately fire-and-forget so session start never blocks
on a slow npm lookup. So cleanup is not waiting on a lagging handle, it is
racing a process that is still writing: rmSync enumerates .memesh, deletes what
it saw, calls rmdir, and the child has created a new file in between. That is
why widening the retry window from 5/100ms to 10/200ms changed nothing — no
retry budget outlasts a writer that has not finished.
Exactly two test files spawn the hook against a HOME they then delete
(release-scripts-safety.test.ts only writes a file *named* session-start.js; it
never runs it). Both now share one helper instead of carrying two copies of the
same reasoning — a duplicated explanation is the defect class this repository
has been bitten by before.
Neither test asserts anything about cleanup, and a leaked temp directory on a
CI runner is harmless, so the honest cleanup is to let the race lose quietly.
ENOTEMPTY/EBUSY/EPERM are absorbed; every other code still throws, and
temp-home.test.ts pins that narrowness — an unqualified `catch {}` would hide a
genuinely broken cleanup, which is worse than the flake it fixes.
No user impact: ~/.memesh is never deleted out from under a real session.
Local evidence:
node scripts/run-tests-isolated.mjs exit=0
Test Files 186 passed (186)
Tests 2563 passed (2563)
npm run typecheck exit=0
break-test — widened the catch to swallow everything:
exit=1, FAIL tests/helpers/temp-home.test.ts > removeTempHome >
rethrows anything else, so a genuinely broken cleanup is still visible
(1 failed | 5 passed); file restored and re-verified byte-identical
…wning hook Windows Node 24 failed again on PR #203, in a third file and with a third error code: EPERM, Permission denied: C:\Users\RUNNER~1\...\Temp\memesh-readonly-budget-1x3nOq tests/hooks/hook-time-budgets.test.ts:160 withTempDb's finally Same cause, different surface. That test spawns session-start.js against a temp DB via MEMESH_DB_PATH, and the detached update-check child inherits the env — so it holds the database file open. On Windows an open file cannot be unlinked, which is EPERM, where the earlier failures were the child *creating* a file mid-cleanup, which is ENOTEMPTY. One race, two symptoms. My earlier scan for exposed files was too narrow: it filtered on tests that set `HOME:`, and this one isolates with `MEMESH_DB_PATH:` instead. Re-scanned on the real criterion — a test that executes a hook which spawns a detached child, and then deletes a temp directory. Exactly two hooks spawn detached children (`grep -c "detached: true"`): session-start.js and session-summary.js. Ten test files run one of them and then rmSync a temp dir. Three had already failed in CI; the other seven are the same trap waiting on runner load, and finding them one full 13-leg re-run at a time is not a plan. All eleven cleanup sites were byte-identical (`maxRetries: 5, retryDelay: 100`), so this is one mechanical substitution, not eleven judgements. Renamed removeTempHome -> removeTempDir, temp-home.ts -> temp-dir.ts. Half these directories are not HOMEs — hook-time-budgets' is a scratch database — and a helper whose name misdescribes half its callers is the kind of small lie this repository has paid for before. Evidence: node scripts/run-tests-isolated.mjs exit=0 Test Files 186 passed (186) Tests 2563 passed (2563) npm run typecheck exit=0 tests/helpers/temp-dir.test.ts exit=0, 6 passed (the narrowness guard: ENOTEMPTY/EBUSY/EPERM absorbed, EACCES rethrown — break-tested in the previous commit, mutant killed)
…evert it
Checking my own sweep. The criterion was "runs a hook that spawns a detached
child, then deletes a temp dir", and I applied it by grepping for files that
reference session-start.js / session-summary.js and execute *something* via
node. install-hooks.test.ts matches that grep but not the criterion: what it
actually executes is `cli.js install-hooks`, and the CLI's only detached spawn
is in `feedback` (opening a browser), not on this path.
So there is no live writer here, and `removeTempDir` would have been a claim
this file makes about itself that is not true — plus an error-swallowing
cleanup in a file with nothing to swallow, which could hide a real failure
later. Reverted to a plain rmSync.
The other seven were re-checked the same way and all genuinely resolve to
scripts/hooks/session-start.js or session-summary.js.
node scripts/run-tests-isolated.mjs tests/core/install-hooks.test.ts
exit=0, Test Files 1 passed (1), Tests 17 passed (17)
npm run typecheck exit=0
This was referenced Aug 24, 2026
Merged
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.
Summary
Continuation of tonight's full-codebase review (the ledger that produced
R1-R19 in v4.7.0). Works through the M-01..M-23 "minor/friction" items and
the R7-a gap the ledger left half-fixed, verified against a real, freshly
installed v4.7.1 (not just source review) — several findings (M-02/M-03/
M-05/M-06/M-18) were confirmed by actually running the released CLI in a
throwaway HOME before being fixed here.
5 commits, in landing order:
3288cc6a— R7-a completion (3 hooks still inherited the 30s busy_timeoutopenHookDb's earlier fix capped only for writers) + M-01 (
statusneveropened the database) + M-14 (
config set autoCapture yessilently storedfalse)aa56a587— M-08/M-09/M-10 (doctor rows a real install should never haveseen) + M-16 (demo tour could wire a real memory into its graph) + M-19
(candidate window had no ORDER BY) + M-21 (stale docstring) + M-22 (three
backfills' read-then-lock race)
ef786453— M-11 (install-hooks marker written silently) + M-12 (MCPnamespace field missing its enum) + M-13 (
.strip()silently discarded amistyped config key) + M-17 (MCP
forgetcould not report failure)145fc2d6— M-07 (a typo'd path was told it was a real, uncommitted file)076f7427— M-05 (whitespace-only observation stored) + M-06 (zero-hitcouldn't say whether semantic search ran) + M-18 (
import --merge appendduplicated without bound;
overwriteand a fresh create printed theidentical line)
Not in this PR, checked and genuinely not defects:
--force-named memory) —memesh recall -- --forcealready works;standard Commander
--behavior, confirmed empirically.(predates this review), confirmed against current tests.
Still open, deliberately deferred (flagged to the user as scope
decisions rather than fixed unilaterally): M-02 (no partial-word FTS
matching), M-03 (emoji queries return nothing), M-20 (two
metadatawriters disagree on handling unparseable values). M-02/M-03 touch FTS5
tokenization and need more careful design than a same-session fix; M-20 is
a product judgment call, not a clear-cut bug.
Test plan
npm run lint— 0npm run typecheck— 0npm run build— 6/6 smoke testsnode scripts/check-doc-claims.mjs— every documented claim matchesnode scripts/audit/verification-audit.mjs— every hit triagednode scripts/run-tests-isolated.mjs— 185 files / 2557 tests passedred → restore → confirm green) — evidence in each commit's
[Verified-By: break-tests ...]footer