From 3493b01e9a23e3513935acf0e4427d071c9226d2 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Mon, 24 Aug 2026 14:46:38 -0600 Subject: [PATCH] feat: add a transactional-integrity review skill to the code review gate Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- assets/agents/code-reviewer/config.yaml | 5 +- assets/agents/file-reviewer/config.yaml | 7 +- .../skills/transactional-integrity/SKILL.md | 87 +++++++++++++++++++ 3 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 assets/skills/transactional-integrity/SKILL.md diff --git a/assets/agents/code-reviewer/config.yaml b/assets/agents/code-reviewer/config.yaml index 71e0aa2..ee76d90 100644 --- a/assets/agents/code-reviewer/config.yaml +++ b/assets/agents/code-reviewer/config.yaml @@ -1,6 +1,6 @@ name: code-reviewer description: CodeRabbit-style code reviewer - spawns per-file reviewers, synthesizes findings -version: 2.1.0 +version: 2.2.0 auto_continue: true max_auto_continues: 20 @@ -75,7 +75,8 @@ instructions: | ## MUST DO - Load `code-review` and `ai-slop-remover` skills before reading any code - - Apply both skill checklists to the diff + - Load `transactional-integrity` as well if this file's diff touches state-changing code (DB writes, transactions, queue/webhook/job handlers, retries, external side effects) + - Apply all loaded skill checklists to the diff - Use targeted fs_read with offset/limit; max 5 file reads - End with REVIEW_COMPLETE diff --git a/assets/agents/file-reviewer/config.yaml b/assets/agents/file-reviewer/config.yaml index ce1f59e..21fbcaa 100644 --- a/assets/agents/file-reviewer/config.yaml +++ b/assets/agents/file-reviewer/config.yaml @@ -1,11 +1,12 @@ name: file-reviewer description: Reviews a single file's diff for bugs, style issues, and cross-cutting concerns -version: 2.0.0 +version: 2.1.0 skills_enabled: true enabled_skills: - code-review - ai-slop-remover + - transactional-integrity variables: - name: project_dir @@ -29,7 +30,9 @@ instructions: | Before reading any code, call `skill__load` for `code-review` and `ai-slop-remover`. They carry your detailed review methodology — the categories to check (correctness, tests, clarity, coupling, footguns), the investigation workflow (how to use the fs tools to build context before reviewing), the slop checklist (useless comments, dishonest naming, defensive handling of impossible cases), and the standard for when to flag vs. skip. - Apply BOTH checklists in every review. Skill bodies are your source of truth for what to flag; this agent's instructions handle workflow and output shape. + Additionally load `transactional-integrity` when the diff touches state-changing code — database writes, transaction blocks, queue/webhook/job handlers, retry logic, or calls to external state-holding systems. It carries the atomicity/race/idempotency/dual-write checklist that generic correctness review misses. Skip it for pure reads, UI, and stateless computation. + + Apply every loaded checklist in every review. Skill bodies are your source of truth for what to flag; this agent's instructions handle workflow and output shape. ## Your Mission diff --git a/assets/skills/transactional-integrity/SKILL.md b/assets/skills/transactional-integrity/SKILL.md new file mode 100644 index 0000000..40955bb --- /dev/null +++ b/assets/skills/transactional-integrity/SKILL.md @@ -0,0 +1,87 @@ +--- +description: Review state-changing code for transactional integrity - atomicity gaps, read-modify-write races, non-idempotent handlers of at-least-once inputs, dual-writes to a DB plus an external system, side effects that escape rollback or re-fire on retry, and isolation-level assumptions. Store-agnostic (SQL transactions, DynamoDB conditional writes, Redis MULTI, document stores). Load when a diff touches DB writes, transactions, queue/webhook/job handlers, or external side effects. Findings fold into the standard code-review severity taxonomy. Grants read-only filesystem access for tracing transaction boundaries. +enabled_tools: fs_read, fs_grep, fs_glob, fs_cat, fs_ls +--- +You are reviewing state-changing code. The generic correctness checklist asks "does this work?"; you ask the three questions that page people at 3am: **"what happens when this runs twice? halfway? concurrently?"** Most production data-corruption incidents are not wrong business logic — they are correct logic executed under a failure mode the author never considered: a retry, a crash between two writes, or a second copy of the process. + +## When to load this skill + +The diff touches ANY of: database writes, transaction blocks, queue/stream consumers, webhook handlers, scheduled/background jobs, retry logic, or calls to external state-holding systems (payment providers, email, other services). If the diff is pure reads, UI, or stateless computation — unload; this checklist has nothing for you. + +## The checklist + +### 1. Atomicity: do multi-step writes share a transaction? + +Find every place the diff performs two or more writes that must succeed or fail together (insert parent + child, update balance + write ledger entry, state transition + audit row). Then verify they actually share an atomic unit: + +- SQL: same transaction — and confirm it by READING the enclosing scope, not by assuming; a helper called from two places may run with and without a wrapping transaction. +- DynamoDB: `TransactWriteItems` or a single-item design, not two `PutItem` calls. +- Redis: `MULTI`/`EXEC` or a Lua script, not sequential commands. +- Document stores: single-document update or multi-document transaction, not two updates. + +A crash between unguarded writes is a FINDING: name the two writes, the window, and the resulting inconsistent state. + +### 2. Read-modify-write: what happens when two run concurrently? + +Every `read → decide → write` sequence is a lost-update race unless something serializes it: + +- `SELECT` then `UPDATE` with no `FOR UPDATE`, no optimistic version/etag check, no atomic `UPDATE ... SET x = x + 1`, no conditional write (`ConditionExpression`, compare-and-set). +- Check-then-insert uniqueness ("does username exist?" then insert) with no DB unique constraint backing it — application-level checks NEVER close the race; the constraint is the fix, the check is UX. +- In-memory caches of DB state mutated alongside the DB without invalidation ordering. + +Ask: is there exactly one writer, structurally guaranteed (singleton job, partition ownership)? If yes, note the assumption and move on — flagging single-writer code for races is noise. If concurrency is possible, the missing guard is a finding. + +### 3. Idempotency: is every at-least-once input handled at-most-once? + +Queue consumers, webhook handlers, scheduled jobs, and anything retried WILL run more than once with the same input. For each handler the diff adds or modifies: + +- Is there an idempotency key, dedupe table, `INSERT ... ON CONFLICT DO NOTHING`, or conditional state transition (`WHERE status = 'pending'`) that makes the second delivery a no-op? +- Does the handler complete its side effects BEFORE acknowledging/deleting the message? Ack-then-process loses work; process-then-ack requires idempotency. +- Partial failure: if the handler does A, B, C and crashes after B, the redelivery re-runs A and B — are they safe to re-run? + +An error path that neither succeeds nor removes the message (so it redelivers forever into a DLQ) is also a finding — poison-message handling is part of idempotency. + +### 4. Dual-write: DB + external system with no reconciliation + +The diff writes to the local DB AND to an external state holder (payment provider, email service, another service's API, a search index) in one flow. One succeeds, the other fails — now the two systems disagree: + +- Look for the outbox pattern (write intent to DB in the transaction, deliver asynchronously), a saga/compensation step, or at minimum an explicit reconciliation job. +- "Call external API inside the DB transaction" is not a fix — it holds locks across network I/O and still diverges when the commit itself fails after the call succeeded. +- Order matters: charging a card before durably recording the intent to charge means a crash produces a charged-but-unprovisioned customer; the reverse produces a recorded-but-unfulfilled intent, which is recoverable. + +Flag the divergence window and which side wins on replay. + +### 5. Side effects vs rollback and retry + +- Anything non-transactional fired INSIDE a transaction (email sent, event published, cache invalidated) happens even when the transaction rolls back. It must move after commit (or into an outbox). +- Anything fired inside a RETRIED scope (job framework with automatic retries, HTTP client with retry middleware) re-fires per attempt unless guarded. +- Metrics/logs are exempt — do not flag observability as a side-effect violation. + +### 6. Isolation assumptions + +Code that is only correct under SERIALIZABLE but runs at the store's default (READ COMMITTED in Postgres, REPEATABLE READ in MySQL) is a latent race. Watch for: aggregate checks before writes ("sum of debits ≤ balance"), multi-row invariants enforced in application code, and phantom-sensitive queries. If the diff's correctness depends on an isolation level, verify the code SETS it rather than assumes it. + +## Ground-truth discipline + +- READ the enclosing scope of every write the diff touches — transaction boundaries live up-stack from the hunk. `fs_grep` the function's callers to learn whether it already runs inside a transaction. +- `fs_grep` the handler registration/config for retry counts, DLQ wiring, and delivery semantics before claiming "this retries." +- Check sibling handlers for the idempotency pattern the codebase already uses (dedupe table, conditional transition) — a new handler skipping the established guard is the strongest form of evidence. +- Do not flag theoretical races in structurally single-writer code; note the single-writer assumption instead so the next reviewer sees it was considered. + +## Finding format and severity + +Fold findings into the standard review severities — no separate verdict: + +- 🔴 CRITICAL — money/data loss or corruption under a realistic failure (dual-write with no reconciliation on a paid flow; lost-update race on a balance; non-idempotent charge handler). +- 🟡 WARNING — inconsistency window or unbounded redelivery with operational (not monetary) blast radius; missing constraint behind a check-then-act. +- 🟢 SUGGESTION — hardening: add the missing unique constraint even though the race is improbable, move the email after commit. + +Every finding names: the writes involved (file:line), the failure mode that triggers it (crash between X and Y / concurrent execution / redelivery), and the concrete fix. "This might have race conditions" is noise; "second `invoice.paid` delivery re-runs the provisioning insert at handler.go:88 because there is no dedupe on event_id — add the unique index the payment handler at handler.go:41 already uses" is signal. + +## Anti-patterns + +- Flagging every read-then-write as a race without checking who else writes. +- Demanding SERIALIZABLE everywhere — the finding is an UNSTATED isolation assumption, not a low isolation level. +- Treating logs/metrics as dual-writes. +- Reviewing the hunk without reading the enclosing transaction scope — most false positives and false negatives in this domain come from not knowing whether you're already inside a transaction. +- Accepting "the framework handles it" without grepping the framework config that proves it.