diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..66b3ee7 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,50 @@ +language: "ja-JP" + +reviews: + profile: assertive + auto_review: + enabled: true + # デフォルトはdraft PRをスキップするが、このリポジトリはDraftで + # Claude/Codexと反復してからReady化する運用(CLAUDE.md「ブランチとPR」)。 + # CodeRabbitはCopilotと違いdraft中も含めて都度レビュー対象にする + # (実際に消費するのはFreeプランのレート制限: GitHub連携のPRレビューは + # 1回/時/開発者。1時間に複数pushしても2回目以降はスキップされうる。 + # 詳細: https://docs.coderabbit.ai/management/plans) + drafts: true + # 既定値5だと5コミット分レビューした時点で自動レビューが止まり、以降は + # `@coderabbitai review` の手動実行が必要になる。pushのたびに反復する + # 運用と相性が悪いため無効化する。PR #35で7コミット分pushした後に + # `@coderabbitai review` を手動実行したところ、応答は「Review rate limited」 + # であり、「レビューがpauseされているので手動実行が必要」という案内には + # ならなかった(pause中ならその旨のメッセージが返る仕様)。つまりauto_pauseの + # 5コミット上限には引っかかっていない = 0が意図通り無効化として機能している + # と判断できる。実際にpushのたびのレビューが止まった原因はレート制限であり、 + # auto_pauseとは別物(詳細はdocs/roadmap.md「CodeRabbitの導入」参照)。 + auto_pause_after_reviewed_commits: 0 + path_instructions: + - path: "**/*.{ts,tsx}" + instructions: | + CLAUDE.md の絶対ルールに違反していないか重点的に確認すること。 + - `as` によるキャスト、`any`、`@ts-ignore` / `@ts-expect-error` / `eslint-disable` による + エラーの黙殺がないか + - common/ に置くべき判断ロジック(フィルタ・並び順・検証・集計・権限判定・日付計算)が + app/ や mcp/ に複製・直書きされていないか + - 型を手書きせず、生成型(supabase/types.ts)かZodスキーマの z.infer から導出しているか + - path: "supabase/migrations/**" + instructions: | + docs/data-model.md・docs/permissions.md の権限マトリクスと突き合わせて確認すること。 + - RLSポリシー内の自己参照サブクエリ(同一テーブルを問い合わせるEXISTS等)は + 呼び出し元のRLS越しに評価され、本来見えるべき行が見えず誤判定することがある + (security definer関数でラップする必要がないか確認する) + - security definer関数を使う場合、`set search_path = ''`または安全な固定値を + 必須にし、内部の参照はスキーマ修飾名(`public.xxx`)にしているか。 + `PUBLIC`ロールへの不要なEXECUTE権限が残っていないか(必要なロールにのみGRANTする) + - path: "test/db/**" + instructions: | + docs/testing.md・docs/permissions.md「RLS検証の必須要件」に沿っているか確認すること。 + - service_roleキーを使っていないか + - 否定側のテストが、対象行を見られない側から実行した結果(RETURNINGやSELECTが空) + だけで判定していないか。`WHERE`不一致で0件なのか、RLSで正しく弾かれて0件なのかは + その視点からは区別できない。UPDATEは対象行を見られる側の視点で値が変化していない + ことを、DELETEは行が存在し続けていることを、INSERTは対象行を見られる側の視点で + 新しい行が作られていないことを、それぞれ確認しているか(service_roleは使わない) diff --git a/CLAUDE.md b/CLAUDE.md index 072b0f0..fbc9c31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,9 +49,31 @@ **mainへの直接pushは禁止。**必ずブランチを作成し、PRを経由してmainにマージする。 mainはRulesetで保護されており、リポジトリ管理者(あなた)のみ緊急時にバイパスできる。 -- PRを作成すると、Claude(`claude-review.yml`)とCodex(`codex-review.yml`)による - 自動レビューが走る。シークレット未設定の間は自動でスキップされる - (`docs/roadmap.md`「保留: 外部アカウント待ち」) +**PRはまずDraftで作成する。** +Claude(`claude-review.yml`)は`CLAUDE_CODE_OAUTH_TOKEN`設定時にdraftのpushごとに走り、 +未設定時はスキップする。Codexは`OPENAI_API_KEY`設定時(`codex-review.yml`)、CodeRabbitは +`drafts: true`(`.coderabbit.yaml`)でdraft中もレビュー対象になるが、CodeRabbitはFreeプランの +レート制限を受ける(下記参照)。 +GitHub Copilotの自動レビュー(`copilot_code_review` Ruleset)は +`review_draft_pull_requests: false`に設定済みのためdraft中は走らない。 +Draftで指摘がなくなるまで反復し、`gh pr ready`でReady for reviewに +変えたタイミングでCopilotの最終レビューを1回受ける。Copilotは1レビューあたり +プレミアムリクエストを消費するため、Claude/Codex/CodeRabbitとの反復で消費しないように +するための運用(PR #18〜#32の実績分析に基づく判断)。 + +- CodeRabbitのFreeプランはGitHub連携のPRレビューが1回/時/開発者に制限されている。 + Draftで短時間に何度もpushしても2回目以降はレート制限でスキップされうる + (`docs/roadmap.md`「保留: 外部アカウント待ち」参照)。反復の主力はClaude/Codexで、 + CodeRabbitは取れたときに追加の視点が入る、という位置づけで期待値を持つこと + (PR #35で実際にレート制限を確認済み。詳細は`docs/roadmap.md`「CodeRabbitの導入」参照) +- Copilotの最終レビューは「プレミアムリクエストのquota上限に達したため実行できなかった」 + という形で失敗することがある(PR #35で発生)。この場合レビューコメントは投稿されるが + 中身のないもので、コードは実際にはレビューされていない。quotaを追加してから + `gh api repos/{owner}/{repo}/pulls/{number}/requested_reviewers -X POST + -f 'reviewers[]=copilot-pull-request-reviewer[bot]'` で再リクエストする +- Ready化後にCopilot/CodeRabbitの指摘で追加修正が発生した場合は、都度pushしてよい + (Ready後はpushのたびに再レビューが走る。ただしCodeRabbitは上記レート制限の対象) +- シークレット未設定の間、Codexは自動でスキップされる(`docs/roadmap.md`「保留: 外部アカウント待ち」) - 人間の承認レビューは必須にしていない(現状は開発者本人のみのため。GitHubは PR作成者自身の承認をカウントしない)。マージの実行自体が「人間の確認」に当たる (`docs/prd.md` 8.5) diff --git a/docs/permissions.md b/docs/permissions.md index 47f419c..a735656 100644 --- a/docs/permissions.md +++ b/docs/permissions.md @@ -75,14 +75,57 @@ service_roleはRLSをバイパスする。これで通ったテストは、**RLS 正常系だけのテストは、権限が全開放されていても通る。否定側が本体。 -``` +```typescript // これだけでは何も検証していない -expect(await asUserA.from("expenses").select()).toHaveLength(3); +const { data: dataA, error: errorA } = await asUserA.from("expenses").select(); +expect(errorA).toBeNull(); +expect(dataA).toHaveLength(3); // これが本体 -expect(await asUserB.from("expenses").select()).toHaveLength(0); +const { data: dataB, error: errorB } = await asUserB.from("expenses").select(); +expect(errorB).toBeNull(); +expect(dataB).toHaveLength(0); +``` + +**UPDATE/DELETEが「成功したかに見える」結果だけで判定しない。** +PostgreSQLのRLSでは、UPDATE/DELETEの対象行はまずコマンド自身のUSING句で絞り込まれる。 +USING句を満たさない行は**エラーなく静かに除外される**(候補行が単に0件になるだけ)。 +「他人の行を更新/削除しようとする」典型的な否定側テスト(例: `expenses_update_own`の +`using (user_id = auth.uid())`)はこのケースに該当し、`data`は空配列、`error`は`null`になる。 + +`UPDATE ... RETURNING`にはこれとは別の落とし穴もある。USING句を通過して実際に +更新された新しい行の内容がテーブルのSELECTポリシーを満たさない場合、PostgreSQL公式 +ドキュメント([CREATE POLICY](https://www.postgresql.org/docs/current/sql-createpolicy.html)) +の通り更新自体がエラーになる("inserted or updated rows to be returned are never +silently ignored")。単に「他人の行を更新しようとする」だけの否定側テストは、通常 +USING句の時点で候補から除外されるため、このエラーには到達しない。 + +つまり「RETURNINGが空で、エラーも出ない」という結果だけでは、(a) `WHERE`条件に一致する行が +最初から無かったのか、(b) USING句によって正しく弾かれたのか、を区別できない。 +UPDATE/DELETEを試みた後は、対象行を見られる側(本人など)の視点で、値が実際に +変化していないこと・行が存在し続けていることまで確認する。 + +```typescript +// 更新前に本人視点で元の値を控えておく +const { data: before, error: beforeError } = await asUserA.from("expenses").select("amount").eq("id", id).single(); +expect(beforeError).toBeNull(); + +// これだけでは何も検証していない(WHERE不一致で0件なのか、正しく弾かれて0件なのか区別できない) +const { data, error } = await asUserB.from("expenses").update({ amount: 1 }).eq("id", id).select(); +expect(error).toBeNull(); +expect(data).toHaveLength(0); + +// これが本体。本人(対象行を見られる側)の視点で値が実際に変化していないことを確認する +const { data: unchanged, error: unchangedError } = await asUserA.from("expenses").select("amount").eq("id", id).single(); +expect(unchangedError).toBeNull(); +expect(unchanged?.amount).toBe(before?.amount); ``` +**INSERTの`RETURNING`にも同じ注意が必要。** 招待のように自分以外のユーザーの行を作成する +操作では、作成した本人(自分)がその行をSELECTポリシー上見られないことがある。この場合 +`INSERT ... RETURNING` はINSERT自体が要件を満たしていてもRLS違反エラーを返す。 +`.select()` を付けずにINSERTするか、作成された行の中身は対象ユーザー自身の視点で確認すること。 + ### 3. マトリクスを表のままテストに写す 上の表の×が1つでもテストされていなければ、それは検証されていない権限とみなす。 diff --git a/docs/roadmap.md b/docs/roadmap.md index 8979a6b..3502b20 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -86,6 +86,7 @@ | Claudeレビューの本稼働化 | **完了。** `CLAUDE_CODE_OAUTH_TOKEN` シークレット追加 + GitHub App([github.com/apps/claude](https://github.com/apps/claude))インストール済み。PRへの総評+インラインコメント投稿を実PRで確認済み | | Copilot自動レビューの有効化 | **完了。** Copilot Proに加入し、Rulesetに `copilot_code_review` ルールを追加。実PRでのコメント投稿を確認済み | | Codexレビューの本稼働化 | **保留(意図的)。** OpenAI APIキーを取得しない方針のため、`codex-review.yml` はシークレット未設定のまま自動スキップし続ける。将来キーを取得したら `OPENAI_API_KEY` をリポジトリシークレットに追加するだけで動き出す | +| CodeRabbitの導入 | **完了。** PR #18〜#32の実績分析でClaude/Copilotの指摘重複率と、Copilotのクレジット消費(実測で1レビューあたりプレミアムリクエスト13回相当。公式の固定値ではなく実績値)を踏まえ、無料の3人目のレビュアーとして追加。GitHub Appをインストール済み。`.coderabbit.yaml`で`drafts: true`を設定し、Draft PRでも反復レビューされることをPR #35で確認済み(Copilotがdraft中は走らずReady化後に走るという挙動は、PR #35で実地確認済み。Draft中の7回のpushではCopilotのレビューは一度も付かず、`gh pr ready`実行後に`copilot-pull-request-reviewer`が動いた。ただし1回目はプレミアムリクエストのquota上限で失敗し、quota追加後に再リクエストして2回成功した。「1回だけ」という想定に反し、quota切れ時は失敗レビューがノーカウントで残る点は注意)。Freeプランのレート制限はGitHub連携のPRレビューが**1回/時/開発者**(IDE/CLIは3回/時)で、当初調べていた「200ファイル/時・4レビュー/時」は誤り(CodeRabbit自身のレビューコメントで訂正された。参照: [docs.coderabbit.ai/management/plans](https://docs.coderabbit.ai/management/plans))。短時間の連続pushでは2回目以降のレビューがスキップされうる前提で運用する。**注記:** PR #35のレビュー実行結果は毎回`Plan: Pro Plus`と表示されていたが、7コミット目のpush以降はレビューが自動発火せず、`@coderabbitai review`を手動実行すると「Review rate limited」と返ってきた(2026-08-07T00:55 UTC時点)。「レビューがpauseされている」旨の案内ではなかったため、`auto_pause_after_reviewed_commits: 0`は意図通り機能しており、止まった原因はレート制限だと判断できる。GitHub App導入直後のPro Plusトライアル期間が既に終了した可能性が高い。以降はFreeプランのレート制限(1回/時/開発者)を前提に運用する | > シークレットは `gh secret set ` などリポジトリの設定画面から**あなた自身が追加すること**。 > Claudeにトークン・APIキーの値を渡さない。