diff --git a/api/config.py b/api/config.py index f8a598a0193..7a7acb4080b 100644 --- a/api/config.py +++ b/api/config.py @@ -4332,6 +4332,7 @@ def get_reasoning_status( model_id: str | None = None, provider_id: str | None = None, base_url: str | None = None, + reasoning_effort: str | None = None, ) -> dict: """Return current reasoning configuration from the active profile's config.yaml — the same source of truth the CLI reads from. @@ -4344,7 +4345,11 @@ def get_reasoning_status( display_cfg = config_data.get("display") or {} agent_cfg = config_data.get("agent") or {} show_raw = display_cfg.get("show_reasoning") if isinstance(display_cfg, dict) else None - effort_raw = agent_cfg.get("reasoning_effort") if isinstance(agent_cfg, dict) else None + effort_raw = ( + reasoning_effort + if reasoning_effort is not None + else agent_cfg.get("reasoning_effort") if isinstance(agent_cfg, dict) else None + ) resolve_model = model_id resolve_provider = provider_id diff --git a/api/gateway_chat.py b/api/gateway_chat.py index 6ee388cf72c..0c84c3546e4 100644 --- a/api/gateway_chat.py +++ b/api/gateway_chat.py @@ -310,12 +310,16 @@ def _gateway_use_runs_api_enabled(config_data=None, environ: dict[str, str] | No return raw in ("1", "true", "yes", "on") -def _gateway_reasoning_effort_for_request(cfg, *, model=None, model_provider=None): +def _gateway_reasoning_effort_for_request(cfg, *, model=None, model_provider=None, reasoning_effort=None): """Read and coerce user-configured reasoning effort for a gateway request.""" try: cfg_data = cfg if isinstance(cfg, dict) else {} effort_cfg = cfg_data.get("agent", {}) if isinstance(cfg_data, dict) else {} - effort_raw = effort_cfg.get("reasoning_effort") if isinstance(effort_cfg, dict) else None + effort_raw = ( + reasoning_effort + if reasoning_effort is not None + else effort_cfg.get("reasoning_effort") if isinstance(effort_cfg, dict) else None + ) coerced = coerce_reasoning_effort_for_model( effort_raw, model, @@ -995,6 +999,7 @@ def put_gateway_event(event, data): cfg, model=model, model_provider=model_provider, + reasoning_effort=getattr(s, "reasoning_effort", None), ) base_url = _gateway_base_url(cfg) api_key = _gateway_api_key() diff --git a/api/models.py b/api/models.py index 2ad051b21b0..1a096ea4de0 100644 --- a/api/models.py +++ b/api/models.py @@ -1196,6 +1196,7 @@ def __init__(self, session_id: str=None, title: str='Untitled', workspace=str(DEFAULT_WORKSPACE), created_workspace=None, model=DEFAULT_MODEL, model_provider=None, + reasoning_effort=None, messages=None, created_at=None, updated_at=None, tool_calls=None, pinned: bool=False, archived: bool=False, project_id: str=None, profile=None, @@ -1263,6 +1264,7 @@ def __init__(self, session_id: str=None, title: str='Untitled', ) self.model = model self.model_provider = str(model_provider).strip().lower() if model_provider else None + self.reasoning_effort = reasoning_effort # #5979: signature of the model the user DELIBERATELY picked this session # (``"\x1f"``), or None. Used by the streaming resolver # to preserve a custom-proxy vendor namespace on a COLD catalog ONLY when @@ -1393,7 +1395,7 @@ def save(self, touch_updated_at: bool = True, skip_index: bool = False) -> None: # without parsing the full messages array (which may be 400KB+). # Fields are listed in the order they should appear in the JSON file. METADATA_FIELDS = [ - 'session_id', 'title', 'workspace', 'created_workspace', 'model', 'model_provider', 'model_explicit_pick_signature', 'created_at', 'updated_at', + 'session_id', 'title', 'workspace', 'created_workspace', 'model', 'model_provider', 'reasoning_effort', 'model_explicit_pick_signature', 'created_at', 'updated_at', 'pinned', 'archived', 'project_id', 'profile', 'input_tokens', 'output_tokens', 'estimated_cost', 'cache_read_tokens', 'cache_write_tokens', diff --git a/api/routes.py b/api/routes.py index 920a3663299..7b39a37e28b 100644 --- a/api/routes.py +++ b/api/routes.py @@ -13383,12 +13383,15 @@ def handle_get(handler, parsed) -> bool: model_id = (query.get("model", [""])[0] or "").strip() or None provider_id = (query.get("provider", [""])[0] or "").strip() or None base_url = (query.get("base_url", [""])[0] or "").strip() or None + session_id = (query.get("session_id", [""])[0] or "").strip() + session = get_session(session_id, metadata_only=True) if session_id else None return j( handler, get_reasoning_status( model_id=model_id, provider_id=provider_id, base_url=base_url, + reasoning_effort=getattr(session, "reasoning_effort", None), ), ) @@ -15412,6 +15415,11 @@ def _commit_prev_session_memory(_sid=prev_session_id): enabled_toolsets=getattr(session, "enabled_toolsets", None), context_length=getattr(session, "context_length", None), threshold_tokens=getattr(session, "threshold_tokens", None), + # Reasoning effort is a per-session override (#6809). Without + # carrying it, the duplicate reads None and silently falls back + # to the profile-global default, so a source session set to + # xhigh produces a copy running at whatever the profile says. + reasoning_effort=getattr(session, "reasoning_effort", None), truncation_watermark=getattr(session, "truncation_watermark", None), truncation_boundary=getattr(session, "truncation_boundary", None), # context_messages is the authoritative model-facing prefix — must be @@ -15553,6 +15561,22 @@ def _commit_prev_session_memory(_sid=prev_session_id): model_id = str(body.get("model") or "").strip() or None provider_id = str(body.get("provider") or "").strip() or None base_url = str(body.get("base_url") or "").strip() or None + session_id = str(body.get("session_id") or "").strip() + if session_id: + raw = str(effort or "").strip().lower() + if raw and raw != "none" and raw not in api_config.VALID_REASONING_EFFORTS: + raise ValueError(f"Unknown reasoning effort '{effort}'.") + session = _get_or_materialize_session(session_id) + with _get_session_agent_lock(session_id): + session.reasoning_effort = raw + session.save() + api_config._evict_session_agent(session_id) + return j(handler, get_reasoning_status( + model_id=model_id, + provider_id=provider_id, + base_url=base_url, + reasoning_effort=raw, + )) return j( handler, set_reasoning_effort( @@ -16271,6 +16295,10 @@ def _draft_mark(name): enabled_toolsets=getattr(source, "enabled_toolsets", None), context_length=getattr(source, "context_length", None), threshold_tokens=getattr(source, "threshold_tokens", None), + # Reasoning effort is a per-session override (#6809). A branch that + # drops it reads None and silently falls back to the profile-global + # default, so a fork of an xhigh session runs at the profile value. + reasoning_effort=getattr(source, "reasoning_effort", None), # context_messages — truncated to fork prefix (not full parent copy) context_messages=copy.deepcopy(forked_context), # Gateway routing — inherit from source @@ -23771,6 +23799,11 @@ def _handle_session_compression_recovery_start(handler, body): enabled_toolsets=copy.deepcopy(getattr(source, "enabled_toolsets", None)), context_length=getattr(source, "context_length", None), threshold_tokens=getattr(source, "threshold_tokens", None), + # Reasoning effort is a per-session override (#6809). The + # focused continuation keeps the source lane's model settings, + # so it must keep the effort too; dropping it reads None and + # silently falls back to the profile-global default. + reasoning_effort=getattr(source, "reasoning_effort", None), gateway_routing=copy.deepcopy(getattr(source, "gateway_routing", None)), gateway_routing_history=copy.deepcopy(getattr(source, "gateway_routing_history", None) or []), parent_session_id=getattr(source, "session_id", sid), diff --git a/api/streaming.py b/api/streaming.py index 4431d4db164..1d94a680e11 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -10203,7 +10203,12 @@ def _fallback_entries(_raw): # the key is absent or invalid, pass None → agent uses its default. try: _effort_cfg = _cfg.get('agent', {}) if isinstance(_cfg, dict) else {} - _effort_raw = _effort_cfg.get('reasoning_effort') if isinstance(_effort_cfg, dict) else None + _session_effort = getattr(_session_meta, 'reasoning_effort', None) if _session_meta else None + _effort_raw = ( + _session_effort + if _session_effort is not None + else _effort_cfg.get('reasoning_effort') if isinstance(_effort_cfg, dict) else None + ) _effort = coerce_reasoning_effort_for_model( _effort_raw, resolved_model, diff --git a/static/commands.js b/static/commands.js index e322c1d887b..b030ec05de0 100644 --- a/static/commands.js +++ b/static/commands.js @@ -1903,17 +1903,100 @@ function cmdReasoning(args){ return true; } if(EFFORTS.includes(arg)){ - // Persist via /api/reasoning → config.yaml agent.reasoning_effort. - // Takes effect on the NEXT session/turn (agent re-reads config at - // construction time), matching CLI semantics where `/reasoning high` - // also forces an agent re-init. - api('/api/reasoning',{method:'POST',body:JSON.stringify({effort:arg})}) + // Scope the write to the active session, exactly like the composer chip. + // Before this, /reasoning POSTed a bare {effort} and hit the + // profile-global default, so a session holding a persisted override kept + // its old effort while this toast claimed the new value applied. + // _reasoningEffortContext() adds session_id only when a session is active; + // with no session there is no override to scope to and the global write is + // the correct behaviour (it is what /reasoning does before the first chat). + // + // These four ui.js symbols are MANDATORY, called directly with no `typeof` + // fallback. index.html loads ui.js (1775) before commands.js (1779), both + // `defer`, so ui.js has run to completion before this handler can exist. A + // fallback here therefore never covers a legitimate load order — it only + // covers dependency failure or bundle skew, and in both of those cases the + // old fallbacks FAILED OPEN into exactly the two defects this change set + // exists to close: a missing context helper POSTed a bare {effort} and + // mutated the PROFILE-GLOBAL default, and a missing sequence or predicate + // helper applied a superseded chip write and toast. Fail closed instead. + // + // A missing symbol now raises a ReferenceError. We catch it here rather + // than let it escape: messages.js:1494 invokes this handler with no + // try/catch, so an escaping throw would abort send() before it clears the + // composer and hides the command dropdown, and send() is async and called + // unawaited (ui.js:8418) so the throw would surface only as an unhandled + // rejection. Report it as a toast and mutate nothing. + // + // RESOLVE AND VALIDATE EVERY DEPENDENCY BEFORE MUTATING ANYTHING. + // `_reasoningFetchSeq` is a SHARED dispatch generation: fetchReasoningChip() + // and syncReasoningChip() in ui.js compare their captured sequence against + // it. Advancing it and then failing supersedes a cold in-flight chip fetch + // that captured the old value. That fetch then returns early at its + // stale-generation check while `_lastReasoningFetchKey` stays set, so a + // same-key syncReasoningChip() short-circuits instead of retrying and chip + // hydration is stranded until something else invalidates the key. Merely + // invoking an unavailable command must not be able to do that. + // + // So: read the context and the query, require a CALLABLE predicate and a + // FINITE SAFE-INTEGER counter, and only then increment and bind. A + // present-but-undefined or non-callable helper is as unusable as an absent + // one — assignment alone succeeding is not evidence the helper works, and + // the old code discovered that only inside .then(), after the POST had + // already changed server state. A counter holding `undefined` is worse than + // absent: prefix increment yields NaN, which throws nothing and makes every + // later generation comparison false. + let payload,key,seq,current; + try{ + const ctx=_reasoningEffortContext(); + payload=Object.assign({effort:arg},ctx); + // Same stale-context guard as the chip POST: a request dispatched from + // session A can resolve after the user switches to session B, and applying + // it there would poison B's chip and cache. Snapshot the dispatch key and + // sequence number now, then discard a superseded response silently — no + // chip write, no toast. + key=_reasoningEffortQuery(); + // Bind the predicate EAGERLY rather than referencing it inside current(). + // A lazy reference resolves only when the response settles, which is + // AFTER the POST has already gone out: the ReferenceError would then fire + // inside .then() as an unhandled rejection, having already mutated + // server state. Capturing it here moves the failure ahead of dispatch, so + // an unavailable predicate sends nothing at all. + const isCurrent=_reasoningDispatchIsCurrent; + if(typeof isCurrent!=='function'){ + throw new TypeError('_reasoningDispatchIsCurrent is not callable'); + } + // Read the counter BEFORE writing it, so an unusable value fails closed + // with the generation untouched. + const prevSeq=_reasoningFetchSeq; + if(!Number.isSafeInteger(prevSeq)){ + throw new TypeError('_reasoningFetchSeq is not a safe integer'); + } + // Every dependency is now validated: this is the first mutation. + seq=++_reasoningFetchSeq; + current=function(){return isCurrent(seq,key);}; + }catch(e){ + // Fail closed: no /api/reasoning POST, no chip write, and no toast that + // claims an effort was saved. The message is a literal, matching the + // status branch's '/reasoning — status unavailable' above: a handler + // whose purpose is surviving a missing dependency must not itself depend + // on t() from i18n.js. + showToast(BRAIN+' /reasoning '+arg+' \u2014 unavailable: ' + +(e&&e.message?e.message:'reasoning helpers missing')); + return true; + } + // Takes effect on the NEXT turn (the agent re-reads its effort at + // construction time), matching CLI semantics where `/reasoning high` also + // forces an agent re-init. + api('/api/reasoning',{method:'POST',body:JSON.stringify(payload)}) .then(function(st){ + if(!current()) return; const eff=(st && st.reasoning_effort)||arg; showToast(BRAIN+' Reasoning effort: '+eff+' (saved; applies to next turn)'); if(typeof _applyReasoningChip==='function') _applyReasoningChip(eff, st||{}); }) .catch(function(e){ + if(!current()) return; showToast(BRAIN+' Failed to set effort: '+(e && e.message ? e.message : arg)); }); return true; diff --git a/static/ui.js b/static/ui.js index 56af87daeaf..03f27fa2c03 100644 --- a/static/ui.js +++ b/static/ui.js @@ -5167,6 +5167,7 @@ function _reasoningEffortContext(){ provider=_modelStateForSelect(sel, model).model_provider||''; } const ctx={}; + if(S&&S.session&&S.session.session_id) ctx.session_id=S.session.session_id; if(model) ctx.model=model; if(provider) ctx.provider=provider; return ctx; @@ -5263,14 +5264,26 @@ function _applyReasoningChip(eff){ // topbar syncs can serve the cached chip state instead of re-hitting the // network. null = never fetched. let _lastReasoningFetchKey=null; -// Monotonic dispatch counter. Each fetchReasoningChip() increments it and the -// async handlers capture their own value; a response (success OR failure) only -// applies if it is still the most recent dispatch. This defeats out-of-order -// resolution even when two fetches share the same model/provider key (e.g. a -// profile switch that resets the cache and refetches the same default model but -// a different agent.reasoning_effort) — #4650 review. +// Monotonic dispatch counter. Every reasoning request — the fetchReasoningChip() +// GET and the chip-selection POST alike — increments it and the async handlers +// capture their own value; a response (success OR failure) only applies if it is +// still the most recent dispatch. This defeats out-of-order resolution even when +// two dispatches share the same model/provider key (e.g. a profile switch that +// resets the cache and refetches the same default model but a different +// agent.reasoning_effort) — #4650 review. let _reasoningFetchSeq=0; +// True when the dispatch identified by (seq, key) is still the one the UI is +// waiting on. Both halves are load-bearing: the sequence number rejects a +// dispatch that a newer one superseded for the SAME session, and the key +// comparison rejects a response that lands after the active session, model, or +// provider changed. A stale response must be discarded silently — applying it +// would write one session's effort onto another session's chip, which is the +// session-confusion class the per-session storage fix removes. +function _reasoningDispatchIsCurrent(seq, key){ + return seq===_reasoningFetchSeq && key===_reasoningEffortQuery(); +} + function fetchReasoningChip(keyOverride){ // Set the cache key OPTIMISTICALLY before the request so rapid routine syncs // while this GET is in flight short-circuit instead of re-dispatching (that @@ -5401,15 +5414,28 @@ document.addEventListener('click',function(e){ // (#6219 round-3) if(opt){ const payload=Object.assign({effort:effort},_reasoningEffortContext()); + // Snapshot the dispatch identity BEFORE the request. A POST dispatched + // from session A can resolve AFTER the user switches to session B; without + // this guard the late response writes A's effort onto B's chip and poisons + // B's cache — the same session-confusion the per-session storage fix + // removes, on the async-completion path. Discard a stale response + // silently: no chip write, no cache write, and no toast, because a toast + // for a session the user already left is itself misinformation. + const key=_reasoningEffortQuery(); + const seq=++_reasoningFetchSeq; api('/api/reasoning',{method:'POST',body:JSON.stringify(payload)}) .then(function(st){ + if(!_reasoningDispatchIsCurrent(seq,key)) return; // For Default (effort=''), the returned reasoning_effort is '' (clear) // — display 'Default' rather than an empty toast. const display=(st&&st.reasoning_effort)||effort||'Default'; _applyReasoningChip((st&&st.reasoning_effort)||effort, st||{}); showToast('🧠 Reasoning effort set to '+display); }) - .catch(function(){showToast('🧠 Failed to set effort');}); + .catch(function(){ + if(!_reasoningDispatchIsCurrent(seq,key)) return; + showToast('🧠 Failed to set effort'); + }); closeReasoningDropdown(); } } diff --git a/tests/test_reasoning_effort_dependency_order.py b/tests/test_reasoning_effort_dependency_order.py new file mode 100644 index 00000000000..55070130643 --- /dev/null +++ b/tests/test_reasoning_effort_dependency_order.py @@ -0,0 +1,406 @@ +"""Ordering and unusable-dependency coverage for ``/reasoning `` (#6809 round 5). + +Blocker from the 3 September re-gate at ``d8a9c33c`` + The effort branch resolved its ownership dependencies in an order that + mutated shared state before it could fail:: + + key = _reasoningEffortQuery(); + seq = ++_reasoningFetchSeq; # <-- mutates FIRST + const isCurrent = _reasoningDispatchIsCurrent; # <-- may throw AFTER + + ``_reasoningFetchSeq`` is a SHARED dispatch generation. ``fetchReasoningChip()`` + and ``syncReasoningChip()`` in ``ui.js`` compare their captured sequence + against it. Advancing it and then failing supersedes a cold in-flight chip + fetch that captured the old value. That fetch returns early at its + stale-generation check while ``_lastReasoningFetchKey`` stays set, so a + same-key ``syncReasoningChip()`` short-circuits instead of retrying, and chip + hydration is stranded. Merely invoking an unavailable command caused that. + + Two related shapes were also unguarded. A ``_reasoningDispatchIsCurrent`` + that EXISTS but is ``undefined`` or non-callable let the assignment succeed, + so the POST went out and only the response callback threw, after server + state changed. A ``_reasoningFetchSeq`` holding ``undefined`` produced ``NaN`` + from the prefix increment, which throws nothing and makes every later + generation comparison false. + +What the sibling module already covers, and what it missed + ``tests/test_reasoning_effort_slash_command_fail_closed.py`` omits each + helper entirely, which only exercises the UNDECLARED shape. It never reports + or asserts the counter, and it assigns ``_pendingReject`` without ever + invoking it. Its 25 passing cases therefore missed the mutation-order and + rejection shapes above. + +Method + Every test drives the REAL effort branch, sliced verbatim out of + ``static/commands.js``, under node. Dependencies are installed as genuine + globals so ``++_reasoningFetchSeq`` performs a real global write the harness + can observe. The counter is reported on every run, and asserted. +""" +import json +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +UI_JS = ROOT.joinpath("static", "ui.js").read_text(encoding="utf-8") +COMMANDS_JS = ROOT.joinpath("static", "commands.js").read_text(encoding="utf-8") + +NODE_TIMEOUT = 30 +START_SEQ = 7 + + +def _balanced_block(src: str, start: int) -> str: + brace = src.index("{", start) + depth = 1 + i = brace + 1 + while depth and i < len(src): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + i += 1 + assert depth == 0, "unbalanced braces while slicing block" + return src[start:i] + + +def _function_source(src: str, name: str) -> str: + return _balanced_block(src, src.index(f"function {name}(")) + + +def _effort_block() -> str: + body = _function_source(COMMANDS_JS, "cmdReasoning") + return _balanced_block(body, body.index("if(EFFORTS.includes(arg)){")) + + +def _run_node(script: str) -> dict: + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + proc = subprocess.run( + [node, "-e", script], capture_output=True, text=True, timeout=NODE_TIMEOUT + ) + assert proc.returncode == 0, f"node harness failed:\n{proc.stderr}" + return json.loads(proc.stdout.strip()) + + +def _dispatch( + *, + predicate: str = "real", + counter: str = "int", + settle: str = "resolve", + cold_fetch: bool = False, +) -> dict: + """Drive the real effort branch and report every observable effect. + + ``predicate``: ``real`` | ``undefined`` | ``noncallable`` | ``absent`` + ``counter``: ``int`` | ``undefined`` | ``nan`` | ``absent`` | ``float`` + ``settle``: ``resolve`` | ``reject`` | ``none`` + ``cold_fetch``: also start a cold chip fetch that captured the PRIOR + generation, then report whether it was superseded. + """ + if predicate == "real": + pred_src = _function_source(UI_JS, "_reasoningDispatchIsCurrent") + elif predicate == "undefined": + pred_src = "var _reasoningDispatchIsCurrent = undefined;" + elif predicate == "noncallable": + pred_src = "var _reasoningDispatchIsCurrent = 42;" + elif predicate == "absent": + pred_src = "" + else: # pragma: no cover + raise AssertionError(predicate) + + counter_src = { + "int": f"var _reasoningFetchSeq = {START_SEQ};", + "undefined": "var _reasoningFetchSeq = undefined;", + "nan": "var _reasoningFetchSeq = NaN;", + "float": "var _reasoningFetchSeq = 1.5;", + "absent": "", + }[counter] + + script = textwrap.dedent( + """ + const calls = []; + const toasts = []; + const chipWrites = []; + + // Minimal ui.js environment the real helpers read. + let _profileTransitionReasoningContext = null; + const S = { session: { session_id: 'A' }, activeProfile: 'default' }; + const $ = () => null; + const _modelStateForSelect = () => ({}); + let _lastReasoningFetchKey = null; + + %(counter)s + %(predicate)s + function _reasoningEffortContext(){ return { session_id: 'A' }; } + function _reasoningEffortQuery(){ return '?session_id=A'; } + + let _pendingResolve = null; + let _pendingReject = null; + function api(path, opts) { + calls.push({ path, body: opts && opts.body ? JSON.parse(opts.body) : null }); + return new Promise((res, rej) => { _pendingResolve = res; _pendingReject = rej; }); + } + function showToast(msg) { toasts.push(String(msg)); } + function _applyReasoningChip(eff, meta) { chipWrites.push(eff); } + + // A cold chip fetch that captured the generation BEFORE the command ran. + // If the command advances the generation and then fails, this fetch is + // wrongly superseded and chip hydration strands. + let coldSeq = null, coldSuperseded = null; + if (%(cold)s) { + coldSeq = (typeof _reasoningFetchSeq === 'number') ? _reasoningFetchSeq : null; + _lastReasoningFetchKey = '?session_id=A'; + } + + const BRAIN = '\\uD83E\\uDDE0'; + const arg = 'high'; + const EFFORTS = ['none','minimal','low','medium','high','xhigh','max']; + + let threw = null; + try { + // The REAL cmdReasoning() effort branch, verbatim. + (function () { + %(block)s + })(); + } catch (e) { + threw = (e && e.name) || 'Error'; + } + + const seqAfter = (typeof _reasoningFetchSeq === 'undefined') + ? '' + : (Number.isNaN(_reasoningFetchSeq) ? 'NaN' : String(_reasoningFetchSeq)); + + if (coldSeq !== null) { + // The cold fetch is superseded when the live generation moved past the + // value it captured. + coldSuperseded = (typeof _reasoningFetchSeq === 'number') + && !Number.isNaN(_reasoningFetchSeq) + && _reasoningFetchSeq !== coldSeq; + } + + let settleThrew = null; + try { + if ('%(settle)s' === 'resolve' && _pendingResolve) { + _pendingResolve({ reasoning_effort: 'high' }); + } else if ('%(settle)s' === 'reject' && _pendingReject) { + _pendingReject(new Error('network down')); + } + } catch (e) { settleThrew = (e && e.name) || 'Error'; } + + setTimeout(() => { + console.log(JSON.stringify({ + calls, toasts, chipWrites, threw, seqAfter, + coldSeq, coldSuperseded, settleThrew, + lastKey: _lastReasoningFetchKey, + })); + }, 0); + """ + ) % { + "counter": counter_src, + "predicate": pred_src, + "block": _effort_block(), + "settle": settle, + "cold": "true" if cold_fetch else "false", + } + return _run_node(script) + + +# ── The harness itself must be trustworthy ─────────────────────────────────── + + +def test_control_advances_the_counter_exactly_once(): + """Positive control, and proof the harness can OBSERVE the counter. + + An earlier version of this harness passed dependencies as function + parameters. Parameters are local bindings, so ``++_reasoningFetchSeq`` + mutated a local and the harness never saw the write. Every counter reading + was meaningless. This test fails if that regresses. + """ + out = _dispatch() + assert out["threw"] is None, out + assert out["seqAfter"] == str(START_SEQ + 1), ( + "the harness must observe the real global increment; " + f"expected {START_SEQ + 1}, got {out['seqAfter']}" + ) + assert len(out["calls"]) == 1, out["calls"] + assert out["chipWrites"] == ["high"] + + +# ── Claim 1: no dependency failure may advance the generation ──────────────── + +_UNUSABLE_PREDICATES = ["absent", "undefined", "noncallable"] + + +@pytest.mark.parametrize("predicate", _UNUSABLE_PREDICATES) +def test_unusable_predicate_does_not_advance_the_generation(predicate): + """The shared dispatch generation must be untouched when the branch fails. + + This is the maintainer's exact finding. Advancing it and then failing + supersedes a cold in-flight chip fetch, which then returns early while + ``_lastReasoningFetchKey`` stays set, so a same-key ``syncReasoningChip()`` + short-circuits and chip hydration strands. + """ + out = _dispatch(predicate=predicate) + assert out["seqAfter"] == str(START_SEQ), ( + f"with a {predicate} predicate the branch advanced the shared generation " + f"{START_SEQ} -> {out['seqAfter']}. A cold in-flight chip fetch that " + "captured the old value is now wrongly superseded." + ) + + +@pytest.mark.parametrize("predicate", _UNUSABLE_PREDICATES) +def test_unusable_predicate_publishes_nothing(predicate): + """No API mutation, no chip write, and no toast claiming a saved effort.""" + out = _dispatch(predicate=predicate) + assert out["calls"] == [], ( + f"a {predicate} predicate still POSTed {out['calls']!r}. Assignment " + "succeeding is not evidence the helper works: the old code discovered " + "that inside .then(), after the request had already changed server state." + ) + assert out["chipWrites"] == [], out["chipWrites"] + liars = [t for t in out["toasts"] if "saved" in t or "Reasoning effort:" in t] + assert liars == [], liars + + +@pytest.mark.parametrize("predicate", _UNUSABLE_PREDICATES) +def test_unusable_predicate_reports_instead_of_throwing(predicate): + out = _dispatch(predicate=predicate) + assert out["threw"] is None, ( + f"a {predicate} predicate threw {out['threw']} out of the handler, which " + "aborts send() before it clears the composer" + ) + assert any("unavailable" in t for t in out["toasts"]), out["toasts"] + + +# ── Claim 3: an unusable counter must not dispatch ─────────────────────────── + + +@pytest.mark.parametrize("counter", ["undefined", "nan", "absent", "float"]) +def test_unusable_counter_publishes_nothing(counter): + """``undefined`` and ``NaN`` never throw on prefix increment. + + ``++undefined`` yields ``NaN``, so the old code dispatched with a generation + that compares false forever. ``float`` is rejected for the same reason: a + non-integer generation cannot be compared for equality reliably. + """ + out = _dispatch(counter=counter) + assert out["calls"] == [], ( + f"a {counter} counter still POSTed {out['calls']!r} with seq=" + f"{out['seqAfter']}" + ) + assert out["chipWrites"] == [], out["chipWrites"] + assert out["threw"] is None, out["threw"] + assert any("unavailable" in t for t in out["toasts"]), out["toasts"] + + +def test_undefined_counter_is_not_silently_turned_into_nan(): + """Pin the NaN shape explicitly: it must fail closed, not dispatch.""" + out = _dispatch(counter="undefined") + assert out["seqAfter"] != "NaN", ( + "the branch incremented an undefined counter into NaN. Prefix increment " + "throws nothing here, so every later generation comparison would be " + "false and no response would ever be applied." + ) + assert out["calls"] == [] + + +# ── Claim 1, end to end: the cold-fetch interleaving ──────────────────────── + + +def test_failed_command_does_not_supersede_a_cold_chip_fetch(): + """Production-shaped interleaving, which is the point of the whole ordering fix. + + A cold ``fetchReasoningChip()`` captures generation N. The user then invokes + ``/reasoning high`` while the predicate is unavailable. If the command + advances the generation to N+1 before failing, the cold fetch's response is + discarded at its stale-generation check while ``_lastReasoningFetchKey`` + remains set, so a same-key ``syncReasoningChip()`` short-circuits and the + chip never hydrates. + """ + out = _dispatch(predicate="absent", cold_fetch=True) + assert out["coldSeq"] == START_SEQ, out + assert out["coldSuperseded"] is False, ( + "invoking an unavailable /reasoning command superseded a cold in-flight " + f"chip fetch (generation {out['coldSeq']} -> {out['seqAfter']}). That " + "strands chip hydration for a command that changed nothing." + ) + assert out["calls"] == [] + + +def test_successful_command_does_supersede_a_cold_chip_fetch(): + """Discriminating control: a SUCCESSFUL command must still supersede. + + Without this, the assertion above passes trivially for any implementation + that never advances the generation at all. + """ + out = _dispatch(cold_fetch=True) + assert out["coldSeq"] == START_SEQ + assert out["coldSuperseded"] is True, ( + "a successful command must advance the generation so a cold fetch's " + "late response cannot overwrite the fresh value" + ) + assert len(out["calls"]) == 1 + + +# ── The rejection path, which the sibling module never invoked ─────────────── + + +def test_rejected_request_reports_failure_and_writes_no_chip(): + """``_pendingReject`` is now actually invoked. + + The sibling module assigns ``_pendingReject`` and never calls it, so the + ``.catch()`` arm of the dispatch was untested. A rejection must report the + failure and must not write the chip. + """ + out = _dispatch(settle="reject") + assert out["settleThrew"] is None, out["settleThrew"] + assert len(out["calls"]) == 1, out["calls"] + assert out["chipWrites"] == [], ( + "a failed request must not write the chip; " + f"got {out['chipWrites']!r}" + ) + assert any("Failed to set effort" in t for t in out["toasts"]), out["toasts"] + assert not [t for t in out["toasts"] if "saved" in t], out["toasts"] + + +def test_unsettled_request_writes_nothing_yet(): + """Control: with the promise left pending, neither arm has run.""" + out = _dispatch(settle="none") + assert len(out["calls"]) == 1 + assert out["chipWrites"] == [] + assert not [t for t in out["toasts"] if "saved" in t] + + +# ── Source contract: the ordering must not regress ─────────────────────────── + + +def test_every_dependency_is_validated_before_the_counter_mutates(): + """Pin the ORDER in source: the increment is the last thing to happen. + + A future edit that moves ``++_reasoningFetchSeq`` back above the predicate + check reintroduces the exact defect, and every behavioural test above would + still pass for the UNDECLARED shape because that one throws earlier. + """ + block = _effort_block() + inc = block.index("++_reasoningFetchSeq") + pred_check = block.index("typeof isCurrent!=='function'") + counter_check = block.index("Number.isSafeInteger") + assert pred_check < inc, ( + "the predicate must be validated BEFORE the shared generation is " + "incremented" + ) + assert counter_check < inc, ( + "the counter must be validated BEFORE it is incremented" + ) + + +def test_counter_is_read_before_it_is_written(): + """The validation must read the PRIOR value, not the incremented one.""" + block = _effort_block() + read = block.index("const prevSeq=_reasoningFetchSeq") + inc = block.index("++_reasoningFetchSeq") + assert read < inc, "the counter must be read before it is incremented" diff --git a/tests/test_reasoning_effort_dispatch_race.py b/tests/test_reasoning_effort_dispatch_race.py new file mode 100644 index 00000000000..d0e92893eba --- /dev/null +++ b/tests/test_reasoning_effort_dispatch_race.py @@ -0,0 +1,238 @@ +"""Regression coverage for the reasoning-effort session-switch race (#6809 review). + +Blocker 1 (chip POST, ``static/ui.js``) + The reasoning-option click handler POSTs ``/api/reasoning`` with the active + session in the payload. The request is asynchronous, so a POST dispatched + from session A can resolve AFTER the user switches to session B. Without a + staleness guard the late response writes A's effort onto B's chip and + poisons B's cached ``_currentReasoningEffort`` — the same session-confusion + class the per-session storage fix removes, on the async-completion path. + +The fix: snapshot the dispatch key, take a sequence number, and apply the result +(chip, cache, toast) only when ``_reasoningFetchSeq`` AND +``_reasoningEffortQuery()`` still match at completion time. + +These tests drive the REAL production block — extracted verbatim from +``static/ui.js`` and executed under node — rather than re-implementing the guard +and asserting on the copy. +""" +import json +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +UI_JS = ROOT.joinpath("static", "ui.js").read_text(encoding="utf-8") + +NODE_TIMEOUT = 30 + + +def _balanced_block(src: str, start: int) -> str: + """Return src[start:] up to and including the close of its first ``{`` block.""" + brace = src.index("{", start) + depth = 1 + i = brace + 1 + while depth and i < len(src): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + i += 1 + assert depth == 0, "unbalanced braces while slicing block" + return src[start:i] + + +def _function_source(src: str, name: str) -> str: + marker = f"function {name}(" + start = src.index(marker) + return _balanced_block(src, start) + + +def _reasoning_option_click_block() -> str: + """Slice the real ``if(opt){ ... }`` POST block out of the click listener.""" + anchor = UI_JS.index("const payload=Object.assign({effort:effort},_reasoningEffortContext());") + start = UI_JS.rindex("if(opt){", 0, anchor) + return _balanced_block(UI_JS, start) + + +# The shared preamble: REAL _reasoningEffortContext / _reasoningEffortQuery / +# _reasoningDispatchIsCurrent from static/ui.js, plus the minimum DOM and app +# state the extracted blocks touch. A deferred `api()` lets each scenario choose +# whether the session changes before or after the response resolves. +_PREAMBLE = """ +const calls = []; +const toasts = []; +const chipWrites = []; + +let _profileTransitionReasoningContext = null; +const S = { session: { session_id: 'A' }, activeProfile: 'default' }; +const $ = () => null; // no modelSelect in this harness +const _modelStateForSelect = () => ({}); + +%(context_fn)s +%(query_fn)s +let _reasoningFetchSeq = 0; +%(guard_fn)s + +let _pendingResolve = null; +let _pendingReject = null; +function api(path, opts) { + calls.push({ path, body: opts && opts.body ? JSON.parse(opts.body) : null }); + return new Promise((res, rej) => { _pendingResolve = res; _pendingReject = rej; }); +} +function showToast(msg) { toasts.push(msg); } +function _applyReasoningChip(eff, meta) { chipWrites.push(eff); } +function closeReasoningDropdown() {} +function removeThinking() {} +function renderMessages() {} +""" + + +def _preamble() -> str: + return _PREAMBLE % { + "context_fn": _function_source(UI_JS, "_reasoningEffortContext"), + "query_fn": _function_source(UI_JS, "_reasoningEffortQuery"), + "guard_fn": _function_source(UI_JS, "_reasoningDispatchIsCurrent"), + } + + +def _run_node(script: str) -> dict: + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + proc = subprocess.run( + [node, "-e", script], capture_output=True, text=True, timeout=NODE_TIMEOUT + ) + assert proc.returncode == 0, f"node harness failed:\n{proc.stderr}" + return json.loads(proc.stdout.strip()) + + +def _run_chip_post(*, switch_to=None, fail=False) -> dict: + """Dispatch the REAL chip-POST block, optionally switching session mid-flight.""" + script = textwrap.dedent( + """ + %(preamble)s + const opt = { dataset: { effort: 'high' } }; + const effort = opt.dataset.effort; + + // Run the REAL click-handler POST block verbatim. + (function () { + %(block)s + })(); + + const switchTo = %(switch_to)s; + if (switchTo) S.session = { session_id: switchTo }; + + %(settle)s + + setTimeout(() => { + console.log(JSON.stringify({ calls, toasts, chipWrites, seq: _reasoningFetchSeq })); + }, 0); + """ + ) % { + "preamble": _preamble(), + "block": _reasoning_option_click_block(), + "switch_to": json.dumps(switch_to), + "settle": ( + "_pendingReject(new Error('boom'));" + if fail + else "_pendingResolve({ reasoning_effort: 'high' });" + ), + } + return _run_node(script) + + +# --------------------------------------------------------------------------- +# Blocker 1 — chip POST +# --------------------------------------------------------------------------- + + +def test_chip_post_sends_the_active_session(): + """Baseline: the POST body carries the session so the write is session-scoped.""" + out = _run_chip_post() + assert len(out["calls"]) == 1 + assert out["calls"][0]["path"] == "/api/reasoning" + assert out["calls"][0]["body"]["session_id"] == "A" + assert out["calls"][0]["body"]["effort"] == "high" + + +def test_chip_post_applies_when_session_is_unchanged(): + """Control: with no session switch the response must apply normally.""" + out = _run_chip_post() + assert out["chipWrites"] == ["high"], "a current response must still update the chip" + assert any("Reasoning effort set to high" in t for t in out["toasts"]), out["toasts"] + + +def test_chip_post_discards_response_after_session_switch(): + """A POST from session A must not write session B's chip when it lands late.""" + out = _run_chip_post(switch_to="B") + assert out["chipWrites"] == [], ( + "a reasoning POST dispatched from session A resolved after the user " + "switched to session B and wrote B's chip — session-switch race (#6809 " + "review blocker 1)" + ) + assert out["toasts"] == [], ( + "a stale reasoning response must be discarded SILENTLY; a toast for a " + "session the user already left is itself misinformation" + ) + + +def test_chip_post_discards_failure_toast_after_session_switch(): + """The staleness guard covers the rejection path too, not just success.""" + out = _run_chip_post(switch_to="B", fail=True) + assert out["toasts"] == [], ( + "a stale reasoning POST failure must not raise a toast on the session " + "the user switched to" + ) + + +def test_chip_post_takes_a_sequence_number_before_dispatch(): + """The counter must advance on dispatch so a superseded POST is rejected.""" + out = _run_chip_post() + assert out["seq"] == 1, ( + "the chip POST must increment _reasoningFetchSeq before the request so a " + "later dispatch supersedes it even when the session key is identical" + ) + + +def test_chip_post_stale_by_sequence_alone_is_discarded(): + """Same session, two dispatches: only the newest may apply. + + The key comparison cannot catch this — both dispatches share one key — so + this is the case that proves the sequence half of the guard is load-bearing. + """ + script = textwrap.dedent( + """ + %(preamble)s + const opt = { dataset: { effort: 'high' } }; + const effort = opt.dataset.effort; + + // First dispatch (the one that will be superseded). + (function () { + %(block)s + })(); + const firstResolve = _pendingResolve; + + // Second dispatch for the SAME session — same _reasoningEffortQuery() key. + (function () { + %(block)s + })(); + + // The stale first response lands last. + firstResolve({ reasoning_effort: 'low' }); + + setTimeout(() => { + console.log(JSON.stringify({ calls, toasts, chipWrites, seq: _reasoningFetchSeq })); + }, 0); + """ + ) % {"preamble": _preamble(), "block": _reasoning_option_click_block()} + out = _run_node(script) + assert out["seq"] == 2 + assert out["chipWrites"] == [], ( + "the superseded first POST wrote the chip with its stale 'low' value; " + "only the most recent dispatch may apply" + ) + assert out["toasts"] == [] diff --git a/tests/test_reasoning_effort_session_copy_inheritance.py b/tests/test_reasoning_effort_session_copy_inheritance.py new file mode 100644 index 00000000000..53cc9ba257a --- /dev/null +++ b/tests/test_reasoning_effort_session_copy_inheritance.py @@ -0,0 +1,315 @@ +"""Reasoning-effort inheritance across the three session-copy paths (#6809 review). + +Blocker 3 (``api/routes.py``) + ``reasoning_effort`` is a per-session override. Three constructors build a + child session from a source session and none of them carried the field, so + the child read ``None`` and silently fell back to the profile-global default: + + - ``POST /api/session/duplicate`` (duplicate) + - ``POST /api/session/branch`` (fork / branch) + - ``POST /api/session/compression-recovery/start`` (focused continuation) + + A source session set to ``xhigh`` therefore produced a child running at + whatever the profile said. The user sees a copy of their session that thinks + at a different level, with no indication anything changed. + +Every test here sets the profile-global default to a value DIFFERENT from the +source session's override. That is the whole point: if the global and the +override were the same value, a silent fall-back to the global would still read +as a pass and the test would prove nothing. The assertions check the child's +persisted value AND the effort that ``get_reasoning_status()`` resolves for it. +""" +import io +import json +from urllib.parse import urlparse + +import pytest + +from api import config as api_config +from api import models, routes +from api.compression_recovery import stamp_compression_exhausted_recovery +from api.models import Session + + +# The source session's override and the profile-global default must never be +# equal, or a silent fall-back to the global passes the test. +SOURCE_EFFORT = "xhigh" +PROFILE_GLOBAL_EFFORT = "low" +assert SOURCE_EFFORT != PROFILE_GLOBAL_EFFORT + + +class _FakeHandler: + def __init__(self, path="/api/session/duplicate"): + self.status = None + self.headers = {"Content-Type": "application/json", "Content-Length": "1"} + self.rfile = io.BytesIO(b"") + self.wfile = io.BytesIO() + self.command = "POST" + self.path = path + self.client_address = ("127.0.0.1", 12345) + + def send_response(self, status): + self.status = status + + def send_header(self, key, value): + self.headers[key] = value + + def end_headers(self): + pass + + +def _capture_route(monkeypatch): + cap = {} + + def _bad(_handler, msg, code=400, **_kwargs): + cap["bad"] = (msg, code) + return True + + def _j(_handler, obj, *_args, **kwargs): + cap["ok"] = obj + cap["status"] = kwargs.get("status", 200) + return True + + monkeypatch.setattr(routes, "bad", _bad) + monkeypatch.setattr(routes, "j", _j) + return cap + + +@pytest.fixture +def isolated_sessions(monkeypatch, tmp_path): + """Sessions on a scratch dir, with a profile-global default that is NOT the override.""" + session_dir = tmp_path / "sessions" + session_dir.mkdir() + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", session_dir / "_index.json") + models.SESSIONS.clear() + routes.SESSIONS.clear() + + # Profile-global default deliberately DIFFERENT from SOURCE_EFFORT, so a + # child that drops the override resolves to PROFILE_GLOBAL_EFFORT and the + # assertions below catch it. + monkeypatch.setattr( + api_config, + "_load_yaml_config_file", + lambda _path: {"agent": {"reasoning_effort": PROFILE_GLOBAL_EFFORT}}, + ) + monkeypatch.setattr( + api_config, "resolve_model_reasoning_efforts", lambda *_a, **_k: ["low", "high", "xhigh"] + ) + monkeypatch.setattr(api_config, "_zai_glm_thinking_toggle_supported", lambda *_a: None) + return session_dir + + +def _assert_child_keeps_the_override(child_id, session_dir): + """The child's persisted value and resolved effort must both be the override.""" + saved = json.loads((session_dir / f"{child_id}.json").read_text(encoding="utf-8")) + assert saved.get("reasoning_effort") == SOURCE_EFFORT, ( + f"the child session persisted reasoning_effort={saved.get('reasoning_effort')!r} " + f"instead of the source override {SOURCE_EFFORT!r}; it will silently fall " + f"back to the profile-global {PROFILE_GLOBAL_EFFORT!r} (#6809 review blocker 3)" + ) + resolved = api_config.get_reasoning_status( + model_id="test-model", reasoning_effort=saved.get("reasoning_effort") + )["reasoning_effort"] + assert resolved == SOURCE_EFFORT, ( + f"the child resolves to {resolved!r}, not the source override {SOURCE_EFFORT!r}" + ) + + +def _sanity_check_the_global_differs(): + """The profile-global really is the other value, so the oracle is meaningful.""" + assert ( + api_config.get_reasoning_status(model_id="test-model")["reasoning_effort"] + == PROFILE_GLOBAL_EFFORT + ) + + +# --------------------------------------------------------------------------- +# duplicate +# --------------------------------------------------------------------------- + + +def test_duplicate_carries_reasoning_effort(isolated_sessions, monkeypatch): + session_dir = isolated_sessions + _sanity_check_the_global_differs() + + source = Session( + session_id="dupsrc1", + title="Deep work", + workspace=str(session_dir), + model="gpt-4o", + model_provider="openai", + messages=[{"role": "user", "content": "think hard"}], + reasoning_effort=SOURCE_EFFORT, + ) + source.save() + + monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) + monkeypatch.setattr(routes, "read_body", lambda _handler: {"session_id": source.session_id}) + monkeypatch.setattr(routes, "_session_is_subagent_view_only", lambda _sid: False) + monkeypatch.setattr(routes, "publish_session_list_changed", lambda *_a, **_k: None) + cap = _capture_route(monkeypatch) + + routes.handle_post(_FakeHandler(), urlparse("/api/session/duplicate")) + + assert "bad" not in cap, cap.get("bad") + child_id = cap["ok"]["session"]["session_id"] + assert child_id != source.session_id + _assert_child_keeps_the_override(child_id, session_dir) + + +def test_duplicate_of_a_session_without_an_override_stays_none(isolated_sessions, monkeypatch): + """A source with no override must produce a child with no override, not a value. + + Carrying the field must not invent one. ``None`` is what keeps existing + sessions, the CLI, and cron on the profile-global fall-back. + """ + session_dir = isolated_sessions + source = Session( + session_id="dupsrc2", + title="No override", + workspace=str(session_dir), + model="gpt-4o", + messages=[{"role": "user", "content": "hi"}], + ) + source.save() + + monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) + monkeypatch.setattr(routes, "read_body", lambda _handler: {"session_id": source.session_id}) + monkeypatch.setattr(routes, "_session_is_subagent_view_only", lambda _sid: False) + monkeypatch.setattr(routes, "publish_session_list_changed", lambda *_a, **_k: None) + cap = _capture_route(monkeypatch) + + routes.handle_post(_FakeHandler(), urlparse("/api/session/duplicate")) + + child_id = cap["ok"]["session"]["session_id"] + saved = json.loads((session_dir / f"{child_id}.json").read_text(encoding="utf-8")) + assert saved.get("reasoning_effort") is None, ( + "a source with no per-session override must not gain one on duplicate" + ) + + +# --------------------------------------------------------------------------- +# branch / fork +# --------------------------------------------------------------------------- + + +def test_branch_carries_reasoning_effort(isolated_sessions, monkeypatch): + session_dir = isolated_sessions + _sanity_check_the_global_differs() + + source = Session( + session_id="brnsrc1", + title="Deep work", + workspace=str(session_dir), + model="gpt-4o", + model_provider="openai", + messages=[{"role": "user", "content": "think hard"}], + reasoning_effort=SOURCE_EFFORT, + ) + source.save() + models.SESSIONS[source.session_id] = source + routes.SESSIONS[source.session_id] = source + + monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) + monkeypatch.setattr(routes, "read_body", lambda _handler: {"session_id": source.session_id}) + monkeypatch.setattr(routes, "_session_is_subagent_view_only", lambda _sid: False) + monkeypatch.setattr(routes, "publish_session_list_changed", lambda *_a, **_k: None) + cap = _capture_route(monkeypatch) + + routes.handle_post(_FakeHandler("/api/session/branch"), urlparse("/api/session/branch")) + + assert "bad" not in cap, cap.get("bad") + child_id = cap["ok"]["session_id"] + assert cap["ok"]["parent_session_id"] == source.session_id + _assert_child_keeps_the_override(child_id, session_dir) + + +# --------------------------------------------------------------------------- +# focused continuation (compression recovery) +# --------------------------------------------------------------------------- + + +def test_focused_continuation_carries_reasoning_effort(isolated_sessions, monkeypatch): + session_dir = isolated_sessions + _sanity_check_the_global_differs() + + source = Session( + session_id="recsrc1", + title="Long task", + workspace=str(session_dir), + model="gpt-4o", + model_provider="openai", + profile="default", + messages=[{"role": "user", "content": "long task"}], + context_messages=[{"role": "user", "content": "large context"}], + reasoning_effort=SOURCE_EFFORT, + ) + stamp_compression_exhausted_recovery(source, message="Context length exceeded.") + source.save() + models.SESSIONS[source.session_id] = source + routes.SESSIONS[source.session_id] = source + + monkeypatch.setattr(routes, "publish_session_list_changed", lambda *_a, **_k: None) + cap = _capture_route(monkeypatch) + + routes._handle_session_compression_recovery_start( + _FakeHandler("/api/session/compression-recovery/start"), + {"session_id": source.session_id}, + ) + + assert "bad" not in cap, cap.get("bad") + child_id = cap["ok"]["session"]["session_id"] + assert child_id != source.session_id + _assert_child_keeps_the_override(child_id, session_dir) + + +# --------------------------------------------------------------------------- +# static backstop — all three constructors, anchored individually +# --------------------------------------------------------------------------- + + +def _constructor_block(src: str, anchor: str, var: str) -> str: + """Slice the `` = Session(...)`` call that follows ``anchor`` in src.""" + at = src.index(anchor) + start = src.index(f"{var} = Session(", at) + open_paren = src.index("(", start + len(var)) + depth = 1 + i = open_paren + 1 + while depth and i < len(src): + if src[i] == "(": + depth += 1 + elif src[i] == ")": + depth -= 1 + i += 1 + assert depth == 0, f"unbalanced parens slicing the {var} constructor" + return src[start:i] + + +def test_all_three_copy_constructors_pass_reasoning_effort(): + """Guard against a fourth copy path landing without the field. + + Each constructor is sliced individually so this cannot be satisfied by the + ``reasoning_effort=getattr(...)`` hunks in the GET and POST ``/api/reasoning`` + ENDPOINTS, which are a different fix and would otherwise make a bare + repo-wide count unfailable. This reads the source rather than the behaviour, + so it is a backstop for the three behavioural tests above, not a replacement. + """ + src = open(routes.__file__, encoding="utf-8").read() + + duplicate = _constructor_block(src, 'parsed.path == "/api/session/duplicate"', "copied_session") + assert 'reasoning_effort=getattr(session, "reasoning_effort", None)' in duplicate, ( + "the /api/session/duplicate constructor must carry reasoning_effort" + ) + + branch = _constructor_block(src, 'parsed.path == "/api/session/branch"', "branch") + assert 'reasoning_effort=getattr(source, "reasoning_effort", None)' in branch, ( + "the /api/session/branch constructor must carry reasoning_effort" + ) + + focused = _constructor_block( + src, "def _handle_session_compression_recovery_start(", "copied_session" + ) + assert 'reasoning_effort=getattr(source, "reasoning_effort", None)' in focused, ( + "the focused-continuation constructor must carry reasoning_effort" + ) diff --git a/tests/test_reasoning_effort_session_scope.py b/tests/test_reasoning_effort_session_scope.py new file mode 100644 index 00000000000..88c722cc587 --- /dev/null +++ b/tests/test_reasoning_effort_session_scope.py @@ -0,0 +1,87 @@ +import json +from pathlib import Path +from types import SimpleNamespace + +from api.models import Session + + +ROOT = Path(__file__).resolve().parents[1] + + +def test_reasoning_effort_is_session_scoped(tmp_path, monkeypatch): + import api.models as models + + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + + low = Session(session_id="reasoning-low", reasoning_effort="low") + high = Session(session_id="reasoning-high", reasoning_effort="high") + low.save() + high.save() + + assert Session(**json.loads(low.path.read_text())).reasoning_effort == "low" + assert Session(**json.loads(high.path.read_text())).reasoning_effort == "high" + + ui = (ROOT / "static" / "ui.js").read_text() + streaming = (ROOT / "api" / "streaming.py").read_text() + gateway = (ROOT / "api" / "gateway_chat.py").read_text() + + assert "ctx.session_id=S.session.session_id" in ui + assert "getattr(_session_meta, 'reasoning_effort', None)" in streaming + assert 'getattr(s, "reasoning_effort", None)' in gateway + + +def test_reasoning_status_prefers_session_override(monkeypatch): + import api.config as config + + monkeypatch.setattr( + config, + "_load_yaml_config_file", + lambda _path: {"agent": {"reasoning_effort": "high"}}, + ) + monkeypatch.setattr(config, "resolve_model_reasoning_efforts", lambda *_args, **_kwargs: ["low", "high"]) + monkeypatch.setattr(config, "_zai_glm_thinking_toggle_supported", lambda *_args: None) + + assert config.get_reasoning_status(model_id="test-model")["reasoning_effort"] == "high" + assert config.get_reasoning_status(model_id="test-model", reasoning_effort="low")["reasoning_effort"] == "low" + assert config.get_reasoning_status(model_id="test-model", reasoning_effort="")["reasoning_effort"] == "" + + +def test_reasoning_post_updates_only_target_session(tmp_path, monkeypatch): + import api.config as config + import api.models as models + import api.routes as routes + + monkeypatch.setattr(models, "SESSION_DIR", tmp_path) + low = Session(session_id="reasoning-low", reasoning_effort="low") + high = Session(session_id="reasoning-high", reasoning_effort="high") + low.save() + high.save() + + monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) + monkeypatch.setattr(routes, "_handle_extension_sidecar_proxy", lambda *_args, **_kwargs: False) + monkeypatch.setattr( + routes, + "read_body", + lambda _handler: {"session_id": low.session_id, "effort": "medium"}, + ) + monkeypatch.setattr(routes, "_get_or_materialize_session", lambda sid: low if sid == low.session_id else high) + monkeypatch.setattr(config, "_evict_session_agent", lambda _sid: None) + monkeypatch.setattr(routes, "get_reasoning_status", lambda **kwargs: kwargs) + responses = [] + monkeypatch.setattr(routes, "j", lambda _handler, payload, **_kwargs: responses.append(payload) or True) + + assert routes.handle_post(SimpleNamespace(), SimpleNamespace(path="/api/reasoning")) is True + assert low.reasoning_effort == "medium" + assert high.reasoning_effort == "high" + assert responses[-1]["reasoning_effort"] == "medium" + + +def test_gateway_prefers_session_override(monkeypatch): + import api.gateway_chat as gateway_chat + + monkeypatch.setattr(gateway_chat, "coerce_reasoning_effort_for_model", lambda effort, *_args, **_kwargs: effort) + cfg = {"agent": {"reasoning_effort": "high"}} + + assert gateway_chat._gateway_reasoning_effort_for_request(cfg) == "high" + assert gateway_chat._gateway_reasoning_effort_for_request(cfg, reasoning_effort="low") == "low" + assert gateway_chat._gateway_reasoning_effort_for_request(cfg, reasoning_effort="") is None diff --git a/tests/test_reasoning_effort_slash_command_fail_closed.py b/tests/test_reasoning_effort_slash_command_fail_closed.py new file mode 100644 index 00000000000..454dbca5c77 --- /dev/null +++ b/tests/test_reasoning_effort_slash_command_fail_closed.py @@ -0,0 +1,361 @@ +"""Fail-closed coverage for the ``/reasoning `` ownership helpers (#6809 review round 4). + +Blocker (``static/commands.js``, effort branch) + The effort branch reached its ownership helpers through ``typeof`` fallbacks:: + + const ctx=(typeof _reasoningEffortContext==='function')?_reasoningEffortContext():{}; + const key=(typeof _reasoningEffortQuery==='function')?_reasoningEffortQuery():''; + const seq=(typeof _reasoningFetchSeq==='undefined')?null:++_reasoningFetchSeq; + const current=function(){ + if(seq===null||typeof _reasoningDispatchIsCurrent!=='function') return true; + ... + }; + + Every one of those fallbacks failed OPEN into the exact defect this change + set exists to close. Without ``_reasoningEffortContext`` the command sent a + bare ``{effort}`` and mutated the PROFILE-GLOBAL default. Without the + sequence counter or the predicate, ``current()`` returned ``true`` + unconditionally and the command applied a superseded chip write and toast. + + ``static/index.html`` loads ``ui.js`` (line 1775) before ``commands.js`` + (line 1779), both ``defer``, so ``ui.js`` runs to completion before this + handler can exist. No legitimate load order needs those fallbacks. They only + covered dependency failure or bundle skew, and in both cases a higher-scope + mutation is worse than no mutation. + + The prior slash-command suite injected all three helpers in every scenario, + so all eight tests passed without ever entering a fallback. That is the gap + these tests close. + +Each test drives the REAL effort branch, sliced verbatim out of +``static/commands.js``, under node with ONE required helper omitted from the +environment. The assertion is that nothing mutates: no ``/api/reasoning`` +request, no chip write, and no toast claiming an effort was saved. +""" +import json +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +UI_JS = ROOT.joinpath("static", "ui.js").read_text(encoding="utf-8") +COMMANDS_JS = ROOT.joinpath("static", "commands.js").read_text(encoding="utf-8") +INDEX_HTML = ROOT.joinpath("static", "index.html").read_text(encoding="utf-8") + +NODE_TIMEOUT = 30 + +# The ui.js symbols the effort branch owns. Each is required; each is omitted in +# turn below. ``_reasoningFetchSeq`` is a mutable counter rather than a function, +# so it carries its own source line. +REQUIRED_HELPERS = ( + "_reasoningEffortContext", + "_reasoningEffortQuery", + "_reasoningDispatchIsCurrent", + "_reasoningFetchSeq", +) + + +def _balanced_block(src: str, start: int) -> str: + """Return src[start:] up to and including the close of its first ``{`` block.""" + brace = src.index("{", start) + depth = 1 + i = brace + 1 + while depth and i < len(src): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + i += 1 + assert depth == 0, "unbalanced braces while slicing block" + return src[start:i] + + +def _function_source(src: str, name: str) -> str: + return _balanced_block(src, src.index(f"function {name}(")) + + +def _cmd_reasoning_effort_block() -> str: + """Slice the real ``if(EFFORTS.includes(arg)){ ... }`` branch out of cmdReasoning().""" + body = _function_source(COMMANDS_JS, "cmdReasoning") + return _balanced_block(body, body.index("if(EFFORTS.includes(arg)){")) + + +def _helper_source(name: str) -> str: + if name == "_reasoningFetchSeq": + return "let _reasoningFetchSeq = 0;" + return _function_source(UI_JS, name) + + +def _preamble(omit: str | None, *, stub_query: bool = False) -> str: + """Recording harness plus the REAL ui.js helpers, minus ``omit``. + + An omitted name is simply never declared, so the production block's direct + call raises a ReferenceError exactly as a failed ``ui.js`` load would. + + ``stub_query`` swaps the real ``_reasoningEffortQuery`` for a self-contained + stub. The real one calls ``_reasoningEffortContext()`` internally + (ui.js:5177), so with the context helper omitted the throw would come from + the query line and would not prove the branch's own direct + ``_reasoningEffortContext()`` call fails closed. The stub isolates that. + """ + parts = [] + for name in REQUIRED_HELPERS: + if name == omit: + continue + if name == "_reasoningEffortQuery" and stub_query: + parts.append("function _reasoningEffortQuery(){ return '?session_id=A'; }") + else: + parts.append(_helper_source(name)) + return textwrap.dedent( + """ + const calls = []; + const toasts = []; + const chipWrites = []; + + let _profileTransitionReasoningContext = null; + const S = { session: { session_id: 'A' }, activeProfile: 'default' }; + const $ = () => null; // no modelSelect in this harness + const _modelStateForSelect = () => ({}); + + %(helpers)s + + let _pendingResolve = null; + let _pendingReject = null; + function api(path, opts) { + calls.push({ path, body: opts && opts.body ? JSON.parse(opts.body) : null }); + return new Promise((res, rej) => { _pendingResolve = res; _pendingReject = rej; }); + } + function showToast(msg) { toasts.push(String(msg)); } + function _applyReasoningChip(eff, meta) { chipWrites.push(eff); } + """ + ) % {"helpers": "\n".join(parts)} + + +def _run_node(script: str) -> dict: + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + proc = subprocess.run( + [node, "-e", script], capture_output=True, text=True, timeout=NODE_TIMEOUT + ) + assert proc.returncode == 0, f"node harness failed:\n{proc.stderr}" + return json.loads(proc.stdout.strip()) + + +def _dispatch( + *, + omit: str | None = None, + session_id: str | None = "A", + stub_query: bool = False, +) -> dict: + """Run the REAL effort branch with ``omit`` absent from the environment. + + ``threw`` records whether the branch let an exception escape the handler. + ``messages.js:1494`` calls the handler with no try/catch inside an async + ``send()`` that ``ui.js:8418`` invokes unawaited, so an escaping throw + strands the composer with no user-visible feedback. + + ``stub_query`` replaces ``_reasoningEffortQuery`` with a self-contained stub. + The real one calls ``_reasoningEffortContext()`` internally (ui.js:5177), so + omitting the context helper otherwise throws from the query line and masks + whether the branch's OWN ``_reasoningEffortContext()`` call fails closed. + """ + script = textwrap.dedent( + """ + %(preamble)s + const BRAIN = '\\uD83E\\uDDE0'; + const arg = 'high'; + const EFFORTS = ['none','minimal','low','medium','high','xhigh','max']; + S.session = %(session)s; + + let threw = null; + try { + // The REAL cmdReasoning() effort branch, verbatim. + (function () { + %(block)s + })(); + } catch (e) { + threw = (e && e.name) || 'Error'; + } + + // Resolve any in-flight request so a fail-open POST gets the chance to + // write the chip and toast. A fail-closed branch never dispatched one. + if (_pendingResolve) _pendingResolve({ reasoning_effort: 'high' }); + + setTimeout(() => { + console.log(JSON.stringify({ calls, toasts, chipWrites, threw })); + }, 0); + """ + ) % { + "preamble": _preamble(omit, stub_query=stub_query), + "block": _cmd_reasoning_effort_block(), + "session": json.dumps({"session_id": session_id} if session_id else None), + } + return _run_node(script) + + +# ── Fail-closed: one required helper missing, nothing may mutate ────────────── + + +@pytest.mark.parametrize("helper", REQUIRED_HELPERS) +def test_missing_ownership_helper_sends_no_request(helper): + """A missing ownership helper must abort BEFORE any /api/reasoning write.""" + out = _dispatch(omit=helper) + assert out["calls"] == [], ( + f"with {helper} unavailable the effort branch still POSTed " + f"{out['calls']!r}. The old `typeof` fallback substituted an empty " + "context, so this request carried no session_id and mutated the " + "PROFILE-GLOBAL default — a higher-scope write than the user asked for, " + "and the exact defect #6809 exists to close. Dependency failure must " + "fail closed." + ) + + +@pytest.mark.parametrize("helper", REQUIRED_HELPERS) +def test_missing_ownership_helper_writes_no_chip(helper): + """A missing ownership helper must never reach _applyReasoningChip().""" + out = _dispatch(omit=helper) + assert out["chipWrites"] == [], ( + f"with {helper} unavailable the effort branch wrote the chip " + f"{out['chipWrites']!r}. With no sequence counter or predicate the old " + "current() returned true unconditionally, so a superseded response " + "still poisoned the chip." + ) + + +@pytest.mark.parametrize("helper", REQUIRED_HELPERS) +def test_missing_ownership_helper_claims_no_saved_effort(helper): + """No toast may claim the effort was saved when ownership is unavailable.""" + out = _dispatch(omit=helper) + liars = [t for t in out["toasts"] if "saved" in t or "Reasoning effort:" in t] + assert liars == [], ( + f"with {helper} unavailable the effort branch raised {liars!r}. A toast " + "asserting the effort applied here, when the write could not be scoped " + "to this session, is misinformation." + ) + + +@pytest.mark.parametrize("helper", REQUIRED_HELPERS) +def test_missing_ownership_helper_reports_instead_of_throwing(helper): + """The branch reports the internal failure rather than stranding the composer. + + ``messages.js:1494`` runs ``_cmd.fn(_parsedCmd.args)`` with no try/catch, + inside an async ``send()`` that ``ui.js:8418`` calls unawaited. An escaping + ReferenceError would skip the composer clear and the dropdown hide and + surface only as an unhandled rejection — the user sees nothing at all. The + ``/pet`` handler at ``messages.js:1505`` already establishes catch-and-report + for a failing command handler. + """ + out = _dispatch(omit=helper) + assert out["threw"] is None, ( + f"with {helper} unavailable the effort branch threw {out['threw']}, " + "which aborts send() before it clears the composer and hides the " + "command dropdown" + ) + assert any("unavailable" in t for t in out["toasts"]), ( + "the user must be told the command failed internally; silence looks " + f"identical to success. toasts={out['toasts']!r}" + ) + + +# ── Fail-closed, context helper isolated from the query helper ──────────────── +# +# The real _reasoningEffortQuery() calls _reasoningEffortContext() internally +# (ui.js:5177). With the context helper omitted, the throw therefore comes from +# the query line, which proves nothing about the branch's OWN direct +# _reasoningEffortContext() call. These tests stub the query helper so the only +# reference to the missing context helper is the production block's own. + + +def test_missing_context_helper_alone_sends_no_request(): + """The branch's own _reasoningEffortContext() call must fail closed. + + This is the fallback the maintainer named first: without it the branch POSTed + a bare ``{effort}`` and mutated the PROFILE-GLOBAL default. The unscoped + write is worse than no write, so a missing context helper must send nothing. + """ + out = _dispatch(omit="_reasoningEffortContext", stub_query=True) + assert out["calls"] == [], ( + "with only _reasoningEffortContext unavailable the effort branch POSTed " + f"{out['calls']!r}. Under the old `typeof` fallback that request carried " + "no session_id, so it silently mutated the profile-global default while " + "the toast claimed the value applied to this session." + ) + assert out["chipWrites"] == [] + assert not [t for t in out["toasts"] if "saved" in t] + + +def test_missing_context_helper_alone_reports_and_does_not_throw(): + """Isolated context failure still reports to the user without throwing.""" + out = _dispatch(omit="_reasoningEffortContext", stub_query=True) + assert out["threw"] is None, out["threw"] + assert any("unavailable" in t for t in out["toasts"]), out["toasts"] + + +# ── Positive controls: the fix must not break the working paths ─────────────── + + +def test_all_helpers_present_still_writes_the_session(): + """Control: with every helper available the scoped write still happens.""" + out = _dispatch() + assert out["threw"] is None + assert len(out["calls"]) == 1, out["calls"] + assert out["calls"][0]["body"]["session_id"] == "A" + assert out["chipWrites"] == ["high"] + + +def test_no_active_session_still_writes_the_profile_global(): + """Control: no session means no override to scope to, so the global write is right. + + This must hold through ``_reasoningEffortContext()`` omitting ``session_id`` + on its own — never through a missing-helper fallback. That distinction is + the whole point of making the helpers mandatory. + """ + out = _dispatch(session_id=None) + assert out["threw"] is None + assert len(out["calls"]) == 1, out["calls"] + body = out["calls"][0]["body"] + assert body["effort"] == "high" + assert "session_id" not in body, ( + "with no active session the command must keep writing the " + f"profile-global default; got {body!r}" + ) + assert out["chipWrites"] == ["high"] + assert any("Reasoning effort: high" in t for t in out["toasts"]), out["toasts"] + + +# ── Source contract: the fallbacks must not come back ───────────────────────── + + +@pytest.mark.parametrize("helper", REQUIRED_HELPERS) +def test_effort_branch_calls_each_helper_directly(helper): + """Pin the source contract: direct calls, no ``typeof`` fallback.""" + block = _cmd_reasoning_effort_block() + assert f"typeof {helper}" not in block, ( + f"the effort branch guards {helper} with `typeof` again. That fallback " + "fails OPEN: the substituted value drives either a profile-global " + "mutation or an unconditional late chip write." + ) + + +def test_index_html_loads_ui_before_commands(): + """The mandatory-helper decision rests on this load order — pin it. + + Both tags are ``defer``, so execution follows document order and ``ui.js`` + completes before ``commands.js`` runs. If a future change reorders these or + makes ``commands.js`` load independently, the direct calls above stop being + safe and this test must fail loudly rather than let a real load-order bug + reach users. + """ + ui = INDEX_HTML.index('src="static/ui.js') + commands = INDEX_HTML.index('src="static/commands.js') + assert ui < commands, "ui.js must load before commands.js" + for name in ("ui.js", "commands.js"): + tag_start = INDEX_HTML.index(f'src="static/{name}') + tag = INDEX_HTML[INDEX_HTML.rindex("", tag_start) + 1] + assert "defer" in tag, f"{name} must stay deferred for ordered execution: {tag}" + assert "async" not in tag, f"{name} must not be async — that breaks order: {tag}" diff --git a/tests/test_reasoning_effort_slash_command_scope.py b/tests/test_reasoning_effort_slash_command_scope.py new file mode 100644 index 00000000000..75687cba2e8 --- /dev/null +++ b/tests/test_reasoning_effort_slash_command_scope.py @@ -0,0 +1,234 @@ +"""Regression coverage for the ``/reasoning `` slash command scope (#6809 review). + +Blocker 2 (``static/commands.js``) + The composer chip POST carries ``_reasoningEffortContext()`` so its write + lands on the active session. The ``/reasoning high`` slash command did not, + so it wrote the profile-global default while its toast claimed the new value + applied here. A session holding a persisted override kept its old effort and + the UI lied about it. + + The command also needs the chip's staleness guard: a POST dispatched from + session A can resolve after the user switches to session B, and applying it + there poisons B's chip and cache. + + One behaviour must NOT change. With no active session there is no override + to scope to, so the command keeps writing the profile-global default. That + is what ``/reasoning`` does before the first chat starts. + +These tests drive the REAL ``cmdReasoning()`` effort branch — extracted verbatim +from ``static/commands.js`` and executed under node against the REAL +``_reasoningEffortContext`` / ``_reasoningEffortQuery`` / +``_reasoningDispatchIsCurrent`` helpers from ``static/ui.js`` — rather than +re-implementing the guard and asserting on the copy. +""" +import json +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +UI_JS = ROOT.joinpath("static", "ui.js").read_text(encoding="utf-8") +COMMANDS_JS = ROOT.joinpath("static", "commands.js").read_text(encoding="utf-8") + +NODE_TIMEOUT = 30 + + +def _balanced_block(src: str, start: int) -> str: + """Return src[start:] up to and including the close of its first ``{`` block.""" + brace = src.index("{", start) + depth = 1 + i = brace + 1 + while depth and i < len(src): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + i += 1 + assert depth == 0, "unbalanced braces while slicing block" + return src[start:i] + + +def _function_source(src: str, name: str) -> str: + marker = f"function {name}(" + start = src.index(marker) + return _balanced_block(src, start) + + +def _cmd_reasoning_effort_block() -> str: + """Slice the real ``if(EFFORTS.includes(arg)){ ... }`` branch out of cmdReasoning().""" + body = _function_source(COMMANDS_JS, "cmdReasoning") + start = body.index("if(EFFORTS.includes(arg)){") + return _balanced_block(body, start) + + +# Shared preamble: the REAL context/query/guard helpers from static/ui.js plus the +# minimum app state the extracted branch touches. A deferred api() lets each +# scenario choose whether the session changes before the response resolves. +_PREAMBLE = """ +const calls = []; +const toasts = []; +const chipWrites = []; + +let _profileTransitionReasoningContext = null; +const S = { session: { session_id: 'A' }, activeProfile: 'default' }; +const $ = () => null; // no modelSelect in this harness +const _modelStateForSelect = () => ({}); + +%(context_fn)s +%(query_fn)s +let _reasoningFetchSeq = 0; +%(guard_fn)s + +let _pendingResolve = null; +let _pendingReject = null; +function api(path, opts) { + calls.push({ path, body: opts && opts.body ? JSON.parse(opts.body) : null }); + return new Promise((res, rej) => { _pendingResolve = res; _pendingReject = rej; }); +} +function showToast(msg) { toasts.push(msg); } +function _applyReasoningChip(eff, meta) { chipWrites.push(eff); } +""" + + +def _preamble() -> str: + return _PREAMBLE % { + "context_fn": _function_source(UI_JS, "_reasoningEffortContext"), + "query_fn": _function_source(UI_JS, "_reasoningEffortQuery"), + "guard_fn": _function_source(UI_JS, "_reasoningDispatchIsCurrent"), + } + + +def _run_node(script: str) -> dict: + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + proc = subprocess.run( + [node, "-e", script], capture_output=True, text=True, timeout=NODE_TIMEOUT + ) + assert proc.returncode == 0, f"node harness failed:\n{proc.stderr}" + return json.loads(proc.stdout.strip()) + + +def _run_cmd_reasoning(*, session_id: str | None = "A", switch_to=None, fail=False) -> dict: + """Dispatch the REAL cmdReasoning() effort branch.""" + script = textwrap.dedent( + """ + %(preamble)s + const BRAIN = '\\uD83E\\uDDE0'; + const arg = 'high'; + const EFFORTS = ['none','minimal','low','medium','high','xhigh','max']; + S.session = %(session)s; + + // Run the REAL cmdReasoning() effort branch verbatim. + (function () { + %(block)s + })(); + + const switchTo = %(switch_to)s; + if (switchTo) S.session = { session_id: switchTo }; + + %(settle)s + + setTimeout(() => { + console.log(JSON.stringify({ calls, toasts, chipWrites, seq: _reasoningFetchSeq })); + }, 0); + """ + ) % { + "preamble": _preamble(), + "block": _cmd_reasoning_effort_block(), + "session": json.dumps({"session_id": session_id} if session_id else None), + "switch_to": json.dumps(switch_to), + "settle": ( + "_pendingReject(new Error('boom'));" + if fail + else "_pendingResolve({ reasoning_effort: 'high' });" + ), + } + return _run_node(script) + + +def test_slash_reasoning_writes_the_active_session(): + """``/reasoning high`` must scope the write to the session, not the profile.""" + out = _run_cmd_reasoning(session_id="A") + assert len(out["calls"]) == 1 + assert out["calls"][0]["path"] == "/api/reasoning" + body = out["calls"][0]["body"] + assert body["effort"] == "high" + assert body.get("session_id") == "A", ( + "/reasoning POSTed without a session_id, so it wrote the " + "profile-global default while the toast claimed the value applied to " + "this session (#6809 review blocker 2)" + ) + + +def test_slash_reasoning_with_no_session_still_writes_the_global(): + """No active session means no override to scope to — the global write is correct.""" + out = _run_cmd_reasoning(session_id=None) + assert len(out["calls"]) == 1 + body = out["calls"][0]["body"] + assert body["effort"] == "high" + assert "session_id" not in body, ( + "with no active session the command must keep writing the profile-global " + "default; inventing a session_id here would break /reasoning before the " + "first chat starts" + ) + + +def test_slash_reasoning_with_no_session_still_toasts_and_updates_the_chip(): + """The no-session path must stay fully functional, not just correctly scoped.""" + out = _run_cmd_reasoning(session_id=None) + assert out["chipWrites"] == ["high"], ( + "the no-session /reasoning path must still update the chip after the " + "global write succeeds" + ) + assert any("Reasoning effort: high" in t for t in out["toasts"]), out["toasts"] + + +def test_slash_reasoning_applies_when_session_is_unchanged(): + """Control: an unswitched ``/reasoning high`` still toasts and updates the chip.""" + out = _run_cmd_reasoning(session_id="A") + assert out["chipWrites"] == ["high"] + assert any("Reasoning effort: high" in t for t in out["toasts"]), out["toasts"] + + +def test_slash_reasoning_discards_response_after_session_switch(): + """The chip POST's staleness guard must cover the slash-command path too.""" + out = _run_cmd_reasoning(session_id="A", switch_to="B") + assert out["chipWrites"] == [], ( + "a /reasoning POST dispatched from session A resolved after a switch to " + "session B and wrote B's chip" + ) + assert out["toasts"] == [], ( + "a stale /reasoning response must be discarded silently; a toast naming " + "an effort for a session the user already left is misinformation" + ) + + +def test_slash_reasoning_discards_failure_toast_after_session_switch(): + """The guard covers the rejection path, not just success.""" + out = _run_cmd_reasoning(session_id="A", switch_to="B", fail=True) + assert out["toasts"] == [], ( + "a stale /reasoning failure must not raise a toast on the session the " + "user switched to" + ) + + +def test_slash_reasoning_takes_a_sequence_number_before_dispatch(): + """The command shares the chip's dispatch counter so either can supersede the other.""" + out = _run_cmd_reasoning(session_id="A") + assert out["seq"] == 1, ( + "/reasoning must increment the shared _reasoningFetchSeq before its " + "request so a later dispatch supersedes it even when the key is identical" + ) + + +def test_slash_reasoning_source_carries_the_context_helper(): + """Static backstop: the effort branch must build its body from the shared helper.""" + block = _cmd_reasoning_effort_block() + assert "_reasoningEffortContext()" in block, ( + "the /reasoning effort branch must include _reasoningEffortContext() in " + "its POST body so the write lands on the session, not the profile" + )