auto-mobile-code-review

Files (2)

• SKILL.md
• openai.yaml · agents/

SKILL.md (first lines)

# AutoMobile Code Review

Review a change — a PR (its number passed as the argument) or, with no argument, the current branch's
diff vs `origin/main`. Few verified findings beat many plausible ones.

**Never post to GitHub.** No review comments, reviews, or summaries. Report findings in the
session. The only permitted GitHub write is resolving a review thread, under the conditions in
_Working our own PR_.

Review against the author's intent and the issue the change serves, not the diff in isolation.
Ask rather than assert when genuinely unsure — sometimes you missed something. Name what's
genuinely good in a sentence; never manufacture it.

## Step 0 — Resolve the diff base

Everything below reuses `$BASE`, so establish it first. Which base is correct depends on the
mode, and getting it wrong silently scopes the review to the wrong commits:

```bash
git fetch origin main

# PR mode — the local checkout is usually NOT the PR branch, so HEAD is unrelated to it.
# The leading + forces the update: without it, re-fetching after a force-push is rejected
# non-fast-forward and Step 0 fails before refreshing anything.
git fetch origin "+pull/<PR>/head:refs/remotes/pr/<PR>"
BASE=$(git merge-base "refs/remotes/pr/<PR>" origin/main)
DIFF_ARGS=("$BASE" "refs/remotes/pr/<PR>")   # two endpoints: the PR head, not your tree

# Branch mode — one endpoint, so the diff includes uncommitted work. Naming HEAD as a
# second endpoint would silently drop every working-tree edit.
BASE=$(git merge-base HEAD origin/main)
DIFF_ARGS=("$BASE")
```

Use `"${DIFF_ARGS[@]}"` for every diff below. The distinction is not cosmetic: in branch mode a
two-endpoint diff omits exactly the uncommitted changes the review is supposed to cover.

Both obvious branch-mode alternatives are wrong in a common case: `origin/main...HEAD` (three
dots) is committed work only and reports **zero files** against a dirty tree, silently scoping
the review to nothing; `git diff origin/main` (no dots) picks up uncommitted work but, when the
branch is behind main, folds main's newer commits in as reverse-diffs.

## Step 1 — Establish the change's real state, before reading code

Most escaped defects here were already red on the PR itself. These checks take seconds and
frequently end the review.

```bash
gh pr checks <N> --json name,state,bucket,link
gh pr view <N> --json mergedAt,mergeable,mergeStateStatus,baseRefOid,headRefOid,autoMergeRequest,statusCheckRollup
```

- **Any `failure` blocks — required or not.** Automerge lands PRs over non-required failures:
  [#3969](https://github.com/kaeawc/auto-mobile/pull/3969) auto-merged with `Build Xcode Projects`, `XCTestRunner Simulator Tests`, and `iOS Build`
  red, the pbxproj drift check having worked exactly as designed while the merge gate never
  consulted it.
- **Unfinished checks with automerge armed block equally.** [#4088](https://github.com/kaeawc/auto-mobile/pull/4088) merged five seconds before
  `Swift Packages (Xcode 26.5)` concluded failure. For a merged PR, verify gates concluded
  _before_ the merge:
  ```bash
  gh api repos/kaeawc/auto-mobile/commits/<headRefOid>/check-runs?per_page=100 \
    --jq '.check_runs[] | "\(.name)\t\(.conclusion)\t\(.completed_at)"'
  ```
  Any `completed_at` at or after `mergedAt` means the gate did not gate.
- **Green is only as current as the base it ran on.** Use the Step 0 `$BASE` — the merge-base
  — rather than `baseRefOid`. `$BASE` is computed from the commit graph, so it answers the
  question that actually matters (what was this head built on top of?) without depending on
  when GitHub refreshes a metadata field, it stays correct when a branch integrates main via a
  _merge_ rather than a rebase, and it works identically in branch mode, where there is no
  `baseRefOid` at all. In practice the two agree — measured on
  [#4106](https://github.com/kaeawc/auto-mobile/pull/4106) (both `abd432675`, 11 behind) and
  [#4041](https://github.com/kaeawc/auto-mobile/pull/4041) (both `333c9923e`, 15 behind) — but
  only one of them is correct by construction.

  Cross-check against GitHub's own count, which needs no interpretation at all:

  ```bash
  gh api "repos/kaeawc/auto-mobile/compare/main...<headRefOid>" --jq '.behind_by'
  ```

  If `$BASE` differs from `origin/main` (equivalently, `behind_by > 0`):

  ```bash
  # --format= suppresses commit messages, so only paths print.
  git log "$BASE"..origin/main --name-only --format= -- \
    .github/workflows scripts/all_fast_validate_checks.sh \
    android/build.gradle.kts android/gradle/libs.versions.toml \
    eslint.config.* eslint-suppressions.json | sort -u
  ```

  Any hit means rebase-and-re-run, not approval. [#4016](https://github.com/kaeawc/auto-mobile/pull/4016) went green at 05:44, [#4005](https://github.com/kaeawc/auto-mobile/pull/4005) turned on the
  detekt gate at 05:48, [#4016](https://github.com/kaeawc/auto-mobile/pull/4016) auto-merged at 05:49 — reddening main. Its `Fast Validation` job
  had no `Run detekt` step at all.

- **Run the tests the diff changed.** [#4070](https://github.com/kaeawc/auto-mobile/pull/4070) landed a deterministically-red assertion because a
  refactor moved argv construction and updated one of two sibling tests. Use the Step 0 `$BASE`, so uncommitted test edits are included, and guard the empty case explicitly rather
  than relying on `xargs -r` (GNU-only on older macOS and other BSDs):
  ```bash
  changed_tests=$(git diff --name-only "${DIFF_ARGS[@]}" -- 'test/**/*.test.ts')
  [ -n "$changed_tests" ] && bun test $changed_tests
  ```

Pull the failing _job's_ log, not the whole run — a finished job's log is readable while the run
continues (see `github-cli`).

**Classify each red result before treating it as a finding**: PR-caused, pre-existing on main,
transient/infrastructure, or unproven — grounded in the job log, changed paths, and current
`origin/main`. Never recommend a code change for an unrelated or unproven failure, but do name
it in the summary. Flag one check name appearing twice on the head SHA with different
conclusions: a stale cancelled run beside a fresh green one silently parks automerge.

## Step 2 — Scope and size the diff

`git fetch origin main` first. With a PR: `gh pr view <N> --json
title,body,headRefOid,files,closingIssuesReferences`, `gh pr diff <N>`, then read the changed
files on disk and the issue it claims to close — a reviewed head can differ from what
squash-**merged**, and a "fix" PR can merge test-only, so check `git log origin/main` for what
actually landed. With no argument, review committed _and_ uncommitted work; read changed files
in full, since the hunk lies by omission.

From kaeawc/auto-mobile, Apache-2.0.

More from kaeawc/auto-mobile