docs(skills): Skill の欠落を補完（実害の出た落とし穴・アセットビルド規約の新設・検証手順） - #7008
Open
ttokoro20240902 wants to merge 9 commits into
Open
docs(skills): Skill の欠落を補完（実害の出た落とし穴・アセットビルド規約の新設・検証手順）#7008ttokoro20240902 wants to merge 9 commits into
ttokoro20240902 wants to merge 9 commits into
Conversation
レビューや調査で実際に誤判断を招いた事実が Skill に載っていなかったため追記する。 いずれも AGENTS.md の一般化テスト（固有のメソッド名・列名を消しても成立するか）と 120 字の上限を当てている。 - eccube-security: `access_control` は `security.yaml` に無く `EccubeExtension::configureFramework()` が動的に注入している。`security.yaml` だけを見ると `^/mypage/` が `ROLE_USER` 前提であることが 読めず、`getUser()` を null 可能と誤検出する。生成される 6 規則を表で明示 - eccube-controller: 配列が来る値を `getString()` でスカラー強制はできない（`InputBag` が例外を投げる） - eccube-service / eccube-purchase-flow: `TransactionListener` が 1 リクエスト＝1 トランザクションで 包み、コミットは `kernel.terminate`。`flush()` は確定ではない - eccube-entity: `@deprecated` なゲッタが CSV 出力項目のアクセサとして現役のことがあり、削除は仕様変更 - eccube-migration: カラム追加へマイグレーションを付けた前例（`Version20260316234241`）が実在するため、 レビューで「マイグレーション欠落 / 不要」を断定しない `eccube-purchase-flow` は「よくある間違い」が既に 12 項で上限超過のため、追記ではなく 基本ルール節に置いた。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughEC-CUBEの開発Skill文書を更新しました。入力、認可、永続化、サービス処理、テスト、レビューの注意事項と確認手順を追加しました。 Changes開発規約ドキュメント更新
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.4 #7008 +/- ##
=======================================
Coverage 77.21% 77.21%
=======================================
Files 563 563
Lines 28111 28111
=======================================
+ Hits 21705 21707 +2
+ Misses 6406 6404 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SCSS / JS バンドルのビルドを扱う Skill が無く、`scss` `style.css` `npm run build` に 言及した規約がどの Skill にも存在しなかった。生成物（`html/template/*/assets/css/` の `.css` `.min.css` `.map` と `html/bundle/`）はすべて git 管理されているため、 ソースだけコミットすると実機に反映されない。この取りこぼしは実害として発生している。 CI がこの不一致を検査しない点も明記した。E2E のワークフローは自分で `npm run build` するため、コミット済み生成物が古くても緑になる（「E2E が緑だから最新」と読めない）。 内容はすべて実装で裏取りした: - パイプライン: gulp 既定タスク = series(scss, scss-min, webpack) - scss: sass → postcss（postcss-import / autoprefixer / postcss-sort-media-queries(mobile-first)）→ 同階層 css/ へ出力 - webpack: front / admin / install の 3 エントリ → html/bundle/*.bundle.js - 生成物のモード（`.css` は 100755 / `.map` は 100644） `postcss-sort-media-queries` が @media を並べ替え・統合するため、生成物に手書きで @media を足した差分は判別できる（レビューでフルビルドか手書きかを見分ける基準）。 AGENTS.md の Skill 索引表にも 1 行追加した。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md が推奨する構成のうち「実行・確認方法」が controller / entity / formtype / migration / repository / service / review-responsibility の 7 件で欠けており、 実装後に何を実行して確かめるかが書かれていなかった。 各層で実際に効くコマンドだけを載せた(存在をコード側で確認済み): - controller: debug:router でルーティング登録を確認 - entity: doctrine:schema:update --dump-sql と eccube:generate:proxies - formtype: debug:form で構成・拡張の反映を確認 - migration: migrate → migrate prev → migrate で down() の往復と冪等性を確認 - repository: DQL/SQL を出して EXISTS の制約漏れ・件数の一致を確認 - service: debug:container で登録とデコレーションの解決先を確認 - review-responsibility: 差分の確定と、変更ファイルに絞った QA 実行 review-responsibility には対象の記載も無かったため追記した(特定ディレクトリではなく 「直前の変更差分」が対象であることを明示)。 なお当初「対象節が 18 件中 4 件のみ」と見立てていたが、これは `## 対象` という見出しだけを 数えた誤りだった。対象パスはタイトル直後の `**対象**:` 行として 19 件に記載済みで、 見出し形式への統一は内容が変わらないため行わない。 Co-Authored-By: Claude Opus 5 (1M context) <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 @.claude/skills/eccube-migration/SKILL.md:
- Around line 35-39: Update the “よくある間違い” guidance in SKILL.md to remove the
blanket prohibition on ALTER TABLE migrations for added columns. State that such
migrations are generally unnecessary when schema:update handles the change, but
are acceptable when required to reliably populate defaults for existing rows or
for another explicit reason, and avoid asserting that a migration is always
missing or unnecessary.
In @.claude/skills/eccube-security/SKILL.md:
- Around line 35-36:
SKILL.mdの「/mypage/配下ではgetUser()が非null」とする説明を、IS_AUTHENTICATED_ANONYMOUSLYの例外を含む内容へ修正してください。/mypage/loginと/mypage/withdraw_completeでは匿名アクセスによりgetUser()がnullになり得るため、これらのルートではnull参照を確認し、それ以外のROLE_USER前提ルートと区別して説明してください。
🪄 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 Plus
Run ID: eaaaa532-0573-4969-98ce-59f284bb8c56
📒 Files selected for processing (6)
.claude/skills/eccube-controller/SKILL.md.claude/skills/eccube-entity/SKILL.md.claude/skills/eccube-migration/SKILL.md.claude/skills/eccube-purchase-flow/SKILL.md.claude/skills/eccube-security/SKILL.md.claude/skills/eccube-service/SKILL.md
いずれも妥当な指摘だったため修正した。 - eccube-security: 「`^/mypage/` 配下は `ROLE_USER` が前提」と一律に書いていたが、 同じ表に列挙した `/mypage/login`・`/mypage/withdraw_complete` は `IS_AUTHENTICATED_ANONYMOUSLY` で匿名到達できる。`AccessMap::getPatterns()` は 最初に一致した規則で return する(first-match-wins)ため、より上に並ぶこの 2 件が 優先される。匿名ルートでは `getUser()` が null になり得る旨を追記し、 `/mypage/change` が逆に `IS_AUTHENTICATED_FULLY` で厳しい点も併記した - eccube-migration: 冒頭に「カラム追加のマイグレーション要否を断定しない」と書いた一方、 「よくある間違い」側は ALTER を一律否定したままで矛盾していた。既定は不要のまま、 既存行への既定値投入など理由があれば書いてよいことを反映した Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ttokoro20240902
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.claude/skills/eccube-asset/SKILL.md:
- Around line 1-3: Update the trigger description in the eccube-asset skill
metadata to distinguish html/template SCSS and JS files from repository-root
build definitions. List webpack.config.js, gulpfile.js, gulp/, and package.json
as root-level targets so edits to package.json also activate the skill, without
implying they are under html/template.
- Around line 53-54: Update the SCSS guidance in the relevant section of
SKILL.md to distinguish files excluded from the build from partials that are
included by the glob but not reflected in the style.scss/app.scss entry outputs.
Replace the assertion that unreachable .scss files are not build targets with
wording that accurately describes their omission from those entry-point outputs.
In @.claude/skills/eccube-repository/SKILL.md:
- Line 84: Update the PHPUnit command example in the repository testing
instructions to use a shell-safe, concrete placeholder such as
TargetRepositoryTest.php instead of angle-bracket syntax. Preserve the existing
tests/Eccube/Tests/Repository path and ensure the command can be executed
directly.
- Line 89: Update the testing guidance in the affected count/population rule so
filter-condition changes are not required to preserve the previous count.
Require tests to validate the expected count or consistency with the defined
population instead, while retaining the surrounding guidance and scope.
In @.claude/skills/eccube-review-responsibility/SKILL.md:
- Around line 80-83: テスト手順のPHPStan確認に、マージ前の全体解析としてvendor/bin/phpstan analyse
srcをlevel 6で実行する手順を追加してください。既存の変更ファイル向けvendor/bin/phpstan
analyse手順は維持し、全体確認も行う運用を明記してください。
- Around line 80-83: Update the PHP-CS-Fixer command in the checklist to use
--dry-run --diff instead of fix mode, so verification does not modify the
working tree while still showing proposed changes.
- Around line 75-78: .claude/skills/eccube-review-responsibility/SKILL.md
の差分確認手順で、ファイル単位の概要を表示する git diff --stat と git diff --cached --stat
に加え、変更内容の実体を表示する git diff と git diff --cached
も記載してください。既存のレビュー範囲を変更箇所に固定する説明は維持してください。
🪄 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 Plus
Run ID: 69340c74-9492-4bb7-a8e5-19d8d198fd96
📒 Files selected for processing (10)
.claude/skills/eccube-asset/SKILL.md.claude/skills/eccube-controller/SKILL.md.claude/skills/eccube-entity/SKILL.md.claude/skills/eccube-formtype/SKILL.md.claude/skills/eccube-migration/SKILL.md.claude/skills/eccube-repository/SKILL.md.claude/skills/eccube-review-responsibility/SKILL.md.claude/skills/eccube-security/SKILL.md.claude/skills/eccube-service/SKILL.mdAGENTS.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .claude/skills/eccube-security/SKILL.md
- .claude/skills/eccube-service/SKILL.md
- .claude/skills/eccube-entity/SKILL.md
- .claude/skills/eccube-controller/SKILL.md
- .claude/skills/eccube-migration/SKILL.md
Contributor
|
先に #7013（esbuild への移行）を取り込む方針にしたいと思います。 主に以下のファイルが削除されます。
このため、eccube-asset を esbuild ベースへの更新が必要になります。 参考: #7013 |
CodeRabbit 指摘対応。 - 山括弧プレースホルダ（`<対象>` 等）はシェルのリダイレクトとして解釈され、 そのまま実行すると失敗するため、実在するパス・ルート名・サービス ID に置き換えた。 指摘は eccube-repository のみだが、同型が controller / formtype / review-responsibility / service にもあったので横並びで直した。 - あわせて `bin/phpunit` を `vendor/bin/phpunit` に是正した。 `bin/` には .htaccess / console / template_jp.php しか無く、実行できないため。 - eccube-repository: 「変更前後で件数が一致するか」を固定する記述を 「期待件数または想定する母集団との整合性」に変更。絞り込み条件を変えれば 件数が変わるのは正常で、正しい変更まで不正として固定してしまうため。 - eccube-review-responsibility: `git diff --stat` だけでは変更行が確定できないため `git diff` / `git diff --cached` を併記。PHPStan は「実装中は絞る／PR 前に src 全体」 の 2 段階にした（CI は src 全体を解析するため絞ったままでは見落とす）。
- eccube-e2e: setup-fixtures.php は各ブロックに try-catch を持たない直列スクリプトで、
途中で Fatal になると以降のフィクスチャが未生成になり無関係なスイートが連鎖的に落ちる。
global-setup.ts は失敗を catch して警告するだけで実行を止めないため spec 側の不具合に見える。
- eccube-plugin: Kernel::registerBundles() は DB の有効/無効を見ず app/Plugin 直下を
Finder で列挙し bundles.php を require するため、依存欠落でカーネル起動自体が失敗する。
- eccube-entity: TraitProxyAttributeDriver は未宣言のときだけ Proxy を require_once する。
まっさらな状態では再現しないので、再現には対象を事前ロードした状態を作る必要がある。
- eccube-service: StockReduceProcessor の悲観ロックのガードに使う
ProductStock::$product_class_id は #[ORM\Column] を持たない非永続プロパティなので、
DB から読むと常に null でロックは毎回走る（条件付き＝デッドコードではない）。
- eccube-purchase-flow: 送料無料は DeliveryFeeFreePreprocessor がカート合計と比較する
条件であり、商品単位の性質ではない。
- eccube-controller: 削除時の外部キー違反はコアが catch (\Exception) +
delete_error_foreign_key で統一している。
- eccube-twig-template: Bootstrap 5.3 と jQuery は併存しており、コアの管理画面
テンプレート 10 以上が $(...).on('show.bs.modal') で動いている。
CartService だけでは Eccube\Service\CartService と Eccube\Twig\Extension\CartServiceExtension の 2 件に一致し、 非対話環境では Aborted で終わるため実際に動かない。
3 tasks
Contributor
Author
|
@dotani1111 ご指摘ありがとうございます。方針を承知しました。 esbuild への移行が必要なのは
これで本 PR は #7013 と独立してレビュー・取り込みが可能になります。お手数ですが、先に本 PR をご確認いただけますでしょうか。 あわせて以下も対応しました。
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Skill の棚卸しで見つかった欠落をまとめて埋めます。前提だった #7007（Skill 名の
eccube-接頭辞化）は 2026-07-30 に 4.4 へマージ済みで、本 PR は独立してレビューできます。当初 #7009 として分けていた「アセットビルド規約の新設」「検証手順の補完」も、
controller/entity/migration/serviceの 4 ファイルが本 PR と重なるため 1 PR に統合しました（#7009 は close）。① 実害が出たのに Skill に載っていなかった落とし穴（5 件）
レビューや調査で実際に誤判断を招いた事実です。追記はすべて #7007 で定めた一般化テスト（固有のメソッド名・列名を消しても項目が成立するか）と 120 字の上限を当てています。
eccube-securityaccess_controlはsecurity.yamlに存在せず、EccubeExtension::configureFramework()がprependExtensionConfig()で動的に注入している。生成される 6 規則を表で明示security.yamlだけを見て^/mypage/の認可が読めず、getUser()を null 可能と扱って「500 になる」と誤指摘したeccube-controllergetString()でスカラー強制はできない（InputBagが非スカラーで例外を投げる）getStringでスカラー強制すればよい」というレビュー提案が実際には機能しないeccube-service/eccube-purchase-flowTransactionListenerが 1 リクエスト＝1 トランザクションで包み、コミットはkernel.terminate。flush()は SQL 発行のみeccube-entity@deprecatedなゲッタが CSV 出力項目（dtb_csv）のアクセサとして現役のことがあるeccube-migrationVersion20260316234241）が実在するため「マイグレーション欠落 / 不要」を断定しないeccube-purchase-flow（12 項）とeccube-security（10 項）は「よくある間違い」が #7007 の上限に達しているため、追記ではなく本文側（基本ルール / アクセス制御モデル節）に書いています。② アセットビルド規約が存在しなかった（新規 Skill
eccube-asset）scss/style.css/npm run buildに言及した Skill がゼロでした。一方で生成物はすべて git 管理下です。html/template/*/assets/css/*.css*.min.css*.map.cssは mode 100755 /.mapは 100644）html/bundle/つまり
.scssだけコミットすると実機のスタイルが変わりません。これは実害として発生しています。さらに CI はこの不一致を検査しません。E2E のワークフロー（
e2e-test.yml:122-123）は自分でnpm ci && npm run buildを実行するため、コミット済み生成物が古くても E2E は緑になります。「E2E が緑だから最新」と読めない点を明記しました。内容はすべて実装で裏取りしています。
npm run build= gulp 既定タスク =series(scss, scss-min, webpack)scss: sass → postcss（postcss-import/autoprefixer/postcss-sort-media-queries(mobile-first)）→ 同階層のcss/へ出力webpack:front/admin/installの 3 エントリ →html/bundle/*.bundle.jspostcss-sort-media-queriesが@mediaを並べ替え・統合するため、生成物に手書きで@mediaを足した差分は判別できます。レビューで「フルビルドか手書き追記か」を見分ける基準として記載しました。③ 検証手順が無かった 7 Skill に「実行・確認方法」を追加
eccube-controllerdebug:routerでルーティング登録を確認eccube-entitydoctrine:schema:update --dump-sqlとeccube:generate:proxieseccube-formtypedebug:formで構成・拡張の反映を確認eccube-migrationmigrate→migrate prev→migrateでdown()の往復と冪等性を確認eccube-repositoryeccube-servicedebug:containerで登録とデコレーションの解決先を確認eccube-review-responsibility案内するコマンドは実在をコード側で確認しました（
RouterDebugCommand/DebugCommand(debug:form) /ContainerDebugCommand/UpdateSchemaDoctrineCommand/MigrateCommand/GenerateProxyCommand）。推測で書いたコマンドはありません。eccube-review-responsibilityは対象の記載も無かったため追記しました（特定ディレクトリではなく「直前の変更差分」が対象であることを明示）。④ CodeRabbit 指摘の反映（2 件・いずれも妥当）
eccube-security— ①で追記した「^/mypage/配下はROLE_USERが前提」が一律の記述になっていましたが、同じ表に列挙した/mypage/login・/mypage/withdraw_completeはIS_AUTHENTICATED_ANONYMOUSLYで匿名到達できます。AccessMap::getPatterns()は最初に一致した規則でreturnする（first-match-wins）ことを実装で確認し、匿名ルートではgetUser()が null になり得る旨と、/mypage/changeが逆にIS_AUTHENTICATED_FULLYで厳しい点を併記しました。追記自体の事実誤りだったため修正必須の指摘でした。eccube-migration— 冒頭に「要否を断定しない」と書いた一方、「よくある間違い」側は ALTER を一律否定したままで矛盾していました。既定は不要のまま、既存行への既定値投入など理由があれば書いてよいことを反映しました。検証
EccubeExtension.php/TransactionListener.php/Version20260316234241.php/dtb_csv.csv、eccube-assetの 17 ファイル）access_controlの 6 規則がEccubeExtension::configureFramework()の実装と一致し、security.yamlに記載が無いことAccessMap::getPatterns()が first-match-wins であることeccube-asset8 項・最長 99 字。追記 3 項は 103 / 116 / 101 字）変更は Markdown のみで、PHP・設定ファイルへの波及はありません。
🤖 Generated with Claude Code
Summary by CodeRabbit