MCPサーバ実装#6832
Open
dotani1111 wants to merge 40 commits into
Open
Conversation
StringClassNameToClassConstantRector を Bundle-1.0.0 / Bundle-1.0.1 の BundleCompilerPass に適用。 husky pre-commit の Rector dry-run が origin/4.4 時点で fail していたものを解消する (修正内容自体は同等動作)。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- symfony/mcp-bundle 依存追加と /admin/mcp ルート配線
- AllowListResolver: Api44 の core.api.allow_list を再利用するヘルパ
- EntityArraySerializer: allow_list 駆動の Reflection ベース変換 (深さ 2、循環検知)
- McpAuditLogger: mcp チャネルへの単一エントリポイント
- OriginContentTypeListener: ^/admin/mcp 配下の Content-Type / Origin 検証
- SearchProductsTool / GetProductTool / GetProductStockTool: 商品/在庫 3 ツール
(#[IsGranted('ROLE_OAUTH2_MCP:PRODUCT:READ')] + allow_list ベース出力)
認証認可と scope は Api44 (別 PR) に依存。 注文/顧客会員/プラグイン管理の
8 ツール / Rate Limiter / 管理 UI / 受入基準テストは後続。
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- SearchProductsToolTest: scope check + 検索 / limit / offset clamp + allow_list 出力 - GetProductToolTest: id / code 取得 + 不在時の空配列 + allow_list 出力 - GetProductStockToolTest: 規格あり / 規格なし / 在庫無制限 / 不在 + allow_list 出力 UsernamePasswordToken に scope role を渡して TokenStorage に直接セットすることで Tool の AuthorizationChecker を満たす形で結合テストを実現。 Api44 が install + enable されている前提 (core.api.allow_list 経由で出力フィールドが allow_list に従う)。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
実機検証で Master entity が Status: [] と空で返るバグを発見。 原因は Doctrine の Lazy Proxy が返ってきたとき、 自動生成された proxy class 名 (Proxies\__CG__\...) で allow_list を引いて未登録扱いになる経路。 Doctrine\Persistence\Proxy 実装 オブジェクトを get_parent_class で実 entity 名に unwrap してから allow_list を 引くように修正。 同時に DEFAULT_MAX_DEPTH を 2 → 1 に変更。 get_product_stock など root が ProductClass の場合、 Product.ProductClasses[] 経由で兄弟 ProductClass の中身が 大量に重複出力されるノイズを抑止。 必要な Tool は明示的に 2 以上を指定する。 - EntityArraySerializer::resolveEntityClass() で Proxy unwrap - DEFAULT_MAX_DEPTH 2 → 1 - 新規テスト 2 件: Proxy 経路 / 新デフォルト深さ - 既存テスト 16 件は深さ変更後も結果不変 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- SearchOrdersTool: キーワード / 注文番号 / ステータス / 期間 / 金額レンジ / 顧客 ID で検索 OrderRepository::getQueryBuilderBySearchDataForAdmin を流用 - GetOrderTool: 注文 ID または注文番号で詳細取得 - GetShippingTool: 注文に紐づく Shipping 一覧 (出荷ステータス / 配送日 / 追跡番号 / 配送先) 必要 scope: mcp:order:read。 出力は Api44 の allow_list (Order / Shipping) の項目のみ。 氏名・住所等の PII が含まれ得る (設計どおり、 scope 付与で運用統制)。 各 Tool に結合テスト追加 (14 件、 全 121 assertions)。 createOrder ヘルパのデフォルト status (PROCESSING) は admin 検索のデフォルト除外条件と衝突するため、 Generator 経由で OrderStatus::NEW の Order を作るヘルパ createOrderInDefaultSearchable を用意。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tomer_orders) - SearchCustomersTool: キーワード / 電話 / ステータス / 登録期間 / 購入合計 / 購入回数で検索 CustomerRepository::getQueryBuilderBySearchData を流用 (email 専用キーは admin 側にないため keyword/multi で兼ねる) - GetCustomerTool: 会員 ID から詳細取得 - GetCustomerOrdersTool: 指定会員の購入履歴を OrderRepository::getQueryBuilderByCustomer 経由で取得 必要 scope: mcp:customer:read。 出力は Api44 の allow_list (Customer / Order) の項目のみ。 氏名・メール・電話・住所等の PII を含み得る (設計どおり、 scope 付与で運用統制)。 各 Tool に結合テスト追加 (12 件、 全 71 assertions)。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- ListPluginsTool: インストール済みプラグイン一覧 (enabledOnly フィルタ可)。 出力は Plugin entity の allow_list (id / name / code / version / enabled / initialized 等) のみ - GetPluginTool: id または code から Plugin entity 詳細 + app/Plugin/<code>/composer.json の description / require をマージ (依存関係を AI が見る用途)。 個別プラグインの設定値 (API キー等の機微データ) には踏み込まない 必要 scope: mcp:plugin:read。 設計 §5 のプラグイン管理境界 (メタ情報まで、 設定値は別 scope の get_plugin_settings として将来検討) に従う。 各 Tool に結合テスト追加 (9 件、 全 42 assertions)。 Api44 自身が install 済みである前提で Api44 を題材に詳細取得・composer.json マージを検証。 これにより設計の全 11 ツール (商品/在庫 3 + 注文 3 + 顧客会員 3 + プラグイン管理 2) が 出揃った。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- ScopeChecker は scope 不足時に ToolCallException を投げる - mcp-bundle が catch し result.isError=true + content text に整形 - role 名を OAuth2 scope 名 (mcp:order:read) に変換して返す - mcp-bundle が Tool 内例外を catch するため HTTP 403 化は不可 (docs/mcp/scope-denied-response.md) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- mcp-bundle は空配列を text では []、 structuredContent では {} と出し分ける
- Get 系 4 Tool の不在分岐を 'data' => ['found' => false] に変更
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- mcp_ip: IP 単位 60/分。 kernel.request priority 14 で admin firewall より先に消費 (認証エラー連発も抑制) - mcp_client: client_id 単位 300/分。 firewall 通過後 OAuth2Token から取得し kernel.controller で消費 - 超過時 429 + retry_after_seconds、 Retry-After / X-RateLimit-* ヘッダ付与 - 監査ログに rate_limited を記録 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- ToolsListContractTest: 11 Tool の DI 登録と #[McpTool] name 一致を検証 - AllowListContractTest: Product/Customer/Order の出力 keys が allow_list の subset、 かつ空でないことを検証 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- NoDirectMcpLoggerInjectionRule: __construct で LoggerInterface $mcpLogger を持つクラスを検出 - McpAuditLogger 以外は error (eccube.mcp.directLoggerInjection) - 監査ログを McpAuditLogger に一本化する規律を core で静的に強制 - カスタマイズ側は phpstan の paths 拡張で同規律を適用できる旨を docblock に明記 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Bearer なし → 401 (oauth2 entry point) - 不正な opaque Bearer → 401 (league validator) - 署名不正な JWT → 401 (signature 検証) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- league の AccessToken + CryptKey で JWT を自前発行 - revoke() 後の JWT → 401 - Member.Work=NON_ACTIVE → MemberProvider 解決失敗で 401 - McpFirewallContractTest の HTTP method / status を定数化 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Api44 が install + enabled + initialized であることを検証 - /admin/mcp の FirewallMap 解決が mcp (stateless oauth2) - /admin/ は admin firewall (cookie based) のまま - McpScope::ROLE_* 定数が §4.1 の scope 文字列と一致 - 「無効化で消える」 は kernel reboot が必要なため手動確認で代替 (docblock 明記) - rector.php: ContainerGetNameToTypeInTestsRector を本テストのみ skip (private service ID は test container で FQCN 解決不可) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- ScopeEnforcingReferenceHandler: Builder::setReferenceHandler に差し込み全 Tool 呼び出しが通過。 McpToolScopeMap で必要 scope を引き、 未登録は fail-closed deny / 不足は ToolCallException / 非 Tool は素通し - McpToolScopeMap: tool 名 → 必要 role の唯一の中央定義 (未登録は全 deny) - McpScopeEnforcementPass: McpPass の Tool ServiceLocator を再利用して inner ReferenceHandler を構築し setReferenceHandler に配線 (優先度 -100) - ToolInvoker: requiredScope と ScopeChecker 呼び出しを撤去 (audit + 計時のみ) - 11 Tool: invoke() から requiredScope を削除 - テスト: 各 Tool の scope テストを ScopeEnforcingReferenceHandlerTest に集約 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- McpAuditLoggerChannelLockPass: $mcpLogger の autowire alias のみ削除し、 mcp チャンネルへの到達を @monolog.logger.mcp 名指しのみに限定 - 他クラスが $mcpLogger を注入しても default チャンネルに解決され mcp は汚れない - LoggerChannelPass の後・AutowirePass の前に走らせるため before-optimization 負優先度で登録 - McpAuditLogger は @monolog.logger.mcp を名指しバインド - NoDirectMcpLoggerInjectionRule と neon 登録を撤去 (DI 方式に一本化し PHPStan を回さなくても常時有効) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- consume() 例外時は 503 rate_limiter_unavailable を返す - 監査ログに InternalError (reason: rate_limiter_unavailable) を記録 - IP / client_id の consume を共通 check() に集約 - 黙ってカウンタを失う劣化 (Redis ダウン時の miss 等) は本層で検知不可 (docblock 明記) - storage 例外で 503 になることをテスト Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- 「401 でない」 から 200 + result 検証に強化し偽陽性を防ぐ - ensureClient の戻り型を ClientInterface に修正 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
scope 未登録の Tool は ScopeEnforcingReferenceHandler が実行時に fail-closed で deny するため、 登録漏れが本番呼び出しまで気付かれない。 これを CI で拾う。 McpToolScopeMapContractTest: - Tool ディレクトリを実走査して #[McpTool] を全件発見し、 各 tool 名が McpToolScopeMap に登録されていることを検証 (登録漏れゼロを保証) - 逆に McpToolScopeMap の各エントリが実在 Tool に対応することも検証 (typo / 削除済み Tool の残骸を検出) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mcp チャンネルが存在するのに想定 id の autowire alias が見つからない場合 (monolog のバージョン/命名規約変更で id がズレた等)、 削除が空振りして 監査チャンネルが誰でも書ける状態に黙って戻る。 これを LogicException で build を止めて検出する (fail loud)。 mcp チャンネル自体が無い構成では 保護対象も無いのでスキップする。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- McpScopeEnforcementPassTest: builder への setReferenceHandler 差し込みと inner 構築を検証 - McpScopeEnforcementIntegrationTest: 実カーネルに tools/call を流し、 充足は result / 不足は isError:true + Insufficient scope を確認 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- safeAudit() で監査ログの例外を握り潰し 503 / 429 の返却を保証 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- safeAudit は監査例外を握り潰しつつ、 default チャンネルに失敗を 1 行記録 - 拒否 (429/503) は維持しつつ mcp 監査チャンネル障害を可観測にする - フォールバックは $logger (default チャンネル) 経由で mcp チャンネルは汚さない - 監査失敗でも 429 が返り fallback に記録されることをテスト Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- revoked / Member 無効化の両 401 で WWW-Authenticate: Bearer を確認 (oauth2 の bearer 拒否であることを担保) - Member 無効化は body "Bad credentials" で user 解決失敗の経路を識別 (token 拒否と区別) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- mcp チャネル専用ハンドラ (rotating_file, info から常時出力) を追加 - 出力先 var/log/<env>/mcp.log、 権限 0640、 保管 90 日 (ECCUBE_MCP_LOG_RETENTION_DAYS) - 1 レコード 1 JSON (eccube.mcp.log.formatter.json) - prod / dev の main(site.log) から mcp チャネルを除外し PII の site.log 流入を防ぐ Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- mcp チャネルに書いたログが mcp.log に出て site.log に漏れないことを検証 (実書き込みで確認) - prod / dev の main ハンドラが mcp チャネルを除外する設定であることを検証 - rector.php: monolog.logger.mcp を ID 取得するため ContainerGetNameToTypeInTestsRector を本テストで skip Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- AuthFailureAuditListener: mcp パスの 401 レスポンスを kernel.response で拾い logAuthEvent(TokenInvalid) を記録 (無トークン・無効トークン両方を捕捉) - mcp パスで 401 を返すのは認証失敗のみ (scope 拒否=200 / rate=429,503 / origin=403,415) なので誤検知しない - 監査書き込み失敗は default チャネルに記録して握り潰し、 401 応答を壊さない - TokenInvalid のログレベルを warning に (error はサーバ障害 InternalError 専用) - 未参照の AuditResult::ValidationError を削除 - AuditResultUsageTest: 全 case が src から参照されること (孤児 case 防止) を検証 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- WWW-Authenticate あり → reason にヘッダ値が入る - WWW-Authenticate なし → reason は fallback 'unauthorized' - 匿名 logger の log() に context の型注釈を付与 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough歩き通しEC-CUBE に Model Context Protocol（MCP）サーバ機能を包括的に統合した。OAuth2 ベースの認証、中央集約された Scope 強制、2 段階レート制限、11 個のビジネス用 Tool、構造化監査ログ、50+ 件のテストスイートを実装。 変更内容主要なコンポーネント:
推定レビュー工数🎯 4 (複雑) | ⏱️ ~60 分 理由:
関連する Issue
推奨ラベル
推奨レビュアー
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
codeception/_data/plugins/Bundle-1.0.0/DependencyInjection/Compiler/BundleCompilerPass.php (1)
22-22: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win戻り値の型宣言を追加してください。
process()メソッドに: voidの戻り値型宣言がありません。コーディングガイドラインに従い、PHPStan level 6 を通すために型宣言が必要です。🔧 提案する修正
- public function process(ContainerBuilder $container) + public function process(ContainerBuilder $container): void {🤖 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 `@codeception/_data/plugins/Bundle-1.0.0/DependencyInjection/Compiler/BundleCompilerPass.php` at line 22, BundleCompilerPass クラスの process() メソッドに戻り値型宣言がないため PHPStan を通りません。クラス内の public function process(ContainerBuilder $container) を見つけて、シグネチャに `: void` を追加して戻り値型を明示してください（メソッド名: process、クラス名: BundleCompilerPass、型: void）。Source: Coding guidelines
codeception/_data/plugins/Bundle-1.0.1/DependencyInjection/Compiler/BundleCompilerPass.php (1)
22-22: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win戻り値の型宣言を追加してください。
process()メソッドに: voidの戻り値型宣言がありません。コーディングガイドラインに従い、PHPStan level 6 を通すために型宣言が必要です。🔧 提案する修正
- public function process(ContainerBuilder $container) + public function process(ContainerBuilder $container): void {🤖 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 `@codeception/_data/plugins/Bundle-1.0.1/DependencyInjection/Compiler/BundleCompilerPass.php` at line 22, process() メソッドに戻り値の型宣言が抜けているため、メソッド宣言 `public function process(ContainerBuilder $container)` を `public function process(ContainerBuilder $container): void` のように戻り値型 `: void` を追加して型宣言に準拠させてください（該当メソッドは BundleCompilerPass クラスの process メソッドです）。Source: Coding guidelines
🧹 Nitpick comments (2)
tests/Eccube/Tests/Service/Mcp/EntityArraySerializerTest.php (1)
267-270: 💤 Low value
crc32()の代わりに static カウンタの検討を推奨テスト用の疑似 ID 生成に
crc32()を使用していますが、以下の点を考慮すると、よりシンプルな実装が望ましい可能性があります：
crc32()は衝突の可能性があり、負の値を返す場合もある- テストの意図としては、null でない一意の ID があれば十分
代替案として、static カウンタを使用する方法があります：
♻️ よりシンプルな実装案
final class SerializerDummyRelated { + private static int $idCounter = 1; public ?string $code = null; public ?SerializerDummyNested $nested = null; public ?SerializerDummyEntity $back = null; public function getId(): ?int { - return null === $this->code ? null : crc32($this->code); + return null === $this->code ? null : self::$idCounter++; } }ただし、現在のテストで使用される値（'X-001', 'A', 'B' など）では実用上問題はありません。
🤖 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 `@tests/Eccube/Tests/Service/Mcp/EntityArraySerializerTest.php` around lines 267 - 270, The getId() method uses crc32($this->code) which can collide and yield negative values; replace this with a simple static counter: add a private property (e.g. $id) and in getId() (method name getId) if $this->id is null and $this->code is not null assign a positive integer from a static counter (e.g. static $next = 1; $this->id = $next++;), then return $this->id (keeping the signature getId(): ?int) so each instance gets a stable, unique, positive test ID.tests/Eccube/Tests/Service/Mcp/Tool/SearchOrdersToolTest.php (1)
72-98: ⚡ Quick winallow_list のハードコードにより保守性が低下しています。
allowedフィールドリスト（行 81-91）がハードコードされています。Api44 の allow_list が変更されるとテストも更新が必要になります。AllowListContractTestが既にAllowListResolverを使って動的に同じ契約を検証しているため、このテストは重複しています。
AllowListResolverから動的に許可フィールドを取得することで、保守性が向上します。♻️ AllowListResolver から動的取得する修正案
+use Eccube\Service\Mcp\AllowListResolver; + final class SearchOrdersToolTest extends EccubeTestCase { private ?SearchOrdersTool $tool = null; + private ?AllowListResolver $resolver = null; public function setUp(): void { parent::setUp(); $this->tool = static::getContainer()->get(SearchOrdersTool::class); + $this->resolver = static::getContainer()->get(AllowListResolver::class); } public function testItemFieldsAreSubsetOfAllowList(): void { $customer = $this->createCustomer('mcp-order-allow@example.com'); $this->createOrderInDefaultSearchable($customer); $result = $this->tool->search(limit: 5); $this->assertNotEmpty($result['items']); - // Api44 の allow_list の `Eccube\Entity\Order` 列挙項目 - $allowed = [ - 'id', 'pre_order_id', 'order_no', 'message', - 'name01', 'name02', 'kana01', 'kana02', 'company_name', 'email', 'phone_number', - 'postal_code', 'addr01', 'addr02', 'birth', - 'subtotal', 'discount', 'delivery_fee_total', 'charge', 'tax', 'total', 'payment_total', - 'payment_method', 'note', 'create_date', 'update_date', 'order_date', 'payment_date', - 'currency_code', 'complete_message', 'complete_mail_message', 'add_point', 'use_point', - 'OrderItems', 'Shippings', 'MailHistories', 'Customer', 'Country', 'Pref', - 'Sex', 'Job', 'Payment', 'DeviceType', - 'CustomerOrderStatus', 'OrderStatusColor', 'OrderStatus', - ]; + $allowed = $this->resolver->getAllowedProperties(Order::class); foreach ($result['items'] as $item) { foreach (array_keys($item) as $key) { $this->assertContains($key, $allowed, sprintf('出力フィールド "%s" は allow_list 外', $key)); } } }🤖 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 `@tests/Eccube/Tests/Service/Mcp/Tool/SearchOrdersToolTest.php` around lines 72 - 98, Replace the hardcoded $allowed array in testItemFieldsAreSubsetOfAllowList with the runtime allow-list from AllowListResolver: retrieve AllowListResolver (e.g. via the test container or DI), call its resolver for the Eccube\Entity\Order allow list to get the allowed field names, then use that returned list in the existing loop over $result['items'] so the assertion still checks each $key against the dynamic allow-list instead of the hardcoded $allowed.
🤖 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 `@composer.json`:
- Line 80: composer.json の依存指定で "symfony/mcp-bundle": "^0.9" は Packagist
上に存在しないため依存解決が失敗しています；composer.json の該当エントリ（"symfony/mcp-bundle"）を Packagist
に存在するバージョン（例: 利用可能な 0.8.x や 1.x 系のいずれか）へ書き換え、"prefer-stable": true
の設定に合う形式でバージョン制約を更新した上で composer update を実行して依存解決が通ることを確認してください。
In `@src/Eccube/DependencyInjection/Compiler/McpScopeEnforcementPass.php`:
- Around line 44-59: The current code always defines the
eccube.mcp.reference_handler.inner (via INNER_REFERENCE_HANDLER_ID and new
Definition(ReferenceHandler::class)) before checking for mcp.server.builder,
which leaves the MCP ReferenceHandler defined even when the builder/bundle is
absent; move the guard earlier so that if
(!$container->hasDefinition('mcp.server.builder')) you return/skip wiring, or
alternatively move the setDefinition(...) call for INNER_REFERENCE_HANDLER_ID
inside the hasDefinition('mcp.server.builder') branch and only then set the
ReferenceHandler and call setReferenceHandler with new
Reference(ScopeEnforcingReferenceHandler::class); ensure resolveToolLocator(...)
is only called when needed or handled safely if left outside.
In `@src/Eccube/Service/Mcp/EntityArraySerializer.php`:
- Around line 148-161: The summarize(object $entity): array method currently
always returns ['id' => $id] when getId() exists, bypassing the serializer's
allow_list; change summarize to first determine the entity's class (e.g.,
get_class($entity)) and consult the serializer's allow_list for that class (the
same allow_list logic used by summarizeMany or the main serialization flow) and
only return ['id' => $id] if the allow_list for that class explicitly permits
'id'; keep the existing try/catch around getId() and otherwise return [] (do not
expose id when not allowed).
In `@src/Eccube/Service/Mcp/Tool/GetOrderTool.php`:
- Around line 79-80: The check uses trim($orderNo) but still passes the
untrimmed $orderNo into the repository search; update the logic in GetOrderTool
(the method using $orderNo and $this->orderRepository->findOneBy) to compute a
trimmed value (e.g. $trimmed = trim($orderNo)), use that for the empty check and
pass $trimmed into findOneBy(['order_no' => $trimmed]) so inputs with
surrounding whitespace are matched correctly.
In `@src/Eccube/Service/Mcp/Tool/GetPluginTool.php`:
- Around line 79-86: In resolvePlugin(?int $id, ?string $code): ?Plugin,
normalize the incoming $code by trimming whitespace into a local variable (e.g.,
$normalizedCode) and use that for both the empty check and the repository
lookup; specifically, replace uses of trim($code) in the condition with the
normalized value and pass $normalizedCode to
$this->pluginRepository->findByCode($normalizedCode) so inputs like " MyPlugin "
will be found.
In `@src/Eccube/Service/Mcp/Tool/SearchCustomersTool.php`:
- Around line 139-144: The date parsing for createDateFrom/createDateTo in
SearchCustomersTool.php can throw exceptions; wrap the DateTime construction in
validation/exception handling: for each of createDateFrom and createDateTo check
the trimmed string, attempt to create a DateTime using
DateTime::createFromFormat or try-catch around new \DateTime(...), verify
success (or check for false/errors via getLastErrors()), and only set
searchData['create_date_start'] / searchData['create_date_end'] when parsing
succeeds; on failure skip setting the field and optionally log or add a
validation error so the Tool does not crash.
In `@src/Eccube/Service/Mcp/ToolInvoker.php`:
- Around line 64-75: The code currently normalizes a missing/invalid
$outcome['data'] into [] and always logs AuditResult::Success; instead detect
when $outcome is missing the 'data' key or when $data is not an array and treat
that as an internal error: in ToolInvoker (around variables $outcome, $data,
$summary) validate presence and type of $outcome['data'] before logging, and if
invalid call $this->auditLogger->logToolCall(...) with result
AuditResult::InternalError (include duration via $this->elapsedMs($startedAt)
and any available $summary/error detail), and propagate an error state to the
caller (e.g., return null or throw) rather than returning a silently-normalized
empty array; only log AuditResult::Success when $data exists and is an array.
In `@tests/Eccube/Tests/Service/Mcp/AuditResultUsageTest.php`:
- Line 66: file_get_contents() can return false on failure and the current
(string) cast hides that, so change the code to capture the result into a
variable (e.g., $data = file_get_contents($file->getPathname())), check if $data
=== false, and if so fail the test or throw a clear exception (use
$this->fail(...) or throw new \RuntimeException) including the
$file->getPathname() in the message; otherwise append $data to $contents. Ensure
you update the usage around $contents and the call to file_get_contents() in
AuditResultUsageTest so failures are not silently ignored.
In `@tests/Eccube/Tests/Service/Mcp/Tool/GetCustomerOrdersToolTest.php`:
- Around line 34-45: The test only checks counts and limit so it will pass with
unrelated existing data; update
GetCustomerOrdersToolTest::testReturnsCustomerOrders to assert that the specific
fixtures are present in the returned items: after creating $customer via
createCustomer(...) and two orders via createOrder($customer) call
$this->tool->get(...) and extract result['items'], then assert that at least one
item has customer_id === $customer->getId() (or assert the created order IDs are
present in result['items']), and also assert items is not empty to guard
enabledOnly behaviour; use the existing helpers createCustomer, createOrder and
the $this->tool->get method and reference result['items'] when adding the
assertions.
- Around line 1-14: Move the EC-CUBE license header so it appears immediately
after the opening "<?php" tag and before the "declare(strict_types=1);"
statement; for the test file (and the other five files flagged) cut the entire
license comment block that currently sits after declare(strict_types=1); and
paste it directly below "<?php", then leave a single blank line and keep
"declare(strict_types=1);" as the next statement to restore the required header
order.
---
Outside diff comments:
In
`@codeception/_data/plugins/Bundle-1.0.0/DependencyInjection/Compiler/BundleCompilerPass.php`:
- Line 22: BundleCompilerPass クラスの process() メソッドに戻り値型宣言がないため PHPStan
を通りません。クラス内の public function process(ContainerBuilder $container) を見つけて、シグネチャに
`: void` を追加して戻り値型を明示してください（メソッド名: process、クラス名: BundleCompilerPass、型: void）。
In
`@codeception/_data/plugins/Bundle-1.0.1/DependencyInjection/Compiler/BundleCompilerPass.php`:
- Line 22: process() メソッドに戻り値の型宣言が抜けているため、メソッド宣言 `public function
process(ContainerBuilder $container)` を `public function
process(ContainerBuilder $container): void` のように戻り値型 `: void`
を追加して型宣言に準拠させてください（該当メソッドは BundleCompilerPass クラスの process メソッドです）。
---
Nitpick comments:
In `@tests/Eccube/Tests/Service/Mcp/EntityArraySerializerTest.php`:
- Around line 267-270: The getId() method uses crc32($this->code) which can
collide and yield negative values; replace this with a simple static counter:
add a private property (e.g. $id) and in getId() (method name getId) if
$this->id is null and $this->code is not null assign a positive integer from a
static counter (e.g. static $next = 1; $this->id = $next++;), then return
$this->id (keeping the signature getId(): ?int) so each instance gets a stable,
unique, positive test ID.
In `@tests/Eccube/Tests/Service/Mcp/Tool/SearchOrdersToolTest.php`:
- Around line 72-98: Replace the hardcoded $allowed array in
testItemFieldsAreSubsetOfAllowList with the runtime allow-list from
AllowListResolver: retrieve AllowListResolver (e.g. via the test container or
DI), call its resolver for the Eccube\Entity\Order allow list to get the allowed
field names, then use that returned list in the existing loop over
$result['items'] so the assertion still checks each $key against the dynamic
allow-list instead of the hardcoded $allowed.
🪄 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: 52587da4-f669-43b0-939e-59080f6a3feb
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (70)
app/config/eccube/bundles.phpapp/config/eccube/packages/dev/monolog.ymlapp/config/eccube/packages/http_discovery.yamlapp/config/eccube/packages/mcp.yamlapp/config/eccube/packages/mcp_rate_limiter.yamlapp/config/eccube/packages/monolog.ymlapp/config/eccube/packages/prod/monolog.ymlapp/config/eccube/routes.yamlapp/config/eccube/services.yamlcodeception/_data/plugins/Bundle-1.0.0/DependencyInjection/Compiler/BundleCompilerPass.phpcodeception/_data/plugins/Bundle-1.0.1/DependencyInjection/Compiler/BundleCompilerPass.phpcomposer.jsondocs/mcp/scope-denied-response.mdrector.phpsrc/Eccube/DependencyInjection/Compiler/McpAuditLoggerChannelLockPass.phpsrc/Eccube/DependencyInjection/Compiler/McpScopeEnforcementPass.phpsrc/Eccube/EventListener/Mcp/AuthFailureAuditListener.phpsrc/Eccube/EventListener/Mcp/OriginContentTypeListener.phpsrc/Eccube/EventListener/Mcp/RateLimitListener.phpsrc/Eccube/Kernel.phpsrc/Eccube/Service/Mcp/AllowListResolver.phpsrc/Eccube/Service/Mcp/AuditResult.phpsrc/Eccube/Service/Mcp/EntityArraySerializer.phpsrc/Eccube/Service/Mcp/McpAuditLogger.phpsrc/Eccube/Service/Mcp/McpScope.phpsrc/Eccube/Service/Mcp/McpToolScopeMap.phpsrc/Eccube/Service/Mcp/ScopeChecker.phpsrc/Eccube/Service/Mcp/ScopeEnforcingReferenceHandler.phpsrc/Eccube/Service/Mcp/Tool/GetCustomerOrdersTool.phpsrc/Eccube/Service/Mcp/Tool/GetCustomerTool.phpsrc/Eccube/Service/Mcp/Tool/GetOrderTool.phpsrc/Eccube/Service/Mcp/Tool/GetPluginTool.phpsrc/Eccube/Service/Mcp/Tool/GetProductStockTool.phpsrc/Eccube/Service/Mcp/Tool/GetProductTool.phpsrc/Eccube/Service/Mcp/Tool/GetShippingTool.phpsrc/Eccube/Service/Mcp/Tool/ListPluginsTool.phpsrc/Eccube/Service/Mcp/Tool/SearchCustomersTool.phpsrc/Eccube/Service/Mcp/Tool/SearchOrdersTool.phpsrc/Eccube/Service/Mcp/Tool/SearchProductsTool.phpsrc/Eccube/Service/Mcp/ToolInvoker.phptests/Eccube/Tests/DependencyInjection/Compiler/McpScopeEnforcementPassTest.phptests/Eccube/Tests/EventListener/Mcp/AuthFailureAuditListenerTest.phptests/Eccube/Tests/EventListener/Mcp/OriginContentTypeListenerTest.phptests/Eccube/Tests/EventListener/Mcp/RateLimitListenerTest.phptests/Eccube/Tests/Service/Mcp/AllowListResolverTest.phptests/Eccube/Tests/Service/Mcp/AuditResultUsageTest.phptests/Eccube/Tests/Service/Mcp/Contract/AllowListContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/Api44LifecycleContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpAuditLogIsolationContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpFirewallContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpTokenRevocationContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpToolScopeMapContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/ToolsListContractTest.phptests/Eccube/Tests/Service/Mcp/EntityArraySerializerTest.phptests/Eccube/Tests/Service/Mcp/FakeAllowList.phptests/Eccube/Tests/Service/Mcp/McpAuditLoggerTest.phptests/Eccube/Tests/Service/Mcp/RecordingReferenceHandler.phptests/Eccube/Tests/Service/Mcp/ScopeEnforcingReferenceHandlerTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetCustomerOrdersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetCustomerToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetOrderToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetPluginToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetProductStockToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetProductToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetShippingToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/ListPluginsToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchCustomersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchOrdersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php
Comment on lines
+44
to
+59
| public function process(ContainerBuilder $container): void | ||
| { | ||
| // 本物の Tool 実行器に渡す Tool ServiceLocator を決める。 | ||
| // mcp-bundle が builder->setContainer に渡したロケータをそのまま再利用し、 集合の乖離を無くす。 | ||
| // (builder 不在 = mcp-bundle 未導入時は mcp.tool タグから自前収集する安全フォールバック) | ||
| $toolLocator = $this->resolveToolLocator($container); | ||
|
|
||
| $container->setDefinition( | ||
| self::INNER_REFERENCE_HANDLER_ID, | ||
| (new Definition(ReferenceHandler::class))->setArguments([$toolLocator]), | ||
| ); | ||
|
|
||
| // 全 Tool 呼び出しが通る referenceHandler を scope 強制版に差し替える。 | ||
| if ($container->hasDefinition('mcp.server.builder')) { | ||
| $container->getDefinition('mcp.server.builder') | ||
| ->addMethodCall('setReferenceHandler', [new Reference(ScopeEnforcingReferenceHandler::class)]); |
There was a problem hiding this comment.
mcp.server.builder 不在時のガードが遅すぎます。
Line 49-54 で eccube.mcp.reference_handler.inner を常に定義してから Line 57 の hasDefinition('mcp.server.builder') を見ているため、builder が無い構成でも MCP 用の ReferenceHandler 定義だけは残ります。今回の意図は「builder 不在なら配線をスキップする」なので、ここは先に return しないと、mcp-bundle の配線点が無い環境でコンテナコンパイルを壊す側に倒れます。
修正案
#[\Override]
public function process(ContainerBuilder $container): void
{
+ if (!$container->hasDefinition('mcp.server.builder')) {
+ return;
+ }
+
// 本物の Tool 実行器に渡す Tool ServiceLocator を決める。
// mcp-bundle が builder->setContainer に渡したロケータをそのまま再利用し、 集合の乖離を無くす。
// (builder 不在 = mcp-bundle 未導入時は mcp.tool タグから自前収集する安全フォールバック)
$toolLocator = $this->resolveToolLocator($container);
$container->setDefinition(
self::INNER_REFERENCE_HANDLER_ID,
(new Definition(ReferenceHandler::class))->setArguments([$toolLocator]),
);
- // 全 Tool 呼び出しが通る referenceHandler を scope 強制版に差し替える。
- if ($container->hasDefinition('mcp.server.builder')) {
- $container->getDefinition('mcp.server.builder')
- ->addMethodCall('setReferenceHandler', [new Reference(ScopeEnforcingReferenceHandler::class)]);
- }
+ // 全 Tool 呼び出しが通る referenceHandler を scope 強制版に差し替える。
+ $container->getDefinition('mcp.server.builder')
+ ->addMethodCall('setReferenceHandler', [new Reference(ScopeEnforcingReferenceHandler::class)]);
}🤖 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 `@src/Eccube/DependencyInjection/Compiler/McpScopeEnforcementPass.php` around
lines 44 - 59, The current code always defines the
eccube.mcp.reference_handler.inner (via INNER_REFERENCE_HANDLER_ID and new
Definition(ReferenceHandler::class)) before checking for mcp.server.builder,
which leaves the MCP ReferenceHandler defined even when the builder/bundle is
absent; move the guard earlier so that if
(!$container->hasDefinition('mcp.server.builder')) you return/skip wiring, or
alternatively move the setDefinition(...) call for INNER_REFERENCE_HANDLER_ID
inside the hasDefinition('mcp.server.builder') branch and only then set the
ReferenceHandler and call setReferenceHandler with new
Reference(ScopeEnforcingReferenceHandler::class); ensure resolveToolLocator(...)
is only called when needed or handled safely if left outside.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat src/Eccube/DependencyInjection/Compiler/McpScopeEnforcementPass.phpLength of output: 3450
@dotani1111 現在のコードを確認しましたが、まだ修正が反映されていないようです。process() メソッドは依然として以下の順序になっています：
resolveToolLocator()を無条件で呼び出すeccube.mcp.reference_handler.innerを無条件で定義する- その後に
hasDefinition('mcp.server.builder')をチェックする
以下のように早期リターンのガードを追加してください：
#[\Override]
public function process(ContainerBuilder $container): void
{
if (!$container->hasDefinition('mcp.server.builder')) {
return;
}
// 本物の Tool 実行器に渡す Tool ServiceLocator を決める。
// mcp-bundle が builder->setContainer に渡したロケータをそのまま再利用し、 集合の乖離を無くす。
$toolLocator = $this->resolveToolLocator($container);
$container->setDefinition(
self::INNER_REFERENCE_HANDLER_ID,
(new Definition(ReferenceHandler::class))->setArguments([$toolLocator]),
);
// 全 Tool 呼び出しが通る referenceHandler を scope 強制版に差し替える。
$container->getDefinition('mcp.server.builder')
->addMethodCall('setReferenceHandler', [new Reference(ScopeEnforcingReferenceHandler::class)]);
}コミットをプッシュ後に再度ご確認ください。
Comment on lines
+1
to
+14
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /* | ||
| * This file is part of EC-CUBE | ||
| * | ||
| * Copyright(c) EC-CUBE CO.,LTD. All Rights Reserved. | ||
| * | ||
| * http://www.ec-cube.co.jp/ | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ |
There was a problem hiding this comment.
ライセンスヘッダを declare(strict_types=1); より前に置いてください。
6 ファイルとも EC-CUBE ライセンスヘッダがファイル先頭ではなく declare の後ろにあります。ガイドライン準拠にするなら、<?php の直後にライセンスヘッダ、その後に declare(strict_types=1); の順にそろえる必要があります。
As per coding guidelines, 「PHP ファイルの先頭に 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 `@tests/Eccube/Tests/Service/Mcp/Tool/GetCustomerOrdersToolTest.php` around
lines 1 - 14, Move the EC-CUBE license header so it appears immediately after
the opening "<?php" tag and before the "declare(strict_types=1);" statement; for
the test file (and the other five files flagged) cut the entire license comment
block that currently sits after declare(strict_types=1); and paste it directly
below "<?php", then leave a single blank line and keep
"declare(strict_types=1);" as the next statement to restore the required header
order.
Source: Coding guidelines
Comment on lines
+34
to
+45
| public function testReturnsCustomerOrders(): void | ||
| { | ||
| $customer = $this->createCustomer('mcp-customer-orders@example.com'); | ||
| $this->createOrder($customer); | ||
| $this->createOrder($customer); | ||
|
|
||
| $result = $this->tool->get(customerId: $customer->getId(), limit: 50); | ||
|
|
||
| $this->assertSame($customer->getId(), $result['customer_id']); | ||
| $this->assertGreaterThanOrEqual(2, $result['total']); | ||
| $this->assertSame(50, $result['limit']); | ||
| } |
There was a problem hiding this comment.
件数しきい値だけの検証では回帰を取り逃します。
GetCustomerOrdersToolTest、ListPluginsToolTest、SearchCustomersToolTest、SearchProductsToolTest の該当テストは、作成した fixture 自体が items に含まれることを確認していません。既存データが 1 件でもあれば緑になり、enabledOnly 側は空配列でも通るので、検索条件やフィルタが壊れても見逃します。作成した顧客 / 商品 / プラグインコード、または返却 items の対象 ID まで固定して検証したほうが安全です。
🤖 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 `@tests/Eccube/Tests/Service/Mcp/Tool/GetCustomerOrdersToolTest.php` around
lines 34 - 45, The test only checks counts and limit so it will pass with
unrelated existing data; update
GetCustomerOrdersToolTest::testReturnsCustomerOrders to assert that the specific
fixtures are present in the returned items: after creating $customer via
createCustomer(...) and two orders via createOrder($customer) call
$this->tool->get(...) and extract result['items'], then assert that at least one
item has customer_id === $customer->getId() (or assert the created order IDs are
present in result['items']), and also assert items is not empty to guard
enabledOnly behaviour; use the existing helpers createCustomer, createOrder and
the $this->tool->get method and reference result['items'] when adding the
assertions.
- 深さ超過/循環時の summarize() が getId を無条件に返し、 allow_list に id を持たない関連 Entity の内部 id を露出していた - isAllowed(class, 'id') を確認してから返し、 「allow_list のみ公開」の不変条件を要約経路でも維持する - id 非許可かつ getId ありの要約で id が漏れないことをガードするテストを追加 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- order_no / plugin code / product code は空判定だけでなく検索値もトリムし、 前後に空白を含む入力の取りこぼしを防ぐ - create_date / order_date は不正書式を InvalidArgumentException に変換し、 new DateTime() の不透明な例外をそのまま投げない Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- data キー欠落・非配列を空の正常応答に正規化せず、 InternalError を記録して例外送出する - 実装バグが空の成功応答に化け、 クライアント・監査ログともに内部エラーと区別できない経路を塞ぐ Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- 件数しきい値だけでは既存データで緑になりフィルタ回帰を見逃すため、 作成した fixture の id/code が items に含まれることを検証する - ListPlugins の enabledOnly は空配列でも通るため、 非空を先に担保する - AuditResultUsageTest の file_get_contents 失敗を例外化し、 孤児 case の偽陰性を防ぐ Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- log() のシグネチャで mixed を宣言済みのため、 @param mixed $level の phpdoc は冗長 - RemoveUselessParamTagRector の dry-run 失敗を解消する Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
OAuth2 は Api44 プラグイン由来で本体の composer 依存に無いため、 本体 src が league の具象クラスを参照すると Api44 未導入環境で phpstan analyse src が落ちる。 - RateLimitListener: OAuth2Token の instanceof を getOAuthClientId() の method_exists 判定に変更し、 league への import を排除 - RateLimitListenerTest: 同メソッドを持つ最小トークンに差し替え、 league 非依存化 - 結合テスト 2 本: league 不在で fatal しないよう interface_exists の skip ガード追加 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.php (1)
173-175: ⚖️ Poor tradeoffプライベートキーのパスがハードコードされています（任意改善）。
Line 173 でプライベートキーのパスが
/app/PluginData/Api44/oauth/private.keyとしてハードコードされています。テスト環境では許容範囲ですが、ファイルが存在しない場合はCryptKeyのコンストラクタで例外が発生します。必要に応じて、ファイル存在チェックやより明示的なエラーメッセージを追加できます。
🤖 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 `@tests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.php` around lines 173 - 175, The hardcoded private key path in the test file at the location where CryptKey is instantiated may not exist and will cause an unhandled exception in the CryptKey constructor with unclear error context. Add a file existence check using file_exists() on the $privateKeyPath variable before passing it to the CryptKey constructor, and throw a more explicit exception with a descriptive error message if the file does not exist, making test failures clearer and easier to debug.tests/Eccube/Tests/EventListener/Mcp/RateLimitListenerTest.php (1)
278-278: 💤 Low valuePHPMD の警告を抑制可能（任意）。
PHPMD が「未使用パラメータ」と報告していますが、これは誤検知です。
$oauthClientIdはプロパティプロモーションで保存され、Line 285 のgetOAuthClientId()で返されています。必要に応じて以下のアノテーションで警告を抑制できます。
♻️ PHPMD 警告を抑制する提案
+ /** + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ private function buildOAuth2Token(string $clientId): TokenInterface🤖 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 `@tests/Eccube/Tests/EventListener/Mcp/RateLimitListenerTest.php` at line 278, Suppress the PHPMD false positive warning for the unused parameter in the constructor of the test class. Add a PHPMD annotation (such as `@SuppressWarnings` or `@codeCoverageIgnore`) above the constructor method that contains the private readonly string $oauthClientId parameter to indicate that this parameter is intentionally used via property promotion and returned in the getOAuthClientId() method, even though PHPMD incorrectly reports it as unused.
🤖 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 `@src/Eccube/Service/Mcp/ToolInvoker.php`:
- Around line 64-75: The issue is that in PHP 8.2/8.3, if the `$work()` function
returns a non-array value, the `array_key_exists()` call on line 66 will throw a
TypeError before the audit logging on lines 67-71 can execute, leaving no audit
trail for the contract violation. Fix this by adding a check to verify that
`$outcome` is an array BEFORE calling `array_key_exists()` on it. Modify the if
condition that currently checks `!\array_key_exists('data', $outcome) ||
!\is_array($outcome['data'])` by prepending `!\is_array($outcome) ||` to ensure
the input is validated as an array first, so the audit logging will execute
regardless of the error type.
---
Nitpick comments:
In `@tests/Eccube/Tests/EventListener/Mcp/RateLimitListenerTest.php`:
- Line 278: Suppress the PHPMD false positive warning for the unused parameter
in the constructor of the test class. Add a PHPMD annotation (such as
`@SuppressWarnings` or `@codeCoverageIgnore`) above the constructor method that
contains the private readonly string $oauthClientId parameter to indicate that
this parameter is intentionally used via property promotion and returned in the
getOAuthClientId() method, even though PHPMD incorrectly reports it as unused.
In
`@tests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.php`:
- Around line 173-175: The hardcoded private key path in the test file at the
location where CryptKey is instantiated may not exist and will cause an
unhandled exception in the CryptKey constructor with unclear error context. Add
a file existence check using file_exists() on the $privateKeyPath variable
before passing it to the CryptKey constructor, and throw a more explicit
exception with a descriptive error message if the file does not exist, making
test failures clearer and easier to debug.
🪄 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: fba9ccc3-0cbd-498c-94c8-e08036b91939
📒 Files selected for processing (18)
src/Eccube/EventListener/Mcp/RateLimitListener.phpsrc/Eccube/Service/Mcp/EntityArraySerializer.phpsrc/Eccube/Service/Mcp/Tool/GetOrderTool.phpsrc/Eccube/Service/Mcp/Tool/GetPluginTool.phpsrc/Eccube/Service/Mcp/Tool/GetProductTool.phpsrc/Eccube/Service/Mcp/Tool/SearchCustomersTool.phpsrc/Eccube/Service/Mcp/Tool/SearchOrdersTool.phpsrc/Eccube/Service/Mcp/ToolInvoker.phptests/Eccube/Tests/EventListener/Mcp/AuthFailureAuditListenerTest.phptests/Eccube/Tests/EventListener/Mcp/RateLimitListenerTest.phptests/Eccube/Tests/Service/Mcp/AuditResultUsageTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpTokenRevocationContractTest.phptests/Eccube/Tests/Service/Mcp/EntityArraySerializerTest.phptests/Eccube/Tests/Service/Mcp/ScopeEnforcingReferenceHandlerTest.phptests/Eccube/Tests/Service/Mcp/Tool/ListPluginsToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchCustomersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php
💤 Files with no reviewable changes (1)
- tests/Eccube/Tests/EventListener/Mcp/AuthFailureAuditListenerTest.php
🚧 Files skipped from review as they are similar to previous changes (13)
- src/Eccube/Service/Mcp/Tool/GetProductTool.php
- src/Eccube/Service/Mcp/Tool/GetOrderTool.php
- tests/Eccube/Tests/Service/Mcp/Tool/SearchCustomersToolTest.php
- tests/Eccube/Tests/Service/Mcp/ScopeEnforcingReferenceHandlerTest.php
- tests/Eccube/Tests/Service/Mcp/AuditResultUsageTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/ListPluginsToolTest.php
- src/Eccube/Service/Mcp/Tool/SearchOrdersTool.php
- src/Eccube/Service/Mcp/Tool/SearchCustomersTool.php
- tests/Eccube/Tests/Service/Mcp/EntityArraySerializerTest.php
- tests/Eccube/Tests/Service/Mcp/Contract/McpTokenRevocationContractTest.php
- src/Eccube/Service/Mcp/Tool/GetPluginTool.php
- src/Eccube/Service/Mcp/EntityArraySerializer.php
Comment on lines
+64
to
+75
| // data キー欠落・非配列は Tool 実装の契約違反。 空の正常応答に化けさせず内部エラーとして扱う | ||
| // (静かに success へ握り潰すと、 クライアントも監査ログも実装バグを検知できない)。 | ||
| if (!\array_key_exists('data', $outcome) || !\is_array($outcome['data'])) { | ||
| $this->auditLogger->logToolCall( | ||
| toolName: $toolName, | ||
| args: $args, | ||
| result: AuditResult::InternalError, | ||
| durationMs: $this->elapsedMs($startedAt), | ||
| ); | ||
|
|
||
| throw new \UnexpectedValueException('Tool result must contain an array `data` key.'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP 8.2/8.3 で、array_key_exists() の第2引数に配列以外を渡した場合はどの例外・エラーになりますか？
💡 Result:
PHP 8.2 および 8.3 において、array_key_exists の第2引数（$array）に配列以外の値（オブジェクト、null、string、int など）を渡した場合、TypeError（具体的には Uncaught TypeError）が発生します [1][2][3]。 エラーメッセージは以下の形式となります [1][3]： Uncaught TypeError: array_key_exists: Argument #2 ($array) must be of type array, %s given ここで、%s の部分は実際に渡された型（例: stdClass, null, string, int など）に置き換わります [1]。 PHP 8.0 より前は、オブジェクトを渡すと配列として扱われる挙動がありましたが、PHP 8.0 でこのサポートは完全に廃止されました [4][5][6]。したがって、現在は厳密な型チェックが行われ、配列以外を渡すと致命的なエラー（Fatal error）となります [1][2]。
Citations:
- 1: https://php-errors.readthedocs.io/en/latest/messages/array_key_exists%28%29%3A-argument-%232-%28%24array%29-must-be-of-type-array%2C-%25s-given.html
- 2: https://stackoverflow.com/questions/75386874/refactor-array-key-exists-in-php8
- 3: https://stackoverflow.com/questions/75892531/jquery-ajax-call-broken-under-php-8-x-php-8-uncaught-typeerror-array-key-exist
- 4: https://php-changed-behaviors.readthedocs.io/en/latest/behavior/array_key_existsOnObjects.html
- 5: https://mirror.math.princeton.edu/pub/php/manual/en/function.array-key-exists.php
- 6: https://phpmyadmin.coders.kiwi.nz/php8/function.array-key-exists.html
🏁 Script executed:
# ファイル全体を確認（行数制限チェック）
wc -l src/Eccube/Service/Mcp/ToolInvoker.phpRepository: EC-CUBE/ec-cube
Length of output: 101
🏁 Script executed:
# ファイル全体を読み込み
cat -n src/Eccube/Service/Mcp/ToolInvoker.phpRepository: EC-CUBE/ec-cube
Length of output: 3502
🏁 Script executed:
# Skill の存在確認
find . -path '*/.claude/skills/service/SKILL.md' 2>/dev/nullRepository: EC-CUBE/ec-cube
Length of output: 93
$work() の戻り値が配列以外の場合、array_key_exists() が先に TypeError を発生させます。
Web 検索で確認した通り、PHP 8.2/8.3 では array_key_exists() の第2引数に配列以外を渡すと Uncaught TypeError が致命的エラーとして発生します。Line 66 の現在の実装では、$work() が配列以外を返した場合、array_key_exists('data', $outcome) が評価される際に TypeError がスローされるため、Line 67-71 の AuditResult::InternalError 監査記録が実行されません。
Tool 実装の契約違反を一元処理し、監査ログを確実に残すため、$outcome 自体が配列かを先に検証してください。
修正案
- if (!\array_key_exists('data', $outcome) || !\is_array($outcome['data'])) {
+ if (!\is_array($outcome) || !\array_key_exists('data', $outcome) || !\is_array($outcome['data'])) {
$this->auditLogger->logToolCall(
toolName: $toolName,
args: $args,
result: AuditResult::InternalError,
durationMs: $this->elapsedMs($startedAt),
);
throw new \UnexpectedValueException('Tool result must contain an array `data` key.');
}🤖 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 `@src/Eccube/Service/Mcp/ToolInvoker.php` around lines 64 - 75, The issue is
that in PHP 8.2/8.3, if the `$work()` function returns a non-array value, the
`array_key_exists()` call on line 66 will throw a TypeError before the audit
logging on lines 67-71 can execute, leaving no audit trail for the contract
violation. Fix this by adding a check to verify that `$outcome` is an array
BEFORE calling `array_key_exists()` on it. Modify the if condition that
currently checks `!\array_key_exists('data', $outcome) ||
!\is_array($outcome['data'])` by prepending `!\is_array($outcome) ||` to ensure
the input is validated as an array first, so the audit logging will execute
regardless of the error type.
MCP は Api44 (OAuth2/scope/allow_list) が前提で、 core CI には Api44 が無いため
これらのテストは本体マトリクスでは成立しない。 skip で握り潰すと Api44 連携の破壊に
気づけないので、 group 分離 + Api44 を入れた専用ジョブでの実走に切り替える。
- Api44 必須の Tool/契約テスト 16 本に #[Group('mcp')] を付与
- メイン unit-test マトリクスは --exclude-group mcp で Api44 非依存に保つ
- mcp 専用ジョブ: eccube-api4@4.4 を mock-package-api 経由で導入 (eccube:composer:require)
し --group mcp を実走
- McpScopeEnforcementIntegrationTest / McpTokenRevocationContractTest の skip ガードを撤去
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php (1)
1-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winライセンスヘッダを
declare(strict_types=1);より前に配置してください。EC-CUBE ライセンスヘッダが
declare(strict_types=1);（Line 3）の後ろ（Lines 5-14）に配置されています。コーディングガイドラインに準拠するため、<?phpの直後にライセンスヘッダ、その後にdeclare(strict_types=1);の順に並べる必要があります。As per coding guidelines, 「PHP ファイルの先頭に EC-CUBE ライセンスヘッダを記載する」。
📝 修正案
Lines 5-14 のライセンスヘッダブロック全体を切り取り、Line 1 の
<?phpの直後に配置し、その後に空行を1行置いてからdeclare(strict_types=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 `@tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php` around lines 1 - 14, The EC-CUBE license header block is incorrectly positioned after the declare(strict_types=1) statement. Move the entire license header comment block (the multi-line comment containing copyright and license information) to appear immediately after the opening PHP tag, then add a blank line, followed by the declare(strict_types=1) statement. This ensures the file structure follows the coding guidelines which require the license header to come first after the opening PHP tag.Source: Coding guidelines
🧹 Nitpick comments (2)
.github/workflows/unit-test.yml (1)
219-225: ⚖️ Poor tradeoffmock-package-api コンテナの起動確認を追加することを検討してください。
Line 224 の
docker runは即座に戻りますが、コンテナが実際にリクエストを受け付けられる状態になるまでに若干の遅延がある可能性があります。Line 248 のeccube:composer:requireが失敗した場合のエラーメッセージが不明瞭になる可能性があります。PostgreSQL の準備状態チェック（Lines 203-205）と同様に、以下のような確認を追加すると CI の診断性が向上します：
- name: Wait for mock-package-api run: | for _ in $(seq 1 30); do curl -sf http://127.0.0.1:8080/health && break || sleep 1; done curl -sf http://127.0.0.1:8080/health || { echo 'mock-package-api did not become ready'; exit 1; }（ヘルスチェックエンドポイントは mock-package-api の実装に依存します）
🤖 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 @.github/workflows/unit-test.yml around lines 219 - 225, The mock-package-api container started on line 224 with docker run returns immediately, but the service may not be ready to accept requests, causing unclear error messages in subsequent operations like the eccube:composer:require step on line 248. Add a new workflow step after the "Setup mock-package-api" step that waits for the service to become healthy by polling its health endpoint (similar to the PostgreSQL readiness check on lines 203-205) with retries, ensuring the mock-package-api is fully ready before dependent tasks execute.tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php (1)
79-100: ⚖️ Poor tradeoffallow_list のハードコードは Api44 変更時に乖離するリスクがあります（任意の改善）。
Lines 88-93 で
Eccube\Entity\Productの許可フィールドをハードコードしています。これはテストの意図を明示的にする利点がありますが、Api44 のcore.api.allow_listが更新された場合に同期が必要になります。より堅牢な代替案として、DI コンテナから
AllowListResolverを取得し、実際の許可リストと照合する方法も検討できます：$resolver = static::getContainer()->get(AllowListResolver::class); $allowed = $resolver->getAllowedProperties(Product::class);現在の実装でもテストは機能しますが、将来的なメンテナンス性を考慮する場合は検討してください。
🤖 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 `@tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php` around lines 79 - 100, The testItemFieldsAreSubsetOfAllowList method hardcodes the allow_list for Product entity fields, which risks becoming out of sync when the Api44 core.api.allow_list configuration is updated. Replace the hardcoded $allowed array (currently listing allowed fields like 'id', 'name', etc.) by retrieving the AllowListResolver from the DI container using static::getContainer()->get(AllowListResolver::class), then calling the getAllowedProperties method with Product::class as the argument to dynamically obtain the actual permitted fields list. This ensures the test always validates against the current configuration rather than a static list.
🤖 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/unit-test.yml:
- Around line 186-194: Both checkout actions in the workflow are missing the
`persist-credentials: false` configuration. Add `persist-credentials: false` to
the "Checkout" action that checks out the main repository and also add
`persist-credentials: false` to the "Checkout Api44 plugin" action that checks
out the eccube-api4 repository. This follows the principle of least privilege by
preventing the GITHUB_TOKEN from being unnecessarily accessible in subsequent
workflow steps.
---
Outside diff comments:
In `@tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php`:
- Around line 1-14: The EC-CUBE license header block is incorrectly positioned
after the declare(strict_types=1) statement. Move the entire license header
comment block (the multi-line comment containing copyright and license
information) to appear immediately after the opening PHP tag, then add a blank
line, followed by the declare(strict_types=1) statement. This ensures the file
structure follows the coding guidelines which require the license header to come
first after the opening PHP tag.
---
Nitpick comments:
In @.github/workflows/unit-test.yml:
- Around line 219-225: The mock-package-api container started on line 224 with
docker run returns immediately, but the service may not be ready to accept
requests, causing unclear error messages in subsequent operations like the
eccube:composer:require step on line 248. Add a new workflow step after the
"Setup mock-package-api" step that waits for the service to become healthy by
polling its health endpoint (similar to the PostgreSQL readiness check on lines
203-205) with retries, ensuring the mock-package-api is fully ready before
dependent tasks execute.
In `@tests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php`:
- Around line 79-100: The testItemFieldsAreSubsetOfAllowList method hardcodes
the allow_list for Product entity fields, which risks becoming out of sync when
the Api44 core.api.allow_list configuration is updated. Replace the hardcoded
$allowed array (currently listing allowed fields like 'id', 'name', etc.) by
retrieving the AllowListResolver from the DI container using
static::getContainer()->get(AllowListResolver::class), then calling the
getAllowedProperties method with Product::class as the argument to dynamically
obtain the actual permitted fields list. This ensures the test always validates
against the current configuration rather than a static list.
🪄 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: 630efed4-a115-460d-b9d3-10329407a7cd
📒 Files selected for processing (17)
.github/workflows/unit-test.ymltests/Eccube/Tests/Service/Mcp/Contract/AllowListContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/Api44LifecycleContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpFirewallContractTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.phptests/Eccube/Tests/Service/Mcp/Contract/McpTokenRevocationContractTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetCustomerOrdersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetCustomerToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetOrderToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetPluginToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetProductStockToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetProductToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/GetShippingToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/ListPluginsToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchCustomersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchOrdersToolTest.phptests/Eccube/Tests/Service/Mcp/Tool/SearchProductsToolTest.php
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/Eccube/Tests/Service/Mcp/Tool/GetCustomerToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/GetShippingToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/ListPluginsToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/GetProductToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/GetOrderToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/SearchCustomersToolTest.php
- tests/Eccube/Tests/Service/Mcp/Contract/Api44LifecycleContractTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/GetProductStockToolTest.php
- tests/Eccube/Tests/Service/Mcp/Contract/McpFirewallContractTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/GetPluginToolTest.php
- tests/Eccube/Tests/Service/Mcp/Tool/SearchOrdersToolTest.php
- tests/Eccube/Tests/Service/Mcp/Contract/McpScopeEnforcementIntegrationTest.php
- tests/Eccube/Tests/Service/Mcp/Contract/AllowListContractTest.php
- tests/Eccube/Tests/Service/Mcp/Contract/McpTokenRevocationContractTest.php
Comment on lines
+186
to
+194
| - name: Checkout | ||
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 | ||
|
|
||
| - name: Checkout Api44 plugin | ||
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 | ||
| with: | ||
| repository: 'EC-CUBE/eccube-api4' | ||
| ref: '4.4' | ||
| path: 'eccube-api4' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check persist-credentials usage across workflows
rg -nP --type=yaml 'uses:\s+actions/checkout' .github/workflows/ -A5 | rg 'persist-credentials'Repository: EC-CUBE/ec-cube
Length of output: 41
🌐 Web query:
actions/checkout persist-credentials default behavior security GitHub token
💡 Result:
The persist-credentials input in actions/checkout defaults to true [1][2][3]. When enabled, this setting configures the local Git environment—specifically the Git configuration—to use the provided GitHub token or SSH key [1][4][2]. This allows subsequent steps in the same GitHub Actions job to execute authenticated Git commands (such as git fetch or git push) without needing to re-authenticate [1][4][2]. From a security perspective, persisting credentials makes them available to any subsequent process or action running within that job [5][6]. Historically, this meant the token was stored directly in.git/config, which made it easily accessible to other steps [5][6]. Recent versions of actions/checkout have improved security by storing these credentials in a separate, temporary file under $RUNNER_TEMP rather than directly in.git/config, though they remain accessible to the workflow process [1][7]. If your security requirements dictate that subsequent steps should not have access to these credentials, you should explicitly set persist-credentials to false in your workflow configuration [1][4][2]: - with: persist-credentials: false While setting this to false prevents the action from configuring Git with the token, it does not affect the inherent accessibility of the GITHUB_TOKEN if it is explicitly passed to other actions or scripts [5][6]. Always ensure your workflow permissions are set to the least privilege necessary [5][6][2].
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 3: https://github.com/actions/checkout/blob/master/action.yml
- 4: https://github.com/actions/checkout
- 5: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 6: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 7: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
🏁 Script executed:
# Verify the specific lines in the workflow file
cat -n .github/workflows/unit-test.yml | sed -n '186,194p'Repository: EC-CUBE/ec-cube
Length of output: 447
checkout アクションに persist-credentials: false の設定を検討してください。
両方の checkout アクション（本体リポジトリと Api44 リポジトリ）で persist-credentials が設定されていません。デフォルトでは true となり、GITHUB_TOKEN がジョブ内の後続ステップからアクセス可能な状態になります。最新バージョンでは認証情報は .git/config ではなく一時ファイル（$RUNNER_TEMP 配下）に保存されていますが、ワークフロープロセスには引き続きアクセス可能です。
最小権限の原則に従い、必要がなければ両方の checkout に persist-credentials: false を追加することを推奨します。
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 186-187: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 189-194: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/unit-test.yml around lines 186 - 194, Both checkout
actions in the workflow are missing the `persist-credentials: false`
configuration. Add `persist-credentials: false` to the "Checkout" action that
checks out the main repository and also add `persist-credentials: false` to the
"Checkout Api44 plugin" action that checks out the eccube-api4 repository. This
follows the principle of least privilege by preventing the GITHUB_TOKEN from
being unnecessarily accessible in subsequent workflow steps.
Source: Linters/SAST tools
mock-package-api へ渡す tarball に .git を含めると composer の dist 展開が 失敗する (Install of ec-cube/api44 failed)。 eccube-api4 自身の CI と同じく プラグインルートで ./* を固める形式に揃える。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MCP の scope 認可と /admin/mcp 用 OAuth2 firewall は Api44 の feat/mcp-server-scorp にあり、 eccube-api4 の 4.4 には未マージ。 4.4 のままだと firewall が prepend されず /admin/mcp が 302 になり契約テストが失敗するため、 当該ブランチを参照する (マージ後に ref を 4.4 へ戻す)。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/unit-test.yml (1)
189-192: ⚡ Quick win一時的なブランチ参照のメンテナンス計画を追跡してください。
feat/mcp-server-scorpブランチへの依存は一時的措置として明記されていますが、以下のリスクがあります:
- ブランチが削除またはリベースされると CI が失敗します。
- 4.4 マージ後の ref 更新を忘れるリスクがあります。
Issue または TODO を作成し、
feat/mcp-server-scorpが 4.4 にマージされた後の ref 更新を追跡することを推奨します。この追跡 Issue の作成をお手伝いしましょうか？
🤖 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 @.github/workflows/unit-test.yml around lines 189 - 192, Create a GitHub Issue or add a TODO comment in the repository to track the temporary branch reference used in the unit-test.yml workflow. Specifically, document that the ref pointing to `feat/mcp-server-scorp` branch (referenced in lines 189-192 of .github/workflows/unit-test.yml) must be changed back to '4.4' once the `feat/mcp-server-scorp` branch is merged into the 4.4 branch. Include in the tracking issue the specific change needed: updating the branch reference from `feat/mcp-server-scorp` to `4.4` in the workflow file, and link this issue to the main tracking so it is not forgotten when the merge occurs.
🤖 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.
Nitpick comments:
In @.github/workflows/unit-test.yml:
- Around line 189-192: Create a GitHub Issue or add a TODO comment in the
repository to track the temporary branch reference used in the unit-test.yml
workflow. Specifically, document that the ref pointing to
`feat/mcp-server-scorp` branch (referenced in lines 189-192 of
.github/workflows/unit-test.yml) must be changed back to '4.4' once the
`feat/mcp-server-scorp` branch is merged into the 4.4 branch. Include in the
tracking issue the specific change needed: updating the branch reference from
`feat/mcp-server-scorp` to `4.4` in the workflow file, and link this issue to
the main tracking so it is not forgotten when the merge occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9555ba0d-9d6c-4198-a7bb-733702bbe41e
📒 Files selected for processing (1)
.github/workflows/unit-test.yml
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6832 +/- ##
==========================================
+ Coverage 74.82% 75.26% +0.44%
==========================================
Files 463 487 +24
Lines 24029 24771 +742
==========================================
+ Hits 17979 18645 +666
- Misses 6050 6126 +76
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:
|
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.
概要(Overview・Refs Issue)
EC-CUBE の管理データ（商品/在庫・注文・顧客会員・プラグイン管理）を、AI クライアント（Claude Desktop / Cursor / VS Code 等）から自然言語で参照できる読み取り専用の MCP サーバを追加します。
/admin/mcpで Streamable HTTP を提供し、4 領域・全 11 ツール（read-only）を公開します。認証認可は独自実装せず、API プラグイン（api44）の OAuth2 に委譲します。MCP サーバ本体は LLM を内包しない薄いアダプタ層で、推論はクライアント側で行います。
Refs #6796
方針(Policy)
unit-test.yml の中で 実行しています。
league/oauth2-server-bundle）の access token をそのまま使う。トークン管理 UI・発行コマンドも api44 の既存機能を流用する。mcp:product:read/mcp:order:read/mcp:customer:read/mcp:plugin:read）。api44 の素のread/writeとはmcp:接頭辞で名前空間を分離し、GraphQL 用トークンで MCP を叩けないようにする。ScopeEnforcingReferenceHandlerでデコレートし、中央レジストリMcpToolScopeMapを引いてIsGrantedで照合。未登録ツールは fail-closed で全 deny。Tool 本体は scope を意識しない。mcp→mcp.log（DB テーブルは追加しない）。MCP 境界で起きた全イベント（tool call／scope 拒否／rate limit／Origin 違反／認証失敗）を client / IP 粒度で記録する。allow_listを流用（GraphQL と共用）。値はマスクせず、PII の露出可否は scope 付与で運用統制する。実装に関する補足(Appendix)
adminfirewall の前に^/admin/mcp用 firewall（stateless: true/oauth2: true）を prepend する。core のsecurity.yamlは変更しない（api44 が^/apiで行う手法と同型）。result.isError = true。mcp-bundle のCallToolHandlerがツール実行中の例外を catch して JSON-RPC に変換するため、kernel.exceptionには届かず HTTP は常に 200 になる（403 化が不可能な理由と代替案比較はdocs/mcp/scope-denied-response.md）。kernel.request）、2 段 Rate Limiter（IP 単位＝認証前 / client_id 単位＝認証後、cache 障害時は 503 fail-closed）。McpAuditLoggerを唯一の書き手とし、compiler pass でmcpチャネルへの注入経路を 1 点に限定。認証失敗（401）はAuthFailureAuditListenerがkernel.responseで拾って記録する。/admin/mcpと api44 の scope 追加が中心。MCP 機能は管理画面から有効/無効を切り替え可能。テスト（Test)
{"found": false}）。McpToolScopeMapに scope を持つ契約テスト（未登録＝fail-closed）。mcp.logへの分離（site.log に漏れない）契約テスト、認証失敗 401 の記録（warning・reason 分岐）、AuditResult全 case が参照される契約テスト。tools/listとallow_listの契約テスト。相談（Discussion）
mcp:product:readがそのままROLE_OAUTH2_MCP:PRODUCT:READに変換され、role 名にコロンが残ります（Symfony 上は動作するが慣習的ではない）。定数で集中管理し後から一括変更可能にしていますが、命名の最終形について意見が欲しいです。マイナーバージョン互換性保持のための制限事項チェックリスト
レビュワー確認項目
Summary by CodeRabbit