From af9bc715ccdf18dc7cbf87d84dd9633108dc7075 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 19 Mar 2026 00:40:42 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20remove=20Rails-isms=20=E2=80=94=20platfo?= =?UTF-8?q?rm-agnostic=20templates=20and=20checklist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - review/checklist.md: multi-framework examples (Rails/Node/Python/Django) - plan-ceo-review: framework-agnostic grep + generic error table - plan-eng-review: "corresponding test" not "JS or Rails test" - CLAUDE.md: Platform-agnostic design principle + Testing section Co-Authored-By: Claude Opus 4.6 (1M context) --- CLAUDE.md | 23 +++++++++++++++++++++++ review/checklist.md | 14 +++++++------- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8b65c8a3..d75a7a45 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,6 +30,17 @@ on `git diff` against the base branch. Each test declares its file dependencies llm-judge, gen-skill-docs) trigger all tests. Use `EVALS_ALL=1` or the `:all` script variants to force all tests. Run `eval:select` to preview which tests would run. +## Testing + +```bash +bun test # run before every commit — free, <2s +bun run test:evals # run before shipping — paid, diff-based (~$4/run max) +``` + +`bun test` runs skill validation, gen-skill-docs quality checks, and browse +integration tests. `bun run test:evals` runs LLM-judge quality evals and E2E +tests via `claude -p`. Both must pass before creating a PR. + ## Project structure ``` @@ -79,6 +90,18 @@ SKILL.md files are **generated** from `.tmpl` templates. To update docs: To add a new browse command: add it to `browse/src/commands.ts` and rebuild. To add a snapshot flag: add it to `SNAPSHOT_FLAGS` in `browse/src/snapshot.ts` and rebuild. +## Platform-agnostic design + +Skills must NEVER hardcode framework-specific commands, file patterns, or directory +structures. Instead: + +1. **Read CLAUDE.md** for project-specific config (test commands, eval commands, etc.) +2. **If missing, AskUserQuestion** — let the user tell you or let gstack search the repo +3. **Persist the answer to CLAUDE.md** so we never have to ask again + +This applies to test commands, eval commands, deploy commands, and any other +project-specific behavior. The project owns its config; gstack reads it. + ## Writing SKILL templates SKILL.md.tmpl files are **prompt templates read by Claude**, not bash scripts. diff --git a/review/checklist.md b/review/checklist.md index 282c9944..bf38b72f 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -35,16 +35,16 @@ Be terse. For each issue: one line describing the problem, one line with the fix ### Pass 1 — CRITICAL #### SQL & Data Safety -- String interpolation in SQL (even if values are `.to_i`/`.to_f` — use `sanitize_sql_array` or Arel) +- String interpolation in SQL (even if values are `.to_i`/`.to_f` — use parameterized queries (Rails: sanitize_sql_array/Arel; Node: prepared statements; Python: parameterized queries)) - TOCTOU races: check-then-set patterns that should be atomic `WHERE` + `update_all` -- `update_column`/`update_columns` bypassing validations on fields that have or should have constraints -- N+1 queries: `.includes()` missing for associations used in loops/views (especially avatar, attachments) +- Bypassing model validations for direct DB writes (Rails: update_column; Django: QuerySet.update(); Prisma: raw queries) +- N+1 queries: Missing eager loading (Rails: .includes(); SQLAlchemy: joinedload(); Prisma: include) for associations used in loops/views #### Race Conditions & Concurrency -- Read-check-write without uniqueness constraint or `rescue RecordNotUnique; retry` (e.g., `where(hash:).first` then `save!` without handling concurrent insert) -- `find_or_create_by` on columns without unique DB index — concurrent calls can create duplicates +- Read-check-write without uniqueness constraint or catch duplicate key error and retry (e.g., `where(hash:).first` then `save!` without handling concurrent insert) +- find-or-create without unique DB index — concurrent calls can create duplicates - Status transitions that don't use atomic `WHERE old_status = ? UPDATE SET new_status` — concurrent updates can skip or double-apply transitions -- `html_safe` on user-controlled data (XSS) — check any `.html_safe`, `raw()`, or string interpolation into `html_safe` output +- Unsafe HTML rendering (Rails: .html_safe/raw(); React: dangerouslySetInnerHTML; Vue: v-html; Django: |safe/mark_safe) on user-controlled data (XSS) #### LLM Output Trust Boundary - LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting. @@ -141,7 +141,7 @@ the agent auto-fixes a finding or asks the user. ``` AUTO-FIX (agent fixes without asking): ASK (needs human judgment): ├─ Dead code / unused variables ├─ Security (auth, XSS, injection) -├─ N+1 queries (missing .includes()) ├─ Race conditions +├─ N+1 queries (missing eager loading) ├─ Race conditions ├─ Stale comments contradicting code ├─ Design decisions ├─ Magic numbers → named constants ├─ Large fixes (>20 lines) ├─ Missing LLM output validation ├─ Enum completeness