feat: formalize general-reviewer in the agent pack

[devonjones]

Summary

Per Devon's direction: with no Gemini/Cursor on this repo, the pack needs a formalized broad-coverage reviewer rather than an ad-hoc fallback.

• .reviewers/general-reviewer.md — whole-PR generalist spec: logic correctness against claims/docs (verified empirically — run the command, decode the token, query the schema), domain/design soundness against upcoming beads tickets, contract fidelity, cross-cutting interactions, PR-body/docstring accuracy. Explicitly non-overlapping with the 13 specialists. Spawns on every PR, every round; retires last; posts a single structured PR comment (Critical/Medium/Minor → P1/P2/P3).
• AGENT-REVIEWERS.md — agent entry, scope-table row, severity mapping, and a "no external review bots" note so future loops skip the 5-minute Gemini --wait polls.

This codifies the pass that ran ad-hoc on the earlier PRs and produced the highest-value catches of each loop: the invalid ruff flag (#223), the JWT AttributeError escape (#224), and the multi-accept AND-vs-ANY bug (#226).

Note: the .beads/issues.jsonl diff also carries routine beads sync bundling (status fields on 750/d7a from earlier in the session), not just this ticket.

Test plan

• Docs-only; CI unaffected.
• Dogfooded: the general-reviewer reviewed this PR using the spec it adds — and correctly caught a misattribution in this very PR body (since fixed).

Beads: openforge_catalog-aq5 (closed via bd close on merge).

🤖 Generated with Claude Code

[devonjones]

Code Review (general-reviewer)

Self-review of the PR that formalizes this very reviewer. Verified factually where the spec's own beat #5 demands it: PR body claims, the cited prior catches on #224-#226, the no-Gemini/Cursor claim, and internal consistency between .reviewers/general-reviewer.md and AGENT-REVIEWERS.md.

Critical Issues

None.

Medium Issues

1. The "invalid ruff flag" catch is misattributed to PRs #224-#226 — it actually happened in PR #223.

File: PR body (and propagated into the commit message and .beads/issues.jsonl — ticket openforge_catalog-aq5)
Quote:

Previously run ad-hoc in PRs #224-#226, where this pass produced the
highest-value catches of each loop (invalid ruff flag, JWT
AttributeError escape, the multi-`accept` AND-vs-ANY bug).

Issue: I pulled the actual review bodies for #224, #225, and #226 (gh api repos/.../pulls/{224,225,226}/reviews) and grepped for ruff/flag/complexity — no ruff-flag finding appears in any of them. The JWT AttributeError catch is genuinely in #224 ("_jwt_exp crashes ... on a structurally-valid JWT whose payload segment decodes to non-object JSON"), and the multi-accept AND-vs-ANY catch is genuinely in #226. But the invalid---max-complexity-flag catch is from PR #223 — the original agent-pack PR itself (.reviewers/complexity-reviewer.md:12's ruff check --select C901 --max-complexity 10, which errors with unexpected argument '--max-complexity'). That's a different PR than the ones being cited, and it predates #224-#226.

This matters because the entire justification for formalizing this reviewer rests on "look at the track record" — getting one of the three cited examples wrong (and by attributing it to the wrong batch of PRs) undercuts the empirical case the PR is making, and the same wrong claim is now baked into beads ticket openforge_catalog-aq5's description, so it'll resurface anywhere that ticket is quoted.

Suggested fix: reword to something like: "an invalid CLI flag caught in the pack's own creation PR (#223), a JWT AttributeError in #224, and the multi-accept AND-vs-ANY bug in #226" — or drop the PR numbers instead of misattributing them. Also correct the description on openforge_catalog-aq5 (bd update before/after close) since it repeats the same claim verbatim.

Minor Issues

1. The general "skip on scope mismatch" rule and the always-spawn rule for general-reviewer don't actually conflict, but only because "everything" trivially always matches — worth a one-line callout.

File: AGENT-REVIEWERS.md:146,161
Quote:

| `general-reviewer` | everything — spawns on every PR, every round |
...
Skip reviewers whose file scope doesn't match the PR diff.

Issue: Checked this for contradiction as instructed — there isn't one, since a scope of "everything" is satisfied by any diff, so the generic skip rule never fires for this reviewer. But it's easy to misread the two lines in isolation (a future editor could narrow the "everything" cell without noticing it silently reintroduces skip-eligibility for a reviewer whose own spec says it must never be skipped). Low cost to make it explicit.

Suggested fix: append "(never skipped — see its spec)" to the table cell, e.g. everything — spawns on every PR, every round (exempt from the scope-skip rule below).

2. This "docs-only" PR's .beads/issues.jsonl diff also silently flips unrelated tickets to in_progress and adds notes to them.

File: .beads/issues.jsonl
Quote:

-{"id":"openforge_catalog-750",...,"status":"open",...}
+{"id":"openforge_catalog-750",...,"status":"in_progress",...}
-{"id":"openforge_catalog-d7a",...,"status":"open",...}
+{"id":"openforge_catalog-d7a",...,"status":"in_progress",...,"notes":"..."}

Issue: The PR body's test plan says "Docs-only; CI unaffected," which is true for code/CI purposes, but the beads diff isn't scoped to this PR's actual subject (formalizing general-reviewer) — it also carries status/notes changes for two unrelated Thingiverse epic tickets (750, d7a) picked up from the shared beads log at commit time. Not a defect (this is how the beads daemon/sync normally bundles whatever changed since the branch's base), but worth a one-line PR-body mention so a reviewer skimming the beads diff doesn't wonder whether those ticket transitions were intentional to this change.

Suggested fix: none required to merge; optionally note in the PR body that the beads diff includes incidental sync of unrelated ticket state.

Positive Observations

• The Gemini/Cursor-not-installed claim holds up empirically: I checked all comments across PRs #219-#227 via the GitHub API and found zero comments from any bot account (gemini-code-assist, cursor, etc.) — every comment on every PR is from devonjones. Consistent with the claim.
• The anti-overlap list in .reviewers/general-reviewer.md's "Explicitly NOT this reviewer's beat" maps cleanly onto all 13 specialist files in .reviewers/ with no gaps and no phantom entries (hooks/async patterns correctly doubles up for both react-hooks-reviewer and js-async-reviewer).
• Severity mapping, spawn rules, and PR-comment-vs-line-comment posting are stated identically in both .reviewers/general-reviewer.md and its AGENT-REVIEWERS.md index entry — no drift between the two.
• The JWT AttributeError (#224) and multi-accept AND-vs-ANY (#226) citations both check out verbatim against the actual PR review history — these are exactly the "empirical, not vibes" catches the spec wants to hold itself to, which makes the one misattributed example (above) worth fixing rather than leaving as an easy nitpick.
• Pack-count phrasing ("13 specialists") is accurate and consistent with the 13 files under .reviewers/ excluding general-reviewer.md itself — no stale "13 vs 14" count anywhere in the repo.

Recommendations Summary

Must fix before merge: none.
Should fix: correct the #223-vs-#224-#226 misattribution of the "invalid ruff flag" catch in the PR body and in beads ticket openforge_catalog-aq5's description.
Nice to have: annotate the scope table's "everything" row as exempt from the scope-skip rule; mention the incidental unrelated-ticket beads sync in the PR body.

Overall Assessment

Solid, well-scoped addition — the spec is internally consistent with its own index entry, the anti-overlap list is complete against the actual reviewer roster, and two of the three cited empirical catches check out exactly as described. The one factual slip (misattributing the ruff-flag catch to #224-#226 instead of #223) is minor in isolation but worth fixing since it's the kind of claim this very reviewer is chartered to catch. Approve once that attribution is corrected; the minor items are optional polish.

[devonjones]

Response to general-reviewer Review

Medium (ruff-flag catch misattributed to #224–#226): Fixed — PR body and beads ticket aq5 now attribute it to #223 (the pack's own creation PR). The commit message is immutable history; corrected everywhere editable. Fitting first catch for a reviewer whose spec says "verify, don't vibe."

Minor 1 (skip-rule vs always-spawn annotation): Fixed — the skip rule now notes general-reviewer's "everything" scope always matches by construction.

Minor 2 (beads diff bundles unrelated status flips): Fixed — PR body now discloses the routine beads sync bundling.