feat: playwrite mcp に変更#12
Conversation
There was a problem hiding this comment.
Pull request overview
Worker のスクリーンショット取得を従来の take-screenshot.mjs 実行から Playwright MCP 経由に切り替え、分析結果から推定した device_type(desktop/mobile)をワーカー実行まで伝搬できるようにする変更です。
Changes:
- Worker に
@playwright/mcpを導入し、MCP ツール経由で before/after スクリーンショットを取得するよう変更 - analyzer が
device_typeを推定してproposals.jsonに含め、backend がそれを読み取り implementation worker まで引き回すよう変更 - K8s worker 実行環境に
CLAUDE_CODE_MAX_OUTPUT_TOKENSを追加し、ログ取得のデフォルト tail を増加
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/worker.Dockerfile | @playwright/mcp のインストール追加、take-screenshot.mjs のコピー削除 |
| docker/worker-analyze.py | MCP サーバ/ツール定義追加、device_type 推定・出力形式変更、MCP でスクリーンショット取得 |
| docker/worker-implement.py | MCP サーバ/ツール定義追加、device_type をもとに MCP でスクリーンショット/デバッグ、plan.json から device_type 読み取り |
| docker/take-screenshot.mjs | 旧スクリーンショットスクリプト削除 |
| backend/app/workflow/state.py | workflow state に device_type を追加 |
| backend/app/workflow/session_implementation_graph.py | worker に渡す plan を S3 上で {plan, device_type} に拡張 |
| backend/app/workflow/session_analyzer_graph.py | S3 の proposals.json から device_type を読み取るよう変更 |
| backend/app/usecase/session_usecase.py | analyzer 結果の device_type をログ/implementation 起動に反映 |
| backend/app/infra/s3_client.py | get_proposals アクセサ削除(呼び出し側が直接 download_json へ) |
| backend/app/infra/k8s_client.py | worker env 追加、ログ tail デフォルト変更 |
| backend/README.md | デプロイ手順の追記 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PLAYWRIGHT_MCP_SERVERS = { | ||
| "playwright": { | ||
| "command": "npx", | ||
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium"], | ||
| }, | ||
| "playwright_mobile": { | ||
| "command": "npx", | ||
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium", "--device", "iPhone 15"], |
There was a problem hiding this comment.
The MCP server is started via npx @playwright/mcp@latest, which is non-deterministic and may trigger network fetches at runtime. Consider pinning the MCP package version (and/or invoking the already-installed global binary) so worker executions don’t depend on latest resolution.
| PLAYWRIGHT_MCP_SERVERS = { | |
| "playwright": { | |
| "command": "npx", | |
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium"], | |
| }, | |
| "playwright_mobile": { | |
| "command": "npx", | |
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium", "--device", "iPhone 15"], | |
| PLAYWRIGHT_MCP_PACKAGE_SPEC = os.environ.get("PLAYWRIGHT_MCP_PACKAGE_SPEC", "@playwright/mcp@0.5.0") | |
| PLAYWRIGHT_MCP_SERVERS = { | |
| "playwright": { | |
| "command": "npx", | |
| "args": [PLAYWRIGHT_MCP_PACKAGE_SPEC, "--headless", "--browser", "chromium"], | |
| }, | |
| "playwright_mobile": { | |
| "command": "npx", | |
| "args": [PLAYWRIGHT_MCP_PACKAGE_SPEC, "--headless", "--browser", "chromium", "--device", "iPhone 15"], |
| PLAYWRIGHT_MCP_SERVERS = { | ||
| "playwright": { | ||
| "command": "npx", | ||
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium"], | ||
| }, | ||
| "playwright_mobile": { | ||
| "command": "npx", | ||
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium", "--device", "iPhone 15"], |
There was a problem hiding this comment.
The MCP server configuration uses npx @playwright/mcp@latest, which is non-deterministic and can require network access at runtime. Pin the MCP package version (and keep it compatible with the pinned Playwright version) to avoid unexpected breakage.
| PLAYWRIGHT_MCP_SERVERS = { | |
| "playwright": { | |
| "command": "npx", | |
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium"], | |
| }, | |
| "playwright_mobile": { | |
| "command": "npx", | |
| "args": ["@playwright/mcp@latest", "--headless", "--browser", "chromium", "--device", "iPhone 15"], | |
| PLAYWRIGHT_MCP_PACKAGE = "@playwright/mcp@0.4.4" | |
| PLAYWRIGHT_MCP_SERVERS = { | |
| "playwright": { | |
| "command": "npx", | |
| "args": [PLAYWRIGHT_MCP_PACKAGE, "--headless", "--browser", "chromium"], | |
| }, | |
| "playwright_mobile": { | |
| "command": "npx", | |
| "args": [PLAYWRIGHT_MCP_PACKAGE, "--headless", "--browser", "chromium", "--device", "iPhone 15"], |
| try: | ||
| await implement_changes(repo_dir, proposal_plan) | ||
| except Exception as e: | ||
| emit_log("implementing", f"Implementation interrupted: {e}", detail=str(e)) |
There was a problem hiding this comment.
implement_changes() exceptions are logged but then ignored, and the workflow continues to launch/screenshot/commit. This can produce “successful” artifacts even when implementation failed. Consider re-raising (or exiting non-zero) after logging so failures don’t get silently converted into partial results.
| emit_log("implementing", f"Implementation interrupted: {e}", detail=str(e)) | |
| emit_log("implementing", f"Implementation interrupted: {e}", detail=str(e)) | |
| raise |
| data = s3.download_json(s3.proposals_json_key(session_id, iteration_index)) | ||
| if isinstance(data, dict) and "proposals" in data: | ||
| proposals = data["proposals"] |
There was a problem hiding this comment.
S3Client.download_json() calls json.loads() and will raise on invalid JSON; extract_results() doesn’t catch that, so a malformed/partial proposals.json will crash the graph instead of returning a clean failure. Wrap the download/parse in a try/except (e.g., json.JSONDecodeError) and return a failed status with a helpful error.
|
|
||
| # Install Playwright and Chromium (without --with-deps since deps are installed above) | ||
| RUN npm install -g playwright@1.49.0 && \ | ||
| RUN npm install -g playwright@1.49.0 @playwright/mcp@latest && \ |
There was a problem hiding this comment.
The npm install -g playwright@1.49.0 @playwright/mcp@latest line pulls @playwright/mcp from npm using the mutable latest dist-tag, which makes worker image builds non-reproducible and exposes the build/runtime to supply-chain risk if that package or its dist-tag is compromised. Because this global dependency runs inside the worker container (likely with access to repo data and possibly credentials), a malicious update to @playwright/mcp would be automatically trusted and executed on the next image build without explicit review. Pin @playwright/mcp to a specific version and update it intentionally as needed to reduce this attack surface.
No description provided.