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

.beads/last-touched

@@ -0,0 +1 @@
+openforge_catalog-1v5

.gitignore

@@ -21,6 +21,13 @@
 scratch
 
 .env
+
+# traffic captures — carry live credentials (JWTs, cookies); never commit.
+# Raw request/response dumps of ANY extension belong in captures/ (ignored)
+# or the session scratchpad, never elsewhere in the repo tree.
+*.har
+captures/
+
 # debug
 npm-debug.log*
 yarn-debug.log*

.reviewers/complexity-reviewer.md

@@ -0,0 +1,50 @@
+# complexity-reviewer
+
+Review **production code only** for function complexity. **Skip all files in `tests/`, `integration_tests/`, and `__tests__/`** — test files often have long fixtures, parametrize tables, and assertion blocks that don't need the same complexity constraints.
+
+This reviewer enforces the project's Code Organization Philosophy (CLAUDE.md): *"Functions should be describable with as few uses of 'and' or 'or' as possible."* It applies to both Python (`*.py`) and TypeScript/React (`*.ts`, `*.tsx`).
+
+## Posting gates (read before flagging anything)
+
+1. **Introduced or worsened only — not pre-existing.** Flag complexity this PR *creates* or *materially worsens*. If a function was already over a threshold before this PR (it was long/complex on the base branch) and this PR only edits a few lines inside it without pushing it further over, it is **out of scope** — do not post it. (You may note it once as a P3 defer-to-beads suggestion, but not as a finding that blocks the PR.) Check the diff: is the threshold breach in *added* lines, or did the PR push an already-borderline function past the limit? If neither, skip.
+2. **Hard violations always; soft heuristics only when they compound.** The objective complexity floor and unambiguous structural smells (depth ≥ 4, > 5 params, > 20 public methods, nested ternaries ≥ 2 levels) post on every occurrence. The **"And/Or" test** and the **one-screen rule** are *advisory*: do **not** post them as standalone findings for a function that passes the objective floor and sits within ~50–60 lines. Raise a soft heuristic only when it compounds a hard violation on the same function.
+
+**Objective floor (Python): McCabe complexity > 10.** Run `ruff check --select C901 .` against the PR head (never a stale local checkout) and flag every function that exceeds it. (Ruff's mccabe `max-complexity` defaults to 10; it is a `pyproject.toml` setting — `[tool.ruff.lint.mccabe]` — not a CLI flag.) For TypeScript, apply the same threshold by inspection (or `eslint` `complexity` rule output if configured).
+
+Apply these heuristics on top of the objective floor:
+
+1. **"And/Or" test** (from CLAUDE.md): minimize the number of "and"/"or" needed to describe what a function does. If you need multiple conjunctions, the function is doing too much.
+   - Good: "This function validates and saves user data" (validation is a prerequisite for saving — cohesive).
+   - Bad: "This component handles state AND rendering AND keyboard events AND mouse drag events."
+
+2. **One-screen rule** (from CLAUDE.md): functions should fit on one screen (~50–60 lines).
+   - **Internal functions don't count**: lines of nested helper `def`s / inner closures do NOT count against the parent's limit — only the main body lines.
+   - Pragmatic exception (also from CLAUDE.md): larger functions are acceptable when breaking them up would genuinely complicate rather than simplify. If invoking this exception, say so and why.
+
+3. **Extractable inner structures**: if a block has a clear purpose, suggest extraction:
+   - Python: module-level `_helper()` first; sibling helper module second.
+   - React: extract event handling and stateful logic into **custom hooks**; extract render fragments into components.
+
+4. **Nesting depth**: flag functions with indent depth ≥ 4 inside the body. Use early returns to flatten (`if not x: return` / `if (!x) return`).
+
+5. **Parameter count**: flag more than **5 positional parameters**. Refactor to a config object (dataclass / TypedDict / props object) or keyword-only args.
+
+6. **Class size (public method count)**: flag classes with **more than 20 public methods**. Private helpers (`_foo`) do NOT count — extracting helpers as private methods is exactly what this reviewer encourages. Advisory (P3): ask whether the public methods cluster around a single responsibility.
+
+7. **Nested ternaries**: chained `x if a else y if b else z` (or JSX `a ? x : b ? y : z`) is hard to read past one level. Recommend `if/elif/else`, a dict-dispatch lookup, or in JSX an early-return / lookup-map pattern. A single ternary is fine; flag at the second level.
+
+8. **Redundant single-call wrappers**: a function that exists only to call one other function with no added validation, normalization, error context, or naming benefit. Single-call, single-caller, no-added-meaning → flag.
+
+9. **Generic identifiers in long functions**: `data`, `temp`, `result`, `value`, `obj` reused for different things in a long function. Flag only when the function is long enough that the generic name actively misleads. Short helpers (≤10 lines) can use generic names.
+
+**Do NOT flag:**
+
+- Test files.
+- Long-but-linear functions (no branching, sequential transformations) up to ~100 lines. Beyond that, still recommend extraction.
+- `match` / `if-elif-else` / `switch` chains where each branch is a short value→action mapping (dispatch tables are inherently flat).
+- Functions whose length comes from a single long literal data structure.
+- JSX render bodies that are long but flat markup — flag only when logic (conditionals, mapping, state juggling) is interleaved with the markup and could move to hooks/helpers.
+
+**DO flag (dispatch-specific):** `match`/`elif`/`switch` chains with multi-statement branch bodies that do their own branching — the flatness exemption applies to dispatch tables, not chains of mini-functions in disguise.
+
+**Note:** It is acceptable to acknowledge complexity and defer refactoring by creating a beads ticket rather than fixing in the current PR. This applies to heuristic findings; objective floor violations should be resolved in-PR unless there's a documented reason.

.reviewers/credentials-hygiene-reviewer.md

@@ -0,0 +1,34 @@
+# credentials-hygiene-reviewer
+
+Review PRs for **secrets and credential handling**. This repo integrates with external services holding real credentials — Thingiverse (client id/secret, app token, JWT access/refresh tokens), the OpenForge API token, AWS, and Cloudflare R2. Credentials live in the environment (`~/.profile.d/`, gitignored `.env`) and must never enter the repo, its fixtures, its logs, or its test data.
+
+**What to flag:**
+
+1. **Literal secrets in code or config (P1):** any token, API key, JWT (`eyJ...`), password, client secret, or connection string with embedded credentials committed in source, JSON/YAML fixtures, test files, or docs. Includes "temporarily for testing" — a committed secret is compromised regardless of intent and requires rotation, not just removal (git history preserves it).
+
+2. **Captured traffic artifacts committed (P1):** HAR files, request/response dumps, `curl -v` transcripts, or debug captures containing `Authorization` headers, cookies, or tokens. **This project explicitly uses HAR captures of the Thingiverse SPA for API reverse-engineering — those files carry live JWTs, refresh tokens, and session cookies, and belong in the session scratchpad, never in the repo.** Flag any `*.har` or capture-shaped JSON in the diff, and check `.gitignore` covers the pattern.
+
+3. **Secrets in URLs (P2):** tokens as query parameters (`?access_token=...`, `?token=...`) in code when a header alternative exists. URLs land in server logs, browser history, and proxies. The Thingiverse v1 API accepts `?access_token=` — use the `Authorization: Bearer` header form instead. Runtime-only exceptions (an API that *requires* a URL token) must confine the URL construction to one place and never log the assembled URL.
+
+4. **Secrets echoed to output (P1):** tokens in exception messages, `print()` diagnostics, CLI output, or assertion messages. (Log-call interpolation is `logging-reviewer`'s beat — this covers the non-logging leak paths.) Also flag debug endpoints or CLI flags that dump full config including credentials.
+
+5. **Insecure storage of tokens the tool persists (P2):** the Thingiverse auth manager persists refresh tokens. Flag: tokens written into the repo tree, into fixture files, into world-readable paths, or into files not covered by `.gitignore`. Acceptable: env files outside the repo, `~/.config`/`~/.openforge`-style dotfiles with `0600`-style expectations, OS keyring.
+
+6. **Test fixtures with realistic-looking credentials (P3):** tests should use obviously-fake values (`"test-token"`, `"fake-client-id"`), not plausible or expired-real ones. An expired-real JWT in a test still reveals account ids, scopes, and endpoint shapes.
+
+7. **New env var credentials without documentation (P3):** a new required credential env var should be named in the relevant doc/README section (name only — never the value) so setup doesn't require reading source.
+
+**Do NOT flag:**
+
+- Reading credentials from `os.environ` / env files — that's the correct pattern.
+- Public identifiers that aren't secrets (thing ids, public URLs, usernames, R2 bucket names).
+- Example placeholders in docs (`<your-token-here>`, `sk-xxxx...`).
+- The word "token"/"secret" in variable names, comments, or docs — the *values* are the concern.
+
+**Review approach:**
+
+1. Scan added lines for high-entropy strings, `eyJ`-prefixed blobs, `Bearer <literal>`, `client_secret=<literal>`, connection strings with passwords.
+2. Check the diff file list for `*.har`, capture dumps, and env-file-shaped additions; verify `.gitignore` coverage for new artifact patterns the PR's tooling produces.
+3. For code that builds authenticated requests: header vs URL token placement; is the assembled URL ever logged/printed?
+4. For token persistence code: where does it write, and is that location inside the repo or gitignored?
+5. For tests/fixtures touching auth: are the values obviously fake?

.reviewers/dead-code-reviewer.md

@@ -0,0 +1,31 @@
+# dead-code-reviewer
+
+Review PRs for **dead code introduction**. `ruff` catches unused imports (`F401`) and unused variables (`F841`); this reviewer fills the gaps — public APIs, reflection-driven code, and partial refactors.
+
+**What to flag:**
+
+1. **Unused module-level functions, classes, constants** that nothing in the repo references.
+2. **Unused exported names** in a package's `__init__.py` that nothing imports (verify by grepping the whole workspace).
+3. **Unused fixtures in `conftest.py`** that no test references.
+4. **Commented-out code** — delete; git history preserves it.
+5. **Partial refactors** — old function name still defined after every call site moved to a new name.
+6. **Stale `__all__` entries** referring to names that no longer exist.
+7. **Unused parameters with default values** (especially after a refactor stopped passing them).
+8. **`if False:` / `if True:` dead branches** — refactor scaffolding; delete.
+9. **`def f(): pass` stubs with no implementation and no callers.**
+10. **Frontend equivalents:** unexported/unimported components, unused props threaded through components, dead CSS-module classes for removed markup, unused exported types.
+
+**Review approach:**
+
+1. For each new/modified file: did the PR remove call sites without removing the called function?
+2. For renamed/moved functions: is the old name still defined somewhere?
+3. For removed features: are all supporting helpers, constants, and types also removed?
+4. Grep the workspace for each flagged symbol to confirm it's truly unreferenced. Include the grep result in the comment so the author can verify.
+
+**Do NOT flag:**
+
+- **Reflection/convention-driven code**: Flask route functions (registered via decorator side-effects), pytest fixtures (discovered by name), Click/argparse callbacks, Next.js page/layout exports (`default`, `metadata`, `generateStaticParams`), React components referenced only in JSX.
+- Code referenced only via `getattr` / `hasattr` / dynamic import (search for the bare string, not just the symbol).
+- Public API surface with no internal callers — the catalog API is consumed by the static frontend and external scripts (`bin/upload_fixture`); verify against route registrations and frontend fetch calls before flagging.
+- Schema migration classes (`openforge/db/schema/version_NN.py`) — invoked by the migration runner via decorator registration, never imported directly.
+- Build-tag-gated or platform-specific code.

.reviewers/error-handling-reviewer.md

@@ -0,0 +1,86 @@
+# error-handling-reviewer
+
+Review Python code for **error handling correctness**. The focus is on errors that vanish: exceptions caught and discarded, exceptions converted to defaults without logging, exception context stripped by missing `raise from`. Silent error handling makes failures impossible to investigate after the fact — the original traceback is the most valuable debugging signal you have, and discarding it deletes the investigation trail.
+
+This matters doubly here: the backend is a single Lambda where you can't attach a debugger — CloudWatch logs and tracebacks are the only forensics available.
+
+**Patterns to FLAG:**
+
+1. **Silent exception swallowing — the most damaging pattern (P1):**
+
+   ```python
+   # BAD — exception silently discarded
+   try:
+       parse_data(html)
+   except Exception:
+       pass
+
+   # BAD — default return masks the failure
+   try:
+       return parse_data(html)
+   except Exception:
+       return {}
+   ```
+
+   If parsing fails, callers see `{}` and assume success. The original error never reaches the operator.
+
+2. **`return` statement inside a `finally` block (P1):** a `return` (or `raise`) in `finally` overrides any pending exception. Almost always a bug.
+
+3. **Generic `except Exception` without re-raise (P2):**
+
+   ```python
+   # BAD — catches everything, logs, continues silently
+   try:
+       do_complex_thing()
+   except Exception as e:
+       logger.warning(f"Error: {e}")
+       # implicit None return
+   ```
+
+   Either re-raise after logging, or document why a sentinel return is correct. In Flask routes, prefer letting the error propagate to an error handler that returns a proper 5xx over returning a fake-success payload.
+
+4. **Bare `except:` (catches `BaseException`) (P2):** use `except Exception:` at minimum — bare `except` turns Ctrl-C and `sys.exit()` into silent no-ops.
+
+5. **Missing exception chaining (`raise ... from`) (P3):**
+
+   ```python
+   # GOOD — preserves the cause for the traceback
+   try:
+       value = int(text)
+   except ValueError as e:
+       raise ParseError(f"invalid number: {text!r}") from e
+   ```
+
+   Use `raise ... from None` only when deliberately suppressing the cause is correct (rare).
+
+6. **Custom exception classes for caller-handleable cases (P3):** errors callers branch on programmatically (not-found, already-exists, validation-failure) should be classes inheriting from a meaningful base — flag ad-hoc `raise ValueError("not found")` where the caller clearly needs to detect the case but can't.
+
+7. **`try/finally` for cleanup when a context manager would do (P3):** `with open(path) as f:` over manual `f.close()` in `finally`.
+
+8. **`assert` used for runtime validation (P2):** asserts are stripped under `python -O`. Use real validation (`if x is None: raise ValueError(...)`) for user-facing or API-input checks; `assert` is for internal invariants only.
+
+**Acceptable patterns:**
+
+- Assertions with context for internal invariants: `assert len(children) == 2, f"expected 2, got {len(children)}"`.
+- Specific exception handling with `raise ... from`.
+- Known-case handling with an explicit comment:
+
+  ```python
+  try:
+      score = int(stat.strip())
+  except ValueError:
+      # Known case: legacy fixture rows use "-" for missing values.
+      score = None
+  ```
+
+- Bare `except` + `raise` for cleanup (re-raises the original).
+- `contextlib.suppress(FileNotFoundError)` for genuinely-ignorable cases — intent is explicit.
+
+**Review approach:**
+
+1. Grep for `except` patterns; for each, confirm the handler either logs AND re-raises, returns the correct value for a documented case (with comment), or has another defensible justification.
+2. Grep for `pass` immediately after `except`. Flag as P1.
+3. Grep for `return` inside `finally`. Flag as P1.
+4. Grep for `try` blocks that could be `with` statements.
+5. Grep for `raise X(...)` after `except`; verify `from e` (or justified `from None`).
+6. For ad-hoc `raise ValueError(...)` where callers need to detect the case, suggest a custom exception class.

.reviewers/frontend-conventions-reviewer.md

@@ -0,0 +1,38 @@
+# frontend-conventions-reviewer
+
+Review TypeScript/React changes (`src/**/*.ts`, `src/**/*.tsx`) for this repo's frontend conventions. The frontend is a **Next.js app compiled to a static export** served from S3 — there is no Node server at runtime, and the API is a separate Flask Lambda reached through `/api`.
+
+**What to flag:**
+
+1. **Hardcoded API base URLs (P1):** CLAUDE.md hard rule — *"always make a relative call to /api. Never encode the base of the url."* Flag any fetch/axios call with `http://`, `https://`, `localhost`, a port number, or an environment-derived base URL prepended to an API path. The correct form is `fetch("/api/blueprints/...")`.
+
+2. **Static-export violations (P1):** anything that requires a server at runtime:
+   - API routes (`app/api/**/route.ts`, `pages/api/**`)
+   - Server actions, SSR data fetching that can't run at build time
+   - `next/image` with the default optimizing loader
+   - Middleware, `headers()`/`cookies()` server functions in runtime paths
+   - Dynamic routes without `generateStaticParams`
+
+3. **Component responsibility (P2):** per CLAUDE.md, components manage their state and delegate event handling to **custom hooks**. Flag components that accumulate state + rendering + keyboard events + drag handling in one body — extract hooks (`useSpriteViewer`, `useDragSelection`) or helper functions. (Complexity thresholds live in `complexity-reviewer`; this reviewer flags the *pattern* — logic that belongs in a hook living inline in a component.)
+
+4. **Type discipline (P2):** new `any` (explicit or via untyped boundaries), `as unknown as X` double-casts, `@ts-ignore`/`@ts-expect-error` without a comment explaining why, `!` non-null assertions where a runtime check is warranted. `npm run type-check` must pass — but these patterns pass the checker while defeating it.
+
+5. **State anti-patterns (P3):** derived state stored in `useState` + synced with `useEffect` (compute it during render or `useMemo`); `useEffect` with missing/over-broad dependencies as a data-flow mechanism; prop drilling through 3+ layers where the existing context/patterns in `src/` offer a home. (Derived-state and deps findings are shared ground with `react-hooks-reviewer` — post under whichever reviewer found it first, don't double-post.)
+
+6. **Data fetching in render paths without cancellation/guards (P2):** fetches in `useEffect` that set state after unmount, missing loading/error states for user-visible data, refetching on every render due to unstable dependencies.
+
+7. **Duplicating utilities that exist in `src/utils/` (P3):** tag parsing, blueprint helpers, clipboard, config processing already have tested homes — grep `src/utils/` before accepting a new inline implementation.
+
+**Do NOT flag:**
+
+- Build-time data fetching that static export supports.
+- `any` in existing code the PR merely brushes against (pre-existing debt — beads ticket at most).
+- Small components keeping trivial handlers inline — hook extraction is for meaningful logic, not `onClick={() => setOpen(true)}`.
+
+**Review approach:**
+
+1. Grep the diff for `http://`, `https://`, `localhost`, `process.env.*URL` in fetch paths.
+2. Check new files/routes against the static-export constraint list.
+3. For each component touched: is new stateful/event logic inline where a hook should be?
+4. Grep for `any`, `@ts-ignore`, `as unknown`, `!` assertions in added lines.
+5. Cross-check new utility-shaped code against `src/utils/`.