fix: attach reindex-fts and repair-summaries to the memory-pro command group

[gorkem2020]

Every user-facing command in this file is meant to live under the memory-pro group (program.command("memory-pro"), built at the top of registerMemoryCLI) so it can be invoked as memory-pro <command>. reindex-fts and repair-summaries were instead registered directly on the root commander program. Since the host's dispatcher only routes the one declared root command (memory-pro), both commands were unreachable through any actual invocation path, no matter how they were called. This looks like it has been the case since whichever commit introduced them; there was no test exercising the actual command tree that would have caught it.

• reindex-fts and repair-summaries are now attached to the memory group variable instead of program, so they're invoked as memory-pro reindex-fts and memory-pro repair-summaries, matching every other command in this file.
• Added a structural test that instantiates createMemoryCLI against a minimal stub context on a fresh commander Command and inspects the actual registered command tree: the root program must expose exactly one command (memory-pro), and both reindex-fts and repair-summaries must be reachable underneath it.

Test plan

• Red-first: test/cli-subcommand-attachment.test.mjs, three tests, all failed before the fix (root program had four commands instead of one; neither reindex-fts nor repair-summaries appeared in the group's subcommand list) and passed after.
• Grepped the test suite and docs for any existing reference to invoking these two commands; found none, so nothing else assumed the old (broken) root-level path.
• npm run build (tsc) clean.
• Full local npm test chain green (one test skipped: a known host-side port 11434 conflict unrelated to this change).
• test/cli-subcommand-attachment.test.mjs registered in both package.json's test script and scripts/ci-test-manifest.mjs; manifest verification passes.

Notes for reviewers

No user-facing invocation path changes for anyone currently on master, since neither command was reachable before this fix either way. This just makes two already-shipped features usable for the first time.

[app3apps]

Reviewed head 3bb1522. The command-tree attachment itself is correct, the three focused tests pass, the build is clean, and GitHub CI is green. The generated dist/index.js catch-up is reproducible from the already-merged source change and is not a blocker.

However, this PR's stated effect is to make these maintenance actions usable through the supported CLI path for the first time, and both action bodies have unsafe observable behavior:

1. cli.ts:2162-2198 treats a mismatch between the first 60 characters of text and l0_abstract as staleness. L0 is designed to be a concise generated abstract, so valid enriched memories are classified as stale; because --dry-run defaults to false, their L0/L1/L2 metadata is overwritten with raw text. Conversely, a real text change after character 60 is missed. Please use reliable source-text provenance/versioning (with a conservative legacy policy), make mutation explicitly opt-in, and add action-level regressions for both cases.

2. src/store.ts:2630-2642 suppresses dropIndex failures. The surviving index then makes recreation a no-op, but the method still marks FTS available and returns success. Please propagate/verify failed drops and verify that a replacement index exists before reporting success, with a failed-drop regression test.

These need to be safe before the PR exposes the commands as supported maintenance operations.

[rwmjhb]

Checked head 37c97a6. GitHub currently reports mergeable=CONFLICTING / merge_state_status=DIRTY, and the branch is behind the latest master, so the deep review is deferred until the resulting diff is stable.

Please rebase onto current master (or merge it into this branch), resolve all conflicts, preserve the existing CI test-manifest entries while adding the new CLI test, rebuild dist, and push the resolved branch. We will re-review the new head; the previously reported action-safety findings also remain subject to that review.

[gorkem2020]

Rebased onto the current master as requested, head is now fe0d45b. The conflict was the package.json test chain; resolved as a union, all existing CI test-manifest entries are preserved and the new CLI test (test/cli-subcommand-attachment.test.mjs) stays registered in both the chain and scripts/ci-test-manifest.mjs. The stale dist rebuild commit was dropped and dist verified against a fresh build on the rebased source (byte-identical). Local checks green: the CLI attachment suite (3/3), cli-smoke, and the manifest regression suite. Ready for the deep review.

[rwmjhb]

Re-reviewed head fe0d45b after the conflict resolution. The command-tree attachment is correct and the focused tests pass, but the previously reported action-safety blockers remain unchanged.

1. repair-summaries still defaults to mutation and decides staleness by comparing the first 60 characters of raw text with l0_abstract, even though L0 is intentionally a concise generated abstract. A normal invocation can overwrite valid L0/L1/L2 metadata with raw text, while a real source change after character 60 is missed. Please use reliable source provenance or a hash/version, make mutation explicitly opt-in, and add action-level regressions for both cases.

2. rebuildFtsIndex still suppresses dropIndex failures. The surviving index then makes creation a no-op, but the command marks FTS available and reports success. Please propagate or verify drop failures and confirm a replacement index exists before returning success, with a failed-drop regression.

Also make repair-summaries return a failing process status when any update throws or returns null. Requesting changes.

[gorkem2020]

Addressed in 4159ba7. repair-summaries: the text-vs-L0 prefix heuristic is gone; detection now reads the RAW stored metadata and flags only mechanically reliable degenerate shapes (missing summary levels, or the legacy parse-fallback signature of all three levels identical), so a healthy generated abstract can never be classified stale, and mutation is opt-in via --apply (report-only default, --dry-run kept as an alias that always wins). rebuildFtsIndex: a failed dropIndex now aborts the rebuild before creation and surfaces in the returned error instead of reporting success against a surviving index. Action-level regressions cover both: healthy summaries never flagged or overwritten even with --apply, missing/degenerate rows repaired only under --apply, drop failure fails the rebuild without running creation.

[rwmjhb]

Re-reviewed head 4159ba7. The new --apply gate and propagated FTS drop failures address the two previous blockers. Three repair-path behaviors still need correction before exposing the command.

1. The scan selects every row missing any L0/L1/L2 field, but current reflection-event, reflection-item, and reflection-mapped rows intentionally omit these smart-summary fields and are excluded by the upgrader. repair-summaries --apply would rewrite valid reflection metadata. Please reuse the current-reflection exclusion and add coverage for all three schemas.

2. A row is selected when only one summary level is missing, but the update always replaces all three levels. For example, repairing a missing L2 destroys valid generated L0/L1 values. Preserve existing valid levels and fill only the missing fields, with a partial-metadata regression.

3. The return value from store.update() is ignored and repaired is incremented even when it returns null. Thrown update errors are counted but the command still exits successfully. Treat null as failure and return a nonzero status whenever any repair fails, with null-return and thrown-error tests.

The full-suite run also exceeded the local 180-second harness limit, but I am requesting changes for the concrete behaviors above rather than treating the timeout as an assertion failure.

[rwmjhb]

The command attachment and the report-only/apply safety work are in good shape. The full verification suite passes, and an independent build leaves dist/ unchanged. I found one small but blocking repair-path bug:

• In repair-summaries, JSON.parse(entry.metadata) is cast directly to Record<string, unknown>. Valid JSON such as null parses successfully, so the catch does not run; the next rawMeta.l0_abstract access throws and aborts the entire scan. One imported or damaged row can therefore prevent every later row from being reported or repaired.

Please accept the parsed value only when it is a non-null, non-array object; otherwise normalize it to {} and treat the summary levels as missing. A focused test with null (and ideally primitive/array metadata) should also verify that scanning continues to subsequent rows. After that, this looks ready.

[gorkem2020]

Fixed in 099c845. The scan now accepts the parsed metadata only when it is a non-null, non-array object; anything else normalizes to {}, so the row reads as missing all summary levels and is reported and repairable instead of aborting the run.

Tests added: a scan-continues case with JSON null metadata on the report path (asserts the later row is still reported and no abort fires), and a null/primitive/array case under --apply verifying all three damaged rows get repaired while a healthy row stays untouched. Also removed the now-unused parseSmartMetadata import.

[rwmjhb]

Re-reviewed head fa4afe7. The command attachment is correct, the report-only/apply behavior and prior reflection/partial-metadata/null-update fixes are substantially improved, and the focused/full suites pass. Two action-safety races remain before these commands should become supported entrypoints.

repair-summaries --apply builds a complete replacement metadata document from the earlier paginated scan snapshot. MemoryStore.update() later replaces the current metadata wholesale, but its write lock does not cover that scan. If the running gateway updates access counters, lifecycle state, relations, or supersession metadata between scan and apply, the repair silently restores the old snapshot and loses those changes. Please add a store-level atomic repair/transform operation that re-reads current metadata under the write lock, re-evaluates whether the summary fields still need repair, and patches only the affected L0/L1/L2 fields. Add an interleaving regression proving unrelated concurrent metadata survives.

rebuildFtsIndex() now propagates drop failures, but it continues dropping all matching indexes and throws only afterward. If an earlier drop succeeds and a later drop fails, creation is skipped and the store is left with a partially deleted FTS index set. Please preflight/serialize this operation so failure cannot leave already-dropped indexes unrecreated, or compensate before returning the error. Add a two-matching-index regression with the second drop failing.

The repeated full-table pagination and scoped NULL-row mismatch are lower-priority operational issues. Requesting changes for the stale metadata replacement and partial-drop state.

[gorkem2020]

Head a0c3e9f addresses both action-safety races.

Atomic repair. The store gains transformMetadata(id, transform, scopeFilter): the row is re-read UNDER the write lock (composing the same locked update body update() uses, now shared), the caller's transform decides from that fresh state, and the returned patch is merged onto the CURRENT metadata rather than replacing it wholesale. repair-summaries --apply now routes every repair through it: the transform re-runs the same missing/degenerate judgment the scan used, so a row the gateway healed between scan and apply is skipped and reported as skipped, and a row still needing repair gets ONLY its L0/L1/L2 fields patched. The interleaving regression seeds a degenerate row on a real store, lands an unrelated bad_recall_count update between the scan page and the apply loop, and asserts that update survives while the levels are still repaired; it failed on the previous head (the counter was reverted by the snapshot-built document). A companion case pins the concurrent-heal path (skipped, nothing rewritten).

Partial FTS drops. rebuildFtsIndex now stops at the first drop failure instead of continuing across the remaining matches, and when at least one index was already dropped it recreates the FTS index before the error propagates, so a failure can no longer leave already-dropped indexes unrecreated. The two-matching-index regression fails the second drop and asserts the compensating creation ran and is named in the error; a second case pins the fail-fast half (first drop fails: nothing else touched, nothing to compensate). The rebuild still reports failure in both shapes, since a rebuild that did not replace the index did not do its job.

The repeated full-table pagination and the scoped NULL-row mismatch from your note stay open as the lower-priority operational items. Full suite, typecheck, and a fresh dist are green on the new head.

[rwmjhb]

Thanks for the update. Current head a0c3e9f has merge conflicts with the latest base, so the new changes cannot be reliably re-reviewed yet. Please rebase onto the latest master, resolve the conflicts, rerun the relevant tests, and push the resolved head for another review.

[rwmjhb]

Re-reviewed head 3dc69ca. The command attachment itself is fixed and the focused CLI/storage tests pass, but two storage blockers remain in the newly reachable repair path.

1. transformMetadata() acquires the write lock and then reads through the existing table handle without checkoutLatestTableForWrite(). Other cross-process-safe write paths refresh after locking. In a two-MemoryStore reproduction with default pinned consistency, one connection wrote bad_recall_count=7; the repair connection read its stale snapshot and persisted 0. Please refresh the latest table under the lock before the current-row read, and fail safely if refresh is unavailable. Add a default-consistency two-connection regression.

2. The transform is not a summary-only raw metadata patch. Routing the row through buildSmartMetadata()/stringifySmartMetadata() materializes unrelated classification, lifecycle, counter, and timestamp defaults. A legacy row consequently gains memory_category and is then treated as current by MemoryUpgrader, preventing its intended enrichment. Please merge only the callback's explicit keys into a validated raw metadata object and preserve every unrelated field and absence exactly. Add a regression proving repaired legacy rows remain upgrader-eligible.

The apply-time reflection recheck and partial FTS-drop compensation are worthwhile follow-ups, but the stale cross-process read and legacy metadata promotion are the merge blockers.

[gorkem2020]

Both blockers are fixed in 3d90c38.

1. Stale read under the lock: transformMetadata now refreshes via checkoutLatestTableForWrite() before the current-row read, matching the other cross-process-safe write paths. Regression added: a two-connection, default-consistency cell where a counter written through a second connection between scan and apply survives the repair (it reproduced the bad_recall_count clobber before the fix).

2. Raw surgical merge: the transform no longer routes through buildSmartMetadata. It parses the row's raw metadata (refusing unparseable or non-object metadata instead of clobbering it), merges only the callback's explicit keys, and preserves every unrelated field and absence exactly. Regression added: a repaired legacy row keeps a keys-exact metadata shape, gains no memory_category, and stays MemoryUpgrader-eligible.

Both cells are red on the previous head and green on this one; the full chain plus the cli-smoke and storage-and-schema groups pass. The apply-time reflection recheck and the partial FTS-drop compensation are tracked as follow-ups on our side.

[rwmjhb]

Re-reviewed head 3d90c38. The prior blockers are fixed: transformMetadata refreshes to the latest table version under the write lock and surgically merges raw metadata without materializing unrelated defaults. All targeted tests, an independent full-suite rerun, and repository CI pass; the earlier aggregate timeout was review-host contention. I found no remaining HIGH/CRITICAL issue. Approving.

Follow-ups: align scan/apply behavior for malformed or non-object metadata; make partial FTS-drop compensation verify that a real FTS index was recreated rather than treating any surviving text-column index as sufficient; and repeat reflection exclusion against the fresh apply-time row.

[rwmjhb]

Thanks for the update. After the recent merges, current head 3d90c38 now has merge conflicts with the latest master, so the approved revision cannot be merged as-is. Please rebase onto the latest master, resolve the conflicts, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

[gorkem2020]

Rebased onto current master (post #952) as requested; review-round history squashed into a single commit for a clean re-verification. Full suite and the cli-smoke group green, dist rebuilt.

[rwmjhb]

Reviewed head 8b2e312 after the rebase. The current-head targeted CLI/storage set passes 8/8, the build and committed dist artifacts match, CI is green, and the previously approved command attachment and repair-safety fixes remain intact. The malformed-metadata scan/apply alignment, apply-time reflection recheck, and strict FTS compensation verification remain the already-accepted follow-ups; I found no new merge blocker.

[rwmjhb]

Thanks for the update. After #934 merged, current head 8b2e312 now has merge conflicts with the latest master, so the approved revision cannot be merged as reviewed. Please rebase onto the latest master, resolve the conflicts while preserving the reviewed repair/FTS behavior, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

[gorkem2020]

Rebased onto current master (post #934): conflict was the package.json test chain only, resolved by union. Typecheck, build (dist recommitted), manifest verifier, full suite, and the cli-smoke group are green. Mergeable again.

[rwmjhb]

Re-reviewed rebased head 7413f18. The conflict resolution is limited to the package test-chain union. The maintenance commands remain attached beneath memory-pro, the focused CLI tests and full suite pass, and GitHub CI is green.

The previously accepted follow-ups remain unchanged: align malformed-metadata scan/apply policy, verify a genuine FTS index after partial-drop compensation, and repeat the reflection exclusion during apply-time revalidation. None is a regression introduced by this rebase or blocks the command-attachment fix.

Approving.

[rwmjhb]

PR #941 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes.

[gorkem2020]

Rebased onto current master (post #941); registration files re-unioned, typecheck, cli-smoke, and the full suite pass, dist rebuilt in-commit.

[rwmjhb]

Re-reviewed rebased head 759c3e3. The conflict resolution preserves the previously approved command attachment and maintenance-operation safeguards; targeted tests, the full suite, source/dist verification, and GitHub CI are green.

The malformed-metadata repair policy, apply-time reflection recheck, FTS compensation/index preservation, and stale-summary detection cases remain non-blocking follow-ups.

Approved.