test(client): Plan 1 — foundation test helpers and client unit coverage

[dubadub]

Summary

Implements Plan 1 of a multi-plan test-coverage effort for the cooklang-sync-client and cooklang-sync-server crates. See the design spec in docs/superpowers/specs/2026-04-15-library-test-coverage-design.md and Plan 1 itself in docs/superpowers/plans/2026-04-15-test-coverage-plan-1-foundation.md.

Plan 1 scope: shared test helpers plus leaf-level client module coverage. Later plans (2–6) will cover registry/indexer, remote/syncer, the server, and end-to-end tests.

What's in this PR

• client/tests/common/mod.rs — shared helpers: fresh_client_pool(), tempdir_with_files(), sample_jwt()
• client/tests/connection_tests.rs — pool creation, migrations, checkout (3 tests, 1 #[ignore] for the slow r2d2-retry error path)
• client/tests/context_tests.rs — SyncContext listener forwarding + parent/child cancellation (6 tests)
• client/tests/file_watcher_tests.rs — async_watcher file-event smoke test (1 test)
• client/tests/lib_jwt_tests.rs — pins extract_uid_from_jwt success and panic paths (6 tests)
• client/src/errors.rs — appended #[cfg(test)] block, 4 tests for SyncError conversion and Display
• client/src/chunker.rs — 4 new tests inside existing #[cfg(test)] block: hashify error, save missing-chunk error, delete(path) round-trip, multi-chunk text

Zero production code changes. One attempted production tweak was reverted during review.

Numbers

• 20 new tests added (65 passing in the client suite, up from 45)
• 1 ignored (slow error-path test; runs with cargo test -- --ignored)
• 0 new warnings in cargo build --workspace
• Full client suite runtime: ~2.5s (dominated by the 2s file-watcher debounce window)

Test plan

• cargo test -p cooklang-sync-client — all green
• cargo test --workspace — all green
• cargo build --workspace — no new warnings
• Two-stage review per task (spec compliance + code quality) plus whole-branch final review

Follow-up plans

Not in this PR — each will land separately:

• Plan 2: client registry + indexer integration tests
• Plan 3: client remote (wiremock) + syncer tests
• Plan 4: server unit tests (auth, chunk_id, metadata models/db/notification, request/response serde)
• Plan 5: server route tests (metadata, chunks, middleware, create_server smoke)
• Plan 6: end-to-end client ↔ server tests

[claude]

Code Review — Plan 1: Foundation Test Helpers + Client Unit Coverage

Overall this is a well-structured, zero-production-change test PR. The test names are descriptive, precondition asserts are used consistently, and the real-filesystem/real-SQLite approach is the right call over mocking. A few things worth addressing before merge:

---

Issues

1. Weak error assertions in two chunker tests

hashify_errors_on_missing_file and save_errors_when_referenced_chunk_is_missing both end with:

let _ = format!("{err:?}");

This doesn't assert anything — it only exercises the Debug impl (which #[derive(Debug)] guarantees anyway). The comment says "specific variant is implementation-dependent," but at a minimum the error kind is knowable and stable:

• hashify on a missing .cook file will always produce SyncError::IoError { .. } via from_io_error.
• save with a missing chunk will always produce SyncError::GetFromCacheError.

Suggested fix:

// hashify test
assert!(matches!(err, SyncError::IoError { .. } | SyncError::IoErrorGeneric(_)));

// save test
assert!(matches!(err, SyncError::GetFromCacheError));

If pinning is intentionally avoided to allow future refactoring flexibility, at least add assert!(format!("{err}").len() > 0) or drop the line entirely — the current form implies an assertion that isn't there.

---

2. delete_removes_file_from_storage_dir diverges from spec intent — cache eviction is untested

The plan spec (Task 7) called for delete_removes_chunk_from_disk_and_cache which would exercise save_chunk → delete_chunk → read_chunk (cache eviction). That test was swapped for one that writes a raw file directly to temp.path() and calls Chunker::delete.

Looking at the source, Chunker::delete is:

pub async fn delete(&mut self, path: &str) -> Result<()> {
    fs::remove_file(full_path).await.map_err(...)
}

It only removes the file from disk — it does not evict any chunk entries from the in-memory cache. This means if a file's chunks are cached and then delete is called, read_chunk would still return those chunks. The landed test only checks disk deletion (which is a thin wrapper over fs::remove_file) and misses the cache layer entirely.

The PR notes that "one attempted production tweak was reverted during review." If delete_chunk doesn't exist yet, consider either:

• Adding a test that verifies save_chunk / read_chunk round-trip (separate from file deletion), or
• Adding a TODO comment in Chunker::delete noting the cache non-eviction is intentional.

---

3. on_complete path has infrastructure but no test

RecordingListener in context_tests.rs tracks completions and exposes a completions() accessor, but no test ever calls ctx.notify_complete(...) / listener.on_complete(...). Either add a test exercising on_complete forwarding (parallel to notify_status_forwards_to_listener), or remove the dead field/method to avoid misleading future readers.

---

4. #[ignore] rationale on Linux

The comment on get_connection_pool_returns_error_for_unwritable_path says r2d2 retries for ~30s. On Linux, /dev/null/does_not_exist/db.sqlite3 fails at open() before r2d2 even attempts a retry (the parent path is a character device). The #[ignore] is still correct and conservative, but it's worth noting that the actual timeout occurs only when r2d2 can connect to SQLite but the connection is flaky — not when the path is obviously invalid. If CI is Linux-only, this test would pass quickly and the #[ignore] could be removed for that path. Low priority; the current state is safe.

---

Nits / Style

• docs/superpowers/plans/ and docs/superpowers/specs/ add 1025 lines to the repo that duplicate the code now in tests/. These agentic planning artifacts are potentially valuable institutional knowledge, but they create a maintenance burden (docs drift from code). Consider a policy decision for future plans: should these live in the repo permanently, or in a dedicated docs/ folder that's .gitignored after merge?

• hashify_text_file_with_multiple_lines_produces_multiple_chunks asserts exactly 3 chunks for 3 \n-terminated lines. This is a correct pinning test for the read_until(b'\n') chunking strategy. The failure message "got {hashes:?}" is helpful — keep it.

• file_watcher_tests.rs: the canonicalize() + path.starts_with(&e.path) fallback for macOS FSEvents is a real improvement over the original spec. Good defensive coding.

• common/mod.rs: #![allow(dead_code)] at the crate level is necessary given not all helpers are used in all test files. Fine as-is, but document which future plan will use tempdir_with_files (Plan 2/3?) so reviewers know it's not abandoned.

---

Summary

Area	Status
Test structure and naming	✅ Good
Real DB/FS (no mocks)	✅ Correct approach
#[should_panic] for panic-path JWT tests	✅ Correct
#[ignore] for slow r2d2 error path	✅ Well-documented
Error variant assertions	⚠️ Missing (see issue 1)
Cache eviction coverage	⚠️ Not addressed (see issue 2)
on_complete listener test	⚠️ Infrastructure dead code (see issue 3)
Plan docs in source tree	💬 Policy question

Happy to discuss any of these — issues 1 and 2 are the ones most worth fixing before merge.

[dubadub]

Thanks for the review. Addressed in 1461fa7:

Fixed:

1. ✅ Weak error assertions — replaced let _ = format!("{err:?}") with matches!(err, SyncError::IoError { .. }) on the missing-file test and matches!(err, SyncError::GetFromCacheError) on the unknown-chunk test.
2. ✅ Cache-path coverage gap — added hashify_then_read_chunk_round_trip_via_cache exercising save_chunk → read_chunk → check_chunk. The existing delete_removes_file_from_storage_dir stays as a disk-only pin (intentional — Chunker::delete does not evict the cache, which is the code's current behavior).
3. ✅ on_complete dead code — added on_complete_forwards_success_and_failure_with_message which calls on_complete(..) via ctx.listener() on the recording listener, mirroring how lib.rs invokes it.

Not fixing in this PR:
4. 💬 #[ignore] on get_connection_pool_returns_error_for_unwritable_path — keeping as-is; the cross-platform story (macOS immediate fail vs Linux r2d2 retry) makes a conditional #[ignore] more brittle than a conservative always-ignored test.
5. 💬 Plan docs in-tree — policy decision, deferring until we have a few more plans to see how drift actually behaves in practice.

PR #16 (stacked on this branch) picks up these fixes automatically after rebase.

[claude]

Code Review — Plan 1: Foundation Test Helpers and Client Unit Coverage

Overall this is a well-structured PR that adds meaningful test coverage without touching production code. The test organization is clean, helpers are thoughtfully designed, and the PR description is detailed. Below are observations ranging from blocking concerns to minor suggestions.

---

Security

extract_uid_from_jwt skips signature verification (worth flagging)

common/mod.rs builds a JWT with a fake signature, and the tests explicitly document that extract_uid_from_jwt does not verify signatures:

// `extract_uid_from_jwt` does NOT verify the signature, so any placeholder works.
let signature = base64_url_nopad(b"test-signature-unverified");

This is fine for the test helper, but the function being tested is used in production to identify users. An attacker who can craft a JWT payload with any uid would bypass authentication entirely. This PR correctly pins the behavior, but a follow-up issue/PR should change extract_uid_from_jwt to actually verify the signature, and these tests should be updated to use a correctly-signed token with the known test secret.

---

Code Quality

#[should_panic] without expected = "..." is fragile

Three tests in lib_jwt_tests.rs use bare #[should_panic]:

#[test]
#[should_panic]
fn extract_uid_panics_on_token_with_missing_segments() { … }

Any panic — including a test infrastructure panic — will make these pass. Prefer #[should_panic(expected = "…")] with a substring of the actual panic message. That way refactors that change the error path (e.g., converting to Result) will fail the test rather than silently passing.

Redundant test in chunker.rs

hashify_text_file_with_multiple_lines_produces_multiple_chunks and hashify_then_read_chunk_round_trip_via_cache both create a multi-line .cook file and call hashify. The round-trip test is strictly more comprehensive (it also exercises check_chunk and read_chunk). The chunk-count test adds little incremental value; consider removing it or merging the count assertion into the round-trip test.

delete_removes_file_from_storage_dir doesn't verify cache eviction

The plan spec says "delete removes chunk from disk and evicts from cache." The actual test only checks the on-disk file is gone — it doesn't assert that check_chunk/read_chunk subsequently fail for any hash that pointed at that file. That means cache-eviction behavior is untested. Either update the test name to match what it actually covers, or add the cache check.

child_tokens_are_independent_of_each_other tests library behavior

fn child_tokens_are_independent_of_each_other() {
    let ctx = SyncContext::new();
    let a = ctx.token();
    let b = ctx.token();
    a.cancel();
    assert!(!b.is_cancelled(), "sibling token should not be affected");
}

Unless SyncContext has custom cancellation logic, this test is asserting that tokio_util::CancellationToken child tokens are independent — which is tokio's responsibility to guarantee, not yours. If SyncContext::token() returns child tokens of a parent token held by the context, this test also doesn't cover what happens when ctx.cancel() is called (which would cancel all children). The test for cancel_propagates_to_child_token covers that direction. Consider whether this test is load-bearing.

---

Potential Bugs / Platform Issues

get_connection_pool_returns_error_for_unwritable_path message assertion is fragile

assert!(
    msg.to_lowercase().contains("connection"),
    "expected ConnectionInitError message, got: {msg}"
);

This relies on SyncError::ConnectionInitError's Display containing the substring "connection". That's true today, but an easy refactor of the error message would silently break this. Asserting on matches!(err, SyncError::ConnectionInitError(_)) (if the error is accessible as a typed value rather than a formatted string) would be more robust.

async_watcher_reports_file_creation has a real 2–6 second wall-clock window

The test is not marked #[ignore] but takes 2–6 seconds every run. This will slow down the test suite noticeably as it grows. Consider either:

• Marking it #[ignore = "slow: 2s debounce window"] and running it in a separate CI step, consistent with how the ignored r2d2 test is handled, or
• Exposing the debounce duration as a parameter (even test-only) to reduce the wait in tests.

---

Minor / Style

docs/superpowers/ documents in the public tree

The two new files (specs/…, plans/…) contain internal AI agent workflow instructions ("For agentic workers: REQUIRED SUB-SKILL…"). Shipping these in the repository is unusual — they're not design docs that future contributors would reference, and they could confuse first-time contributors. Consider a .gitignored directory or a private internal wiki instead.

#![allow(dead_code)] in common/mod.rs is broad

It suppresses warnings for all dead code in the module. If wiremock_remote is added in a later plan and temporarily unused, it silently passes. A narrower #[allow(dead_code)] on specific items would make it easier to notice genuinely unused helpers.

---

What's working well

• Helper lifetime discipline (TempDir paired with return value so callers can't accidentally drop it early) is correct and the comment explains why.
• The macOS symlink canonicalization in file_watcher_tests.rs and the path-containment check (path.starts_with(&e.path)) are thoughtful portability fixes.
• The #[ignore] annotation with a clear explanation for the slow r2d2 test is good practice.
• RecordingListener in context_tests.rs is a clean, minimal test double.
• The round-trip test in chunker.rs is thorough — it verifies that hashify populates the cache and that read_chunk reconstructs the original bytes.