-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
chore: add .claude/REVIEW.md with CI drift check #3561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+132
−0
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
be84d0f
chore: add .claude/REVIEW.md with CI drift check
ericallam f1b3378
chore: switch drift check to claude-code-action
ericallam 0cbed6c
chore: drop /code-review reference from REVIEW.md preamble
ericallam 453aab5
chore: drop issues:write permission from drift audit workflow
ericallam File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # REVIEW.md — Trigger.dev OSS | ||
|
|
||
| Injected by `/code-review` as the highest-priority block in the reviewer's prompt. Repo-specific signal only; generic review rules live in the skill. | ||
|
|
||
| ## What makes a 🔴 Important finding here | ||
|
|
||
| Reserve 🔴 for things that would page someone or block a rollback. In this codebase, that means: | ||
|
|
||
| - **Rolling-deploy breakage.** Old and new versions of the webapp/supervisor run side-by-side during deploys. A change is broken if: | ||
| - A Lua script's behavior changes for a given key set without versioning (rename the script with a behavior-descriptive suffix like `Tracked` rather than `V2` — both versions must coexist safely). | ||
| - A Redis data shape used by both versions changes in place. New shapes need a new key namespace. | ||
| - A migration is not backward-compatible with the prior image. | ||
| - **Schema / migration safety.** Prisma migrations must be backward-compatible with the prior deploy. Adding NOT NULL without a default, dropping a column an old image still reads, renaming a column — all 🔴. | ||
| - **Queue / concurrency correctness.** RunQueue, MarQS (V1, legacy), redis-worker — any change to enqueue / dequeue / locking semantics. Re-derive the invariant on paper before flagging or accepting. | ||
| - **Missing index on a hot table.** New Prisma queries against `TaskRun`, `TaskRunExecutionSnapshot`, `JobRun`, `Project`, etc. must use an existing index. Check `internal-packages/database/prisma/schema.prisma` for the relevant `@@index` lines — don't guess and don't propose `EXPLAIN`. | ||
| - **Recovery-path queries.** Any `TaskRun.findFirst` / `findMany` added to a schedule, run-recovery, or restart loop. Recovery fan-outs (Redis crash, restart storms) turn "rare indexed query" into a DB incident. 🔴 even if indexed. | ||
| - **Aggregations on hot tables.** No `COUNT` / `GROUP BY` on `TaskRun` or other multi-million-row tables. Use Redis or ClickHouse for counts. | ||
| - **Prod Redis blast-radius.** New code paths that `SCAN` with broad patterns (`*foo*`) on prod-shaped Redis, or `EVAL` Lua with `SCAN` loops inside. Both are 🔴. | ||
| - **`@trigger.dev/core` direct import** from anywhere outside the SDK package. Always import from `@trigger.dev/sdk`. Core direct imports are 🔴 — they break the public API contract. | ||
| - **Heavy execute-deps imported into request-handler bundles.** Specifically `chat.handover` and similar split-bundle entry points must not transitively import the agent task's execute path. Watch for new imports added at module top-level of route files. | ||
| - **V1 engine code modified in a "V2 only" PR.** The `apps/webapp/app/v3/` directory contains both. If the PR description says V2-only but it touches `triggerTaskV1`, `cancelTaskRunV1`, `MarQS`, etc. — 🔴. | ||
|
|
||
| ## Always check | ||
|
|
||
| - **Tests use testcontainers, not mocks.** Vitest with `redisTest` / `postgresTest` / `containerTest` from `@internal/testcontainers`. Any new `vi.mock(...)` on Redis, Postgres, BullMQ, or other infra is wrong here — 🔴 if added in production-path tests, 🟡 if isolated unit test. | ||
| - **Public-package changes have a changeset.** `pnpm run changeset:add` produces `.changeset/*.md`. Required for any edit under `packages/*`. Missing → 🟡; missing on a breaking change → 🔴. | ||
| - **Server-only changes have `.server-changes/*.md`.** Required for `apps/webapp/`, `apps/supervisor/` edits with no public-package change. Body should be 1-2 sentences (it has to fit as one bullet in a future changelog). Missing → 🟡. | ||
| - **Lua script naming.** Coexisting scripts use behavior-descriptive suffixes (`Tracked`), never `V2`. Old name must keep working until the next deploy clears it. | ||
| - **RunQueue payload shape.** V2 run-queue payload's `projectId` is consumed by `workerQueueResolver` for override matching. If a PR drops it from the payload, 🔴. | ||
| - **`safeSend` scope.** Defensive IPC wrappers belong on loop / interval / handler contexts, not one-shot terminal sends. If the PR adds `safeSend` to a single terminal call for consistency, 🟡 with a "remove this" suggestion. | ||
| - **Zod version.** Pinned to `3.25.76` monorepo-wide. New package adding zod with a different version or range — 🔴. | ||
|
|
||
| ## Skip (do NOT flag) | ||
|
|
||
| - Anything Prettier / ESLint catches. CI runs both. | ||
| - TypeScript style preferences (`type` vs `interface`) — already covered by repo standards. | ||
| - Test coverage exhortations as a generic suggestion. Only flag missing tests when a specific code path is genuinely untested and the path has prior incidents. | ||
| - `agentcrumbs` markers (`// @crumbs`, `// #region @crumbs`) and `agentcrumbs` imports — these are temporary debug instrumentation stripped before merge. | ||
| - `// removed comments for removed code`, renamed `_unused` vars, re-exported types as "backwards compatibility shims" — also covered by repo standards. | ||
| - Suggestions to "add error handling" without naming a specific scenario that breaks. | ||
| - Documentation prose nitpicks in `docs/*` MDX files unless factually wrong. | ||
|
|
||
| ## Things V1/legacy that should NOT block a PR | ||
|
|
||
| The `apps/webapp/app/v3/` directory name is misleading — most code there is V2. Only specific files are V1-only legacy: `MarQS` queue, `triggerTaskV1`, `cancelTaskRunV1`, and a handful of others (see `apps/webapp/CLAUDE.md` for the exact list). Don't flag "you should refactor this to use V2" on those — they're frozen. | ||
|
|
||
| ## Confidence calibration for this repo | ||
|
|
||
| The most common false-positive pattern: speculating about race conditions in code paths the agent doesn't have runtime visibility into. If the only evidence is "this *could* race", drop it. If you can point to a specific interleaving with file:line for each step, surface it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| #!/usr/bin/env bash | ||
| # Check that paths and packages cited in .claude/REVIEW.md still exist. | ||
| # Exits 1 if any cited path/package is missing, 0 otherwise. | ||
| # Designed to be runnable locally and from CI. | ||
|
|
||
| set -uo pipefail | ||
|
|
||
| REVIEW="${1:-.claude/REVIEW.md}" | ||
| REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" | ||
| cd "$REPO_ROOT" | ||
|
|
||
| if [[ ! -f "$REVIEW" ]]; then | ||
| echo "No $REVIEW found — skipping check" | ||
| exit 0 | ||
| fi | ||
|
|
||
| declare -a ERRORS=() | ||
| declare -a WARNINGS=() | ||
|
|
||
| # Extract every backtick-quoted token. We deliberately ignore fenced code | ||
| # blocks (```...```) — those are illustrative examples, not citations to | ||
| # verify. | ||
| mapfile -t REFS < <( | ||
| awk ' | ||
| /^```/ { inblock = !inblock; next } | ||
| !inblock { | ||
| while (match($0, /`[^`]+`/)) { | ||
| print substr($0, RSTART+1, RLENGTH-2) | ||
| $0 = substr($0, RSTART+RLENGTH) | ||
| } | ||
| } | ||
| ' "$REVIEW" | sort -u | ||
| ) | ||
|
|
||
| is_path_like() { | ||
| local s="$1" | ||
| # Path-like: contains a slash, OR ends in a recognized file extension | ||
| case "$s" in | ||
| */*) return 0 ;; | ||
| *.ts|*.tsx|*.js|*.jsx|*.mjs|*.cjs|*.json|*.md|*.mdx|*.sql|*.prisma|*.lua|*.yml|*.yaml|*.sh|*.toml) return 0 ;; | ||
| *) return 1 ;; | ||
| esac | ||
| } | ||
|
|
||
| is_package_like() { | ||
| local s="$1" | ||
| [[ "$s" == @*/?* ]] | ||
| } | ||
|
|
||
| # Heuristic skips: things in backticks that aren't paths or packages. | ||
| # These are usually code snippets, function names, table names, etc. | ||
| # We do NOT check them — too many false positives. | ||
| should_skip() { | ||
| local s="$1" | ||
| # Slash-command form (starts with / and has no further /): e.g. /code-review | ||
| if [[ "$s" == /* && "$s" != /*/* ]]; then | ||
| return 0 | ||
| fi | ||
| case "$s" in | ||
| *" "*) return 0 ;; # contains space — prose | ||
| *"("*|*")"*) return 0 ;; # function call syntax | ||
| *"{"*|*"}"*) return 0 ;; | ||
| *"="*|*";"*) return 0 ;; | ||
| *"<"*|*">"*) return 0 ;; | ||
| *) return 1 ;; | ||
| esac | ||
| } | ||
|
|
||
| # Resolve glob-bearing paths to their longest static prefix dir. | ||
| # `packages/*` → `packages` | ||
| # `.changeset/*.md` → `.changeset` | ||
| # Pure paths return unchanged. | ||
| resolve_glob_prefix() { | ||
| local s="$1" | ||
| case "$s" in | ||
| *"*"*|*"?"*) | ||
| # Strip from the first segment containing a wildcard onward | ||
| printf '%s\n' "${s%%/\**}" | sed 's:/$::' | ||
| ;; | ||
|
ericallam marked this conversation as resolved.
Outdated
|
||
| *) | ||
| printf '%s\n' "$s" | ||
| ;; | ||
| esac | ||
| } | ||
|
|
||
| for ref in "${REFS[@]}"; do | ||
| # Strip leading/trailing punctuation that snuck through | ||
| ref="${ref#[(\[]}" | ||
| ref="${ref%[,.):\]]}" | ||
|
|
||
| [[ -z "$ref" ]] && continue | ||
| should_skip "$ref" && continue | ||
|
|
||
| if is_package_like "$ref"; then | ||
| # Check that any package.json in the repo declares this name | ||
| if ! grep -rqE "\"name\":[[:space:]]*\"${ref}\"" --include=package.json . 2>/dev/null; then | ||
| ERRORS+=("package not found in workspace: \`$ref\`") | ||
| fi | ||
| continue | ||
| fi | ||
|
|
||
| if is_path_like "$ref"; then | ||
| resolved="$(resolve_glob_prefix "$ref")" | ||
| case "$ref" in | ||
| */) | ||
| if [[ ! -d "${resolved%/}" ]]; then | ||
| ERRORS+=("directory missing: \`$ref\`") | ||
| fi | ||
| ;; | ||
| *) | ||
| # Accept file OR directory (some refs omit trailing slash) | ||
| if [[ ! -e "$resolved" ]]; then | ||
| ERRORS+=("path missing: \`$ref\`") | ||
| fi | ||
| ;; | ||
| esac | ||
| continue | ||
| fi | ||
|
|
||
| # Anything else: not checked. | ||
| done | ||
|
|
||
| if [[ ${#ERRORS[@]} -gt 0 ]]; then | ||
| echo "REVIEW.md cites paths/packages that no longer exist:" | ||
| printf ' - %s\n' "${ERRORS[@]}" | ||
| echo | ||
| echo "Either restore the referenced paths or update .claude/REVIEW.md." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ${#WARNINGS[@]} -gt 0 ]]; then | ||
| echo "Warnings:" | ||
| printf ' - %s\n' "${WARNINGS[@]}" | ||
| fi | ||
|
|
||
| echo "REVIEW.md OK — ${#REFS[@]} backtick refs scanned, all cited paths/packages exist." | ||
| exit 0 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| name: 🔎 REVIEW.md drift check | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - ".claude/REVIEW.md" | ||
| - ".claude/scripts/check-review-md.sh" | ||
| schedule: | ||
| # Mondays 09:00 UTC — catches drift introduced by merges that deleted | ||
| # paths referenced by REVIEW.md without updating it. | ||
| - cron: "0 9 * * 1" | ||
| workflow_dispatch: | ||
|
|
||
| concurrency: | ||
| group: check-review-md-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| check: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| issues: write | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - name: Run check | ||
| id: check | ||
| run: | | ||
| set +e | ||
| bash .claude/scripts/check-review-md.sh | tee check-output.txt | ||
| echo "exit_code=$?" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Open / update tracking issue (scheduled runs only) | ||
| if: github.event_name == 'schedule' && steps.check.outputs.exit_code != '0' | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| set -euo pipefail | ||
| TITLE="REVIEW.md is out of sync with the codebase" | ||
| BODY=$'The weekly drift check found stale references in `.claude/REVIEW.md`:\n\n```\n'"$(cat check-output.txt)"$'\n```\n\nFix by updating REVIEW.md to remove or correct the stale citations.' | ||
|
|
||
| EXISTING=$(gh issue list --label "review-md-drift" --state open --json number --jq '.[0].number // empty') | ||
| if [[ -n "$EXISTING" ]]; then | ||
| gh issue comment "$EXISTING" --body "$BODY" | ||
| else | ||
| gh label create "review-md-drift" --color "fbca04" --description "REVIEW.md drift detected by scheduled check" 2>/dev/null || true | ||
| gh issue create --title "$TITLE" --label "review-md-drift" --body "$BODY" | ||
| fi | ||
|
|
||
| - name: Fail PR if check failed | ||
| if: github.event_name == 'pull_request' && steps.check.outputs.exit_code != '0' | ||
| run: | | ||
| echo "REVIEW.md check failed. See log above." | ||
| exit 1 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.