diff --git a/studio/backend/core/research_runs.py b/studio/backend/core/research_runs.py index 09ec9e061b..c229720e84 100644 --- a/studio/backend/core/research_runs.py +++ b/studio/backend/core/research_runs.py @@ -645,6 +645,29 @@ def _split_rag_result(result: str) -> tuple[str, list[dict[str, Any]]]: return text.rstrip(), sources +def _trim_url_tail(raw: str) -> str: + """Strip trailing prose punctuation that ``_RAW_URL`` swallowed. + + Mirrors GFM extended autolink path validation: walk right to left, dropping + ``.,;:!?`` and any ``)`` that has no matching ``(`` inside the URL, stopping at the + first character that is neither. Both rules must run in one interleaved pass, else + ``https://x/y.)`` keeps a stray dot. Without this, ``(https://x/y)`` never matches + the catalog and the citation is dropped from the report. + """ + end = len(raw) + opening, closing = raw.count("("), raw.count(")") + while end: + char = raw[end - 1] + if char == ")": + if closing <= opening: + break + closing -= 1 + elif char not in ".,;:!?": + break + end -= 1 + return raw[:end] + + def _research_step_failed(web_result: str, rag_sources: list[dict]) -> bool: return is_tool_error(web_result) and not rag_sources @@ -746,10 +769,11 @@ def _validate_report_sources(report: str, sources: list[dict]) -> str: def replace_raw_url(match: re.Match) -> str: # Cite whole source URLs; drop other raw URLs. Whole-match avoids prefix collisions. raw = match.group(0) - core = raw.rstrip(".,;:!?") + core = _trim_url_tail(raw) if core in source_by_url: return (citation(core) or core) + raw[len(core) :] - return "" + # Keep the trimmed tail so dropping the URL cannot unbalance the prose. + return raw[len(core) :] validated = replace_markdown_links(report) validated = _AUTOLINK.sub(replace_autolink, validated) diff --git a/studio/backend/routes/research_runs.py b/studio/backend/routes/research_runs.py index 087f889055..74faf9db2b 100644 --- a/studio/backend/routes/research_runs.py +++ b/studio/backend/routes/research_runs.py @@ -183,6 +183,11 @@ def _sanitize_config(payload: CreateResearchRun, thread: dict) -> dict: status_code = 400, detail = f"Unsupported inferenceRequest fields: {', '.join(sorted(unknown))}", ) + # Mirrors the ragScope guard below. Every allowed field is a scalar; the numeric/bool/enum ones + # reject a container while coercing, but "model" is stringified, so {"auth": "sk-..."} slips past + # the sensitive-key scan (inner key unlisted) into the durable config as the model id. + if any(isinstance(value, (dict, list, tuple)) for value in request.values()): + raise HTTPException(status_code = 400, detail = "Invalid inferenceRequest value") model = str(request.get("model") or thread.get("modelId") or "").strip() if not model: raise HTTPException(status_code = 400, detail = "A selected local model is required") diff --git a/studio/backend/tests/test_research_runs_hardening.py b/studio/backend/tests/test_research_runs_hardening.py index f8759658c3..349672c347 100644 --- a/studio/backend/tests/test_research_runs_hardening.py +++ b/studio/backend/tests/test_research_runs_hardening.py @@ -133,6 +133,23 @@ def test_sanitize_config_rejects_nested_inference_credential(): _sanitize_config(payload, {"modelId": "m"}) +def test_sanitize_config_rejects_nonscalar_inference_request_value(): + # Companion to the ragScope case below. "model" is the one allowed field coerced with str(), + # which never raises, so a container whose inner key is not on the sensitive list ("auth" is + # not) was stringified into the durable run config as the model id. + for request in ({"model": {"auth": "sk-private-value"}}, {"model": ["sk-private-value"]}): + with pytest.raises(Exception): + _sanitize_config(_make_payload(inferenceRequest = request), {"modelId": "m"}) + + +def test_sanitize_config_accepts_scalar_inference_request(): + # Well-formed runs must be unaffected by the rejection above. + request = {"model": "m", "temperature": 0.7, "topP": 0.9, + "maxTokens": 1024, "enableThinking": True, "reasoningEffort": "high"} + config = _sanitize_config(_make_payload(inferenceRequest = dict(request)), {"modelId": "other"}) + assert config["inferenceRequest"] == request + + def test_sanitize_config_rejects_nested_rag_scope_secret(): payload = _make_payload(ragScope = {"kb_id": {"token": "rag-secret"}}) with pytest.raises(Exception): @@ -199,3 +216,33 @@ def test_raw_url_citation_does_not_collide_on_prefix(): ) assert "[Report](https://ex.com/report)" in out assert "/report)-attack" not in out + + +def test_raw_url_in_prose_parentheses_keeps_its_citation(): + # ``_RAW_URL`` swallows the closing paren, so the catalog lookup used to miss and the + # whole citation was deleted, leaving an unbalanced "(" in the report. + sources = [{"url": "https://ex.com/report", "title": "Report"}] + out = _validate_report_sources("Public (https://ex.com/report) today.", sources) + assert out == "Public ([Report](https://ex.com/report)) today." + + +def test_raw_url_keeps_parentheses_that_belong_to_the_url(): + # Only unmatched trailing parens are prose; Wikipedia-style URLs must survive both bare + # and wrapped (GFM extended autolink path validation). + url = "https://en.wikipedia.org/wiki/Mercury_(planet)" + sources = [{"url": url, "title": "Mercury"}] + assert f"[Mercury]({url})" in _validate_report_sources(f"Bare {url} ok.", sources) + assert f"[Mercury]({url})" in _validate_report_sources(f"Wrapped ({url}) ok.", sources) + + +def test_raw_url_trailing_punctuation_is_trimmed_in_one_pass(): + # Trimming parens and punctuation in separate passes leaves a stray "." on ".)"; both + # rules have to run right to left in the same loop. + sources = [{"url": "https://ex.com/x", "title": "X"}] + assert "[X](https://ex.com/x)." in _validate_report_sources("End (https://ex.com/x.).", sources) + + +def test_dropped_raw_url_does_not_unbalance_prose(): + # An uncataloged URL is still removed, but the paren it swallowed belongs to the prose. + out = _validate_report_sources("Claim (https://nope.com/x) here.", []) + assert out == "Claim () here." diff --git a/studio/frontend/src/features/chat/api/chat-adapter.ts b/studio/frontend/src/features/chat/api/chat-adapter.ts index 50ac898540..9907108eaf 100644 --- a/studio/frontend/src/features/chat/api/chat-adapter.ts +++ b/studio/frontend/src/features/chat/api/chat-adapter.ts @@ -2170,7 +2170,13 @@ export function createOpenAIStreamAdapter( (runtime.reasoningStyle === "reasoning_effort" || runtime.reasoningStyle === "enable_thinking_effort") ) { - inferenceRequest.reasoningEffort = runtime.reasoningEffort; + // Clamp like normal chat does. reasoningEffort is one shared persisted setting and + // the load paths refresh reasoningEffortLevels without re-clamping it, so a level + // this model lacks is dropped by llama.cpp and the run falls back to the default. + inferenceRequest.reasoningEffort = clampReasoningEffortToLevels( + runtime.reasoningEffort, + runtime.reasoningEffortLevels, + ); } const researchProjectId = await resolveProjectId(resolvedThreadId); const projectRagEnabled = researchProjectId diff --git a/tests/studio/test_deep_research_frontend_contract.py b/tests/studio/test_deep_research_frontend_contract.py index 4fd6974839..d122e4dae8 100644 --- a/tests/studio/test_deep_research_frontend_contract.py +++ b/tests/studio/test_deep_research_frontend_contract.py @@ -83,6 +83,20 @@ def test_research_mode_is_single_chat_and_detaches_without_cancel() -> None: assert "resolveChatInstructions" in adapter +def test_research_reasoning_effort_is_clamped_to_the_loaded_model() -> None: + # A level the loaded model lacks is dropped by llama.cpp, so the whole durable run + # would silently fall back to the template default. Must use the same helper and the + # same levels as normal local chat so the two paths cannot drift apart again. + adapter = source("features/chat/api/chat-adapter.ts") + branch = adapter.split("Deep research requires a selected local model.", 1)[1].split( + "createdRun = await createResearchRun({", 1 + )[0] + assert "inferenceRequest.reasoningEffort = runtime.reasoningEffort;" not in branch + assert "inferenceRequest.reasoningEffort = clampReasoningEffortToLevels(" in branch + assert "runtime.reasoningEffortLevels," in branch + assert "const localReasoningEffort = clampReasoningEffortToLevels(" in adapter + + def test_research_metadata_and_server_merge_are_persisted() -> None: adapter = source("features/chat/api/chat-adapter.ts") runtime = source("features/chat/runtime-provider.tsx")