Code Review

Reviewing sortie pull requests

.github/copilot-instructions.md covers the mechanical checks: layering, concurrency, path containment, SQLite rules, error wrapping, resource lifecycle, adapter boundaries, style. Do not repeat them. This skill covers what those checks cannot see: defects that are locally correct and globally wrong.

Two rules govern every finding.

Verify before flagging. Open the file and read the line. A finding that cites a line that says something else costs the maintainer more than silence. State the evidence inline: file, symbol, and what it actually does.

One grounded finding beats five speculative ones. Uncertain findings are noise. If a check below needs a fact you cannot establish from the diff plus the files it touches, say what you could not verify instead of guessing.

Start from the pull request description

.github/pull_request_template.md makes the author declare Intent, Sensitive Areas, Breaking Changes, and Migrations. Treat each as a claim to audit, not as context to absorb.

• Intent says opt-in or default-off. Find the branch that makes it inert when unconfigured, and confirm nothing outside it changed behavior. An opt-in feature that alters a default path is the failure this claim hides.
• Sensitive Areas names a file. Review it first and hardest. An empty Sensitive Areas on a diff that touches orchestrator state, an adapter boundary, or workspace removal is itself a finding.
• Breaking Changes says none. Check exported signatures, domain struct fields consumed by adapters, config field names and defaults, and log or metric names an operator may depend on.
• Migrations says none. Check internal/persistence/sql/ and any new column read.

Documentation is part of the change

The specification under docs/architecture/ is the contract implementation follows, so drift is a defect, not a nicety. For any behavior change, find the section that describes that behavior and confirm the PR updates it.

• A documented guarantee with no enforcing line. When a document claims a safety property ("marked dispatched synchronously, which prevents duplicate dispatch"), locate the code that provides it. If the code does it later, elsewhere, or not at all, the document is wrong and the PR that touched the area should fix it.
• Absolute claims. "No path releases X", "the only mechanism", "always", "never". One counterexample falsifies these permanently, and they rot silently. Ask whether the diff introduces a second path that makes an existing absolute false.
• Counts in prose. "has nine parts", "three tables", "two packages implement". A new pass, table, or implementation makes them false and nobody notices. Flag the count, and propose a formulation that states the ordering or the property instead.
• Enumerations that must grow. Adapter lists, reaction kind lists, supported-forge tables. If the PR adds a member, every enumeration of that set is a review target.
• Operator-visible surface, separate repository. The docs site lives outside this repository. A PR adding a config field, environment variable, CLI flag, or reaction kind cannot update it here. Flag the gap so it is tracked; do not ask for a file this repository does not hold.
• A changelog bullet in the wrong version section. A diff hunk header names the category (### Fixed), never the enclosing ## [x.y.z] heading, so a bullet inserted into an already-released section reviews as clean. Resolve each added hunk's line number to the nearest preceding ## [ heading and require [Unreleased], then check the bullet appends to the bottom of its category and wraps to its neighbours' width.
• Accepted decision records are immutable. docs/decisions/ records what was decided and why. Do not propose adding issue numbers, section numbers, or line numbers to one; do not re-litigate a decision recorded there. If the code contradicts an accepted record, that is the finding.

Orchestrator state: the defects that survive unit tests

State lives in maps keyed by issue and reaction kind, mutated across passes that run in a fixed order each tick. Locally correct edits break neighbors.

• The retry slot is one per issue. ScheduleRetry cancels any queued retry for that issue first. Any pass that schedules a retry discards whatever another kind had queued, along with its continuation context. When a diff adds or moves a ScheduleRetry call, ask what it displaces and whether the victim can re-detect its own work.
• Cleanup must be scoped to the escalating kind. An issue-wide clear takes sibling reactions' entries, counters, and fingerprint rows with it. A kind parked in a handoff state has no worker exit left to reseed it, so the loss is permanent until restart.
• Every pending entry needs a release path. For a new or modified reaction kind, establish who seeds the entry, what drops it (age, terminal state, or its own completion), and what happens across a restart. A kind with no expiry and no terminal check leaks the entry and polls the forge forever.
• Claim and counter are not the entry. state.Claimed, state.ReactionAttempts, and state.PendingReactions have independent lifetimes. A change that releases one is not evidence the others are released.
• Kind-keyed maps must gain the new member. A new reaction kind needs an entry wherever kinds are enumerated: pinning behavior, TTL wiring, recovery seeding, validation, metrics labels. Grep the kind constant across the package; a missing entry usually means a silent default, not a compile error.
• The fingerprint protocol has an order. Upsert the fingerprint, read it back, act, and mark dispatched only after the action succeeded. Marking before the action loses the retry on a transient failure; deleting the row on success re-arms the same observation on the next tick; retaining it blocks a legitimate second episode. Judge which of the three the diff wants, then check it does that.