Adapter Review

Adapter Review

Review along two axes — Standards (repo docs, contributing rules, ADRs) and Spec (issue / PR body / upstream behavior) — plus the traps below. Report the axes separately; anything both hit independently is top priority.

Principles

• No blocker without a repro. Decisive evidence at a pinned commit (minimal fixture driving the real provider code, e.g. sessions=[] or a wrong count). Re-measure PR-reported test counts, benchmarks, and CI status yourself; attribute red tests to environment (missing deps, unbuilt packages) before charging them to the PR.
• Upstream ground truth over assumptions. Verify every format assumption — file layout, field case, version constants, ID generation, env vars, retention — against the host's real source and real local data. "Not on my machine" ≠ "doesn't exist" (the upstream repo may have moved); "zero rows locally" never justifies skipping a source structure (a later upstream migration silently invalidates present-state filtering).
• Test claims ≠ what tests prove. Open fixtures behind every "covered" claim: their ordering and fields must actually reach the bug (not route around it), and leak no machine paths. Confirm new test files actually run in CI where the workflow lists files explicitly. Hunt mocks over production paths, circular verification (verifying discover() with discover()), and test comments whose premise primary docs refute. Revert the changed file to confirm new tests fail for the right reason.
• Attribute before grading. git show <base>:<file> each finding: introduced by this PR → blocking; pre-existing but amplified → judge the amplification; purely pre-existing → follow-up issue, don't block. Withdraw findings whose premises fail upstream/data validation, and say why.
• Fixes are a new attack surface. Each round: re-reproduce prior findings on the new head, then review the fix code itself — especially new destructive paths. git range-diff before merge so what lands is byte-identical to what you reviewed.

Where adapter PRs actually fail

1. Identity/key asymmetry — cursor key, session id, unit key derived differently across discover / parse / persist / delete. Test fakes are often symmetric exactly where the real adapter is not, so green suites hide it. Family-parameterized adapters must keep persisted identity byte-identical for existing sources.
2. Destructive tombstone/retract misfires — walk four scenarios: whole-directory delete, cross-workspace move, concurrent change during enumeration, source unavailable (distinguish ENOENT from EACCES). Deletes require complete inventory; changed-paths is a routing hint, never deletion evidence. Tombstones must leave a non-null cursor or deletes replay forever.
3. Migration never converges — check fresh DB × old-format DB × already-migrated DB against the upgrade trigger. Wire-format changes: strict version rejection + self-healing fallback, timed to ride an existing full-reindex window. Fields newly written to existing canonical rows: bump the version marker, add a backfill test simulating an old DB (old marker as a literal, not the constant), and confirm persist-layer null-merge can't drop stored values.
4. Undeclared behavior change — split the diff into new capability vs semantic change to existing paths; every line of the latter needs a declaration in PR body or commit message.
5. Fast-path correctness debt — when the correctness invariant spans a wider scope (tree, cursor window) than the optimization assumes (single file). If successive review rounds share one root cause, stop patching: escalate to fast-path/correctness-fallback with an equivalence property test (fast ≡ full), and quantify the win before accepting the state machine. Keep adversarial constructions inside the host's real threat model.
6. Swallowed errors & state edges — existsSync folding EACCES into ENOENT, try/finally without catch, partial state committed on failure; SQL three-valued logic (NULL != 'x' filters rows); independent flags combined inconsistently.
7. Failure-mode sweep — legal intermediate states (header-only file, torn frame), crash residue (WAL without shm), Windows semantics (ctime/inode/chmod), symlink/realpath aliases.
8. New adapters: census the data first — real-data distribution (row counts, type mix, anomalous session classes) surfaces internal-mechanism pollution at design time, not after launch.
9. Dead contract promises — for every promise in type comments / ADRs ("setting this retracts atomically"), grep all consumers and verify it is kept; a promise with no real consumer is a blocking finding.
10. Sibling drift — a bug class fixed in one adapter is a sweep of all siblings with the same frame; a shared-helper change must not silently move sibling adapters (fail-open/fail-closed predicates stay adapter-local unless deliberately shared).
11. Process hygiene — verdict every PR/issue claim one by one (Correct/Partial/Incorrect); mixed-scope PRs (unrelated work riding along), PR body drifted from the final diff, and wrong issue linkage are findings, not chores.