Correctness Review

Imported from benord-labs/frink-primitives (.claude/skills/correctness/SKILL.md). Install upstream with npx skills add benord-labs/frink-primitives --skill correctness. Copyright stays with the author.

Correctness Review

Review Script

CORRECTNESS_SKILL=""
for candidate in \
  .agents/skills/correctness \
  .claude/skills/correctness \
  .cursor/skills/correctness
do
  if [ -f "$candidate/scripts/checklist.mjs" ]; then
    CORRECTNESS_SKILL="$candidate"
    break
  fi
done
if [ -z "$CORRECTNESS_SKILL" ]; then
  echo "Correctness Review checklist unavailable: run devkit sync-skills" >&2
  exit 2
fi
SCRIPT="$CORRECTNESS_SKILL/scripts/checklist.mjs"

node $SCRIPT generate     # Enumerate review items from staged source files (all declared roots)
node $SCRIPT status       # Show progress
node $SCRIPT check-item <name> --pass   # Mark item passed
node $SCRIPT check-item <name> --fail "reason"  # Mark item failed
node $SCRIPT finalize     # Verify every item was resolved; refuses if any are pending or failed
node $SCRIPT cleanup      # Remove checklist

The roots the script scans are the UNION of scanRoots, review.backendRoots and review.frontendRoots from guard.config.json — correctness is not domain-sliceable, so a backend writer and its frontend reader are reviewed together. Source files only.

Exactly four items (state-transitions, concurrency-races, writer-reader-contracts, error-and-edge-classification) are ALWAYS enumerated when any source file is staged — a correctness bug has no reliable lexical signature, so they never regex-gate to zero, and never more than four: each lens is a pass over the same diff, so item count multiplies judge wall-clock. Broadcast/dedup rides the contracts lens; retries and discarded returns ride the state lens.

State & Data Integrity

• Every status/state write needs a compare-and-set or expected-state guard (expectStatuses, WHERE status IN (…)) — an unconditional update lets a concurrent path clobber a real result.
• Trace each written state to EVERY consumer: pollers, filters, queries. A writer that sets a state a reader filters out creates a permanently stuck row.

// BAD: unconditional — a concurrent advance to 'running' is overwritten back
await db.update(nodeRuns).set({ status: 'running', nodeOutput: null });

// GOOD: CAS — a concurrent advance matches 0 rows instead of clobbering
await unparkNodeRunInPlace(id, { expectStatuses: ['cancelled'] });

// BAD: retry flips the row to a state the poller filters out — stuck forever
await tasks.update({ status: 'pending' }); // poller: WHERE startMode != 'wait'

Temporal & Concurrency

• For every read-then-write, walk the interleaving where another actor (second process, second window, timer, boot sweep) runs BETWEEN the read and the write.
• Re-entry paths (resume, wake, reconnect) must be idempotent; timers and retries must not double-fire.

Contract & Boundary Handling

• For every emit/broadcast/send: enumerate the listeners. A broadcast consumed by N windows/processes each holding its own queue executes the effect N times — require targeting or dedup.

// BAD: every window enqueues and sends the same retry prompt
broadcastToAllWindows('task:chat-ready', { subChatId, isRetry: true });

• Changed signatures/payload shapes: grep ALL call sites; every one must still hold the contract.

Recovery & Failure Modes

• A discarded return value is a finding — the caller proceeds as if the operation succeeded:

// BAD: resumeFailedFlowInPlace returns false on every precondition miss — silently dropped
try { await resumeFailedFlowInPlace(runId); } catch { /* only exceptions handled */ }

// GOOD: the boolean is load-bearing
const resumed = await resumeFailedFlowInPlace(runId);
if (!resumed) log.warn('run not unparked — agent done will be dropped', { runId });

• Retry/resume paths must land in a state some sweep re-drives; a transient condition must not be marked permanent (near-expiry ≠ expired; a rotated token must be re-read).

Classifier & Parsing Edge Cases

• For every regex/string classifier, construct one valid input that misclassifies:

// BAD: a bare `{` anchor — the model's own JSON output classifies as an API error
const ANCHOR_RE = /^\s*(\{|Failed to authenticate|API Error:)/;

// GOOD: only the CLI's own prefixes anchor
const ANCHOR_RE = /^\s*(Failed to authenticate|API Error:)/;

• Boundary conditions: near-expiry ≠ expired, 0 ≠ absent, empty string ≠ missing.

Verdict bar

FAIL only for a reproducible correctness defect introduced by this diff, stated with its concrete failing interleaving or input. Style, performance and security belong to the other reviewers.