test(client): Plan 2 — registry + indexer integration coverage

[dubadub]

Summary

• client/tests/registry_tests.rs — 9 integration tests pinning every public registry fn (create, update_jid, delete, non_deleted, updated_locally, latest_jid) against a real SQLite tempdir pool, including append-only delete semantics, latest-per-path queries, namespace isolation, and NotFound-on-empty latest_jid.
• client/tests/indexer_tests.rs — 7 filesystem-driven tests for check_index_once covering new / no-op / modified (append-not-UPDATE) / deleted / ineligible-filter+recursion / dotfile allowlist / symlink-skip paths.
• client/tests/indexer_run_tests.rs — 2 multi-thread async tests for indexer::run covering initial-scan + event-driven re-scan + graceful cancel, plus a pre-cancelled-token fast-exit.

No production code changed. All 18 new tests plus the whole cooklang-sync-client suite stay green; clippy is clean on the new files.

Stacked on #15 (Plan 1) — base branch is test-coverage-plan-1, not main. Once Plan 1 merges, this PR retargets to main automatically.

Plan 2 behavior pins (load-bearing for later refactors)

• registry::delete is append-only: asserting two rows exist after delete, newest with deleted=true — guards the non_deleted query's max(id) subselect.
• indexer::check_index_once appends a new CreateForm on modified files (no UpdateForm path): asserts exactly 2 rows exist for the path after mutation.
• registry::latest_jid returns diesel::result::Error::NotFound (not Ok(0)) when no row has a jid: callers must handle this distinction deliberately.
• Cross-namespace isolation: exercised on every query test with a second-namespace row that would fail the assertion if it leaked.
• Whole-second modified_at equality: modified-file tests wait ≥1.1s before rewriting so truncate_to_seconds produces a distinguishable value.

Test plan

• cargo test -p cooklang-sync-client --test registry_tests — 9/9 pass
• cargo test -p cooklang-sync-client --test indexer_tests — 7/7 pass
• cargo test -p cooklang-sync-client --test indexer_run_tests — 2/2 pass
• cargo test -p cooklang-sync-client (full suite) — all green
• cargo clippy -p cooklang-sync-client --tests --no-deps -- -D warnings — no new warnings on the three new files
• No production code modified (git diff --stat on client/src/ is empty for this delta)
• Spec-compliance + code-quality review each passed per task group
• Final whole-branch review: READY TO MERGE

Follow-up plans (out of scope here)

• Plan 3 — client/src/remote.rs (wiremock) + client/src/syncer.rs
• Plan 4 — server unit tests (auth, chunk_id, metadata, notification, request/response)
• Plan 5 — server route tests (metadata routes, chunks routes, middleware, create_server smoke)
• Plan 6 — end-to-end client↔server tests on an ephemeral port

[claude]

Code Review — Plan 2: registry + indexer integration coverage

Overall this is a high-quality test PR. The tests are well-structured, cover the right behavioral invariants, and the namespace-isolation checks in every query test are exactly the right thing to pin. A few things worth addressing:

---

Correctness issues

non_deleted ordering assertion diverges from plan but is correct in the test

The planning doc (Task 4) expected [("a.cook", 11), ("b.cook", 20)] but the committed test at registry_tests.rs:497 correctly asserts [("b.cook", 20), ("a.cook", 11)]. Since non_deleted uses ORDER BY id ASC and b.cook (id=2) was inserted before the modified a.cook (id=3), the committed assertion is right. Minor continuity issue between the plan doc and the actual test, but the test itself is correct — no action needed.

assert!(rows[1].modified_at >= rows[0].modified_at) should be strict >

indexer_tests.rs:230 — the test sleeps 1.1s specifically so the whole-second modified_at values are distinct. The assertion should enforce that:

// Current (weak):
assert!(rows[1].modified_at >= rows[0].modified_at);

// Better:
assert!(rows[1].modified_at > rows[0].modified_at,
    "mtime must advance after >=1s sleep; if equal, the sleep was too short");

If CI ever runs this under heavy load and the kernel rounds both timestamps to the same second, >= silently passes even though the behavioral guarantee has failed.

---

Minor style nits

Redundant sort after ordered query (registry_tests.rs:592-593)

let mut rows: Vec<FileRecord> = file_records::table
    ...
    .order(file_records::id.asc())
    .load(conn)
    .unwrap();
rows.sort_by_key(|r| r.id); // ← redundant; Diesel already returns these in id order

The .sort_by_key is a no-op here and may mislead future readers into thinking the query doesn't guarantee ordering.

fs_tx.clone().send(...) in indexer_run_tests.rs:65-69

fs_tx
    .clone()      // ← clone just to get a mutable owner
    .send(Ok(Vec::new()))
    .await
    .expect("push synthetic debounce event");

Declare fs_tx as mut to avoid the throwaway clone:

let (mut fs_tx, fs_rx) = mpsc::channel::<...>(8);
// ...
fs_tx.send(Ok(Vec::new())).await.expect("...");

Pointless let _ = result; in run_exits_cleanly_when_cancelled_immediately (indexer_run_tests.rs:1449-1466)

The .expect("run returns Ok") already unwraps the Ok(()). Assigning to result and then let _ = result; is a no-op:

// Current:
let result = timeout(...).await.expect("...").expect("...");
let _ = result;  // discards ()

// Simpler:
timeout(...).await.expect("...").expect("...");

---

Missing edge cases (worth adding to a later plan)

1. check_index_once on a completely empty directory from scratch should return false (no changes). Currently the no-op test populates a file first, then checks idempotency. The zero-file case is distinct and easy to add.

2. updated_locally with a deleted-but-unsynced file — if a file is created and deleted before its first sync (both rows have jid = NULL, tombstone has deleted = true), updated_locally's max(id) will return the tombstone row. It has jid IS NULL so it would appear in the results. Is that intentional? There's no test pinning this behavior.

3. latest_jid when a row exists with jid = Some(0) — registry.rs returns Ok(r.jid.unwrap_or(0)), so a row with jid=0 and one with no jid at all produce the same Ok(0) return. The current test only covers the NotFound case; Ok(0) is tested implicitly but not the Some(0) path specifically.

---

Planning document artifact

docs/superpowers/plans/2026-04-15-test-coverage-plan-2-registry-indexer.md is 904 lines and contains the full test code twice (once as plan steps, once inline). For readers trying to understand the tests, the plan doc adds noise without adding information beyond what the test files themselves contain. Consider whether this belongs in the repo or lives better in a linked issue/PR description. (The PR description already captures the key behavioral pins concisely.)

---

Summary

Area	Verdict
Test correctness	✅ All 18 tests appear sound
Namespace isolation coverage	✅ Excellent — every query test exercises it
Append-only delete pin	✅ Critical invariant properly guarded
latest_jid NotFound pin	✅ Surfaced and documented clearly
Timing-sensitive tests	⚠️ >= assertions should be strict >
Style nits	⚠️ Redundant sort, unnecessary clone
Missing edges	ℹ️ Suggested above for a follow-up plan

The core coverage is solid. I'd suggest fixing the >= → > assertion before merging to avoid a latent flakiness risk under CI load.

[dubadub]

Thanks for the review. Addressed in 9857bfa:

Fixed:

1. ✅ >= → strict > on modified_at — the 1100ms sleep exists specifically so the whole-second mtime readings are distinct; >= would silently mask a regression where truncate_to_seconds collapsed them. Now strict with a message explaining the invariant.
2. ✅ fs_tx.clone().send(...) → mut fs_tx — dropped the throwaway clone by marking fs_tx mutable at the let-binding. Also added a one-line comment explaining that the empty payload is intentional (the loop re-scans regardless of event contents).

Already resolved before review:
3. ✔️ non_deleted assertion order divergence from the plan doc — fixed in 0bdf98d (delivered code is correct; only the plan-doc example was stale).
4. ✔️ Redundant rows.sort_by_key after an ordered query — fixed in the same commit 0bdf98d.
5. ✔️ Pointless let _ = result; in run_exits_cleanly_when_cancelled_immediately — dropped by the implementer during initial execution.

Deferred to later plans (explicitly noted in review as "worth adding to a later plan"):
6. 📋 check_index_once on an empty dir returns false — Plan 3 will add this alongside syncer-side empty-dir handling.
7. 📋 updated_locally surfacing an unsynced tombstone — behavior is currently unspecified; will pin in Plan 3 alongside the syncer's upload-then-mark flow where the question actually matters.
8. 📋 latest_jid with Some(0) — the unwrap_or(0) collapse is a real gotcha; will cover in Plan 3 if the syncer actually exercises jid = 0 anywhere.

Plus the plan-docs-in-tree policy question — same answer as on PR #15.

Force-pushed after rebasing onto the updated Plan 1 branch so PR #15's fixes come along.

[claude]

Code Review — Plan 2: registry + indexer integration coverage

Overall this is a solid test-coverage PR. The tests use real SQLite (no mocks), exercise the public API surface faithfully, and the namespace-isolation checks in every query test are especially good. A few things worth discussing before merge.

---

Correctness issues

1. Plan doc has a wrong assertion (doc inconsistency, not a code bug)

docs/superpowers/plans/…/test-coverage-plan-2-registry-indexer.md Task 4 shows:

// Plan doc says:
assert_eq!(paths, vec![("a.cook", 11), ("b.cook", 20)]);

The actual test correctly asserts vec![("b.cook", 20), ("a.cook", 11)], which matches non_deleted's ORDER BY id ASC (b.cook id=2 < a.cook-modified id=3). The code is right; the plan doc is wrong. If the plan is intended as a durable reference, fix it to avoid confusing future readers.

The plan also has a weaker assertion in Task 7 Step 4:

// Plan doc uses a weak disjunction:
assert!(rows[0].size != rows[1].size || rows[0].modified_at != rows[1].modified_at);

The real test improves on this with two separate assert_ne! calls — the stronger version is better and should be reflected in the doc.

---

2. Untested edge case: deleted-but-unsynced files in updated_locally

Looking at registry.rs:55–74, updated_locally queries for rows where jid IS NULL — it does not filter by deleted = false. A tombstone row (deleted=true, jid=None) would be returned, which is presumably intentional (the deletion needs to be synced). But no test covers this path, so the intent is unclear. Consider adding a test like:

// File is deleted locally and not yet synced — should it appear in updated_locally?
let tombstone = registry::delete(conn, &vec![sample_delete(&live)]).unwrap();
let pending = registry::updated_locally(conn, 1).unwrap();
// Assert whichever is the intended behavior

This is also a good guard against a future refactor accidentally dropping the delete-sync path.

---

Code quality nits

3. Redundant .sort_by_key after ordered query (registry_tests.rs, latest_jid test)

let rows: Vec<FileRecord> = file_records::table
    // ...
    .order(file_records::id.asc())  // already sorted
    .load(conn)
    .unwrap();
rows.sort_by_key(|r| r.id);  // redundant

No harm, but it signals doubt about the query's ordering guarantee. Either trust the ORDER BY and drop the sort, or add a comment explaining why it's defensive.

4. &vec![...] temporary borrows throughout registry tests

registry::create(conn, &vec![sample_create("a.cook", 42, 1)]).unwrap();

The production signature is &Vec<CreateForm> (a known lint target: clippy::ptr_arg). For the test call sites, &[sample_create(...)] would be slightly cleaner, but since changing the production API is out of scope, a #[allow(clippy::ptr_arg)] on the production function or a note in a follow-up is fine.

---

Plan document question

The docs/superpowers/plans/ file is 904 lines of AI-agent planning instructions (it even contains > **For agentic workers:** REQUIRED SUB-SKILL: …). Is this file intended as a permanent part of the repo history, or was it meant to be ephemeral scaffolding? If the goal is to document why these behavior pins exist, the existing code comments and PR description do that more concisely. Committing agent planning artifacts as source can be confusing for human contributors who might treat them as authoritative specs.

---

What's working well

• Using a real SQLite tempdir pool (no mocking) means these tests catch schema/query regressions that unit tests with mocks would miss.
• Every query test includes a cross-namespace row that would break the assertion if namespace scoping leaked — this is exactly the right level of paranoia.
• The delete_appends_tombstone_row_rather_than_updating test explicitly counts rows, which pins the append-only semantics that non_deleted's max(id) logic depends on.
• check_index_once_appends_a_new_row_when_file_is_modified asserts both size and modified_at change independently, catching regressions where only one diff path survives.
• #[cfg(unix)] guard on the symlink test is correct.
• run_exits_cleanly_when_cancelled_immediately is a cheap but valuable smoke test for the cancellation path.

---

Summary: The two substantive points are (1) the untested delete-sync path in updated_locally and (2) the plan doc inconsistencies. Everything else is minor style. Happy to approve once the updated_locally + deleted-tombstone behavior is either tested or explicitly noted as out-of-scope for this plan.