diff --git a/CHANGELOG.md b/CHANGELOG.md index 24243cc3..849b0a16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,15 @@ sidebar, subagent sessions disappeared because `state.db` never gives them a `project_id`. Only subagent rows now inherit their parent's project, resolved once per lineage, so forks keep their own "No project" assignment and deep lineages stay fast. (#7765 by @carlotestor) +- **Saving a cron job with the model picker at "Default" keeps its provider-only pin.** Editing a job + pinned to a provider with no model (the usual shape for a self-hosted OpenAI-compatible router) + sent `provider: null` and wiped the pin. The editor now preserves a provider-only pin when the + model picker is left at Default. (#7779 by @cushingw) +- **A consumed mid-turn `/steer` no longer leaves its out-of-band wrapper in the settled chat (#7600).** + A `/steer` reaches the agent as an `[OUT-OF-BAND USER MESSAGE …]` block appended to the turn's + last tool result. After the turn settled, that wrapper stayed visible in the chat transcript. The + settled-transcript writeback now scrubs the consumed wrapper from the rows it is built from. + (#7610 by @webtecnica) - **Saving a very large session no longer reads and parses the whole file just to count messages.** The #1558 backup safeguard in `Session.save()` needs the on-disk message count; it obtained it by loading the entire sidecar. On a real 203 MB / 266,940-message session that made every save diff --git a/api/streaming.py b/api/streaming.py index 22c674c1..6f2b3b55 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -2319,6 +2319,14 @@ def _prepare_marker_clean_writeback( cleaned, has_verification_nudge = _clean_synthetic_control_messages_with_provenance( result_messages ) + # Same internal-control class, second home: a consumed mid-turn /steer is + # appended to the turn's last tool result wrapped in + # [OUT-OF-BAND USER MESSAGE ...] ... [/OUT-OF-BAND USER MESSAGE]. Strip it + # here, on the rows both writebacks are built from, so neither + # session.messages (rendered verbatim) nor session.context_messages keeps + # the raw wrapper. Stripping the incoming rows too keeps them identity-equal + # to the marker-free rows persisted by earlier turns. (#7600) + cleaned = _strip_oob_markers_from_messages(cleaned) provenance = { 'verification_nudge_seen': has_verification_nudge, 'active_turn_identity': copy.deepcopy(active_turn_identity), @@ -2431,6 +2439,11 @@ def _settle_result_messages( source=source, verification_nudge_provenance=verification_nudge_provenance, ) + # The merge carries earlier display rows across turns verbatim, so a row + # settled before this guard existed would keep its raw wrapper forever. + # Scrub the persisted display copy too — after the merge, so identity + # matching above still saw the rows unchanged. (#7600) + session.messages = _strip_oob_markers_from_messages(session.messages) _annotate_media_snapshots_for_settled_messages(session.messages) _compact_session_image_parts_for_persistence(session) _advance_truncation_watermark_after_commit(session) # #3831 @@ -5593,6 +5606,94 @@ def _strip_oob_blocks(content): return content +_OOB_ANY_OPEN_RE = re.compile( + r'\[OUT-OF-BAND\s+USER\s+MESSAGE(?:\s*(?:—|-)\s*.*?)?\]', + re.IGNORECASE, +) +_OOB_ANY_CLOSE_RE = re.compile( + r'\[/OUT-OF-BAND\s+USER\s+MESSAGE\]', + re.IGNORECASE, +) + + +def _unwrap_single_oob_frame(content: str) -> str | None: + """Unwrap exactly one fully-validated [OUT-OF-BAND USER MESSAGE] frame. + + Returns the extracted inner user text if and only if ``content`` consists of + exactly one valid opening tag and one valid closing tag wrapping the user + content. If markers are multiple, nested, incomplete, or ambiguous, returns + None so caller preserves the row byte-for-byte. + """ + if not isinstance(content, str): + return None + stripped = content.strip() + if not stripped: + return None + + open_matches = list(_OOB_ANY_OPEN_RE.finditer(stripped)) + close_matches = list(_OOB_ANY_CLOSE_RE.finditer(stripped)) + + # Must have exactly one opening marker and one closing marker + if len(open_matches) != 1 or len(close_matches) != 1: + return None + + open_m = open_matches[0] + close_m = close_matches[0] + + # Opening marker must be at the very start of stripped content + if open_m.start() != 0: + return None + + # Closing marker must be at the very end of stripped content + if close_m.end() != len(stripped): + return None + + # Opening marker must end before closing marker starts + if open_m.end() > close_m.start(): + return None + + inner = stripped[open_m.end():close_m.start()] + # Strip surrounding whitespace/newlines from the extracted user text + return inner.strip('\r\n').strip() + + +def _unwrap_steer_row_oob_marker(message: dict) -> None: + """Extract inner steer text from a typed steer row in place (#7600). + + The Hermes Agent emits mid-turn steers as standalone typed user rows + (`role == 'user'`, `display_kind == 'steer'`). The control wrapper + `[OUT-OF-BAND USER MESSAGE ...] ... [/OUT-OF-BAND USER MESSAGE]` is + extracted to preserve only the user-authored instruction. + + Mutates caller-row in place to maintain object identity. If the marker + frame is malformed, nested, multiple, or legacy, preserves the row + byte-for-byte. + """ + if not isinstance(message, dict): + return + if message.get('role') != 'user' or message.get('display_kind') != 'steer': + return + content = message.get('content') + if isinstance(content, str): + unwrapped = _unwrap_single_oob_frame(content) + if unwrapped is not None: + message['content'] = unwrapped + elif isinstance(content, list): + if len(content) == 1 and isinstance(content[0], dict): + part = content[0] + if part.get('type') == 'text' and isinstance(part.get('text'), str): + unwrapped = _unwrap_single_oob_frame(part['text']) + if unwrapped is not None: + part['text'] = unwrapped + + +def _strip_oob_markers_from_messages(messages): + """Unwrap OOB steer markers from typed steer rows in place (#7600).""" + for message in messages or []: + _unwrap_steer_row_oob_marker(message) + return messages + + def _content_has_reasoning_only_parts(content) -> bool: if not isinstance(content, list) or not content: return False diff --git a/docs/rfcs/webui-pending-intent-controls.md b/docs/rfcs/webui-pending-intent-controls.md index d8c3e65a..d2195e74 100644 --- a/docs/rfcs/webui-pending-intent-controls.md +++ b/docs/rfcs/webui-pending-intent-controls.md @@ -215,6 +215,14 @@ settle. Steer must not exist only as a toast or transient DOM state. It must survive refresh, session switch, replay, and settled render. +**The transport wrapper is not the Steer row.** A Steer is emitted as a standalone +typed user row (`role: user`, `display_kind: "steer"`). The surrounding +`[OUT-OF-BAND USER MESSAGE …] … [/OUT-OF-BAND USER MESSAGE]` block is transport-level +control data: settlement unwraps the single validated frame in place, extracting +and preserving the user's authored steer instruction while dropping the wrapper. +Legacy tool rows and rows where markers are multiple, nested, incomplete, or +contain ambiguous delimiters are preserved byte-for-byte. + ### Relationship to system control events Steer is similar to system-delivered control events such as tool-iteration-limit diff --git a/static/panels.js b/static/panels.js index 9215cc77..4f708aae 100644 --- a/static/panels.js +++ b/static/panels.js @@ -25,6 +25,7 @@ let _currentCronDetail = null; // full cron job object let _currentCronDetailKey = ''; let _cronMode = 'empty'; // 'empty' | 'read' | 'create' | 'edit' let _cronPreFormDetail = null; // snapshot of prior selection when entering a form +let _cronModelPickerTouched = false; // true once the user changes the model picker in the current form let _showAllCronProfiles = false; let _cronOtherProfileCount = 0; let _currentWorkspaceDetail = null; // { path, name, is_default } @@ -1550,6 +1551,7 @@ function duplicateCurrentCron(){ const job = _currentCronDetail; if (typeof switchPanel === 'function' && _currentPanel !== 'tasks') switchPanel('tasks'); _cronPreFormDetail = { ...job }; + _cronModelPickerTouched = false; _editingCronId = null; _cronMode = 'create'; _cronIsDuplicate = true; @@ -1606,6 +1608,7 @@ let _cronDeliveryOptionsCache=null; function openCronCreate(){ if (typeof switchPanel === 'function' && _currentPanel !== 'tasks') switchPanel('tasks'); _cronPreFormDetail = _currentCronDetail ? { ..._currentCronDetail } : null; + _cronModelPickerTouched = false; _editingCronId = null; _cronMode = 'create'; _cronIsDuplicate = false; @@ -1623,6 +1626,7 @@ function openCronCreate(){ function openCronEdit(job){ if (!job) return; _cronPreFormDetail = { ...job }; + _cronModelPickerTouched = false; _editingCronId = job.id; _cronMode = 'edit'; _cronSelectedSkills = Array.isArray(job.skills) ? [...job.skills] : []; @@ -1855,6 +1859,9 @@ async function _populateCronFormModelSelect(selectedModel, selectedProvider, dis sel.appendChild(opt); } sel.dataset.loaded = '1'; + // Track deliberate picker changes so an untouched "Default" can keep a + // hidden provider-only pin on save (see _cronProviderForClear). + sel.addEventListener('change', () => { _cronModelPickerTouched = true; }); } catch (e) { console.warn('Failed to load cron model picker:', e.message); // Load failed: dataset.loaded stays unset so saveCronForm omits model/provider @@ -1940,6 +1947,18 @@ function _cronModelBareName(model, provider) { return _modelBareNameForProvider(model, provider); } +function _cronProviderForClear(prevDetail, pickerTouched) { + // Provider to submit when the picker shows "Default" (no model) on save. + // An explicit return to Default honors "Default = no overrides" (#4030) and + // clears the provider. An untouched picker must not: provider-only jobs + // (a pinned provider with no model) render as "Default" in the combined + // picker, so saving any other field would silently erase the pin the user + // never touched. + const prev = prevDetail || {}; + if (pickerTouched) return null; + return (prev.model == null) ? prev.provider : null; +} + async function saveCronForm(){ const nameEl=$('cronFormName'); const schEl=$('cronFormSchedule'); @@ -1977,8 +1996,11 @@ async function saveCronForm(){ updates.model = _cronModelBareName(modelState.model, modelState.model_provider) || null; updates.provider = modelState.model_provider || null; } else if (modelLoaded) { + // "Default" selected (no model). An untouched picker preserves a + // hidden provider-only pin; a deliberate return to Default clears + // it (#4030 "Default = no overrides"). updates.model = null; - updates.provider = null; + updates.provider = _cronProviderForClear(_cronPreFormDetail, _cronModelPickerTouched); } // else: select not yet populated — omit model/provider to preserve saved value } diff --git a/tests/test_cron_model_override.py b/tests/test_cron_model_override.py index bf3b4b89..6c9501c8 100644 --- a/tests/test_cron_model_override.py +++ b/tests/test_cron_model_override.py @@ -8,6 +8,8 @@ import sys import types from pathlib import Path +import pytest + REPO = Path(__file__).resolve().parents[1] PANELS_JS = (REPO / "static" / "panels.js").read_text(encoding="utf-8") @@ -170,3 +172,89 @@ def test_cron_model_picker_marks_loaded_only_after_successful_population(): catch_block = body.split("} catch (e)", 1)[1] assert "sel.dataset.loaded = '1'" in try_block assert "sel.dataset.loaded = '1'" not in catch_block + + +def test_cron_update_preserves_provider_only_pin_when_model_cleared(): + """Clearing the picker to "Default" must not wipe a provider-only pin. + + #4030 intentionally made "clearing the picker = no overrides", which is + correct for model+provider pins but silently destroyed provider-only + pins (jobs that pin a provider with no model, e.g. self-hosted LLM + endpoints). The combined picker cannot represent that state, so + saveCronForm must preserve prev.provider when the job carried no model + AND the user never changed the picker; a deliberate return to Default + must clear it. + """ + save_body = _function_body("saveCronForm") + helper_body = _function_body("_cronProviderForClear") + + # The clear branch delegates to the helper and records picker changes. + assert "_cronProviderForClear(_cronPreFormDetail, _cronModelPickerTouched)" in save_body + assert "preserveProvider" not in save_body + + # The helper distinguishes an explicit clear (pickerTouched) from an + # untouched provider-only pin. + assert "if (pickerTouched) return null" in helper_body + assert "prev.model == null" in helper_body + + # The form entry points reset the touched flag. + for fn in ("openCronCreate", "openCronEdit", "duplicateCurrentCron"): + assert "_cronModelPickerTouched = false" in _function_body(fn) + # The picker records deliberate changes. + pop_body = _function_body("_populateCronFormModelSelect") + assert "_cronModelPickerTouched = true" in pop_body + + +def test_cron_provider_for_clear_behavior(): + """Exercise _cronProviderForClear through node for both pin shapes.""" + import shutil + import subprocess + + node = shutil.which("node") + if node is None: + pytest.skip("node not on PATH") + + # Locate the full declaration (signature + body) of the helper. + src = PANELS_JS + start = src.find("function _cronProviderForClear(") + assert start != -1, "helper declaration not found" + brace = src.find("{", start) + depth = 0 + end = None + for i in range(brace, len(src)): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + if depth == 0: + end = i + 1 + break + decl = src[start:end] + + driver = f""" +{decl} +const cases = [ + ['provider_only_untouched', {{model: null, provider: 'custom:llama-swap'}}, false], + ['provider_only_touched', {{model: null, provider: 'custom:llama-swap'}}, true], + ['model_pin_touched', {{model: 'gpt-5', provider: 'openai'}}, true], + ['no_override_untouched', {{model: null, provider: null}}, false], + ['no_prev', null, false], +]; +const out = {{}}; +for (const [name, detail, touched] of cases) out[name] = _cronProviderForClear(detail, touched) ?? null; +process.stdout.write(JSON.stringify(out)); +""" + proc = subprocess.run([node, "-e", driver], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node driver failed: {proc.stderr}" + r = json.loads(proc.stdout) + + # Untouched picker on a provider-only job: the pin survives. + assert r["provider_only_untouched"] == "custom:llama-swap" + # Deliberate return to Default: "Default = no overrides" wins. + assert r["provider_only_touched"] is None + # Model-pinned job, explicit clear: provider removed too. + assert r["model_pin_touched"] is None + # No overrides stored, untouched picker: nothing to preserve. + assert r["no_override_untouched"] is None + # No pre-form snapshot (create form): no provider. + assert r["no_prev"] is None diff --git a/tests/test_issue7600_oob_display_writeback.py b/tests/test_issue7600_oob_display_writeback.py new file mode 100644 index 00000000..3edb7183 --- /dev/null +++ b/tests/test_issue7600_oob_display_writeback.py @@ -0,0 +1,185 @@ +"""Settled writeback must extract inner steer text from typed steer rows (#7600). + +A mid-turn ``/steer`` is emitted as a standalone typed user row +(``role: 'user'``, ``display_kind: 'steer'``). The surrounding +``[OUT-OF-BAND USER MESSAGE ...] ... [/OUT-OF-BAND USER MESSAGE]`` block is +transport-level control data. Settlement unwraps the single validated frame +in place, preserving the user's authored steer instruction while dropping +the wrapper. + +Legacy tool rows and rows where markers are multiple, nested, incomplete, +or contain ambiguous delimiters are preserved byte-for-byte. +""" +from __future__ import annotations + +import copy +import json + +from api.models import Session +from api.streaming import _settle_result_messages, _unwrap_steer_row_oob_marker +from api import streaming as _streaming + +PROMPT = "please run the smoke checks" + +OOB_OPEN = ( + "[OUT-OF-BAND USER MESSAGE — a direct message from the user, delivered once " + "at this position; not tool output and not a new delivery when replayed from " + "conversation history]" +) +OOB_CLOSE = "[/OUT-OF-BAND USER MESSAGE]" +OOB_BLOCK = f"{OOB_OPEN}\nsteer: use the staging bucket this time\n{OOB_CLOSE}" +OOB_VARIANT = f"[OUT-OF-BAND USER MESSAGE]steer: also bump the timeout{OOB_CLOSE}" + + +def _contains_oob(value) -> bool: + return "OUT-OF-BAND USER MESSAGE" in json.dumps(value) + + +def _session_with_prior_turn() -> Session: + """Prior turn already settled: user -> assistant(tool_call) -> tool -> answer.""" + display = [ + {"role": "user", "content": "deploy the app", "timestamp": 1788439000}, + { + "role": "assistant", + "content": "", + "tool_calls": [{"id": "call-1"}], + "timestamp": 1788439005, + }, + { + "role": "tool", + "tool_call_id": "call-1", + "content": "deploy finished: revision 41", + "timestamp": 1788439010, + }, + {"role": "assistant", "content": "Deploy done (revision 41).", "timestamp": 1788439015}, + ] + session = Session(session_id="7" * 12, title="steer writeback", messages=copy.deepcopy(display)) + session.context_messages = copy.deepcopy(display) + return session + + +def _settle_turn_with_steer(session, monkeypatch, *, tool_content: str = "checks passed", steer_rows=None): + """Settle one turn with an optional steer row emitted by the Agent.""" + monkeypatch.setattr( + _streaming, "_annotate_media_snapshots_for_settled_messages", lambda messages: None + ) + previous = list(session.messages) + previous_context = list(session.context_messages) + ts = 1788440000 + steers = list(steer_rows or []) + result = copy.deepcopy(previous_context) + [ + {"role": "user", "content": PROMPT, "timestamp": ts}, + { + "role": "assistant", + "content": "", + "tool_calls": [{"id": "call-2"}], + "timestamp": ts + 1, + }, + { + "role": "tool", + "tool_call_id": "call-2", + "content": tool_content, + "timestamp": ts + 2, + }, + *steers, + {"role": "assistant", "content": "Smoke checks passed.", "timestamp": ts + 3}, + ] + _settle_result_messages(session, previous, previous_context, result, PROMPT, "webui", None) + return previous + + +def test_settle_unwraps_steer_row_in_place_preserving_user_text(monkeypatch): + """A typed steer row is unwrapped in place: inner text preserved, wrapper gone.""" + session = _session_with_prior_turn() + steer_row = { + "role": "user", + "display_kind": "steer", + "content": OOB_BLOCK, + "timestamp": 1788440002.5, + } + _settle_turn_with_steer(session, monkeypatch, steer_rows=[steer_row]) + + # The outer wrapper is dropped from display and context + assert not _contains_oob(session.messages) + assert not _contains_oob(session.context_messages) + + # The user's inner steer text is preserved exactly + steer_contents = [ + m.get("content") + for m in session.messages + if isinstance(m, dict) and m.get("display_kind") == "steer" + ] + assert steer_contents == ["steer: use the staging bucket this time"] + + # In-place mutation preserved the dictionary object identity + assert steer_row["content"] == "steer: use the staging bucket this time" + + +def test_settle_unwraps_list_based_content_steer_row(monkeypatch): + """List-based content parts on a steer row are unwrapped too.""" + session = _session_with_prior_turn() + steer_row = { + "role": "user", + "display_kind": "steer", + "content": [{"type": "text", "text": OOB_VARIANT}], + "timestamp": 1788440002.5, + } + _settle_turn_with_steer(session, monkeypatch, steer_rows=[steer_row]) + + assert not _contains_oob(session.messages) + assert not _contains_oob(session.context_messages) + assert steer_row["content"] == [{"type": "text", "text": "steer: also bump the timeout"}] + + +def test_settle_leaves_legacy_tool_rows_alone(monkeypatch): + """Legacy tool rows carrying raw markers are left alone (not truncated/deleted).""" + session = _session_with_prior_turn() + tool_text = f"checks passed\n\n{OOB_BLOCK}" + _settle_turn_with_steer(session, monkeypatch, tool_content=tool_text) + + tool_bodies = [ + m.get("content") for m in session.messages if isinstance(m, dict) and m.get("role") == "tool" + ] + assert tool_text in tool_bodies + + +def test_settle_preserves_malformed_nested_and_multiple_blocks_byte_for_byte(): + """Nested markers, multiple frames, and incomplete markers degrade gracefully.""" + nested = f"{OOB_OPEN}\nhello {OOB_VARIANT} world\n{OOB_CLOSE}" + multiple = f"{OOB_VARIANT} and {OOB_BLOCK}" + incomplete = "[OUT-OF-BAND USER MESSAGE — truncated" + + for malformed in (nested, multiple, incomplete): + steer_row = { + "role": "user", + "display_kind": "steer", + "content": malformed, + "timestamp": 1788440002.5, + } + _unwrap_steer_row_oob_marker(steer_row) + assert steer_row["content"] == malformed, f"malformed content was corrupted: {malformed!r}" + + +def test_settle_keeps_normal_user_pasted_marker_intact(monkeypatch): + """A normal user row that quotes a complete marker survives byte-for-byte.""" + pasted = ( + "our bot log shows this block, is that normal?\n" + f"{OOB_BLOCK}\n" + "the docs say the gateway adds it" + ) + session = _session_with_prior_turn() + session.messages[0]["content"] = pasted + session.context_messages = copy.deepcopy(session.messages) + + steer_row = { + "role": "user", + "display_kind": "steer", + "content": OOB_BLOCK, + "timestamp": 1788440002.5, + } + _settle_turn_with_steer(session, monkeypatch, steer_rows=[steer_row]) + + # The normal user row still carries its quoted block intact + assert session.messages[0]["content"] == pasted + # The steer row had its wrapper unwrapped + assert session.messages[-2]["content"] == "steer: use the staging bucket this time"