code-review-evidence

Files (11)

• SKILL.md
• ai-badger-hook-feature-review.md · references/
• dotnet-pr-review-lane.md · references/
• dotnet-sdk-exception-probes.md · references/
• env-gated-suite-audit.md · references/
• hermes-memory-provider-review.md · references/
• hermes-state-db-data-path-review.md · references/
• sqlite-memory-semantics.md · references/
• test-harness-qa.md · references/
• vacuous-negative-guard-tests.md · references/
• wrapped-code-evidence.md · references/

SKILL.md (first lines)

# Code Review Evidence

Companion to the mechanical checklist (`code-review-checklist`) and the
risk-ranking skill (`review-changes`). This one is about **whether claims and
tests are evidence at all**. Two failure modes recur:

1. Behavior claims about wrapped third-party code (native SQLite extensions,
   SDKs, CLIs) are taken from the wrapper's comments or the feature spec — both
   of which can be wrong. The wrapper may faithfully translate a spec that
   contradicts the library's real behavior.
2. Integration tests assert values the method under test constructed itself,
   so they pass while the feature is entirely non-functional against the real
   backend.

## Steps


> Step 1 (verify wrapped-code semantics from the upstream source, not the spec): read `references/wrapped-code-evidence.md` when reviewing code that wraps external libs, SDKs, or stores.

 3. **Check integration tests assert observable state, not constructed values.**
   For every integration test over a real backend, ask: *could this test pass
   while the feature does nothing?* If the assertion compares a field of the
   record the method returned (e.g. `result.Context == "shared"` where the
   method built that record), it verifies a constant — re-read the row from the
   store, or run the operation that consumes it (a search, a list). A green
   tautology is the same failure as a missing test.

4. **Read test headers for honest coverage claims.** Tests that explicitly
   skip when the real backend is unavailable (e.g. catch-and-return on native
   extension load) are honest; note what they *don't* cover and cross-check the
   spec's testing table against the actual test list — sections the spec
   promised (search round-trip, deferred embeddings) may exist only as "manual
   test" notes.

5. **Trace acceptance criteria to code, then to evidence.** For each AC:
   feature scenario -> implementation path -> test that would fail if the path
   were reverted. Name the missing test case explicitly when it doesn't exist.

6. **Harness-level QA (a whole suite, not one wrapper).** When the deliverable
   is "would this harness catch a regression in X?", read EVERY test file AND
   the production code it exercises (full pairing, not sampling — the gap is
   usually in the file you'd skip), then:
   - Classify each integration assertion: **tautology** (asserts a field of the
     record the method constructed itself), **weakly-observable** (real
     read-back but insufficient — e.g. content-derived hash equality that a
     duplicate-row regression survives), or **observable** (fresh re-query via a
     different SQL path or a consuming operation).
   - Audit the fakes: do they return data for rows that would not exist in the
     real backend (phantom meta entries, hardcoded ratings for every hash)? Can
     they model the semantics the guarantee depends on (global dedup, global
     delete)? A fake that cannot model the semantics reduces a guarantee test
     (e.g. "shared is sweep-exempt") to a loop-shape check ("shared was never
     queried") that the real backend can still violate.
   - Audit skip honesty: `return;` on an unavailable backend reports PASSED,
     not SKIPPED; a broad catch (any SqliteException around the probe) conflates
     "backend not provisioned" with "backend broken" — the suite is green with
     zero coverage either way. Gate the skip on the missing precondition
     (extension files absent), use Assert.Skip, and let real failures propagate.
   - Flag snapshot/change-detector tests (production SQL/string constants
     asserted against typed-in literal copies): they fail only on a text edit,
     never on executability — and they often omit exactly the statements that
     are broken. List which constants have NO snapshot at all.
   - End with the exact list of new tests the fix batch must add, each named
     `name — asserts — why it catches the bug`; a verdict stating whether the
     harness would catch a regression in each claimed capability is the
     deliverable's spine.


> Step 7 (env-gated and hook/plugin suites): read `references/env-gated-suite-audit.md` when auditing env-gated suites, hook features, or precedent-parity claims.

## Gotchas
- **Dapper scalar queries: affinity decides, not the alias.** For
  `QueryAsync<string>` on a single-column `SELECT x AS Alias`, the alias is
  irrelevant (scalar path — no record-ctor matching); correctness hinges on the
  column's declared type → affinity in CREATE TABLE: TEXT → string works,
  INTEGER → long → `(string)long` cast throws at runtime, BLOB → byte[] → throws.
  Read the schema before verdicting, and confirm a port/contract test runs the
  query against the real database. (The record-ctor alias-matching path is a
  different trap — blob-affinity columns defeated it in this repo's history.)
- **Per-item `catch (Exception)` swallowing OCE in hosted-service loops.** Outer
  loop may have the `catch (OCE) when (stopping.IsCancellationRequested)` idiom
  while the per-item catch inside the loop swallows OCE: shutdown then logs N
  spurious warnings and pays a DB round-trip per remaining item. Also check that
  EVERY awaited read sits inside the try — a settings/interval re-read placed
  after the catch faults `ExecuteAsync`, and the .NET 8+ default
  `BackgroundServiceExceptionBehavior.StopHost` kills the whole host, contradicting
  any "best-effort" doc claim.
- **Config knobs: prove a writer exists.** `git grep <key> <branch> -- src/ tests/ docs/`
  — if the matches are only the const + reads + test fixtures, the knob is dead
  (displayed by `list`, never settable). A verb that advertises an unsettable
  value is a SHOULD-FIX, not a nit.
- **Don't trust `read_file` output that shows `***` in a string literal.** The
  tool masks secret-looking substrings for display; the file on disk may be
  fine (`$"apikey:{apiKey}"` rendered as `apikey:***`). Verify with `sed -n
  'N,Mp' file | od -c` before reporting the file as corrupted or containing a
  literal mask. Note: terminal `grep`/`cat -v` output is masked the same way —
  only `od -c` shows the raw bytes.
- **Global dedup vs per-context rows.** A "promotion" or "share" operation that
  re-inserts the same content into another context is a silent no-op if the
  library dedups by content hash globally. Read the dedup predicate
  (`SELECT ... WHERE hash = ? LIMIT 1` without a context filter = global).
- **Per-connection defaults that persist.** Setting an option on every
  connection open can clobber a persisted setting written by a configure
  operation on an earlier connection — the "configure then it works" path
  breaks only on the *next* connection.
- **NULL binds to strict-argument functions.** A wrapper passing `DBNull` for
  an optional limit/parameter can hit an upstream "expects a positive INTEGER"
  error path that no unit test exercises (fakes never validate).
- **Dead provisioning/wiring.** A downloader/installer class with tests but no
  production caller (and an empty checksum manifest) means "first run" fails;
  grep for the caller before assuming the README's "provisioned on first run"
  claim. Test setup that copies pre-provisioned files into a temp root masks
  the gap — check what the tests set up manually that production never does.
- **Skip that reports PASSED.** Integration tests that `return;` when the real
  backend is unavailable show up as green passes, not skips; a broad exception
  catch makes a broken-but-loadable backend indistinguishable from an absent
  one. Gate the skip on the missing precondition, not on the exception.
- **Fakes that fabricate rows.** A fake returning a rating/entry for hashes
  that would not exist in the real store lets a wrong "missing ⇒ default"

From Arasz/ai-badger, MIT.

More from Arasz/ai-badger