fix(agent): allow remaining actions for an approved task (#6113)

* fix(agent): allow remaining actions for an approved task * fix(agent): make approval continuation control-only * fix(ci): preserve approval taint and cache-buster contract * fix(ui): keep tool approvals in current chat * fix(ui): route tool approvals through chat submit * test(ui): pin approval submit routing * fix(agent): complete approval denial flow * fix(ui): avoid duplicate ask-user close icon * fix(agent): retain approved tool in continuation set * revert(ui): keep PR 6113 scoped to approval continuation * fix(agent): add task and chat approval scopes * fix(ui): prevent duplicate ask-user close icon * feat(ui): add ask-user option shortcuts * fix(compare): route ask-user choices per pane * fix(agent): keep skill-test approvals to a single action The chat card now reuses the wire value `approve` to mean chat-session scope, and `consume()` returned `allow_remaining_actions=True` for it unconditionally. The skill-test approval route was never updated: it still sends `approve` meaning "once", and its button still reads "Allow once", but the grant it got back set `approval_gate_bypassed` for the rest of the resumed run. That surface wraps the skill body and every transcript byte as untrusted context, so it is the last place where one click should ungate everything that follows. Give `consume()` an explicit `allow_continuation` flag. Callers that own a resumable chat keep the scope the user picked; callers that do not — the skill tester, unattended audits — get SINGLE_ACTION and the gate re-arms behind the sealed action, which is what their label promises. * fix(ui): cache-bust every module the approval click depends on chatStream.js, compare/index.js and compare/stream.js all changed behaviour but kept their old `?v=`, while chat.js and chatRenderer.js were bumped. A returning browser therefore serves the new chat.js — which now deliberately leaves the composer empty and clicks the send button — next to the cached chatStream.js that has no interceptor. With an empty composer that button sits at `data-mode="newchat"`, so the click opens a new chat and the approval is dropped. Bump the three, and version compare/stream.js's chatRenderer import to match everyone else's so the ask_user keydown listener binds to one module instance instead of two. * fix(ui): keep the digit shortcuts off tool approval cards With an approval card on screen and focus anywhere outside an input, a bare `1` fired `approve_task` — the widest of the three grants — with no modifier and no confirmation. That card is the one control whose entire purpose is deliberate consent after untrusted context influenced the run, and Deny sits at 3. Label the card with its kind and skip the shortcut for approvals. Ordinary ask_user questions keep 1-3. * fix(compare): restore a pane's ask_user card instead of dropping the choice renderAskUserCard removes the card as soon as onSubmit accepts, but the resume loop gave up silently after 10s if the originating stream still owned the pane. The user saw the click land, the card vanish, and nothing happen, with no way to get it back. Re-render the card on that deadline and say why. The reroll case still returns without sending — that choice belongs to a stream that no longer exists. * refactor(chat): drop the unreachable deny branch `if decision != "deny"` is always true — the deny path returns a StreamingResponse a few lines above. It reads as if deny still falls through to the toggle restore. --------- Co-authored-by: Léo <leograndcontact@gmail.com>
2026-10-22 23:02:23 +02:00 · 2026-08-19 08:01:34 -06:00
parent 5c835014ac
commit 981652358e
20 changed files with 1361 additions and 119 deletions
@@ -624,6 +624,8 @@ async def build_chat_context(
    agent_mode: bool = False,
    allow_tool_preprocessing: bool = True,
    defer_context_shaping: bool = False,
+    continuation_context_message: str | None = None,
+    persist_user_message: bool = True,
 ) -> ChatContext:
    """Build the full context (preface + messages) for an LLM call.

@@ -647,14 +649,14 @@ async def build_chat_context(
    # Add user message to history. Nobody/incognito uses a request-local
    # transcript store instead of session history so stale saved chats cannot
    # bleed into context and the turn is not persisted.
-    if incognito:
+    if persist_user_message and incognito:
        user_meta = {"attachments": preprocessed.attachment_meta} if preprocessed.attachment_meta else None
        _append_incognito_message(session_id, "user", preprocessed.user_content, user_meta)
-    else:
+    elif persist_user_message:
        add_user_message(sess, chat_handler, preprocessed, incognito=False)

    # Fire events
-    if not incognito:
+    if persist_user_message and not incognito:
        fire_message_event(request, webhook_manager, session_id, sess, message, compare_mode)

    # Resolve owner-scoped prefs/context. Browser requests keep the cookie user;
@@ -666,7 +668,12 @@ async def build_chat_context(
        getattr(chat_handler, "upload_handler", None),
        getattr(sess, "owner", None),
    )
-    casual_low_signal = _is_casual_low_signal(message)
+    context_message = (
+        str(continuation_context_message).strip()
+        if continuation_context_message
+        else message
+    )
+    casual_low_signal = _is_casual_low_signal(context_message)

    # Memory enabled?
    mem_enabled = not incognito and not no_memory and uprefs.get("memory_enabled", True)
@@ -703,7 +710,15 @@ async def build_chat_context(
    # Build context preface
    # The stream path uses enhanced_message (with CoT/preprocessing applied),
    # the sync path uses text_for_context.
-    _ctx_msg = preprocessed.enhanced_message if use_enhanced_message else preprocessed.text_for_context
+    _ctx_msg = (
+        context_message
+        if continuation_context_message
+        else (
+            preprocessed.enhanced_message
+            if use_enhanced_message
+            else preprocessed.text_for_context
+        )
+    )
    _preface_kwargs = dict(
        message=_ctx_msg,
        session=sess,
@@ -89,6 +89,65 @@ def _stream_failure_status(chunk: str) -> Optional[int]:
    return None


+def _mark_tool_approval_resolved(sess, approval_id: Any, decision: Any) -> bool:
+    """Persist a consumed approval decision on its existing tool event."""
+
+    approval_key = str(approval_id or "")
+    normalized_decision = str(decision or "").strip().lower()
+    if not approval_key or normalized_decision not in {"approve", "approve_task", "deny"}:
+        return False
+
+    message_id = None
+    resolved_metadata = None
+    for item in reversed(getattr(sess, "history", []) or []):
+        metadata = getattr(item, "metadata", None)
+        if not isinstance(metadata, dict):
+            continue
+        tool_events = metadata.get("tool_events")
+        if not isinstance(tool_events, list):
+            continue
+        for event in reversed(tool_events):
+            ask_user = event.get("ask_user") if isinstance(event, dict) else None
+            if not isinstance(ask_user, dict):
+                continue
+            if str(ask_user.get("approval_id") or "") != approval_key:
+                continue
+            ask_user["resolved"] = normalized_decision
+            message_id = metadata.get("_db_id")
+            resolved_metadata = {
+                key: value for key, value in metadata.items() if key != "_db_id"
+            }
+            break
+        if resolved_metadata is not None:
+            break
+
+    if resolved_metadata is None or not message_id:
+        return False
+
+    db = SessionLocal()
+    try:
+        db_message = db.query(DBChatMessage).filter(
+            DBChatMessage.id == message_id,
+            DBChatMessage.session_id == str(getattr(sess, "id", "")),
+        ).first()
+        if db_message is None:
+            return False
+        db_message.meta_data = json.dumps(resolved_metadata)
+        db.commit()
+        return True
+    except Exception:
+        db.rollback()
+        logger.exception("Failed to persist tool approval resolution")
+        return False
+    finally:
+        db.close()
+
+
+async def _tool_approval_resolution_stream(decision: str) -> AsyncGenerator[str, None]:
+    yield f"data: {json.dumps({'type': 'tool_approval_resolved', 'decision': decision})}\n\n"
+    yield "data: [DONE]\n\n"
+
+
 def _chat_candidate_request_factory(
    messages,
    fallback_context_length: int = 0,
@@ -917,6 +976,7 @@ def setup_chat_routes(
        exact_tool_approval = None
        pending_tool_approval = None
        retired_tool_approval_taint = False
+        external_untrusted_context_seen = False
        tool_approval_continuation = False
        # Workspace: confine the agent's file/shell tools to this folder.
        workspace, workspace_rejected = _resolve_request_workspace(
@@ -1050,14 +1110,14 @@ def setup_chat_routes(
            )

        try:
-            # Attachment-only sends: skip the message-required check when the
-            # user has attached one or more files (the attachment IS the action).
+            # Attachment-only sends and approval controls may omit message text.
            _has_atts = (
                bool(body and isinstance(body.get("attachments"), list) and body["attachments"])
                or bool(form_data.get("attachments"))
            )
            message, session = coerce_message_and_session(
-                body, message, session, session_manager, allow_empty=_has_atts,
+                body, message, session, session_manager,
+                allow_empty=(_has_atts or bool(tool_approval_id)),
            )
            # Verify ownership AFTER coerce (which may resolve a default session)
            # but BEFORE loading. Prevents cross-user session hijack.
@@ -1076,8 +1136,14 @@ def setup_chat_routes(
                        409,
                        "This tool approval is invalid, expired, or belongs to another thread.",
                    )
+                pending_taint = bool(
+                    pending_tool_approval.external_untrusted_context_seen
+                )
+                external_untrusted_context_seen = (
+                    external_untrusted_context_seen or pending_taint
+                )
                decision = str(tool_approval_decision or "").strip().lower()
-                if decision not in {"approve", "deny"}:
+                if decision not in {"approve", "approve_task", "deny"}:
                    raise HTTPException(400, "Invalid tool approval decision.")
                if plan_mode:
                    raise HTTPException(
@@ -1091,37 +1157,46 @@ def setup_chat_routes(
                    session_id=session,
                )
                tool_approval_continuation = True
-                if decision == "approve" and exact_tool_approval is None:
+                if (
+                    decision in {"approve", "approve_task"}
+                    and exact_tool_approval is None
+                ):
                    raise HTTPException(
                        409,
                        "This tool approval could not be consumed.",
                    )
-                if decision == "approve":
-                    message = (
-                        f"Approved the exact {pending_tool_approval.tool_name} action "
-                        "shown above once."
+                if not _mark_tool_approval_resolved(
+                    sess,
+                    tool_approval_id,
+                    decision,
+                ):
+                    logger.warning(
+                        "Tool approval %s was consumed but its persisted card could not be marked resolved",
+                        tool_approval_id,
                    )
-                    # The sealed server record, not mutable composer state,
-                    # restores the original action workspace.
-                    workspace = pending_tool_approval.workspace or None
-                    workspace_rejected = None
-                    if pending_tool_approval.document_id:
-                        active_doc_id = pending_tool_approval.document_id
-                    # The approval click is the per-turn opt-in for this exact
-                    # sealed action. Restore only the coarse request toggle
-                    # that would otherwise disable it because the synthetic
-                    # "Approved…" message no longer resembles the original
-                    # shell/web request. Current privilege, global-disable,
-                    # incognito, compare, and tool-policy gates still run.
-                    if pending_tool_approval.tool_name == "bash":
-                        allow_bash = "true"
-                    if pending_tool_approval.tool_name in WEB_TOOL_NAMES:
-                        allow_web_search = "true"
-                        _search_enabled = True
-                else:
-                    message = (
-                        f"Denied the {pending_tool_approval.tool_name} action shown above."
+                if decision == "deny":
+                    return StreamingResponse(
+                        _tool_approval_resolution_stream(decision),
+                        media_type="text/event-stream",
                    )
+                # Approval is a control-plane continuation, not a new user turn.
+                # Reuse the sealed interrupted request only for internal context,
+                # retrieval, and policy reconstruction; never persist or display it.
+                message = pending_tool_approval.continuation_query
+                # The sealed server record, not mutable composer state,
+                # restores the original action workspace.
+                workspace = pending_tool_approval.workspace or None
+                workspace_rejected = None
+                if pending_tool_approval.document_id:
+                    active_doc_id = pending_tool_approval.document_id
+                # Restore only the coarse request toggle needed by the exact
+                # sealed action. Current privilege, global-disable, incognito,
+                # compare, and tool-policy gates still run.
+                if pending_tool_approval.tool_name == "bash":
+                    allow_bash = "true"
+                if pending_tool_approval.tool_name in WEB_TOOL_NAMES:
+                    allow_web_search = "true"
+                    _search_enabled = True
                chat_mode = "agent"
            else:
                # A normal user message supersedes the card that was waiting
@@ -1132,6 +1207,9 @@ def setup_chat_routes(
                    owner=owner,
                    session_id=session,
                )
+                external_untrusted_context_seen = (
+                    external_untrusted_context_seen or retired_tool_approval_taint
+                )
            _reconcile_selected_route_from_request(request, sess, session, form_data, owner=owner)
            if _clear_orphaned_session_endpoint(sess, owner=owner):
                raise HTTPException(400, "Selected model endpoint was removed. Pick another model in Settings.")
@@ -1261,6 +1339,14 @@ def setup_chat_routes(
            agent_mode=(chat_mode == "agent"),
            allow_tool_preprocessing=allow_tool_preprocessing,
            defer_context_shaping=foreground_policy.enabled,
+            continuation_context_message=(
+                pending_tool_approval.continuation_query
+                if exact_tool_approval
+                and pending_tool_approval
+                and pending_tool_approval.continuation_query
+                else None
+            ),
+            persist_user_message=not tool_approval_continuation,
        )

        _research_flags = {"do": do_research}  # Mutable container for generator scope
@@ -1663,7 +1749,11 @@ def setup_chat_routes(
                if foreground_policy.enabled
                else ctx.messages
            )
-            messages = _ensure_current_request_is_latest_user(context_source, message)
+            messages = (
+                list(context_source)
+                if tool_approval_continuation
+                else _ensure_current_request_is_latest_user(context_source, message)
+            )

            # Auto-compact notification
            if ctx.was_compacted:
@@ -2134,7 +2224,10 @@ def setup_chat_routes(
                                    incognito=incognito, compare_mode=compare_mode,
                                    character_name=ctx.preset.character_name,
                                    owner=_user,
-                                    allow_background_extraction=not tool_policy.block_all_tool_calls,
+                                    allow_background_extraction=(
+                                        not tool_policy.block_all_tool_calls
+                                        and not tool_approval_continuation
+                                    ),
                                )
                            _stream_set(session, status="done")
                            yield chunk
@@ -2223,17 +2316,17 @@ def setup_chat_routes(
                        plan_mode=plan_mode,
                        approved_plan=approved_plan or None,
                        workspace=workspace or None,
+                        relevant_tools=(
+                            set(pending_tool_approval.selected_tools)
+                            if exact_tool_approval
+                            and pending_tool_approval
+                            and pending_tool_approval.selected_tools
+                            else None
+                        ),
                        forced_tools=_forced_tools,
                        uploaded_files=ctx.uploaded_files,
                        defer_context_shaping=_foreground_policy.enabled,
-                        external_untrusted_context_seen=bool(
-                            retired_tool_approval_taint
-                            or (
-                                tool_approval_continuation
-                                and pending_tool_approval
-                                and pending_tool_approval.external_untrusted_context_seen
-                            )
-                        ),
+                        external_untrusted_context_seen=external_untrusted_context_seen,
                        exact_approval=exact_tool_approval,
                    ):
                        if chunk.startswith("data: ") and not chunk.startswith("data: [DONE]"):
@@ -2401,8 +2494,14 @@ def setup_chat_routes(
                                    agent_tool_calls=_agent_tool_calls,
                                    skills_manager=skills_manager,
                                    owner=_user,
-                                    extract_skills=user_requested_agent,
-                                    allow_background_extraction=not tool_policy.block_all_tool_calls,
+                                    extract_skills=(
+                                        user_requested_agent
+                                        and not tool_approval_continuation
+                                    ),
+                                    allow_background_extraction=(
+                                        not tool_policy.block_all_tool_calls
+                                        and not tool_approval_continuation
+                                    ),
                                )
                            _stream_set(session, status="done")
                            yield chunk
@@ -1603,6 +1603,9 @@ def setup_skills_routes(skills_manager: SkillsManager) -> APIRouter:
            decision=decision,
            owner=user,
            session_id=None,
+            # The button here says "Allow once" and there is no chat to carry a
+            # scope into, so the gate must re-arm behind the sealed action.
+            allow_continuation=False,
        )

        if decision == "approve" and exact_approval is None: