From 8e6efb0f14a30021e859d81631572bd7e4116799 Mon Sep 17 00:00:00 2001 From: Devon Jones Date: Mon, 6 Jul 2026 12:36:44 -0600 Subject: [PATCH 1/3] feat: add agent reviewer pack for pr-review-loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root AGENT-REVIEWERS.md indexing 11 reviewers with spawn conditions, severity conventions (P1/P2/P3 mapped to beads), output format, and deferral rules. Specs in .reviewers/: 7 adapted from 521-studios/wyrd (complexity, error-handling, test-coverage, resource-leak, dead-code, logging, importlib-resources) and 4 project-specific (migration- discipline, serverless-architecture, frontend-conventions, credentials-hygiene). Adds *.har to .gitignore since upcoming Thingiverse work uses HAR captures that carry live credentials. Closes openforge_catalog-1v5 Co-Authored-By: Claude Fable 5 πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .beads/issues.jsonl | 10 + .beads/last-touched | 1 + .gitignore | 4 + .reviewers/complexity-reviewer.md | 50 +++++ .reviewers/credentials-hygiene-reviewer.md | 34 +++ .reviewers/dead-code-reviewer.md | 31 +++ .reviewers/error-handling-reviewer.md | 86 ++++++++ .reviewers/frontend-conventions-reviewer.md | 38 ++++ .reviewers/importlib-resources-reviewer.md | 33 +++ .reviewers/logging-reviewer.md | 59 +++++ .reviewers/migration-discipline-reviewer.md | 47 ++++ .reviewers/resource-leak-reviewer.md | 56 +++++ .../serverless-architecture-reviewer.md | 35 +++ .reviewers/test-coverage-reviewer.md | 72 +++++++ AGENT-REVIEWERS.md | 202 ++++++++++++++++++ 15 files changed, 758 insertions(+) create mode 100644 .beads/last-touched create mode 100644 .reviewers/complexity-reviewer.md create mode 100644 .reviewers/credentials-hygiene-reviewer.md create mode 100644 .reviewers/dead-code-reviewer.md create mode 100644 .reviewers/error-handling-reviewer.md create mode 100644 .reviewers/frontend-conventions-reviewer.md create mode 100644 .reviewers/importlib-resources-reviewer.md create mode 100644 .reviewers/logging-reviewer.md create mode 100644 .reviewers/migration-discipline-reviewer.md create mode 100644 .reviewers/resource-leak-reviewer.md create mode 100644 .reviewers/serverless-architecture-reviewer.md create mode 100644 .reviewers/test-coverage-reviewer.md create mode 100644 AGENT-REVIEWERS.md diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index b375fe19..3311dcbb 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,13 +1,23 @@ +{"id":"openforge_catalog-1v5","title":"Agent reviewer pack: AGENT-REVIEWERS.md + .reviewers/ specs","description":"Create the pr-review-loop agent reviewer pack for openforge-catalog, modeled on 521-studios/wyrd's. Root AGENT-REVIEWERS.md (index: what each reviewer checks / when to spawn / severity convention / output format / beads deferral) + 11 specs under .reviewers/: 7 adapted from wyrd (complexity, error-handling, test-coverage, resource-leak, dead-code, logging, importlib-resources) and 4 project-specific (migration-discipline, serverless-architecture, frontend-conventions, credentials-hygiene). Also adds *.har to .gitignore (HAR captures carry live credentials). Prerequisite for running pr-review-loop on all Thingiverse epic PRs.","status":"in_progress","priority":1,"issue_type":"task","owner":"devon.jones@gmail.com","created_at":"2026-07-06T12:35:21.661801834-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T12:35:22.3405376-06:00"} {"id":"openforge_catalog-3cx","title":"Frontend: Interactive thumbnail viewer component","description":"Build React component to display sprite sheet thumbnails with interactive controls. Uses CSS background-position or canvas to show specific tile from sprite sheet grid. Supports: (1) Drag-to-rotate - maps drag distance to grid positions. (2) Keyboard navigation - arrow keys step through angles. (3) Smooth transitions between angles. (4) Current angle indicator. Fetches sprite sheet metadata from API. Falls back gracefully for legacy single thumbnails.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:40.821869557-07:00","updated_at":"2025-12-21T10:06:05.817382285-07:00","dependencies":[{"issue_id":"openforge_catalog-3cx","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:19.396687592-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-3cx","depends_on_id":"openforge_catalog-5tg","type":"blocks","created_at":"2025-12-21T09:59:36.157360762-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-4ji","title":"CLI tool: Generate sprite sheet for single STL file","description":"Create standalone script/CLI command to generate multi-angle sprite sheet for a single STL file. Usage: './generate_sprite.py path/to/model.stl' or 'bd-sprite --file model.stl'. Generates 10 isometric angles using stl-thumb, combines into sprite sheet, optionally uploads to R2 and updates database. Allows testing camera positions, grid layout, and image quality on individual files before bulk processing. Include --dry-run mode to preview without saving.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T10:13:53.370535751-07:00","updated_at":"2025-12-21T13:24:51.337669635-07:00","closed_at":"2025-12-21T13:24:51.337669635-07:00","close_reason":"Completed: Created bin/generate_sprite CLI tool for generating sprite sheets from single STL files. Features: --upload for R2 upload, --output-json for fixture integration, --dry-run for testing, --verbose for debugging. Supports tile size customization. Cleans up local files after upload. All 319 tests passing.","dependencies":[{"issue_id":"openforge_catalog-4ji","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T10:14:18.362448248-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-4ji","depends_on_id":"openforge_catalog-ubp","type":"blocks","created_at":"2025-12-21T10:14:23.25273996-07:00","created_by":"daemon"}]} +{"id":"openforge_catalog-4kx","title":"Epic: Thingiverse publish \u0026 sync tool","description":"Build a Python CLI in openforge/ that (1) creates new Thingiverse things pre-populated from catalog blueprint data with per-thing manifest overrides, and (2) keeps created things in sync with a directed set of files on subsequent runs. Uses the v2 JWT API (personal account via /v2/auth/login, NOT the v1 Joaquin app token). Write endpoints (create thing, upload STL, file hashes) are absent from the public OpenAPI spec β€” contract will be reverse-engineered from a HAR capture of the SPA. Sync state lives in catalog Postgres; diff engine keyed on file hashes (algorithm TBD, likely SHA-256 per Devon; catalog currently stores MD5).","notes":"PRIOR ART: ~/Dropbox/projects/Hardware/objects/OpenForgeDevelopment/openforge β€” Devon's old Python2 v1 integration. thingiverse/things.py (small requests client: create_thing POST /things/, Thing.update PATCH, get_my_things with Link-header pagination), bin/tv_* scripts, bin/new.*.json templates. The v1 create payload was {name, license, category, tags[], description} with human-readable license/category strings. Concepts to carry forward, not code (py2, has_key, kconfig).","status":"open","priority":1,"issue_type":"epic","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:41:52.221094986-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T12:00:33.405724586-06:00"} {"id":"openforge_catalog-5gb","title":"Frontend: Admin camera position controls UI","description":"Build admin interface to preview and set default camera angles for models. Shows live preview of each angle, allows selecting which is default. Integrates with admin set camera position API. Consider batch operations for setting defaults across similar models.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:45.023938263-07:00","updated_at":"2025-12-21T09:58:45.023938263-07:00","dependencies":[{"issue_id":"openforge_catalog-5gb","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:19.862145404-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-5gb","depends_on_id":"openforge_catalog-hqe","type":"blocks","created_at":"2025-12-21T09:59:36.561390068-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-5na","title":"Phased rollout: Test subset before bulk backfill","description":"Implement safe migration strategy: (1) Generate sprite sheets for small test set (~10-20 diverse models). (2) Deploy frontend with fallback to legacy single thumbnails. (3) Validate with users - test interactive controls, image quality, performance. (4) Iterate on camera positions/layout based on feedback. (5) Only after validation, run bulk backfill for all 8000+ STL files. Include ability to pause/resume bulk processing. Track which models have sprite sheets vs legacy thumbnails.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-21T10:13:57.517770238-07:00","updated_at":"2025-12-21T10:13:57.517770238-07:00","dependencies":[{"issue_id":"openforge_catalog-5na","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T10:14:18.760726353-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-5na","depends_on_id":"openforge_catalog-4ji","type":"blocks","created_at":"2025-12-21T10:14:23.673079682-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-5na","depends_on_id":"openforge_catalog-3cx","type":"blocks","created_at":"2025-12-21T10:14:24.071569003-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-5na","depends_on_id":"openforge_catalog-dd4","type":"blocks","created_at":"2025-12-21T10:14:24.494018624-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-5tg","title":"Backend API: Get thumbnail variants endpoint","description":"Create Flask endpoint to return sprite sheet info for a model. Returns: (1) Sprite sheet URL, (2) Grid layout (rows, cols, tile dimensions), (3) Angle metadata (name, position in grid, camera coords), (4) Default angle index. Frontend uses this to display correct region from sprite sheet. Handles models with legacy single thumbnail gracefully.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:32.287223899-07:00","updated_at":"2025-12-21T14:00:25.231595972-07:00","closed_at":"2025-12-21T14:00:25.231595972-07:00","close_reason":"Completed: Created GET /api/blueprints/\u003cblueprint_id\u003e/thumbnail-variants endpoint. Returns sprite sheet URL, grid layout (rows/cols/tile_size), angle metadata with camera positions, and default angle index. Gracefully handles legacy single thumbnails. All 319 tests passing.","dependencies":[{"issue_id":"openforge_catalog-5tg","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:18.52350897-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-5tg","depends_on_id":"openforge_catalog-ubp","type":"blocks","created_at":"2025-12-21T09:59:35.26824895-07:00","created_by":"daemon"}]} +{"id":"openforge_catalog-750","title":"Postgres migration: thingiverse sync-state tables","description":"New schema version adding: thingiverse_thing (blueprint_id \u003c-\u003e thing_id, status, published_at, last_synced_at) and thingiverse_file (thing ref, catalog file ref + local hash, remote file id + remote hash, last_synced_at) β€” the diff ledger for the sync engine. Follow existing openforge/db/schema/version_NN.py migration pattern. If the HAR ticket proves the remote hash is SHA-256 (catalog stores MD5), include storage for the second hash algorithm.","notes":"SCOPE NOTES: (1) thingiverse_file must carry a file-type/role discriminator (model/image/zip/other) since things contain mixed file types. (2) This runs against the LOCAL db; prod gets a subset via the fixture path (see openforge_catalog-8yb) β€” keep the mapping (blueprint_id, thing_id, public_url) cleanly separable from local-only operational state (hash ledger, sync timestamps).","status":"open","priority":2,"issue_type":"task","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:25.104764326-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T11:55:49.425508213-06:00","dependencies":[{"issue_id":"openforge_catalog-750","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:52.476138903-06:00","created_by":"Devon Jones"}]} +{"id":"openforge_catalog-7dg","title":"Auth/token manager for Thingiverse v2 (login + refresh)","description":"Python module: interactive 'login' does POST /v2/auth/login {usernameOrEmail, password} -\u003e AuthTokensResponse (handle 202 2FA via /v2/auth/2fa/login), persists refresh token securely (env file/keyring, never in repo); runtime path auto-refreshes access JWT via POST /v2/auth/refresh. Password is never stored. Buildable and testable today against the live API β€” verify a Devon-scoped JWT via GET /v2/users/me. Note: THINGIVERSE_APP_TOKEN in ~/.profile.d/099.api_keys.sh is v1-only and scoped to the wrong account (Joaquin Munguia, id 5577639) β€” do not use.","status":"open","priority":1,"issue_type":"feature","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:24.708129858-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T11:42:24.708129858-06:00","dependencies":[{"issue_id":"openforge_catalog-7dg","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:52.071225891-06:00","created_by":"Devon Jones"}]} +{"id":"openforge_catalog-8yb","title":"Propagate thingiverse sync data to AWS prod (fixture + API upsert path)","description":"The tool runs against the LOCAL Postgres, but prod (AWS Lambda + serverless Postgres) is populated by replaying fixture JSON through the Flask API (bin/upload_fixture with OPENFORGE_BASE_URL, same pattern as blueprint fixtures). Thingiverse data (blueprint_id \u003c-\u003e thing_id mapping, public thing URLs) must ride that path so the app can render Thingiverse links in prod and support deeper integration later. Deliverables: fixture format for thingiverse mappings (exported from the local sync tables), an authenticated API upsert endpoint in the Flask app, and upload_fixture support. Note: prod likely only needs the mapping/links, not the per-file hash ledger β€” that is local operational state; decide the split when designing the fixture.","status":"open","priority":2,"issue_type":"feature","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:55:33.474857151-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T11:55:33.474857151-06:00","dependencies":[{"issue_id":"openforge_catalog-8yb","depends_on_id":"openforge_catalog-750","type":"blocks","created_at":"2026-07-06T11:55:47.512062985-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-8yb","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:55:47.882392741-06:00","created_by":"Devon Jones"}]} +{"id":"openforge_catalog-ad5","title":"Sync engine: diff local files vs remote hashes, reconcile","description":"For each managed thing: diff manifest-directed local files against remote files using hash comparison (via thingiverse_file ledger) -\u003e upload new, replace changed, delete removed, patch drifted metadata. Idempotent; dry-run mode showing planned actions before mutating. Depends on hash-algorithm verification: if remote is SHA-256, compute+store SHA-256 alongside catalog MD5s.","notes":"PRIOR ART (old v1 tool): tv_update_description did bulk find/replace of boilerplate across all things matching a name filter, with --dry-run (desc_update.json shows real mass migrations happened). tv_tag_dl mirrored all things to local JSON with 3s sleeps (self-rate-limiting; v1 had practical rate limits). Lessons: (1) metadata drift detection should cover the shared-boilerplate case so a boilerplate change re-syncs every managed thing's description; (2) throttle API calls; (3) dry-run is a first-class requirement, already in scope.","status":"open","priority":1,"issue_type":"feature","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:26.401028262-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T12:00:34.191543122-06:00","dependencies":[{"issue_id":"openforge_catalog-ad5","depends_on_id":"openforge_catalog-gn2","type":"blocks","created_at":"2026-07-06T11:42:43.166238543-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-ad5","depends_on_id":"openforge_catalog-750","type":"blocks","created_at":"2026-07-06T11:42:43.606420026-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-ad5","depends_on_id":"openforge_catalog-d7a","type":"blocks","created_at":"2026-07-06T11:42:44.00974978-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-ad5","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:53.860069002-06:00","created_by":"Devon Jones"}]} {"id":"openforge_catalog-bll","title":"Multi-Angle Thumbnail Viewing System","description":"Enable users to view STL models from 10 isometric camera angles with interactive controls. Admins can set default viewing angles per model. System generates 10 isometric-perspective angles (6 primary directions + 4 diagonal corners) during file scan using stl-thumb --cam-pos, combined into single sprite sheet PNG. Frontend has drag-to-rotate and keyboard navigation. All angles maintain 3/4 view for depth, not flat orthographic.","status":"open","priority":1,"issue_type":"epic","created_at":"2025-12-21T09:57:50.607001989-07:00","updated_at":"2025-12-21T10:09:20.457266019-07:00"} +{"id":"openforge_catalog-d7a","title":"Manifest schema + metadata assembler","description":"Define per-thing manifest format (YAML): explicit file list for the thing plus field overrides. Assembler builds the thing payload with defaults pulled from catalog Postgres blueprint records (name, description, tag hierarchy -\u003e Thingiverse tags, category, license, images) and applies manifest overrides on top. Zero-manual-entry defaults, curated where it matters. Payload field names/shape finalized against the HAR contract.","notes":"PRIOR ART (old v1 tool): bin/new.*.json templates were per-product-line defaults (openforge2, openlock, construction_kit, miniature, tools): license, category, tags, boilerplate description. Key design lesson: descriptions = per-thing content + SHARED BOILERPLATE FOOTER (Patreon/tutorials/catalog links) that changes over time. Model boilerplate as a shared block composed at assembly time (stored once, referenced by manifests), not baked into each manifest β€” enables bulk boilerplate updates via re-sync. Old templates: ~/Dropbox/projects/Hardware/objects/OpenForgeDevelopment/openforge/bin/new.*.json","status":"open","priority":2,"issue_type":"feature","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:25.496132115-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T12:00:33.768875881-06:00","dependencies":[{"issue_id":"openforge_catalog-d7a","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:52.892986073-06:00","created_by":"Devon Jones"}]} {"id":"openforge_catalog-dd4","title":"Integration: Wire scanner pipeline for multi-angle generation","description":"Wire multi-angle sprite sheet generation into file scanning pipeline, BUT ONLY AFTER phased rollout validation. Scanner should: (1) Support both legacy single thumbnails AND new sprite sheets during transition. (2) Have feature flag to enable/disable sprite generation. (3) Track which models have sprites vs legacy. (4) Handle failures gracefully - don't block scan if sprite generation fails. (5) Be pausable/resumable for bulk backfill. Only enable bulk processing after small test set is validated with users.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:49.223198843-07:00","updated_at":"2025-12-21T14:35:20.810908751-07:00","closed_at":"2025-12-21T14:35:20.810908751-07:00","close_reason":"Completed: Integrated sprite sheet generation into scanner pipeline with feature flag (ENABLE_SPRITE_THUMBNAILS). Created create_and_upload_thumbnail() helper that handles both sprite sheets and legacy thumbnails. Updated scanner.py and incremental.py to use new function. Gracefully handles failures without blocking scans. Supports transition period with both formats. All 319 tests passing.","dependencies":[{"issue_id":"openforge_catalog-dd4","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:20.290625315-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-dd4","depends_on_id":"openforge_catalog-ubp","type":"blocks","created_at":"2025-12-21T09:59:37.011721674-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-dd4","depends_on_id":"openforge_catalog-5tg","type":"blocks","created_at":"2025-12-21T09:59:37.450897079-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-dd4","depends_on_id":"openforge_catalog-hqe","type":"blocks","created_at":"2025-12-21T09:59:37.840969584-07:00","created_by":"daemon"}]} +{"id":"openforge_catalog-gn2","title":"Thingiverse v2 API client (write endpoints from HAR contract)","description":"Python client wrapping the v2 API using the auth/token manager: create thing, patch metadata, upload model file, replace file, delete file, list files w/ hashes, publish. Read endpoints (things/{id}/complete etc.) from the public spec (docs/thingiverse-api-v2.md); write endpoints implemented per the HAR-derived contract. Stub the write calls if started before the HAR ticket completes.","status":"open","priority":1,"issue_type":"feature","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:25.975705436-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T11:42:25.975705436-06:00","dependencies":[{"issue_id":"openforge_catalog-gn2","depends_on_id":"openforge_catalog-hnr","type":"blocks","created_at":"2026-07-06T11:42:42.234078586-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-gn2","depends_on_id":"openforge_catalog-7dg","type":"blocks","created_at":"2026-07-06T11:42:42.679151017-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-gn2","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:53.406992128-06:00","created_by":"Devon Jones"}]} +{"id":"openforge_catalog-hnr","title":"Capture HAR of Thingiverse SPA create/upload/edit flow and extract API contract","description":"Devon captures a HAR (Network tab, preserve log, Fetch/XHR, 'Save all as HAR with content') of a complete flow: create draft thing, set name/description/tags/category/license, upload image, upload STL, replace the STL, delete a file, publish. HAR goes to scratchpad only β€” contains live JWT/refresh/cookies, never commit; rotate tokens after. From the HAR, extract and document: create-thing endpoint+payload, model-file upload mechanism (direct POST vs presigned URL β€” matters for Lambda), file object shape incl. hash field, metadata patch, and file delete. Also verify hash algorithm: hash the exact uploaded STL locally with MD5 and SHA-256 and match against the hash Thingiverse reports (Devon guesses SHA-256). Output: docs/thingiverse-api-v2-private.md contract doc.","notes":"SCOPE ADDITION: things will contain photos, zip files, and other non-STL files. During the HAR capture, upload at least one photo, one zip, and one STL, and capture whether they all go through the same upload endpoint/mechanism or different ones (the public spec only shows an image upload route). Verify whether the hash field is reported for ALL file types or only models.","status":"open","priority":1,"issue_type":"task","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:24.225732535-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T11:55:48.284424468-06:00","dependencies":[{"issue_id":"openforge_catalog-hnr","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:51.659042706-06:00","created_by":"Devon Jones"}]} {"id":"openforge_catalog-hqe","title":"Backend API: Admin set default camera position","description":"Create Flask endpoint for admins to set/update the default camera position for individual models. Accepts model_id and camera position (x,y,z). Requires admin authentication. Triggers thumbnail regeneration if needed.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:36.544944445-07:00","updated_at":"2025-12-21T14:01:55.202764217-07:00","closed_at":"2025-12-21T14:01:55.202764217-07:00","close_reason":"Completed: Created PATCH /api/blueprints/\u003cblueprint_id\u003e/thumbnail-variants/default-angle endpoint. Admin-only, requires authentication and CSRF protection. Validates angle index (0-9), updates sprite_metadata.default_angle in database. Returns error for legacy single thumbnails. All 319 tests passing.","dependencies":[{"issue_id":"openforge_catalog-hqe","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:18.965408581-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-hqe","depends_on_id":"openforge_catalog-jnl","type":"blocks","created_at":"2025-12-21T09:59:35.746577657-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-hr4","title":"Admin UI: Manually trigger sprite generation per model","description":"Add admin interface to manually trigger sprite sheet generation for individual models. Useful for: (1) Testing different camera positions on specific models. (2) Regenerating sprites after adjusting defaults. (3) Fixing failed generations. Shows progress, preview of result, ability to approve/reject before replacing existing thumbnail. Can mark models as 'sprite sheet validated' to track rollout progress.","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-21T10:14:01.648534686-07:00","updated_at":"2025-12-21T10:14:01.648534686-07:00","dependencies":[{"issue_id":"openforge_catalog-hr4","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T10:14:19.184588659-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-hr4","depends_on_id":"openforge_catalog-4ji","type":"blocks","created_at":"2025-12-21T10:14:24.909562746-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-jnl","title":"Database schema for camera positions and thumbnail variants","description":"Add database schema to store: (1) Sprite sheet metadata - grid dimensions (rows, cols), tile size, angle positions in grid. (2) Camera position per angle (x,y,z coords). (3) Default angle index per model. (4) Sprite sheet file path/URL in R2. Consider if this extends existing thumbnail tables or needs new camera_angles table. Must support models with legacy single thumbnail during migration.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:19.570114017-07:00","updated_at":"2025-12-21T13:07:55.263321459-07:00","closed_at":"2025-12-21T13:07:55.263321459-07:00","close_reason":"Completed: Added sprite_metadata JSONB column to images table with GIN index. Migration version 16 created and tested. All tests passing.","dependencies":[{"issue_id":"openforge_catalog-jnl","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:17.171165836-07:00","created_by":"daemon"}]} +{"id":"openforge_catalog-lpe","title":"CLI commands: thingiverse create/sync/status","description":"CLI surface in openforge/: 'thingiverse login' (auth manager), 'thingiverse create \u003cmanifest\u003e' (create + populate + upload files + record in sync tables), 'thingiverse sync [thing|--all]' (run sync engine), 'thingiverse status' (show managed things, pending diffs, last sync). Follow existing openforge CLI patterns.","status":"open","priority":2,"issue_type":"feature","owner":"devon.jones@gmail.com","created_at":"2026-07-06T11:42:26.871182106-06:00","created_by":"Devon Jones","updated_at":"2026-07-06T11:42:26.871182106-06:00","dependencies":[{"issue_id":"openforge_catalog-lpe","depends_on_id":"openforge_catalog-ad5","type":"blocks","created_at":"2026-07-06T11:42:44.422774979-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-lpe","depends_on_id":"openforge_catalog-d7a","type":"blocks","created_at":"2026-07-06T11:42:44.898169896-06:00","created_by":"Devon Jones"},{"issue_id":"openforge_catalog-lpe","depends_on_id":"openforge_catalog-4kx","type":"parent-child","created_at":"2026-07-06T11:42:54.325118338-06:00","created_by":"Devon Jones"}]} {"id":"openforge_catalog-qjn","title":"Testing and documentation for multi-angle thumbnails","description":"Write tests: backend API tests, thumbnail generation tests, frontend component tests. Document: camera angle coordinate system, API endpoints, how to add new angles, admin workflow for setting defaults. Migration guide for existing thumbnails.","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-21T09:58:53.460986877-07:00","updated_at":"2025-12-21T09:58:53.460986877-07:00","dependencies":[{"issue_id":"openforge_catalog-qjn","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:20.738830826-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-qjn","depends_on_id":"openforge_catalog-dd4","type":"blocks","created_at":"2025-12-21T09:59:38.217939989-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-szh","title":"Design R2 storage structure for multi-angle thumbnails","description":"Define R2 storage using SPRITE SHEET approach: Generate single PNG containing all 10 isometric-perspective angles in a grid layout (5x2 or 2x5 grid). Angles: 6 primary isometric views (front-iso, back-iso, left-iso, right-iso, top-iso, bottom-iso) + 4 diagonal corners (NE, NW, SE, SW). All maintain 3/4 perspective for depth, not flat orthographic. File naming: {model_id}_angles.png or {md5}_angles.png. Each angle occupies equal-sized tile. Store grid layout metadata in database. One file per model instead of 10 separate files.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:23.809557729-07:00","updated_at":"2025-12-21T13:09:23.10795275-07:00","closed_at":"2025-12-21T13:09:23.10795275-07:00","close_reason":"Completed: Created comprehensive design document (docs/sprite-sheet-storage-design.md) specifying R2 storage structure, sprite layout (2x5 grid, 512x512 tiles), 10 camera angles, path structure (sprites/{md5[:6]}/{md5}.png), and integration strategy.","dependencies":[{"issue_id":"openforge_catalog-szh","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:17.627098048-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-szh","depends_on_id":"openforge_catalog-jnl","type":"blocks","created_at":"2025-12-21T09:59:34.44071374-07:00","created_by":"daemon"}]} {"id":"openforge_catalog-ubp","title":"Update stl-thumb integration to generate 8+ camera angles","description":"Enhance thumbnail generation to: (1) Use 'stl-thumb --cam-pos x y z' to generate 10 isometric angles maintaining the DEFAULT STYLE: camera at distance ~4.9 from origin, elevation ~24Β°, same visual perspective as default (2.0, -4.0, 2.0). (2) 10 angles by rotating camera around model: Front-iso (default: 2,-4,2), Back-iso (~-2,4,2), Left-iso (~-4,-2,2), Right-iso (~4,2,2), Top-iso (higher z, maintain angle), Bottom-iso (lower z), plus 4 diagonal corners (NE, NW, SE, SW at ~4,Β±2,2 variants). FOV auto-adjusts. (3) Combine all 10 into SPRITE SHEET PNG using PIL/Pillow (5x2 grid). (4) Store camera coords and grid metadata. Generate during file scan.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-21T09:58:27.974391955-07:00","updated_at":"2025-12-21T13:22:04.806661311-07:00","closed_at":"2025-12-21T13:22:04.806661311-07:00","close_reason":"Completed: Added sprite sheet generation functions to openforge/data/io.py. Implemented create_sprite_sheet() with 10 isometric camera angles (6 primary + 4 diagonal), PIL/Pillow integration for sprite combining (2x5 grid, 512x512 tiles), and sprite_metadata generation. Added Pillow==11.1.0 to requirements. All 319 tests passing.","dependencies":[{"issue_id":"openforge_catalog-ubp","depends_on_id":"openforge_catalog-bll","type":"parent-child","created_at":"2025-12-21T09:59:18.076987259-07:00","created_by":"daemon"},{"issue_id":"openforge_catalog-ubp","depends_on_id":"openforge_catalog-szh","type":"blocks","created_at":"2025-12-21T09:59:34.873857445-07:00","created_by":"daemon"}]} diff --git a/.beads/last-touched b/.beads/last-touched new file mode 100644 index 00000000..4dfac3ab --- /dev/null +++ b/.beads/last-touched @@ -0,0 +1 @@ +openforge_catalog-1v5 diff --git a/.gitignore b/.gitignore index 93c43a69..b92ddf86 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,10 @@ scratch .env + +# traffic captures β€” carry live credentials (JWTs, cookies); never commit +*.har + # debug npm-debug.log* yarn-debug.log* diff --git a/.reviewers/complexity-reviewer.md b/.reviewers/complexity-reviewer.md new file mode 100644 index 00000000..ae06c749 --- /dev/null +++ b/.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 --max-complexity 10` against the PR head (never a stale local checkout) and flag every function that exceeds it. 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. diff --git a/.reviewers/credentials-hygiene-reviewer.md b/.reviewers/credentials-hygiene-reviewer.md new file mode 100644 index 00000000..b76806d1 --- /dev/null +++ b/.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 (``, `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 `, `client_secret=`, 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? diff --git a/.reviewers/dead-code-reviewer.md b/.reviewers/dead-code-reviewer.md new file mode 100644 index 00000000..03a0a16d --- /dev/null +++ b/.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. diff --git a/.reviewers/error-handling-reviewer.md b/.reviewers/error-handling-reviewer.md new file mode 100644 index 00000000..2e38ea85 --- /dev/null +++ b/.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. diff --git a/.reviewers/frontend-conventions-reviewer.md b/.reviewers/frontend-conventions-reviewer.md new file mode 100644 index 00000000..8355628b --- /dev/null +++ b/.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. + +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/`. diff --git a/.reviewers/importlib-resources-reviewer.md b/.reviewers/importlib-resources-reviewer.md new file mode 100644 index 00000000..4aac64e5 --- /dev/null +++ b/.reviewers/importlib-resources-reviewer.md @@ -0,0 +1,33 @@ +# importlib-resources-reviewer + +Review Python code for **correct package data access**. Package data files (fixture JSON/YAML, SQL, templates) must be loaded via `importlib.resources`, not `Path(__file__).parent`. The `__file__` pattern silently breaks when a module moves β€” and **breaks outright in the Lambda zip deployment**, where the package layout differs from the dev checkout. + +This repo already follows the correct convention β€” see `openforge/db/fixtures/__init__.py` (`from importlib import resources as impresources`). New code must match it. + +**FLAG when a file in the `openforge/` package contains:** + +- `Path(__file__).parent / "data"` (or any path navigation from `__file__`) used to locate bundled data +- `os.path.dirname(__file__)` for the same purpose +- `__file__.parents[N]` for sibling resources + +**Acceptable pattern:** + +```python +from importlib import resources as impresources + +data_file = impresources.files("openforge.db.fixtures").joinpath("blueprints/cave.json") +data = json.loads(data_file.read_text()) +``` + +**Do NOT flag:** + +- `__file__` used for **write paths** (tests writing next to themselves, scripts emitting output) β€” resources are read-only by definition. +- `__file__` references in `tests/` / `integration_tests/` β€” tests aren't packaged. +- `__file__` in entry-point scripts under `bin/` that are not part of the package surface. +- Runtime data paths that are *deliberately external* to the package (Dropbox scan roots, `~/.openforge`-style config) β€” those are configuration, not package data. + +**Review approach:** + +1. Grep the PR diff for `Path(__file__)`, `dirname(__file__)`, `__file__.parents`. +2. For each hit in package code, check whether the path resolves to a bundled data file. If so, recommend the `impresources` form used by `openforge/db/fixtures/`. +3. For each new package data file added under `openforge/`, verify it will ship in the Lambda package (declared in the packaging config / not excluded), otherwise it 404s only in production. diff --git a/.reviewers/logging-reviewer.md b/.reviewers/logging-reviewer.md new file mode 100644 index 00000000..adbc5d40 --- /dev/null +++ b/.reviewers/logging-reviewer.md @@ -0,0 +1,59 @@ +# logging-reviewer + +Review Python code for **logging discipline**. Logging is observability infrastructure β€” it determines what operators see in CloudWatch when the Lambda misbehaves β€” and it's a security boundary (logs are shipped off-host and indexed by tools that don't redact secrets). Two contexts with different rules: + +- **The Flask app and library code** (`openforge/`) β€” logs ship to CloudWatch. +- **CLI tools** (`bin/`, script entry points) β€” Unix rule of silence: terse by default, verbose on request. + +### Patterns to FLAG β€” universal + +1. **Sensitive data in log messages (P1 β€” security leak):** + + ```python + # BAD β€” credentials in logs that ship off-host + logger.info(f"user={user}, token={token}") + logger.debug(f"request headers: {request.headers}") # includes Authorization + ``` + + Flag interpolation of variables named `token`, `password`, `secret`, `api_key`, `credential`, `auth`, `cookie`, `session`, `jwt`, `refresh`, `bearer`, `client_secret` into any log call. Also flag dumping entire request/response objects without redaction. **This repo handles Thingiverse JWTs/refresh tokens and API tokens β€” none may ever appear in a log line.** + + Acceptable: explicit redaction (`token[:8] + "..."`) or logging a hash/fingerprint. + +2. **`logger.error(...)` inside `except` without traceback:** use `logger.exception("work failed")` (shorthand for `error` + `exc_info=True`) inside `except` blocks unless there's a specific reason to omit the traceback. In Lambda, the traceback in CloudWatch is often the only forensic evidence. + +3. **`logging.basicConfig()` called from library code:** libraries call `logging.getLogger(__name__)`; `basicConfig` belongs in entry points only. + +4. **Eager formatting in hot paths:** `logger.debug(f"processed {item.expensive_repr()}")` formats unconditionally. Use lazy `%s` formatting (`logger.debug("processed %s", item)`) in loops over blueprint/file collections (scanner code iterates 12,000+ files). + +### Patterns to FLAG β€” CLI entry points only + +A file is a CLI entry point when it contains `if __name__ == "__main__":`, builds an `argparse.ArgumentParser` / uses Click decorators, or lives under `bin/`. + +5. **Default log level too verbose:** CLIs default to WARNING; INFO/DEBUG belong behind verbosity flags. + +6. **No verbosity flag:** a CLI that logs without a `-v`/`-q` ladder gives the user no dial. Canonical: `-v` INFO, `-vv` DEBUG, `-q` ERROR, via `action="count"` / `count=True`. + +7. **Verbosity flag that doesn't stack:** `action="store_true"` for `-v` means `-vv` does nothing. Use `action="count"`. + +8. **`print()` for diagnostics in CLI tools:** stdout is for the data the user asked for; diagnostics go to stderr via the logger. Mixing them breaks pipelines (`mycli | jq`). Note `bin/load_all.sh`-style scripts redirect stdout to fixture files β€” stray diagnostic prints corrupt the output. + +### Patterns to FLAG β€” library / service only + +9. **`print()` in library or Flask code:** bypasses the logger, can't be filtered, no timestamps. Flag in any module that isn't a CLI entry point. + +10. **Log-level discipline:** DEBUG = fine-grained detail; INFO = milestones; WARNING = recoverable (retry, fallback); ERROR = actionable failures. Flag `logger.info` in hot loops (should be DEBUG) and `logger.error` on recoverable retries (should be WARNING). + +### Do NOT flag + +- `print()` in tests and ad-hoc scripts. +- `print()` as the *data output* of a CLI (e.g., emitting fixture JSON to stdout by design). +- Logging configuration in a canonical entry point. +- Eager formatting in cold paths (once-at-startup lines). + +### Review approach + +1. Classify each `*.py` file in the diff: CLI entry point, library, or Flask service code. +2. Grep for `logger.` / `logging.` / `print(` calls. +3. For each log call: sensitive names interpolated? `logger.error` inside `except`? eager f-string in a hot path? +4. For CLI files: default level, verbosity ladder, stdout/stderr discipline. +5. For library files: flag any `basicConfig` (P2) and any `print()` (P2). diff --git a/.reviewers/migration-discipline-reviewer.md b/.reviewers/migration-discipline-reviewer.md new file mode 100644 index 00000000..bce00376 --- /dev/null +++ b/.reviewers/migration-discipline-reviewer.md @@ -0,0 +1,47 @@ +# migration-discipline-reviewer + +Review PRs for **database schema discipline**. All schema changes go through the versioned migration system in `openforge/db/schema/` β€” never ad-hoc DDL from application code, fixtures, or scripts. + +**The pattern (see `openforge/db/schema/version_12.py` as the reference):** + +```python +@SchemaVersionDecorator(NN) +class SchemaVersionNN(SchemaBase): + def up_impl(self, curs: cursor): + self.add_thing_table(curs) # small named helper methods + self.create_thing_indexes(curs) + + def down_impl(self, curs: cursor): # every up has a working down + self.drop_thing_indexes(curs) + self.drop_thing_table(curs) +``` + +**What to flag:** + +1. **DDL outside the migration system (P1):** `CREATE TABLE` / `ALTER TABLE` / `CREATE INDEX` / `DROP ...` in application code, route handlers, fixtures loaders, or scripts. The only home for DDL is a `version_NN.py` migration (and the SQL helper modules it calls). + +2. **Missing or stubbed `down_impl` (P2):** every migration must reverse cleanly. A `down_impl` that's `pass` or that doesn't mirror `up_impl` step-for-step (in reverse order) leaves the test↔main promotion path without a rollback. If a step is genuinely irreversible (data-destroying), it must say so in a comment and the docstring. + +3. **Version number problems (P1):** duplicate `@SchemaVersionDecorator(NN)` numbers, or a number that doesn't follow the current maximum. Check existing files in `openforge/db/schema/` for the highest version before assigning. (Note: version 15 is historically absent β€” gaps exist; duplicates are the bug.) + +4. **Missing module docstring (P3):** each migration opens with a docstring summarizing every change it makes (see version_12.py). The docstring is the changelog β€” it must list all tables/columns/indexes/functions touched. + +5. **Monolithic `up_impl` (P2):** the house style is small named helper methods per logical step, called in sequence from `up_impl`/`down_impl` β€” not one giant method of inline SQL. This mirrors the CLAUDE.md one-screen rule. + +6. **Schema change without downstream updates (P2):** a new table/column that application code reads must also appear in the relevant SQL helper module (`openforge/db/sql/`); if fixture data populates it, the fixture loader and (when applicable) the API upsert path must handle the new shape. A migration that lands alone with no consumer is fine when explicitly staged β€” say so in the PR. + +7. **Data migrations mixed silently into schema migrations (P3):** backfills/UPDATEs inside a schema migration are allowed but must be called out in the docstring, be idempotent where possible, and be sized for serverless (a full-table rewrite on the production DB should be flagged for discussion, not discovered). + +8. **Raw string interpolation in migration SQL (P1):** use `psycopg.sql` composition (`sql.SQL`, `sql.Identifier`) for any dynamic identifiers, matching the existing imports in migration files. Static DDL strings are fine. + +**Do NOT flag:** + +- `CREATE TEMPORARY TABLE` or session-scoped constructs in query code. +- Test fixtures that build schema in a throwaway test database via the migration runner itself. +- The absence of an Alembic-style framework β€” this repo's hand-rolled versioning is deliberate; review within its conventions, don't suggest replacing it. + +**Review approach:** + +1. Grep the diff for DDL keywords outside `openforge/db/schema/`. +2. For each new `version_NN.py`: verify decorator number is max+1 and unique, `down_impl` mirrors `up_impl` reversed, docstring enumerates the changes, helpers are small and named. +3. Cross-check new tables/columns against `openforge/db/sql/` accessors and fixture loaders touched (or explicitly staged) in the same PR. diff --git a/.reviewers/resource-leak-reviewer.md b/.reviewers/resource-leak-reviewer.md new file mode 100644 index 00000000..d87a30ba --- /dev/null +++ b/.reviewers/resource-leak-reviewer.md @@ -0,0 +1,56 @@ +# resource-leak-reviewer + +Review Python code for **resource leaks**. Two contexts matter in this repo, and the second raises the stakes: + +- **CLI tools / scanner runs** β€” batch processes where leaked fds pile up across thousands of files (the catalog manages 12,000+ STLs). +- **The Flask Lambda** β€” a warm Lambda container reuses the process across invocations, so anything leaked per-request accumulates exactly like a daemon. **Serverless Postgres has a hard connection cap** β€” a leaked connection isn't just memory, it's a production outage vector. Connection discipline is P1 here. + +**Patterns to FLAG:** + +1. **Database connections/cursors not closed or returned (P1):** + + ```python + # BAD β€” connection leaked; warm Lambda accumulates until Postgres refuses connections + conn = psycopg.connect(dsn) + curs = conn.cursor() + curs.execute(query) + + # GOOD + with psycopg.connect(dsn) as conn: + with conn.cursor() as curs: + curs.execute(query) + ``` + + Follow the existing connection-handling pattern in `openforge/app/` β€” new code must not invent its own connection lifecycle. Every code path (including exception paths) must release the connection. + +2. **`open()` without a context manager:** `with open(path) as f:` β€” always. In scanner code iterating thousands of files, a leaked handle per file exhausts the fd limit mid-run. + +3. **HTTP responses/sessions not closed:** for `requests`, use a `requests.Session` reused across calls (module/app level) rather than per-call construction in loops; use `with requests.get(url, stream=True) as resp:` for streamed downloads (STL files are large). + +4. **`subprocess.Popen` without `.wait()` or context manager:** use `subprocess.run(...)` for one-shot commands. + +5. **Unbounded `resp.content` / `.read()` from external sources:** an API or R2 object can be gigabytes. Check `Content-Length` or stream with bounded chunks when the size isn't controlled by us. + +6. **`functools.lru_cache` without `maxsize=` on unbounded key spaces:** set a cap when keys derive from external input. In Lambda, module-level caches survive across invocations. + +7. **Long-lived module-level dicts/sets as caches without eviction:** grows for the life of the warm container. Use `lru_cache` with maxsize or explicit eviction. + +8. **`tempfile.NamedTemporaryFile(delete=False)` without explicit cleanup:** pair with `finally: os.unlink(name)` or use `tempfile.TemporaryDirectory()`. Lambda's `/tmp` is limited and persists across warm invocations. + +9. **Manual `try/finally` chains protecting two or more resources:** use `contextlib.ExitStack`. + +10. **boto3/R2 clients constructed per call:** construct S3-compatible clients once (module/app level) and reuse β€” per-call construction defeats connection pooling and slows every invocation. + +**Do NOT flag:** + +- `pathlib.Path.read_text()` / `read_bytes()` β€” closes internally. +- `with` blocks that close resources in normal flow. +- Bounded `lru_cache` with explicit `maxsize`. +- One-shot scripts that exit within seconds β€” EXCEPT database connections, which are always in scope given the serverless Postgres cap. + +**Review approach:** + +1. For each `psycopg.connect` / cursor acquisition: verify release on every return path, and that it follows the app's established pattern. P1 on violation. +2. For each `open()`, `subprocess.Popen`, `requests.*`: verify a `with` block or explicit close. +3. For each `.read()` / `.content` from external input: verify a size bound. +4. For each module-level cache: ask about eviction; remember Lambda containers are long-lived. diff --git a/.reviewers/serverless-architecture-reviewer.md b/.reviewers/serverless-architecture-reviewer.md new file mode 100644 index 00000000..51c20908 --- /dev/null +++ b/.reviewers/serverless-architecture-reviewer.md @@ -0,0 +1,35 @@ +# serverless-architecture-reviewer + +Review PRs against this project's **cost-conscious serverless architecture** (CLAUDE.md: Patreon-funded, small budget). The backend is a single Flask Lambda; the database is serverless Postgres with a hard connection cap; files live in Cloudflare R2 (free egress); the frontend is a static React build on S3. Design decisions that are harmless on a beefy VPS are outages or bills here. + +**What to flag:** + +1. **Heavy new dependencies in `requirements.txt` (P2):** every megabyte in the Lambda package slows cold starts. Flag additions of large libraries (pandas, numpy, scipy, pillow-heavy stacks, ML/vision packages) or anything with big native wheels when a lighter tool or a few lines of stdlib would do. Ask: does this need to be in the *Lambda* at all, or only in local CLI tooling? (Local-only tools can live in a dev/optional dependency group that the Lambda package excludes.) + +2. **Per-request or per-call database connections without going through the app's connection handling (P1):** serverless Postgres connection limits are the system's scarcest resource. New code must use the app's established connection acquisition pattern β€” no private `psycopg.connect` sprinkled through business logic, no per-request pool construction. (Leak shapes are `resource-leak-reviewer`'s beat; this reviewer flags *architecturally wrong acquisition*, even leak-free.) + +3. **N+1 query patterns and chatty DB access (P2):** a loop issuing one query per blueprint/file/tag when a single batched query (`WHERE id = ANY(...)`, joined fetch, `executemany`, or `COPY`) would do. With ~1,400 designs and 12,000+ files, per-item queries are both slow and expensive. The same applies to R2: batch object operations where the API allows. + +4. **Direct S3 usage for file storage (P1):** file/object storage goes to **Cloudflare R2** (S3-compatible API, free egress) β€” never AWS S3 directly. Flag new `boto3` clients pointed at AWS S3 endpoints for content storage. (The static website deploy buckets are the existing exception β€” they're deployment infrastructure, not content storage.) + +5. **Features requiring persistent state or long-running processes (P1):** background threads, in-process schedulers, websockets, sticky in-memory session state, anything that assumes the process outlives the request. Lambda containers are disposable; state lives in Postgres or R2. Long-running work belongs in local CLI tooling, not the Lambda. + +6. **Work that scales with catalog size inside a single request (P2):** a request handler that scans all 12,000 files or recomputes catalog-wide aggregates per call. Precompute at fixture-load/scan time, store the result, or paginate. + +7. **Frontend assumptions of a server (P1):** the Next.js app compiles to a static export β€” no SSR, no API routes under the app, no server components with dynamic data, no `next/image` optimization endpoints, nothing that requires a Node server at runtime. (Detailed frontend conventions live in `frontend-conventions-reviewer`; this reviewer flags the architectural class.) + +8. **Unbounded Lambda invocation patterns (P3):** client code that polls an endpoint in a tight loop, or fan-out that turns one user action into many invocations, where batching or caching would do. + +**Do NOT flag:** + +- Local CLI tooling (`bin/`, scanner, the Thingiverse sync tool) doing long-running or memory-hungry work β€” that runs on Devon's machine by design; only its *Lambda-deployed* surface is constrained. +- Dev-only dependencies that don't ship in the Lambda package. +- Reasonable in-request caching (module-level memoization of static lookups) β€” that's a warm-container win, not persistent state. + +**Review approach:** + +1. `requirements.txt` diff β†’ size/necessity of each addition; does it ship to Lambda? +2. New DB access β†’ does it go through the established connection pattern? Any queries in loops? +3. Grep for `boto3`/S3 endpoints β†’ R2 or AWS? Content storage or deploy infra? +4. New endpoints β†’ per-request work bounded? State assumptions? +5. Frontend diff β†’ anything that breaks static export? diff --git a/.reviewers/test-coverage-reviewer.md b/.reviewers/test-coverage-reviewer.md new file mode 100644 index 00000000..fce1f285 --- /dev/null +++ b/.reviewers/test-coverage-reviewer.md @@ -0,0 +1,72 @@ +# test-coverage-reviewer + +Review code changes to **ensure the code touched by the PR has test coverage**. The goal is to incrementally grow the test suite β€” every PR either adds coverage or preserves it. + +**Core rule: Code touched by a PR must be covered by tests. Either the coverage already exists, or this PR adds it.** + +The unit of obligation is *coverage of the touched code*, not *net-new test functions for every diff*. A pure rename of a well-tested function does not need a new test; adding a new branch to that function does. + +**Where tests live in this repo:** + +- Python unit tests: `tests/test_.py`, pytest. Run: `pytest tests/`. +- Python integration tests: `integration_tests/` (require Flask running on port 5328) β€” these are NOT run per-PR in CI; treat them as supplemental coverage, never the sole coverage for branching logic. +- Frontend tests: `src/**/__tests__/*.test.ts(x)`, Jest. Run: `npm test` or `npx jest --findRelatedTests `. + +**Rules to enforce:** + +1. **Coverage required for all touched code.** For any modified or new function/method/hook/util, verify that *some* test exercises it β€” existing or new. If the function is uncovered today, this PR adds coverage. Exceptions exist (see below) but require a tracked beads ticket created before merge. + +2. **Refactors preserve coverage, not duplicate it.** Pure refactors don't need net-new tests if existing tests still exercise the refactored code and stay green. Flag a refactor only when the touched code was uncovered before β€” that's the moment to add the test. + +3. **New behavior needs new assertions.** Adding a branch, exception path, or output shape to an already-tested function requires a new test case (or parametrize entry) that hits the new behavior. + +4. **Recognize as valid coverage:** `def test_*` functions, `class Test...:` methods, `@pytest.mark.parametrize` entries, Jest `it`/`test` blocks, fixtures consumed by actual tests (fixtures alone are infrastructure, not tests). + +5. **Bug fix documentation.** If the change fixes a bug: require a comment or commit message explaining what was broken and why the fix works, plus a regression test that would have failed before the fix. + +**Integration tests as coverage β€” acceptable boundary:** + +Integration tests count as coverage when the unit boundary is **glue code with no branching logic** β€” e.g., a Flask route that decodes JSON and dispatches to a SQL helper, or a CLI that parses args and calls one function. Demanding a separate unit test for such glue produces low-value mock-heavy tests. + +Integration tests do **not** count when the unit being touched has its own branching, validation, parsing, or business logic that can be exercised in isolation β€” flag the missing unit test even if an integration test exists. Remember integration tests don't run in per-PR CI here. + +**Test quality patterns to flag (P2):** + +1. **`mock.patch` at point-of-definition instead of point-of-use:** + + ```python + # BAD β€” doesn't intercept the call inside openforge.data.scanner + @mock.patch("os.path.exists") + # GOOD β€” patches the symbol the module actually consults + @mock.patch("openforge.data.scanner.os.path.exists") + ``` + + The consuming module has its OWN reference from its import β€” patching the origin doesn't rebind it. The single most common reason `mock.patch` "doesn't work." + +2. **Mocking what you don't own:** mock at YOUR boundary (`openforge.thingiverse.client.fetch`), not the third-party library's internals (`requests.get`). Mocking third parties couples tests to library internals. + +3. **Over-mocking:** a test that mocks five or more collaborators mostly verifies that mocks return what mocks return. Flag when mock count exceeds assertion count. Fix is usually testing at a higher level or refactoring for fewer collaborators. + +4. **`@pytest.mark.skip` without `reason=` (or Jest `it.skip`/`xit` without a comment):** a skipped test with no reason is a dead test. Fix it, file a beads ticket and reference it in the reason, or delete it. + +5. **Tests that don't assert:** flag the absence of `assert`, `pytest.raises`, `expect(...)`, mock assertion calls, or equivalent. Acceptable exception: a body that is entirely a `with pytest.raises(...):` block. + +**Review approach:** + +1. Identify all functions/methods/hooks added or modified in the PR. +2. For each, grep `tests/` and `src/**/__tests__/` for tests that name or call it. +3. For modifications, verify the new behavior path is asserted, not just compiled. +4. If coverage is missing, name the specific functions and suggest test cases from edge cases visible in the code (`None` inputs, empty collections, boundary numbers). +5. Distinguish: "no coverage" (block) / "coverage exists but doesn't hit the new branch" (block) / "pure refactor of covered code" (allow). +6. Scan new/modified test files for the five quality patterns. + +**Do NOT allow:** + +- "Verified manually" as a substitute when the unit has branching logic. +- Marking coverage "out of scope" without an accompanying beads ticket. + +**Acceptable exceptions (each requires a beads ticket created before merge):** + +- Adding tests to legacy uncovered code is genuinely larger than this PR. +- The change is a config/data/fixture-content edit with no executable logic. +- The change is a dependency bump with no source change. diff --git a/AGENT-REVIEWERS.md b/AGENT-REVIEWERS.md new file mode 100644 index 00000000..fdd85079 --- /dev/null +++ b/AGENT-REVIEWERS.md @@ -0,0 +1,202 @@ +# Agents + +Each H2 below names a reviewer. The one-line summary tells the main loop **what the reviewer checks and when to spawn it** β€” use it to decide whether the PR diff is in scope. The body points at `.reviewers/.md`, which the spawned Task reads as its complete specification. + +## complexity-reviewer + +**What it checks:** McCabe complexity > 10 (hard floor), the CLAUDE.md "And/Or" test and one-screen rule, nesting depth β‰₯ 4, parameter count > 5, class size > 20 public methods, nested ternaries, redundant single-call wrappers, generic identifiers in long functions. Covers Python and TypeScript/React. +**When to spawn:** PR touches production `*.py`, `*.ts`, or `*.tsx` (skip `tests/`, `integration_tests/`, `__tests__/`). Skip if the diff is data / docs / SQL / config only. +**Post only what this PR owns:** (1) **introduced/worsened only** β€” do NOT post complexity in a function that was already over a threshold before this PR unless the PR pushes it *further* over; (2) **no borderline soft flags** β€” the "And/Or" test and one-screen rule are advisory; post them only when they compound a hard objective violation on the same function. + +Read `.reviewers/complexity-reviewer.md` and follow it as your complete review specification. + +--- + +## error-handling-reviewer + +**What it checks:** silent exception swallows, `return` inside `finally`, generic `except Exception` without re-raise, bare `except:`, missing `raise ... from`, `assert` used for runtime validation. +**When to spawn:** PR touches production `*.py` (skip if changes are docs / configs / tests only). + +Read `.reviewers/error-handling-reviewer.md` and follow it as your complete review specification. + +--- + +## test-coverage-reviewer + +**What it checks:** every PR-touched function has a test that exercises it (pytest for `openforge/`, Jest for `src/`); flags missing tests for new branches/exceptions, weak mocking (patch point-of-use, don't mock third parties), tests that don't assert, skips without reasons. +**When to spawn:** PR modifies any `*.py` outside `tests/` or any `src/**/*.ts(x)` outside `__tests__/` (and ideally also when test files change β€” to lint the new tests themselves). + +Read `.reviewers/test-coverage-reviewer.md` and follow it as your complete review specification. + +--- + +## resource-leak-reviewer + +**What it checks:** psycopg connections/cursors not released (P1 β€” serverless Postgres connection cap), `open()`/`subprocess` without context managers, HTTP sessions constructed per-call in loops, unbounded reads from external sources, unbounded caches in the warm-Lambda process, per-call boto3/R2 client construction. +**When to spawn:** PR touches `*.py` that allocates fds, sockets, DB connections, subprocesses, or long-lived caches. Skip if the diff is pure logic / tests / docs. + +Read `.reviewers/resource-leak-reviewer.md` and follow it as your complete review specification. + +--- + +## dead-code-reviewer + +**What it checks:** unused module-level symbols, unused `__init__.py` re-exports, commented-out code, partial refactors leaving the old name, stale `__all__` entries, `pass`-only stubs, unused React components/props/types. +**When to spawn:** PR touches `*.py` or `src/**/*.ts(x)`, especially when it removes call sites, renames functions, or extracts/moves code. Skip for pure additive changes with no refactor surface. + +Read `.reviewers/dead-code-reviewer.md` and follow it as your complete review specification. + +--- + +## logging-reviewer + +**What it checks:** sensitive data (tokens, JWTs, secrets) interpolated into log calls (P1), `logger.error` inside `except` without traceback, `logging.basicConfig` from library code, eager debug formatting in hot paths; for `bin/` CLIs also default level / verbosity ladder / stdout-vs-stderr discipline. +**When to spawn:** PR touches `*.py` that uses `logger.`, `logging.`, or `print(` for diagnostic output. Skip if the diff is data files / tests only. + +Read `.reviewers/logging-reviewer.md` and follow it as your complete review specification. + +--- + +## importlib-resources-reviewer + +**What it checks:** package data loaded via `Path(__file__).parent` instead of `importlib.resources` β€” breaks in the Lambda zip. The repo convention is `impresources` (see `openforge/db/fixtures/__init__.py`). +**When to spawn:** PR touches `*.py` inside the `openforge/` package that loads bundled data files (fixtures, SQL, templates). Skip for tests, `bin/` scripts, and pure logic edits. + +Read `.reviewers/importlib-resources-reviewer.md` and follow it as your complete review specification. + +--- + +## migration-discipline-reviewer + +**What it checks:** schema changes go through `openforge/db/schema/version_NN.py` (`@SchemaVersionDecorator`, paired `up_impl`/`down_impl`, docstring changelog, small named helpers, correct version numbering); no DDL from application code; new columns/tables have consumers or are explicitly staged. +**When to spawn:** PR touches `openforge/db/schema/`, or the diff contains DDL keywords (`CREATE TABLE`, `ALTER TABLE`, `CREATE INDEX`, `DROP`) anywhere outside it. Skip otherwise. + +Read `.reviewers/migration-discipline-reviewer.md` and follow it as your complete review specification. + +--- + +## serverless-architecture-reviewer + +**What it checks:** the cost-conscious serverless constraints β€” heavy Lambda dependencies, DB access outside the established connection pattern, N+1 queries, direct AWS S3 for content storage (must be R2), persistent-state/long-running features, per-request work that scales with catalog size, static-export violations. +**When to spawn:** PR touches `requirements.txt`, backend code in `openforge/app/` or `openforge/db/`, storage client code, or adds new endpoints. Skip for pure frontend styling / docs / test-only diffs. + +Read `.reviewers/serverless-architecture-reviewer.md` and follow it as your complete review specification. + +--- + +## frontend-conventions-reviewer + +**What it checks:** hardcoded API base URLs (must be relative `/api` β€” CLAUDE.md hard rule), static-export violations (API routes, SSR, `next/image` optimizer), inline logic that belongs in custom hooks, `any`/`@ts-ignore` type escapes, derived-state-via-useEffect anti-patterns, duplication of `src/utils/` helpers. +**When to spawn:** PR touches `src/**/*.ts` or `src/**/*.tsx`. Skip for backend-only diffs. + +Read `.reviewers/frontend-conventions-reviewer.md` and follow it as your complete review specification. + +--- + +## credentials-hygiene-reviewer + +**What it checks:** literal secrets/JWTs committed anywhere (P1), HAR/traffic captures with live tokens in the diff (P1 β€” this project uses HAR captures for Thingiverse reverse-engineering; they carry live credentials and stay out of the repo), tokens in URLs when a header works, tokens echoed to output, insecure persistence of refresh tokens, realistic credentials in test fixtures. +**When to spawn:** PR touches auth/token code, adds fixtures or data files, adds `*.har`/capture artifacts, modifies `.gitignore`/env handling, or touches the Thingiverse integration. Cheap to run β€” when in doubt, spawn it. + +Read `.reviewers/credentials-hygiene-reviewer.md` and follow it as your complete review specification. + +--- + +# Guidelines + +## How the pack runs + +**This file is consumed BY the `pr-review-loop` skill β€” do not run these reviewers directly.** If you've read this file and are about to spawn the agents yourself (outside the skill), stop and invoke `pr-review-loop` instead: the skill owns the parts this file doesn't define β€” agents posting findings as PR line comments, per-thread replies, per-agent retirement, CI gating, and exit conditions. + +Each reviewer runs independently and reports findings without coordination. A reviewer's silence on something is not an endorsement β€” it just means that reviewer didn't see anything in its scope. + +**Per-reviewer file scope:** + +| Reviewer | Files in scope | +|----------|----------------| +| `complexity-reviewer` | `*.py`, `*.ts`, `*.tsx` (skips tests) | +| `error-handling-reviewer` | `*.py` (production code) | +| `test-coverage-reviewer` | `*.py`, `src/**/*.ts(x)` | +| `resource-leak-reviewer` | `*.py` | +| `dead-code-reviewer` | `*.py`, `src/**/*.ts(x)` | +| `logging-reviewer` | `*.py` (rules differ for `bin/` CLIs vs the Flask app) | +| `importlib-resources-reviewer` | `openforge/**/*.py` (skips tests and `bin/`) | +| `migration-discipline-reviewer` | `openforge/db/schema/**` + any diff containing DDL | +| `serverless-architecture-reviewer` | `requirements.txt`, `openforge/app/**`, `openforge/db/**`, storage clients | +| `frontend-conventions-reviewer` | `src/**/*.ts(x)` | +| `credentials-hygiene-reviewer` | everything (auth code, fixtures, artifacts, `.gitignore`) | + +Skip reviewers whose file scope doesn't match the PR diff. + +## Branch targeting + +PRs target **`test`** (the development default branch), never `main`. `main` is the production deployment branch; `test` β†’ `main` merges are intentional production releases only. A PR opened against `main` that isn't an explicit deploy is itself a finding. + +## Tooling assumed in CI + +CI runs (see `.github/workflows/test.yaml`): `pytest tests/ --cov=openforge`, `npm test`, `npm run lint:all` (ESLint + `tsc --noEmit`), `ruff check .`, `ruff format --check .`. Pre-commit mirrors these locally. + +Reviewers do not duplicate what CI already enforces β€” they cover what it misses (C901 complexity, bandit-class security checks, dead-code detection have no CI gate here, so the corresponding reviewers carry that weight themselves). Suggesting a new CI tool is a P3 advisory, not a blocking finding. + +## Severity convention + +Every finding must be tagged with a beads-style priority: + +| Priority | Disposition | Examples | +|----------|-------------|----------| +| **P1** | Blocking β€” must fix before merge | Committed secret or HAR with live tokens, leaked/rogue DB connection against serverless Postgres, silent exception swallow, DDL outside the migration system, hardcoded API base URL, static-export violation, direct S3 for content storage, sensitive data in logs | +| **P2** | Should fix in this PR | Missing tests for new branches, complexity floor violation, missing `raise ... from` on new chains, missing `down_impl`, N+1 queries, `any`-typed new code, tokens in URLs | +| **P3** | Advisory β€” deferrable with a beads ticket | Complexity heuristic findings, dead code, log-level nits, state-management patterns, utility duplication, docs for new env vars | + +**Default severity per reviewer:** + +- `credentials-hygiene-reviewer`, `resource-leak-reviewer` (DB connections): **P1** by default. +- `error-handling-reviewer`: **P2** by default; **P1** for silent swallows and `return` in `finally`. +- `logging-reviewer`: **P2** by default; **P1** for sensitive data in log messages. +- `migration-discipline-reviewer`: **P1** for DDL outside the migration system and version-number collisions; **P2** otherwise. +- `serverless-architecture-reviewer`: **P1** for S3-instead-of-R2, persistent-state features, and connection-pattern violations; **P2** otherwise. +- `frontend-conventions-reviewer`: **P1** for hardcoded base URLs and static-export violations; **P2** otherwise. +- `test-coverage-reviewer`: **P2** by default. +- `complexity-reviewer`: **P2** for objective floor violations; **P3** for heuristic findings. +- `dead-code-reviewer`: **P3** by default. + +A reviewer may promote or demote a specific finding from its default, but must state why. + +## Output format + +Findings must be structured. Use this template: + +``` +[] [] + +File: path/to/file.py:LINE +Quote: + <1-5 lines of code or text being flagged> + +Issue: +Suggested fix: + +Reason (optional): +``` + +Structured findings are diff-able, easy to triage, and easy to deduplicate when multiple reviewers flag the same line. + +## Deferring findings with beads + +To defer a P2 or P3 finding to a follow-up: + +1. Create a beads ticket capturing reviewer name, severity, file, and quote. +2. Link the ticket in the PR description or as a reply to the reviewer's comment. +3. The reviewer accepts the deferral only when the beads ticket exists. + +**P1 findings are not deferrable** β€” they must be fixed in-PR. + +## Reviewing external-bot suggestions + +Per CLAUDE.md: be skeptical of Gemini's suggestions β€” it lacks project context. Agent reviewers in this pack take precedence when they conflict with a generic "best practice" suggestion that doesn't fit this project's philosophy (pragmatism, cost-consciousness, existing patterns). Not every bot suggestion needs implementation; "won't fix β€” conflicts with project convention X" is a valid disposition when X is real and named. + +# Context + +OpenForge Catalog is a content management system for 12,000+ 3D-printable STL files (~1,400 designs). Backend: Python/Flask deployed as a single AWS Lambda; PostgreSQL on serverless (hard connection limits); file storage on Cloudflare R2 (never AWS S3 for content). Frontend: Next.js compiled to a **static export** served from S3 β€” no server at runtime; all API calls are relative to `/api`. Data flows from a Dropbox-scanning pipeline into fixture JSON (`openforge/db/fixtures/`), loaded locally and replayed to production through the Flask API (`bin/upload_fixture`). Schema changes use a hand-rolled versioned migration system (`openforge/db/schema/version_NN.py`). Tests: pytest (`tests/`), Jest (`src/**/__tests__/`), plus local-only integration tests (`integration_tests/`, Flask on :5328). + +Active work includes a Thingiverse publish/sync tool (see beads epic `openforge_catalog-4kx`): v2 JWT auth, HAR-derived API contract, file-hash-based sync. That work handles live credentials (JWTs, refresh tokens, client secrets from `~/.profile.d/`) and HAR captures β€” hence `credentials-hygiene-reviewer`. From 1094753295eca0789aa89cfdfff6d01728965a52 Mon Sep 17 00:00:00 2001 From: Devon Jones Date: Mon, 6 Jul 2026 12:39:38 -0600 Subject: [PATCH 2/3] feat: add dedicated SPA reviewers (react-hooks, js-async) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catalog frontend is a full SPA, so hook correctness (deps arrays, effect cleanup, fetch races, list rendering at catalog scale) and promise correctness (floating promises, resp.ok, swallowed rejections) get dedicated reviewers rather than riding along in frontend-conventions. Part of openforge_catalog-1v5 Co-Authored-By: Claude Fable 5 πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .reviewers/js-async-reviewer.md | 82 ++++++++++++++++++++++++++++++ .reviewers/react-hooks-reviewer.md | 63 +++++++++++++++++++++++ AGENT-REVIEWERS.md | 22 ++++++++ 3 files changed, 167 insertions(+) create mode 100644 .reviewers/js-async-reviewer.md create mode 100644 .reviewers/react-hooks-reviewer.md diff --git a/.reviewers/js-async-reviewer.md b/.reviewers/js-async-reviewer.md new file mode 100644 index 00000000..8bd383ff --- /dev/null +++ b/.reviewers/js-async-reviewer.md @@ -0,0 +1,82 @@ +# js-async-reviewer + +Review TypeScript/JavaScript (`src/**/*.ts`, `src/**/*.tsx`) for **async and promise correctness** β€” the JS counterpart of `error-handling-reviewer`. Promise bugs fail silently: an unawaited rejection disappears into the console (or nowhere), and the UI shows stale data or a spinner that never resolves. In a static SPA there is no server log β€” the browser is the only witness, so errors that vanish client-side vanish entirely. + +**Patterns to FLAG:** + +1. **Floating promises (P2):** + + ```ts + // BAD β€” fire-and-forget; rejection is unobserved, completion unordered + saveBlueprint(data); + navigate("/catalog"); + + // GOOD + await saveBlueprint(data); + navigate("/catalog"); + ``` + + A promise-returning call whose result is neither `await`ed, `.then/.catch`ed, nor deliberately marked (`void doThing()` with a comment) is a bug until proven otherwise. Flag especially in event handlers and effects where sequencing matters. + +2. **`fetch` without checking `resp.ok` (P2):** + + ```ts + // BAD β€” 4xx/5xx does NOT reject; .json() parses an error body as data + const data = await (await fetch("/api/blueprints")).json(); + + // GOOD + const resp = await fetch("/api/blueprints"); + if (!resp.ok) throw new Error(`GET /api/blueprints: ${resp.status}`); + const data = await resp.json(); + ``` + + `fetch` only rejects on network failure. Every fetch call site (or the shared wrapper it goes through) must handle non-2xx explicitly. If `src/` has a shared API helper, flag raw `fetch` calls that bypass it. + +3. **Swallowed rejections (P1):** + + ```ts + // BAD β€” same disease as Python's `except: pass` + try { + await saveBlueprint(data); + } catch (e) { + // nothing, or console.log(e) with no user-visible consequence + } + ``` + + A user clicked save, it failed, and the UI pretends it worked. Every catch must either surface to the user (error state, toast), rethrow, or carry a comment for the documented-ignorable case. `console.error` alone is P3-acceptable only for genuinely non-actionable telemetry paths. + +4. **Async event handlers with no error path (P2):** `onClick={async () => { await mutate(); }}` β€” the rejection escapes to the void; React does not catch it and no error boundary sees it (error boundaries only catch render-phase errors). Wrap the body in try/catch that sets error state, or route through a helper that does. + +5. **`Promise.all` where one failure should not sink the batch (P3):** `Promise.all` rejects fast on the first failure and abandons the rest. For independent operations (uploading N files, fetching N thumbnails), suggest `Promise.allSettled` with per-item error handling. Conversely, `allSettled` whose results are never inspected is a swallow β€” flag as P2. + +6. **Sequential awaits for independent work (P3):** + + ```ts + // BAD β€” serial waterfall, 3Γ— latency + const tags = await fetchTags(); + const textures = await fetchTextures(); + + // GOOD β€” concurrent + const [tags, textures] = await Promise.all([fetchTags(), fetchTextures()]); + ``` + + Flag when the calls are visibly independent and on a user-facing path. + +7. **Missing loading/error UI states for new async flows (P3):** a new user-triggered async operation with no pending indication and no failure rendering. The catalog's users wait on real network calls; silence reads as breakage. (Component-structure aspects belong to `frontend-conventions-reviewer`.) + +8. **`async` executor / promise-constructor anti-pattern (P2):** `new Promise(async (resolve, reject) => ...)` β€” rejections inside the async executor are lost. Almost always the surrounding code should just be an async function. + +**Do NOT flag:** + +- `void somePromise()` with a comment β€” that's the explicit fire-and-forget marker. +- Rejections handled by an established shared wrapper (verify the wrapper actually handles them before crediting it). +- Test code awaiting patterns that Jest manages. +- Top-level orchestration in scripts where an unhandled rejection crashing the process is the desired behavior. + +**Review approach:** + +1. Grep added lines for promise-returning calls (`fetch(`, `.then(`, `async ` functions invoked bare); for each, find the await/catch/void disposition. +2. For each `fetch`: `resp.ok` handled here or in the wrapper it uses? +3. For each `catch` block: does the error reach the user, a rethrow, or a documented ignore? +4. For each async handler in JSX props: error path? +5. For `Promise.all`: is fail-fast the right semantic for that batch? diff --git a/.reviewers/react-hooks-reviewer.md b/.reviewers/react-hooks-reviewer.md new file mode 100644 index 00000000..cf6a47f5 --- /dev/null +++ b/.reviewers/react-hooks-reviewer.md @@ -0,0 +1,63 @@ +# react-hooks-reviewer + +Review React code (`src/**/*.tsx`, hook files in `src/**/*.ts`) for **hook correctness and render performance**. Hooks bugs are the SPA's equivalent of concurrency bugs: they compile clean, pass type-check, work in the happy path, and fail as stale UI, phantom re-renders, or memory leaks. The catalog renders large collections (1,400+ designs, sprite sheets, tag trees), so render performance is a correctness concern, not a nicety. + +**Patterns to FLAG:** + +1. **Missing or wrong dependency arrays (P2):** + + ```tsx + // BAD β€” stale closure: filter changes but effect never re-runs + useEffect(() => { + fetchBlueprints(filter).then(setResults); + }, []); + ``` + + Every value from component scope used inside the effect/callback belongs in the deps array. Flag deps arrays that omit used values, and `// eslint-disable-next-line react-hooks/exhaustive-deps` without a comment explaining why the omission is safe. + +2. **Missing effect cleanup (P2):** subscriptions, event listeners (`window.addEventListener`), timers (`setInterval`/`setTimeout`), and observers (`IntersectionObserver`, `ResizeObserver`) registered in an effect must be released in the cleanup function. A sprite viewer that adds a keydown listener per mount and never removes it leaks a listener per navigation. + +3. **State updates after unmount / unaborted fetches (P2):** + + ```tsx + // BAD β€” setState fires after unmount if navigation happens mid-fetch + useEffect(() => { + fetch(`/api/blueprints/${id}`).then(r => r.json()).then(setData); + }, [id]); + + // GOOD β€” abort on cleanup + useEffect(() => { + const ctrl = new AbortController(); + fetch(`/api/blueprints/${id}`, { signal: ctrl.signal }) + .then(r => r.json()).then(setData) + .catch(e => { if (e.name !== "AbortError") throw e; }); + return () => ctrl.abort(); + }, [id]); + ``` + + Also the race variant: two rapid `id` changes resolve out of order and the stale response wins. Abort or a staleness guard fixes both. + +4. **Conditional hooks (P1):** hooks called inside `if`/loops/early-return paths violate the Rules of Hooks and corrupt hook state across renders. ESLint usually catches this; flag it if the disable comment shows up instead. + +5. **Derived state stored in state (P3):** `useState` + `useEffect` to keep a computed value in sync with props/state. Compute during render, or `useMemo` if expensive. (Shared with `frontend-conventions-reviewer` β€” post under whichever found it first.) + +6. **Unstable references re-triggering children (P3 β€” P2 in list-rendering paths):** new object/array/function literals passed as props to memoized children or used as effect deps (`style={{...}}`, `onSelect={() => ...}` into a 1,400-row list). Flag when the receiver is memoized, is an effect dep, or renders in a large list; don't flag for cheap leaf components. + +7. **Expensive list rendering without keys/memo (P2):** catalog-scale lists (blueprints, tags, files) rendered without stable `key`s (or keyed by array index while reorderable/filterable), or re-mapped with heavy per-item computation each render without `useMemo`. Suggest virtualization only when the list is genuinely unbounded β€” match existing patterns in `src/` first. + +8. **`useEffect` as a data-flow bus (P3):** chains where effect A sets state that triggers effect B that sets more state. Usually one event handler or one derived computation. Flag chains β‰₯ 2 hops. + +9. **Custom hooks that break the contract (P2):** hooks that conditionally call other hooks, return unstable identities every render (fresh callbacks/objects without `useCallback`/`useMemo`) while documented as stable, or hide required cleanup from the consumer. + +**Do NOT flag:** + +- Intentionally-empty deps (`[]`) for genuinely mount-only effects with cleanup, when nothing from component scope is used inside. +- Unstable props into cheap unmemoized leaves β€” memoizing everything is its own disease. +- Existing patterns in `src/` that already work β€” match the codebase's conventions (this repo delegates event handling to custom hooks per CLAUDE.md; recommend that shape). + +**Review approach:** + +1. For each `useEffect`/`useCallback`/`useMemo` in the diff: deps complete? cleanup present when it registers anything? disable-comments justified? +2. For each fetch-in-effect: abort/staleness handling on param change and unmount? +3. For each list render: stable keys, per-item cost, memo boundaries. +4. For each new custom hook: stable return identity, cleanup ownership, no conditional hooks. diff --git a/AGENT-REVIEWERS.md b/AGENT-REVIEWERS.md index fdd85079..3a823a27 100644 --- a/AGENT-REVIEWERS.md +++ b/AGENT-REVIEWERS.md @@ -93,6 +93,24 @@ Read `.reviewers/frontend-conventions-reviewer.md` and follow it as your complet --- +## react-hooks-reviewer + +**What it checks:** hook correctness and render performance β€” missing/wrong dependency arrays, missing effect cleanup (listeners, timers, observers), setState-after-unmount and fetch races (missing abort), conditional hooks, unstable references into memoized children/effect deps, catalog-scale list rendering without stable keys/memo, `useEffect` chains as data-flow. +**When to spawn:** PR touches `src/**/*.tsx` or hook files (`use*.ts`) in `src/`. Skip for backend-only or non-component TS diffs. + +Read `.reviewers/react-hooks-reviewer.md` and follow it as your complete review specification. + +--- + +## js-async-reviewer + +**What it checks:** floating promises, `fetch` without `resp.ok` handling, swallowed rejections (P1 β€” the JS `except: pass`), async event handlers with no error path, `Promise.all` vs `allSettled` semantics, serial awaits for independent work, missing loading/error states, async promise-executor anti-pattern. +**When to spawn:** PR touches `src/**/*.ts(x)` containing `async`, `await`, `.then(`, or `fetch(`. Skip otherwise. + +Read `.reviewers/js-async-reviewer.md` and follow it as your complete review specification. + +--- + ## credentials-hygiene-reviewer **What it checks:** literal secrets/JWTs committed anywhere (P1), HAR/traffic captures with live tokens in the diff (P1 β€” this project uses HAR captures for Thingiverse reverse-engineering; they carry live credentials and stay out of the repo), tokens in URLs when a header works, tokens echoed to output, insecure persistence of refresh tokens, realistic credentials in test fixtures. @@ -124,6 +142,8 @@ Each reviewer runs independently and reports findings without coordination. A re | `migration-discipline-reviewer` | `openforge/db/schema/**` + any diff containing DDL | | `serverless-architecture-reviewer` | `requirements.txt`, `openforge/app/**`, `openforge/db/**`, storage clients | | `frontend-conventions-reviewer` | `src/**/*.ts(x)` | +| `react-hooks-reviewer` | `src/**/*.tsx`, `src/**/use*.ts` | +| `js-async-reviewer` | `src/**/*.ts(x)` with async/promise code | | `credentials-hygiene-reviewer` | everything (auth code, fixtures, artifacts, `.gitignore`) | Skip reviewers whose file scope doesn't match the PR diff. @@ -156,6 +176,8 @@ Every finding must be tagged with a beads-style priority: - `migration-discipline-reviewer`: **P1** for DDL outside the migration system and version-number collisions; **P2** otherwise. - `serverless-architecture-reviewer`: **P1** for S3-instead-of-R2, persistent-state features, and connection-pattern violations; **P2** otherwise. - `frontend-conventions-reviewer`: **P1** for hardcoded base URLs and static-export violations; **P2** otherwise. +- `react-hooks-reviewer`: **P2** by default; **P1** for conditional hooks; **P3** for unstable-reference and derived-state advisories. +- `js-async-reviewer`: **P2** by default; **P1** for swallowed rejections that hide user-facing failures. - `test-coverage-reviewer`: **P2** by default. - `complexity-reviewer`: **P2** for objective floor violations; **P3** for heuristic findings. - `dead-code-reviewer`: **P3** by default. From 85ada0144271494c405034a270c9c1d3ffc67eff Mon Sep 17 00:00:00 2001 From: Devon Jones Date: Mon, 6 Jul 2026 13:06:59 -0600 Subject: [PATCH 3/3] fix: address round-1 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - correct ruff C901 invocation (max-complexity is pyproject config, not a CLI flag; ruff default is already 10) - add severity precedence rule: spec per-item tags win over the index defaults table - document why File: stays in the finding body despite post-line-comment.sh anchoring - make hooks/frontend-conventions overlap acknowledgment mutual - broaden capture hygiene: ignored captures/ dir for raw traffic dumps of any extension (credentials-hygiene-reviewer finding) Part of openforge_catalog-1v5 Co-Authored-By: Claude Fable 5 πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .gitignore | 5 ++++- .reviewers/complexity-reviewer.md | 2 +- .reviewers/frontend-conventions-reviewer.md | 2 +- AGENT-REVIEWERS.md | 4 +++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index b92ddf86..77feeeef 100644 --- a/.gitignore +++ b/.gitignore @@ -22,8 +22,11 @@ scratch .env -# traffic captures β€” carry live credentials (JWTs, cookies); never commit +# 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* diff --git a/.reviewers/complexity-reviewer.md b/.reviewers/complexity-reviewer.md index ae06c749..11a1be5d 100644 --- a/.reviewers/complexity-reviewer.md +++ b/.reviewers/complexity-reviewer.md @@ -9,7 +9,7 @@ This reviewer enforces the project's Code Organization Philosophy (CLAUDE.md): * 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 --max-complexity 10` against the PR head (never a stale local checkout) and flag every function that exceeds it. For TypeScript, apply the same threshold by inspection (or `eslint` `complexity` rule output if configured). +**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: diff --git a/.reviewers/frontend-conventions-reviewer.md b/.reviewers/frontend-conventions-reviewer.md index 8355628b..432611b6 100644 --- a/.reviewers/frontend-conventions-reviewer.md +++ b/.reviewers/frontend-conventions-reviewer.md @@ -17,7 +17,7 @@ Review TypeScript/React changes (`src/**/*.ts`, `src/**/*.tsx`) for this repo's 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. +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. diff --git a/AGENT-REVIEWERS.md b/AGENT-REVIEWERS.md index 3a823a27..9063aad1 100644 --- a/AGENT-REVIEWERS.md +++ b/AGENT-REVIEWERS.md @@ -182,7 +182,7 @@ Every finding must be tagged with a beads-style priority: - `complexity-reviewer`: **P2** for objective floor violations; **P3** for heuristic findings. - `dead-code-reviewer`: **P3** by default. -A reviewer may promote or demote a specific finding from its default, but must state why. +A reviewer may promote or demote a specific finding from its default, but must state why. **Where a spec under `.reviewers/` tags a specific pattern with an explicit priority, the spec's tag wins over this table** β€” the table gives each reviewer's default, not a ceiling/floor. ## Output format @@ -203,6 +203,8 @@ Reason (optional): Structured findings are diff-able, easy to triage, and easy to deduplicate when multiple reviewers flag the same line. +When posting via `post-line-comment.sh`, the comment is already anchored to a file/line by the script's arguments β€” keep the `File:` line in the body anyway (it survives quoting, consolidation into PR-level summaries, and beads-ticket copy/paste, where the anchor doesn't). + ## Deferring findings with beads To defer a P2 or P3 finding to a follow-up: