層の境界(common/ lib/ app/ mcp/)をESLintで強制する - #60
Conversation
CLAUDE.md「ディレクトリ構成」が定める依存の向きを、no-restricted-imports と no-restricted-syntax で固定する。向きが逆でもtscは通りテストも緑になるため、 機械が止めないと誰も気づかない箇所だった。 - common/ は app/ lib/ mcp/ とフレームワーク・Supabaseへの依存を禁止 - lib/ は app/ mcp/ と、common/ の値としてのimportを禁止(import type は可) - app/ は mcp/ と @supabase/* を禁止 - mcp/ は app/ と next*/react*、@supabase/* を禁止 - app/ mcp/ では判断ロジックの実体である配列操作(filter/reduce/sort 等)を禁止。 .map() は描画のための変換として許可 app/ から lib/ へのimport自体は禁止しない。正当なI/O呼び出しであり、塞ぐと Supabaseクライアントを直接握ったクエリが app/ に生えるだけになるため。 禁止したいのは「importすること」ではなく「結果を使って判断すること」なので、 そちらは no-restricted-syntax で押さえる。 導入時点で app/ はNext.jsの雛形のみ、lib/ と mcp/ は空。違反0件のため drainなしで最初からerrorで入れた。 Closes #43 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
Changes層境界Lint
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const WEB_FRAMEWORK_PACKAGES = [ | ||
| "next", | ||
| "next/*", | ||
| "next/**", | ||
| "react", | ||
| "react/*", | ||
| "react-dom", | ||
| "react-dom/*", | ||
| ]; |
There was a problem hiding this comment.
WEB_FRAMEWORK_PACKAGES は next / next/* / next/** / react / react/* / react-dom / react-dom/* のみで、これは「完全一致 or / の後のワイルドカード」しか捕まえません。docs/lint-policy.md の表とこのファイルのコメント(L27, L44-45)はどちらも「next* react* を禁止」と書いていますが、実際のパターンにはハイフン直後に続く形(next-auth next-themes next-seo react-hook-form react-icons react-query 等)にマッチするワイルドカードがありません。
例えば common/ から import { useForm } from "react-hook-form" を書いても、このルールでは検出できません(react/* は react の直後が / である場合にしかマッチしないため)。
現状の依存関係にはこれらのパッケージは無いので今すぐ壊れるわけではありませんが、Phase1でフォーム系ライブラリ等が追加された瞬間に静かに抜け道になります。next* react* の意図どおりにするなら "next-*" "react-*" のようなパターンを追加するか、ドキュメントの表現を実装に合わせて「next / next/** 配下のみ」に修正しておくべきだと思います。
| // 内部モジュールは `@/` エイリアスで書く規約だが、相対パス(`../../lib/x`)で書けば | ||
| // すり抜けられてしまうので、両方の表記を並べる。npmパッケージの深いパス | ||
| // (`somepkg/lib/...`)を巻き込まないよう `**/lib/**` のような広いパターンは使わない。 | ||
| const RELATIVE_DEPTHS = [1, 2, 3, 4, 5]; |
There was a problem hiding this comment.
RELATIVE_DEPTHS が [1, 2, 3, 4, 5] で打ち切られているので、@/ エイリアスを使わずに6階層以上深い相対パス(例: ../../../../../../lib/x)で層を越えるimportを書くと、このルールをすり抜けます。
Next.js App Routerはルートグループ・動的セグメント・ネストしたlayoutで app/ 配下が5階層を超えるのは珍しくありません(例: app/(dashboard)/events/[eventId]/participants/[participantId]/edit/page.tsx はこの時点で6階層)。そこから相対パスで mcp/ や @supabase/* を叩けば検出されないことになります。
PR本文では「相対は5階層まで」と実装上の割り切りとして触れられていますが、docs/lint-policy.mdの「残っているギャップ」にはlib/の.reduce()の件だけが明記されていて、この上限のことは書かれていません。深さの上限を持つ設計だと今後気づかれにくいので、docs/lint-policy.mdにも既知のギャップとして明記するか、RELATIVE_DEPTHSの列挙をやめて"../**/lib" / "../**/lib/**"のような深さに依存しないパターンに置き換えることを検討したほうが良さそうです(相対パスは必ず../または./から始まるので、npmパッケージの深いパスと誤検知することもないはずです)。
レビュー総評
CLAUDE.md / docs方針との整合性
気になった点(インラインコメント参照)
どちらも現状のリポジトリ構成(ファイル数が少ない)では今すぐ壊れる話ではなく、将来コードが増えたときに気づかれにくい形で抜け道になるという指摘です。マージのブロッカーというよりは、 その他
|
1. next-* / react-* 系のパッケージが抜けていた。`next/**` `react/**` は スラッシュ以降しかマッチしないため、`next-auth` `react-hook-form` を common/ や mcp/ からimportしてもすり抜けていた。 2. 相対パスの検出に階層数の上限があった。`../` の個数を1〜5で列挙していたため、 6階層以上深いファイルからの層越えを検出できなかった。App Routerのルート木は 5階層を簡単に超える。`../**/lib` の形にして深さ非依存にした。 合わせて docs/lint-policy.md の表記を実装に合わせ、「残っているギャップ」に 層と同名サブディレクトリの誤検知と、no-restricted-syntax がメソッド名しか 見ていない点を追記した。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
レビュー指摘の分類 (1巡目: Claude)CodeRabbitは1巡目がレート制限で走らなかった(
どちらも「今は壊れないが、コードが増えたときに静かに抜ける」種類の指摘で、まさにこのPRが 修正後に再検証したこと追加したパターンが効いているか、壊して確認した。
削除後 「静的解析で拾えたはずか」の自問 (
|
| "no-restricted-syntax": [ | ||
| "error", | ||
| ...JUDGEMENT_ARRAY_METHODS.map((method) => ({ | ||
| selector: `CallExpression[callee.type="MemberExpression"][callee.property.name="${method}"]`, |
There was a problem hiding this comment.
Nitpick（今回のスコープ外で構いません）: このセレクタは callee.property.name を見ているため、ブラケット記法 arr["filter"](fn) / arr["re" + "duce"](fn) は素通りします。docs/lint-policy.md「残っているギャップ」には「メソッド名しか見ていない」ことによる誤検知（配列でないオブジェクトの .find()）は書かれていますが、このすり抜け（ブラケット記法での回避）は書かれていません。他の抜け道（next/headers、相対パスの階層数）をここまで丁寧に塞いだPRなので、ギャップとして一行足しておくと将来「なぜ通った」で悩まずに済むかもしれません。ブロッキングではありません。
|
レビュー結果 CLAUDE.md / docs/lint-policy.md / docs/decision-policy.md の観点で確認しました。ブロッキングな指摘はありません。 良かった点
指摘した観点との照合
軽微な指摘（inline済み、ブロッキングではない）
総評 lint設定のみの変更で、アプリケーションの挙動には触れていません。層の境界という「機械が最も止めやすいのに今まで止めていなかった」ルールを、抜け道の検証込みで丁寧に導入しており、CLAUDE.md の「これは機械が止められるか？」という原則にまっすぐ沿ったPRです。承認できる状態だと思います。 |
Claudeレビューのnitpick対応。誤検知(配列でないオブジェクトの .find())は 書いていたが、逆方向のすり抜け(rows["filter"](fn))が抜けていた。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eslint.config.mjs`:
- Around line 111-126: Extend the ESLint configuration’s import restrictions to
cover dynamic import() expressions, not only the existing no-restricted-imports
rules. Add a no-restricted-syntax pattern or suitable dedicated rule targeting
ImportExpression that enforces the same layer and Supabase constraints for both
`@/` aliases and relative paths, reusing the existing layer(),
SUPABASE_CLIENT_PACKAGES, WEB_FRAMEWORK_PACKAGES, and MESSAGES.commonPure
symbols.
- Around line 109-110: eslint.config.mjs の層境界設定で、共通ヘルパーに .ts・.tsx・.mts
を許可拡張子として集約し、各拡張子の fixture を追加して同じ境界ルールを検証してください。no-restricted-imports が動的
import() を検査しないため、動的 import を許可する設定では別の構文ルールにも同一の層境界制約を適用してください。
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5081863-8721-44fc-b8a6-05fcf8e1163a
📒 Files selected for processing (3)
docs/lint-policy.mddocs/roadmap.mdeslint.config.mjs
| "reduceRight", | ||
| ]; | ||
|
|
||
| const layerBoundaries = [ |
There was a problem hiding this comment.
no-restricted-imports はコアの ImportDeclaration / re-export 系構文しか見ておらず、動的な import() 式(ImportExpression)は対象外のはずです。そのため common/ から await import("react")、app/ から await import("@/mcp/...") のような書き方をすると、ここで禁止しているはずの層越え・フレームワーク依存がそのまま素通りします。
rows["filter"](fn) のようなブラケット記法のすり抜け(docs/lint-policy.md「残っているギャップ」に追記済み)は意図的な回避行為ですが、動的importはNext.jsのコード分割で自然に出てくる書き方なので、うっかり踏み抜く可能性がブラケット記法より高いと思います。
ImportExpression を対象にした no-restricted-syntax の追加を検討するか、最低限このPRが明記している他の既知の穴と同様に docs/lint-policy.md「残っているギャップ」へ一行追記しておくと、次にここを触る人が気づけると思います。マージのブロッカーではありません。
|
|
||
| const layerBoundaries = [ | ||
| { | ||
| files: ["common/**/*.ts", "common/**/*.tsx"], |
There was a problem hiding this comment.
軽微ですが、層境界の files パターンが *.ts / *.tsx のみで *.mts を含んでいません(lib/ app/ mcp/ の各ブロックも同様)。一方、型情報を要する4ルールのブロック(216行目付近)は "**/*.mts" も対象にしています。現状 common/ lib/ app/ mcp/ 配下に .mts ファイルは無く今すぐ壊れる話ではありませんが、非対称なので気になりました。
レビュー総評
観点別の確認結果
設計そのもの( 10パターンの手動break-and-fix検証(壊してlintが赤くなることを確認してから戻す)がPR本文に記録されており、CIも全緑です。過去2巡のレビューで指摘された 気になった点(インラインコメント参照)
総評lint設定のみの変更で、アプリケーションの挙動には触れていません。CLAUDE.mdの「これは機械が止められるか?」という原則に沿って層境界を機械化しており、レビューサイクルの中で実際の抜け穴(next-/react-, 相対パスの階層数上限)を2件潰した上での状態です。動的importの穴以外にブロッキングな指摘はなく、承認できる水準だと思います。 |
CodeRabbitとClaudeの双方から指摘された2件。
1. no-restricted-imports は静的なimport宣言しか見ないため、
`await import("react")` で全部すり抜けていた。同じ制約を
no-restricted-syntax の ImportExpression セレクタにも掛けた。
グロブと正規表現を対で管理する必要があるので、その旨を明記した。
2. files が *.ts / *.tsx だけで .mts .cts 素のJSが対象外だった。
拡張子リストを1か所に置いて組み立てるようにした。
検証中に、.mts ファイルを置くと yarn lint がクラッシュする問題が
main から存在することが判明した。層の境界とは別件なので issue #61 に
切り出し、docs/lint-policy.md の「残っているギャップ」に記載した。
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
レビュー指摘の分類 (2巡目: Claude / CodeRabbit)CodeRabbitは3回目のpushでようやくレート制限が解けて走った。両者の指摘は独立に同じ2点へ収束した。
指摘1は特に妥当だった。Next.jsはコード分割で動的importが自然に出てくるので、 なお動的import側はグロブではなく正規表現で書くことになるため、**グロブ側と正規表現側で 壊して確認したこと (追加分)
削除後 スコープ外として分離したもの検証中に、
「静的解析で拾えたはずか」の自問**該当しない。**1〜3はいずれも「ESLint設定自身の網羅性」に関する指摘で、 指摘が尽きたと判断したので、Ready化してCopilotの最終レビューを受ける。 |
| const JUDGEMENT_ARRAY_METHODS = [ | ||
| "filter", | ||
| "find", | ||
| "findIndex", | ||
| "findLast", | ||
| "findLastIndex", | ||
| "flatMap", | ||
| "every", | ||
| "some", | ||
| "sort", | ||
| "toSorted", | ||
| "reduce", | ||
| "reduceRight", | ||
| ]; |
There was a problem hiding this comment.
JUDGEMENT_ARRAY_METHODS に includes(および indexOf)が入っていません。
このPRの狙いは「app/・mcp/ が lib/ の結果を使ってその場で判断してしまう」ことを機械的に止めることですが、ALLOWED_IDS.includes(userId) のようなメンバーシップ判定による権限判定は、意味的には some(id => id === userId) と同じ判断ロジックです。後者はこのリストで捕まりますが、前者はすり抜けます。.includes() は配列の真偽値判定として .some() よりむしろ書かれやすい書き方なので、抜け穴として踏まれやすいと思います。
docs/lint-policy.mdの「残っているギャップ」節には、ブラケット記法・動的メソッド名・.mtsのクラッシュ・同名サブディレクトリの誤検知は明記されていますが、この「対象メソッド一覧そのものの抜け」には触れられていません。意図的に対象外にしたのであれば理由をコメントかドキュメントに残し、見落としであればリストに追加を検討してください。
レビュー結果このPRは 観点別チェック
設計の妥当性
指摘1点、インラインコメントで指摘しました。 それ以外に、方針・実装上のブロッカーは見つかりませんでした。 |
Claudeレビューの指摘。ALLOWED_IDS.includes(userId) は ALLOWED_IDS.some((id) => id === userId) と意味的に同じ権限判定なので、 some だけ止めても書きやすいほうへ逃げられて抜け道になる。 文字列の .includes() まで巻き込む誤検知は承知の上。 「厳しすぎて例外が出る」ほうが「判断が静かに app/ に残る」より 戻しやすいという非対称性で判断し、その理由を docs/lint-policy.md の「残っているギャップ」に明記した。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
レビュー指摘の分類 (3巡目: Claude)
「意図的な対象外か見落としか」という問いへの回答: 見落とし。 承知の上で受け入れた誤検知
それでも対象に含めたのは非対称性による。「厳しすぎて例外が出る」は例外リストに 確認
指摘が尽きたので、次で Ready 化して Copilot の最終レビューを受ける。 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eslint.config.mjs`:
- Around line 105-106:
ESLint設定の回帰をfixtureテストで追加し、app/およびmcpのコードで.includes()と.indexOf()が検出されることを固定してください。.map()は許可されるfixtureも追加し、文字列に対する.includes()と.indexOf()は意図した誤検知として検出されるケースも含めてください。
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 088ab5f7-8fe2-4164-a676-c399076e4ced
📒 Files selected for processing (2)
docs/lint-policy.mdeslint.config.mjs
| "includes", | ||
| "indexOf", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n \
--glob '*.{test,spec}.{js,mjs,cjs,ts,tsx,mts,cts}' \
'RuleTester|no-restricted-syntax|includes|indexOf|\.map\(' .Repository: reitojike/oshikatsu-manager
Length of output: 165
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | sed -n '1,160p'
printf '%s\n' '--- eslint config outline and relevant section ---'
ast-grep outline eslint.config.mjs 2>/dev/null || true
sed -n '70,135p' eslint.config.mjs
printf '%s\n' '--- package scripts and test configuration references ---'
if [ -f package.json ]; then
node - <<'JS'
const p = JSON.parse(require('fs').readFileSync('package.json', 'utf8'));
console.log(JSON.stringify({scripts: p.scripts, devDependencies: p.devDependencies, dependencies: p.dependencies}, null, 2));
JS
fi
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
'RuleTester|fixture|fixtures|eslint|no-restricted-syntax|includes|indexOf|\.map\(' . \
| sed -n '1,260p'Repository: reitojike/oshikatsu-manager
Length of output: 22915
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- eslint rule application ---'
sed -n '145,252p' eslint.config.mjs
printf '%s\n' '--- lint policy ---'
sed -n '100,190p' docs/lint-policy.md
printf '%s\n' '--- test setup ---'
cat -n vitest.config.ts
sed -n '1,180p' docs/testing.md
printf '%s\n' '--- tracked test files ---'
git ls-files 'test/**' | sort
printf '%s\n' '--- current state ---'
git status --short
if [ -d node_modules ]; then
printf '%s\n' 'node_modules: present'
node -e 'console.log(require.resolve("eslint"))' 2>&1 || true
else
printf '%s\n' 'node_modules: absent'
fiRepository: reitojike/oshikatsu-manager
Length of output: 10725
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node - <<'JS'
const { Linter } = require("eslint");
const methods = [
"filter", "find", "findIndex", "findLast", "findLastIndex",
"flatMap", "every", "some", "includes", "indexOf", "sort",
"toSorted", "reduce", "reduceRight",
];
const linter = new Linter({ configType: "flat" });
const config = [{
languageOptions: {
ecmaVersion: 2022,
sourceType: "module",
},
rules: {
"no-restricted-syntax": [
"error",
...methods.map((method) => ({
selector: `CallExpression[callee.type="MemberExpression"][callee.property.name="${method}"]`,
message: `.${method}() restricted`,
})),
],
},
}];
const cases = {
arrayIncludes: "const ok = ids.includes(userId);",
arrayIndexOf: "const ok = ids.indexOf(userId);",
stringIncludes: 'const ok = pathname.includes("/events");',
stringIndexOf: 'const ok = pathname.indexOf("/events");',
mapAllowed: "const ok = rows.map((row) => row.name);",
};
for (const [name, code] of Object.entries(cases)) {
const messages = linter.verify(code, config, { filename: "app/example.js" });
console.log(name, JSON.stringify(messages.map(({ ruleId, message, line }) => ({ ruleId, message, line }))));
}
JSRepository: reitojike/oshikatsu-manager
Length of output: 553
ESLint設定の回帰をfixtureテストで固定してください。
app/ と mcp/ で .includes() と .indexOf() を検出し、.map() を許可するケースを追加してください。文字列の .includes() と .indexOf() も意図した誤検知として固定してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@eslint.config.mjs` around lines 105 - 106,
ESLint設定の回帰をfixtureテストで追加し、app/およびmcpのコードで.includes()と.indexOf()が検出されることを固定してください。.map()は許可されるfixtureも追加し、文字列に対する.includes()と.indexOf()は意図した誤検知として検出されるケースも含めてください。
| "every", | ||
| "some", | ||
| "includes", | ||
| "indexOf", |
There was a problem hiding this comment.
nitpick: docs/testing.md「新しいテストは、対象を壊したときに赤くなるかを確認する」/ PR本文の「壊して確認した(完了条件)」表は、最初のコミット時点の10パターン(.filter() .reduce() 等の静的検出)のみを記録しています。
その後の2つのフォローアップコミット
動的import()と.mts/.cts/JSを層の境界の対象に含めるincludes / indexOf も判断ロジックの対象に含める(このブロック)
で検出範囲を広げていますが、PR本文の表はこれらに対応する行(例: app/ で await import("@/mcp/...")、app/ で ALLOWED_IDS.includes(userId))を追加していないようです。コード自体は読んだ限り正しく見えますが、このPRが完了条件として掲げている「壊して赤くなることを確認」という自己申告の検証がこの2件については記録に残っていません。表を追記するか、確認済みであればその旨をPRコメントに残すことをお勧めします。
レビュー結果
確認した観点
設計・実装の妥当性
指摘(1件、nitpick)インラインコメントの通り、フォローアップ2コミット(動的 |
レビュー指摘の分類 (4巡目: Claude / CodeRabbit)
指摘1は的確だった。個々の検証は各巡のコメントに書いていたが、PR本文が初回の状態で 指摘2は初回のPR本文「見送ったもの」で既に理由を書いたもので、判断は変えない。
「入れない」ではなく「別途方針として決める」なので、必要になったらIssueを立てる。 指摘が出尽くしたので Ready 化し、Copilot の最終レビューを受ける。 |
There was a problem hiding this comment.
Pull request overview
ESLintのルールで common/ / lib/ / app/ / mcp/ の層境界を機械的に強制し、層越えimportや app/・mcp/ での判断ロジック（配列操作）をCIで検出できるようにするPR。
Changes:
eslint.config.mjsに層境界の制約（no-restricted-imports/no-restricted-syntax）を追加docs/lint-policy.mdに層境界ルールの全体像・理由・落とし穴・残ギャップを追記docs/roadmap.mdのフェーズ3注意点を「ESLintで機械化済み」に更新
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| eslint.config.mjs | 層境界のimport制約と、app/・mcp/での判断ロジック（配列操作）禁止をESLintで強制 |
| docs/roadmap.md | フェーズ3の注意点を、層境界のESLint機械化に合わせて更新 |
| docs/lint-policy.md | 層境界ポリシーの仕様・理由・設定注意点・残ギャップを文書化 |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - `app/` に判断ロジックを書かない。`common/` を経由しているか、コンポーネントごとに確認する | ||
| - これを人力の注意力に頼らないため、**`app/` から `lib/` のクエリ結果を直接フィルタ・集計する | ||
| コードが書かれていないかを、レビュー時のチェック項目として明文化する** | ||
| (ESLintの `no-restricted-imports` 等で機械化できないか、このフェーズで検討する) | ||
| - これは人力の注意力に頼らず、**ESLintで機械化済み**(issue #43。フェーズ0で先行導入した)。 |
Copilotレビューの指摘。「コンポーネントごとに確認する」(手動)と 「人力に頼らずESLintで機械化済み」が別々の箇条書きに残っていて、 どちらが方針か読み取れなかった。1つにまとめた。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
レビュー指摘の分類 (5巡目: GitHub Copilot / Ready後の最終レビュー)
これは的確な指摘だった。既存の行を消さずに機械化の説明を足したため、 Copilot再レビューは行わない
CIとClaude/CodeRabbitは通常どおり走るので、機械的なバックストップは失われていない。 全5巡の総括
**誤検知ゼロ、本物の修正6件。**うち5件が「層を機械化するPR自身に残っていた層の抜け穴」 CI全緑を確認のうえマージする。 |
| const WEB_FRAMEWORK_PACKAGES = [ | ||
| "next", | ||
| "next/**", | ||
| "next-*", | ||
| "next-*/**", | ||
| "react", | ||
| "react/**", | ||
| "react-*", | ||
| "react-*/**", | ||
| ]; |
There was a problem hiding this comment.
nit: WEB_FRAMEWORK_PACKAGES / WEB_FRAMEWORK_MODULE_REGEX (L127) still only cover unscoped next/react specifiers. Scoped packages in the same ecosystem — e.g. @next/font, @next/bundle-analyzer, @next/env — don't start with next or react, so common/ or mcp/ could import ... from "@next/env" (or await import("@next/env")) without tripping either rule. Same category of gap as the next-*/react-* hyphenated-package fix already made in this PR (docs/lint-policy.md「設定を書くときの落とし穴」) — worth either extending the pattern to @next/**/@react-*/** or adding one line to「残っているギャップ」so it's a documented, not silent, gap. Not a blocker; this PR has already been through 5 review rounds and closed every other package-name hole found.
レビュー総評
このPRは既に5巡のレビュー(Claude×4, CodeRabbit, Copilot)を経ており、 CLAUDE.md / docs方針との整合性
設計・実装
指摘(nit、インライン参照)
総評CIは全緑(lint / typecheck / unit-test / db-test)。設定のみの変更で製品挙動への影響なし。ブロッキングな指摘はありません。承認できる水準だと思います。 |
Claudeレビューの指摘。@next/env や @react-three/fiber は next / react で 始まらないため、既に直したハイフン系(next-auth 等)と同じ理由で common/ や mcp/ から素通りしていた。 グロブ側に @next/* @next/** @react-*/* @react-*/** を追加し、 動的import側の正規表現も同じ範囲を指すよう対で直した。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
レビュー指摘の分類 (6巡目: Claude)
指摘のとおり、既に直したハイフン系( 確認
Copilotに手動で再レビューを依頼する
|
レビュー総評このPR自体は
その上でESLint設定本体を精査しました。 良い点
指摘 インラインコメントを1件投稿しました。 🤖 Generated with Claude Code |
| { | ||
| patterns: [ | ||
| { group: SUPABASE_CLIENT_PACKAGES, message: MESSAGES.noSupabaseOutsideLib }, | ||
| { group: WEB_FRAMEWORK_PACKAGES, message: MESSAGES.mcpNoFramework }, |
There was a problem hiding this comment.
mcp/ のNext.js/React依存禁止に、lib/ 経由の間接依存が抜けています。
このルールが検出できるのは mcp/ 配下のファイルに直接書かれたimport文だけです。一方 lib/ 層(layerFiles("lib") のルールブロック、eslint.config.mjs内)にはフレームワーク系パッケージの制約が一切掛かっておらず、mcp/ から lib/ へのimportは(意図的に)許可されています。
このため、lib/ 内のあるファイルが Supabase の Server Client を組み立てる定番の書き方(next/headers の cookies() を使うパターン)に依存していた場合、mcp/ がそのファイルを許可された経路で import するだけで、mcp/ プロセスがNext.js依存を静かに引きずり込みます。この経路はどちらの no-restricted-imports/no-restricted-syntax 設定でも検出できません。
CLAUDE.mdがこの禁止importの根拠として挙げている「stdioサーバーがNext.jsを丸ごと読み込むのを防ぐ」(MESSAGES.mcpNoFramework、docs/lint-policy.md の該当表)は、lib/ 経由の間接依存に対しては実現できていない状態です。
現時点では mcp/ が空(.gitkeepのみ)のため実害はありませんが、フェーズ5で mcp/ が lib/ の関数を呼び始めたときに、lintは緑のまま実行時エラーとして初めて顕在化します。docs/lint-policy.md「残っているギャップ」には他の既知の限界(lib/に構文ルールが無い、同名サブディレクトリの誤検知、.mtsのクラッシュ等)が丁寧に列挙されていますが、この間接依存のギャップは記載がありません。
「残っているギャップ」への追記、もしくは lib/ 側でNext専用APIを使うファイルを命名規則等で分離して mcp/ からの誤importに気づきやすくする設計を検討することを提案します。
Claudeレビューの指摘。no-restricted-imports は依存グラフを辿らないため、 lib/ のファイルが next/headers に依存していると、mcp/ がそれを許可された 経路でimportするだけでNext.jsが入る。lintは緑のままフェーズ5で 実行時に顕在化する。 lib/ 側にフレームワーク制約を掛けることはできない(Supabaseの サーバークライアントが cookies() を使うため)ので、対処は 「Next.js専用APIに触れるファイルをパスで分離し、mcp/ からそのパスへの importを禁止する」になる。lib/ が空の今は決められないので、 フェーズ3のチェックリストに作業として書いた。 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
レビュー指摘の分類 (7巡目: Claude / Copilot再レビュー)Copilot(手動再リクエスト、1回): Claude: 1件。
指摘のとおりで、しかもこのPRの仕組みでは原理的に塞げない。 なので設計で対処するしかない。Next.js専用APIに触れるファイルをパスで分離し
これで「lintが緑だから安全」という誤認は起きない。 収束したのでマージする
|
レビュー結果CLAUDE.md / docs(prd, data-model, permissions, lint-policy, testing, decision-policy)の方針に沿って確認しました。今回のPRは 確認した観点と結果
実装内容そのものについて
インラインコメントは指摘なしのため投稿していません。 |
PR #68 のClaude Reviewの指摘に対応。指摘は正しかった。 誤り: 「300行を超えた5本」は事実と異なり、素の差分では9本だった (#31 307 / #59 365 / #60 380 / #30 441 / #29 617 / #18 679 / #41 698 / #32 1063 / #16 3528)。同じ本文が書いていた「300行以下が65%(17/26)」は 超過9本を含意しており、記述が自己矛盾していた。 原因: 母集団の統計(中央値・65%)は素の差分で数え、外れ値の説明だけに 除外規則を適用していた。数え方を混在させたうえ、超過リストを上位5本で 打ち切って全件確認しなかった。 訂正: 26本すべてを git show --numstat で数え直し、除外の段階ごとに 表で示す。素の差分(中央値135行 / 65% / 超過9本)、パス名で機械的に 判定できる除外まで(中央値128行 / 85% / 超過4本)、除外規則を最後まで 適用(超過2本 = #59・#60)。落ちる7本の内訳も明記した。 あわせて、3段階目の中央値を出さない理由を書いた。supabase/config.toml は #29 では supabase init の出力(416行)、#56 では根拠コメント付きで手で 直した6行で、同じパスでも扱いが逆になる。パス名では決まらないことが、 この節が機械的ゲートになり得ない理由そのものなので、「lintではない」の 段落の根拠もこの実測に差し替えた。 Refs #44
* docs: Issueの粒度とPR差分サイズの目安をCLAUDE.mdに明文化 判断ポイントは1 Issueに3個まで(5個超で分割)、PR差分は300行を目安とする。 記事の実測値をそのまま採らず、main にマージ済みのPR 26本(中央値135行、 300行以下65%)で裏を取ってから採用した。行数の数え方から生成物・ロック ファイル・権限マトリクスを写したテスト表を除外する根拠も、超過した PR #16 / #32 の実態から示した。 機械的ゲートにしない旨と、3回ルール(モデルを上げる) / PO確認(判断を 下せる層に上げる) / 粒度超過(Issueを分ける)の対処の違いを表で整理。 docs/roadmap.md はポインタ1行に留め、根拠は1箇所にだけ置く。 Refs #44 * docs: 判断ポイント数の境界を一本化し、裏取り済みの数値と外部実測を書き分ける PR #68 のClaude Reviewの指摘2件に対応。 指摘1: 「3個まで、5個を超えるなら分割」で4個の扱いが未定義だった。 閾値を「3個まで。4個目が出てきたら分ける」に一本化する。機械的ゲートに しない方針である以上、「検討」と「必ず」の二段構えは実効性のない false precisionになるため、緩衝域を作らず単一の線にした。 指摘2: 裏取り済みの300行と、外部実測のままの3個が同じ文脈に並んでいた。 「2つの数字は裏付けの強さが違う」として段落を分け、300行はこのリポジトリの 実測(PR 26本、中央値135行/300行以下65%)で検証済み、3個は外部実測のみを 根拠とする未検証のヒューリスティックであると明示した。過去Issueの判断数は 記録がなく後から数え直せないため、このリポジトリでの裏取りが今はできない 理由も併記。採用の根拠はコストの非対称性に置いた。 あわせて、外部実測に対応値のない4個/5個を推定して線を引いていないことと、 実績が溜まったら見直す旨を記載した。 Refs #44 * docs: PR実測の集計を数え直し、超過本数の誤りを訂正する PR #68 のClaude Reviewの指摘に対応。指摘は正しかった。 誤り: 「300行を超えた5本」は事実と異なり、素の差分では9本だった (#31 307 / #59 365 / #60 380 / #30 441 / #29 617 / #18 679 / #41 698 / #32 1063 / #16 3528)。同じ本文が書いていた「300行以下が65%(17/26)」は 超過9本を含意しており、記述が自己矛盾していた。 原因: 母集団の統計(中央値・65%)は素の差分で数え、外れ値の説明だけに 除外規則を適用していた。数え方を混在させたうえ、超過リストを上位5本で 打ち切って全件確認しなかった。 訂正: 26本すべてを git show --numstat で数え直し、除外の段階ごとに 表で示す。素の差分(中央値135行 / 65% / 超過9本)、パス名で機械的に 判定できる除外まで(中央値128行 / 85% / 超過4本)、除外規則を最後まで 適用(超過2本 = #59・#60)。落ちる7本の内訳も明記した。 あわせて、3段階目の中央値を出さない理由を書いた。supabase/config.toml は #29 では supabase init の出力(416行)、#56 では根拠コメント付きで手で 直した6行で、同じパスでも扱いが逆になる。パス名では決まらないことが、 この節が機械的ゲートになり得ない理由そのものなので、「lintではない」の 段落の根拠もこの実測に差し替えた。 Refs #44 * docs: PR #29/#32の除外理由が2段階なのに1段階しか書いていなかった記述漏れを修正 Claude Reviewの指摘どおり、#29(617→22)はsupabase/config.toml(416行) だけでなくsupabase/types.ts(179行)も、#32(1063→79)はテスト表(924行) だけでなくyarn.lock(60行)も除外して初めて数字が再現できる。 片方しか書いていなかったため、追試すると数値が合わなかった。 Refs #44
* レビュー指摘の重複による修正ラリーを減らす 同一根拠の指摘がファイルごとに個別投稿され、修正→push→再指摘のラウンドが かさむ問題(PR #32, #60, #63, #73)への対処。指摘を直す側には横展開確認の 規律を、レビューボット側には同一根拠の指摘を1件に集約する指示を追加する。 Codexレビュー本格導入前の準備。 Closes #79 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Copilotの指摘反映: PR #32/#60の例が読点で連結され読みにくい問題を修正 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Closes #43
PO確認ゲートの判定 (
docs/decision-policy.md)確認不要と判定し、そのまま実装した。
docs/roadmap.mdフェーズ3が「検討する」、
docs/lint-policy.mdに層の節が無いだけで、両者は両立している)、外から観測できる製品の挙動(権限の可否・既定値・保存されるデータ・画面表示)は動かない。
docs/prd.md/docs/data-model.md/docs/permissions.mdには一切触れていない外れても設定を戻せば元に戻る(マイグレーション・永続データ・公開APIに触れない)
docs/lint-policy.mdとdocs/roadmap.mdの更新はIssue app/がlib/の結果を直接フィルタ・集計しないようno-restricted-importsで層を強制する #43 の完了条件に明記された追記で、「Issueで内容が確定している追記」に当たる
決定した方針
1. 対象の粒度 —
app/→lib/のimportは禁止しないapp/がlib/のクエリ関数を呼ぶこと自体は正当なI/O呼び出しである。ここを塞ぐと、app/からデータに到達する経路が無くなり(common/は純粋に保つ必要があるので中継できない)、結局Supabaseクライアントを直接握ったクエリが
app/に生えるだけになる。新しい中間層を作る案も検討したが、それは
CLAUDE.mdのディレクトリ構成そのものの変更になるため見送った。**禁止したいのは「importすること」ではなく「結果を使って判断すること」**なので、
importの向き(
no-restricted-imports)と判断の実体(no-restricted-syntax)を分けて掛けた。common/app/lib/mcp// Next.js・React系 /@supabase/*lib/app/mcp//common/の値としてのimport(import typeは可)app/mcp//@supabase/*mcp/app// Next.js・React系 /@supabase/*配列操作の禁止対象:
filterfindfindIndexfindLastfindLastIndexflatMapeverysomeincludesindexOfsorttoSortedreducereduceRight。.map()は描画のための変換として正当なので除いた。この一覧はCLAUDE.md「ルールをpure関数に切り出す」の「フィルタ、並び順、検証、集計、権限判定、日付計算」をそのまま機械語に落としたもの。
2.
mcp/は対象に含めたapp/と同じくcommon/を経由せずlib/を直接叩いて判断する余地があり、片方だけ古いルールで動き続けるのがこのリポジトリで最も痛い壊れ方(
CLAUDE.md「MCPサーバーとWeb UIは同じ操作を2経路持つ」)。着手時点で
mcp/は空なので、含めるコストはゼロだった。加えて
mcp/からはNext.js・React系も禁止した(stdioサーバーがNext.jsを丸ごと読み込むのを防ぐ)。
3.
lib/に構文ルールを掛けなかった理由PostgRESTのクエリビルダが
.filter()を持つ(supabase.from(...).select().filter("col", "eq", v))ため、同名で誤検知する。
lib/側は「common/を値としてimportしない」制約だけで押さえた。lib/内での.reduce()による集計は機械では止まらないという残ったギャップはdocs/lint-policy.md「残っているギャップ」に明記した。4.
messageの文言「何が禁止か」ではなく**「代わりにどこへ書くか」**を書き、根拠の成果物と節名を必ず添えた。
例(
app//mcp/の配列操作):5. drainは不要 — 最初からerror
app/はNext.jsの雛形2ファイルのみ、lib/とmcp/は.gitkeepだけ。既存違反0件のためdocs/lint-policy.mdのdrain→ratchetでいう「新規開発の段階では最初からerrorで入れられる」に該当する。
実装上の落とし穴(設定に反映済み)
すべて
docs/lint-policy.md「設定を書くときの落とし穴」にも書いた。この設定は「1つの制約を2つの書き方で表現する」箇所が多く、片方だけ直すと静かに穴が開く。
pathsに書くとすり抜ける。pathsは完全一致しか見ないため、nextを禁止してもnext/headersが通る。patternsに移したnextnext/xだけでなく、ハイフン系(next-authreact-hook-form)とスコープ付き(@next/env@react-three/fiber)も別に列挙が要る@/lib/xを禁止しても../../lib/xは通る。かといって../の個数を列挙するとその数が検出の上限になる(App Routerのルート木は5階層を簡単に超える)。
../**/libの形で受けて深さ非依存にしたimport()はno-restricted-importsの対象外。**静的なimport宣言しか見ないためawait import("react")で全部すり抜ける。no-restricted-syntaxのImportExpressionセレクタに同じ制約を掛けた。こちらは正規表現で書くのでグロブ側と対で管理する
filesの拡張子。*.ts*.tsxだけだと.mts.ctsや素のJSが対象外になる。拡張子リストを1か所に置いて組み立てた
allowTypeImportsは@typescript-eslint/no-restricted-importsにしかない。型を二重定義しない方針と両立させるため、
lib/→common/の制約だけ拡張ルールを使った壊して確認した (完了条件)
レビューで対象が2回広がった(動的
import()、includes/indexOf、スコープ付きパッケージ)ので、最終状態の
eslint.config.mjsに対して全パターンをまとめて実行し直した。一時ファイルを置いて
yarn lintが赤くなることを確認し、削除して緑に戻ることを確認している。静的import (19件、すべてerror)
app/lib/の結果に.filter()app/lib/の結果に.reduce()app/ALLOWED_IDS.includes(userId)app/ALLOWED_IDS.indexOf(userId) !== -1app/import { createClient } from "@supabase/supabase-js"app/import ... from "@/mcp/..."common/import { useState } from "react"common/import ... from "@/lib/..."common/import ... from "../lib/..."common/a/b/c/d/e/f/import ... from "../../../../../../lib/..."(6階層)common/import { useForm } from "react-hook-form"common/import { auth } from "next-auth"common/import { env } from "@next/env"common/import { Canvas } from "@react-three/fiber"common/import { createClient } from "@supabase/supabase-js"common/import { cookies } from "next/headers"lib/import { canJoinEvent } from "@/common/permissions"(値)lib/import ... from "../mcp/..."mcp/import { cookies } from "next/headers"動的
import()(13件、すべてerror)common/await import("react")common/await import("next/headers")common/await import("next-auth")common/await import("@next/env")common/await import("@react-three/fiber")common/await import("@supabase/supabase-js")common/await import("@/lib/x")common/await import("../lib/x")app/deep/x.ctsawait import("@/mcp/tool")app/deep/x.ctsawait import("@supabase/ssr")app/deep/x.ctsrows.filter(...)mcp/x.jsawait import("next/headers")mcp/x.jsawait import("@/app/page").ctsと.jsを混ぜてあるのは、拡張子の網羅(ts tsx mts cts js jsx mjs cjs)が効いていることを同時に確認するため。
通ることも確認した (誤検知がないこと)
common/からimport { z } from "zod"/await import("zod")→ 通るcommon/からawait import("@reduxjs/toolkit")→ 通る(@react-*と紛らわしいが別物)lib/からimport type { InviteContext } from "@/common/permissions"→ 通るapp/から@/lib/...(I/O)と@/common/...(判断)をimportし、.map()で描画 → 通る一時ファイル削除後、
yarn lint/yarn typecheck/yarn testはいずれも緑。承知の上で受け入れた誤検知
.includes()/.indexOf()はレシーバの型を見ないので、文字列に対するpathname.includes("/events")も止まる。それでも対象に含めたのは非対称性による。**「厳しすぎて例外が出る」は例外リストに1行足せば戻せるが、「判断が静かに
app/に残る」は誰も気づかない。**引っかかったときの手順も
docs/lint-policy.mdに書いた。スコープ外として分離したもの
.mtsファイルを1つ置くとyarn lintがlintエラーではなくクラッシュする。型情報を要するルールのブロックが
**/*.mtsを対象にしている一方、eslint-config-nextのパーサーが
.mtsにparserOptions.projectを転送しないため。mainでも再現するこのPR以前からの別件なので #61 に切り出した。
見送ったもの
ESLint#lintTextで層越えコードを流すユニットテストを検討したが、
projectServiceが有効なため実在しない仮想ファイルの扱いが不安定(実際に
.mtsの実ファイルで型情報ルールがクラッシュしており、.mts ファイルを置くと yarn lint がクラッシュする(型情報ルールのパーサー設定漏れ) #61 に分離した)。また
eslint.config.mjsの他のルールも同様にテストされておらず、ここだけ入れると非対称になる。入れるなら設定全体に対する方針として別途決める
eslint-plugin-boundariesなどの専用プラグイン。**依存を増やさずビルトインで表現できたcommon/でのnew Date()禁止(docs/roadmap.mdフェーズ2の書き方の制約)。同種の「文書化済みの制約を機械化する」話だが、このIssueのスコープ外なので触っていない
🤖 Generated with Claude Code