feat: robust plan quality gate, iterative refinement, and amend UX (#41)

* feat: robust plan quality gate, iterative refinement, and amend UX Plan agent output from weak models often produced garbage (11-char plans that echo the prompt). Two fixes: 1. Quality validation (_validate_plan) checks length, section structure, echo detection, and refusal patterns. Fails trigger one automatic retry with a coaching message injected into the agent's existing conversation, preserving all prior exploration context. 2. Iterative feedback loop — user feedback at plan review re-runs the plan agent via _refine_plan() instead of appending text to the tool result. Up to 5 refinement rounds. The plan file path is always included in the tool result so the outer model knows where it lives. UI improvements: - Web: Reject button dynamically becomes "Amend" (amber) when feedback is typed. Key hint badges (Esc/Enter) on plan buttons. Main input disabled during review. Light-theme contrast fix via --on-color var. - CLI: Prompt shows all three actions (approve/amend/reject). - Bridge: Race condition fix — clear pending entry before HTTP POST so sequential plan reviews from the refinement loop aren't skipped. 15 new tests covering validation, retry, and refinement. * fix: address PR 41 review feedback - Escape key in plan dialog now mirrors the Amend button: if feedback is typed, Esc sends the feedback (amend); if empty, Esc rejects. Previously Esc always hard-coded "reject", discarding typed feedback. - Coaching message for plan retry now says "should include at least two of" instead of "MUST include these", matching the actual validation rule (_MIN_PLAN_SECTIONS = 2). * feat: render plan inline in chat after approval After the plan review dialog closes, the plan content is now rendered as a collapsible inline block in the chat stream — styled with a status header (approved/rejected/amending), markdown-rendered body, and feedback note when amending. Uses the same makeCollapsible pattern as tool output blocks. * fix: prevent plan approval hang when inline render fails The authFetch call that unblocks the server must fire before the cosmetic inline plan rendering. Previously _addInlinePlan ran first and any JS error (e.g. from renderMarkdown) prevented the API call, leaving the session thread blocked forever. - Move authFetch before _addInlinePlan - Wrap _addInlinePlan in try-catch - Guard against empty content - Only auto-collapse plans longer than 12 lines * fix: address PR 41 review feedback (round 2) - Max refinement rounds no longer implicitly approve: the loop now shows the final plan for explicit approve/reject before proceeding. Previously exhausting 5 rounds silently accepted the last revision. - Plan inline block: correct aria-label from "Tool output" to "Plan content" when makeCollapsible is applied. - XSS concern (not applicable): renderMarkdown is used for all assistant messages — plan content follows the same trust model. - Test loop concern (acknowledged): refinement tests verify component logic; full _execute_tools integration would require extensive mocking for marginal coverage gain. * feat: thinking spinner + inline plan hardening * fix lint
2026-08-12 23:12:23 -06:00 · 2026-03-11 20:22:41 -07:00
parent c6ba8d59b0
commit a2e2ffacd8
8 changed files with 684 additions and 39 deletions
@@ -144,12 +144,21 @@ class TestChatSessionConstruction:
 class TestPlanExec:
    """Tests for _exec_plan: unique session-scoped plan file and existing-plan injection."""

-    def _run_plan(self, session, prompt, agent_return="# Plan\n\nDo the thing."):
+    _VALID_PLAN = (
+        "## Goal\n\nDo the thing.\n\n"
+        "## Current State\n\nFile foo.py has bar().\n\n"
+        "## Plan\n\n1. Edit foo.py line 10.\n\n"
+        "## Risks\n\nNone."
+    )
+
+    def _run_plan(self, session, prompt, agent_return=None):
        """Invoke _exec_plan with _run_agent patched to avoid LLM calls.

        Returns (call_id_returned, content_returned, captured_messages) where
        captured_messages is the agent_messages list passed to _run_agent.
        """
+        if agent_return is None:
+            agent_return = self._VALID_PLAN
        captured = {}

        def fake_run_agent(messages, **kwargs):
@@ -175,10 +184,9 @@ class TestPlanExec:
        """Written plan file contains the agent's output verbatim."""
        monkeypatch.chdir(tmp_path)
        session = _make_session()
-        plan_content = "## Goal\n\nAdd a new endpoint."
-        self._run_plan(session, "add endpoint", agent_return=plan_content)
+        self._run_plan(session, "add endpoint")
        plan_file = tmp_path / f".plan-{session._ws_id}.md"
-        assert plan_file.read_text() == plan_content
+        assert plan_file.read_text() == self._VALID_PLAN

    def test_two_sessions_produce_different_files(self, tmp_db, tmp_path, monkeypatch):
        """Two ChatSession instances never collide on the same plan file."""
@@ -262,10 +270,292 @@ class TestPlanExec:
        """_exec_plan returns (call_id, agent_output)."""
        monkeypatch.chdir(tmp_path)
        session = _make_session()
-        agent_output = "## Goal\n\nBuild it."
-        call_id, content, _ = self._run_plan(session, "do stuff", agent_return=agent_output)
+        call_id, content, _ = self._run_plan(session, "do stuff")
        assert call_id == "test-call-1"
-        assert content == agent_output
+        assert content == self._VALID_PLAN
+
+    def test_exec_plan_retries_on_garbage(self, tmp_db, tmp_path, monkeypatch):
+        """When _run_agent returns garbage, _exec_plan retries once."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        good_plan = (
+            "## Goal\n\nAdd feature X.\n\n"
+            "## Current State\n\nFile foo.py has bar().\n\n"
+            "## Plan\n\n1. Edit foo.py:bar()\n\n"
+            "## Risks\n\nNone."
+        )
+        call_count = 0
+
+        def fake_run_agent(messages, **kwargs):
+            nonlocal call_count
+            call_count += 1
+            if call_count == 1:
+                return "Sure, do the thing."
+            return good_plan
+
+        item = {"call_id": "c1", "prompt": "add feature X"}
+        with patch.object(session, "_run_agent", side_effect=fake_run_agent):
+            _, content = session._exec_plan(item)
+
+        assert call_count == 2
+        assert "## Goal" in content
+
+    def test_exec_plan_warning_on_double_failure(self, tmp_db, tmp_path, monkeypatch):
+        """When both attempts produce garbage, content gets a warning prefix."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+
+        def fake_run_agent(messages, **kwargs):
+            return "nope"
+
+        item = {"call_id": "c1", "prompt": "add feature X"}
+        with patch.object(session, "_run_agent", side_effect=fake_run_agent):
+            _, content = session._exec_plan(item)
+
+        assert content.startswith("[Warning:")
+
+    def test_retry_continues_agent_conversation(self, tmp_db, tmp_path, monkeypatch):
+        """Retry appends coaching to the same agent_messages list."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        captured_messages: list[list] = []
+
+        def fake_run_agent(messages, **kwargs):
+            captured_messages.append(list(messages))
+            if len(captured_messages) == 1:
+                return "garbage"
+            return (
+                "## Goal\n\nDone.\n\n## Current State\n\nx\n\n## Plan\n\n1. x\n\n## Risks\n\nNone."
+            )
+
+        item = {"call_id": "c1", "prompt": "add feature X"}
+        with patch.object(session, "_run_agent", side_effect=fake_run_agent):
+            session._exec_plan(item)
+
+        assert len(captured_messages) == 2
+        # Second call should have more messages (coaching appended)
+        assert len(captured_messages[1]) > len(captured_messages[0])
+        # Last user message in second call is the coaching message
+        assert "did not follow" in captured_messages[1][-1]["content"]
+
+
+# ---------------------------------------------------------------------------
+# Plan validation
+# ---------------------------------------------------------------------------
+
+
+class TestPlanValidation:
+    """Tests for ChatSession._validate_plan quality gate."""
+
+    GOOD_PLAN = (
+        "## Goal\n\nAdd authentication to the API.\n\n"
+        "## Current State\n\nFile server.py:45 has no auth middleware.\n\n"
+        "## Plan\n\n1. Add AuthMiddleware to server.py.\n"
+        "2. Create auth.py with JWT verification.\n\n"
+        "## Risks\n\nToken expiry handling may need tuning."
+    )
+
+    def test_valid_plan_passes(self):
+        valid, issues = ChatSession._validate_plan(self.GOOD_PLAN, "add auth")
+        assert valid
+        assert issues == []
+
+    def test_too_short_fails(self):
+        valid, issues = ChatSession._validate_plan("Do the thing.", "do stuff")
+        assert not valid
+        assert any("too short" in i for i in issues)
+
+    def test_no_sections_fails(self):
+        content = "A" * 150  # long enough but no sections
+        valid, issues = ChatSession._validate_plan(content, "build it")
+        assert not valid
+        assert any("missing plan sections" in i for i in issues)
+
+    def test_echo_detection(self):
+        goal = "deliver a simpsons quote from a specific episode"
+        content = "Deliver a Simpsons quote from a specific episode"
+        valid, issues = ChatSession._validate_plan(content, goal)
+        assert not valid
+        assert any("echo" in i for i in issues)
+
+    def test_refusal_detection(self):
+        content = "I cannot create a plan for this task because " + "x" * 100
+        valid, issues = ChatSession._validate_plan(content, "do stuff")
+        assert not valid
+        assert any("refusal" in i for i in issues)
+
+    def test_partial_sections_passes(self):
+        """2 out of 4 sections is enough to pass."""
+        content = (
+            "## Goal\n\nFix the bug in parsing.\n\n"
+            "## Plan\n\n1. Edit parser.py line 42.\n"
+            "2. Add boundary check.\n"
+            "This is enough detail to proceed with confidence."
+        )
+        valid, issues = ChatSession._validate_plan(content, "fix bug")
+        assert valid
+
+    def test_one_section_fails(self):
+        """Only 1 out of 4 sections is not enough."""
+        content = (
+            "## Goal\n\nFix the bug.\n\n"
+            "We should probably edit parser.py and add some checks "
+            "to the boundary handling code path for safety."
+        )
+        valid, issues = ChatSession._validate_plan(content, "fix bug")
+        assert not valid
+        assert any("missing plan sections" in i for i in issues)
+
+
+# ---------------------------------------------------------------------------
+# Plan refinement loop
+# ---------------------------------------------------------------------------
+
+
+class TestPlanRefinement:
+    """Tests for the iterative plan refinement loop in _execute_tools."""
+
+    GOOD_PLAN = TestPlanValidation.GOOD_PLAN
+
+    def test_feedback_triggers_refinement(self, tmp_db, tmp_path, monkeypatch):
+        """User feedback causes _refine_plan to run, then approval exits."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        refine_called = []
+
+        review_responses = iter(["add error handling", ""])
+        session.ui = MagicMock(spec_set=NullUI)
+        session.ui.on_plan_review.side_effect = lambda c: next(review_responses)
+        session.ui.on_info = MagicMock()
+        session.ui.on_state_change = MagicMock()
+
+        revised = self.GOOD_PLAN + "\n\n3. Add error handling."
+
+        def fake_refine(content, goal, feedback):
+            refine_called.append(feedback)
+            return revised
+
+        with patch.object(session, "_refine_plan", side_effect=fake_refine):
+            items = [
+                {
+                    "func_name": "create_plan",
+                    "call_id": "c1",
+                    "prompt": "add auth",
+                }
+            ]
+            results = [("c1", self.GOOD_PLAN)]
+            # Manually invoke the post-plan gate portion of _execute_tools.
+            # We test the loop by calling the gate code directly.
+            session.auto_approve = False
+
+            original_goal = items[0].get("prompt", "")
+            output = results[0][1]
+            refinement_round = 0
+            while refinement_round < session._MAX_PLAN_REFINEMENTS:
+                resp = session.ui.on_plan_review(output)
+                if resp.lower() in ("n", "no", "reject"):
+                    break
+                elif resp:
+                    output = session._refine_plan(output, original_goal, resp)
+                    refinement_round += 1
+                else:
+                    break
+
+        assert len(refine_called) == 1
+        assert refine_called[0] == "add error handling"
+        assert "error handling" in output
+
+    def test_reject_skips_refinement(self, tmp_db, tmp_path, monkeypatch):
+        """Rejection exits immediately without calling _refine_plan."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        session.ui = MagicMock(spec_set=NullUI)
+        session.ui.on_plan_review.return_value = "reject"
+
+        with patch.object(session, "_refine_plan") as mock_refine:
+            output = self.GOOD_PLAN
+            resp = session.ui.on_plan_review(output)
+            if resp.lower() in ("n", "no", "reject"):
+                output += "\n\n---\nUser REJECTED"
+            elif resp:
+                output = session._refine_plan(output, "g", resp)
+
+        mock_refine.assert_not_called()
+        assert "REJECTED" in output
+
+    def test_approve_skips_refinement(self, tmp_db, tmp_path, monkeypatch):
+        """Empty response (enter) approves without refinement."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        session.ui = MagicMock(spec_set=NullUI)
+        session.ui.on_plan_review.return_value = ""
+
+        with patch.object(session, "_refine_plan") as mock_refine:
+            output = self.GOOD_PLAN
+            resp = session.ui.on_plan_review(output)
+            if resp.lower() in ("n", "no", "reject"):
+                output += "\n\n---\nUser REJECTED"
+            elif resp:
+                output = session._refine_plan(output, "g", resp)
+
+        mock_refine.assert_not_called()
+        assert "REJECTED" not in output
+
+    def test_max_refinement_rounds(self, tmp_db, tmp_path, monkeypatch):
+        """Loop stops after _MAX_PLAN_REFINEMENTS rounds with a final review."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        session.ui = MagicMock(spec_set=NullUI)
+        session.ui.on_plan_review.return_value = "more detail please"
+        session.ui.on_info = MagicMock()
+
+        refine_count = 0
+
+        def fake_refine(content, goal, feedback):
+            nonlocal refine_count
+            refine_count += 1
+            return content + f"\n(revision {refine_count})"
+
+        with patch.object(session, "_refine_plan", side_effect=fake_refine):
+            output = self.GOOD_PLAN
+            original_goal = "add auth"
+            refinement_round = 0
+            while True:
+                resp = session.ui.on_plan_review(output)
+                if (
+                    resp.lower() in ("n", "no", "reject")
+                    or not resp
+                    or refinement_round >= session._MAX_PLAN_REFINEMENTS
+                ):
+                    break
+                output = session._refine_plan(output, original_goal, resp)
+                refinement_round += 1
+
+        assert refine_count == session._MAX_PLAN_REFINEMENTS
+        # User gets one extra review call after max rounds (the final prompt)
+        assert session.ui.on_plan_review.call_count == session._MAX_PLAN_REFINEMENTS + 1
+
+    def test_refine_plan_message_structure(self, tmp_db, tmp_path, monkeypatch):
+        """_refine_plan passes system + prior plan + feedback to _run_agent."""
+        monkeypatch.chdir(tmp_path)
+        session = _make_session()
+        captured = {}
+
+        def fake_run_agent(messages, **kwargs):
+            captured["messages"] = list(messages)
+            return self.GOOD_PLAN
+
+        with patch.object(session, "_run_agent", side_effect=fake_run_agent):
+            session._refine_plan(self.GOOD_PLAN, "add auth", "add tests too")
+
+        msgs = captured["messages"]
+        assert msgs[0]["role"] == "system"
+        assert msgs[1]["role"] == "assistant"
+        assert msgs[1]["tool_calls"][0]["function"]["name"] == "create_plan"
+        assert msgs[2]["role"] == "tool"
+        assert msgs[2]["content"] == self.GOOD_PLAN
+        assert msgs[3]["role"] == "user"
+        assert "add tests too" in msgs[3]["content"]


 # ---------------------------------------------------------------------------
@@ -204,7 +204,8 @@ class TerminalUI(SessionUI):
        try:
            prompt_text = (
                f"    \001{BOLD}\002Plan ready.\001{RESET}\002 "
-                f"\001{DIM}\002[enter to approve, or give feedback]\001{RESET}\002 "
+                f"\001{DIM}\002[enter to approve, feedback to amend, "
+                f"ctrl-c to reject]\001{RESET}\002 "
            )
            resp = input(prompt_text).strip()
        except EOFError:
@@ -1551,28 +1551,73 @@ class ChatSession:
            with concurrent.futures.ThreadPoolExecutor(max_workers=4) as pool:
                results = list(pool.map(run_one, items))

-        # Post-plan gate: prompt user on main thread after plan completes
+        # Post-plan gate: iterative review loop.  When the user gives
+        # feedback the plan agent re-runs and the revised plan is shown
+        # again, up to _MAX_PLAN_REFINEMENTS rounds.
        for i, item in enumerate(items):
-            if (
-                item.get("func_name") == "create_plan"
-                and not item.get("error")
-                and not item.get("denied")
-                and not self.auto_approve
-            ):
-                cid, output = results[i]
-                assert isinstance(output, str)  # plan always returns text
-                # Let the UI present the plan for review
-                self._emit_state("attention")
-                resp = self.ui.on_plan_review(output)
-                self._emit_state("running")
-                if resp.lower() in ("n", "no", "reject"):
-                    output += (
-                        "\n\n---\nUser REJECTED this plan. Do not proceed "
-                        "with implementation. Ask the user what they want instead."
-                    )
-                elif resp:
-                    output += f"\n\n---\nUser feedback on this plan: {resp}"
-                results[i] = (cid, output)
+            if item.get("func_name") != "create_plan" or item.get("error") or item.get("denied"):
+                continue
+
+            cid, output = results[i]
+            assert isinstance(output, str)  # plan always returns text
+            plan_path = f".plan-{self._ws_id}.md"
+
+            if not self.auto_approve:
+                original_goal = item.get("prompt", "")
+
+                refinement_round = 0
+                while True:
+                    self._emit_state("attention")
+                    resp = self.ui.on_plan_review(output)
+                    self._emit_state("running")
+
+                    if resp.lower() in ("n", "no", "reject"):
+                        output += (
+                            "\n\n---\nUser REJECTED this plan. Do not "
+                            "proceed with implementation. Ask the user "
+                            "what they want instead."
+                        )
+                        break
+                    elif not resp:
+                        break  # empty response = approve
+                    elif refinement_round >= self._MAX_PLAN_REFINEMENTS:
+                        self.ui.on_info("[plan] max refinement rounds reached")
+                        break
+                    else:
+                        # Re-run plan agent with user feedback.
+                        # Strip any internal warning prefix so the
+                        # agent sees the raw plan content.
+                        raw = output
+                        _warn = "[Warning: plan may be incomplete or poorly structured]\n\n"
+                        if raw.startswith(_warn):
+                            raw = raw[len(_warn) :]
+                        try:
+                            output = self._refine_plan(
+                                raw,
+                                original_goal,
+                                resp,
+                            )
+                            refinement_round += 1
+                        except (KeyboardInterrupt, GenerationCancelled):
+                            output += "\n\n---\n(plan refinement interrupted)"
+                            break
+                        except Exception as e:
+                            self.ui.on_info(f"[plan refinement error] {e}")
+                            output += f"\n\n---\nUser feedback: {resp}"
+                            break
+                        # Loop continues → show revised plan to user
+
+                # Write final version to disk (overwrites initial write)
+                try:
+                    with open(plan_path, "w") as f:
+                        f.write(output)
+                except OSError:
+                    pass
+
+            # Always include file path in the tool result so the
+            # outer model knows where the plan lives on disk.
+            output += f"\n\n---\nPlan saved to `{plan_path}`"
+            results[i] = (cid, output)

        return results, user_feedback

@@ -2811,6 +2856,61 @@ class ChatSession:
        "and functions in every step."
    )

+    _MIN_PLAN_LENGTH = 100
+    _PLAN_REQUIRED_SECTIONS = ("## goal", "## current state", "## plan", "## risks")
+    _MIN_PLAN_SECTIONS = 2
+    _MAX_PLAN_REFINEMENTS = 5
+
+    @staticmethod
+    def _validate_plan(content: str, goal: str) -> tuple[bool, list[str]]:
+        """Check if plan output meets minimum quality bar.
+
+        Returns ``(valid, issues)`` where *issues* is a list of
+        human-readable problem descriptions (empty when valid).
+        """
+        issues: list[str] = []
+        stripped = content.strip()
+        stripped_lower = stripped.lower()
+
+        # 1. Minimum length
+        if len(stripped) < ChatSession._MIN_PLAN_LENGTH:
+            issues.append(
+                f"too short ({len(stripped)} chars, minimum {ChatSession._MIN_PLAN_LENGTH})"
+            )
+
+        # 2. Section structure
+        found_sections = sum(
+            1 for section in ChatSession._PLAN_REQUIRED_SECTIONS if section in stripped_lower
+        )
+        if found_sections < ChatSession._MIN_PLAN_SECTIONS:
+            issues.append(
+                f"missing plan sections (found {found_sections}/"
+                f"{len(ChatSession._PLAN_REQUIRED_SECTIONS)}, "
+                f"need at least {ChatSession._MIN_PLAN_SECTIONS})"
+            )
+
+        # 3. Echo detection: plan is basically just the goal repeated
+        goal_stripped = goal.strip().lower()
+        if (
+            goal_stripped
+            and len(stripped) < len(goal_stripped) * 2
+            and goal_stripped in stripped_lower
+        ):
+            issues.append("plan appears to echo the goal without elaboration")
+
+        # 4. Refusal detection
+        refusal_starts = (
+            "i cannot",
+            "i'm sorry",
+            "i am sorry",
+            "error:",
+            "i can't",
+        )
+        if any(stripped_lower.startswith(r) for r in refusal_starts):
+            issues.append("plan appears to be a refusal or error")
+
+        return (len(issues) == 0, issues)
+
    def _exec_plan(self, item: dict[str, Any]) -> tuple[str, str]:
        """Run a planning agent and write the result to .plan-<ws_id>.md."""
        call_id, prompt = item["call_id"], item["prompt"]
@@ -2853,6 +2953,41 @@ class ChatSession:
            self.ui.on_info(f"[plan error] {e}")
            return call_id, f"Plan error: {e}"

+        # Validate plan quality — retry once with coaching on failure
+        valid, issues = self._validate_plan(content, prompt)
+        if not valid:
+            self.ui.on_info(f"[plan] quality issues: {', '.join(issues)}")
+            preview = content[:200] + ("..." if len(content) > 200 else "")
+            coaching = (
+                "Your previous response did not follow the required plan "
+                "format. A valid plan should include at least two of "
+                "these markdown sections:\n"
+                "## Goal (1-2 sentences)\n"
+                "## Current State (files/line numbers found)\n"
+                "## Plan (numbered steps with file names and functions)\n"
+                "## Risks (edge cases and unknowns)\n\n"
+                f'Your previous response was: "{preview}"\n\n'
+                "Please try again. Explore the codebase first, then write "
+                "the plan."
+            )
+            agent_messages.append({"role": "user", "content": coaching})
+            try:
+                content = self._run_agent(
+                    agent_messages,
+                    label="plan",
+                    reasoning_effort="high",
+                )
+            except (KeyboardInterrupt, GenerationCancelled):
+                return call_id, "(plan interrupted by user)"
+            except Exception as e:
+                self.ui.on_info(f"[plan retry error] {e}")
+                return call_id, f"Plan error: {e}"
+
+            valid2, issues2 = self._validate_plan(content, prompt)
+            if not valid2:
+                self.ui.on_info(f"[plan] still has issues after retry: {', '.join(issues2)}")
+                content = "[Warning: plan may be incomplete or poorly structured]\n\n" + content
+
        # Write to file separately — always return content even if write fails
        try:
            with open(plan_path, "w") as f:
@@ -2863,6 +2998,60 @@ class ChatSession:

        return call_id, content

+    def _refine_plan(
+        self,
+        original_content: str,
+        original_goal: str,
+        feedback: str,
+    ) -> str:
+        """Re-run the plan agent incorporating user feedback."""
+        tc_id = f"plan_refine_{uuid.uuid4().hex[:8]}"
+        agent_messages: list[dict[str, Any]] = [
+            {"role": "system", "content": self._PLAN_IDENTITY},
+            {
+                "role": "assistant",
+                "content": None,
+                "tool_calls": [
+                    {
+                        "id": tc_id,
+                        "type": "function",
+                        "function": {
+                            "name": "create_plan",
+                            "arguments": json.dumps({"goal": original_goal}),
+                        },
+                    }
+                ],
+            },
+            {
+                "role": "tool",
+                "tool_call_id": tc_id,
+                "content": original_content,
+            },
+            {
+                "role": "user",
+                "content": (
+                    "The user reviewed this plan and provided feedback:\n\n"
+                    f"{feedback}\n\n"
+                    "Please revise the plan accordingly. Keep the same "
+                    "format (## Goal, ## Current State, ## Plan, ## Risks) "
+                    "and address the feedback."
+                ),
+            },
+        ]
+
+        self.ui.on_info("[plan] revising based on feedback...")
+        content = self._run_agent(
+            agent_messages,
+            label="plan",
+            reasoning_effort="high",
+        )
+
+        valid, issues = self._validate_plan(content, original_goal)
+        if not valid:
+            self.ui.on_info(f"[plan] revised plan has issues: {', '.join(issues)}")
+
+        return content
+
    def _exec_remember(self, item: dict[str, Any]) -> tuple[str, str]:
        """Save a persistent memory."""
        call_id, key, value = item["call_id"], item["key"], item["value"]
@@ -709,6 +709,11 @@ class Bridge:
        def _wait_plan() -> None:
            try:
                raw_resp = self._broker.pop_response(request_id, timeout=self._approval_timeout)
+                # Clear pending entry *before* posting response so that
+                # a subsequent plan review event (from the refinement
+                # loop) is not skipped by the duplicate guard.
+                with self._lock:
+                    self._pending_plan_reviews.pop(ws_id, None)
                if raw_resp:
                    resp_msg = InboundMessage.from_json(raw_resp)
                    feedback = getattr(resp_msg, "feedback", "")
@@ -716,9 +721,16 @@ class Bridge:
                else:
                    log.warning("Plan review timeout for ws %s — rejecting", ws_id)
                    self._http.post("/v1/api/plan", json={"feedback": "reject", "ws_id": ws_id})
-            finally:
+            except Exception:
                with self._lock:
                    self._pending_plan_reviews.pop(ws_id, None)
+                # Best-effort rejection so the server doesn't hang
+                with contextlib.suppress(Exception):
+                    self._http.post(
+                        "/v1/api/plan",
+                        json={"feedback": "reject", "ws_id": ws_id},
+                    )
+                raise

        threading.Thread(target=self._run_in_context(_wait_plan), daemon=True).start()

@@ -28,6 +28,7 @@
  --yellow: #fbbf24;
  --cyan: #67e8f9;
  --magenta: #c084fc;
+  --on-color: var(--bg);

  /* Glow variants for LED effects */
  --green-glow: rgba(52, 211, 153, 0.25);
@@ -66,6 +67,7 @@
  --yellow: #b45309;
  --cyan: #0e7490;
  --magenta: #7c3aed;
+  --on-color: #ffffff;
  --green-glow: rgba(4, 120, 87, 0.25);
  --red-glow: rgba(220, 38, 38, 0.25);
  --yellow-glow: rgba(180, 83, 9, 0.25);
@@ -1570,20 +1570,45 @@ function scrollToBottom(force) {
 }

 // --- Plan review dialog ---
+var _planContent = "";
 function showPlanDialog(content) {
+  _planContent = content;
  document.getElementById("plan-content").textContent = content;
-  document.getElementById("plan-feedback").value = "";
+  var feedbackEl = document.getElementById("plan-feedback");
+  feedbackEl.value = "";
+  _updatePlanRejectBtn();
+  inputEl.disabled = true;
+  sendBtn.disabled = true;
  document.getElementById("plan-overlay").classList.add("active");
  setTimeout(function () {
-    document.getElementById("plan-feedback").focus();
+    feedbackEl.focus();
  }, 50);
 }

+function _updatePlanRejectBtn() {
+  var btn = document.getElementById("btn-plan-reject");
+  var hasFeedback =
+    document.getElementById("plan-feedback").value.trim().length > 0;
+  btn.innerHTML = hasFeedback
+    ? '<span class="key">Esc</span> Amend'
+    : '<span class="key">Esc</span> Reject';
+  btn.style.background = hasFeedback ? "var(--accent)" : "";
+  btn.style.color = hasFeedback ? "var(--on-color)" : "";
+  btn.onclick = function () {
+    resolvePlan(hasFeedback ? "" : "reject");
+  };
+}
+
 function resolvePlan(defaultFeedback) {
  let feedback = document.getElementById("plan-feedback").value.trim();
  if (!feedback && defaultFeedback) feedback = defaultFeedback;
  document.getElementById("plan-overlay").classList.remove("active");
+  inputEl.disabled = false;
+  sendBtn.disabled = false;
  inputEl.focus();
+
+  // Critical: fire the API call first — this unblocks the server.
+  // The inline rendering below is cosmetic and must never prevent it.
  authFetch("/v1/api/plan", {
    method: "POST",
    headers: { "Content-Type": "application/json" },
@@ -1591,6 +1616,65 @@ function resolvePlan(defaultFeedback) {
  }).catch(function (err) {
    addErrorMessage("Connection error: " + err.message);
  });
+
+  // Render plan inline in the chat (best-effort)
+  try {
+    var isReject = feedback === "reject";
+    var isAmend = feedback && !isReject;
+    var action = isReject ? "rejected" : isAmend ? "amending" : "approved";
+    _addInlinePlan(_planContent, action, feedback);
+  } catch (err) {
+    console.error("Failed to render inline plan:", err);
+    addInfoMessage("Plan " + action);
+  }
+
+  // Show spinner while the model processes the plan result
+  setBusy(true);
+  addThinkingIndicator();
+}
+
+function _addInlinePlan(content, action, feedback) {
+  if (!content) return;
+  var wrapper = document.createElement("div");
+  wrapper.className = "plan-inline";
+
+  var header = document.createElement("div");
+  header.className = "plan-inline-header";
+  var label =
+    action === "rejected"
+      ? "Plan rejected"
+      : action === "amending"
+        ? "Plan — amending"
+        : "Plan approved";
+  header.innerHTML =
+    '<span class="plan-inline-label plan-' + action + '">' + label + "</span>";
+  wrapper.appendChild(header);
+
+  var body = document.createElement("div");
+  body.className = "plan-inline-body";
+  try {
+    body.innerHTML = renderMarkdown(content);
+  } catch (e) {
+    body.textContent = content;
+  }
+  if (content.split("\n").length > 12) {
+    makeCollapsible(body);
+    body.setAttribute(
+      "aria-label",
+      "Plan content (collapsed). Activate to expand.",
+    );
+  }
+  wrapper.appendChild(body);
+
+  if (feedback && action === "amending") {
+    var fb = document.createElement("div");
+    fb.className = "plan-inline-feedback";
+    fb.textContent = "Feedback: " + feedback;
+    wrapper.appendChild(fb);
+  }
+
+  messagesEl.appendChild(wrapper);
+  scrollToBottom();
 }

 // --- Send message ---
@@ -1647,6 +1731,9 @@ function autoResize() {
 }

 inputEl.addEventListener("input", autoResize);
+document
+  .getElementById("plan-feedback")
+  .addEventListener("input", _updatePlanRejectBtn);
 inputEl.addEventListener("keydown", function (e) {
  if (e.key === "Enter" && !e.shiftKey) {
    e.preventDefault();
@@ -1749,7 +1836,9 @@ document.addEventListener("keydown", function (e) {
      resolvePlan("");
    } else if (e.key === "Escape") {
      e.preventDefault();
-      resolvePlan("reject");
+      var hasFb =
+        document.getElementById("plan-feedback").value.trim().length > 0;
+      resolvePlan(hasFb ? "" : "reject");
    } else if (e.key === "Tab") {
      var focusable = document.querySelectorAll(
        "#plan-dialog input, #plan-dialog button",
@@ -76,10 +76,10 @@
  <div id="plan-dialog" role="dialog" aria-modal="true" aria-labelledby="plan-dialog-title">
    <h3 id="plan-dialog-title">Plan Review</h3>
    <div id="plan-content"></div>
-    <input type="text" id="plan-feedback" placeholder="Feedback (empty = approve)...">
+    <input type="text" id="plan-feedback" placeholder="feedback (optional)" aria-label="Plan feedback">
    <div id="plan-buttons">
-      <button id="btn-plan-reject" onclick="resolvePlan('reject')">Reject</button>
-      <button id="btn-plan-approve" onclick="resolvePlan('')">Approve</button>
+      <button id="btn-plan-reject"><span class="key">Esc</span> Reject</button>
+      <button id="btn-plan-approve" onclick="resolvePlan('')"><span class="key">&crarr;</span> Approve</button>
    </div>
  </div>
 </div>
@@ -497,11 +497,73 @@
  cursor: pointer;
  font-weight: 600;
  letter-spacing: 0.02em;
-  transition: filter 0.15s;
+  display: inline-flex;
+  align-items: center;
+  gap: 5px;
+  transition: filter 0.15s, background 0.2s;
 }
 #plan-buttons button:hover { filter: brightness(1.1); }
-#btn-plan-approve { background: var(--green); color: var(--bg); }
-#btn-plan-reject { background: var(--red); color: var(--bg); }
+#plan-buttons button:focus-visible { outline: 2px solid var(--fg-bright); outline-offset: 2px; }
+#plan-buttons button .key {
+  display: inline-block;
+  background: rgba(0,0,0,0.2);
+  border-radius: var(--radius-sm);
+  padding: 0 4px;
+  font-size: 10px;
+  font-weight: 600;
+}
+#btn-plan-approve { background: var(--green); color: var(--on-color); }
+#btn-plan-reject { background: var(--red); color: var(--on-color); }
+
+/* Inline plan block (rendered in chat after plan review) */
+.plan-inline {
+  margin: 8px 0;
+  border: 1px solid var(--border);
+  border-radius: var(--radius);
+  overflow: hidden;
+}
+.plan-inline-header {
+  padding: 6px 12px;
+  background: var(--bg-surface);
+  border-bottom: 1px solid var(--border);
+  font-size: 12px;
+  font-family: var(--font-display);
+  font-weight: 600;
+  letter-spacing: 0.02em;
+}
+.plan-inline-label.plan-approved { color: var(--green); }
+.plan-inline-label.plan-rejected { color: var(--red); }
+.plan-inline-label.plan-amending { color: var(--accent); }
+.plan-inline-body {
+  padding: 10px 14px;
+  background: var(--code-bg);
+  font-size: 12px;
+  line-height: 1.6;
+  max-height: 300px;
+  overflow-y: auto;
+}
+.plan-inline-body.collapsed { max-height: 150px; position: relative; }
+.plan-inline-body.collapsed::after {
+  content: 'click to expand';
+  position: absolute;
+  bottom: 0; left: 0; right: 0;
+  text-align: center;
+  padding: 8px 0 4px;
+  background: linear-gradient(transparent, var(--code-bg));
+  color: var(--fg-dim);
+  font-size: 11px;
+  cursor: pointer;
+}
+.plan-inline-body h2, .plan-inline-body h3 { font-size: 13px; margin: 10px 0 4px; color: var(--accent); }
+.plan-inline-body p { margin: 4px 0; }
+.plan-inline-body ol, .plan-inline-body ul { margin: 4px 0; padding-left: 20px; }
+.plan-inline-feedback {
+  padding: 6px 12px;
+  border-top: 1px solid var(--border);
+  font-size: 12px;
+  color: var(--accent);
+  font-style: italic;
+}

 /* ==========================================================================
   Focus indicators — server-specific overrides