From b3b02744039794dd090fe34c8f13d76f501bc679 Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:32:43 -0400 Subject: [PATCH] feat: Re-land deferred upstream features on modular webhooks (#80) (#128) * fix(webhooks): fall back to vision model for Slack/Linear image threads Re-land upstream #1626 onto the modular webhook structure. When a Slack mention or Linear issue carries images but the resolved model is text-only, fall back to a vision-capable model instead of dropping the images. Re-points default_vision_model_pair at the fork's image-capable models (Opus 4.8 default, else any supports_images model) rather than upstream's openai:/anthropic: provider filter. Refs #80, upstream #1626 * fix(slack): persist trace_message_ts so web-handoff updates the trace reply Re-land upstream #1630 onto the modular structure. The first-mention store_slack_run_mapping call did not pass trace_message_ts, so it was never persisted (nothing to preserve from on first mention) and _notify_slack_web_handoff always skipped the trace-reply update on web handoff. Pass it through and cover it with a test. Refs #80, upstream #1630 * feat(slack): include channel context in Slack prompts Re-land upstream #1633 onto the modular structure. Fetch cached Slack channel metadata once per event (_get_slack_channel_context) and thread it through the docs-plz gate, repo resolution, and process_slack_mention so prompts carry the channel name and a clearly-marked untrusted channel description. Avoids duplicate conversations.info calls. Refs #80, upstream #1633 * feat(tools): add slack_start_new_thread breakout tool Re-land upstream #1638 onto the modular structure. Adds the slack_start_new_thread tool (posts a top-level Slack message and dispatches a fresh agent run for a broken-out task via the durable dispatch_agent_run contract), wires it into the agent tool list and tools/__init__, adds prompt guidance, and excludes it from plan mode so it can't bypass the approval flow. Tool imports only live modules. Refs #80, upstream #1638 * feat(plan): notify Slack on plan approval Re-land upstream #1632 onto the modular structure. When a plan is approved via the dashboard approve endpoint, post a thread reply to the originating Slack thread noting the comment count and approver, after the follow-up run is dispatched. Slack post failures never break approval. Adapted to the fork's approve_plan (no plan_markdown read). Refs #80, upstream #1632 * feat(plan): publish plans from sandbox files Re-land upstream #1635 onto the modular structure, completing the partially-ported change so dev is internally consistent. save_plan now takes a plan_file_path, reads the agent-authored Markdown file from /workspace/plans/ (validating extension/location/UTF-8/size) and publishes it, instead of taking a plan_markdown string. Removes write_file/edit_file from PLAN_MODE_EXCLUDED_TOOLS so the agent can author the plan file, updates enter_plan_mode/reject_plan guidance and the e2e fake LLM. Skips the #1610-only update_plan hunk (not on dev). Refs #80, upstream #1635 * fix(security): SSRF-harden server-side image fetch + stop logging raw image URLs INJ-01 (high): fetch_image_block used follow_redirects=True with no per-hop revalidation and discarded the resolved-IP pin, so an attacker-authored Slack/ Linear image URL could 302-redirect the fetch to an internal host / cloud metadata endpoint (blind SSRF), and DNS-rebinding could bypass the one-shot is_url_safe check. Route image fetches through the same per-hop resolve+pin+ revalidate loop the http_request tool uses, lifted into url_safety as the shared request_with_safe_redirects. Also strip the per-host Slack/Linear bearer token on redirect so it can't be replayed to a redirect target. SC-1 (low): linear.py logged full image URLs (which can carry signed tokens) at DEBUG; multimodal logged them at INFO on every fetch. Log host-only. Sink lived in multimodal.py (unchanged by the feature work) but PR #128 widened its reach by no longer dropping images for text-only models. Fixing on the base branch so #130/#129 inherit it on rebase. Adds fetch_image_block SSRF regression tests (redirect-to-internal blocked; auth stripped on redirect). --- agent/dashboard/options.py | 11 +- agent/dashboard/plan_api.py | 49 ++++++- agent/server.py | 2 - agent/tools/enter_plan_mode.py | 20 +-- agent/tools/http_request.py | 83 ++--------- agent/tools/save_plan.py | 108 +++++++++----- agent/utils/multimodal.py | 48 +++---- agent/utils/url_safety.py | 97 ++++++++++++- agent/webapp.py | 2 +- agent/webhooks/linear.py | 38 ++--- agent/webhooks/slack.py | 31 ++-- tests/e2e/fake_llm.py | 2 +- tests/test_http_security.py | 65 +++++++++ tests/test_plan_mode.py | 6 +- tests/test_plan_review.py | 168 +++++++++++++++++++++- tests/test_slack_context.py | 151 +++++++++++++++++++ tests/test_slack_start_new_thread_tool.py | 7 +- 17 files changed, 700 insertions(+), 188 deletions(-) diff --git a/agent/dashboard/options.py b/agent/dashboard/options.py index 7a150466..401e411a 100644 --- a/agent/dashboard/options.py +++ b/agent/dashboard/options.py @@ -167,15 +167,20 @@ def default_model_pair() -> tuple[str, str]: def default_vision_model_pair() -> tuple[str, str]: - """Default OpenAI/Anthropic model pair to use when image input is required.""" + """Default (model_id, reasoning_effort) to use when image input is required. + + Prefers the configured default model when it is vision-capable (Opus 4.8), + otherwise the first vision-capable supported model. The fork ships only + Bedrock/Fireworks models, so this selects on ``supports_images`` rather than + upstream's ``openai:``/``anthropic:`` provider filter. + """ if ( DEFAULT_MODEL_ID in SUPPORTED_MODEL_IDS and model_supports_images(DEFAULT_MODEL_ID) and model_supports_effort(DEFAULT_MODEL_ID, DEFAULT_MODEL_EFFORT) - and DEFAULT_MODEL_ID.startswith(("openai:", "anthropic:")) ): return DEFAULT_MODEL_ID, DEFAULT_MODEL_EFFORT for model in SUPPORTED_MODELS: - if model["id"].startswith(("openai:", "anthropic:")) and model["supports_images"]: + if model["supports_images"]: return model["id"], model["default_effort"] return default_model_pair() diff --git a/agent/dashboard/plan_api.py b/agent/dashboard/plan_api.py index 28eeac04..bde2a5dd 100644 --- a/agent/dashboard/plan_api.py +++ b/agent/dashboard/plan_api.py @@ -22,6 +22,7 @@ from langgraph_sdk import get_client from pydantic import BaseModel from ..dispatch import dispatch_agent_run +from ..utils.slack import post_slack_thread_reply from .oauth import require_same_origin_for_mutations, require_session from .plan_store import ( PLAN_STATUS_APPROVED, @@ -139,7 +140,8 @@ async def approve_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) - raise HTTPException(403, "only the plan owner can approve") # Read comments BEFORE mutating state: a store failure here aborts the # decision (500) rather than dispatching the run without the feedback. - feedback = _format_comments(await list_plan_comments(thread_id, raise_on_error=True)) + comments = await list_plan_comments(thread_id, raise_on_error=True) + feedback = _format_comments(comments) await set_plan_status(thread_id, PLAN_STATUS_APPROVED, plan_mode=False) if feedback: text = ( @@ -149,6 +151,9 @@ async def approve_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) - else: text = "The plan has been approved. Implement it now as described in the plan." await _dispatch_followup(thread_id, metadata, text, plan_mode=False) + await _maybe_post_plan_approved_to_slack( + metadata, comment_count=len(comments), actor=_approval_actor_name(session) + ) return {"status": PLAN_STATUS_APPROVED} @@ -161,7 +166,8 @@ async def reject_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) -> await set_plan_status(thread_id, PLAN_STATUS_REVISING, plan_mode=True) text = ( "The plan needs changes before implementation. Address this reviewer " - "feedback and publish an updated plan with the save_plan tool:\n\n" + "feedback in the existing Markdown file under /workspace/plans/, then " + "publish an updated plan with the save_plan tool:\n\n" f"{feedback or '(no specific comments were left)'}" ) await _dispatch_followup(thread_id, metadata, text, plan_mode=True) @@ -181,6 +187,45 @@ def _format_comments(comments: list[dict[str, Any]]) -> str: return "\n".join(lines) +def _approval_actor_name(session: dict[str, Any]) -> str: + return str(session.get("name") or session.get("sub") or "User").strip() or "User" + + +def _slack_thread_from_metadata(metadata: dict[str, Any]) -> tuple[str, str] | None: + source_context = metadata.get("source_context") + if not isinstance(source_context, dict): + return None + slack_thread = source_context.get("slack_thread") + if not isinstance(slack_thread, dict): + return None + channel_id = slack_thread.get("channel_id") + thread_ts = slack_thread.get("thread_ts") + if isinstance(channel_id, str) and channel_id and isinstance(thread_ts, str) and thread_ts: + return channel_id, thread_ts + return None + + +def _plan_approved_slack_text(comment_count: int, actor: str) -> str: + return f"Plan approved with {comment_count} comments by {actor}\nbeginning implementation" + + +async def _maybe_post_plan_approved_to_slack( + metadata: dict[str, Any], *, comment_count: int, actor: str +) -> None: + slack_thread = _slack_thread_from_metadata(metadata) + if slack_thread is None: + return + channel_id, thread_ts = slack_thread + text = _plan_approved_slack_text(comment_count, actor) + try: + posted = await post_slack_thread_reply(channel_id, thread_ts, text) + except Exception: # noqa: BLE001 + logger.warning("Failed to post plan-approval notice to Slack", exc_info=True) + return + if not posted: + logger.warning("Plan-approval Slack notice was not posted (channel=%s)", channel_id) + + async def _dispatch_followup( thread_id: str, metadata: dict[str, Any], text: str, *, plan_mode: bool ) -> None: diff --git a/agent/server.py b/agent/server.py index bfdf485f..37de7367 100644 --- a/agent/server.py +++ b/agent/server.py @@ -620,8 +620,6 @@ MODEL_CALL_RECURSION_LIMIT = 5_000 # intent. PLAN_MODE_EXCLUDED_TOOLS: frozenset[str] = frozenset( { - "write_file", - "edit_file", "task", "http_request", "open_pull_request", diff --git a/agent/tools/enter_plan_mode.py b/agent/tools/enter_plan_mode.py index 191e1ee7..938cd6f7 100644 --- a/agent/tools/enter_plan_mode.py +++ b/agent/tools/enter_plan_mode.py @@ -15,10 +15,11 @@ from ..dashboard.plan_store import PLAN_STATUS_PLANNING, set_plan_status logger = logging.getLogger(__name__) _ENTERED_MESSAGE = ( - "Plan mode is active. Stay read-only: research the codebase, then record your " - "implementation plan with the `save_plan` tool (it publishes the plan to the " - "review page) and share the plan-review link in the source channel. Do not edit " - "files, commit, push, or open a PR — wait for the user to approve the plan." + "Plan mode is active. Stay read-only for the target repo: research the codebase, " + "create or edit a dated, concise plan file under `/workspace/plans/`, then publish " + "it with the `save_plan` tool and share the plan-review link in the source channel. " + "Do not edit repo files, commit, push, or open a PR — wait for the user to approve " + "the plan." ) @@ -31,11 +32,12 @@ async def enter_plan_mode(tool_call_id: Annotated[str, InjectedToolCallId]) -> C NOT triggered by the word "plan" appearing in the request; use your judgment about whether planning is genuinely warranted. - Once activated, stay read-only: research the codebase, then record your plan - with the ``save_plan`` tool (it publishes the plan to the review page) and - share the plan-review link with the user. Do not edit files, commit, push, - or open a PR — the user reviews the plan and approves it before you - implement. + Once activated, stay read-only for the target repo: research the codebase, + create or edit a dated, concise Markdown plan outside any repo (for example, + ``/workspace/plans/YYYY-MM-DD-short-task-slug.md``), then publish it with + the ``save_plan`` tool and share the plan-review link with the user. Do not + edit repo files, commit, push, or open a PR — the user reviews the plan and + approves it before you implement. """ thread_id = _thread_id_from_config() if thread_id: diff --git a/agent/tools/http_request.py b/agent/tools/http_request.py index aeea541f..1adb6e75 100644 --- a/agent/tools/http_request.py +++ b/agent/tools/http_request.py @@ -1,13 +1,10 @@ from typing import Any -from urllib.parse import urljoin, urlparse, urlunparse import httpx -from ..utils.url_safety import resolve_and_validate as _resolve_and_validate - -_MAX_REDIRECTS = 5 - -_REDIRECT_CODES = {301, 302, 303, 307, 308} +from ..utils.url_safety import pinned_url as _pinned_url # noqa: F401 (kept for tests) +from ..utils.url_safety import request_with_safe_redirects +from ..utils.url_safety import resolve_and_validate as _resolve_and_validate # noqa: F401 def _blocked_response(url: str, reason: str) -> dict[str, Any]: @@ -20,79 +17,19 @@ def _blocked_response(url: str, reason: str) -> dict[str, Any]: } -def _pinned_url(url: str, ip: str) -> str: - """Rewrite ``url`` so the connection targets ``ip`` while keeping the path/query. - - The original hostname is preserved separately for the ``Host`` header and TLS - SNI/cert verification (via httpx's ``sni_hostname`` request extension). - """ - parsed = urlparse(url) - host_literal = f"[{ip}]" if ":" in ip else ip - netloc = f"{host_literal}:{parsed.port}" if parsed.port else host_literal - return urlunparse(parsed._replace(netloc=netloc)) - - async def _request_with_safe_redirects( client: httpx.AsyncClient, method: str, url: str, **kwargs: Any, ) -> tuple[httpx.Response | None, dict[str, Any] | None]: - """Issue a request while validating every redirect target before following it. - - The hostname is resolved once per hop and the connection is pinned to the - validated IP, closing the DNS-rebinding race where a controlled resolver - returns a public IP at validation time and a private IP at connect time. - """ - current_method = method.upper() - current_url = url - request_kwargs = dict(kwargs) - # Pop caller headers/extensions ONCE so they're reused on every redirect hop - # (the per-hop Host + SNI are layered on top each time). Popping inside the - # loop dropped the caller's Authorization/Accept/etc. on the first redirect. - caller_headers = dict(request_kwargs.pop("headers", None) or {}) - caller_extensions = dict(request_kwargs.pop("extensions", None) or {}) - - for redirect_count in range(_MAX_REDIRECTS + 1): - is_safe, reason, hostname, addr_infos = _resolve_and_validate(current_url) - if not is_safe or hostname is None or addr_infos is None: - return None, _blocked_response(current_url, reason) - - pinned_ip = addr_infos[0][4][0] - parsed = urlparse(current_url) - headers = {**caller_headers, "Host": parsed.netloc} - extensions = {**caller_extensions, "sni_hostname": hostname} - - response = await client.request( - current_method, - _pinned_url(current_url, pinned_ip), - follow_redirects=False, - headers=headers, - extensions=extensions, - **request_kwargs, - ) - - if response.status_code not in _REDIRECT_CODES: - return response, None - - location = response.headers.get("Location") - if not location: - return response, None - - if redirect_count == _MAX_REDIRECTS: - return None, _blocked_response(current_url, "Too many redirects") - - current_url = urljoin(current_url, location) - - if response.status_code == 303 or ( - response.status_code in {301, 302} and current_method not in {"GET", "HEAD"} - ): - current_method = "GET" - request_kwargs.pop("data", None) - request_kwargs.pop("content", None) - request_kwargs.pop("json", None) - - return None, _blocked_response(current_url, "Too many redirects") + """Thin wrapper over the shared SSRF-safe redirect loop that shapes a blocked + hop into the tool's error-response dict.""" + response, blocked = await request_with_safe_redirects(client, method, url, **kwargs) + if blocked is not None: + blocked_url, reason = blocked + return None, _blocked_response(blocked_url, reason) + return response, None async def http_request( diff --git a/agent/tools/save_plan.py b/agent/tools/save_plan.py index 3016a0fc..e8fde295 100644 --- a/agent/tools/save_plan.py +++ b/agent/tools/save_plan.py @@ -1,47 +1,59 @@ -"""Tool: ``save_plan``. Record the implementation plan for review. +"""Tool: ``save_plan``. Publish the sandbox plan file for review. -Writes the plan as a real ``plan.md`` file in the sandbox (the artifact the -agent produces and can re-read) and publishes it to the plan-review page, where -the user and collaborators read it, comment inline, and approve or request -changes. Available in plan mode (it does not modify the repository under review). +Reads the Markdown plan file the agent created in the sandbox and publishes it to +the plan-review page, where the user and collaborators read it, comment inline, +and approve or request changes. Available in plan mode (it does not modify the +repository under review). """ from __future__ import annotations import logging +from collections.abc import Mapping from typing import Any from langgraph.config import get_config -from ..dashboard.plan_store import PLAN_STATUS_READY, save_plan_content +from ..dashboard.plan_store import PLAN_FILE_DIRECTORY, PLAN_STATUS_READY, save_plan_content +from ..utils.sandbox_state import get_sandbox_backend logger = logging.getLogger(__name__) -PLAN_FILE_PATH = "plan.md" +_MAX_PLAN_LINES = 20_000 +_MARKDOWN_EXTENSIONS = (".md", ".markdown") -async def save_plan(plan_markdown: str) -> dict[str, Any]: - """Write your implementation plan as a markdown file and publish it for review. +async def save_plan(plan_file_path: str) -> dict[str, Any]: + """Publish a Markdown plan file from the sandbox for review. - Use this in plan mode once your plan is ready. The plan is saved as - ``plan.md`` in the sandbox and published to the plan-review page linked in - the conversation, where the user (the owner) and any collaborators can read - it, leave inline comments, and then approve it or request changes. Call it - again to overwrite the plan with a revised version when addressing feedback. + Use this in plan mode once your plan is ready. First create a Markdown file + under ``/workspace/plans/`` using a dated, descriptive filename, then pass + that file path here. The file contents are published to the plan-review page + linked in the conversation, where the user (the owner) and any collaborators + can read it, leave inline comments, and then approve it or request changes. + Call it again to publish a revised file when addressing feedback. Write the plan in standard Markdown — headings, bullet/numbered lists, and - fenced code blocks all render. Structure it clearly (overview, files to - change, ordered steps, risks). + fenced code blocks all render. Keep it concise and high level, focusing on + approach, decisions/tradeoffs, risks, and verification; avoid file/function + details unless they are unusually tricky or controversial. Args: - plan_markdown: The full plan, as a Markdown document. + plan_file_path: Path to the Markdown plan file in the sandbox. Returns: ``{success: True, path}`` on success, or ``{success: False, error}``. """ - content = plan_markdown.strip() - if not content: - return {"success": False, "error": "plan_markdown cannot be empty"} + if not isinstance(plan_file_path, str): + return {"success": False, "error": "plan_file_path must be a string"} + path = plan_file_path.strip() + if not path: + return {"success": False, "error": "plan_file_path cannot be empty"} + if not _is_markdown_path(path): + return { + "success": False, + "error": f"plan_file_path must point to a Markdown file in {PLAN_FILE_DIRECTORY}", + } try: config = get_config() @@ -53,28 +65,52 @@ async def save_plan(plan_markdown: str) -> dict[str, Any]: return {"success": False, "error": "no thread_id in run config"} try: - path = await _save(str(thread_id), content) + content = (await _read_plan_file(str(thread_id), path)).strip() + if not content: + return {"success": False, "error": "plan file cannot be empty"} + await _save(str(thread_id), content, path) except Exception as exc: # noqa: BLE001 logger.exception("save_plan failed for thread %s", thread_id) return {"success": False, "error": f"failed to save plan: {exc}"} return {"success": True, "path": path} -async def _save(thread_id: str, content: str) -> str: - sandbox_path = await _write_to_sandbox(thread_id, content) - await save_plan_content(thread_id, markdown=content, status=PLAN_STATUS_READY) - return sandbox_path +async def _save(thread_id: str, content: str, path: str) -> None: + await save_plan_content( + thread_id, markdown=content, status=PLAN_STATUS_READY, plan_file_path=path + ) -async def _write_to_sandbox(thread_id: str, content: str) -> str: - """Write ``plan.md`` into the thread's sandbox. Best-effort: a missing sandbox - must not block publishing the plan to the review page.""" - try: - from ..utils.sandbox_state import get_sandbox_backend +async def _read_plan_file(thread_id: str, path: str) -> str: + backend = await get_sandbox_backend(thread_id) + result = await backend.aread(path, offset=0, limit=_MAX_PLAN_LINES) + error = _value(result, "error") + if error: + raise ValueError(error) + file_data = _value(result, "file_data") + if file_data is None: + raise ValueError("plan file could not be read") + encoding = _value(file_data, "encoding") + if encoding is not None and encoding != "utf-8": + raise ValueError("plan file must be UTF-8 text") + content = _value(file_data, "content") + if not isinstance(content, str): + raise ValueError("plan file content was not text") + if content.count("\n") + 1 >= _MAX_PLAN_LINES: + raise ValueError("plan file is too large") + return content - backend = await get_sandbox_backend(thread_id) - await backend.awrite(PLAN_FILE_PATH, content) - return PLAN_FILE_PATH - except Exception: - logger.warning("Could not write plan.md to sandbox for %s", thread_id, exc_info=True) - return PLAN_FILE_PATH + +def _value(value: Any, key: str) -> Any: + if isinstance(value, Mapping): + return value.get(key) + return getattr(value, key, None) + + +def _is_markdown_path(path: str) -> bool: + if "\x00" in path or not path.startswith(f"{PLAN_FILE_DIRECTORY}/"): + return False + filename = path.removeprefix(f"{PLAN_FILE_DIRECTORY}/") + if not filename or "/" in filename: + return False + return filename.lower().endswith(_MARKDOWN_EXTENSIONS) diff --git a/agent/utils/multimodal.py b/agent/utils/multimodal.py index 67709dc5..0bd2b0fc 100644 --- a/agent/utils/multimodal.py +++ b/agent/utils/multimodal.py @@ -13,7 +13,7 @@ from urllib.parse import urlparse import httpx from langchain_core.messages.content import create_image_block -from .url_safety import is_url_safe +from .url_safety import request_with_safe_redirects logger = logging.getLogger(__name__) @@ -52,23 +52,23 @@ async def fetch_image_block( image_url: str, client: httpx.AsyncClient, ) -> dict[str, Any] | None: - """Fetch image bytes and build an image content block.""" + """Fetch image bytes and build an image content block. + + The fetch validates and pins every redirect hop (SSRF guard) and drops any + Authorization header once the URL redirects, so a per-host token is never + replayed to a redirect target the caller never chose to authenticate to. + URLs are logged host-only — a signed image URL can carry a bearer token. + """ + host = (urlparse(image_url).hostname or "").lower() try: - safe, reason = is_url_safe(image_url) - if not safe: - logger.warning("Refusing to fetch image (SSRF guard) %s: %s", image_url, reason) - return None - logger.debug("Fetching image from %s", image_url) headers = None - host = (urlparse(image_url).hostname or "").lower() if host == "uploads.linear.app" or host.endswith(".uploads.linear.app"): linear_api_key = os.environ.get("LINEAR_API_KEY", "") if linear_api_key: headers = {"Authorization": linear_api_key} else: logger.warning( - "LINEAR_API_KEY not set; cannot authenticate image fetch for %s", - image_url, + "LINEAR_API_KEY not set; cannot authenticate image fetch for %s", host ) elif host == "files.slack.com" or host.endswith(".files.slack.com"): slack_bot_token = os.environ.get("SLACK_BOT_TOKEN", "") @@ -76,41 +76,39 @@ async def fetch_image_block( headers = {"Authorization": f"Bearer {slack_bot_token}"} else: logger.warning( - "SLACK_BOT_TOKEN not set; cannot authenticate image fetch for %s", - image_url, + "SLACK_BOT_TOKEN not set; cannot authenticate image fetch for %s", host ) - response = await client.get(image_url, headers=headers, follow_redirects=True) + + response, blocked = await request_with_safe_redirects( + client, "GET", image_url, headers=headers, strip_auth_on_redirect=True + ) + if blocked is not None: + _, reason = blocked + logger.warning("Refusing to fetch image (SSRF guard) from %s: %s", host, reason) + return None response.raise_for_status() content_type = response.headers.get("Content-Type", "").split(";")[0].strip() if not content_type: guessed, _ = mimetypes.guess_type(image_url) if not guessed: - logger.warning( - "Could not determine content type for %s; skipping image", - image_url, - ) + logger.warning("Could not determine content type from %s; skipping image", host) return None content_type = guessed supported_types = {"image/jpeg", "image/png", "image/gif", "image/webp"} if content_type not in supported_types: logger.warning( - "Unsupported content type '%s' for %s; skipping image", - content_type, - image_url, + "Unsupported content type '%s' from %s; skipping image", content_type, host ) return None encoded = base64.b64encode(response.content).decode("ascii") logger.info( - "Fetched image %s (%s, %d bytes)", - image_url, - content_type, - len(response.content), + "Fetched image from %s (%s, %d bytes)", host, content_type, len(response.content) ) return create_image_block(base64=encoded, mime_type=content_type) except Exception: - logger.exception("Failed to fetch image from %s", image_url) + logger.exception("Failed to fetch image from %s", host) return None diff --git a/agent/utils/url_safety.py b/agent/utils/url_safety.py index 69330e7a..7eb2ecbf 100644 --- a/agent/utils/url_safety.py +++ b/agent/utils/url_safety.py @@ -9,7 +9,13 @@ from __future__ import annotations import ipaddress import socket -from urllib.parse import urlparse +from typing import Any +from urllib.parse import urljoin, urlparse, urlunparse + +import httpx + +DEFAULT_MAX_REDIRECTS = 5 +_REDIRECT_CODES = {301, 302, 303, 307, 308} def resolve_and_validate(url: str) -> tuple[bool, str, str | None, list | None]: @@ -61,3 +67,92 @@ def is_url_safe(url: str) -> tuple[bool, str]: """Check if a URL is safe to request (not targeting private/internal networks).""" is_safe, reason, _, _ = resolve_and_validate(url) return is_safe, reason + + +def pinned_url(url: str, ip: str) -> str: + """Rewrite ``url`` so the connection targets ``ip`` while keeping the path/query. + + The original hostname is preserved separately for the ``Host`` header and TLS + SNI/cert verification (via httpx's ``sni_hostname`` request extension). + """ + parsed = urlparse(url) + host_literal = f"[{ip}]" if ":" in ip else ip + netloc = f"{host_literal}:{parsed.port}" if parsed.port else host_literal + return urlunparse(parsed._replace(netloc=netloc)) + + +async def request_with_safe_redirects( + client: httpx.AsyncClient, + method: str, + url: str, + *, + max_redirects: int = DEFAULT_MAX_REDIRECTS, + strip_auth_on_redirect: bool = False, + **kwargs: Any, +) -> tuple[httpx.Response | None, tuple[str, str] | None]: + """Issue a request, validating every redirect target before following it. + + The hostname is resolved once per hop and the connection is pinned to the + validated IP, closing the DNS-rebinding race where a controlled resolver + returns a public IP at validation time and a private IP at connect time. + + Returns ``(response, None)`` on success, or ``(None, (blocked_url, reason))`` + when a hop fails validation or the redirect budget is exhausted. When + ``strip_auth_on_redirect`` is set, the caller's ``Authorization`` header is + dropped once the request leaves the original URL, so a bearer token can't be + replayed to a redirect target the caller never chose to authenticate to. + """ + current_method = method.upper() + current_url = url + request_kwargs = dict(kwargs) + # Pop caller headers/extensions ONCE so they're reused on every redirect hop + # (the per-hop Host + SNI are layered on top each time). Popping inside the + # loop dropped the caller's Authorization/Accept/etc. on the first redirect. + caller_headers = dict(request_kwargs.pop("headers", None) or {}) + caller_extensions = dict(request_kwargs.pop("extensions", None) or {}) + + for redirect_count in range(max_redirects + 1): + is_safe, reason, hostname, addr_infos = resolve_and_validate(current_url) + if not is_safe or hostname is None or addr_infos is None: + return None, (current_url, reason) + + pinned_ip = addr_infos[0][4][0] + parsed = urlparse(current_url) + headers = {**caller_headers, "Host": parsed.netloc} + extensions = {**caller_extensions, "sni_hostname": hostname} + + response = await client.request( + current_method, + pinned_url(current_url, pinned_ip), + follow_redirects=False, + headers=headers, + extensions=extensions, + **request_kwargs, + ) + + if response.status_code not in _REDIRECT_CODES: + return response, None + + location = response.headers.get("Location") + if not location: + return response, None + + if redirect_count == max_redirects: + return None, (current_url, "Too many redirects") + + current_url = urljoin(current_url, location) + + if strip_auth_on_redirect: + caller_headers = { + k: v for k, v in caller_headers.items() if k.lower() != "authorization" + } + + if response.status_code == 303 or ( + response.status_code in {301, 302} and current_method not in {"GET", "HEAD"} + ): + current_method = "GET" + request_kwargs.pop("data", None) + request_kwargs.pop("content", None) + request_kwargs.pop("json", None) + + return None, (current_url, "Too many redirects") diff --git a/agent/webapp.py b/agent/webapp.py index c47fd231..2c170190 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -27,7 +27,7 @@ from .dashboard.agent_overrides import ( ) from .dashboard.enabled_repos import is_review_repo_enabled from .dashboard.oauth import build_settings_url -from .dashboard.options import model_supports_images # noqa: F401 +from .dashboard.options import default_vision_model_pair, model_supports_images # noqa: F401 from .dashboard.profiles import ( # noqa: F401 get_profile, get_valid_access_token, diff --git a/agent/webhooks/linear.py b/agent/webhooks/linear.py index d50366c4..104b1b51 100644 --- a/agent/webhooks/linear.py +++ b/agent/webhooks/linear.py @@ -5,6 +5,7 @@ object (``webapp.X``) so tests that monkeypatch them keep working. """ from typing import Any +from urllib.parse import urlparse import httpx from langchain_core.messages.content import create_text_block @@ -163,31 +164,33 @@ async def process_linear_issue( # noqa: PLR0912, PLR0915 f"When you're done, commit and push your changes. {tag_instruction}" ) content_blocks: list[dict[str, Any]] = [create_text_block(prompt)] + image_model_override: tuple[str, str] | None = None if image_urls: image_urls = webapp.dedupe_urls(image_urls) linear_login = ( await webapp.resolve_login_from_email_async(user_email) if user_email else None ) resolved_model_id = await webapp.resolve_agent_model_id(linear_login) - if webapp.model_supports_images(resolved_model_id): - webapp.logger.info("Preparing %d image(s) for multimodal content", len(image_urls)) - webapp.logger.debug("Image URLs: %s", image_urls) - - async with httpx.AsyncClient(timeout=webapp.DEFAULT_HTTP_TIMEOUT) as client: - for image_url in image_urls: - image_block = await webapp.fetch_image_block(image_url, client) - if image_block: - content_blocks.append(image_block) - webapp.logger.info("Built %d content block(s) for prompt", len(content_blocks)) - else: - webapp.logger.warning( - "Skipping %d image(s) for Linear issue: model %s does not support images", + if not webapp.model_supports_images(resolved_model_id): + fallback_model_id, fallback_effort = webapp.default_vision_model_pair() + webapp.logger.info( + "Using vision fallback model %s for %d Linear image(s); configured model %s " + "does not support images", + fallback_model_id, len(image_urls), resolved_model_id, ) - prompt += webapp.vision_not_supported_warning(resolved_model_id, len(image_urls)) - content_blocks[0] = create_text_block(prompt) - image_urls = [] + resolved_model_id = fallback_model_id + image_model_override = (fallback_model_id, fallback_effort) + webapp.logger.info("Preparing %d image(s) for multimodal content", len(image_urls)) + webapp.logger.debug("Image hosts: %s", [urlparse(u).hostname for u in image_urls]) + + async with httpx.AsyncClient(timeout=webapp.DEFAULT_HTTP_TIMEOUT) as client: + for image_url in image_urls: + image_block = await webapp.fetch_image_block(image_url, client) + if image_block: + content_blocks.append(image_block) + webapp.logger.info("Built %d content block(s) for prompt", len(content_blocks)) linear_project_id = "" linear_issue_number = "" @@ -210,6 +213,9 @@ async def process_linear_issue( # noqa: PLR0912, PLR0915 "user_email": user_email, "source": "linear", } + if image_model_override: + configurable["agent_model_id"] = image_model_override[0] + configurable["agent_effort"] = image_model_override[1] await webapp.upsert_agent_thread_owner_metadata( thread_id, diff --git a/agent/webhooks/slack.py b/agent/webhooks/slack.py index 1a250f8f..01f14280 100644 --- a/agent/webhooks/slack.py +++ b/agent/webhooks/slack.py @@ -240,24 +240,26 @@ async def _process_slack_mention_impl( if not mapped_login and user_email: mapped_login = await webapp.login_for_email(user_email) + image_model_override: tuple[str, str] | None = None if image_urls: resolved_model_id = await webapp.resolve_agent_model_id(mapped_login) - if webapp.model_supports_images(resolved_model_id): - webapp.logger.info("Preparing %d image(s) for Slack mention", len(image_urls)) - async with httpx.AsyncClient(timeout=webapp.DEFAULT_HTTP_TIMEOUT) as http_client: - for image_url in image_urls: - image_block = await webapp.fetch_image_block(image_url, http_client) - if image_block: - content_blocks.append(image_block) - else: - webapp.logger.warning( - "Skipping %d image(s) for Slack mention: model %s does not support images", + if not webapp.model_supports_images(resolved_model_id): + fallback_model_id, fallback_effort = webapp.default_vision_model_pair() + webapp.logger.info( + "Using vision fallback model %s for %d Slack image(s); configured model %s " + "does not support images", + fallback_model_id, len(image_urls), resolved_model_id, ) - prompt += webapp.vision_not_supported_warning(resolved_model_id, len(image_urls)) - content_blocks[0] = create_text_block(prompt) - image_urls = [] + resolved_model_id = fallback_model_id + image_model_override = (fallback_model_id, fallback_effort) + webapp.logger.info("Preparing %d image(s) for Slack mention", len(image_urls)) + async with httpx.AsyncClient(timeout=webapp.DEFAULT_HTTP_TIMEOUT) as http_client: + for image_url in image_urls: + image_block = await webapp.fetch_image_block(image_url, http_client) + if image_block: + content_blocks.append(image_block) # Open SWE opens PRs as the triggering user, so a run only proceeds when we # have a valid user GitHub token. Users who have never signed in with @@ -321,6 +323,9 @@ async def _process_slack_mention_impl( } if mapped_login: configurable["github_login"] = mapped_login + if image_model_override: + configurable["agent_model_id"] = image_model_override[0] + configurable["agent_effort"] = image_model_override[1] thread_plan_mode = await webapp._get_thread_plan_mode(thread_id) if thread_plan_mode is not None: diff --git a/tests/e2e/fake_llm.py b/tests/e2e/fake_llm.py index 99b7f85b..318fcdf5 100644 --- a/tests/e2e/fake_llm.py +++ b/tests/e2e/fake_llm.py @@ -231,7 +231,7 @@ def _save_plan_step(_messages: list[BaseMessage]) -> AIMessage: tool_calls=[ { "name": "save_plan", - "args": {"plan_markdown": PLAN_MARKDOWN}, + "args": {"plan_file_path": PLAN_FILE_PATH}, "id": "call-save-plan", } ], diff --git a/tests/test_http_security.py b/tests/test_http_security.py index 45266413..9771c4c2 100644 --- a/tests/test_http_security.py +++ b/tests/test_http_security.py @@ -20,6 +20,7 @@ fetch_url_tool = sys.modules["agent.tools.fetch_url"] http_request_tool = sys.modules["agent.tools.http_request"] # SSRF resolution now lives in the shared validator; patch DNS there. url_safety = importlib.import_module("agent.utils.url_safety") +multimodal = importlib.import_module("agent.utils.multimodal") _NO_JSON = object() @@ -310,3 +311,67 @@ async def test_http_request_returns_timeout_result(monkeypatch) -> None: assert result["success"] is False assert result["status_code"] == 0 assert "timed out after 7 seconds" in result["content"] + + +# --- fetch_image_block SSRF (INJ-01 regression) ------------------------------ + + +def _image_dns(monkeypatch, ip_map: dict[str, str]) -> None: + monkeypatch.setattr( + url_safety.socket, + "getaddrinfo", + lambda host, port, *a, **k: [_addr_info(ip_map[host], port)], + ) + + +async def test_fetch_image_block_blocks_redirect_to_internal_host(monkeypatch) -> None: + """A public image URL that 302-redirects to the metadata endpoint must not be + followed: the redirect target is re-validated and the internal hop blocked.""" + _image_dns( + monkeypatch, + {"public.example": "93.184.216.34", "internal.evil": "169.254.169.254"}, + ) + + def responder(method, url, **kwargs): + return FakeResponse( + status_code=302, + url=url, + headers={"Location": "http://internal.evil/latest/meta-data/"}, + ) + + client = FakeAsyncClient(responder) + result = await multimodal.fetch_image_block("http://public.example/x.png", client) + + assert result is None + # Exactly one hop was issued (to the validated public host); the redirect to + # the internal host was blocked before any request left the process. + assert len(client.calls) == 1 + assert all("169.254.169.254" not in call["url"] for call in client.calls) + + +async def test_fetch_image_block_strips_auth_header_on_redirect(monkeypatch) -> None: + """The per-host Slack bearer token must not be replayed to a redirect target.""" + monkeypatch.setenv("SLACK_BOT_TOKEN", "xoxb-secret") + _image_dns( + monkeypatch, + {"files.slack.com": "93.184.216.34", "cdn.public": "93.184.216.35"}, + ) + + def responder(method, url, **kwargs): + if len(client.calls) == 1: + return FakeResponse( + status_code=302, url=url, headers={"Location": "https://cdn.public/x.png"} + ) + resp = FakeResponse(status_code=200, url=url, headers={"Content-Type": "image/png"}) + resp.content = b"\x89PNG\r\n" + return resp + + client = FakeAsyncClient(responder) + result = await multimodal.fetch_image_block("https://files.slack.com/x.png", client) + + assert result is not None + assert len(client.calls) == 2 + first_auth = client.calls[0]["headers"].get("Authorization") + second_auth = client.calls[1]["headers"].get("Authorization") + assert first_auth == "Bearer xoxb-secret" + assert second_auth is None diff --git a/tests/test_plan_mode.py b/tests/test_plan_mode.py index cbf8ca59..32b3392b 100644 --- a/tests/test_plan_mode.py +++ b/tests/test_plan_mode.py @@ -24,8 +24,6 @@ def test_plan_mode_prompt_absent_by_default() -> None: def test_plan_mode_excluded_tools_cover_mutating_tools() -> None: excluded = server.PLAN_MODE_EXCLUDED_TOOLS for tool in ( - "write_file", - "edit_file", "task", "open_pull_request", "request_pr_review", @@ -38,6 +36,10 @@ def test_plan_mode_excluded_tools_cover_mutating_tools() -> None: # Read-only tools must stay available. assert "read_file" not in excluded assert "execute" not in excluded + # File edit tools stay available so the agent can author the plan file under + # /workspace/plans/ (restricted to that path via prompt guidance). + assert "write_file" not in excluded + assert "edit_file" not in excluded class _FakeThreadsClient: diff --git a/tests/test_plan_review.py b/tests/test_plan_review.py index 74894594..74804d38 100644 --- a/tests/test_plan_review.py +++ b/tests/test_plan_review.py @@ -99,12 +99,12 @@ async def test_save_plan_requires_run_context() -> None: from agent.tools.save_plan import save_plan # No LangGraph run context → no thread_id → graceful error, not a crash. - result = await save_plan("## Plan") + result = await save_plan("/workspace/plans/2026-07-08-test-plan.md") assert result["success"] is False assert "thread_id" in result["error"] -async def test_save_plan_rejects_empty_markdown() -> None: +async def test_save_plan_rejects_empty_path() -> None: from agent.tools.save_plan import save_plan result = await save_plan(" ") @@ -112,6 +112,70 @@ async def test_save_plan_rejects_empty_markdown() -> None: assert "empty" in result["error"] +async def test_save_plan_rejects_non_markdown_path() -> None: + from agent.tools.save_plan import save_plan + + result = await save_plan("/workspace/plans/plan.txt") + assert result["success"] is False + assert "Markdown" in result["error"] + + +async def test_save_plan_rejects_markdown_outside_plans_dir() -> None: + from agent.tools.save_plan import save_plan + + result = await save_plan("/workspace/plan.md") + assert result["success"] is False + assert "/workspace/plans" in result["error"] + + +async def test_save_plan_reads_markdown_file_from_sandbox( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import importlib + + save_plan_tool = importlib.import_module("agent.tools.save_plan") + + saved: dict[str, Any] = {} + reads: list[tuple[str, int, int]] = [] + + class _Backend: + async def aread(self, file_path: str, offset: int = 0, limit: int = 2000) -> dict[str, Any]: + reads.append((file_path, offset, limit)) + return {"file_data": {"encoding": "utf-8", "content": "# Plan\n\nDo it.\n"}} + + async def fake_backend(thread_id: str) -> _Backend: + assert thread_id == "thread-1" + return _Backend() + + async def fake_save_content( + thread_id: str, *, markdown: str, status: str, plan_file_path: str | None = None + ) -> None: + saved.update( + thread_id=thread_id, markdown=markdown, status=status, plan_file_path=plan_file_path + ) + + monkeypatch.setattr( + save_plan_tool, + "get_config", + lambda: {"configurable": {"thread_id": "thread-1"}}, + ) + monkeypatch.setattr(save_plan_tool, "get_sandbox_backend", fake_backend) + monkeypatch.setattr(save_plan_tool, "save_plan_content", fake_save_content) + + result = await save_plan_tool.save_plan("/workspace/plans/2026-07-08-test-plan.md") + + assert result == {"success": True, "path": "/workspace/plans/2026-07-08-test-plan.md"} + assert reads == [ + ("/workspace/plans/2026-07-08-test-plan.md", 0, save_plan_tool._MAX_PLAN_LINES) + ] + assert saved == { + "thread_id": "thread-1", + "markdown": "# Plan\n\nDo it.", + "status": "ready", + "plan_file_path": "/workspace/plans/2026-07-08-test-plan.md", + } + + def test_plan_routes_registered() -> None: from agent.webapp import app @@ -145,12 +209,27 @@ def test_plan_status_constants() -> None: assert plan_store.PLAN_STATUS_REVISING == "revising" +def test_plan_file_path_for_thread_uses_plans_dir_and_slug() -> None: + from agent.dashboard import plan_store + + path = plan_store.plan_file_path_for_thread("Thread ABC/123") + assert path.startswith("/workspace/plans/") + assert path.endswith("-thread-abc-123.md") + + def test_http_request_excluded_in_plan_mode() -> None: from agent.server import PLAN_MODE_EXCLUDED_TOOLS assert "http_request" in PLAN_MODE_EXCLUDED_TOOLS +def test_file_edit_tools_available_in_plan_mode_for_plan_file() -> None: + from agent.server import PLAN_MODE_EXCLUDED_TOOLS + + assert "write_file" not in PLAN_MODE_EXCLUDED_TOOLS + assert "edit_file" not in PLAN_MODE_EXCLUDED_TOOLS + + class _FakeReq: def __init__(self, tools: list[Any], state: dict[str, Any]) -> None: self.tools = tools @@ -182,3 +261,88 @@ def test_plan_mode_middleware_self_activation_via_state() -> None: # After enter_plan_mode sets state: the next request is filtered. on = _FakeReq([{"name": "read_file"}, {"name": "write_file"}], {"plan_mode": True}) assert _names(mw._filter(on)) == {"read_file"} + + +def test_plan_approved_slack_text_mentions_comments_actor_and_start() -> None: + from agent.dashboard.plan_api import _plan_approved_slack_text + + text = _plan_approved_slack_text(3, "Alice") + assert text == "Plan approved with 3 comments by Alice\nbeginning implementation" + + +async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.MonkeyPatch) -> None: + from agent.dashboard import plan_api + + metadata = { + "github_login": "alice", + "source_context": {"slack_thread": {"channel_id": "C1", "thread_ts": "1700000000.0001"}}, + } + posted: dict[str, Any] = {} + + async def fake_thread_metadata(thread_id: str) -> dict[str, Any]: + return metadata + + def fake_user_owns_thread(md: dict, sub: str, email: str | None) -> bool: + return True + + async def fake_list_plan_comments( + thread_id: str, *, raise_on_error: bool = False + ) -> list[dict]: + return [{"author": "bob", "body": "tweak this"}] + + async def fake_set_plan_status(thread_id: str, status: str, *, plan_mode: bool) -> None: + posted["status"] = {"status": status, "plan_mode": plan_mode} + + async def fake_dispatch_followup( + thread_id: str, md: dict, text: str, *, plan_mode: bool + ) -> None: + posted["dispatch"] = {"text": text, "plan_mode": plan_mode} + + async def fake_post_slack_thread_reply(channel_id: str, thread_ts: str, text: str) -> bool: + posted["slack"] = {"channel_id": channel_id, "thread_ts": thread_ts, "text": text} + return True + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) + monkeypatch.setattr(plan_api, "_user_owns_thread", fake_user_owns_thread) + monkeypatch.setattr(plan_api, "list_plan_comments", fake_list_plan_comments) + monkeypatch.setattr(plan_api, "set_plan_status", fake_set_plan_status) + monkeypatch.setattr(plan_api, "_dispatch_followup", fake_dispatch_followup) + monkeypatch.setattr(plan_api, "post_slack_thread_reply", fake_post_slack_thread_reply) + + result = await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"}) + + assert result == {"status": plan_api.PLAN_STATUS_APPROVED} + assert posted["dispatch"]["plan_mode"] is False + assert posted["slack"] == { + "channel_id": "C1", + "thread_ts": "1700000000.0001", + "text": "Plan approved with 1 comments by Alice\nbeginning implementation", + } + + +async def test_set_plan_status_preserves_plan_file_path(monkeypatch: pytest.MonkeyPatch) -> None: + from agent.dashboard import plan_store + + existing = { + "markdown": "# Plan", + "status": "ready", + "plan_file_path": "/workspace/plans/foo.md", + } + saved: dict[str, Any] = {} + + class _Store: + async def get_item(self, *a: Any, **k: Any) -> Any: + return {"value": existing} + + async def put_item(self, namespace: Any, key: str, value: Any, *a: Any, **k: Any) -> None: + saved.update(value) + + async def fake_merge(thread_id: str, metadata: dict[str, Any]) -> None: + return None + + monkeypatch.setattr(plan_store, "_client", lambda: _fake_client(_Store())) + monkeypatch.setattr(plan_store, "_merge_thread_metadata", fake_merge) + + await plan_store.set_plan_status("t", plan_store.PLAN_STATUS_REVISING, plan_mode=True) + assert saved["plan_file_path"] == "/workspace/plans/foo.md" + assert saved["status"] == plan_store.PLAN_STATUS_REVISING diff --git a/tests/test_slack_context.py b/tests/test_slack_context.py index 6d44fa0e..9bfd8c3f 100644 --- a/tests/test_slack_context.py +++ b/tests/test_slack_context.py @@ -17,6 +17,9 @@ from agent.utils.slack import ( ) from agent.webapp import generate_thread_id_from_slack_thread +_TEXT_ONLY_MODEL = "fireworks:accounts/fireworks/models/glm-5p2" +_VISION_MODEL = "bedrock_converse:us.anthropic.claude-opus-4-8" + class _FakeNotFoundError(Exception): status_code = 404 @@ -689,6 +692,154 @@ def test_process_slack_mention_creates_thread_first_run_with_trace_reply( assert "## Latest Mention Request\ncontinue on the branch" in prompt_block["text"] +def test_process_slack_mention_uses_vision_fallback_for_image_thread( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: dict[str, object] = {} + _setup_slack_mention_fakes(monkeypatch, captured) + + async def fake_thread_exists(thread_id: str) -> bool: + return False + + async def fake_fetch_slack_thread_messages(channel_id: str, thread_ts: str) -> list[dict]: + return [ + { + "ts": "1700000000.000100", + "text": "<@UBOT> please inspect this", + "user": "U123", + "files": [ + { + "mimetype": "image/png", + "url_private": "https://files.slack.com/screenshot.png", + } + ], + } + ] + + async def fake_resolve_agent_model_id(login: str | None) -> str: + assert login == "mason-gh" + return _TEXT_ONLY_MODEL + + async def fake_fetch_image_block(image_url: str, client: object) -> dict[str, str]: + captured["image_url"] = image_url + return {"type": "image", "source_type": "base64", "mime_type": "image/png", "data": "abc"} + + monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists) + monkeypatch.setattr(webapp, "fetch_slack_thread_messages", fake_fetch_slack_thread_messages) + monkeypatch.setattr(webapp, "resolve_agent_model_id", fake_resolve_agent_model_id) + monkeypatch.setattr(webapp, "fetch_image_block", fake_fetch_image_block) + + asyncio.run( + webapp.process_slack_mention( + { + "channel_id": "C123", + "thread_ts": "1700000000.000100", + "event_ts": "1700000000.000100", + "user_id": "U123", + "text": "<@UBOT> please inspect this", + "bot_user_id": "UBOT", + }, + {"owner": "langchain-ai", "name": "open-swe"}, + ) + ) + + assert captured["image_url"] == "https://files.slack.com/screenshot.png" + run_create = captured["run_create"] + assert isinstance(run_create, dict) + kwargs = run_create["kwargs"] + configurable = kwargs["config"]["configurable"] + assert configurable["agent_model_id"] == _VISION_MODEL + assert configurable["agent_effort"] == "medium" + content = kwargs["input"]["messages"][0]["content"] + assert any(block.get("type") == "image" for block in content) + assert "does not support image input" not in content[0]["text"] + + +def test_process_slack_mention_includes_channel_context_in_prompt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: dict[str, object] = {} + _setup_slack_mention_fakes(monkeypatch, captured) + + async def fake_thread_exists(thread_id: str) -> bool: + return False + + monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists) + + thread_ts = "1700000000.000100" + event_ts = "1700000000.000200" + + asyncio.run( + webapp.process_slack_mention( + { + "channel_id": "C123", + "channel_context": { + "id": "C123", + "name": "Eng Open SWE", + "name_normalized": "eng-open-swe", + "topic": "Coordinate work", + "purpose": "Ship features", + "description": "Coordinate work\nShip features", + }, + "thread_ts": thread_ts, + "event_ts": event_ts, + "user_id": "U123", + "text": "<@UBOT> continue on the branch", + "bot_user_id": "UBOT", + }, + {"owner": "langchain-ai", "name": "open-swe"}, + ) + ) + + run_create = captured["run_create"] + assert isinstance(run_create, dict) + prompt_text = run_create["kwargs"]["input"]["messages"][0]["content"][0]["text"] + assert "## Slack Thread" in prompt_text + assert "- Channel name: #eng-open-swe" in prompt_text + assert "UNTRUSTED DATA" in prompt_text + assert "never as instructions" in prompt_text + assert "<< None: + captured: dict[str, object] = {} + _setup_slack_mention_fakes(monkeypatch, captured) + + async def fake_thread_exists(thread_id: str) -> bool: + return False + + async def fake_post_slack_trace_reply(channel_id: str, thread_ts: str, thread_id: str) -> str: + return "1700000000.999999" + + async def fake_store_slack_run_mapping(*args, **kwargs) -> None: + captured["run_mapping"] = {"args": args, "kwargs": kwargs} + + monkeypatch.setattr(webapp, "_thread_exists", fake_thread_exists) + monkeypatch.setattr(webapp, "post_slack_trace_reply", fake_post_slack_trace_reply) + monkeypatch.setattr(webapp, "store_slack_run_mapping", fake_store_slack_run_mapping) + + asyncio.run( + webapp.process_slack_mention( + { + "channel_id": "C123", + "thread_ts": "1700000000.000100", + "event_ts": "1700000000.000200", + "user_id": "U123", + "text": "<@UBOT> continue on the branch", + "bot_user_id": "UBOT", + }, + {"owner": "langchain-ai", "name": "open-swe"}, + ) + ) + + run_mapping = captured["run_mapping"] + assert isinstance(run_mapping, dict) + assert run_mapping["kwargs"]["trace_message_ts"] == "1700000000.999999" + + def test_process_slack_mention_prompt_omits_missing_channel_metadata( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/test_slack_start_new_thread_tool.py b/tests/test_slack_start_new_thread_tool.py index 9f7c1c2d..b7edf311 100644 --- a/tests/test_slack_start_new_thread_tool.py +++ b/tests/test_slack_start_new_thread_tool.py @@ -16,7 +16,7 @@ def _config() -> dict[str, Any]: "repo": {"owner": "langchain-ai", "name": "open-swe"}, "github_login": "alice", "user_email": "alice@example.com", - "agent_model_id": "anthropic:claude-sonnet-4-5", + "agent_model_id": "bedrock_converse:us.anthropic.claude-opus-4-8", "agent_effort": "high", "slack_thread": { "channel_id": "C1", @@ -170,7 +170,10 @@ async def test_slack_start_new_thread_success(monkeypatch: pytest.MonkeyPatch) - assert dispatch["configurable"]["slack_thread"]["thread_ts"] == new_ts assert dispatch["configurable"]["repo"] == {"owner": "langchain-ai", "name": "open-swe"} assert dispatch["configurable"]["github_login"] == "alice" - assert dispatch["configurable"]["agent_model_id"] == "anthropic:claude-sonnet-4-5" + assert ( + dispatch["configurable"]["agent_model_id"] + == "bedrock_converse:us.anthropic.claude-opus-4-8" + ) assert "Breakout Instructions" in dispatch["content"] assert captured["trace"] == { "channel_id": "C1",