Skip to content

AIコーディングエージェント向けの共通規約基盤(AGENTS.md + Skills) - #6817

Merged
dotani1111 merged 17 commits into
4.4from
feature/ai-agent-skills
Jun 11, 2026
Merged

AIコーディングエージェント向けの共通規約基盤(AGENTS.md + Skills)#6817
dotani1111 merged 17 commits into
4.4from
feature/ai-agent-skills

Conversation

@ttokoro20240902

@ttokoro20240902 ttokoro20240902 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

概要

複数の AI コーディングエージェント(Claude Code / Cursor / Codex CLI / Google Antigravity / Gemini CLI)で共有できる、EC-CUBE 4.4 のコーディング規約基盤を追加します。AGENTS.md をベンダー中立な正典とし、レイヤ別規約を docs/rules/ に 1 ソースで管理、各ツールには「Skill」として薄い発火スタブを配置します。

ドラフトです。方向性についてのフィードバックを歓迎します。

なぜ

  • AI 支援開発で、EC-CUBE 本体規約に沿わないコードや Fat コントローラが生まれるのを抑えたい。
  • ツール固有ファイル(CLAUDE.md 等)は他 CLI から読めない。規約の本文は 1 ソースに集約し、各ツールは自分の作法でそこを参照する形にしたい。

設計のポイント

  • 正典は AGENTS.md(最も横断対応が広い)。CLAUDE.md / GEMINI.md@AGENTS.md を参照する薄いポインタに変更(参照は一方向・循環なし)。
  • 規約本文は docs/rules/*.md(純 Markdown・GitHub でも読める)。常時ロードせずオンデマンド。
  • 各ツールへは Skill スタブを配置SKILL.md 形式は 4 ツール共通。フォルダだけ異なるため .claude/ を正本に .codex/ .agents/ へ同期(symlink は Windows で壊れるため不使用)。
    • 同期: `php tools/sync-ai-skills.php`(冪等・`--check` 対応)
  • Skill 命名規則: リポジトリ内のため `eccube-` 接頭辞なし。レイヤ規約系はトピック名(`controller` 等)、人が実行するアクション系は動詞前置(`review-responsibility`)。

追加した規約 / Skill

規約 (docs/rules/) Skill 内容
phpunit.md `phpunit` テスト規約(基底クラス・フィクスチャ・`#[DataProvider]`)
controller.md `controller` 責務分離・Fat 化防止(業務ロジックは Service へ)
service.md `service` 単一責任・HTTP 非依存・Controller 依存禁止
entity.md `entity` AbstractEntity 継承・PHP8 属性マッピング
repository.md `repository` QueryBuilder+パラメータバインド・データアクセス専念
formtype.md `formtype` AbstractType・`getBlockPrefix(): string`・CSRF 維持
migration.md `migration` スキーマ源泉は Entity 属性/変更時は冪等なマイグレーションを同一 PR で追加
controller.md・service.md `review-responsibility` 実装直後の全層責務レビュー(`tools/check-architecture.php`)

Fat 化・責務分離の方針(重要)

CI で既存コードを落とす方式は採りません(既存の巨大コントローラを fail させるとコントリビュートの妨げになるため)。第1段階として、

  • 書く時: Skill が規約を参照させる
  • 実装直後: `tools/check-architecture.php`(依存追加なし・助言用・CI 非搭載)で観点を可視化

に留めます。将来、合意が取れれば PHPMD / Deptrac + ベースラインで段階的に強化できる余地を残しています。

補足

  • 内容は EC-CUBE 4.4 の実コードで裏取りしています(例: Entity は XML でなく PHP8 属性、ルーティングは `#[Route]`、PHPStan は level 6、マイグレーションは `app/DoctrineMigrations/` に実在)。
  • twig/css/js/yaml 等のスタイル系規約は本筋から外れるため今回は含めていません(必要になった時点で同じ構成で追加)。

🤖 Generated with Claude Code

Summary by CodeRabbit

リリースノート

  • Documentation
    • 開発チーム向けの包括的なコーディング規約ドキュメントを追加しました。コントローラ、エンティティ、フォームタイプ、マイグレーション、テスト、リポジトリ、サービス層における実装ガイドラインを整備し、開発効率とコード品質の向上を支援します。

ttokoro20240902 and others added 6 commits June 8, 2026 19:30
AGENTS.md をベンダー中立な正典(single source of truth)とし、
Claude Code / Cursor / Codex CLI / Antigravity / Gemini CLI で共有できる
コーディング規約・Skill の仕組みを導入する。

- AGENTS.md: 正典。旧 CLAUDE.md の内容を 4.4 正確版へ修正して移設
  (XMLマッピング→PHP8属性, @route→#[Route], PHPStan level1→6)
- CLAUDE.md / GEMINI.md: @AGENTS.md を参照する薄いポインタ(一方向・循環なし)
- docs/rules/: レイヤ別規約の本文を1ソースで管理(初回は phpunit.md)
- .claude/.codex/.agents/skills/: 各ツール用の発火スタブ(本文は docs/rules を参照)
- tools/sync-ai-skills.php: スタブ同期スクリプト(symlink不使用・Windows安全・冪等)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CIで既存コードを落とす方式は採らず、「書く時」と「実装直後」の
2点でFatコントローラ/責務分離の崩れを防ぐ第1段階(摩擦ゼロ)を導入する。

- docs/rules/controller.md: 薄いコントローラ/業務ロジックはServiceへ、の規約(4.4実コードで裏取り)
- .claude/.codex/.agents/skills/eccube-controller: 実装時に規約を参照させる発火スタブ
- .claude/.codex/.agents/skills/eccube-responsibility-review: 実装直後の責務分離レビュー
- tools/check-fat-controller.php: 依存追加なしの助言用検査(メソッド長/コンストラクタ依存数/persist・flush直書き)
- AGENTS.md / docs/rules/README.md: 索引と方針を更新

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Skill命名規則を導入: eccube-接頭辞を廃止。レイヤ規約系はトピック名
  (controller/service/phpunit)、人が実行するアクション系は動詞前置
  (review-responsibility)に統一
- docs/rules/service.md: Service責務規約(単一責任/HTTP非依存/Controller依存禁止)を追加
- skills/service: Service実装時に規約を参照させる発火スタブを追加
- review-responsibility: controller専用から全層(Controller+Service)レビューへ一般化
- tools/check-fat-controller.php → tools/check-architecture.php に一般化
  (Serviceのメソッド長/依存数/Controller依存=レイヤ違反も検査)
- AGENTS.md / docs/rules/README.md: 索引・命名規則・方針を更新

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
enterpriseの「マイグレーションを作らない」はOSS 4.4には当てはまらない。
OSSはEntity属性をスキーマの源泉としつつ、既存環境へ届けるため
app/DoctrineMigrations/にマイグレーションを作成する(実例: Version20260316234241)。

- docs/rules/migration.md: スキーマ源泉=Entity属性、変更時は冪等な
  マイグレーションを同一PRで追加(up/down両実装、hasTable/hasColumnガード)、
  初期データはimport_csv、を4.4実コードで裏取りして規約化
- skills/migration: スキーマ変更時に規約を参照させる発火スタブ
- AGENTS.md / docs/rules/README.md: 索引を更新

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4.4実コードで裏取りして規約化:
- entity.md: AbstractEntity継承/PHP8属性マッピング/class_existsラッパ/
  setter fluent/dtb_mtb_命名/スキーマ変更はmigration連携/Customizeはtrait
- repository.md: AbstractRepository<T>継承/QueryBuilder+setParameterバインド/
  getQueryBuilderBySearchData/save・delete/データアクセスに専念
- formtype.md: AbstractType/getBlockPrefix():string必須/FormEvents/
  data_class/CSRF維持/既存拡張はFormTypeExtension
- skills/entity・repository・formtype: 各規約への発火スタブ
- AGENTS.md / docs/rules/README.md: 索引を更新

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
twig/css/js/yamlは責務分離・バックエンド品質の本筋から外れるため作らない。
未完成・約束に見える「順次追加予定」を排し、「必要になった観点を必要なときに
同じ構成で追加する」方針に変更。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

EC-CUBE 4.4 向けの統合的なコーディング規約体系を導入。AI エージェント用共通ハブ(AGENTS.md)、レイヤ別実装規約 8 種(Controller/Service/Entity/FormType/Repository/Migration/PHPUnit/review-responsibility)、スキル配布インフラ(symlink)を追加した。

Changes

EC-CUBE 4.4 開発規約とアーキテクチャガバナンス

Layer / File(s) Summary
AI エージェント向け統一参照ハブ
AGENTS.md, CLAUDE.md, GEMINI.md
EC-CUBE 4.4 の共通参照ハブとして AGENTS.md を導入。プロジェクト概要、開発手順、アーキテクチャ、Skill 配置、実装原則をまとめ、CLAUDE.md / GEMINI.md をポインタに置換した。
HTTP 処理と業務ロジック分離
.claude/skills/controller/SKILL.md, .claude/skills/service/SKILL.md
Controller は HTTP 入出力変換に特化(ルーティング・認可・CSRF)し、業務ロジックはコンストラクタインジェクションで Service に委譲。Service は永続化・複数 Repository 横断・PurchaseFlow 呼び出しを受け持ち、HTTP 知識を持たないことを規定した。
Data 層責務分担
.claude/skills/entity/SKILL.md, .claude/skills/formtype/SKILL.md, .claude/skills/repository/SKILL.md
Entity は AbstractEntity 継承し状態ロジック・計算に限定(永続化は外部委譲)。FormType は入力・検証・data_class に責務を限定。Repository は QueryBuilder / DQL 実装・N+1 回避・save / delete メソッド提供に特化した。
DB マイグレーション・テスト規約
.claude/skills/migration/SKILL.md, .claude/skills/phpunit/SKILL.md
スキーマ源泉を Entity 属性に固定し、単純カラム変更は schema:update に委譲。マイグレーションは手書き・冪等性必須・up() / down() 対で構成。PHPUnit は基底クラス選択・フィクスチャヘルパ・#[DataProvider] 属性化で統一した。
責務分離自己チェック手順
.claude/skills/review-responsibility/SKILL.md
実装直後の自己チェック手順を定義。Controller/Service 責務境界、業務ロジック残留、PurchaseFlow 外への計算/永続化漏れ、HTTP 知識の混入、共通化不足、責務混在を確認し、各指摘に具体的な移動先メソッドを付与する運用を明記した。
スキル配布インフラ
.agents/skills, .codex/skills
.claude/skills をシンボリックリンク先として統一し、複数エージェントツール(Claude / Gemini / 他)で同一のスキル定義を参照できる体制を整備した。

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

ウサギの記す規約の書(ほん)
Controller は HTTP に徹して
Service は業務ロジック頼み
Entity は状態を守る
Repository はデータ守護神ー🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed タイトルはPR全体の主要な変更内容(AIエージェント向けの共通規約基盤の追加とAGENTS.mdおよび各Skillファイルの新規作成)を明確に要約しており、複数のドキュメント追加の全容を簡潔に表現している。
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/ai-agent-skills

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ttokoro20240902 and others added 3 commits June 9, 2026 08:23
- AGENTS.md: ルーティング属性の FQCN を実コードに合わせて修正
  (Symfony\Component\Routing\Annotation\Route → Attribute\Route。
   src/Eccube では Attribute\Route が 68 件、Annotation は 0 件)
- docs/rules/controller.md: Route 名前空間の二重記述を解消し FQCN は共通規約に一本化
  (末端ドキュメントは上流を参照し返さない一方向ルールを維持)。
  例の `final class` をコア慣習(0/71 が final)に合わせて除去。getUser() が
  Symfony 親クラス由来である旨を明記
- docs/rules/README.md: ディレクトリ図の `eccube-<layer>` を実ディレクトリ/命名ルールに合わせ
  `<layer>` へ修正
- tools/check-architecture.php: --changed が未追跡(add 前)の新規 Controller/Service も
  拾うよう git ls-files --others を追加
- tools/sync-ai-skills.php: ファイル削除で空になったディレクトリを掃除

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
.codex/.agents 配下は .claude/skills を正本に同期生成される一方向の生成物だが、
ファイル単体では正本がどこか分からず、誤って生成先を編集すると次回 sync で
静かに上書き消失する動線だった。全コピー(正本含む)の冒頭(フロントマター直後)に
同一の自動生成マーカーを入れ、どのコピーを開いても「正本は .claude、生成先は
直接編集しない」と分かるようにする。マーカーは全コピー同一内容のため
sync --check の整合比較は壊れない。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- CI: tools/sync-ai-skills.php --check を実行する再利用ワークフローを追加し、
  main.yml の lint 段(rector/phpstan/php-cs-fixer と同列・unit/e2e の前段ゲート)に組み込む。
  正本 .claude/skills と .codex/.agents の同期忘れを検知する(標準PHPのみ・composer不要)。
- docs/rules/controller.md: 認可・CSRF 観点を追記。
  認可=admin ファイアウォール(^/%eccube_admin_route%/)配下に置く(コアは #[IsGranted] 不使用)、
  CSRF=GET 以外の状態変更/削除/Ajax は $this->isTokenValid() を検証、という実慣習を明文化。
- docs/rules/entity.md: 例の use ブロックに Doctrine\DBAL\Types\Types の import を補完
  (Types::INTEGER を使う例だが import が抜けていた)。

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@nanasess

nanasess commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

レビュー: 規約本文(docs/rules)の方針について

この規約基盤について、AIエージェントに「ツールでは補えない判断・ドメイン知識」を
渡すという観点から、見直しを提案したい点を3つ挙げます。優先度順です。

1.【要修正】migration.md / entity.md が EC-CUBE のスキーマ運用と逆を教えている

現状の migration.md は「列の追加時はマイグレーション(ALTER TABLE)を作る」を規範とし、
entity.md も「属性を変更したら同一PRでマイグレーション追加」としています。
これは EC-CUBE 4 系の実際のスキーマ運用と食い違います。

EC-CUBE のアップデートは 2段構えです(doc4 アップデート手順):

  1. bin/console doctrine:schema:update --force --dump-sql
    エンティティ属性 #[ORM\Column] から導けるカラム追加/変更を自動反映
  2. bin/console doctrine:migrations:migrate
    → schema:update で扱えないもの(マスタ/初期データの INSERT、型変更・リネーム等)

つまり単純なカラム追加に ALTER マイグレーションは不要(schema:update が拾う)。
これはリポジトリの実体でも裏付けられます:

  • app/DoctrineMigrations/19本中12本が INSERT INTO(マスタ/初期データ投入)
    ALTER ... ADD(列追加)はわずか1本
  • しかも migration.md が手本として載せている Version20260316234241
    (dtb_base_info への option_guest_purchase 純ALTER)が、その唯一の例外そのもの。
    → 例外を規範として教えてしまっています。
  • 前例 PR Googleアナリティクス機能を追加 #4912(Googleアナリティクス機能)も BaseInfo にカラムを追加しましたが、
    ALTER マイグレーションは作らず schema:update に委ねています。

修正方針:

  • 生成コマンドを doctrine:migrations:diff(エンティティ差分から ALTER を自動生成)ではなく
    **doctrine:migrations:generate(空の雛形を手書き)**に変更。
  • 本文の前提を「カラム追加は schema:update が反映。マイグレーションが必要なのは
    マスタ/初期データの INSERT と、schema:update が安全に扱えない構造変更(型変更・リネーム等)のみ
    」へ転換。
  • サンプルを ALTER TABLE ... ADD から マスタ/初期データの INSERT 例に差し替え。
  • entity.md の該当記述(「属性変更したらマイグレーション追加」)も同様に修正。

2.【方針】機械が扱える内容は規範化せず、ツール実行に委ねる

ツールが検出・整形・計測できるものを Skill の散文で重ねて持つと、コンテキストを食う上に
「強制もされず・判断も鈍る」どっちつかずに陥ります。これらは ローカルでツールを実行することで
担保し、規約本文はその実行を促す方向に寄せたい(CI は最後の砦であって一次防衛線ではない)。

開発フローとして PR #6761(.husky/pre-commitrector(dry-run) → phpstan → php-cs-fixer
を commit 前に実行)が取り込まれれば、staged な .php を含む commit 時にこれらが自動で走る想定です。

(a) 整形・変換系: 以下は rector / phpstan / php-cs-fixer が扱うので、個別の散文や
「よくある間違い」表で重複説明しない

項目 担うツール
ライセンスヘッダ・PSR-12 整形 php-cs-fixer
引数/戻り値の型宣言・静的解析違反 phpstan
@Route#[Route]・アノテーション→PHP8属性 rector (ANNOTATIONS_TO_ATTRIBUTES/Symfony set)
コンストラクタDI(private readonly) rector (SYMFONY_CONSTRUCTOR_INJECTION)
@dataProvider#[DataProvider] rector (PHPUNIT_110)

代わりに各規約へ「実装後にローカルで実行すること」を一貫した短い形で記す:

vendor/bin/rector process --dry-run --config=rector.php
vendor/bin/phpstan analyse src
vendor/bin/php-cs-fixer fix

(#6761 マージ後は pre-commit が同じチェックを自動実行)

(b) 数値メトリクス(代理指標): controller.md / service.md
1メソッド約50行」「依存約7個」といった閾値も、Skill の散文には置かないことを提案します。

  • これらは「単一責任か?」という本質の代理指標にすぎず、数値で線を引くと
    「51行だから機械的に分割/49行だからOK」というカーゴカルト的判断を誘発します。
  • 計測を強制したいなら、それはツールの仕事(PHPMD の ExcessiveMethodLength 等、
    または本PR同梱の tools/check-architecture.php)。散文に数値・ツールに計測、で二重持ちになっています。
  • Skill には数値ではなく、質的な Fat 化シグナルだけを残したい:
    業務的な計算・判定(金額/在庫/送料/ポイント)の混入、persist()/flush() の業務的直書き、
    複数 Repository またぎ・外部API・メール・ファイルIO、同一処理のコピペ重複。
    ——これらは「何を Service / PurchaseFlow へ出すか」という判断で、まさに Skill の中身です。

これにより Skill は「ツールが見られない判断・ドメイン知識」に集中でき、
整形・変換・計測はツール(将来 pre-commit)へ委ねられます。

3.【拡充】EC-CUBE 固有のドメイン知識を厚くする

ツールで補えない知識こそ Skill の本来価値ですが、現状は薄めです:

  • PurchaseFlow が規約に未登場。 service.md は「業務ロジック=Service」と一般化していますが、
    注文に関する計算・検証・確定(在庫引当・採番・ポイント付与・値引き)は
    PurchaseFlow パイプライン
    (ItemPreprocessor/ItemValidator/PurchaseProcessor/DiscountProcessor)に置く、
    という EC-CUBE の核を明記したい。
  • Entity の「状態」と「処理」の線引き。 entity.md の「業務ロジックは書きすぎない」は雑。
    自身のプロパティから導く状態ロジック(OrderItem::getTotalPrice() 等)は Entity に実装してよい一方、
    在庫引当・採番などの「処理」は PurchaseFlow へ、と区別を示す。
  • getId(): ?int が nullable な理由(未永続=ID未採番 / IDENTITY 採番)を一言添える。
  • ドメイン用語集: 「販売種別(SaleType)=決済を分割する単位」「Shipping=出荷単位(1受注に複数可)」
    「OrderItem の明細種別=商品/送料/手数料/値引き」など。

ttokoro20240902 and others added 3 commits June 9, 2026 12:12
PR #6817 のレビュー(#issuecomment-4655303640)に対応。

1. スキーマ運用の誤りを修正(要修正)
   - 「カラム追加=マイグレーション」を撤回し、schema:update --force →
     migrations:migrate の2段構えを明記。単純なカラム追加にALTERは不要。
   - マイグレーションの用途をマスタ/初期データのINSERTと、型変更・リネーム等
     schema:updateで扱えない構造変更に限定。
   - 生成コマンドを migrations:diff から migrations:generate(空雛形を手書き)へ。
   - 手本コードをALTER ADDからmtb_sale_typeへの冪等なINSERT例に差し替え。

2. 機械が扱える内容はツールへ委譲
   - 「約50行/約7依存」の数値メトリクスを散文から削除し質的シグナルへ転換。
     計測は tools/check-architecture.php に一本化。
   - rector/phpstan/php-cs-fixer のローカル実行節を新設(PR #6761 pre-commit を参照)。

3. EC-CUBE固有のドメイン知識を拡充
   - service.md に PurchaseFlow パイプライン節を追加。
   - entity.md に状態ロジック(Entity可)と処理(PurchaseFlow/Service)の線引き、
     getId() が nullable な理由を追記。
   - AGENTS.md に SaleType / OrderItem明細種別 / Shipping のドメイン用語集を追加。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
migration.md / migration スキルで、マスタ・初期データの追加手順が
「CSVを足すだけ」と誤解されないよう補強した。

- import_csv の CSV は eccube:fixtures:load(インストール時のみ実行・
  composer auto-scripts に非搭載)で投入されるため、CSV 追記だけでは
  既存環境の DB に届かない。既存環境へは INSERT マイグレーションで配布する。
- よって マスタ/初期データ追加は CSV 追記 + INSERT マイグレーションを
  同一 PR で両方行う、と用途・反映タイミングを表で明示。
- 実例を 2021 年の PR #4912 から 2024-03 の Version20240312170000
  (dtb_block + dtb_block.csv を同時追加)に更新。INSERT マイグレーション運用は
  2022〜2024 年も継続中であることを補足。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
レビュー指摘の3語(SaleType/OrderItemType/Shipping)に加え、
コードで裏取りした上でAIエージェントが誤りやすい根幹概念を補強した。

- 受注ステータス(OrderStatus): PROCESSING/PENDING は確定前の仮受注で、
  OrderHelper はカート確定入口で PROCESSING の受注を生成し購入完了で NEW へ遷移。
  集計・一覧は OrderStatusFilter で除外。「Order=確定済み」の誤解を防ぐ。
- ProductClass と「規格なし商品」: 在庫・価格は ProductClass 単位。規格を持たない
  商品も内部的に ProductClass を1つ持つ(hasProductClass で判定)。
- 単一テーブル継承(STI)と discriminator_type: mtb_*/dtb_block 等で必須。
  INSERT マイグレーションでの指定漏れを防ぐ。
- Payment/Delivery: 受注の基本マスタ。利用可否は SaleType に依存。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ttokoro20240902

Copy link
Copy Markdown
Contributor Author

@nanasess
ご指摘ありがとうございます。3点修正しました。
1. スキーマ運用(要修正)

  • 2段構え(schema:update --forcemigrations:migrate)を明記し、単純なカラム追加に ALTER 不要へ転換。
  • マイグレーションの用途を INSERT・型変更/リネーム等に限定、生成を migrations:diffmigrations:generate へ、手本も冪等な INSERT 例に差し替え。
  • 補足: eccube:fixtures:load はインストール時のみ実行のため、マスタ/初期データは CSV+INSERTマイグレーションの両方が必要Version20240312170000 等、2022〜2024 も継続)と確認し明記。

2. ツールへの委譲

3. ドメイン知識の拡充

  • service.md に PurchaseFlow 節、entity.md に状態ロジック/処理の線引きと getId(): ?int の理由を追記。
  • 用語集(AGENTS.md)に SaleType/OrderItem明細種別/Shipping を追加。さらに精査し、受注ステータス(PROCESSING/PENDING=仮受注)・ProductClass と規格なし商品・STI の discriminator_type も補強。

引き続きドラフトですので、方向性も含めご意見ください。

@ttokoro20240902
ttokoro20240902 marked this pull request as ready for review June 10, 2026 05:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (8)
tools/check-architecture.php (1)

198-204: ⚡ Quick win

persist/flushuse ...Controller... の検出が文字列/コメントでも誤検知します。

生テキスト正規表現だとコメント・文字列リテラルも拾うため、助言品質が下がります。token_get_all() の結果で T_COMMENT/T_DOC_COMMENT/T_CONSTANT_ENCAPSED_STRING を除外して判定する実装に寄せるのが安全です。

🤖 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 `@tools/check-architecture.php` around lines 198 - 204, The current regex
checks (the preg_match_all calls that search $code for
'->\s*(persist|flush)\s*\(' when $isController and for '\buse ...Controller'
when $isService) wrongly match inside comments and string literals; change the
implementation to first run token_get_all($code), filter out tokens T_COMMENT,
T_DOC_COMMENT and T_CONSTANT_ENCAPSED_STRING (and optionally
T_ENCAPSED_AND_WHITESPACE), reconstruct the remaining source text, and then run
the existing preg_match_all checks against that cleaned text so only real code
is inspected.
AGENTS.md (1)

34-68: 💤 Low value

Markdown linting: Fenced code block に言語指定を追加してください。

34行目の ```(開始) に言語指定がありません。ディレクトリ構造を示すブロックなので、text または言語なしでも構いませんが、markdownlint の警告を解消するには以下のように修正できます。

📝 修正案
 ## ディレクトリ構成
 
-```
+```text
 src/Eccube/           # コアアプリケーション
   Controller/         # HTTP コントローラ(管理画面・フロント)
🤖 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 `@AGENTS.md` around lines 34 - 68, The fenced code block that starts with "```"
(showing the directory tree) is missing a language specifier; update the opening
fence to include a language (e.g., change "```" to "```text") in AGENTS.md so
markdownlint warnings are resolved while keeping the directory-listing content
unchanged.

Source: Linters/SAST tools

docs/rules/README.md (1)

13-18: 💤 Low value

Markdown linting: Fenced code block に言語指定を追加してください。

13行目の ``` に言語指定がありません。ディレクトリ構造を示すブロックなので、text を指定できます。

📝 修正案
-```
+```text
 docs/rules/<layer>.md                ← 規約の本文(実体・1 ソース)
 .claude/skills/<layer>/SKILL.md      ← Claude Code 用スタブ
🤖 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 `@docs/rules/README.md` around lines 13 - 18, The fenced code block in the
README lacks a language specifier; open the README's triple-backtick block that
contains the directory listing and add the language identifier text immediately
after the opening backticks (i.e., change ``` to ```text) so markdown linters
recognize it; also scan for any other plain ``` blocks in the same file and
apply the same fix for consistency.

Source: Linters/SAST tools

docs/rules/formtype.md (2)

61-62: ⚡ Quick win

管理画面検索フォームでのCSRF保護の根拠を明確化

管理画面の検索フォームでCSRF保護を維持する理由が明示されていません。一般的に、GETメソッドで状態を変更しない検索フォームではCSRF保護は不要とされることが多いですが、EC-CUBEで異なる方針を採る場合は、その根拠(例:POSTメソッド使用、状態変更を伴う検索、過去のセキュリティインシデント等)を記載すると、開発者が方針を理解しやすくなります。

🤖 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 `@docs/rules/formtype.md` around lines 61 - 62, Update the
docs/rules/formtype.md paragraph about keeping CSRF enabled to explicitly state
the rationale: explain whether admin search forms in this project may use POST
or perform state-changing actions (or reference past security incidents) and why
that requires keeping 'csrf_protection' => true in configureOptions(), or
conversely cite that GET-only, side-effect-free searches are allowed to omit
CSRF; mention configureOptions() and the 'csrf_protection' => true default so
readers can locate the implementation point. Keep the explanation concise and
factual so developers understand the policy and where to change it.

53-54: ⚡ Quick win

notInRangeMessage 使用の根拠を明記

Range 制約で min/max 両方指定時に notInRangeMessage を使う理由が明記されていません。これがSymfonyのベストプラクティスなのか、EC-CUBE固有の要件なのか、またその背景(デフォルトメッセージとの違い、ユーザー体験の向上等)を補足すると、規約の理解が深まります。

🤖 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 `@docs/rules/formtype.md` around lines 53 - 54, The doc line mentions using
validation constraints like Assert\NotBlank and using Range with
notInRangeMessage when both min and max are set but lacks rationale; update the
docs to explicitly state that when using the Range constraint with both min and
max specified you should set notInRangeMessage because Symfony uses that single
combined message for out-of-range values (providing clearer, single-message UX
compared to separate min/max messages), and clarify whether this guidance is
Symfony-native behavior or an EC-CUBE convention (and note the difference from
the default messages and the UX benefit).
tools/sync-ai-skills.php (3)

72-72: 💤 Low value

エラー抑制演算子の使用を避け、明示的な存在チェックを推奨。

@unlink() はファイルが存在しない場合だけでなく、権限エラーなど他の問題も抑制します。冪等性のためなら file_exists() チェックの方が意図が明確です。

♻️ 改善案
         foreach (array_keys($dstFiles) as $rel) {
-            `@unlink`($target.'/'.$rel);
+            $path = $target.'/'.$rel;
+            if (file_exists($path)) {
+                unlink($path);
+            }
         }
🤖 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 `@tools/sync-ai-skills.php` at line 72, Replace the error-suppressed unlink
call (`@unlink`($target.'/'.$rel)) with an explicit existence and type check
before calling unlink: first build the path variable (e.g., $path = $target .
'/' . $rel), use file_exists() or is_file() to confirm the file is present, then
call unlink($path) and handle/ log any failure (e.g., check the return value and
emit an error or throw) instead of suppressing errors with @; this keeps
behavior idempotent and surfaces permission or other unlink errors.

49-49: ⚡ Quick win

file_get_contents() の戻り値をチェックしていない。

file_get_contents() が失敗した場合は false を返すため、文字列キャストすると空文字列になります。読み込み失敗を検出できず、誤った同期結果になる可能性があります。

🛡️ 改善案: 戻り値チェックの追加
         if ($file->isFile()) {
             $rel = substr($file->getPathname(), strlen($base) + 1);
-            $files[$rel] = (string) file_get_contents($file->getPathname());
+            $content = file_get_contents($file->getPathname());
+            if ($content === false) {
+                fwrite(STDERR, "failed to read: {$file->getPathname()}\n");
+                exit(1);
+            }
+            $files[$rel] = $content;
         }
🤖 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 `@tools/sync-ai-skills.php` at line 49, file_get_contents()
の戻り値を直接キャストしているため失敗時に false が空文字列に変換されて検出できません; まず
file_get_contents($file->getPathname()) を変数(例: $contents)に代入して === false
をチェックし、失敗時は適切にログ出力または例外を投げて処理を中止/スキップし、成功時のみ $files[$rel]
に文字列を格納するように修正してください(参照箇所: file_get_contents, $files, $rel,
$file->getPathname())。

79-79: ⚡ Quick win

file_put_contents() の戻り値チェックがない。

書き込み失敗(ディスク容量不足、権限エラー等)を検出できず、不完全な同期のまま成功と報告される可能性があります。

🛡️ 改善案: 戻り値チェックの追加
             $path = $target.'/'.$rel;
             if (!is_dir(dirname($path))) {
                 mkdir(dirname($path), 0o775, true);
             }
-            file_put_contents($path, $content);
+            if (file_put_contents($path, $content) === false) {
+                fwrite(STDERR, "failed to write: {$path}\n");
+                exit(1);
+            }
         }
🤖 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 `@tools/sync-ai-skills.php` at line 79, file_put_contents($path, $content)
の戻り値をチェックして書き込み失敗を検出するように修正してください: file_put_contents が false を返した場合にエラーログを出力し(例:
processLogger や error_log
を使う)、必要なら例外を投げて同期処理を中断するかリトライ/クリーンアップを行うようにし、成功時は返却値(書き込まれたバイト数)を利用して期待通り書き込めたか確認してください(参照箇所:
file_put_contents, $path, $content)。
🤖 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 @.github/workflows/sync-ai-skills.yml:
- Around line 15-16: The Checkout step using
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd currently does not set
persist-credentials; update the "Checkout" step (the actions/checkout
invocation) to include persist-credentials: false so the default GITHUB_TOKEN
credentials are not persisted to the repository git config for downstream steps
that may run PR code.

In `@tools/check-architecture.php`:
- Line 177: The code casts file_get_contents($file) to string which hides
failures; change the logic around the $code assignment to first call $contents =
file_get_contents($file) and check if $contents === false, then emit a
warning/error mentioning $file (e.g. using your existing logger or
fwrite(STDERR, ...)) and skip parsing this file instead of casting to an empty
string; only set $code = (string)$contents when the read succeeded so the
check-architecture flow doesn’t treat unreadable files as empty code.

---

Nitpick comments:
In `@AGENTS.md`:
- Around line 34-68: The fenced code block that starts with "```" (showing the
directory tree) is missing a language specifier; update the opening fence to
include a language (e.g., change "```" to "```text") in AGENTS.md so
markdownlint warnings are resolved while keeping the directory-listing content
unchanged.

In `@docs/rules/formtype.md`:
- Around line 61-62: Update the docs/rules/formtype.md paragraph about keeping
CSRF enabled to explicitly state the rationale: explain whether admin search
forms in this project may use POST or perform state-changing actions (or
reference past security incidents) and why that requires keeping
'csrf_protection' => true in configureOptions(), or conversely cite that
GET-only, side-effect-free searches are allowed to omit CSRF; mention
configureOptions() and the 'csrf_protection' => true default so readers can
locate the implementation point. Keep the explanation concise and factual so
developers understand the policy and where to change it.
- Around line 53-54: The doc line mentions using validation constraints like
Assert\NotBlank and using Range with notInRangeMessage when both min and max are
set but lacks rationale; update the docs to explicitly state that when using the
Range constraint with both min and max specified you should set
notInRangeMessage because Symfony uses that single combined message for
out-of-range values (providing clearer, single-message UX compared to separate
min/max messages), and clarify whether this guidance is Symfony-native behavior
or an EC-CUBE convention (and note the difference from the default messages and
the UX benefit).

In `@docs/rules/README.md`:
- Around line 13-18: The fenced code block in the README lacks a language
specifier; open the README's triple-backtick block that contains the directory
listing and add the language identifier text immediately after the opening
backticks (i.e., change ``` to ```text) so markdown linters recognize it; also
scan for any other plain ``` blocks in the same file and apply the same fix for
consistency.

In `@tools/check-architecture.php`:
- Around line 198-204: The current regex checks (the preg_match_all calls that
search $code for '->\s*(persist|flush)\s*\(' when $isController and for '\buse
...Controller' when $isService) wrongly match inside comments and string
literals; change the implementation to first run token_get_all($code), filter
out tokens T_COMMENT, T_DOC_COMMENT and T_CONSTANT_ENCAPSED_STRING (and
optionally T_ENCAPSED_AND_WHITESPACE), reconstruct the remaining source text,
and then run the existing preg_match_all checks against that cleaned text so
only real code is inspected.

In `@tools/sync-ai-skills.php`:
- Line 72: Replace the error-suppressed unlink call (`@unlink`($target.'/'.$rel))
with an explicit existence and type check before calling unlink: first build the
path variable (e.g., $path = $target . '/' . $rel), use file_exists() or
is_file() to confirm the file is present, then call unlink($path) and handle/
log any failure (e.g., check the return value and emit an error or throw)
instead of suppressing errors with @; this keeps behavior idempotent and
surfaces permission or other unlink errors.
- Line 49: file_get_contents() の戻り値を直接キャストしているため失敗時に false が空文字列に変換されて検出できません;
まず file_get_contents($file->getPathname()) を変数(例: $contents)に代入して === false
をチェックし、失敗時は適切にログ出力または例外を投げて処理を中止/スキップし、成功時のみ $files[$rel]
に文字列を格納するように修正してください(参照箇所: file_get_contents, $files, $rel,
$file->getPathname())。
- Line 79: file_put_contents($path, $content) の戻り値をチェックして書き込み失敗を検出するように修正してください:
file_put_contents が false を返した場合にエラーログを出力し(例: processLogger や error_log
を使う)、必要なら例外を投げて同期処理を中断するかリトライ/クリーンアップを行うようにし、成功時は返却値(書き込まれたバイト数)を利用して期待通り書き込めたか確認してください(参照箇所:
file_put_contents, $path, $content)。
🪄 Autofix (Beta)

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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e7821fe2-ddbf-4466-870b-291c006d9ee7

📥 Commits

Reviewing files that changed from the base of the PR and between 9820448 and 6d2ef13.

📒 Files selected for processing (39)
  • .agents/skills/controller/SKILL.md
  • .agents/skills/entity/SKILL.md
  • .agents/skills/formtype/SKILL.md
  • .agents/skills/migration/SKILL.md
  • .agents/skills/phpunit/SKILL.md
  • .agents/skills/repository/SKILL.md
  • .agents/skills/review-responsibility/SKILL.md
  • .agents/skills/service/SKILL.md
  • .claude/skills/controller/SKILL.md
  • .claude/skills/entity/SKILL.md
  • .claude/skills/formtype/SKILL.md
  • .claude/skills/migration/SKILL.md
  • .claude/skills/phpunit/SKILL.md
  • .claude/skills/repository/SKILL.md
  • .claude/skills/review-responsibility/SKILL.md
  • .claude/skills/service/SKILL.md
  • .codex/skills/controller/SKILL.md
  • .codex/skills/entity/SKILL.md
  • .codex/skills/formtype/SKILL.md
  • .codex/skills/migration/SKILL.md
  • .codex/skills/phpunit/SKILL.md
  • .codex/skills/repository/SKILL.md
  • .codex/skills/review-responsibility/SKILL.md
  • .codex/skills/service/SKILL.md
  • .github/workflows/main.yml
  • .github/workflows/sync-ai-skills.yml
  • AGENTS.md
  • CLAUDE.md
  • GEMINI.md
  • docs/rules/README.md
  • docs/rules/controller.md
  • docs/rules/entity.md
  • docs/rules/formtype.md
  • docs/rules/migration.md
  • docs/rules/phpunit.md
  • docs/rules/repository.md
  • docs/rules/service.md
  • tools/check-architecture.php
  • tools/sync-ai-skills.php

Comment thread .github/workflows/sync-ai-skills.yml Outdated
Comment thread tools/check-architecture.php Outdated
- sync-ai-skills.yml: checkout に persist-credentials: false を追加し GITHUB_TOKEN の永続化を防止
- check-architecture.php: file_get_contents の false を明示処理し、読込失敗ファイルを空コード扱いしない

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ttokoro20240902

Copy link
Copy Markdown
Contributor Author

シンボリックリンク利用でOK。

ttokoro20240902 and others added 4 commits June 10, 2026 17:05
レビュー指摘(シンボリックリンク利用でOK)を受け、Skillスタブ同期の仕組みを見直す。

- 規約本文を docs/rules/*.md から各 .claude/skills/<name>/SKILL.md へ一本化
  (発火時に詳細が必要なため、要点スタブと本文を分ける二層を廃止)
- .codex/skills・.agents/skills を .claude/skills への symlink(../.claude/skills)に変更
- コピー同期スクリプト tools/sync-ai-skills.php と同期CI sync-ai-skills.yml を削除し
  main.yml の job/needs から除去
- docs/rules/ を廃止し、AGENTS.md / GEMINI.md / tools/check-architecture.php の参照を
  .claude/skills/*/SKILL.md へ張り替え

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
廃止した docs/rules/README.md にあった「規約の書き方」指針を、新規Skill追加者向けに
AGENTS.md のレイヤ別規約セクションへ1行で取り込む。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
「メソッド50行/依存7個」の数値しきい値判定は、規約が掲げる「数値で線を引かない
(質的シグナルで判断する)」方針と矛盾するため、tools/check-architecture.php を廃止する。

- tools/check-architecture.php を削除(tools/ 配下は空になり消滅)
- controller / service SKILL から「計測はツールの仕事」「数値メトリクス」の記述と
  ツール実行例を除去(整形・型・変換は rector/phpstan/php-cs-fixer に委ねる旨は維持)
- review-responsibility SKILL の手順1(ツールによる機械的可視化)を廃止し、
  persist/flush 直書き・レイヤ違反の検査観点は目視レビューのチェックリストへ統合

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants