From c715df1cf418c41d5ea3b09d7410b0c090e841a9 Mon Sep 17 00:00:00 2001 From: danielhanchen Date: Sun, 26 Jul 2026 12:14:57 +0000 Subject: [PATCH] Fix citation loss, effort clamping and nested inferenceRequest for PR #7219 Three review findings, each with a regression test that fails without the fix. Citation dropped for a bare URL in prose parentheses. _RAW_URL swallows the closing paren and the old trim set only stripped ".,;:!?", so the catalog lookup missed and the validator deleted the whole citation, leaving an unbalanced "(" in the report. New _trim_url_tail follows GFM extended autolink path validation: one right-to-left pass that interleaves punctuation and unmatched-")" trimming. Both rules must run in the same loop, else "https://x/y.)" keeps a stray dot. Balanced parens inside a URL (Wikipedia-style) still survive. Output verified against cmark-gfm on nine cases, including "https://x/foo)bar)" which must keep ")bar". Research runs forwarded reasoningEffort unclamped. The local chat path clamps to the loaded model's advertised levels; the research branch did not, and the backend only validates enum membership, so llama.cpp dropped a level the model lacks and the whole durable run silently fell back to the template default. Now uses the same helper and the same levels as normal chat. Note this makes "max" on a gpt-oss low|medium|high model resolve to "low" rather than falling through to the template default, matching normal chat exactly; the divergence between the two paths was the bug. Nested inferenceRequest values were persisted. Every allowed field is a scalar and the numeric/bool/enum ones reject a container while coercing, but "model" is stringified with str(), which never raises, so {"auth": "sk-..."} slipped past the sensitive-key scan ("auth" is not on the list) into the durable run config as the model id. Mirrors the ragScope guard already in this PR. Verified: 542 passed across the research/web/sandbox/chat-history backend suites, frontend contract 10 passed, tsc --noEmit clean. --- studio/backend/core/research_runs.py | 28 ++++++++++- studio/backend/routes/research_runs.py | 5 ++ .../tests/test_research_runs_hardening.py | 47 +++++++++++++++++++ .../src/features/chat/api/chat-adapter.ts | 8 +++- .../test_deep_research_frontend_contract.py | 14 ++++++ 5 files changed, 99 insertions(+), 3 deletions(-) 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")