feat: プロンプト修正#15
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the “analyze / implement / create PR” worker pipeline by centralizing prompt text and shared worker utilities, and adjusts the plan JSON shape passed through S3 so downstream steps have richer context.
Changes:
- Added shared modules for worker prompts (
docker/prompts.py) and common worker utilities (docker/worker_common.py), and updated workers to use them. - Updated analysis/implementation flow to include richer plan context (title/concept/files) and improved patch/PR creation handling (e.g., empty patch early-exit).
- Removed React
StrictModewrapper in the frontend entrypoint.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/main.tsx | Removes StrictMode from app root render. |
| docker/worker_common.py | New shared logging/S3/tool-stream helpers for workers. |
| docker/worker.Dockerfile | Copies new shared worker modules into the worker image. |
| docker/worker-implement.py | Switches to shared prompts/common utils; adjusts plan handling and patch creation flow. |
| docker/worker-createpr.py | Uses shared prompts/common utils; adds plan context to PR prompt; skips PR creation on empty patch. |
| docker/worker-analyze.py | Uses shared prompts/common utils; validates proposals; includes instruction in proposals.json. |
| docker/prompts.py | New centralized prompt definitions and prompt-building helpers. |
| backend/app/workflow/session_implementation_graph.py | Changes how plan.json is constructed before uploading to S3. |
| backend/app/usecase/session_usecase.py | Passes richer per-proposal plan JSON into implementation workflow. |
Comments suppressed due to low confidence (1)
frontend/src/main.tsx:25
- React StrictMode was removed entirely. This disables important dev-only checks (including double-invocation to surface side effects) and may hide issues that StrictMode would catch. If the goal is to avoid StrictMode behavior only for the worker/screenshot flow, consider keeping StrictMode enabled for normal dev builds and gating it behind an env flag instead of removing it globally.
createRoot(document.getElementById('root')!).render(<RouterProvider router={router} />)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raw_plan = state["proposal_plan"] | ||
| plan_obj = _json.loads(raw_plan) if isinstance(raw_plan, str) else raw_plan | ||
| if not isinstance(plan_obj, dict): | ||
| plan_obj = {"plan": plan_obj} | ||
| plan_obj["device_type"] = state.get("device_type", "desktop") | ||
| plan_obj["instruction"] = state.get("instruction", "") | ||
| plan_with_device = _json.dumps(plan_obj, ensure_ascii=False) |
There was a problem hiding this comment.
create_k8s_job now unconditionally json.loads() a string proposal_plan. If proposal_plan is ever a non-JSON string (e.g., legacy plain-text plans / already-formatted prompts), this will raise JSONDecodeError and fail the whole job creation path. To keep backward compatibility and avoid hard failures, wrap the loads in try/except JSONDecodeError and fall back to treating raw_plan as the plan value (e.g., {"plan": raw_plan}).
|
|
||
|
|
There was a problem hiding this comment.
After centralizing message processing, tool-use logging now relies on _emit_tool_detail, but it only handles Read/Write/Edit. Bash tool invocations (and potentially other tools) will no longer produce the helpful "Running: ..." logs that existed before, reducing observability when debugging worker failures. Consider extending _emit_tool_detail to handle Bash (and any other relevant tools) similarly to the previous per-worker implementations.
| elif tool_name in ("Bash", "bash"): | |
| # Log Bash tool invocations with a concise "Running: ..." message. | |
| cmd = ( | |
| params.get("command") | |
| or params.get("cmd") | |
| or params.get("script") | |
| ) | |
| if cmd is None: | |
| return | |
| if not isinstance(cmd, str): | |
| try: | |
| cmd_str = json.dumps(cmd, ensure_ascii=False) | |
| except TypeError: | |
| cmd_str = str(cmd) | |
| else: | |
| cmd_str = cmd | |
| # Use only the first line for the summary to keep logs compact. | |
| summary = cmd_str.strip().splitlines()[0][:200] | |
| emit_log(phase, f"Running: {summary}", detail=cmd_str) | |
| elif tool_name in PLAYWRIGHT_MCP_TOOLS: | |
| # Emit a concise log for Playwright MCP tools to aid debugging. | |
| emit_log(phase, f"Running Playwright tool: {tool_name}", detail=json.dumps(params, ensure_ascii=False)) |
| # Pass the full plan object (title, concept, plan, files) to implement_changes | ||
| proposal_plan = { | ||
| k: v | ||
| for k, v in plan_data.items() | ||
| if k not in ("device_type", "instruction") |
There was a problem hiding this comment.
The implementation worker drops "instruction" from the plan object before passing it into implement_changes (it filters out "instruction"), but implement_changes formats the plan using plan_data.get("instruction") for the "User Request" header. This will usually render as "N/A", removing critical user context from the implementation prompt. Keep "instruction" in the plan passed to implement_changes, or explicitly inject the separate instruction variable when building formatted_plan.
| # Pass the full plan object (title, concept, plan, files) to implement_changes | |
| proposal_plan = { | |
| k: v | |
| for k, v in plan_data.items() | |
| if k not in ("device_type", "instruction") | |
| # Pass the full plan object (title, concept, plan, files, instruction) to implement_changes | |
| proposal_plan = { | |
| k: v | |
| for k, v in plan_data.items() | |
| if k != "device_type" |
No description provided.