# Bug Check

> Systematic bug hunt focused on patterns that survive code review and test audit — description-vs-implementation mismatches, SQL correctness, type coercion bugs, boundary/off-by-one errors, behavioral asymmetry, and input validation gaps. Derived from analysis of 40+ bugs found by Qodo and CodeRabbit that the ship-check pipeline (pr-review, code-quality, test-audit) missed. Use when asked to "bug check", "check for bugs", "deep correctness check", "look for subtle bugs", or as part of the ship-check pipeline. NOT for: code style (use code-quality), test design (use test-audit), security review (use security-review), or high-level correctness review (use pr-review).

- Skill: `aliasunder/bug-check` (Agent Skill)
- Install (CLI): `npx skillmds@latest add aliasunder/bug-check`
- Raw SKILL.md: https://api.skillmd.com/api/skills/aliasunder/bug-check/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: DevOps & Infra
- Author: aliasunder (https://skillmd.com/u/aliasunder)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/aliasunder/bug-check

---


# Bug Check

Systematic hunt for bugs that survive intuitive code review. This skill applies
**pattern-based checks** derived from what Qodo and CodeRabbit consistently find that
human and AI reviewers miss.

How this differs from pr-review: pr-review reads the diff holistically and reasons
about what could go wrong. Bug-check applies a **checklist of known miss patterns**
to every changed file. It is systematic where pr-review is intuitive.

## Before starting

1. Load AGENTS.md for project conventions.
2. Load code standards + preference recall — discover the standards notes first
   (the set grows; hardcoded lists go stale), read the pass-relevant results,
   then recall the dated evidence trail for the change's domain (surfaces
   preferences newer than the notes):
   - `vault_search({ query: "code standards", filters: { tags: ["code-standards"], type: "reference", properties: { lifecycle: "living" } } })`
   - `vault_read_note` the results (currently typescript and docs), plus any newer
     note matching the repo's language/stack
   - `vault_memory_recall({ query: "<change domain>" })`
3. Identify all files changed in the branch vs main:
   ```
   git diff main --name-only
   ```
4. **Scope check**: Production code (`.ts`, `.js`), infrastructure (`.yml`, `.yaml`,
   `Dockerfile`, `docker-compose*`), environment config (`.env`, `.env.*`,
   `.env.example`), and config files (`sst.config.*`, `*.json`) are in scope. CI/workflow files ARE in scope — dimension 1
   (description-vs-implementation) applies to workflow step descriptions, job names,
   and conditional logic. User-facing documentation (`.md` guides, READMEs) is in
   scope for dimension 1 when the docs make factual claims about the system —
   privacy/security guarantees, architecture, data flow, storage locations, capability
   claims, or tool categorization. Verify each claim against the implementation. Pure
   formatting/prose changes to docs are out of scope — but a docs PR that
   says "no external communication" when the system has an outbound sync service is a
   D1 bug, not a style issue. Structural changes (reordering sections, changing which
   method leads, folding content into asides) are NOT out of scope: they trigger the
   D1 structural self-reference check and the D5 alternative-command-path check.
   Only report "docs only — nothing to check" when no factual claims about the
   system are present AND no doc was restructured.
5. Skip test files — test-audit handles those.
6. **Read each changed file in full** — not just the diff. Bugs hide in how new
   code interacts with surrounding context.

## Dimensions

Run each dimension against every changed production file. Dimension 1 gets the most
time — it is the highest-yield check (40%+ of bot findings fall here).

### 1. Description-vs-implementation verification

The #1 source of missed bugs. The description says one thing; the code does another.

**For every function, tool definition, or doc comment in changed files:**

1. **Quote each sentence of the description verbatim** before checking it. This is
   mandatory proof-of-work — it surfaces truncated, garbled, or incomplete text that
   looks fine at a glance but fails on close read. A sentence like "even if a folder
   can't be." is obviously incomplete when quoted, but easy to skip over when skimming.
2. Extract every **factual claim** from the quoted sentences:
   - "Returns results sorted by X" → requires ORDER BY X
   - "Detectable via tool_Y" → tool_Y must actually do that
   - "Creates X if not found" → creation logic must exist
   - "Requires parameter Z" → Z must be validated as required
   - "Returns empty array, not an error" → verify no throw on empty
3. **Trace each claim to the implementation.** Read the actual code path.
4. **Flag any mismatch**: claim not implemented, implemented differently,
   references the wrong function/tool/concept, or is truncated/garbled text.
5. **Decide which side is wrong before fixing.** A mismatch has two possible
   fixes, and rewriting the description to match the code is the cheaper one — so
   it is the one this check drifts toward, and it hides the bug instead of fixing
   it. The description states the intent. If the intent is what the project wants,
   the code is the bug and the fix goes in the code. The code is the wrong side
   when: the description matches a user-facing promise (a setting is honored, a
   copy is preserved), the code's actual behavior is the destructive or less
   capable branch, or the mismatch comes from an implementation shortcut such as
   an env var standing in for a state the code should detect directly. Every D1
   finding names which side changed and why. Examples of the trap: a description
   says the tool honors a vault setting and the code skips the setting whenever
   one env var is present — qualifying the description leaves every deployment
   that sets the state another way behaving wrong; a doc comment says a fallback
   value is not cached and the code caches it — matching the comment to the code
   means a later config change is never seen. In both, the code was the bug.

**Cross-references are a known hot spot.** When a description mentions another tool
or function by name:
1. **Read the referenced tool/function's description and implementation** — don't
   just check the name exists. Confirm it actually does what the referencing
   description claims it does.
2. **Verify directionality** — the most common error is naming the wrong sibling
   (e.g., "use findOrphans to find broken links" when orphans are nodes with no
   *incoming* links, not nodes with broken *outgoing* links; or referencing
   `getOutgoingLinks` when `getBacklinks` is the correct direction).
3. **Verify workflows end-to-end** — when a description prescribes a multi-step
   procedure ("after doing X, use Y to find Z"), trace the full workflow. Each
   step must produce the output the next step expects. A cross-reference that is
   individually correct can still be wrong in context if the workflow logic is
   flawed.

**Conditional capabilities described unconditionally**: When a description claims a
capability (e.g., "Hybrid search combining FTS and vector similarity"), check whether
that capability is gated behind a feature flag, env var, or optional dependency
(embedder, API key, external service). If it is, the description must either qualify
the claim ("when embeddings are enabled") or describe the degraded mode ("falls back
to keyword-only search"). A description that presents a conditional capability as
always-available is a D1 bug — the LLM reading it will instruct users about features
that may not exist in their deployment. This applies to tool descriptions, README
capability lists, and API documentation. Trace the code path: if there's an early
return or `if (!dependency)` guard that skips the advertised behavior, the description
is wrong for that branch.

**Mechanism mischaracterization**: When a description uses lifecycle or state language
— "starts X then switches to Y", "caches results", "batches requests", "streams
results", "lazy-loads on first use", "queues work" — verify the code actually
implements that mechanism, not just the outcome. A per-query fallback is not a state
transition. A synchronous call is not a queue. The behavior may be correct (users get
the right result) but the mechanism claim teaches a false mental model that misleads
debugging and integration. Signal words to check: "starts", "transitions", "switches",
"initializes once" in descriptions of stateless per-call logic; "batches", "queues",
"pools" in descriptions of sequential per-item processing. Example: a README that says
"search starts FTS-only while the vector index builds, then switches to hybrid
automatically" when the code actually checks `vectorHits.length === 0` per-query with
no global mode state — the fallback is stateless, not a one-time transition.

**Stale claims:** Check that descriptions still match after refactoring. A renamed
function, moved parameter, or changed return type can leave the description accurate
for the old code but wrong for the new.

**Structural self-references in docs**: prose that references the document's own
structure — "shown below", "the manual setup above", "the following section", "as
described earlier" — is a claim about the document, and restructuring breaks it the
same way refactoring breaks code claims. For each such reference in a changed doc,
resolve it against the CURRENT document: the target content must still exist, still
sit in the stated position, and still be what the sentence says it is. A reference
to content that was replaced, moved, or demoted into a collapsible aside is a D1
bug even when every command in the document still works. References that still
resolve correctly need no change.

**CI/workflow files** (`.yml`): Apply dimension 1 to step names, job names, and
conditional logic. A step named "Configure Tailscale" that is always skipped due to
an `if:` scoping bug is a description-vs-implementation mismatch. Also check:
- **Multi-trigger event guards**: when a workflow has multiple triggers in `on:`
  (e.g., `pull_request` + `issue_comment`), check whether each job or step should
  run for ALL triggers or only specific ones. Steps that make sense only for PRs
  (review phases, diff analysis, CI checks) need `if: github.event_name ==
  'pull_request'` guards — otherwise a comment-triggered run executes them too.
  The trigger: a workflow with >1 event in `on:` and steps/jobs without `if:`
  guards on `github.event_name`. Trace each step's purpose and ask "does this
  make sense when triggered by [other event]?"
- `if:` conditions reference variables that are actually visible at evaluation time
  (GitHub evaluates `if:` before the step runs — step-level `env:` is not visible)
- **Action pinning inconsistency**: if some actions in the workflow are pinned to
  commit SHAs and others use mutable tags (`v2`, `@main`), the unpinned ones are
  a D1 mismatch — the workflow's security posture is inconsistent. Check all
  `uses:` lines; flag any third-party action on a tag when siblings are on SHAs
- **Permissions wider than usage**: compare `permissions:` grants against what the
  job's steps actually use. If every step authenticates via a GitHub App token and
  never uses `GITHUB_TOKEN` for writes, `contents: write` or `pull-requests: write`
  on the default token is unnecessary privilege
- Secrets are scoped to the narrowest level (step, not job) to limit exposure
- `persist-credentials: false` on checkout steps
- Deploy workflows have `concurrency:` blocks to prevent overlapping runs

### 2. SQL correctness

**For every SQL query in changed files:**

1. **Description-query alignment**: Does the query implement what the tool description
   promises? "Sorted by modification date" needs `ORDER BY mtime DESC`.
2. **Aggregation accuracy**: COUNT(*) vs COUNT(DISTINCT column) — overcounting is
   common when JOINs multiply rows.
3. **Determinism**: Any `LIMIT` without `ORDER BY` returns nondeterministic results.
   If the function's contract implies stable ordering, this is a bug.
4. **WHERE completeness**: Could rows leak through that shouldn't? Check that all
   documented filter conditions appear in the query.
5. **GROUP BY correctness**: Every non-aggregated SELECT column must be in GROUP BY
   (SQLite is lenient here, but the results are undefined for missing columns).

### 3. Type safety and coercion

**Look for these patterns in changed code:**

- **Truthiness bugs**: `if (!x)` where `x` could legitimately be `0`, `""`, or
  `false`. Replace with `x === undefined` or `x === null` for absence checks.
- **Loose equality**: `== 0` or `== ""` almost always wants strict `===`.
- **Type assertions**: `as` casts and `!` non-null assertions bypass the compiler.
  Verify each is safe, or replace with a runtime guard.
- **Missing narrowing**: After a type check (`typeof x === 'string'`), using `x`
  outside the narrowed block where it is still the union type.
- **Buffer/ArrayBuffer view aliasing**: When code accesses `.buffer` on a
  `TypedArray` (Float32Array, Uint8Array, etc.) and passes it to `Buffer.from()`,
  `new DataView()`, or another constructor — check that `byteOffset` and
  `byteLength` are also passed. `typedArray.buffer` returns the *entire*
  backing `ArrayBuffer`, which is larger than the view when the typed array
  is a subarray or slice. The 1-arg form `Buffer.from(arr.buffer)` silently
  produces the wrong data with no error. Fix: use the 3-arg form
  `Buffer.from(arr.buffer, arr.byteOffset, arr.byteLength)`. This pattern is
  especially common in embedding/ML pipelines where providers may return
  Float32Array views over shared buffers.

### 4. Boundary and off-by-one

**For each function with numeric parameters (limit, offset, index, count), and for
each comparison against a sentinel or a bound:**

1. **Empty input**: What happens with an empty array, string, or result set?
2. **Truncation indicators**: If showing "N+" for overflow, does the code fetch
   `limit + 1` to detect truncation? Showing "5+" when exactly 5 results exist
   is a known bug pattern.
3. **Inclusive vs exclusive**: Is the range `[start, end]` or `[start, end)`?
   Off-by-one in slice, substring, and SQL LIMIT/OFFSET.
4. **Zero and one**: These values expose special-case bugs. Does the function
   handle `limit: 0` or `offset: 0` correctly?
5. **Sentinel-returning searches**: `indexOf`, `lastIndexOf`, `findIndex`, and
   `search` return -1 for absent and 0 for found-at-start. A `> 0` guard treats
   found-at-start as absent; the check is `>= 0` or `!== -1`. Boundary: "the
   input can never start with the delimiter because it is validated upstream" is
   not a boundary — the guard is wrong on its own terms, and when the string has a
   formal structure the fix is the stdlib parser (`path.parse`, `URL`) that removes
   the guard entirely.

### 5. Behavioral consistency

**Look for asymmetric handling of similar constructs:**

1. **Parallel code paths**: If function A handles one input variant one way and
   a sibling variant another way, verify the difference is intentional.
   Common: one path handles an edge case that the other path misses. Also check
   **merge/fusion points** — when parallel data sources (e.g., FTS + vector
   search, cache + DB, local + remote) are combined into a single result set,
   verify that constraints (filters, access controls, limits) are applied
   equivalently to each source BEFORE the merge. Asymmetric filtering distorts
   the merged ranking — one source contributes unfiltered items that dilute or
   displace filtered results from the other source. Flag even if intentional —
   the reviewer decides scope, you decide what to report.
2. **Overly broad transformations**: An operation applied everywhere when it should
   be scoped to a specific context (e.g., stripping escape characters globally
   instead of only in table cells where they appear). Concrete instance:
   `trim()` vs `trimEnd()` — when only trailing whitespace cleanup is intended
   but leading whitespace carries semantic meaning (indentation, list nesting
   level), `trim()` silently destroys structure. Flag `trim()` on any string
   where leading whitespace encodes hierarchy or formatting.
3. **Shared helpers not used**: Before accepting a language primitive or stdlib
   call in new code, grep `src/utils/` and nearby modules for an existing helper
   that does the same job. This includes bounded alternatives to unbounded
   primitives — if `mapWithConcurrency` exists and new code uses bare
   `Promise.all` for the same fan-out pattern, that's a miss even though
   `Promise.all` isn't "reimplementing" the helper. The trigger is: new code does
   concurrent work, file manipulation, error formatting, or string processing →
   grep for helpers first, then justify why the primitive is correct if one exists.
4. **Inconsistent defaults**: Same parameter with different defaults in different
   functions — one uses `true`, another uses `false`, with no documented reason.
5. **Config default divergence across file types**: When a PR adds or modifies
   environment variables, check that defaults agree across all layers —
   docker-compose (`${VAR:-value}`), `.env.example`, CLI-generated `.env`
   templates (e.g., `env.ts`), and the config parser in code. Check ALL
   config surfaces — a var added to docker-compose and `.env.example` but
   missing from CLI templates means `npx <tool> init` users never see it.
   Also check that `.env.example` comments match their values: a comment
   saying "Disable X" with a value of `# X=true` is a D1 bug — uncommenting
   enables X, not disables it. A common trap: compose uses `${VAR:-}` (empty
   string when unset) but the config parser uses `.default("true")` which
   only applies when the var is absent, not when it's empty. Empty-string
   defaults bypass code-level defaults and can cause startup failures or
   silent misconfiguration.
6. **Alternative command paths in docs**: when documentation offers more than one
   literal command sequence for the same task (quick-start one-liner vs config
   file vs installer/CLI tool), treat them as parallel code paths. Compare the
   resource identifiers they create or reference — data volume/directory names,
   container/service names, project names, ports, generated file paths. Mismatched
   identifiers mean a user who switches methods silently loses access to existing
   data or state; either the identifiers must match across methods or the doc must
   state that the methods are not interchangeable. Also compare configuration
   flags: an alternative that silently omits behavior the other methods set up
   (health checks, restart policy, log limits) needs a note saying what it omits.
   Skip when the methods are explicitly documented as independent of each other.
   Example: a manual `docker run` one-liner mounting `-v vault_data:/vault` while
   the CLI and Compose paths both use the prefixed `vault-cortex_vault_data`
   volume — a user who starts with the one-liner and later runs the CLI gets
   empty volumes and a full re-sync.

### 6. Input validation and error paths

**For each function that accepts parameters:**

1. **Parameter combinations**: What happens with unexpected combos? (e.g.,
   `section` without `file`, `heading_level` without `heading`). Look for silent
   fallthrough where an error or early return is needed.
2. **Input normalization**: Case-sensitive comparisons on user input that could
   arrive in mixed case (URLs, file extensions, header values).
3. **Error message safety**: Do error messages include internal file paths,
   internal state, or implementation details that shouldn't reach clients?
4. **Guard correctness**: Is a guard condition (`if (value !== "")`) the right
   check? Could it be unconditional, or does it need a different condition?
5. **Silent catches**: `.catch(() => {})` and `catch (e) {}` swallow errors with
   no trace. Every catch must either log or re-throw. Catching without logging is
   worse than not catching — it actively hides failures that affect debuggability.
   When fixing a silent catch, always add a log call with the error and enough
   context (file path, operation name) to diagnose the failure from the log alone.
6. **Widened eligibility**: When a filter or eligibility check is broadened
   (new condition added via `||`, new file type accepted, new input source
   supported), trace what guarantees the old filter implicitly provided.
   The new branch must provide equivalent guarantees — or add explicit
   validation for the ones it loses. Common: `isFile()` → `isFile() ||
   isSymbolicLink()` without adding `realpath()` containment, `stat().isFile()`,
   or broken-symlink handling. Also: widening a query scope, accepting a new
   auth method, or supporting a new content type without corresponding validation.
7. **Fallback direction on failure**: When a read, parse, or lookup fails and
   the code falls back to a default, check which branch that default selects. If
   the default is the destructive or irreversible branch (permanent delete,
   overwrite, skip a validation, grant access), a transient failure silently does
   the worst thing. Separate "not configured" (ENOENT, key absent) from "could
   not read" (EACCES, EISDIR, EIO, a parse error): the first may take the
   default; the second must fail or take the conservative branch. A catch that
   logs at debug level and returns the destructive default is a finding even
   though it is not a silent catch.
8. **I/O error paths**: For each filesystem or network call in changed code
   (`rename`, `mkdir`, `unlink`, `link`, `fetch`), list the errors it raises in
   the intended scenario — `EEXIST`, `ENOTDIR`, `EISDIR`, `EACCES`, `EXDEV` — and
   trace what the caller reports for each. A raw errno message that names an
   absolute path, an operation left half-done (the source still present after a
   "delete"), or an operation that previously always succeeded turned into a hard
   failure by the new path are each findings. Existing call sites in the same
   module show what the error contract is; the new call must meet it.

### 7. Platform and encoding

**Check for assumptions that break on real-world input:**

- **CRLF**: Does text processing assume `\n`? Content from Windows or mixed
  sources may contain `\r\n`. Check line splitting, blank-line detection, and
  regex anchors.
- **Timezone**: Are date operations consistent? Hardcoded UTC when the rest of the
  codebase uses local time (or vice versa) is a subtle bug.
- **Unicode**: String length/slice operations on multi-byte characters. Regex
  patterns that assume ASCII.
- **Symlinks and filesystem indirection**: Does code that walks directories
  or reads files follow symlinks without validating targets? Check for:
  (a) containment — `realpath()` to resolve the target, then verify it stays
  within the allowed root (lexical prefix checks on the original path are
  insufficient); (b) type verification — after resolving, `stat().isFile()`
  to reject directories masquerading as files (e.g., a directory named `*.md`);
  (c) broken links — `realpath()` throws ENOENT for dangling symlinks, catch
  and skip gracefully; (d) defense in depth — validate at every entry point
  (search root, directory listing root, individual entries), not just the
  innermost level. A symlinked search root can bypass all per-entry checks.
- **TOCTOU (time-of-check-time-of-use)**: Is there a gap between checking a
  file's properties (stat, readdir) and acting on it (readFile, index)? In
  concurrent environments, the file can change between check and use.
- **Snapshot staleness in background tasks**: When a PR introduces background
  processing using a startup snapshot (e.g., "snapshot all notes, then embed
  them in the background"), check whether the live code path (file watcher,
  API handler) can invalidate entries in the snapshot while it's being consumed.
  Content-hash gating protects against stale updates (the hash won't match) but
  NOT against deletes — if an entry is deleted after the snapshot but before the
  background loop reaches it, the loop recreates stale data for a deleted entity.
  Check: does the background loop verify the entity still exists before writing?

## Rules that override intuition

These rules exist because agents consistently rationalize skipping findings that
turn out to be real and fixable. Each rule addresses a specific failure pattern.

### No silent skipping

Every finding must appear in the output — including pre-existing gaps discovered
during analysis. "Pre-existing, not introduced by this PR" is context for
categorization, not a reason to omit the finding. If you noticed it, report it.

The orchestrator and user cannot act on findings they never see. A bug you found
and silently dismissed is worse than a bug you missed — you confirmed the problem
exists and then hid it.

### Pre-existing analog gaps

When the PR correctly implements a pattern (e.g., adds `EMBEDDING_ENABLED` to all
6 docker-compose files), and you notice an analogous existing feature is missing
from those same files (e.g., `MEMORY_ENABLED` absent from all of them) — that is a
finding. Report it. The PR's correct implementation revealed the gap; the fix is
typically mechanical (copy the pattern the PR already established).

Categorize as: `pre-existing gap` with a note on whether the fix is trivial.
The orchestrator decides scope; you decide what to report.

### No environment-specific dismissals

Don't use one deployment's specs to dismiss resource, performance, or scaling
concerns. "On Lightsail with 4GB RAM and 772 notes, this is negligible" is not a
valid dismissal for an OSS project where users may have 10x the data on half the
RAM. Evaluate concerns against the worst reasonable use case for the project's
audience — not the maintainer's current setup.

The same rule covers deployment shape and upstream validation. "One vault per
process", "the input is always `.md` because it was validated earlier", and "this
can't happen in practice" describe today's callers, not the code. A guard that is
wrong on its own terms, a module-level cache that is not keyed by the thing it
caches for, or a fallback that picks the destructive branch is a finding whether
or not the current deployment can reach it. Report it, categorize the reach as
context, and let the orchestrator decide scope.

And regardless of impact assessment: if the fix is trivial, fix it. A one-line
null-out after data is consumed costs nothing and eliminates the concern entirely.

### Verify effort before claiming it

Before calling any fix "high lift," "would change every call site," or "complex
refactor" — **grep for actual usages**. The difference between "every call site
across the codebase" and "one call site" is the difference between deferring a
finding and fixing it in 30 seconds. Never estimate effort from intuition when a
10-second grep gives the real answer.

### Your fixes must follow AGENTS.md

You loaded AGENTS.md in "Before starting." Apply it to the code you WRITE, not
just the code you review. If AGENTS.md bans `as` assertions, your fix uses a
runtime guard instead. If it requires early returns, your fix uses early returns.
Bug-check runs after code-quality in the pipeline — no convention pass follows
yours. Convention violations in your fixes ship unchecked.

## Procedure

1. **For each changed production file**, read the full file content (not just the
   diff). Log that you read it — cite what you checked as proof of work:
   ```
   vault-crud-tools.ts (650 lines) — 3 tool defs, 5 cross-refs, 0 SQL, 2 guards
   ```
   "0 findings" after 60 seconds across multiple files means the checks were
   skipped, not that the code is clean.
2. **Weight effort by dimension yield**: dimension 1 (description-vs-code) gets
   the most scrutiny. For each description, quote every sentence verbatim (step 1 of
   dimension 1) then extract and trace claims. This is not optional — it's the
   mechanism that catches truncated text and garbled prose that skimming misses.
   Dimensions 6-7 are quicker but still require reading the code.
3. **One line per finding, then fix it:**
   ```
   [D1] file.ts:612 — description refs vault_find_orphans, should be vault_get_backlinks → description fixed (code matches intent)
   [D1] file.ts:940 — description says setting is honored, code skips it when an env var is set → code fixed (env var was a proxy for state)
   [D4] file.ts:88 — off-by-one in LIMIT, fetches N not N+1 for truncation → fixed (low confidence, trivial fix)
   [D5] file.ts:200 — async init race if called concurrently → flagged (complex fix — needs mutex or queue)
   ```
   Don't describe the planned fix — the diff speaks for itself. A D1 line always
   says which side changed, description or code, and why that side was wrong.
   **Decide fix vs. flag on two axes** — diagnosis confidence and fix complexity.
   **Call `sequentialthinking` before each disposition decision** — input the finding,
   confidence level, and fix complexity; output which matrix cell it falls in and why:
   - **High/medium confidence** → fix directly.
   - **Low confidence + trivial fix** (< 5 lines, no interface change) → fix it.
     The cost of a safe no-op is near zero; the cost of missing a real bug is not.
     "Low-risk" is a reason TO fix, not a reason to defer.
   - **Low confidence + complex/risky fix** → flag with category.
   When flagging, categorize as: `uncertain diagnosis`, `complex fix`,
   `needs design decision`, or `pre-existing gap`. The orchestrator uses
   these categories to triage.
4. Stage, commit, and push all fixes.
5. Report:

```
Bug check complete:
- Files checked: N
- Reviewed at: <PR head SHA>
- Bugs found: N (M fixed, K flagged for review)
- By dimension:
  - Description mismatch: A
  - SQL correctness: B
  - Type safety: C
  - Boundary/off-by-one: D
  - Behavioral consistency: E
  - Input validation: F
  - Platform/encoding: G
- Confidence: N high, M medium, K low
- Dismissed: N (proof-of-dismissal one-liners follow — or "none")
```

**Output honesty (both modes):**

- **State what you reviewed.** The summary names the PR head SHA actually
  reviewed — a review that doesn't say what it checked is indistinguishable
  from one that checked nothing. It also lets the orchestrator cross-check
  what this phase actually saw against the delta-review baseline it records
  itself at Phase 4 close.
- **Close with proof of dismissal.** One line per suspicion you seriously
  considered and dropped, with the reason it doesn't bite — or "none". This
  extends "No silent skipping" to the negative space: findings you confirmed
  go in the report, and suspicions you cleared go in the dismissal list —
  without them, "no findings" could mean a clean diff or an unexamined one,
  and the reader can't tell which.

## Comment mode

When the dispatch prompt says **COMMENT MODE**, do not edit files, commit, or push.
Instead, collect all findings and post them as a single GitHub PR review with inline
comments.

### Procedure

1. **Hunt normally** — read every changed file in full, apply all 7 dimensions, use
   sequential thinking for disposition decisions. The only difference is the output path.
2. **Collect findings** as you go. Each finding needs: file path (relative to repo root),
   line number, dimension tag, confidence level, and description.
3. **Still categorize on two axes** — confidence × complexity. In comment mode, `fixed`
   becomes `would fix` (high/medium confidence, trivial change) and flagged findings keep
   their category.
4. **Post a single PR review** with all findings as inline comments:

```bash
gh api "repos/OWNER_REPO/pulls/PR_NUMBER/reviews" \
  --method POST --input - <<'REVIEW'
{
  "event": "COMMENT",
  "body": "## Phase 4: Bug Check\n\nN bugs found across M files. Reviewed at <HEAD_SHA>.\nConfidence: A high, B medium, C low\n\nDismissed: <proof-of-dismissal one-liners — or \"none\">\n\n---\n*🔍 ship-check · bug-check · MODEL_ID*",
  "comments": [
    {
      "path": "src/file.ts",
      "line": 612,
      "body": "**[D1]** Description refs `vault_find_orphans`, should be `vault_get_backlinks`\n\nThe description says \"find notes with broken links\" but `vault_find_orphans` finds notes with no *incoming* links. `vault_get_backlinks` is the correct tool for outgoing link validation.\n\n*Would fix (trivial, high confidence)*\n\n---\n*🔍 ship-check · bug-check · MODEL_ID*"
    }
  ]
}
REVIEW
```

Replace `OWNER_REPO` and `PR_NUMBER` with values from the dispatch prompt. Replace
`MODEL_ID` with your own model ID (from your system prompt).

5. **If 0 findings and no dismissals**, skip the API call — report "0 findings"
   to the orchestrator only. With 0 findings but cleared suspicions, post a
   body-only review carrying the dismissal list — that is the artifact that lets
   a PR reader tell a clean diff from an unexamined one.
6. **Footer on every comment.** Append `\n\n---\n*🔍 ship-check · bug-check · MODEL_ID*`
   to the review body AND each inline comment body.
7. **Format each inline comment body** as:
   - Bold dimension tag: `**[D1]**`, `**[D3]**`, etc.
   - One-line description of the bug
   - Evidence: quote the description sentence, trace the code path, explain the mismatch
   - Suggested fix (code snippet when possible)
   - Disposition: `Would fix (trivial, <confidence>)` or `Flagged: <category>`
   - Footer (see above)