feat: add agent reviewer pack for pr-review-loop

[devonjones]

Summary

Creates the agent reviewer pack consumed by the pr-review-loop skill, modeled on 521-studios/wyrd's pack. This is the prerequisite for running the review loop on the upcoming Thingiverse publish/sync epic PRs (openforge_catalog-4kx).

• AGENT-REVIEWERS.md — index of 13 reviewers with what-it-checks / when-to-spawn lines, per-reviewer file scopes, severity conventions (P1/P2/P3 mapped to beads deferral rules), structured output format, and branch-targeting guidance (test, never main).
• .reviewers/*.md — full per-reviewer specs:
  • Adapted from wyrd: complexity (extended to TS/React, wired to CLAUDE.md's And/Or test + one-screen rule), error-handling, test-coverage (pytest + Jest), resource-leak (serverless Postgres connection cap = P1), dead-code, logging, importlib-resources (Lambda zip).
  • Project-specific: migration-discipline (version_NN.py system), serverless-architecture (Lambda size, R2-not-S3, no persistent state, N+1), frontend-conventions (relative /api rule, static-export constraints, hooks), credentials-hygiene (secrets/JWTs/HAR captures — timely for the Thingiverse work).
  • Dedicated SPA reviewers: react-hooks (deps, cleanup, fetch races, catalog-scale list rendering), js-async (floating promises, resp.ok, swallowed rejections).
• .gitignore — adds *.har and an ignored captures/ directory (traffic captures carry live credentials).

Test plan

• Docs-only change — no code paths. CI (pytest/jest/lint) passes untouched.
• Pack gets exercised end-to-end by the first Thingiverse PR review loop.

Beads: openforge_catalog-1v5 (closed via bd close when this merges — GitHub's Closes keyword can't close beads tickets).

🤖 Generated with Claude Code

[devonjones]

/gemini review

[devonjones]

Code Review (Claude)

I've reviewed the changes in this PR. Here's my analysis:

I read every file in full (AGENT-REVIEWERS.md, all 13 .reviewers/*.md, the .gitignore diff, root CLAUDE.md, openforge/CLAUDE.md, src/CLAUDE.md), and cross-checked the pack's factual claims against the actual repo: openforge/db/fixtures/__init__.py, openforge/db/schema/version_12.py and the migration runner, the Postgres connection pattern in openforge/app/, .github/workflows/test.yaml, package.json, next.config.js, and the pr-review-loop skill's discover-agents.sh parser + SKILL.md (to check the pack's format is actually consumable by the tool it's written for).

Critical Issues

1. The complexity-reviewer's stated ruff invocation doesn't run.
   .reviewers/complexity-reviewer.md:12:

   **Objective floor (Python): McCabe complexity > 10.** Run `ruff check --select C901 --max-complexity 10` against the PR head...
   

   I ran this exact command against the repo: ruff check --select C901 --max-complexity 10 fails with error: unexpected argument '--max-complexity' found. Ruff has no --max-complexity CLI flag — mccabe.max-complexity is a pyproject.toml/ruff.toml setting only, and this repo's pyproject.toml doesn't set it. The correct command is just ruff check --select C901 . (ruff's mccabe default is already 10 — confirmed by running it here, e.g. openforge/app/routes/__init__.py:8: C901 'authenticate' is too complex (15 > 10)). An agent following the spec literally will hit a shell error on its very first review action for every Python PR. This is the reviewer's headline objective-floor check — it needs to work.

2. The PR claims to close a ticket that its own diff shows as still open.
   Both commit messages and the PR body say "Closes openforge_catalog-1v5". But the .beads/issues.jsonl entry this PR adds for that exact ticket (.beads/issues.jsonl:1) is:

   {"id":"openforge_catalog-1v5", ..., "status":"in_progress", ...}

   — no close_reason, no closed_at. Compare to how this repo's own convention marks a ticket done, e.g. openforge_catalog-4ji in the same file: "status":"closed","closed_at":"...","close_reason":"Completed: ...". Either the "Closes" trailer is wrong (the work isn't actually done from beads' point of view) or the ticket update was forgotten. This is a self-contradiction shipped inside the PR's own diff, not a stylistic nit.

Medium Issues

1. AGENT-REVIEWERS.md's "Default severity per reviewer" table doesn't match the explicit per-item tags in several specs it summarizes. Since the workflow does tell spawned agents to read the full .reviewers/<name>.md (so the explicit inline tags should win in practice), this is a documentation-accuracy problem rather than a live bug, but it's the kind of drift the index exists to prevent:

   • credentials-hygiene-reviewer: index says "P1 by default" (unqualified). The spec (.reviewers/credentials-hygiene-reviewer.md) only tags 3 of 7 patterns P1 (items 1, 2, 4); items 3 and 5 are explicitly P2, items 6 and 7 explicitly P3.
   • serverless-architecture-reviewer: index says "P1 for S3-instead-of-R2, persistent-state features, and connection-pattern violations; P2 otherwise." But the spec explicitly tags item 7 ("Frontend assumptions of a server") P1 — not listed in the index's P1 set — and item 8 ("Unbounded Lambda invocation patterns") P3, not P2 as "otherwise" implies.
   • migration-discipline-reviewer: index says "P2 otherwise," but the spec explicitly tags item 4 (missing docstring) and item 7 (silent data migrations) P3.
   • frontend-conventions-reviewer: index says "P2 otherwise," but the spec explicitly tags item 5 (state anti-patterns) and item 7 (utility duplication) P3.
   • js-async-reviewer and react-hooks-reviewer have the same pattern (js-async items 5/6/7 and react-hooks item 8 are explicit P3s not captured by the index's "P2 by default" summary).

   Net effect: a reader who trusts the index table alone (rather than opening every spec) will over-promote several intentionally-advisory P3 findings to P2/P1. Recommend either fixing the summaries or adding "see spec for exceptions" next to each.

2. The "Output format" template doesn't account for how the consuming skill actually anchors comments. AGENT-REVIEWERS.md's output template requires every finding to embed File: path/to/file.py:LINE in the comment body. But pr-review-loop's actual posting mechanism (scripts/post-line-comment.sh <PR> <file> <line> <agent-name> "<text>", per that skill's SKILL.md) already takes file and line as separate arguments that anchor the GitHub comment to that exact location. The template's embedded File:/line header is therefore redundant with the anchor the harness already provides — harmless, but the spec doesn't acknowledge it, and a reviewer copying the "File:" line from the quoted example verbatim instead of using the real anchor argument risks the two disagreeing.

Minor Issues

1. One-directional overlap acknowledgment. .reviewers/react-hooks-reviewer.md item 5 explicitly notes "(Shared with frontend-conventions-reviewer, post under whichever found it first.)" for derived-state-in-useState findings. .reviewers/frontend-conventions-reviewer.md item 5 describes the same pattern ("derived state stored in useState + synced with useEffect") but doesn't reciprocally reference react-hooks-reviewer. Since these two reviewers' spawn conditions overlap on almost every touched .tsx file, it'd be worth stating the cross-reference from both sides rather than one.

2. PR description reviewer count is stale. The PR body says "AGENT-REVIEWERS.md — index of 11 reviewers," but the second commit ("add dedicated SPA reviewers") brought the total to 13 (react-hooks-reviewer and js-async-reviewer were added after that sentence was written). Not a file in the shipped pack, but worth a quick edit before merge so the PR record isn't misleading to future readers of git history.

Positive Observations

• Every factual claim I could verify against the repo checked out true: openforge/db/fixtures/__init__.py's importlib.resources usage, the PgDB.connection() pattern reused uniformly across openforge/app/routes/*.py, version_12.py's exact @SchemaVersionDecorator/SchemaBase/up_impl/down_impl shape, the version-15 gap, the CI job commands (pytest tests/ --cov=openforge, npm test, npm run lint:all, ruff check ., ruff format --check .) matching .github/workflows/test.yaml verbatim, frontend test locations/commands matching package.json, and the static-export (next.config.js: output: "export") / single-Lambda (aws_lambda_wsgi) architecture claims being current rather than stale. This pack is clearly grounded in the actual codebase, not generic boilerplate.
• AGENT-REVIEWERS.md's own structure (# Agents H1 → H2 reviewer names → # Guidelines → # Context) matches the pr-review-loop skill's discover-agents.sh parser exactly — this file will actually parse correctly by the tool that's supposed to consume it, which isn't a given for a hand-authored spec file.
• Thoughtful, explicit division of labor between overlapping reviewers in most places: frontend-conventions-reviewer explicitly defers complexity thresholds to complexity-reviewer; serverless-architecture-reviewer explicitly defers leak shapes to resource-leak-reviewer while keeping acquisition-pattern violations for itself; credentials-hygiene-reviewer explicitly defers log-interpolation leaks to logging-reviewer.
• Strong "Do NOT flag" sections throughout (e.g. dead-code-reviewer correctly excludes Flask route decorators, pytest fixtures, and Next.js reserved exports — the exact false-positive traps naive dead-code tools fall into) show real familiarity with this codebase's idioms, not copy-pasted generic advice.
• The .gitignore addition (*.har) is correctly justified and timed — it lands ahead of the Thingiverse HAR-capture work described in the linked beads epic, exactly the kind of "prerequisite" hygiene this PR claims to deliver.

Recommendations Summary

Must fix before merge:

1. Fix the broken ruff check --select C901 --max-complexity 10 command in .reviewers/complexity-reviewer.md:12 — drop --max-complexity 10 (ruff's default is already 10) or move it to pyproject.toml config if a different threshold is ever wanted.
2. Resolve the openforge_catalog-1v5 status contradiction — either update the beads entry to closed with a close_reason, or remove the "Closes" claims from the commit messages/PR body if the ticket is intentionally left open for follow-up work.

Should fix:
3. Correct the "Default severity per reviewer" summaries in AGENT-REVIEWERS.md (or annotate them as approximations) so they match the explicit P1/P2/P3 tags actually present in credentials-hygiene-reviewer.md, serverless-architecture-reviewer.md, migration-discipline-reviewer.md, frontend-conventions-reviewer.md, js-async-reviewer.md, and react-hooks-reviewer.md.
4. Clarify that the "Output format" template's File:/line header is descriptive only — the actual comment anchor comes from post-line-comment.sh's <file> <line> arguments, not from text in the body.

Nice to have:
5. Make the react-hooks-reviewer ↔ frontend-conventions-reviewer overlap acknowledgment bidirectional.
6. Update the PR description's "11 reviewers" to 13 before merge.

Overall Assessment

This is a well-researched, codebase-grounded reviewer pack — nearly every factual claim I checked against the repo held up, and the reviewer-to-reviewer scoping shows real thought about avoiding duplicate ownership. The two concrete defects (a non-functional ruff invocation that's the complexity-reviewer's core check, and a "Closes" claim contradicted by the ticket's own shipped status) are both easy, mechanical fixes; once those and the severity-table drift are addressed, this is ready to merge.

[devonjones]

Response to Claude Review

Critical 1 (invalid ruff command in complexity-reviewer.md): Fixed — command corrected to ruff check --select C901 . with a note that max-complexity is a pyproject.toml setting ([tool.ruff.lint.mccabe]), not a CLI flag, and that ruff's default is already 10. Good catch — a reviewer agent would have hit the error mid-review.

Critical 2 ("Closes openforge_catalog-1v5" vs ticket in_progress): Fixed in process — PR body now says the ticket is closed via bd close at merge time (GitHub's Closes keyword only operates on GitHub issues; the beads ticket stays in_progress while the PR is under review, which is the accurate state). The commit-message trailers stay as-is — rewriting pushed history for a wording nit isn't worth it.

Medium 1 (severity table vs per-item spec tags): Fixed — added an explicit precedence rule to the Guidelines: a spec's per-item priority tag wins over the index table; the table gives defaults, not ceilings/floors.

Medium 2 (File:/LINE in body redundant with post-line-comment.sh anchoring): Fixed via clarification — kept the File: line deliberately (it survives quoting, PR-level consolidation, and beads copy/paste where the anchor doesn't) and documented that rationale next to the template.

Minor 1 (one-directional overlap acknowledgment): Fixed — frontend-conventions-reviewer #5 now cross-references react-hooks-reviewer with a don't-double-post rule, mirroring the acknowledgment on the hooks side.

Minor 2 (stale "11 reviewers" count in PR body): Fixed — body updated to 13 and now lists the SPA reviewers.

[devonjones]

Verification Review (Claude)

Verified all 6 round-1 findings fixed at 85ada01 (fix: address round-1 review findings):

1. Critical 1 (ruff command) — Fixed. .reviewers/complexity-reviewer.md:12 now says ruff check --select C901 . with a note that max-complexity is a pyproject.toml setting, not a CLI flag. Ran it against the repo head: no shell error (the old --max-complexity flag failed with 'unexpected argument'; the corrected command exits 1 only because it reports 18 real pre-existing C901 violations, which is expected/correct tool behavior).
2. Critical 2 (Closes ticket vs in_progress) — Addressed in process, not code: PR body now states the beads ticket closes via bd close at merge (.beads/issues.jsonl:1 correctly still shows in_progress, matching reality while the PR is under review).
3. Medium 1 (severity precedence) — Fixed. AGENT-REVIEWERS.md:185 adds: "Where a spec under .reviewers/ tags a specific pattern with an explicit priority, the spec's tag wins over this table."
4. Medium 2 (File:/LINE rationale) — Fixed. AGENT-REVIEWERS.md:206 documents why File: stays in the body despite post-line-comment.sh anchoring (survives quoting/consolidation/beads copy-paste).
5. Minor 1 (mutual overlap) — Fixed. frontend-conventions-reviewer.md:20 now reciprocally cross-references react-hooks-reviewer with a don't-double-post note, matching the existing reference on the hooks side.
6. Minor 2 (stale count / .gitignore) — Fixed. PR body says 13 reviewers and lists the SPA additions; .gitignore adds captures/ alongside *.har for raw-dump captures of any extension. Confirmed no existing captures/ directory or tracked files are newly ignored.

No new issues introduced by the fix commit — all four changed files (.gitignore, .reviewers/complexity-reviewer.md, .reviewers/frontend-conventions-reviewer.md, AGENT-REVIEWERS.md) are minimal, surgical additions with no side effects. LGTM.