From 7d9621788334e66804f07883436f74b7329936ad Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:29:00 +0000 Subject: [PATCH 01/15] Stop the idle chat shell from scheduling frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent --- frontend/src/components/ChatView/ChatView.jsx | 9 +- .../__tests__/framePipelineQuiescence.test.js | 133 ++++++++++++++++++ .../src/components/ChatView/useFileUpload.js | 31 ++-- frontend/src/components/Drawer/Drawer.css | 45 +++--- frontend/src/components/Drawer/Drawer.jsx | 10 +- .../src/lib/__tests__/vite-env-loader.mjs | 11 +- 6 files changed, 203 insertions(+), 36 deletions(-) create mode 100644 frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js diff --git a/frontend/src/components/ChatView/ChatView.jsx b/frontend/src/components/ChatView/ChatView.jsx index 92127323e..7cbe56b7d 100644 --- a/frontend/src/components/ChatView/ChatView.jsx +++ b/frontend/src/components/ChatView/ChatView.jsx @@ -888,7 +888,14 @@ export default function ChatView({ // layout-derived floor and re-applies the active mode under the reader gate // (design §2). Skipped entirely for single-pane chats (paneContentHeight // null) so today's resize behavior is untouched. - useEffect(() => { + // + // Must be a layout effect: this is the only automatic scroll write in the + // controller that would otherwise run after paint. Every other one is + // pre-paint (syncLayout in a layout effect, the tail follow in a + // ResizeObserver callback, settleStreamingPin in rAF), and running this one + // post-paint shows the reader a frame at the old scroll position before the + // correction lands — visible as a jump when pane geometry changes. + useLayoutEffect(() => { if (paneContentHeight != null) paneResized(paneContentHeight) }, [paneContentHeight, paneResized]) diff --git a/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js b/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js new file mode 100644 index 000000000..a273f326c --- /dev/null +++ b/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js @@ -0,0 +1,133 @@ +// The idle shell must stop asking the browser for frames. Three separate +// sources kept the frame pipeline alive with the app doing nothing, and each +// one is pinned below: +// 1. useFileUpload handed the composer a fresh callback identity on every +// render, re-allocating ChatView's `doSend` and re-rendering the whole +// transcript on each keystroke. +// 2. the pane-resize correction ran post-paint, so a geometry change showed +// one frame at the stale scroll position before the fix landed. +// 3. the drawer's streaming dot animated on an `infinite` loop, and the +// drawer is always mounted. +// +// Run with: +// cd frontend && node --loader=./src/lib/__tests__/vite-env-loader.mjs \ +// --test src/components/ChatView/__tests__/framePipelineQuiescence.test.js +// +// The loader aliases `react` -> react-hook-shim for useFileUpload.js so the +// hook can be driven from node without a renderer. See react-hook-shim.mjs. + +import test from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { renderHook } from '../hooks/__tests__/react-hook-shim.mjs' +import useFileUpload from '../useFileUpload.js' + +const chatView = readFileSync(new URL('../ChatView.jsx', import.meta.url), 'utf8') +// Comments stripped: the rules below are documented by prose that names the +// very properties and keywords being asserted against ("the animation never +// ended", "never `infinite`"), so a whole-file scan has to look at declarations +// only or it reads the warning as the violation. +const drawerCss = readFileSync( + new URL('../../Drawer/Drawer.css', import.meta.url), + 'utf8', +).replace(/\/\*[\s\S]*?\*\//g, '') + +const ACTIONS = ['addFiles', 'removeFile', 'releaseFiles', 'clearFiles', 'restoreFiles'] + +// ChatView rebuilds this argument object — and a fresh `onFilesChange` arrow — +// on every render, which is exactly the churn the memoization has to absorb. +function props(chatId = 'chat-1') { + return { chatId, onFilesChange: () => {} } +} + +test('attachment actions keep their identity when the composer re-renders', () => { + const { result, rerender } = renderHook(useFileUpload, props()) + const first = { ...result.current } + + rerender(props()) + rerender(props()) + + for (const action of ACTIONS) { + assert.equal( + result.current[action], + first[action], + `${action} must not acquire a new identity on every composer render`, + ) + } +}) + +test('attachment actions keep their identity while files are being staged', () => { + const { result, rerender } = renderHook(useFileUpload, props()) + const first = { ...result.current } + + // A state change inside the hook, not just a parent re-render: restoreFiles + // commits new files, which is the path a draft restore takes. + result.current.restoreFiles([{ id: 'f1', name: 'a.png', status: 'done' }]) + rerender(props()) + + assert.equal(result.current.files.length, 1, 'guard: the commit actually landed') + for (const action of ACTIONS) { + assert.equal(result.current[action], first[action], `${action} churned on a file commit`) + } +}) + +test('attachment actions that talk to a chat are reissued when the chat changes', () => { + // The other half of the contract: memoization that never invalidates would + // leave addFiles/removeFile calling the previous chat's upload endpoint. + const { result, rerender } = renderHook(useFileUpload, props('chat-1')) + const first = { ...result.current } + + rerender(props('chat-2')) + + assert.notEqual(result.current.addFiles, first.addFiles) + assert.notEqual(result.current.removeFile, first.removeFile) + // These three never read chatId, so they legitimately stay put. + for (const action of ['releaseFiles', 'clearFiles', 'restoreFiles']) { + assert.equal(result.current[action], first[action]) + } +}) + +// Whitespace- and formatting-independent: find the call that encloses the +// scroll write rather than matching one specific line layout. +function enclosingEffectHook(source, needle) { + const at = source.indexOf(needle) + assert.notEqual(at, -1, `${needle} not found in ChatView.jsx`) + const before = source.slice(0, at) + // 'useLayoutEffect(' does not contain 'useEffect(' as a substring, so the + // later of the two indexes is unambiguously the enclosing hook. + return before.lastIndexOf('useLayoutEffect(') > before.lastIndexOf('useEffect(') + ? 'useLayoutEffect' + : 'useEffect' +} + +test('pane resize correction runs before paint', () => { + assert.equal( + enclosingEffectHook(chatView, 'paneResized(paneContentHeight)'), + 'useLayoutEffect', + 'a post-paint pane-resize correction shows one frame at the stale scroll position', + ) +}) + +function ruleBody(selector) { + const rule = drawerCss.match(new RegExp(`\\${selector}\\s*\\{([^}]*)\\}`)) + assert.ok(rule, `${selector} rule must remain present in Drawer.css`) + return rule[1].trim() +} + +test('always-mounted streaming chrome has no infinite animation', () => { + assert.doesNotMatch(ruleBody('.drawer__streaming-dot'), /animation\s*:/) + assert.doesNotMatch(drawerCss, /@keyframes\s+drawer-streaming-pulse/) + assert.doesNotMatch(drawerCss, /animation[^;}]*\binfinite\b/) +}) + +test('streaming and attention rows stay tellable apart without motion', () => { + // Removing the pulse left these two rulesets byte-identical, which collapsed + // "the agent is working" and "it finished while you were away" into one + // visual. The distinction must stay static — re-adding an infinite animation + // to always-mounted chrome is the bug this file exists to prevent. + const streaming = ruleBody('.drawer__streaming-dot') + const attention = ruleBody('.drawer__attention-dot') + assert.notEqual(streaming, attention, 'the two drawer row states render identically') + assert.match(streaming, /background:\s*var\(--accent\)/) + assert.match(attention, /background:\s*var\(--green\)/) +}) diff --git a/frontend/src/components/ChatView/useFileUpload.js b/frontend/src/components/ChatView/useFileUpload.js index 0f5678663..adaee9091 100644 --- a/frontend/src/components/ChatView/useFileUpload.js +++ b/frontend/src/components/ChatView/useFileUpload.js @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect } from 'react' +import { useState, useRef, useEffect, useCallback } from 'react' import { getAuthHeaders, BASE } from '../../api/client.js' /** @@ -32,7 +32,12 @@ export default function useFileUpload({ chatId, initialFiles = [], onFilesChange const onFilesChangeRef = useRef(onFilesChange) onFilesChangeRef.current = onFilesChange - function commitFiles(nextOrUpdater) { + // Every action below is wrapped in useCallback and reads its inputs through + // refs, so the returned identities only change when `chatId` does. Callers + // put these in dependency arrays (ChatView's `doSend`), and an unstable + // identity there re-allocates `doSend` on every render, which breaks + // MsgContent's memo and re-renders the whole transcript on each keystroke. + const commitFiles = useCallback((nextOrUpdater) => { const next = typeof nextOrUpdater === 'function' ? nextOrUpdater(filesRef.current) : nextOrUpdater @@ -40,7 +45,7 @@ export default function useFileUpload({ chatId, initialFiles = [], onFilesChange setFiles(next) onFilesChangeRef.current?.(next) return next - } + }, []) // Revoke any surviving object URLs when the component unmounts — // e.g. the user navigated away while files were still staged. @@ -50,7 +55,7 @@ export default function useFileUpload({ chatId, initialFiles = [], onFilesChange } }, []) - async function addFiles(fileList) { + const addFiles = useCallback(async (fileList) => { if (!fileList.length) return const newChips = fileList.map(f => ({ @@ -97,9 +102,9 @@ export default function useFileUpload({ chatId, initialFiles = [], onFilesChange )) } } - } + }, [chatId, commitFiles]) - function removeFile(id) { + const removeFile = useCallback((id) => { // Extract the side effects (URL revoke + network DELETE) from the // setFiles updater. React may double-invoke state updaters in // Strict Mode, which would fire two DELETE requests for the same @@ -113,24 +118,24 @@ export default function useFileUpload({ chatId, initialFiles = [], onFilesChange headers: getAuthHeaders(), }).catch(() => {}) } - } + }, [chatId, commitFiles]) - function releaseFiles(fileList) { + const releaseFiles = useCallback((fileList) => { for (const f of fileList || []) { if (f.objectUrl) URL.revokeObjectURL(f.objectUrl) } - } + }, []) - function clearFiles({ revoke = true } = {}) { + const clearFiles = useCallback(({ revoke = true } = {}) => { const current = filesRef.current if (revoke) releaseFiles(current) commitFiles([]) - } + }, [releaseFiles, commitFiles]) - function restoreFiles(fileList) { + const restoreFiles = useCallback((fileList) => { const restored = Array.isArray(fileList) ? fileList : [] commitFiles(restored) - } + }, [commitFiles]) return { files, addFiles, removeFile, clearFiles, restoreFiles, releaseFiles } } diff --git a/frontend/src/components/Drawer/Drawer.css b/frontend/src/components/Drawer/Drawer.css index 91a253839..9063886f0 100644 --- a/frontend/src/components/Drawer/Drawer.css +++ b/frontend/src/components/Drawer/Drawer.css @@ -630,13 +630,26 @@ border-top: 1px solid var(--border-light); } -/* ── Streaming pulse dot ───────────────────────────── */ +/* ── Row status dots ───────────────────────────────── */ /* 6px accent dot in front of a row label. Marks chats whose agent is mid-turn so a user switching to another chat still sees the - background build progressing in the drawer. Pulse via opacity so - the dot reads as alive but doesn't shimmer aggressively — the - slow 1.5s loop keeps it ambient. */ + background build progressing in the drawer. + + Deliberately static. This dot used to pulse on a 1.5s infinite loop, + but the drawer is always mounted and a long-running agent keeps its + row on screen for hours, so the animation never ended — which kept + the compositor producing 60 frames a second even with the app fully + idle, and every one of those frames paid for the composer's + backdrop-filter. Measured: disabling this animation (with the tab + title marquee) took DrawFrame, BeginFrame and render passes to zero + at idle. The reduced-motion path below already settled that a steady + dot is an acceptable representation and that "streaming" surfaces via + aria-label + presence; this generalises that decision to everyone. + + If motion is ever wanted back here, it must be bounded — a few + iterations re-armed when the row's status CHANGES, never `infinite` + on always-mounted chrome. */ .drawer__streaming-dot { flex-shrink: 0; width: 6px; @@ -644,31 +657,31 @@ margin-right: 2px; border-radius: 50%; background: var(--accent); - animation: drawer-streaming-pulse 1.5s ease-in-out infinite; } +/* "The latest background run FINISHED while you were elsewhere" — a + different row state from the streaming dot above, and it has to stay + legible as one now that motion no longer separates them. Colour is the + channel that survives at 6px (a hollow ring or a one-pixel size delta + does not), and --green is already the palette's finished/healthy token + — SettingsView uses it for a connected provider — against --accent for + work still in flight. Colour is not the only channel: both dots carry + an aria-label and a title (Drawer.jsx), which is what a colour-blind or + screen-reader user reads. */ .drawer__attention-dot { flex-shrink: 0; width: 6px; height: 6px; margin-right: 2px; border-radius: 50%; - background: var(--accent); -} - -@keyframes drawer-streaming-pulse { - 0% { opacity: 1; } - 50% { opacity: 0.4; } - 100% { opacity: 1; } + background: var(--green); } -/* Respect prefers-reduced-motion — for users who turn off animations - the pulse becomes a steady dot rather than a flashing one. The - information ("streaming") still surfaces via aria-label + presence. */ +/* Respect prefers-reduced-motion. The streaming dot no longer needs an + override here — it is static for everyone now (see above). */ @media (prefers-reduced-motion: reduce) { .drawer, .drawer-overlay { transition: none; } - .drawer__streaming-dot { animation: none; } .drawer__more:active { transform: none; } } diff --git a/frontend/src/components/Drawer/Drawer.jsx b/frontend/src/components/Drawer/Drawer.jsx index ec2d21eb7..3e7a8b41b 100644 --- a/frontend/src/components/Drawer/Drawer.jsx +++ b/frontend/src/components/Drawer/Drawer.jsx @@ -53,19 +53,19 @@ export default function Drawer({ onDeleteAppData, onSettings, // Set of chat ids whose agent is currently streaming. Used to - // pulse a small accent dot next to the row label so the user can + // show a small accent dot next to the row label so the user can // see at a glance which background builds are still running. // Sourced from Shell (the only place that knows when a turn is // active across the whole app). Defaults to an empty Set so the // drawer renders cleanly if no parent supplies the prop. streamingChatIds, // Set of chat ids whose latest background run finished while the - // user was elsewhere. Rendered as a steady attention dot, distinct - // from the animated streaming dot above. + // user was elsewhere. Rendered as a green attention dot, distinct by + // colour from the accent streaming dot above (neither animates). attentionChatIds, // Set of app ids that first appeared in the fetched list this session - // (freshly built or App-Store-installed). Rendered as the same steady - // accent dot as chat attention, cleared by Shell when the app is opened — + // (freshly built or App-Store-installed). Rendered as the same green + // dot as chat attention, cleared by Shell when the app is opened — // an arrival cue for an app that otherwise lands silently at the bottom // of the oldest-first list. newAppIds, diff --git a/frontend/src/lib/__tests__/vite-env-loader.mjs b/frontend/src/lib/__tests__/vite-env-loader.mjs index b7104c05b..f015e582f 100644 --- a/frontend/src/lib/__tests__/vite-env-loader.mjs +++ b/frontend/src/lib/__tests__/vite-env-loader.mjs @@ -19,10 +19,19 @@ const REACT_SHIM = new URL( import.meta.url, ).href +// Modules whose `react` import is redirected to the hook shim so a test in +// this suite can drive them with renderHook. Opt-in per module rather than +// blanket: most files here are read as source text, and a global alias would +// silently swap React out from under anything that later imports for real. +const REACT_SHIMMED_MODULES = [ + '/components/Shell/useAppIntentNavigation.js', + '/components/ChatView/useFileUpload.js', +] + export async function resolve(specifier, context, nextResolve) { if ( specifier === 'react' - && context.parentURL?.endsWith('/components/Shell/useAppIntentNavigation.js') + && REACT_SHIMMED_MODULES.some(m => context.parentURL?.endsWith(m)) ) { return { url: REACT_SHIM, shortCircuit: true, format: 'module' } } From f9ac5bc65e56a14c2cd22935b7d17c66f5d40065 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:29:00 +0000 Subject: [PATCH 02/15] Report a self-requested Codex stop as interrupted, not a provider error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent --- backend/app/codex_sdk_runner.py | 122 ++++++-- backend/tests/test_codex_sdk_contract.py | 18 ++ backend/tests/test_codex_sdk_runner.py | 294 ++++++++++++++++++ .../tests/test_runner_registry_integration.py | 1 + 4 files changed, 414 insertions(+), 21 deletions(-) diff --git a/backend/app/codex_sdk_runner.py b/backend/app/codex_sdk_runner.py index 9bd90b298..d81a901db 100644 --- a/backend/app/codex_sdk_runner.py +++ b/backend/app/codex_sdk_runner.py @@ -569,7 +569,11 @@ def _sdk_imports() -> dict[str, Any]: """ from openai_codex import ApprovalMode, AsyncCodex, Sandbox from openai_codex.client import CodexConfig - from openai_codex.errors import CodexRpcError, InvalidParamsError + from openai_codex.errors import ( + CodexRpcError, + InvalidParamsError, + TransportClosedError, + ) from openai_codex.types import ReasoningEffort, ReasoningSummary from openai_codex.generated.v2_all import ( AgentMessageDeltaNotification, @@ -667,6 +671,7 @@ def _sdk_imports() -> dict[str, Any]: "ThreadTokenUsageUpdatedNotification": ( ThreadTokenUsageUpdatedNotification ), + "TransportClosedError": TransportClosedError, "TurnCompletedNotification": TurnCompletedNotification, "TurnStatus": TurnStatus, "WebSearchThreadItem": WebSearchThreadItem, @@ -1463,19 +1468,52 @@ def _file_change_patch_summary(changes: list[Any]) -> str: return "\n".join(lines) -def _is_closed_turn_error(exc: BaseException) -> bool: - """Returns True when the live turn handle is already closed/dead.""" +def _is_transport_death(exc: BaseException) -> bool: + """Returns True when the app-server connection itself died. + + Strictly the transport: the SDK's own TransportClosedError, or an RPC + error the app-server raised about a closed/dead channel. Deliberately + excludes plain RuntimeErrors, because the runner reraises every + non-retryable provider ErrorNotification as `RuntimeError(message)` — + an MCP server "is not running" is a provider fault to report, not a + dead pipe. + + The transport class comes from `_sdk_imports()` and is matched with + isinstance, not by class name: this predicate decides whether an error + reaches the owner at all, so it must be bound to the real symbol (and + must match its subclasses) rather than to anything that happens to + share a name. + """ sdk: dict[str, Any] | None = None try: sdk = _sdk_imports() - except ModuleNotFoundError: + except ImportError: + # ImportError, not ModuleNotFoundError: an SDK that renames or drops + # TransportClosedError fails the `from ... import` the same way a missing + # package does, and this predicate runs INSIDE the turn's except handler — + # raising here would mask the very exception it was asked to classify. sdk = None - if sdk is not None and isinstance( - exc, (sdk["InvalidParamsError"], sdk["CodexRpcError"]) - ): + if sdk is None: + # Without the SDK no turn can have started, so no transport of ours can + # have died. Matching on a class name alone would be worse than useless + # here: it would let any look-alike be mistaken for the real thing. + return False + if isinstance(exc, sdk["TransportClosedError"]): + return True + if isinstance(exc, (sdk["InvalidParamsError"], sdk["CodexRpcError"])): text = str(exc).lower() return "closed" in text or "not running" in text or "broken pipe" in text - if exc.__class__.__name__ == "TransportClosedError": + return False + + +def _is_closed_turn_error(exc: BaseException) -> bool: + """Returns True when the live turn handle is already closed/dead. + + Wider than `_is_transport_death`: a steer against a finished turn also + surfaces as a plain RuntimeError, and there the cost of a false positive + is only a refused steer. + """ + if _is_transport_death(exc): return True if isinstance(exc, RuntimeError): text = str(exc).lower() @@ -1917,6 +1955,33 @@ async def run_codex_sdk_turn( def abort_requested() -> bool: return bool(should_abort and should_abort()) + def stop_requested() -> bool: + """True when Möbius, not the provider, ended this turn. + + The single definition of "we did this to ourselves", shared by terminal + validation (which sees a clean TurnStatus.interrupted) and the except + path (which sees the transport die mid-stream because force_stop killed + the turn's process group). Both need the same fact. + + Includes the superseded-generation abort: a newer turn taking over this + chat is still Möbius ending this one. That leg can be true before + `active_turn` exists, which is deliberate — a teardown during startup is + no more the provider's fault than one mid-stream. + """ + return bool( + (active_turn is not None and active_turn.interrupt_requested) + or abort_requested() + ) + + def with_usage(result: RunnerResult) -> RunnerResult: + """Attaches whatever the turn spent before it ended, however it ended.""" + if final_token_usage is not None: + result["usage"] = _model_dump(final_token_usage) + result["usage_metrics"] = normalize_codex_usage( + first_token_usage, final_token_usage, + ) + return result + def aborted_result() -> RunnerResult: return { "session_id": current_session_id, @@ -2295,33 +2360,48 @@ def aborted_result() -> RunnerResult: error_text, terminal_status, final_message_phase = _codex_terminal_error( completed_turn, sdk, - interrupt_requested=bool( - (active_turn and active_turn.interrupt_requested) - or abort_requested() - ), + interrupt_requested=stop_requested(), completed_message_phases=completed_message_phases, ) - result: RunnerResult = { + result: RunnerResult = with_usage({ "session_id": current_session_id, "cost_usd": None, "error": error_text, - } - if final_token_usage is not None: - result["usage"] = _model_dump(final_token_usage) - result["usage_metrics"] = normalize_codex_usage( - first_token_usage, final_token_usage, - ) + }) if terminal_status is not None: result["terminal_status"] = terminal_status if final_message_phase is not None: result["final_message_phase"] = final_message_phase return result except Exception as exc: - return { + if _is_transport_death(exc) and stop_requested(): + # Our own teardown, seen from the inside. A stop interrupts the turn; + # when that times out the escalation SIGTERMs the turn's private + # process group, so the transport dies mid-stream instead of + # delivering turn/completed. Surfacing that as a provider failure is + # both wrong and destructive: the raw string overwrites the stall note + # (chat._pause_note) published moments earlier, because error blocks + # coalesce latest-wins and drop every events.ERROR_PASSTHROUGH_FIELDS + # the new event omits — taking the note's one-tap Resume with it and + # leaving the owner an unexplained error and no way back. + # + # Expected, so INFO — but the transport's dying words are the only + # forensics a kill leaves behind, and nothing downstream logs them + # once `error` is None. + log.info( + "Codex transport closed by our own stop chat_id=%s: %s", chat_id, exc, + ) + return with_usage({ + "session_id": current_session_id, + "cost_usd": None, + "error": None, + "terminal_status": _enum_wire_value(sdk["TurnStatus"].interrupted), + }) + return with_usage({ "session_id": current_session_id, "cost_usd": None, "error": str(exc), - } + }) finally: deferred_cancel: asyncio.CancelledError | None = None if process_group_capture_stop is not None: diff --git a/backend/tests/test_codex_sdk_contract.py b/backend/tests/test_codex_sdk_contract.py index 402ad9caf..218e530a3 100644 --- a/backend/tests/test_codex_sdk_contract.py +++ b/backend/tests/test_codex_sdk_contract.py @@ -239,3 +239,21 @@ def test_reasoning_effort_enum_tolerates_unknown_efforts(): for value in ("high", "xhigh", "max", "ultra", "some-future-effort"): assert ReasoningEffort(value).value == value + + +def test_transport_closed_error_is_still_exposed_by_the_sdk(): + """_is_transport_death decides whether an error reaches the owner at all. + + It binds to this class by isinstance, and a stop that SIGTERMs the turn's + process group is reclassified as a clean interrupt only when the raised + exception is one. If a future SDK renames or drops the symbol, the runner + would stop recognizing its own teardown and go back to publishing a raw + provider error over the pause note — so fail here first. + """ + pytest.importorskip("openai_codex") + from openai_codex.errors import CodexError, TransportClosedError + + assert issubclass(TransportClosedError, CodexError) + # Not a RuntimeError: the runner relies on that to keep provider + # ErrorNotifications (reraised as plain RuntimeErrors) out of this branch. + assert not issubclass(TransportClosedError, RuntimeError) diff --git a/backend/tests/test_codex_sdk_runner.py b/backend/tests/test_codex_sdk_runner.py index 0404459c4..0cdf0dcd6 100644 --- a/backend/tests/test_codex_sdk_runner.py +++ b/backend/tests/test_codex_sdk_runner.py @@ -17,6 +17,18 @@ # - CodexRpcError: /usr/local/lib/python3.12/site-packages/openai_codex/errors.py:24 # - InvalidParamsError: /usr/local/lib/python3.12/site-packages/openai_codex/errors.py:40 +try: + from openai_codex.errors import ( + TransportClosedError as _SdkTransportClosedError, + ) +except ImportError: # pragma: no cover - SDK is an optional install + # openai-codex is installed in its own Docker step, not via + # requirements.txt, so this module must still import without it. The real + # symbol's continued existence is pinned by test_codex_sdk_contract, which + # is where its disappearance should be reported. + class _SdkTransportClosedError(Exception): + pass + class _FakeBroadcast: def __init__(self): @@ -177,6 +189,7 @@ def __init__(self, code: int, message: str, data=None): "ReasoningSummaryTextDeltaNotification": _Dummy, "ReasoningTextDeltaNotification": _Dummy, "ThreadTokenUsageUpdatedNotification": _Dummy, + "TransportClosedError": _SdkTransportClosedError, "TurnCompletedNotification": _FakeTurnCompletedNotification, "TurnStatus": _FakeTurnStatus, "WebSearchThreadItem": _Dummy, @@ -568,6 +581,50 @@ def test_is_closed_turn_error_does_not_treat_arbitrary_oserror_as_closed(): assert codex_sdk_runner._is_closed_turn_error(OSError("disk full")) is False +def test_is_transport_death_rejects_provider_runtime_error_about_not_running( + monkeypatch, +): + """The narrow predicate is the whole reason the two exist separately. + + The runner reraises every non-retryable provider ErrorNotification as a + plain `RuntimeError(message)`, and those messages routinely say "is not + running". _is_closed_turn_error accepts that text because the only cost + of a false positive there is a refused steer; the except-path + reclassification must not, or a stop racing a genuine MCP failure would + swallow the only report the owner ever gets. Widening the guard back to + _is_closed_turn_error fails here. + """ + sdk = _fake_sdk(async_codex_cls=object) + monkeypatch.setattr(codex_sdk_runner, "_sdk_imports", lambda: sdk) + + provider_fault = RuntimeError("MCP server 'x' is not running") + + assert codex_sdk_runner._is_transport_death(provider_fault) is False + assert codex_sdk_runner._is_closed_turn_error(provider_fault) is True + + +def test_is_transport_death_binds_to_the_sdk_class_not_to_its_name(): + """Uses the installed SDK, because the binding is what is under test. + + isinstance against the symbol `_sdk_imports()` exposes means a genuine + subclass matches and a same-named impostor does not — neither of which a + class-name comparison could get right. + """ + pytest.importorskip("openai_codex") + + class _MoreSpecificTransportError(_SdkTransportClosedError): + pass + + impostor = type("TransportClosedError", (Exception,), {}) + + assert codex_sdk_runner._is_transport_death( + _MoreSpecificTransportError("closed stdout") + ) is True + assert codex_sdk_runner._is_transport_death( + impostor("closed stdout") + ) is False + + def test_run_codex_sdk_turn_resume_mismatch_returns_error(monkeypatch): mismatched_thread = _FakeThread("actual-thread", _FakeTurnHandle()) @@ -1288,6 +1345,243 @@ def _mark_finished(self): assert mark_finished_calls == [True] +class _KilledTransportError(_SdkTransportClosedError): + """A subclass of the SDK's own TransportClosedError. + + Subclassing the real symbol rather than redeclaring its name is the point: + the runner reclassifies on isinstance, so a look-alike would prove nothing + and a genuine subclass would not match a name check. It is also not a + RuntimeError, which keeps the narrow transport branch distinguishable from + the looser bare-RuntimeError-text branch that belongs to + _is_closed_turn_error alone. + """ + + +def _run_turn_whose_stream_dies( + monkeypatch, + exc: Exception, + *, + on_register=None, + should_abort=None, + notifications=None, + sdk_patch=None, +): + """Runs one turn whose stream raises `exc`, optionally mid-teardown. + + `on_register` fires against the handle the runner has just registered — + the same window in which the real Stop / stall watchdog reaches a live + turn — so a test can mark the teardown as ours before the stream dies. + `notifications` are delivered before the death, so a test can give the + turn something to have spent; `sdk_patch` supplies the payload classes + those notifications need to be recognized as. + """ + turn_handle = _FakeTurnHandle(notifications, stream_exc=exc) + thread = _FakeThread("thread-1", turn_handle) + + class FakeAsyncCodex: + def __init__(self, config=None): + self.config = config + + async def __aenter__(self): + return self + + async def __aexit__(self, _exc_type, _exc, _tb): + return None + + async def thread_start(self, *_args, **_kwargs): + return thread + + sdk = _fake_sdk(FakeAsyncCodex) + sdk.update(sdk_patch or {}) + monkeypatch.setattr(codex_sdk_runner, "_sdk_imports", lambda: sdk) + + if on_register is not None: + original_register = registry.register + + def _register(handle): + on_register(handle) + return original_register(handle) + + monkeypatch.setattr(registry, "register", _register) + + bc = _FakeBroadcast() + result = asyncio.run( + codex_sdk_runner.run_codex_sdk_turn( + user_message="hello", + session_id=None, + base_env={}, + cwd="/tmp", + chat_id="chat-1", + bc=bc, + pending_questions={}, + db=None, + **({"should_abort": should_abort} if should_abort else {}), + ) + ) + return result, bc + + +def _mark_interrupted(handle): + handle._interrupt_requested = True + + +def test_run_codex_sdk_turn_reports_self_requested_kill_as_interrupted( + monkeypatch, +): + """A stop we asked for must not read as a provider failure. + + Stop and the stall watchdog both interrupt first and then, on timeout, + SIGTERM the turn's private process group — so the transport dies + mid-stream instead of delivering turn/completed. Reporting the resulting + "closed stdout" as an error is wrong twice over: it blames Codex for our + own teardown, and the error block overwrites the stop/stall note in the + transcript and strips its one-tap Resume. + """ + result, bc = _run_turn_whose_stream_dies( + monkeypatch, + _KilledTransportError("Codex process closed stdout. stderr_tail="), + on_register=_mark_interrupted, + ) + + assert result["error"] is None + assert result["terminal_status"] == "interrupted" + assert [e for e in bc.events if e.get("type") == "error"] == [] + assert registry.get_handle("chat-1", RunnerKind.CODEX_SDK) is None + + +def test_run_codex_sdk_turn_unrequested_transport_death_stays_an_error( + monkeypatch, +): + """The same transport death with no stop pending is still a real failure. + + Only a teardown we asked for may be reclassified, so a provider-side + crash keeps its error. + """ + result, _bc = _run_turn_whose_stream_dies( + monkeypatch, + _KilledTransportError("Codex process closed stdout. stderr_tail="), + ) + + assert result["error"] == "Codex process closed stdout. stderr_tail=" + assert result.get("terminal_status") is None + assert registry.get_handle("chat-1", RunnerKind.CODEX_SDK) is None + + +def test_run_codex_sdk_turn_non_transport_failure_during_stop_stays_an_error( + monkeypatch, +): + """A pending stop must not launder an unrelated bug into a clean stop. + + Without this, any defect that happens to fire inside a stop window + returns "interrupted" with no error and no log line — invisible in both + the transcript and chat.log. + """ + result, _bc = _run_turn_whose_stream_dies( + monkeypatch, + ValueError("unexpected notification payload"), + on_register=_mark_interrupted, + ) + + assert result["error"] == "unexpected notification payload" + assert result.get("terminal_status") is None + + +def test_run_codex_sdk_turn_provider_fault_during_stop_keeps_its_error( + monkeypatch, +): + """A provider fault that merely looks closed-ish must still be reported. + + Non-retryable provider errors reach this except path as plain + RuntimeErrors carrying the app-server's own words, and "is not running" + is ordinary phrasing for a broken MCP server. Only the transport itself + dying may be reclassified: if the guard is widened to + _is_closed_turn_error, a stop in flight turns a real failure into a + silent clean interrupt. + """ + result, _bc = _run_turn_whose_stream_dies( + monkeypatch, + RuntimeError("MCP server 'x' is not running"), + on_register=_mark_interrupted, + ) + + assert result["error"] == "MCP server 'x' is not running" + assert result.get("terminal_status") is None + + +def test_run_codex_sdk_turn_failed_turn_still_reports_what_it_spent( + monkeypatch, +): + """Tokens burned before a crash are still tokens burned. + + Every other exit routes its usage through with_usage; the raw error exit + used to drop it, so a turn that streamed for minutes and then died looked + free in the budget ledger. + """ + class TokenUsageUpdated: + def __init__(self, token_usage): + self.token_usage = token_usage + + class Usage: + def __init__(self, total_tokens): + self.last = SimpleNamespace( + input_tokens=200, cached_input_tokens=100, output_tokens=100, + reasoning_output_tokens=50, total_tokens=300, + ) + self.total = SimpleNamespace( + input_tokens=1_000, cached_input_tokens=400, output_tokens=100, + reasoning_output_tokens=50, total_tokens=total_tokens, + ) + self.model_context_window = 200_000 + + def model_dump(self, **_kwargs): + return { + "last": vars(self.last), + "total": vars(self.total), + "modelContextWindow": self.model_context_window, + } + + result, _bc = _run_turn_whose_stream_dies( + monkeypatch, + ValueError("unexpected notification payload"), + notifications=[ + SimpleNamespace( + method="thread/tokenUsage/updated", + payload=TokenUsageUpdated(Usage(1_100)), + ), + ], + sdk_patch={"ThreadTokenUsageUpdatedNotification": TokenUsageUpdated}, + ) + + assert result["error"] == "unexpected notification payload" + assert result["usage"]["total"]["total_tokens"] == 1_100 + # One update means no thread delta to compute, so the exact numbers are + # normalize_codex_usage's business; what this test owns is that the error + # exit carries the metrics at all. + assert "usage_metrics" in result + + +def test_run_codex_sdk_turn_superseded_generation_kill_is_interrupted( + monkeypatch, +): + """A newer turn taking over the chat is also Möbius ending this one. + + This leg needs no ActiveCodexTurn at all, so it is the one that reaches + a teardown during startup — pinned here so a later narrowing of + stop_requested() to the interrupt flag alone goes red. + """ + superseded = {"value": False} + + result, _bc = _run_turn_whose_stream_dies( + monkeypatch, + _KilledTransportError("Codex process closed stdout. stderr_tail="), + on_register=lambda _handle: superseded.update(value=True), + should_abort=lambda: superseded["value"], + ) + + assert result["error"] is None + assert result["terminal_status"] == "interrupted" + + def test_run_codex_sdk_turn_error_notification_will_retry_continues(monkeypatch): class AgentMessageDeltaNotification: def __init__(self, delta: str): diff --git a/backend/tests/test_runner_registry_integration.py b/backend/tests/test_runner_registry_integration.py index 47066af94..e3ea6cd0b 100644 --- a/backend/tests/test_runner_registry_integration.py +++ b/backend/tests/test_runner_registry_integration.py @@ -152,6 +152,7 @@ async def thread_start(self, *_args, **_kwargs): "AsyncCodex": FakeAsyncCodex, "CodexConfig": lambda **kwargs: SimpleNamespace(**kwargs), "CodexRpcError": RuntimeError, + "TransportClosedError": type("TransportClosedError", (Exception,), {}), "CommandExecutionOutputDeltaNotification": type( "CommandExecutionOutputDeltaNotification", (), {} ), From 699bd61e7388f1d0b1c7a3812f14bcfe62f59ce9 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:33:11 +0000 Subject: [PATCH 03/15] Cite the notes a turn recalled from memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent --- backend/app/chat.py | 47 ++++ backend/app/chat_transcript.py | 37 ++- backend/app/events.py | 19 +- backend/app/memory_recall.py | 255 ++++++++++++++++++ backend/tests/test_memory_recall.py | 209 ++++++++++++++ frontend/src/components/ChatView/ChatView.css | 45 ++++ .../components/ChatView/MessageSources.jsx | 119 +++++++- .../src/components/ChatView/MsgContent.jsx | 9 +- .../__tests__/accessibilityHardening.test.js | 36 ++- .../ChatView/__tests__/memoryRecall.test.js | 120 +++++++++ .../src/components/ChatView/memoryRecall.js | 104 +++++++ .../src/components/ChatView/streamReducers.js | 5 + .../components/ChatView/toolActivityLabel.js | 33 ++- .../ChatView/useStreamConnection.js | 4 + 14 files changed, 1025 insertions(+), 17 deletions(-) create mode 100644 backend/app/memory_recall.py create mode 100644 backend/tests/test_memory_recall.py create mode 100644 frontend/src/components/ChatView/__tests__/memoryRecall.test.js create mode 100644 frontend/src/components/ChatView/memoryRecall.js diff --git a/backend/app/chat.py b/backend/app/chat.py index 380913f56..4af9f2973 100644 --- a/backend/app/chat.py +++ b/backend/app/chat.py @@ -78,6 +78,7 @@ process_event, undo_question_scrub, ) +from app.memory_recall import recall_from_command, recall_from_output from app.providers import effective_agent_settings, get_provider, get_skill_path from app.runner_registry import registry from app.runtime_types import ChatEvent @@ -389,6 +390,44 @@ def _log_if_failed(fut, _kind=type(cmd).__name__, _cid=self.chat_id): ack.add_done_callback(_log_if_failed) + def _tool_was_memory_recall(self, tool_use_id) -> bool: + """Whether this tool was identified as a Memory lookup at input time. + + Read-only by design: resolving the block is a question, not the place to + adopt a legacy id (`process_event` still owns that a moment later). Only a + tool whose COMMAND named memory_search may go on to cite notes, so output + text alone can never mint a citation. + """ + for blk in reversed(self.assistant_blocks): + if blk.get("type") != "tool": + continue + if tool_use_id: + if blk.get("tool_use_id") == tool_use_id: + return isinstance(blk.get("recall"), dict) + continue + # Legacy events without an id: the newest still-open tool is the only + # safe candidate, matching `_tool_block_for_event`'s fallback. + if blk.get("status") != "done": + return isinstance(blk.get("recall"), dict) + return False + + def _stamp_memory_recall(self, event: ChatEvent) -> None: + """Name a Memory-app recall on the event, in two lifecycle phases. + + At input time the command identifies the lookup, so the live turn can say + it is remembering while the search still runs. At output time that same + tool's stdout is parsed into the notes it actually returned — including the + honest "found nothing" outcome, which is otherwise indistinguishable from + never having looked. + """ + if event.get("type") == "tool_input": + recall = recall_from_command(event.get("input")) + if recall is not None: + event["recall"] = recall + return + if self._tool_was_memory_recall(event.get("tool_use_id")): + event["recall"] = recall_from_output(event.get("content")) + def _reduce_tool_output(self, event: ChatEvent) -> None: """Move a large tool_output's full text OFF the wire (contract rule 6). @@ -505,6 +544,14 @@ def publish(self, event: ChatEvent) -> bool: # its full text BEFORE process_event (which copies content onto the block) # and before the broadcast below, so the rewritten event is the single # source feeding the persisted block, the live wire, and the catch-up log. + # + # Memory recall is stamped just BEFORE that reduction, for the same reason + # and on the same funnel: a full lookup's stdout exceeds the inline + # threshold, so parsing after rule-6 carving would silently lose the note + # titles that make a citation readable. One stamp here reaches the persisted + # block, the live wire, and the catch-up log alike, for both runners. + if event_type in ("tool_input", "tool_output"): + self._stamp_memory_recall(event) if event_type == "tool_output": self._reduce_tool_output(event) if event_type == "thinking": diff --git a/backend/app/chat_transcript.py b/backend/app/chat_transcript.py index 4da520fbd..b2f953840 100644 --- a/backend/app/chat_transcript.py +++ b/backend/app/chat_transcript.py @@ -4,6 +4,8 @@ import re +from app.memory_recall import RECALL_HIT, merge_recall_notes + _QUESTION_TOOLS = {"AskUserQuestion", "request_user_input"} _IMAGE_PATH_RE = re.compile( @@ -116,7 +118,14 @@ def historical_tool_output_ids( def _distinctive_activity(block: dict) -> bool: """Keep notable one-line activity beats out of a folded metadata run.""" - if block.get("type") != "tool" or block.get("tool") != "Read": + if block.get("type") != "tool": + return False + # Consulting Memory is a beat worth seeing on its own, not shell housekeeping + # folded into "ran commands". The marker was stamped from the command itself, + # so this needs no knowledge of how that command is spelled. + if isinstance(block.get("recall"), dict): + return True + if block.get("tool") != "Read": return False raw = block.get("input") if isinstance(raw, dict): @@ -161,6 +170,11 @@ def _compact_activity_item(block: dict) -> dict: # tool input remains in the on-demand activity detail. if block.get("tool") == "Read" and isinstance(block.get("input"), str): tool["input"] = block["input"][:2048] + # A Memory recall is already a bounded citation set, and it is what the + # collapsed line says ("Recalled 4 notes from Memory"). Dropping it here + # would make the beat visible live and gone on the next chat load. + if isinstance(block.get("recall"), dict): + tool["recall"] = block["recall"] return tool @@ -242,6 +256,23 @@ def _compact_activity_run( if len(sources) >= _MAX_COMPACT_SOURCES: break + # Memory citations roll up for the same reason web sources do: the message + # renders them once per turn, so they must outlive the individual tool blocks + # this projection folds away. `_compact_activity_entries` keeps only two + # entries per tool name, so without this a third lookup's notes would vanish. + recall_notes: list[dict] = [] + seen_recall_paths: set[str] = set() + recall_status = "" + for _, block in blocks: + recall = block.get("recall") + if not isinstance(recall, dict): + continue + # Any lookup that returned something outranks one that came back empty: + # the turn did remember, even if another probe found nothing. + if recall_status != RECALL_HIT: + recall_status = recall.get("status") or recall_status + merge_recall_notes(recall_notes, seen_recall_paths, recall) + start = blocks[0][0] end = blocks[-1][0] + 1 return { @@ -255,6 +286,10 @@ def _compact_activity_run( block.get("type") == "tool" for _, block in blocks ), **({"sources": sources} if sources else {}), + **( + {"recall": {"status": recall_status, "notes": recall_notes}} + if recall_status else {} + ), } diff --git a/backend/app/events.py b/backend/app/events.py index 2aee549d7..ab69bdf04 100644 --- a/backend/app/events.py +++ b/backend/app/events.py @@ -581,12 +581,21 @@ def process_event(event: dict, assistant_blocks: list) -> bool: # Backfill the input summary. Prefer an exact tool_use_id match (Codex # backfills a WebSearch query at completion, when several searches may be # in flight); older id-less events retain the earliest input-less fallback. + def _apply_tool_input(blk: dict) -> None: + blk["input"] = event.get("input", "") + # A Memory lookup is identified from the command it runs (stamped on the + # event by the sink). Carrying that marker onto the block is what lets the + # turn name the recall while it is still running, and is the ONLY thing + # that authorizes the later output phase to cite notes. + if isinstance(event.get("recall"), dict): + blk["recall"] = event["recall"] + tool_use_id = event.get("tool_use_id") if tool_use_id: for blk in assistant_blocks: if (blk.get("type") == "tool" and blk.get("tool_use_id") == tool_use_id): - blk["input"] = event.get("input", "") + _apply_tool_input(blk) return True candidates = [ blk for blk in assistant_blocks @@ -596,12 +605,12 @@ def process_event(event: dict, assistant_blocks: list) -> bool: ] if len(candidates) == 1: candidates[0]["tool_use_id"] = tool_use_id - candidates[0]["input"] = event.get("input", "") + _apply_tool_input(candidates[0]) return True return False for blk in assistant_blocks: if blk.get("type") == "tool" and not blk.get("input"): - blk["input"] = event.get("input", "") + _apply_tool_input(blk) break return True @@ -621,6 +630,10 @@ def process_event(event: dict, assistant_blocks: list) -> bool: exit_code = event.get("output_exit_code") if exit_code is not None: blk["output_exit_code"] = exit_code + # Settle a Memory lookup from "searching" to what it actually recalled. + # The sink parsed this from the FULL output, before the carving above. + if isinstance(event.get("recall"), dict): + blk["recall"] = event["recall"] return True return False diff --git a/backend/app/memory_recall.py b/backend/app/memory_recall.py new file mode 100644 index 000000000..f00ab9a70 --- /dev/null +++ b/backend/app/memory_recall.py @@ -0,0 +1,255 @@ +"""Recognize Memory-app recall lookups so a turn can cite what it remembered. + +The Memory app is an ordinary installed app: the agent consults it by running +``memory_search.py`` through Bash, and the notes it read come back as ordinary +tool output. Without this module that lookup is indistinguishable from any +other shell command, so the owner cannot tell "it remembered something" from +"it ran housekeeping" — nor, more importantly, "it looked and found nothing" +from "it never looked". + +Detection is deliberately two-phase and keyed off the tool's own lifecycle +rather than the shape of its output: + +* ``recall_from_command`` matches the *command being run*. This is the only + positive identification; nothing here ever concludes "this was a memory + lookup" from output text alone, so an unrelated command that happens to + print ``FILES:`` can never mint a citation. +* ``recall_from_output`` is then trusted to parse that command's stdout, and is + only ever called for a tool already identified by the first phase. + +The stdout contract belongs to the Memory app (``memory_search.py``'s +``retrieve``/``run``) and is stable:: + + Relevant memories: + - : <excerpt> [<relative/path.md>] + - <title>: <excerpt> [<relative/path.md>] + FILES: notes/a.md, notes/b.md + +or, for a lookup that matched nothing:: + + No relevant memories. + +``FILES:`` is authoritative: those paths were opened by confined Python after +the graph commit was pinned, whereas the ``- title: excerpt [path]`` lines are +presentation. So paths come from ``FILES:`` and the section lines only enrich +them with a title and excerpt. That split also makes the parse robust to the +head+tail carving a large tool output receives (``events.py`` +``excerpt_tool_output``): both markers sit at the very start and very end of +the stream, so a carved middle costs some titles but never the citation set. + +Every failure mode degrades to *less* metadata, never to a wrong citation: +an unparseable body still yields a bounded "hit" with path-derived titles, and +a missing command summary yields no recall at all. +""" + +from __future__ import annotations + +import re + +# Recall metadata rides inline on the SSE event, the persisted tool block, and +# the compacted activity summary — the same budget the web-source citations +# live within. memory_search.py itself returns at most 6 files with 900-char +# excerpts; these ceilings leave headroom without letting a malformed or +# hostile stdout inflate every transcript read. +MAX_RECALL_NOTES = 12 +MAX_RECALL_TITLE_CHARS = 120 +MAX_RECALL_EXCERPT_CHARS = 300 +MAX_RECALL_PATH_CHARS = 256 +_MAX_OUTPUT_SCAN_CHARS = 262_144 +_MAX_SECTION_LINES_SCANNED = 256 + +RECALL_SEARCHING = "searching" +RECALL_HIT = "hit" +RECALL_EMPTY = "empty" + +# The command summary the backend builds for Bash is the verbatim command +# string, so identification means answering "did this command RUN the search +# script?" — not "does this command mention it?". Substring matching gets that +# wrong in the most ordinary way possible: `grep -rn memory_search.py …` and +# `cat memory_search.py` both name the script while doing something else +# entirely, and either would mint a citation from unrelated output. +# +# So the command is split into segments and each segment's HEAD is inspected: +# a lookup is either the script executed directly or an interpreter invoked on +# it. Anything where the script is a mere argument is correctly ignored. +_MAX_COMMAND_SCAN_CHARS = 8192 +_SEGMENT_RE = re.compile(r"[;&|\n]+") +_ENV_ASSIGN_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") +_INTERPRETER_RE = re.compile(r"^(?:.*/)?python[0-9.]*$") +_SCRIPT_RE = re.compile(r"^(?:.*/)?memory_search\.py$") + +_EMPTY_RE = re.compile(r"^No relevant memories\.\s*$", re.MULTILINE) +_FILES_RE = re.compile(r"^FILES:[ \t]*(.+)$", re.MULTILINE) +# "- <title>: <excerpt> [<path>]" — the excerpt is greedy-free and the path is +# anchored to the end of the line, matching how memory_search.py builds it. +_SECTION_RE = re.compile(r"^- (.*?): (.*) \[([^\]]+)\]$", re.MULTILINE) + +# A citation path is only ever a repository-relative markdown pointer. Refusing +# anything else keeps traversal, absolute paths, and control characters out of +# a value the client turns into a deep link. +_PATH_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]*\.md$") + + +def _clean(value: str, limit: int) -> str: + """Collapse whitespace and bound a label taken from tool output.""" + if not isinstance(value, str): + return "" + # Slice before normalizing so a pathological line cannot allocate another + # full-size string merely to produce a short label. + return re.sub(r"\s+", " ", value[: limit * 2]).strip()[:limit] + + +def _note_id(path: str) -> str: + """The graph node id for a citation path: its file stem.""" + tail = path.rsplit("/", 1)[-1] + return tail[:-3] if tail.endswith(".md") else tail + + +def _title_from_path(path: str) -> str: + """A readable fallback when the titled section line was carved away.""" + return _note_id(path).replace("-", " ").replace("_", " ").strip() + + +def _safe_path(value: str) -> str: + if not isinstance(value, str): + return "" + candidate = value.strip() + if not candidate or len(candidate) > MAX_RECALL_PATH_CHARS: + return "" + if ".." in candidate or candidate.startswith("/"): + return "" + return candidate if _PATH_RE.match(candidate) else "" + + +def _segment_runs_search(segment: str) -> bool: + """Whether one command segment EXECUTES the memory search script.""" + tokens = [token.strip("'\"") for token in segment.split()] + tokens = [token for token in tokens if token] + # `FOO=bar python3 script.py` — leading environment assignments are not the + # command being run. + index = 0 + while index < len(tokens) and _ENV_ASSIGN_RE.match(tokens[index]): + index += 1 + if index >= len(tokens): + return False + head = tokens[index] + if _SCRIPT_RE.match(head): + return True + if not _INTERPRETER_RE.match(head): + return False + # `python3 -u script.py` — interpreter flags may precede the script, but the + # first non-flag token is the thing actually being run. + for token in tokens[index + 1:]: + if token.startswith("-"): + continue + return bool(_SCRIPT_RE.match(token)) + return False + + +def recall_from_command(command: object) -> dict | None: + """Return a pending recall marker when this command RUNS a memory lookup. + + Called at tool-input time so the live turn can name what it is doing while + the lookup is still in flight. Returning ``None`` means "not a memory + lookup", which is also the safe answer for a missing or oversized command + summary — and, deliberately, for any command that merely names the script. + """ + if not isinstance(command, str) or not command: + return None + if len(command) > _MAX_COMMAND_SCAN_CHARS: + return None + # Cheap reject before tokenizing: the overwhelming majority of commands are + # not memory lookups and should cost one substring scan. + if "memory_search.py" not in command: + return None + for segment in _SEGMENT_RE.split(command): + if _segment_runs_search(segment): + return {"status": RECALL_SEARCHING} + return None + + +def recall_from_output(text: object) -> dict: + """Parse a known memory lookup's stdout into a bounded citation set. + + Only ever called for a tool ``recall_from_command`` already identified, so + an unrecognized body is a lookup whose output we could not read — not a + reason to claim nothing was found. That case returns a note-less ``hit``, + which renders as a plain "recalled from Memory" beat rather than the far + stronger (and possibly false) "nothing relevant" claim. + """ + if not isinstance(text, str) or not text.strip(): + # No output at all: the command produced nothing readable. Keep the beat, + # drop the claim. + return {"status": RECALL_HIT, "notes": []} + + body = text[:_MAX_OUTPUT_SCAN_CHARS] + + files_match = None + for files_match in _FILES_RE.finditer(body): + # The last FILES: line wins — a head+tail carve keeps the tail, and the + # real one is always the final line memory_search.py prints. + pass + + if files_match is None: + if _EMPTY_RE.search(body): + return {"status": RECALL_EMPTY} + return {"status": RECALL_HIT, "notes": []} + + titles: dict[str, tuple[str, str]] = {} + for index, section in enumerate(_SECTION_RE.finditer(body)): + if index >= _MAX_SECTION_LINES_SCANNED: + break + raw_title, raw_excerpt, raw_path = section.groups() + path = _safe_path(raw_path) + if path and path not in titles: + titles[path] = ( + _clean(raw_title, MAX_RECALL_TITLE_CHARS), + _clean(raw_excerpt, MAX_RECALL_EXCERPT_CHARS), + ) + + notes: list[dict[str, str]] = [] + seen: set[str] = set() + for raw_path in files_match.group(1).split(","): + path = _safe_path(raw_path) + if not path or path in seen: + continue + seen.add(path) + title, excerpt = titles.get(path, ("", "")) + note = { + "id": _note_id(path), + "path": path, + "title": title or _title_from_path(path) or path, + } + if excerpt: + note["excerpt"] = excerpt + notes.append(note) + if len(notes) >= MAX_RECALL_NOTES: + break + + # A FILES: line whose every entry failed validation is a malformed citation + # set, not an empty lookup — same conservative fallback as an unreadable body. + return {"status": RECALL_HIT, "notes": notes} + + +def merge_recall_notes( + target: list[dict[str, str]], + seen: set[str], + recall: object, +) -> None: + """Accumulate one block's notes into a deduped, bounded citation list. + + Shared by the transcript compaction rollup so the projection and the live + block agree on ordering (first occurrence owns the position) and on the cap. + """ + if not isinstance(recall, dict): + return + for note in recall.get("notes") or []: + if not isinstance(note, dict): + continue + path = note.get("path") + if not isinstance(path, str) or not path or path in seen: + continue + seen.add(path) + target.append(note) + if len(target) >= MAX_RECALL_NOTES: + return diff --git a/backend/tests/test_memory_recall.py b/backend/tests/test_memory_recall.py new file mode 100644 index 000000000..25d9c0962 --- /dev/null +++ b/backend/tests/test_memory_recall.py @@ -0,0 +1,209 @@ +"""Memory recall citations: identification, parsing, and survival on read. + +The behaviour under test is what lets an owner tell three states apart — +the turn recalled these notes / it looked and Memory had nothing / it never +looked. The third is the absence of a citation, so the tests below care as +much about what is NOT stamped as about what is. +""" + +from app.chat_transcript import ( + _compact_activity_item, + _compact_activity_run, + _distinctive_activity, +) +from app.events import process_event +from app.memory_recall import ( + MAX_RECALL_NOTES, + RECALL_EMPTY, + RECALL_HIT, + RECALL_SEARCHING, + recall_from_command, + recall_from_output, +) + +MEMORY_CMD = 'python3 /data/apps/memory/memory_search.py "what does he prefer" "chat-1"' + +# Synthetic notes. Fixtures here become a public diff, so they must never carry +# anything from a real owner's graph — a memory note is personal by definition. +HIT_OUTPUT = """Relevant memories: +- Apps render in a sandboxed frame: Each mini-app runs isolated. [notes/apps-render-in-a-sandboxed-frame.md] +- Theme variables are shared: Colors come from one stylesheet. [notes/theme-variables-are-shared.md] +FILES: notes/apps-render-in-a-sandboxed-frame.md, notes/theme-variables-are-shared.md""" + + +# --- identification ------------------------------------------------------- + +def test_a_memory_search_command_is_identified_as_a_lookup(): + assert recall_from_command(MEMORY_CMD) == {"status": RECALL_SEARCHING} + + +def test_a_command_merely_mentioning_memory_search_is_not_a_lookup(): + # Identification gates everything downstream, so a false positive here would + # mint citations from an unrelated command's output. + # Every one of these is an ordinary thing to do WHILE working on Memory, and + # each names the script without running it. + assert recall_from_command("grep -rn memory_search.py /data/platform") is None + assert recall_from_command("cat /data/apps/memory/memory_search.py") is None + assert recall_from_command("wc -l memory_search.py") is None + assert recall_from_command("ls -la /data/apps/memory/memory_search.py") is None + assert recall_from_command("vim memory_search.py") is None + assert recall_from_command("python3 -m py_compile app/memory_search.py") is None + assert recall_from_command("echo memory_search.python") is None + assert recall_from_command("ls /data/apps/memory/") is None + assert recall_from_command("") is None + assert recall_from_command(None) is None + + +def test_the_script_is_recognized_however_it_is_invoked(): + assert recall_from_command("memory_search.py 'q'") is not None + assert recall_from_command('cd /x && python3 ./memory_search.py "q"') is not None + assert recall_from_command('python3 -u "/a/b/memory_search.py" "q"') is not None + assert recall_from_command('MEMORY_READER_PROVIDER=none python3 /a/memory_search.py "q"') is not None + assert recall_from_command('/usr/bin/python3.12 /a/memory_search.py "q"') is not None + + +# --- parsing -------------------------------------------------------------- + +def test_a_successful_lookup_cites_the_notes_it_opened(): + recall = recall_from_output(HIT_OUTPUT) + assert recall["status"] == RECALL_HIT + assert [note["id"] for note in recall["notes"]] == [ + "apps-render-in-a-sandboxed-frame", "theme-variables-are-shared", + ] + assert recall["notes"][0]["title"] == "Apps render in a sandboxed frame" + assert recall["notes"][0]["excerpt"] == "Each mini-app runs isolated." + + +def test_a_lookup_that_found_nothing_says_so(): + assert recall_from_output("No relevant memories.") == {"status": RECALL_EMPTY} + + +def test_a_carved_output_keeps_every_citation_and_falls_back_on_titles(): + # A full lookup exceeds the inline threshold and is reduced to head+tail, so + # the titled section lines in the middle can be lost. The FILES: line is + # authoritative and sits in the tail, so the citation SET must survive whole. + carved = "Relevant memories:\n- A: b [notes/a.md]\n…\nFILES: notes/a.md, notes/z.md" + recall = recall_from_output(carved) + assert [note["id"] for note in recall["notes"]] == ["a", "z"] + assert recall["notes"][1]["title"] == "z", "a title-less note still reads" + + +def test_an_unreadable_body_never_claims_nothing_was_found(): + # "Nothing relevant" is a strong claim about the owner's memory. Only the + # app's own marker may make it; anything else degrades to a note-less beat. + for body in ("", " ", "some unrelated text", "Traceback (most recent call last):"): + recall = recall_from_output(body) + assert recall["status"] == RECALL_HIT + assert recall["notes"] == [] + + +def test_a_citation_path_may_not_escape_the_graph(): + recall = recall_from_output( + "FILES: ../../etc/passwd, /abs/x.md, notes/../secret.md, notes/ok.md" + ) + assert [note["path"] for note in recall["notes"]] == ["notes/ok.md"] + + +def test_repeated_and_excessive_citations_are_bounded(): + paths = ", ".join(f"notes/n{i}.md" for i in range(40)) + recall = recall_from_output(f"FILES: notes/dup.md, notes/dup.md, {paths}") + assert len(recall["notes"]) == MAX_RECALL_NOTES + assert recall["notes"][0]["path"] == "notes/dup.md" + assert len({note["path"] for note in recall["notes"]}) == len(recall["notes"]) + + +def test_the_last_files_line_wins_after_a_head_tail_carve(): + # A carve keeps the head, so a stale FILES: from the head could linger above + # the real one. memory_search.py always prints it last. + recall = recall_from_output( + "FILES: notes/stale.md\n…carved…\nRelevant memories:\nFILES: notes/real.md" + ) + assert [note["path"] for note in recall["notes"]] == ["notes/real.md"] + + +# --- the block carries it through persistence ------------------------------ + +def _tool_blocks(recall_in, recall_out, output=HIT_OUTPUT): + blocks: list = [] + process_event({"type": "tool_start", "tool": "Bash", "tool_use_id": "t1"}, blocks) + event_in = {"type": "tool_input", "tool_use_id": "t1", "input": MEMORY_CMD} + if recall_in is not None: + event_in["recall"] = recall_in + process_event(event_in, blocks) + event_out = {"type": "tool_output", "tool_use_id": "t1", "content": output} + if recall_out is not None: + event_out["recall"] = recall_out + process_event(event_out, blocks) + return blocks + + +def test_the_lookup_marker_reaches_the_persisted_block_and_then_settles(): + blocks = _tool_blocks( + {"status": RECALL_SEARCHING}, + {"status": RECALL_HIT, "notes": [{"id": "a", "path": "notes/a.md", "title": "A"}]}, + ) + assert blocks[0]["recall"]["status"] == RECALL_HIT + assert blocks[0]["recall"]["notes"][0]["id"] == "a" + + +def test_an_ordinary_command_gains_no_recall_field(): + blocks = _tool_blocks(None, None, output="total 0\n") + assert "recall" not in blocks[0] + + +# --- survival through the read-side projection ----------------------------- + +def test_consulting_memory_is_its_own_activity_beat(): + assert _distinctive_activity({"type": "tool", "tool": "Bash", + "recall": {"status": RECALL_HIT, "notes": []}}) + assert not _distinctive_activity({"type": "tool", "tool": "Bash"}) + + +def test_the_compacted_line_still_knows_what_it_recalled(): + # Without this the beat renders live and reverts to "Ran a command" on the + # next chat load, which is worse for trust than never having shown it. + item = _compact_activity_item({ + "type": "tool", "tool": "Bash", "status": "done", + "input": MEMORY_CMD, + "recall": {"status": RECALL_HIT, "notes": [{"id": "a", "path": "notes/a.md"}]}, + }) + assert item["recall"]["notes"][0]["id"] == "a" + + +def test_citations_roll_up_so_a_folded_run_keeps_them(): + # _compact_activity_entries keeps only two entries per tool name, so a third + # lookup's notes exist ONLY on the run summary. + blocks = [ + (i, {"type": "tool", "tool": "Bash", "status": "done", + "recall": {"status": RECALL_HIT, + "notes": [{"id": f"n{i}", "path": f"notes/n{i}.md"}]}}) + for i in range(3) + ] + run = _compact_activity_run(blocks, message_index=0) + assert [note["id"] for note in run["recall"]["notes"]] == ["n0", "n1", "n2"] + assert run["recall"]["status"] == RECALL_HIT + + +def test_a_run_with_no_lookup_carries_no_recall_key(): + blocks = [(0, {"type": "tool", "tool": "Bash", "status": "done"})] + assert "recall" not in _compact_activity_run(blocks, message_index=0) + + +def test_a_remembered_note_outranks_an_empty_probe_in_the_same_run(): + blocks = [ + (0, {"type": "tool", "tool": "Bash", "status": "done", + "recall": {"status": RECALL_EMPTY}}), + (1, {"type": "tool", "tool": "Bash", "status": "done", + "recall": {"status": RECALL_HIT, + "notes": [{"id": "a", "path": "notes/a.md"}]}}), + ] + run = _compact_activity_run(blocks, message_index=0) + assert run["recall"]["status"] == RECALL_HIT + assert [note["id"] for note in run["recall"]["notes"]] == ["a"] + + +def test_an_all_empty_run_still_reports_that_it_looked(): + blocks = [(0, {"type": "tool", "tool": "Bash", "status": "done", + "recall": {"status": RECALL_EMPTY}})] + run = _compact_activity_run(blocks, message_index=0) + assert run["recall"] == {"status": RECALL_EMPTY, "notes": []} diff --git a/frontend/src/components/ChatView/ChatView.css b/frontend/src/components/ChatView/ChatView.css index c24a6a020..3e5b91800 100644 --- a/frontend/src/components/ChatView/ChatView.css +++ b/frontend/src/components/ChatView/ChatView.css @@ -978,6 +978,51 @@ line-height: 1; } +/* A recalled note reads as the same KIND of citation as a web source, so it + shares the chip entirely and diverges only in its mark: a glyph rather than + a domain letter, tinted to separate "remembered" from "read on the web" + without becoming a second visual language. */ +.chat__source-glyph { + width: 13px; + height: 13px; +} + +/* A note title is prose, not a domain, so it needs more room than a host chip + and is the part that should absorb any shortfall. */ +.chat__source-chip--memory { + max-width: 320px; +} + +.chat__source-chip--memory .chat__source-icon { + color: color-mix(in srgb, var(--accent) 62%, var(--text)); + background: color-mix(in srgb, var(--accent-dim) 52%, var(--surface2)); +} + +/* "Memory" is a fixed six-character label; letting it share the shrink budget + with the title collapses it to "Me…" and says nothing. */ +.chat__source-chip--memory .chat__source-host { + flex: 0 0 auto; +} + +/* An empty lookup is a fact about the answer, not a destination — so it is + stated plainly and styled back, with no hover or pointer affordance. */ +.chat__source-chip--quiet { + max-width: none; + color: var(--muted); /* CONTRACT: low-contrast text on opaque fill */ + background: color-mix(in srgb, var(--surface2) 34%, var(--surface)); + border-style: dashed; +} + +.chat__source-chip--quiet .chat__source-icon { + color: var(--muted); /* CONTRACT: low-contrast text on opaque fill */ + background: color-mix(in srgb, var(--surface2) 60%, var(--surface)); + border-color: transparent; +} + +.chat__source-chip--quiet .chat__source-title { + font-style: italic; +} + .chat__source-copy { display: flex; align-items: baseline; diff --git a/frontend/src/components/ChatView/MessageSources.jsx b/frontend/src/components/ChatView/MessageSources.jsx index 7150abc45..d32f0535d 100644 --- a/frontend/src/components/ChatView/MessageSources.jsx +++ b/frontend/src/components/ChatView/MessageSources.jsx @@ -1,26 +1,131 @@ import { messageSources, sourceHost, sourceLabel } from './messageSources.js' +import { messageRecall, noteHref, noteLabel } from './memoryRecall.js' function sourceMark(host) { const displayHost = String(host || '').replace(/^www\./i, '') return displayHost.match(/[a-z0-9]/i)?.[0]?.toUpperCase() || '•' } -// The web sources that informed an answer, surfaced ONCE at the end of the -// message — see messageSources.js for where the data comes from and why it is -// derived rather than carried as its own content block. +// A recalled note is not a website, so it gets a mark of its own rather than a +// domain letter. Local and inline for the same reason the web chip's mark is: +// nothing about viewing an answer should contact a remote server. +function MemoryMark() { + return ( + <svg + className="chat__source-glyph" + viewBox="0 0 16 16" + aria-hidden="true" + focusable="false" + > + <path + d="M8 2.2c-1.5 0-2.6 1-2.8 2.3-1.1.3-1.9 1.2-1.9 2.4 0 .5.2 1 .4 1.4-.3.4-.5.9-.5 1.5 0 1.4 1.2 2.5 2.7 2.5.5 0 1-.1 1.4-.4.2.5.4.9.7 1.2V2.2z" + fill="currentColor" + opacity="0.85" + /> + <path + d="M8 2.2c1.5 0 2.6 1 2.8 2.3 1.1.3 1.9 1.2 1.9 2.4 0 .5-.2 1-.4 1.4.3.4.5.9.5 1.5 0 1.4-1.2 2.5-2.7 2.5-.5 0-1-.1-1.4-.4-.2.5-.4.9-.7 1.2V2.2z" + fill="currentColor" + opacity="0.55" + /> + </svg> + ) +} + +// Everything that informed an answer, surfaced ONCE at the end of the message: +// the notes the agent recalled from Memory, then the web sources it read. See +// memoryRecall.js / messageSources.js for where each comes from and why both +// are derived rather than carried as their own content blocks. // -// Message level rather than inside the tool row, because a source is a +// Message level rather than inside the tool row, because a citation is a // property of the ANSWER, not of the individual search that happened to find // it: collapsed tool rows hid them, and one search's results are rarely the // whole citation set. +// +// The recall row exists to make three states distinguishable at a glance — +// remembered these notes / looked and found nothing / never looked (no row). +// The middle state is the one that earns trust, and it is also the prompt to +// write the note that was missing. -export default function MessageSources({ blocks }) { +export default function MessageSources({ blocks, onInternalNav }) { const sources = messageSources(blocks) - if (sources.length === 0) return null + const recall = messageRecall(blocks) + const notes = recall?.notes || [] + if (sources.length === 0 && !recall) return null + + const handleNoteClick = (event, href) => { + if (!onInternalNav || !href) return + // Let the browser own a modified click (new tab, download, middle button) + // exactly as it does for an ordinary link. + if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey + || event.button !== 0) return + let url + try { + url = new URL(href, window.location.href) + } catch { + return + } + event.preventDefault() + onInternalNav(url) + } return ( - <section className="chat__sources" aria-label="Source links"> + <section className="chat__sources" aria-label="What informed this answer"> <ul className="chat__sources-list"> + {notes.map(note => { + const label = noteLabel(note) + const href = noteHref(note) + const body = ( + <> + <span className="chat__source-icon" aria-hidden="true"> + <MemoryMark /> + </span> + <span className="chat__source-copy"> + <span className="chat__source-title">{label}</span> + <span className="chat__source-host" aria-hidden="true"> + Memory + </span> + </span> + </> + ) + return ( + <li key={note.path || note.id} className="chat__source-item"> + {href ? ( + <a + className="chat__source-chip chat__source-chip--memory" + href={href} + title={note.excerpt || label} + aria-label={`${label} — recalled from Memory`} + onClick={event => handleNoteClick(event, href)} + > + {body} + </a> + ) : ( + <span + className="chat__source-chip chat__source-chip--memory" + title={note.excerpt || label} + > + {body} + </span> + )} + </li> + ) + })} + {/* A lookup that came back empty. Deliberately quiet and unclickable: + it is a fact about the answer, not a destination. */} + {recall?.empty && ( + <li className="chat__source-item"> + <span className="chat__source-chip chat__source-chip--memory chat__source-chip--quiet"> + <span className="chat__source-icon" aria-hidden="true"> + <MemoryMark /> + </span> + <span className="chat__source-copy"> + <span className="chat__source-title"> + Looked back — nothing on this yet + </span> + </span> + </span> + </li> + )} {sources.map(source => { const label = sourceLabel(source) const host = sourceHost(source.url) diff --git a/frontend/src/components/ChatView/MsgContent.jsx b/frontend/src/components/ChatView/MsgContent.jsx index 4abd666ef..4dfcd04dc 100644 --- a/frontend/src/components/ChatView/MsgContent.jsx +++ b/frontend/src/components/ChatView/MsgContent.jsx @@ -324,11 +324,12 @@ function MsgContentInner({ } return renderBlock(node.single.item, node.single.idx) })} - {/* The turn's web sources, collected from its tool blocks and shown - once after the answer. Renders nothing when the turn did no web - search, so an ordinary reply is unchanged. */} + {/* What informed the turn — notes recalled from Memory, then web + sources — collected from its tool blocks and shown once after the + answer. Renders nothing when the turn neither searched the web nor + consulted Memory, so an ordinary reply is unchanged. */} {msg.role === 'assistant' && !isStreaming && ( - <MessageSources blocks={msg.blocks} /> + <MessageSources blocks={msg.blocks} onInternalNav={onInternalNav} /> )} </> ) diff --git a/frontend/src/components/ChatView/__tests__/accessibilityHardening.test.js b/frontend/src/components/ChatView/__tests__/accessibilityHardening.test.js index 9e7c218c2..3d7cb838d 100644 --- a/frontend/src/components/ChatView/__tests__/accessibilityHardening.test.js +++ b/frontend/src/components/ChatView/__tests__/accessibilityHardening.test.js @@ -75,7 +75,10 @@ test('message sources expose list semantics, keyboard focus, and touch targets', const msgContent = read('../MsgContent.jsx') const css = read('../ChatView.css') - assert.match(source, /<section className="chat__sources" aria-label="Source links">/) + // The section carries both web sources and notes recalled from Memory, so + // its accessible name names the shared idea rather than only the web half. + assert.match(source, + /<section className="chat__sources" aria-label="What informed this answer">/) assert.doesNotMatch(source, />Sources</, 'source links should stand on their own at the end of the answer') assert.match(source, /<ul className="chat__sources-list">/) @@ -93,3 +96,34 @@ test('message sources expose list semantics, keyboard focus, and touch targets', assert.match(css, /\.chat__source-chip\s*\{[^}]*border-radius:\s*999px/s) assert.match(css, /@media\s*\(pointer:\s*coarse\)\s*\{\s*\.chat__source-chip\s*\{\s*min-height:\s*44px/s) }) + +test('recalled memory notes are named, navigable in-shell, and honest when empty', () => { + const source = read('../MessageSources.jsx') + + // A recalled note reads as a citation, not a bare file name: its accessible + // name says where it came from, since "Memory" is only a visual sibling chip. + assert.match(source, /aria-label=\{`\$\{label\} — recalled from Memory`\}/) + assert.match(source, /<li key=\{note\.path \|\| note\.id\} className="chat__source-item">/) + + // A note lives inside this Möbius instance, so it navigates the shell rather + // than opening a tab. Guarding the absence of target=_blank on the memory + // chip keeps that from being "fixed" back into the web-source shape. + assert.doesNotMatch( + source, + /chat__source-chip--memory"[\s\S]{0,200}target="_blank"/, + 'a recalled note opens in the workspace, not a new browser tab', + ) + assert.match(source, /onClick=\{event => handleNoteClick\(event, href\)\}/) + // A modified click must stay the browser's to handle. + assert.match(source, /event\.metaKey \|\| event\.ctrlKey \|\| event\.shiftKey \|\| event\.altKey/) + + // The empty outcome is the whole point of the row: it separates "looked and + // found nothing" from "never looked". It states that in words, and is inert + // rather than a link to nowhere. + assert.match(source, /Looked back — nothing on this yet/) + assert.match( + source, + /chat__source-chip--quiet">\s*<span className="chat__source-icon"/, + 'an empty lookup is a statement, not a destination', + ) +}) diff --git a/frontend/src/components/ChatView/__tests__/memoryRecall.test.js b/frontend/src/components/ChatView/__tests__/memoryRecall.test.js new file mode 100644 index 000000000..53ad7eddd --- /dev/null +++ b/frontend/src/components/ChatView/__tests__/memoryRecall.test.js @@ -0,0 +1,120 @@ +import test from 'node:test' +import assert from 'node:assert/strict' + +import { + MAX_RECALLED_NOTES, + messageRecall, + noteHref, + noteLabel, + safeNoteId, +} from '../memoryRecall.js' + +const note = (id, extra = {}) => ({ + id, + path: `notes/${id}.md`, + title: id.replace(/-/g, ' '), + ...extra, +}) + +const toolBlock = recall => ({ type: 'tool', tool: 'Bash', recall }) + +test('a turn that never consulted Memory yields no row at all', () => { + assert.equal(messageRecall([{ type: 'text', content: 'hi' }]), null) + assert.equal(messageRecall([{ type: 'tool', tool: 'Bash' }]), null) + assert.equal(messageRecall([]), null) + assert.equal(messageRecall(null), null) +}) + +test('a lookup that found nothing is reported, not silently dropped', () => { + // The distinction this whole row exists for: an owner must be able to tell a + // gap in their memory from an agent that ignored it. + const recall = messageRecall([toolBlock({ status: 'empty' })]) + assert.deepEqual(recall, { notes: [], empty: true }) +}) + +test('an in-flight lookup is a live beat, not yet a citation', () => { + assert.equal(messageRecall([toolBlock({ status: 'searching' })]), null) +}) + +test('recalled notes are collected in first-seen order', () => { + const recall = messageRecall([ + toolBlock({ status: 'hit', notes: [note('alpha'), note('beta')] }), + ]) + assert.deepEqual(recall.notes.map(n => n.id), ['alpha', 'beta']) + assert.equal(recall.empty, false) +}) + +test('the same note recalled twice in a turn is cited once', () => { + const recall = messageRecall([ + toolBlock({ status: 'hit', notes: [note('alpha')] }), + toolBlock({ status: 'hit', notes: [note('alpha'), note('beta')] }), + ]) + assert.deepEqual(recall.notes.map(n => n.id), ['alpha', 'beta']) +}) + +test('one empty probe does not erase what another lookup remembered', () => { + const recall = messageRecall([ + toolBlock({ status: 'empty' }), + toolBlock({ status: 'hit', notes: [note('alpha')] }), + ]) + assert.deepEqual(recall.notes.map(n => n.id), ['alpha']) + assert.equal(recall.empty, false, 'the turn did remember something') +}) + +test('compacted activity carries citations so they survive a reload', () => { + // _compact_activity_run rolls recall onto the activity summary for exactly + // this reason: the individual tool blocks are folded away on read. + const recall = messageRecall([ + { type: 'activity', recall: { status: 'hit', notes: [note('alpha')] } }, + ]) + assert.deepEqual(recall.notes.map(n => n.id), ['alpha']) +}) + +test('a lookup whose output could not be parsed still counts as looking', () => { + const recall = messageRecall([toolBlock({ status: 'hit', notes: [] })]) + assert.deepEqual(recall, { notes: [], empty: false }, + 'no notes to cite, but no false "nothing relevant" claim either') +}) + +test('citations are bounded so one turn cannot flood the transcript', () => { + const many = Array.from({ length: 40 }, (_, i) => note(`note-${i}`)) + const recall = messageRecall([toolBlock({ status: 'hit', notes: many })]) + assert.equal(recall.notes.length, MAX_RECALLED_NOTES) +}) + +test('only a well-formed note id may build a deep link', () => { + assert.equal(safeNoteId('theme-variables-are-shared'), 'theme-variables-are-shared') + assert.equal(safeNoteId('../../etc/passwd'), '') + assert.equal(safeNoteId('notes/alpha'), '') + assert.equal(safeNoteId('a b'), '') + assert.equal(safeNoteId(''), '') + assert.equal(safeNoteId(null), '') + assert.equal(safeNoteId('x'.repeat(200)), '') +}) + +test('a note links into the Memory app through the shell intent contract', () => { + assert.equal( + noteHref({ id: 'theme-variables-are-shared' }), + '/shell/?app=memory&intent=note%3Atheme-variables-are-shared', + ) + assert.equal(noteHref({ id: '../evil' }), '', + 'an unsafe id yields no link rather than an unsafe one') +}) + +test('a note without a title still reads as words, never blank', () => { + assert.equal(noteLabel({ id: 'x', title: 'Real Title' }), 'Real Title') + // A large tool output is carved head+tail, which can drop the titled section + // lines; the id is the fallback and must not surface raw dashes. + assert.equal(noteLabel({ id: 'theme-variables-are-shared' }), 'theme variables are shared') + assert.equal(noteLabel({ id: '../evil' }), '') +}) + +test('a malformed note is skipped without dropping its siblings', () => { + const recall = messageRecall([ + toolBlock({ + status: 'hit', + notes: [{ id: '../evil', path: 'notes/evil.md' }, note('alpha'), null], + }), + ]) + assert.deepEqual(recall.notes.map(n => n.id), ['alpha']) +}) diff --git a/frontend/src/components/ChatView/memoryRecall.js b/frontend/src/components/ChatView/memoryRecall.js new file mode 100644 index 000000000..077efae08 --- /dev/null +++ b/frontend/src/components/ChatView/memoryRecall.js @@ -0,0 +1,104 @@ +// Pure derivation behind the Memory citations on an answer, kept separate so +// the collection/dedupe contract is directly testable. +// +// Sibling of messageSources.js and deliberately the same shape of idea: a +// recalled note is the same KIND of fact as a cited web page — something from +// outside the model's turn that shaped the reply — so it is derived from the +// turn's tool blocks and rendered once, after the answer, rather than carried +// as its own content block (which would fragment a continuous thinking run). +// +// It does NOT reuse the `sources` field. Both `_safe_http_url` (backend) and +// `safeSourceUrl` here hard-require a complete http(s) URL because that value +// goes straight into an `<a href>`; a local note pointer would be silently +// dropped, and relaxing that gate for non-web data would weaken a shared XSS +// path for every citation. A sibling field riding the same tool block keeps it +// intact. +// +// The data itself is stamped by the backend (`memory_recall.py`, applied on the +// one `publish()` funnel), so both runners are covered by construction and the +// live wire, the catch-up log, and the persisted transcript agree. + +export const MAX_RECALLED_NOTES = 12 +const MAX_RECALL_ROWS_SCANNED = 256 + +// Only a note id may reach a deep link. The backend already validates the +// path, but this value builds a URL that navigates the shell, so re-check here +// rather than trusting an upstream call site to stay correct forever. +const SAFE_NOTE_ID = /^[A-Za-z0-9][A-Za-z0-9._-]*$/ + +export function safeNoteId(value) { + if (typeof value !== 'string') return '' + const candidate = value.trim() + if (!candidate || candidate.length > 128) return '' + return SAFE_NOTE_ID.test(candidate) ? candidate : '' +} + +// Where a pill points: the Memory app, asked to open this note. `?app=<slug>& +// intent=<text>` is the shell's existing internal-nav contract (the same one +// artifact links use), so this adds no new navigation mechanism. +export function noteHref(note) { + const id = safeNoteId(note?.id) + return id + ? `/shell/?app=memory&intent=${encodeURIComponent(`note:${id}`)}` + : '' +} + +// Titles come from the Memory app's own graph. A lookup whose titled section +// lines were carved out of a large tool output still yields a readable label +// from the note id, so a pill is never blank. +export function noteLabel(note) { + const title = typeof note?.title === 'string' ? note.title.trim() : '' + if (title) return title + const id = safeNoteId(note?.id) + return id ? id.replace(/[-_]+/g, ' ') : '' +} + +/** + * What the turn recalled from Memory, or null when it never looked. + * + * Three outcomes, and the difference between them is the whole point: + * { notes: [...] } — it remembered these notes + * { notes: [], empty: true } — it looked and Memory had nothing + * null — it never looked + * + * Without the middle case an owner cannot tell a memory gap from an agent that + * ignored its memory, which is exactly the distinction that earns trust. + */ +export function messageRecall(blocks) { + if (!Array.isArray(blocks)) return null + const notes = [] + const seen = new Set() + let looked = false + let empty = false + let scannedRows = 0 + + outer: + for (const block of blocks) { + // Compact historical activity carries the same bounded recall metadata on + // its summary block, so citations remain visible without loading the full + // tool timeline merely to rediscover them. + if (!['tool', 'activity'].includes(block?.type)) continue + const recall = block.recall + if (!recall || typeof recall !== 'object') continue + // A lookup still in flight is a live activity beat, not yet a citation. + if (recall.status === 'searching') continue + looked = true + if (recall.status === 'empty') empty = true + if (!Array.isArray(recall.notes)) continue + for (const note of recall.notes) { + scannedRows += 1 + if (scannedRows > MAX_RECALL_ROWS_SCANNED) break outer + const id = safeNoteId(note?.id) + const key = typeof note?.path === 'string' && note.path ? note.path : id + if (!id || !key || seen.has(key)) continue + if (notes.length >= MAX_RECALLED_NOTES) continue + seen.add(key) + notes.push(note) + } + } + + if (!looked) return null + // A lookup that returned notes is a hit even if another probe came back + // empty: the turn did remember something, and the notes say what. + return { notes, empty: notes.length === 0 && empty } +} diff --git a/frontend/src/components/ChatView/streamReducers.js b/frontend/src/components/ChatView/streamReducers.js index d4f4983bd..4d98962c1 100644 --- a/frontend/src/components/ChatView/streamReducers.js +++ b/frontend/src/components/ChatView/streamReducers.js @@ -337,6 +337,11 @@ export function attachToolOutput(prev, content, event = null) { if (event?.tool_use_id && !block.tool_use_id) { block.tool_use_id = event.tool_use_id } + // Settle a Memory lookup from "searching" to the notes it actually returned. + // The backend parsed these from the full output, before the reduction below. + if (event?.recall) { + block.recall = event.recall + } if (event?.output_truncated) { block.output_truncated = true block.output_full_len = event.output_full_len diff --git a/frontend/src/components/ChatView/toolActivityLabel.js b/frontend/src/components/ChatView/toolActivityLabel.js index b598e36f5..c2dc3997c 100644 --- a/frontend/src/components/ChatView/toolActivityLabel.js +++ b/frontend/src/components/ChatView/toolActivityLabel.js @@ -31,6 +31,10 @@ const ACTIVITY_LABELS = new Map([ // image file, mapped here by extension via effectiveToolName. Plural here — // the summary swaps to the singular "Viewing an image" for a lone one. ['ViewImage', 'Viewing images'], + // Consulting the Memory app. A Bash call to its search script, classified by + // the `recall` marker the backend stamps from the command — see + // effectiveToolName. Uncountable, so it has no singular twin. + ['MemoryRecall', 'Searching Memory'], ]) // Past-tense twins for SETTLED lines — "Ran commands", not a "Running @@ -57,6 +61,7 @@ const PAST_LABELS = new Map([ ['AskUserQuestion', 'Asked you'], ['Skill', 'Used a skill'], ['ViewImage', 'Viewed images'], + ['MemoryRecall', 'Recalled from Memory'], ]) // Singular twins for a ONE-occurrence activity: a lone Bash reads "Ran a @@ -97,6 +102,7 @@ const ACTIVITY_ICONS = new Map([ ['AskUserQuestion', 'dot'], ['Skill', 'dot'], ['ViewImage', 'image'], + ['MemoryRecall', 'search'], ]) // An unknown tool falls back to its raw name (then the generic 'Tool' for a @@ -149,6 +155,11 @@ const INSTANCE_VERBS = new Map([ export function toolCallLabel(tool) { const name = effectiveToolName(tool) || 'Tool' + // A memory lookup says what it FOUND, not what it ran: the raw command is + // implementation vocabulary, and the count is the fact worth reading at a + // glance. "Nothing relevant" is stated explicitly, because a silent recall + // is indistinguishable from never having looked. + if (name === 'MemoryRecall') return memoryRecallLabel(tool) const input = typeof tool?.input === 'string' ? tool.input.trim() : '' const verbs = INSTANCE_VERBS.get(name) if (!verbs) return name + (input ? `: ${input}` : '') @@ -171,6 +182,12 @@ export function toolCallLabel(tool) { const IMAGE_PATH_RE = /\.(png|jpe?g|gif|webp|bmp|avif)(?:[?#].*)?$/i export function effectiveToolName(tool) { const name = tool?.tool + // A Memory lookup is a Bash call the backend already identified from its + // command (memory_recall.py, stamped on the one publish() funnel). Keying off + // that stamp rather than re-matching the command here means one detection + // point for both runners, and it keeps working after transcript compaction + // strips the command string from a settled activity block. + if (tool?.recall && typeof tool.recall === 'object') return 'MemoryRecall' if (name === 'Read') { // On the wire tool.input is the STRING summary the backend builds // (summarize_tool_input -> the bare file_path for a Read), never the raw @@ -193,7 +210,21 @@ export function effectiveToolName(tool) { // compaction, and subagent spawns join here once the backend surfaces their // events (today only image views are frontend-detectable — a Skill load is // swallowed into a chip, not a tool block). -const DISTINCTIVE_ACTIVITIES = new Set(['ViewImage']) +const DISTINCTIVE_ACTIVITIES = new Set(['ViewImage', 'MemoryRecall']) + +// The one-line story of a memory lookup, in the three states that matter. +// Reading the count from the citation set the backend already parsed keeps the +// label and the pills under the answer from ever disagreeing. +export function memoryRecallLabel(tool) { + const recall = tool?.recall + if (recall?.status === 'searching' || tool?.status === 'running') { + return 'Searching Memory' + } + if (recall?.status === 'empty') return 'Searched Memory — nothing relevant' + const count = Array.isArray(recall?.notes) ? recall.notes.length : 0 + if (count === 0) return 'Recalled from Memory' + return `Recalled ${count} note${count === 1 ? '' : 's'} from Memory` +} export function isDistinctiveActivityTool(item) { return item?.type === 'tool' && DISTINCTIVE_ACTIVITIES.has(effectiveToolName(item)) } diff --git a/frontend/src/components/ChatView/useStreamConnection.js b/frontend/src/components/ChatView/useStreamConnection.js index 390dfa25b..08361df67 100644 --- a/frontend/src/components/ChatView/useStreamConnection.js +++ b/frontend/src/components/ChatView/useStreamConnection.js @@ -920,6 +920,10 @@ export default function useStreamConnection(chatId, { updated[i] = { ...updated[i], input: event.input, + // A Memory lookup names itself from its command, so the live + // line can read "Searching Memory…" while the search runs + // rather than a generic "Running a command". + ...(event.recall ? { recall: event.recall } : {}), ...(event.tool_use_id && !updated[i].tool_use_id ? { tool_use_id: event.tool_use_id } : {}), From 04ff2a5a67737edabddb7d3de018eee7e0bed97d Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:22:47 +0000 Subject: [PATCH 04/15] fix(chat): publish the steer cut where the transcript actually splits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steering a message into a live Claude turn made the assistant's output paint twice for the rest of that turn: everything streamed inside a multi-second window appeared once in the sealed pre-steer message and again at the head of the continuation. The bad boundary replayed out of the event log, so it reproduced on every reconnect. `steered_into_turn` is the client's only "seal the live stream here and re-base it" instruction. On the Claude path the steer route published it the moment the HTTP request arrived, but `ActiveClaudeClient.steer()` merely BUFFERS the request there; the runner performs the real transcript split much later, at its next interrupt boundary. Every block emitted inside that window was persisted into the sealed pre-steer message AND retained at the head of the client's freshly cleared stream. The same 202 also reported an optimistically emptied queue on that path, so the steered row left the tray while the server still held it. Codex was never affected: `turn.steer()` has no interrupt boundary, so for Codex the route genuinely is the split point. Regression from the change that moved the split to the runner and left the signal behind at the route. - One builder for the event, `steered_into_turn_event()` in `app/chat.py`, so the two publishers cannot drift on the wire shape. - The route publishes the cut only on the non-deferred (Codex) path, where the split really happens. Codex behaviour is unchanged. - The Claude cut is published by `_seal_steer_split()` itself, immediately after `split_for_steer` commits, on the sink's own broadcast, with no `await` between the split and the publish. A split with no resolvable broadcast still commits, and logs loudly. Announcing the cut can never raise out of the seal: the turn-end `finally` awaits it before unregistering the handle, so an escaping publish error would strand a live handle and leave the chat looking permanently busy. - The 202 now reports the pending queue as it actually is and marks the deferred case with `cut_deferred: true`. The client keeps the row in the tray until the cut retires it, and resolves only its own row by cid, so the 202 and the cut may land in either order. - `interrupt()` (Stop) now drops the buffered transcript rows as well as the provider-facing text: Stop's clear-and-resend path owns those rows from that point, and leaving them buffered let the dying turn's seal append the very row the client was re-sending. Tests: backend/tests/test_chats_stream_steer.py 31 passed (26 before); frontend `npm test` 1976 passed, 0 failed (1967 before). New `steerCutBoundary.test.js` pins the client half of the contract; three deferred-window ordering cases added to `usePendingQueue.test.js`. Mutation-checked: re-introducing the arrival-time publish, or letting a publish error escape the seal, each fail a named test. Backend changes require a server restart to take effect. Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/chat.py | 39 +++ backend/app/claude_sdk_runner.py | 97 +++++- backend/app/routes/chats_stream.py | 86 +++-- backend/tests/test_chats_stream_steer.py | 329 +++++++++++++++++- frontend/src/components/ChatView/ChatView.jsx | 135 +++++-- .../__tests__/steerCutBoundary.test.js | 195 +++++++++++ .../hooks/__tests__/usePendingQueue.test.js | 56 +++ .../ChatView/useStreamConnection.js | 58 ++- 8 files changed, 889 insertions(+), 106 deletions(-) create mode 100644 frontend/src/components/ChatView/__tests__/steerCutBoundary.test.js diff --git a/backend/app/chat.py b/backend/app/chat.py index 380913f56..82bfdb0b5 100644 --- a/backend/app/chat.py +++ b/backend/app/chat.py @@ -61,6 +61,7 @@ StashToolOutput, alloc_run_token, await_ack as _await_ack, + cid_of, get_writer, next_message_ts as _next_message_ts, update_last_assistant_message as _update_last_assistant_message, @@ -226,6 +227,44 @@ def active_sink_memory_diagnostics(*, include_payloads: bool = True) -> list[dic return diagnostics +def steered_into_turn_event(stored_messages: list[dict]) -> dict: + """Build the `steered_into_turn` SSE payload for a batch of steered rows. + + `steered_into_turn` is the AUTHORITATIVE CUT and the client's ONLY "seal the + live stream here and re-base it" signal. It means the transcript split has + COMMITTED: A1 sealed, these rows appended after it, the sink reset for A2. So + it may only be published from the instant the split really happens — by the + Claude runner immediately after `split_for_steer`, and by the steer route for + Codex (whose `turn.steer()` has no interrupt boundary, so the route IS the + seal point). Two publishers, one builder, so the wire shape cannot drift. + + Publishing it at HTTP arrival on the deferred (Claude) path is what made a + steer paint duplicated output for the rest of the turn: every block the runner + streamed between arrival and the real seal was accumulated into the sealed A1 + AND left at the head of the client's freshly re-based stream. The deferred + path publishes NOTHING at arrival — the 202's own `pending_messages` is what + keeps the accepted row visible until the cut, so there is no second channel + reconciling the same tray. + """ + return { + "type": "steered_into_turn", + "messages": [ + { + "role": "user", + "ts": msg.get("ts"), + "cid": cid_of(msg), + "content": msg.get("content", ""), + **({"attachments": msg.get("attachments")} if msg.get("attachments") else {}), + } + for msg in stored_messages + ], + # Backward-compatible shape for any existing client still expecting a + # single steered row. + "ts": stored_messages[-1].get("ts"), + "content": stored_messages[-1].get("content", ""), + } + + class _ChatEventSink: """Bridges SDK-runner events to broadcast + the chat-writer actor. diff --git a/backend/app/claude_sdk_runner.py b/backend/app/claude_sdk_runner.py index 1e61436b5..2afdfed3f 100644 --- a/backend/app/claude_sdk_runner.py +++ b/backend/app/claude_sdk_runner.py @@ -408,13 +408,26 @@ async def interrupt(self) -> None: bound at the call site; this inner timeout protects any other direct caller. - Stop is the hard, immediate-cut path: it drops any buffered steer - (clearing `pending_steer` + `_steer_requested` so no boundary cut or - requery fires for work the user just abandoned) and interrupts the - live turn right now, without waiting for a content-block boundary. + Stop is the hard, immediate-cut path: it drops the buffered steer + ENTIRELY — the provider-facing text (`pending_steer` + `_steer_requested`, + so no boundary cut or requery fires for work the user just abandoned) AND + the transcript-side rows (`_steer_user_msgs` + `_steer_consume_cids`, so the + turn-end seal appends nothing). + + Both halves have to go, because Stop OWNS those rows from here on: a + deferred steer's row is still a durable entry in `chat.pending_messages` + (the split that would consume it never ran), `/chat/stop` clears that queue + and reports the cleared cids, and the client re-sends exactly them as one + fresh turn. Leaving the rows buffered meant the dying turn's seal appended + the same row into the transcript while the client re-sent it — the row + appeared twice, once interrupted and once answered. Nothing is lost by + dropping them here: they were never in the transcript, and Stop's own + clear-and-resend path is what preserves them. """ self.pending_steer = [] self._steer_requested = False + self._steer_user_msgs = [] + self._steer_consume_cids = [] await self._client.interrupt() try: await asyncio.wait_for(asyncio.shield(self._finished), timeout=5.0) @@ -506,12 +519,16 @@ async def _seal_steer_split(bc, active_client, chat_id: str) -> None: Called at each requery boundary (so A1 is sealed before the answer A2 streams) AND unconditionally in the turn-end `finally` (so a steer that was - buffered but never sealed — an exception/early-return before the requery, or - a hard Stop that cleared `pending_steer` — is still persisted rather than - discarded with the handle). A1 is the sink's accumulated pre-interrupt - content — complete once the turn closes — so `split_for_steer` seals it as - its own message, appends the steered row(s) after it, and resets the sink so - A2 lands fresh: reload order Q1, A1, Q2, A2. + buffered but never sealed — an exception/early-return before the requery — is + still persisted rather than discarded with the handle). A hard Stop is NOT one + of those cases: `interrupt()` drops the buffered rows outright because Stop's + clear-and-resend path owns them from that point (see `interrupt`), so the + finally finds an empty buffer and appends nothing to the turn it just killed. + + A1 is the sink's accumulated pre-interrupt content — complete once the turn + closes — so `split_for_steer` seals it as its own message, appends the steered + row(s) after it, and resets the sink so A2 lands fresh: reload order Q1, A1, + Q2, A2. This is the fix for the steer-merge: the route cannot know where A1 ends (at HTTP arrival A1 has not streamed yet, so a route-side split sealed an empty @@ -519,6 +536,16 @@ async def _seal_steer_split(bc, active_client, chat_id: str) -> None: does. `bc` is the live `_ChatEventSink`; a non-sink `bc` (legacy path / a test double) cannot persist here and drops the buffered rows. + This is ALSO where the client's cut lands. `steered_into_turn` is the client's + only "seal the live stream here and re-base it" signal, so it must be + published from the same instant as the durable split — deferring the split to + here while the route published the cut at HTTP arrival meant every block + streamed in between was BOTH folded into the sealed A1 and left at the head of + the client's re-based stream, painting twice for the rest of the turn. No + split (no live sink, or a failed write) publishes no cut: the client must + never re-base earlier than the server's actual seal. A split with no + publisher is the one asymmetric case — it commits and logs, see below. + Durability contract (adversarial-review hardening): - The rows are snapshotted BEFORE the await and only the snapshotted count is removed on success, so a second steer landing during `split_for_steer`'s @@ -535,6 +562,27 @@ async def _seal_steer_split(bc, active_client, chat_id: str) -> None: return consume = list(active_client._steer_consume_cids) split = getattr(bc, "split_for_steer", None) + # Resolve the client-facing publisher BEFORE committing anything. Take the + # broadcast off the SINK rather than re-resolving it by chat_id: the cut + # belongs in the same event log that carries A1's blocks (so a reconnect + # replays the boundary at its true position), and a lookup could hand back a + # successor turn's broadcast when this runs from the turn-end `finally`. + # + # A missing publisher does NOT abort the split: the rows are already durable + # in the pending queue and the client was told the steer landed, so + # persistence wins over notification. It does mean this seal produces no cut, + # leaving the client's live stream un-rebased until its next authoritative + # fetch — a real divergence, so it is logged loudly here rather than returned + # away silently after the write has already committed. + raw_bc = getattr(bc, "bc", None) + if raw_bc is not None and not callable(getattr(raw_bc, "publish", None)): + raw_bc = None + if split is not None and raw_bc is None: + log.error( + "steer split has no broadcast to publish the cut on chat_id=%s; " + "the transcript will be split but the client stream cannot re-base " + "until it refetches", chat_id, + ) if split is None: # No live sink (legacy/test caller): there is no streamed A1 to seal # against and no way to persist here — drop the buffer. @@ -544,7 +592,7 @@ async def _seal_steer_split(bc, active_client, chat_id: str) -> None: ) return try: - await split(rows, consume) + stored_result = await split(rows, consume) except Exception: # Leave the buffer intact so the turn-end finally retries the write. log.exception( @@ -557,6 +605,33 @@ async def _seal_steer_split(bc, active_client, chat_id: str) -> None: active_client._steer_consume_cids = ( active_client._steer_consume_cids[len(consume):] ) + # Publish the cut now that A1 + Q2 are committed, on the broadcast resolved + # above. No await separates the split from this publish, so no continuation + # block can slip in front of it. + if raw_bc is None: + return + from app.chat import steered_into_turn_event + + stored_messages = ( + stored_result.get("stored_messages") if isinstance(stored_result, dict) + else None + ) + if not isinstance(stored_messages, list) or not stored_messages: + # The writer echoes the rows it stored; fall back to the rows we handed it + # so an older/leaner ack shape still produces a well-formed cut. + stored_messages = rows + try: + raw_bc.publish(steered_into_turn_event(stored_messages)) + except Exception: + # The split already COMMITTED, so failing to announce it is a notification + # loss, not a durability one — same asymmetry as the missing-publisher case + # above. Swallow and log: this function is awaited from the turn-end + # `finally`, where a raise would skip unregistering the handle and + # disconnecting the client, leaving the chat looking permanently live. + log.exception( + "publishing the steer cut failed chat_id=%s; the split committed but the " + "client stream cannot re-base until it refetches", chat_id, + ) def _skill_file_read_name( diff --git a/backend/app/routes/chats_stream.py b/backend/app/routes/chats_stream.py index ef645af64..710c0130c 100644 --- a/backend/app/routes/chats_stream.py +++ b/backend/app/routes/chats_stream.py @@ -21,6 +21,7 @@ is_draining, mark_starting, run_chat, + steered_into_turn_event, ) from app import chat_queue from app.chat_writer import ( @@ -409,16 +410,30 @@ async def _split_steer_at_route( def _steered_response( - chat_id: str, pending_messages: list[dict] | None = None, + chat_id: str, + pending_messages: list[dict] | None = None, + *, + cut_deferred: bool = False, ) -> JSONResponse: """202 for a message steered into the live turn. - The message is in the TRANSCRIPT (not the pending queue) and the live - turn already saw it via `steer()`, so the client renders it inline as - content growth rather than a queued-tray entry.""" + The live turn has seen the message via `steer()`. Where the message LIVES + right now depends on the provider, and `cut_deferred` states which: + + - absent/False (Codex): the transcript split already ran at the route, so the + row is in the TRANSCRIPT and out of the pending queue. The client drops its + queued-tray entry and renders the row inline as content growth. + - True (Claude): the split is deferred to the runner's interrupt boundary, so + the row is STILL in `pending_messages` (echoed above) and will move into the + transcript when the runner publishes `steered_into_turn` at the real seal. + The client keeps showing the queued row until then — dropping it here left + the owner's message with nowhere to render for the length of the window. + """ payload = {"status": "steered", "chat_id": chat_id} if pending_messages is not None: payload["pending_messages"] = pending_messages + if cut_deferred: + payload["cut_deferred"] = True return JSONResponse(status_code=202, content=payload) @@ -819,8 +834,10 @@ def _coerce_chat_settings(c): # Stop's queue-collapse path may pass force_steer to turn already # queued messages into a live steer. Codex injects into the running # SDK turn; Claude interrupts and re-prompts on the same client. On - # success the user message goes into the transcript and a - # `steered_into_turn` event tells the client to render it inline. + # success a `steered_into_turn` event tells the client the transcript has + # been split and the user row is in it — published here for Codex (the + # route is its seal point) and by the runner for Claude, whose split waits + # for the interrupt boundary (see the publish below). provider = chat.provider or "claude" if ( is_chat_running(chat_id) @@ -872,14 +889,16 @@ def _coerce_chat_settings(c): steered = False if steered: if defer_to_runner: - # The optimistic response mirrors the runner's cid conversion. - consumed = set(consume_cids) + # Nothing has been converted yet: the rows are buffered on the handle + # and remain in `chat.pending_messages` until the runner's seal + # consumes them. So report the queue AS IT IS — an optimistic list + # with the rows already subtracted told the client they had left the + # queue while the server still held them there, which is precisely + # the window the client must keep showing them (see `cut_deferred` + # on the response below). stored_result = { "stored_messages": user_msgs, - "pending": [ - m for m in (chat.pending_messages or []) - if cid_of(m) not in consumed - ], + "pending": list(chat.pending_messages or []), } else: # Codex append and pending removal commit atomically for one cid. @@ -900,31 +919,28 @@ def _coerce_chat_settings(c): status_code=503, detail="Could not save your message; please refresh.", ) - bc = get_broadcast(chat_id) - if bc is not None: - stored_messages = stored_result.get("stored_messages") - if not isinstance(stored_messages, list) or not stored_messages: - stored = stored_result.get("stored") or user_msg - stored_messages = [stored] - bc.publish({ - "type": "steered_into_turn", - "messages": [ - { - "role": "user", - "ts": msg.get("ts"), - "cid": cid_of(msg), - "content": msg.get("content", ""), - **({"attachments": msg.get("attachments")} if msg.get("attachments") else {}), - } - for msg in stored_messages - ], - # Backward-compatible shape for any existing client still - # expecting a single steered row. - "ts": stored_messages[-1].get("ts"), - "content": stored_messages[-1].get("content", ""), - }) + # `steered_into_turn` is the client's ONLY "cut the live stream here" + # signal, so it may only be published where the transcript is really + # split. Codex splits HERE (`_split_steer_at_route` above ran to + # completion), so the route is its seal point and publishes the cut + # unchanged. Claude defers the split to the runner's interrupt boundary, + # seconds later — publishing the cut here re-based the client's stream + # while the runner was still emitting blocks that the deferred seal then + # folded into A1, so those blocks painted twice for the rest of the turn. + # The deferred path publishes NOTHING here: the 202 below carries the + # still-queued row, which is the single signal that keeps it visible + # until `_seal_steer_split` publishes the cut. + if not defer_to_runner: + bc = get_broadcast(chat_id) + if bc is not None: + stored_messages = stored_result.get("stored_messages") + if not isinstance(stored_messages, list) or not stored_messages: + stored = stored_result.get("stored") or user_msg + stored_messages = [stored] + bc.publish(steered_into_turn_event(stored_messages)) return _steered_response( chat_id, stored_result.get("pending"), + cut_deferred=defer_to_runner, ) if body.force_steer: return _not_steered_response(chat_id) diff --git a/backend/tests/test_chats_stream_steer.py b/backend/tests/test_chats_stream_steer.py index e9fe048eb..92b53f63f 100644 --- a/backend/tests/test_chats_stream_steer.py +++ b/backend/tests/test_chats_stream_steer.py @@ -561,6 +561,154 @@ async def split_for_steer(self, rows, consume): asyncio.run(_run()) +def test_seal_publishes_the_cut_on_the_sinks_own_broadcast(): + """The cut goes to the broadcast the SINK holds, never to a fresh lookup. + + `_seal_steer_split` also runs from the turn-end `finally`, by which point a + successor turn can already have registered a NEW broadcast for the same chat. + Resolving by chat_id there would strand the cut in an event log no client is + reading: A1's blocks live in the old log, so the client would never re-base + and would paint the continuation onto the sealed segment for the rest of the + turn. Also covers a leaner writer ack (no `stored_messages`): the cut still + names the buffered rows rather than going out empty. + """ + from app.broadcast import create_broadcast, get_broadcast + from app.claude_sdk_runner import _seal_steer_split + + chat_id = "sealbroadcast" + handle = _make_active_claude_client(chat_id) + turn_bc = create_broadcast(chat_id) + + async def _run(): + handle._steer_user_msgs = [ + {"role": "user", "content": "Q2", "ts": 10, "cid": "c-q2"} + ] + handle._steer_consume_cids = ["c-q2"] + + class _SinkLike: + """Mirrors `_ChatEventSink`: holds the broadcast it was built with.""" + + def __init__(self, bc): + self.bc = bc + + async def split_for_steer(self, rows, consume): + # The writer ack shape without the echoed rows. + return {"pending": []} + + # A successor turn registers its own broadcast before this seal runs. + successor_bc = create_broadcast(chat_id) + assert get_broadcast(chat_id) is successor_bc + assert successor_bc is not turn_bc + + await _seal_steer_split(_SinkLike(turn_bc), handle, chat_id) + + assert [e.get("type") for e in successor_bc.event_log] == [] + cuts = [e for e in turn_bc.event_log if e.get("type") == "steered_into_turn"] + assert len(cuts) == 1 + assert [m["content"] for m in cuts[0]["messages"]] == ["Q2"] + assert [m["cid"] for m in cuts[0]["messages"]] == ["c-q2"] + + asyncio.run(_run()) + + +def test_a_failing_publisher_cannot_escape_the_seal(): + """Announcing the cut must never raise out of `_seal_steer_split`. + + The turn-end `finally` awaits this function BEFORE it unregisters the handle + and disconnects the client, so an escaping exception would strand a live + handle in the registry and leave the chat looking permanently busy. The split + has already COMMITTED by the time the cut is published, so a broken publisher + is a notification loss, not a durability one — exactly the asymmetry the + missing-publisher branch already takes. Swallow it, log it, and still consume + the sealed rows so the turn-end retry does not double-append them. + """ + from app.claude_sdk_runner import _seal_steer_split + + chat_id = "sealpublishfail" + handle = _make_active_claude_client(chat_id) + + async def _run(): + handle._steer_user_msgs = [ + {"role": "user", "content": "Q2", "ts": 10, "cid": "c-q2"} + ] + handle._steer_consume_cids = ["c-q2"] + + class _ExplodingBroadcast: + def publish(self, event): + raise RuntimeError("broadcast is gone") + + class _SinkLike: + def __init__(self, bc): + self.bc = bc + self.splits = 0 + + async def split_for_steer(self, rows, consume): + self.splits += 1 + return {"stored_messages": list(rows)} + + sink = _SinkLike(_ExplodingBroadcast()) + await _seal_steer_split(sink, handle, chat_id) + + assert sink.splits == 1 + # The rows were committed, so they must not be re-appended by the retry. + assert handle._steer_user_msgs == [] + assert handle._steer_consume_cids == [] + + asyncio.run(_run()) + + +def test_stop_drops_the_buffered_steer_instead_of_appending_it(): + """A hard Stop abandons a deferred steer ENTIRELY. + + Stop's contract: `/chat/stop` clears `chat.pending_messages`, reports the + cleared cids, and the client re-sends exactly them as one fresh turn. A + deferred steer's row is still IN that queue (its split never ran), so if the + runner kept the row buffered, the turn-end seal appended it to the turn Stop + had just killed while the client re-sent it — the same message twice, once + interrupted and once answered. `interrupt()` therefore clears the + transcript-side buffer too, which makes the finally's seal a no-op. + """ + from app.claude_sdk_runner import ActiveClaudeClient, _seal_steer_split + + class _Client: + async def interrupt(self): + return None + + async def _run(): + # Built inside THIS loop: interrupt() waits on `_finished`, which is + # loop-bound, so a handle constructed in a throwaway loop cannot be awaited + # here. mark_finished() stands in for the runner's own teardown. + handle = ActiveClaudeClient(_Client(), chat_id="stopsteer") + handle.mark_finished() + handle.pending_steer = ["Q2"] + handle._steer_requested = True + handle._steer_user_msgs = [ + {"role": "user", "content": "Q2", "ts": 10, "cid": "c-q2"} + ] + handle._steer_consume_cids = ["c-q2"] + + await handle.interrupt() + + assert handle.pending_steer == [] + assert handle._steer_requested is False + assert handle._steer_user_msgs == [] + assert handle._steer_consume_cids == [] + + # The turn-end catch-all now has nothing to append. + class _Bc: + def __init__(self): + self.splits = 0 + + async def split_for_steer(self, rows, consume): + self.splits += 1 + + bc = _Bc() + await _seal_steer_split(bc, handle, "stopsteer") + assert bc.splits == 0 + + asyncio.run(_run()) + + def test_claude_force_steer_defers_to_runner_and_reorders(client, auth): """A Claude fast-forward (force_steer) defers its split to the runner, same as an ordinary steer, so the fast-forwarded rows land AFTER the sealed @@ -974,6 +1122,12 @@ def test_steers_into_live_claude_turn_reserves_durable_pending( assert res.status_code == 202, res.text assert res.json()["status"] == "steered" + # The split is deferred, so the response says so and echoes the row as the + # still-queued row it is; the client keeps showing it until the cut. + assert res.json()["cut_deferred"] is True + assert [m["content"] for m in res.json()["pending_messages"]] == [ + "actually use blue" + ] chat = _read_chat(chat_id) assert [m["content"] for m in chat.pending_messages] == ["actually use blue"] @@ -986,21 +1140,14 @@ def test_steers_into_live_claude_turn_reserves_durable_pending( assert cid_of(handle._steer_user_msgs[0]) == reserved_cid assert handle._steer_consume_cids == [reserved_cid] - # A `steered_into_turn` event was broadcast for the inline render. + # NO event at HTTP arrival on the deferred path. The 202's own + # `pending_messages` (asserted above) is the single signal that keeps the row + # visible; the CUT (`steered_into_turn`) belongs to the runner's seal — see + # test_claude_steer_cut_event_is_published_at_the_seal_not_at_http_arrival. + # A second "accepted" event reconciling the same tray would be a parallel + # channel racing the response it duplicates. bc = get_broadcast(chat_id) - steered_events = [ - e for e in bc.event_log if e.get("type") == "steered_into_turn" - ] - assert len(steered_events) == 1 - assert steered_events[0]["content"] == "actually use blue" - assert steered_events[0]["messages"] == [ - { - "role": "user", - "ts": handle._steer_user_msgs[0]["ts"], - "cid": reserved_cid, - "content": "actually use blue", - } - ] + assert bc.event_log == [] def test_claude_runner_splits_steer_at_boundary_not_http_arrival( @@ -1074,6 +1221,160 @@ async def _drive_runner(): assert _read_chat(chat_id).pending_messages in (None, []) +def test_claude_steer_cut_event_is_published_at_the_seal_not_at_http_arrival( + client, auth, +): + """`steered_into_turn` is the client's only "cut the live stream here" signal, + so on the deferred (Claude) path it must be published by the runner at the + seal — AFTER every block that belongs to A1 — and never by the route. + + The regression this pins: the route published the cut at HTTP arrival while + the split stayed at the runner's interrupt boundary seconds later. Everything + Claude streamed in the gap was accumulated into the sealed A1 AND kept at the + head of the client's freshly re-based stream, so it painted twice for the rest + of the turn. The window is never empty — the AssistantMessage that triggers + the boundary interrupt is dispatched to the broadcast before the interrupt + check runs — so this duplicated on EVERY Claude steer. + """ + from app.chat import _ChatEventSink, register_active_sink + from app.claude_sdk_runner import _seal_steer_split + + chat_id = "claudecutorder" + db = SessionLocal() + try: + db.add(models.Chat( + id=chat_id, title="Claude chat", provider="claude", + messages=[{"role": "user", "content": "Q1", "ts": 1}], + agent_settings_json={"steer_enabled": True}, + )) + db.commit() + finally: + db.close() + handle = _make_active_claude_client(chat_id) + registry.register(handle) + bc = create_broadcast(chat_id) + sink = _ChatEventSink(bc, chat_id, run_token="run-cut-order") + register_active_sink(chat_id, sink) + + # A1's first block is already on the wire when the steer POST arrives. + sink.publish({"type": "text", "content": "A1 first"}) + + res = client.post( + f"/api/chats/{chat_id}/messages", + json={"content": "Q2"}, headers=auth, + ) + assert res.status_code == 202, res.text + assert res.json()["status"] == "steered" + + # HTTP arrival publishes NOTHING on the deferred path: the response body is + # the display signal. Publishing the cut here re-based the client's stream + # too early. + assert [e.get("type") for e in bc.event_log] == ["text"] + + async def _drive_runner(): + # The rest of A1 streams AFTER arrival — the duplication window. + sink.publish({"type": "text", "content": " A1 rest"}) + await _seal_steer_split(sink, handle, chat_id) + # A2's first block follows the seal. It exists here so the cut's position + # is pinned from BOTH sides: a cut that slipped in front of a continuation + # block would fold A2's head into the sealed A1 and re-base after it — + # the mirror image of the fixed bug, and invisible to a lower bound alone. + sink.publish({"type": "text", "content": "A2 head"}) + + asyncio.run(_drive_runner()) + + # The cut is published exactly once, at the seal: after every A1 block and + # before every A2 block. + cut_positions = [ + i for i, e in enumerate(bc.event_log) + if e.get("type") == "steered_into_turn" + ] + assert len(cut_positions) == 1 + # The sink coalesces contiguous text into one event per segment, so the whole + # of A1 is one event and A2's head is another. Both bounds matter: after the + # last A1 event (the fixed bug) AND before the first A2 event (its mirror + # image, which would fold A2's head into the sealed A1). + text_positions = [ + i for i, e in enumerate(bc.event_log) if e.get("type") == "text" + ] + assert [bc.event_log[i].get("content") for i in text_positions] == [ + "A1 first A1 rest", "A2 head", + ] + assert text_positions[0] < cut_positions[0] < text_positions[1] + # The cut names the DURABLE rows the split committed, so the client inserts + # the same identity the transcript now holds. + cut = bc.event_log[cut_positions[0]] + steered_row = [m for m in _read_chat(chat_id).messages if m["role"] == "user"][-1] + assert cut["messages"] == [{ + "role": "user", + "ts": steered_row["ts"], + "cid": cid_of(steered_row), + "content": "Q2", + }] + # And A1 really was sealed at the boundary the cut names. + assert [(m["role"], m.get("content")) for m in _read_chat(chat_id).messages] == [ + ("user", "Q1"), + ("assistant", "A1 first A1 rest"), + ("user", "Q2"), + ] + + +def test_codex_steer_still_publishes_the_cut_at_the_route( + client, auth, monkeypatch, +): + """Codex is unaffected by moving the Claude cut. + + Its `turn.steer()` injects into the SAME running turn, so the route's own + `_split_steer_at_route` IS the seal: signal and cut are the same instant + there. The route must therefore keep publishing `steered_into_turn` at + arrival, with the response shape unchanged (no `cut_deferred`). + """ + chat_id = "codexcutroute" + db = SessionLocal() + try: + db.add(models.Chat( + id=chat_id, title="Codex chat", provider="codex", + messages=[{"role": "user", "content": "Q1", "ts": 1}], + agent_settings_json={"steer_enabled": True}, + )) + db.commit() + finally: + db.close() + registry.register(_make_active_codex_turn(chat_id)) + sink = _register_sink_with_partial(chat_id, "run-codex-cut", "A1") + + async def _fake_steer(cid, message): + return True + + monkeypatch.setattr( + "app.codex_sdk_runner.steer_into_active_turn", _fake_steer, + ) + + res = client.post( + f"/api/chats/{chat_id}/messages", + json={"content": "Q2"}, headers=auth, + ) + assert res.status_code == 202, res.text + body = res.json() + assert body["status"] == "steered" + assert "cut_deferred" not in body + # The row left pending at the route, so the echoed queue no longer holds it. + assert body["pending_messages"] == [] + + bc = get_broadcast(chat_id) + assert [e.get("type") for e in bc.event_log] == ["steered_into_turn"] + cut = [e for e in bc.event_log if e.get("type") == "steered_into_turn"][0] + assert [m["content"] for m in cut["messages"]] == ["Q2"] + # The route sealed A1 and appended Q2 before publishing — the cut is + # truthful at the instant it is sent. + assert [(m["role"], m.get("content")) for m in _read_chat(chat_id).messages] == [ + ("user", "Q1"), + ("assistant", "A1"), + ("user", "Q2"), + ] + assert sink.assistant_blocks == [] + + def test_claude_reserved_row_survives_process_loss_and_sweep( client, auth, monkeypatch, ): diff --git a/frontend/src/components/ChatView/ChatView.jsx b/frontend/src/components/ChatView/ChatView.jsx index 92127323e..692da30af 100644 --- a/frontend/src/components/ChatView/ChatView.jsx +++ b/frontend/src/components/ChatView/ChatView.jsx @@ -1262,13 +1262,18 @@ export default function ChatView({ }, onLiveQuestion: setLiveQuestionId, onSteeredIntoTurn: ({ ts, content, messages: steeredBatch }) => { - // A send was injected mid-turn into a live turn (steering — fired for - // both providers when Stop is pressed with a queued message). The - // backend seals the assistant text streamed so far, persists the user - // message, and then continues the assistant after that boundary. Mirror - // that exact shape locally: first promote the current live stream - // segment into `messages`, then append the steered user row, then let - // future text deltas build a fresh streaming assistant block. + // The steer's transcript split has COMMITTED (fired for both providers, + // including when Stop is pressed with a queued message): the backend has + // sealed the assistant text streamed up to the split, persisted the user + // message after it, and reset for the continuation. Mirror that exact + // shape locally: first promote the current live stream segment into + // `messages`, then append the steered user row, then let future text + // deltas build a fresh streaming assistant block. + // + // The split is the route's own write for Codex and the runner's seal for + // Claude, so this event always arrives AFTER the last block belonging to + // the sealed segment. That ordering is what makes promoting the live + // stream here correct; it is not a guess about where the server cut. // // It still follows the one visible-row scroll rule. Automatic queue // promotion keeps the original submit snapshot; an explicit fast-forward @@ -1320,6 +1325,14 @@ export default function ChatView({ // instead of blindly appending: if a fetch/replay already committed the // post-steer assistant row, the steered user still belongs before it. commitMessages(prev => insertMessageBatchByTs(prev, steeredMessages)) + // The rows have now genuinely left `chat.pending_messages`, so retire the + // tray entries the deferred-cut window kept visible. A no-op on the + // route-split (Codex) path, where the send's own 202 already dropped + // them — the cut is simply the one place that owns the hand-off. + for (const msg of steeredMessages) { + const cid = cidOf(msg) + if (cid != null) pendingQueue.cancelByCid(cid) + } steerPinIntentRef.current = null }, }) @@ -2214,14 +2227,48 @@ export default function ChatView({ await handleSteerOneRef.current?.(cid) } } - // Mid-turn steer: the backend delivered the send into the live - // provider turn and persisted it in the transcript. The - // `steered_into_turn` SSE event (handled in useStreamConnection's - // onSteeredIntoTurn) renders the message inline, so drop the - // optimistic queued-tray entry here — it never queued. + // Mid-turn steer: the backend delivered the send into the live provider + // turn. Where the row LIVES right now is what `cut_deferred` states. if (result?.status === 'steered') { - pendingQueue.cancelByCid(queuedMsg.cid) - forgetQueuedPinIntent({ cid: queuedMsg.cid }) + if (result.cut_deferred) { + // Claude: the transcript split waits for the runner's interrupt + // boundary, so the row is STILL queued server-side and its tray + // entry stays — dropping it here left the owner's message with + // nowhere to render for the whole deferred window. Resolve THIS + // send's own row and nothing else: confirm it by cid against the + // server's echoed entry (which carries the durable ts fast-forward + // needs). + // + // Not `hydrate(result.pending_messages)`: that list is a snapshot + // taken at steer time, and a wholesale reconcile against it is + // wrong in both directions. It would DROP a row queued and + // confirmed while this 202 was in flight (absent from the snapshot, + // no longer in-flight), and it would RESURRECT this row if the + // runner's cut landed first (the cut retires the tray entry, then + // the snapshot puts it back while it is already inline). Confirming + // one cid is a no-op once the cut has retired it, so the two can + // land in either order. + const serverRow = Array.isArray(result.pending_messages) + ? result.pending_messages.find(m => cidOf(m) === queuedMsg.cid) + : null + pendingQueue.confirmQueued(queuedMsg.cid, { + ts: serverRow?.ts ?? queuedMsg.ts, + position: serverRow?.position, + serverMsg: serverRow, + }) + // The pin intent lives on in `inlineSteerPinIntentRef` (set at + // submit), which is what `onSteeredIntoTurn` reads at the cut. Its + // `takeQueuedPinIntent` fallback is short-circuited by that ref, so + // without this the map entry would never be taken and would leak + // for the life of the mounted chat. + forgetQueuedPinIntent({ cid: queuedMsg.cid }) + } else { + // Codex: the split already ran at the route, so the row is in the + // transcript and `steered_into_turn` (already sent) renders it + // inline — drop the optimistic queued-tray entry, it never queued. + pendingQueue.cancelByCid(queuedMsg.cid) + forgetQueuedPinIntent({ cid: queuedMsg.cid }) + } } // Race: server said "started" though we expected queued. if (result?.status === 'started') { @@ -2270,7 +2317,10 @@ export default function ChatView({ } // Invariant: every observable queue-path status must resolve // the optimistic entry's in-flight flag. queued/steered/started - // each clear it above (confirmQueued / cancelByCid). Any + // each clear it above, unconditionally, via confirmQueued or + // cancelByCid — including BOTH steered branches (confirmQueued when + // the cut is deferred, cancelByCid when the route already split), so + // no response shape can slip through leaving the mark set. Any // other status — e.g. streamSend's `not_steered` — leaves the // entry as an ordinary queued row, so clear the flag here or it // leaks forever and a later hydrate would wrongly preserve it. @@ -2984,10 +3034,12 @@ export default function ChatView({ // or waiting for turn-end (the default queue drain). Mirrors handleStop's // structure — re-entry guard, snapshot-before-await — but never // interrupts the running turn. The backend force-steers (bypassing the - // steer_enabled opt-in) for BOTH providers; on success the steered - // message lands in the transcript and renders inline via the - // `steered_into_turn` SSE event (onSteeredIntoTurn above), so we just - // drop those rows from the local tray. + // steer_enabled opt-in) for BOTH providers; the rows render inline when the + // `steered_into_turn` SSE event reports the transcript split (onSteeredIntoTurn + // above), and THAT is when they leave the local tray. Codex splits at the + // route, so the split is already done when the POST resolves; Claude splits at + // its next content-block boundary, so its 202 comes back `cut_deferred` and + // the rows stay in the tray until the cut lands. // The shared force-steer core: given serverTs-CONFIRMED queue rows (in // queue order), optimistically hide them, POST one force_steer selecting // them by cid, and reconcile or restore. Restore re-hydrates the full @@ -3032,9 +3084,10 @@ export default function ChatView({ let queueAfterOptimisticPromote = null function restoreOptimisticSteerQueue() { // If another path touched the queue while the POST was in flight - // (notably the natural turn-end drain), every pendingQueue mutation - // assigns a fresh array. In that case the other path won the race, - // so restoring our stale snapshot would resurrect duplicate chips. + // (notably the natural turn-end drain, or a deferred steer's own cut + // arriving before its 202 resolves), every pendingQueue mutation assigns + // a fresh array. In that case the other path won the race, so restoring + // our stale snapshot would resurrect duplicate chips. if ( queueAfterOptimisticPromote !== null && pendingQueue.pendingMessagesRef.current === queueAfterOptimisticPromote @@ -3067,6 +3120,13 @@ export default function ChatView({ // visible "down, then up" fast-forward jump. Hide only the confirmed // rows this request is steering; restore the snapshot below if the // backend says the turn was not steered. + // + // A `cut_deferred` steer (Claude) puts these rows straight back below: + // they are still queued server-side until the runner's seal, and the pin + // is armed at the cut, not here, so this hide buys nothing there. We + // cannot know which case it is before the POST resolves, and the response + // restores them in the same round-trip, so the deferred path just doesn't + // get the pre-pin hide. pendingQueue.promoteManyByCid(consumePendingCids) queueAfterOptimisticPromote = pendingQueue.pendingMessagesRef.current const result = await streamSend(content, attachments, { @@ -3080,13 +3140,24 @@ export default function ChatView({ })), }) if (result?.status === 'steered') { - // The steered rows now render inline (onSteeredIntoTurn promotes - // them from the SSE event + transcript). Drop them from the local - // tray. Reconcile against the server's authoritative remaining - // queue when present, else remove exactly the steered cids. - if (Array.isArray(result.pending_messages)) { + if (result.cut_deferred) { + // Claude: the split waits for the runner's interrupt boundary, so + // these rows are still queued server-side and the optimistic hide + // above has to come back. Undo it with the SAME identity-guarded + // restore the not_steered path uses, not with the response's echoed + // queue: that list is a snapshot from steer time, so re-adding from + // it would resurrect a row the cut had already retired (the cut can + // land before this 202 resolves) and would drop a row queued while + // the POST was in flight. The guard makes the restore a no-op exactly + // when something else — the cut included — already owns the queue. + restoreOptimisticSteerQueue() + } else if (Array.isArray(result.pending_messages)) { + // Codex: the route already split, so the echoed queue no longer holds + // these rows and onSteeredIntoTurn has rendered them inline. pendingQueue.hydrate(result.pending_messages) } else { + // A backend that echoes no queue at all: remove exactly the steered + // cids. for (const c of consumePendingCids) pendingQueue.cancelByCid(c) } forgetQueuedPinIntent({ cidList: consumePendingCids }) @@ -3111,10 +3182,12 @@ export default function ChatView({ // or waiting for turn-end (the default queue drain). Mirrors handleStop's // structure — re-entry guard, snapshot-before-await — but never // interrupts the running turn. The backend force-steers (bypassing the - // steer_enabled opt-in) for BOTH providers; on success the steered - // message lands in the transcript and renders inline via the - // `steered_into_turn` SSE event (onSteeredIntoTurn above), so we just - // drop those rows from the local tray. + // steer_enabled opt-in) for BOTH providers; the rows render inline when the + // `steered_into_turn` SSE event reports the transcript split (onSteeredIntoTurn + // above), and THAT is when they leave the local tray. Codex splits at the + // route, so the split is already done when the POST resolves; Claude splits at + // its next content-block boundary, so its 202 comes back `cut_deferred` and + // the rows stay in the tray until the cut lands. async function handleSteer() { if (handlingSteerRef.current) return handlingSteerRef.current = true diff --git a/frontend/src/components/ChatView/__tests__/steerCutBoundary.test.js b/frontend/src/components/ChatView/__tests__/steerCutBoundary.test.js new file mode 100644 index 000000000..4fc91dedd --- /dev/null +++ b/frontend/src/components/ChatView/__tests__/steerCutBoundary.test.js @@ -0,0 +1,195 @@ +/* + * Where a steer's CUT is published, and who reconciles the tray. + * + * `steered_into_turn` is the client's only "seal the live stream here and + * re-base it" signal. Publishing it at HTTP arrival while the transcript split + * waited for the Claude runner's interrupt boundary is what made a live turn + * paint duplicated output for the rest of the turn: every block streamed in + * between was folded into the sealed pre-steer message AND left at the head of + * the client's re-based stream. The cut now comes from the seal itself. + * + * The event POSITION and the resulting durable order are proven by execution in + * backend/tests/test_chats_stream_steer.py (the cut lands after the last + * pre-steer block and before the first continuation block, on the sink's own + * broadcast). What these tests pin is the client half of the same contract: + * ONE steer event, ONE tray reconciler per steer, and an order-independent + * reconcile on the deferred path. The queue mechanics that make it + * order-independent are executed against the real hook in + * hooks/__tests__/usePendingQueue.test.js. + */ + +import { readFileSync } from 'node:fs' +import { test } from 'node:test' +import assert from 'node:assert/strict' + +const streamSource = readFileSync( + new URL('../useStreamConnection.js', import.meta.url), + 'utf8', +) +const chatViewSource = readFileSync( + new URL('../ChatView.jsx', import.meta.url), + 'utf8', +) + +function sliceBranch(source, fromNeedle, toNeedle) { + const from = source.indexOf(fromNeedle) + assert.ok(from >= 0, `expected to find ${fromNeedle}`) + const to = source.indexOf(toNeedle, from) + assert.ok(to > from, `expected to find ${toNeedle} after ${fromNeedle}`) + return source.slice(from, to) +} + +test('a steer has exactly one event and one tray reconciler', () => { + // The send's own 202 already carries `cut_deferred` + the still-queued row, + // so a second "accepted" SSE event would be a parallel channel reconciling + // the same tray from the same pre-cut snapshot — racing the response it + // duplicates. The steer wire has one event: the cut. + assert.ok( + !streamSource.includes('steer_accepted'), + 'no second steer event may reconcile the tray beside the 202', + ) + assert.ok( + !chatViewSource.includes('onSteerAccepted'), + 'ChatView takes the accepted row from its own response, not a second event', + ) + const steerEvents = [...streamSource.matchAll(/event\.type === '(steer[^']*)'/g)] + .map(m => m[1]) + assert.deepEqual(steerEvents, ['steered_into_turn']) +}) + +test('only the cut re-bases the stream, and a replay refetches instead', () => { + const cut = sliceBranch( + streamSource, + "event.type === 'steered_into_turn'", + "event.type === 'done'", + ) + assert.match( + cut, /flushBuffer\(\)/, + 'the buffered pre-steer frame belongs to the sealed segment, not the next', + ) + const replay = sliceBranch(cut, 'if (isCatchUp) {', '} else {') + assert.match( + replay, /catchUpItems = \[\]/, + 'the cut is the boundary a reconnect reconstructs: drop everything ' + + 'replayed before it, keep the continuation', + ) + assert.ok( + !replay.includes('onSteeredIntoTurnRef'), + 'promoting replayed items would duplicate the already-sealed segment', + ) + assert.match( + replay, /onNeedsRefreshRef\.current\?\./, + 'dropping the replayed segment is only truthful if the sealed message and ' + + 'the steered row are loaded — a socket that died before the cut arrived ' + + 'live has neither, so the replay asks for the authoritative read', + ) +}) + +test('the cut hands the steered rows off the tray and into the transcript', () => { + const handler = sliceBranch( + chatViewSource, + 'onSteeredIntoTurn: ({', + '\n })\n', + ) + assert.match( + handler, + /promoteStreamToMessages\(\{ keepTurnOpen: true \}\)/, + 'the cut seals the live segment as its own message', + ) + assert.match( + handler, + /pendingQueue\.cancelByCid\(cid\)/, + 'the cut is when the rows genuinely leave chat.pending_messages', + ) +}) + +test('a deferred steer resolves only its OWN row, in any order', () => { + // The 202's `pending_messages` is a snapshot from steer time, so the runner's + // cut can already have retired the row by the time it resolves. Confirming + // one cid is a no-op then; reconciling the whole tray against the snapshot + // would resurrect that row and drop anything queued since (proven on the + // real hook in usePendingQueue.test.js). + const steeredBranch = sliceBranch( + chatViewSource, + "if (result?.status === 'steered') {", + "if (result?.status === 'started') {", + ) + const deferred = sliceBranch(steeredBranch, 'if (result.cut_deferred) {', '} else {') + assert.match( + deferred, + /pendingQueue\.confirmQueued\(queuedMsg\.cid, \{/, + 'the deferred 202 confirms this send\'s row and keeps it in the tray', + ) + assert.ok( + !deferred.includes('pendingQueue.hydrate'), + 'a wholesale reconcile against the pre-cut snapshot is what drops a ' + + 'concurrently-queued row and resurrects a retired one', + ) + assert.match( + deferred, + /forgetQueuedPinIntent\(\{ cid: queuedMsg\.cid \}\)/, + 'the cut reads the pin intent from inlineSteerPinIntentRef, so the map ' + + 'entry must be released here or it leaks for the life of the chat', + ) + assert.match( + steeredBranch, + /\} else \{[\s\S]*?pendingQueue\.cancelByCid\(queuedMsg\.cid\)/, + 'a route-split (Codex) steer still drops the tray entry immediately', + ) +}) + +test('every steered branch resolves the optimistic in-flight mark', () => { + // `clearInFlight`'s fallthrough deliberately excludes `steered`, so a steered + // branch that resolves nothing would leak the mark forever and every later + // hydrate would preserve the row as a permanent ghost chip. + const steeredBranch = sliceBranch( + chatViewSource, + "if (result?.status === 'steered') {", + "if (result?.status === 'started') {", + ) + for (const resolver of [ + /pendingQueue\.confirmQueued\(/, + /pendingQueue\.cancelByCid\(/, + ]) { + assert.match(steeredBranch, resolver) + } + // Neither resolver may sit behind a shape check on the response body: a + // stripped/older `pending_messages` must not leave the mark set. + const deferred = sliceBranch(steeredBranch, 'if (result.cut_deferred) {', '} else {') + const confirmAt = deferred.indexOf('pendingQueue.confirmQueued(') + const guardAt = deferred.indexOf('Array.isArray(result.pending_messages)') + assert.ok(guardAt >= 0 && confirmAt > guardAt) + assert.ok( + !/if \(Array\.isArray\(result\.pending_messages\)\) \{[\s\S]*?confirmQueued/ + .test(deferred), + 'the confirm must run for every deferred response shape, not only when the ' + + 'server echoed a usable queue', + ) +}) + +test('the fast-forward path undoes its own optimistic hide, guarded', () => { + const ff = sliceBranch( + chatViewSource, + 'async function steerRowsImpl(steerRowsList) {', + '\n // STEER (fast-forward): inject the queued messages into the LIVE turn', + ) + const deferred = sliceBranch(ff, 'if (result.cut_deferred) {', '} else if (') + assert.match( + deferred, /restoreOptimisticSteerQueue\(\)/, + 'the rows are still queued server-side, so the pre-pin hide is undone with ' + + 'the identity-guarded local restore', + ) + assert.ok( + !deferred.includes('pendingQueue.hydrate'), + 're-adding rows from the pre-cut snapshot resurrects whatever the cut ' + + 'already retired', + ) + // The guard is what makes it order-independent: any other writer to the + // queue (the cut included) wins and the restore becomes a no-op. + const restore = sliceBranch(ff, 'function restoreOptimisticSteerQueue() {', '\n }\n') + assert.match( + restore, + /pendingQueue\.pendingMessagesRef\.current === queueAfterOptimisticPromote/, + 'restore only when nothing else has touched the queue since the promote', + ) +}) diff --git a/frontend/src/components/ChatView/hooks/__tests__/usePendingQueue.test.js b/frontend/src/components/ChatView/hooks/__tests__/usePendingQueue.test.js index 57231c326..f45997729 100644 --- a/frontend/src/components/ChatView/hooks/__tests__/usePendingQueue.test.js +++ b/frontend/src/components/ChatView/hooks/__tests__/usePendingQueue.test.js @@ -392,3 +392,59 @@ test('two distinct in-flight rows with identical text stay distinct (cid disambi assert.ok(list.find(m => m.cid === 'local-1')) assert.ok(list.find(m => m.cid === 'local-2')) }) + +// ── The deferred steer's 202 must confirm one row, not hydrate the tray ────── +// A Claude steer is accepted at HTTP arrival but its transcript split (and so +// the cut that retires the tray row) happens later, at the runner's interrupt +// boundary. The 202 therefore echoes a queue snapshot taken BEFORE the cut, +// and the two can land in either order. These pin why doSend confirms the one +// cid it owns instead of reconciling the whole tray against that snapshot. + +test('confirming the deferred steer row is a no-op once the cut retired it', () => { + const { result } = renderHook(usePendingQueue) + result.current.add(fixtureMsg({ cid: 'steered', ts: 500 }), { inFlight: true }) + // The runner's cut lands first: onSteeredIntoTurn renders the row inline and + // cancels its tray entry. + result.current.cancelByCid('steered') + // ...and only then does the steer's own 202 resolve. + result.current.confirmQueued('steered', { ts: 500 }) + assert.deepEqual( + result.current.pendingMessagesRef.current, [], + 'the row is already inline in the transcript; confirming must not put a ' + + 'second copy back in the tray', + ) +}) + +test('hydrating the deferred steer snapshot WOULD resurrect and drop rows', () => { + // The failure mode the confirm avoids, asserted on the real hook so the + // reason for not calling hydrate here is executable rather than a comment. + const { result } = renderHook(usePendingQueue) + result.current.add(fixtureMsg({ cid: 'steered', ts: 500 }), { inFlight: true }) + // A second send queues and its own POST acks while the steer's 202 is still + // in flight, so it is confirmed and no longer in-flight. + result.current.add(fixtureMsg({ cid: 'later', ts: 600 }), { inFlight: true }) + result.current.confirmQueued('later', { ts: 600 }) + // The cut retires the steered row. + result.current.cancelByCid('steered') + // Now the stale snapshot arrives. + result.current.hydrate([{ role: 'user', content: 'hi', ts: 500, cid: 'steered' }]) + assert.deepEqual( + result.current.pendingMessagesRef.current.map(m => m.cid), ['steered'], + 'wholesale reconcile against a pre-cut snapshot resurrects the row the cut ' + + 'retired AND drops the row queued after the snapshot was taken', + ) +}) + +test('confirming one cid leaves a concurrently-queued row alone', () => { + const { result } = renderHook(usePendingQueue) + result.current.add(fixtureMsg({ cid: 'steered', ts: 500 }), { inFlight: true }) + result.current.add(fixtureMsg({ cid: 'later', ts: 600 }), { inFlight: true }) + result.current.confirmQueued('later', { ts: 600 }) + result.current.confirmQueued('steered', { ts: 500 }) + const list = result.current.pendingMessagesRef.current + assert.deepEqual(list.map(m => m.cid), ['steered', 'later']) + assert.ok( + list.every(m => m.serverTs === true), + 'both rows are server-confirmed, so fast-forward stays available on both', + ) +}) diff --git a/frontend/src/components/ChatView/useStreamConnection.js b/frontend/src/components/ChatView/useStreamConnection.js index 390dfa25b..68ed4c71e 100644 --- a/frontend/src/components/ChatView/useStreamConnection.js +++ b/frontend/src/components/ChatView/useStreamConnection.js @@ -128,12 +128,19 @@ const BROADCAST_REGISTRATION_WINDOW_MS = 1500 * block in place (see streamReducers.js). * queued_turn_starting Backend about to promote a queued message * { ts }. Notifies caller via callback. - * steered_into_turn A send was steered into a live provider turn - * instead of queued { ts, content }. Codex uses - * true SDK steer; Claude interrupts and - * re-prompts. Notifies caller so it drops the - * optimistic queued-tray entry and renders the - * message inline as content growth. + * steered_into_turn THE CUT. The transcript split has committed: the + * pre-steer assistant segment is sealed and the + * steered user row(s) are in the transcript + * { ts, content, messages }. Published by the steer + * route for Codex (which splits there) and by the + * Claude runner at its seal. Notifies caller so it + * promotes the live stream into its own message, + * re-bases streamItems for the continuation, and + * renders the row inline as content growth. There is + * no separate "accepted" event: on the deferred + * (Claude) path the send's own 202 echoes the row as + * the still-queued row it is, so the tray has ONE + * reconciler until the cut arrives. * catch_up_done Replay burst finished; live events follow. * error { message }. Surfaced inline. * done Turn complete; SSE closes. @@ -159,8 +166,9 @@ const BROADCAST_REGISTRATION_WINDOW_MS = 1500 * identifies the first pending entry in the promoted group and `message` * is the backend-authoritative combined user message when available. * @param {(info: {ts: number|null, content: string}) => void} [callbacks.onSteeredIntoTurn] - * Fired when a send was steered into a live provider turn. The caller - * drops the optimistic queued-tray entry and renders the message inline. + * Fired when the steer's transcript split has COMMITTED. The caller seals the + * live stream into its own message, drops the queued-tray entry, and renders + * the steered message inline. * @param {(questionId: string|null) => void} [callbacks.onLiveQuestion] * Fired when the stream shows the currently-live AskUserQuestion card. * @@ -1061,9 +1069,10 @@ export default function useStreamConnection(chatId, { message: event.message || null, }) } else if (event.type === 'steered_into_turn') { - // The backend already put the user message in the transcript; - // the caller drops the optimistic queued-tray entry and renders - // it inline as content growth (no send-time spacer/scroll-pin). + // THE CUT. The backend has committed the split — the pre-steer + // assistant segment is sealed and the user message is in the + // transcript — so the caller drops the queued-tray entry and + // renders it inline as content growth (no send-time spacer/pin). // Flush the typewriter buffer FIRST so the pre-steer assistant // text is fully in latestItemsRef before the caller promotes it // to its own finished message — without this, the last frame of @@ -1072,16 +1081,35 @@ export default function useStreamConnection(chatId, { flushBuffer() if (isCatchUp) { // Replay during the catch-up burst (a mid-A2 reconnect or - // remount): the DB fetch already returned the sealed pre-steer - // assistant (A1) AND the steered user row (Q2), so promoting the - // replayed pre-steer text into a fresh message would DUPLICATE - // A1, and re-inserting Q2 is redundant. Drop the replayed + // remount): the sealed pre-steer assistant (A1) and the steered + // user row (Q2) are DB rows now, so promoting the replayed + // pre-steer text into a fresh message would DUPLICATE A1 and + // re-inserting Q2 would duplicate the row. Drop the replayed // pre-steer segment from streamItems so the post-steer // continuation (A2) accumulates fresh and promotes as its own // assistant after Q2. Clear the off-screen catch-up buffer; the // visible stream is replaced only when catch-up commits. + // + // This reconstructs the SERVER's boundary because the cut sits at + // its true position in the replayed log: the publisher is the seal + // itself, so every event before it belongs to A1 and every event + // after it belongs to A2. While the cut was published at HTTP + // arrival instead, the A1 blocks that followed it survived here + // and replayed as the head of A2 — the duplication, reproduced on + // every reconnect. catchUpItems = [] forceNewTextBlockRef.current = false + // Dropping the replayed segment only reconstructs the transcript + // if those DB rows are actually loaded, and this branch is exactly + // the case where they may not be: a socket that died before the + // cut arrived live means A1 was never promoted locally and Q2 was + // never inserted, so discarding the replay here would erase A1 + // from view and leave Q2 sitting in the tray until turn end. The + // cut cannot be applied on replay (promoting replayed items is + // the duplication), so the DB is the only place both rows exist — + // ask for it. One authoritative read at a known boundary; the + // event log replays the cut once per connection, not on a timer. + onNeedsRefreshRef.current?.({ force: true }) } else { onSteeredIntoTurnRef.current?.({ ts: event.ts ?? null, From e9cd21836129ead405d8424b7798f3c9ed829446 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:23:00 +0000 Subject: [PATCH 05/15] fix(chat): observe the node the card publishes, not a lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "tap to answer" cue for a pending question could stick on screen while the question card was fully visible and — because the turn is parked waiting on the answer — never clear on its own. Only a reload cleared it. The resume cue had the identical shape. `useOffscreenNudge` resolved its observed target with a one-shot `querySelector` inside its own layout effect, while callers passed a hand-maintained list of rebind triggers. The pending question card genuinely changes DOM node mid-turn: it is rendered by the live streaming surface first and by the durable message row after promotion. Any commit that performed that handoff without one of the enumerated triggers changing left the `IntersectionObserver` bound to a node React had already detached, so it reported nothing ever again. With the turn parked awaiting an answer, nothing re-renders to rebind it. Whether a refresh landed inside that particular commit was a race, which is why it presented as intermittent. The hook now takes the ELEMENT rather than a finder plus a trigger list, so node identity IS the effect dependency and a stale observer becomes unrepresentable. Identity is published by `useNudgeTargetRef()`, a `useState`-backed callback ref that is stable across renders, threaded to `QuestionCard` through both render paths, so the live-to-durable handoff reaches the observer as an ordinary node swap. Only the card that actually blocks the turn publishes: the unanswered tail question, and the tail resumable note. The tail gate lives at the publication site because `MsgContent` renders a Resume button on every resumable block of the last message and one shared callback ref has exactly one slot — the same `.pop()` semantics the old `querySelectorAll` lookup had, now expressed where it cannot drift from what arms the cue. Tests: frontend `npm test` 1977 passed, 0 failed (1967 before). Nine new `useOffscreenNudge` cases cover a card remounting onto a new node, the live-to-durable handoff (including a batched one, which must never blank the cue), a handoff onto an already-visible row, the cue retiring when the pending state ends, ref stability across renders so memoized rows keep publishing, and observer release on unmount. `resumeAffordance.test.js` pins that both nudges observe a node published by the card rather than a lookup. The hook test shim gained real effect cleanup and `unmount()` semantics, without which a leaked observer is indistinguishable from a released one. Mutation-checked: restoring the lookup-based bind fails a named test. Frontend only; no restart required. Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- .../ChatView/ActiveAssistantSurface.jsx | 4 + frontend/src/components/ChatView/ChatView.jsx | 52 ++-- .../src/components/ChatView/MsgContent.jsx | 31 +++ .../src/components/ChatView/QuestionCard.jsx | 9 + .../components/ChatView/StreamingMessage.jsx | 4 + .../__tests__/resumeAffordance.test.js | 86 +++++++ .../hooks/__tests__/react-hook-shim.mjs | 50 +++- .../hooks/__tests__/useOffscreenNudge.test.js | 236 ++++++++++++++++-- .../ChatView/hooks/useOffscreenNudge.js | 49 ++-- 9 files changed, 450 insertions(+), 71 deletions(-) diff --git a/frontend/src/components/ChatView/ActiveAssistantSurface.jsx b/frontend/src/components/ChatView/ActiveAssistantSurface.jsx index 7828949c5..867e4b5c3 100644 --- a/frontend/src/components/ChatView/ActiveAssistantSurface.jsx +++ b/frontend/src/components/ChatView/ActiveAssistantSurface.jsx @@ -32,6 +32,8 @@ function ActiveAssistantSurface({ onAutoResumeChange, submissionBlocked, liveQuestionId, + pendingQuestionRef, + resumeCardRef, isStreaming, }) { const msg = useMemo(() => { @@ -63,6 +65,8 @@ function ActiveAssistantSurface({ onAutoResumeChange={onAutoResumeChange} submissionBlocked={submissionBlocked} liveQuestionId={liveQuestionId} + pendingQuestionRef={pendingQuestionRef} + resumeCardRef={resumeCardRef} isStreaming={isStreaming} /> ) diff --git a/frontend/src/components/ChatView/ChatView.jsx b/frontend/src/components/ChatView/ChatView.jsx index 92127323e..4944fa8c3 100644 --- a/frontend/src/components/ChatView/ChatView.jsx +++ b/frontend/src/components/ChatView/ChatView.jsx @@ -23,7 +23,7 @@ import { getOnlineSnapshot } from '../../lib/connectivityStore.js' import useSystemEventStream from '../../hooks/useSystemEventStream.js' import usePendingQueue from './hooks/usePendingQueue.js' import useBridgePartial from './hooks/useBridgePartial.js' -import useOffscreenNudge from './hooks/useOffscreenNudge.js' +import useOffscreenNudge, { useNudgeTargetRef } from './hooks/useOffscreenNudge.js' import ChatInputBar from './ChatInputBar.jsx' import { hasSendablePayload } from './composerSubmission.js' import AgentContextInspector from './AgentContextInspector.jsx' @@ -421,7 +421,8 @@ export default function ChatView({ // The pending-question and resume "tap to jump to it" nudges each track // whether their card is scrolled out of the viewport. Both use one shared // observer hook (useOffscreenNudge, below); their booleans are computed near - // hasPendingQuestion / hasPendingResume where the card finders live. + // hasPendingQuestion / hasPendingResume, alongside the callback refs the + // cards themselves use to publish the node being observed. const [showInspector, setShowInspector] = useState(false) const [showSummary, setShowSummary] = useState(false) const [visibleTimestampKey, setVisibleTimestampKey] = useState(null) @@ -3518,30 +3519,30 @@ export default function ChatView({ } }, [pendingLimitResetAt]) - // Visibility of that card is a pure viewport question — an - // IntersectionObserver rooted at the scroll container is the signal, - // no scroll math and no interaction with the spacer machinery. The - // card's DOM node is stable across streaming ticks (keyed children), - // so the observer only needs re-binding when the rendering surface - // can change: pending-flag flips, stream↔messages promotion, or a - // messages commit. Both nudges share useOffscreenNudge (above). - // The LAST un-answered card is the pending one: it lives in the last - // assistant message or the streaming <li>. - const findPendingQuestionCard = () => - [...(scrollRef.current?.querySelectorAll('.qcard:not(.qcard--answered)') ?? [])].pop() + // Visibility of either card is a pure viewport question — an + // IntersectionObserver rooted at the scroll container is the signal, no + // scroll math and no interaction with the spacer machinery. ChatView must + // NOT look the card up: the pending question moves between two rendering + // surfaces (the live streaming <li> and the durable message row) at a moment + // ChatView cannot enumerate, and a lookup taken at bind time then observes a + // node React has since detached — with the turn parked on the answer nothing + // re-renders, so the cue would stick forever. Instead the element that IS + // the card publishes its node through these refs (see useNudgeTargetRef); + // both surfaces publish through the same channel, so the live→durable + // handoff reaches the observer as an ordinary node swap. + const [pendingQuestionEl, pendingQuestionRef] = useNudgeTargetRef() const pendingCardOffscreen = useOffscreenNudge( - scrollRef, hasPendingQuestion, findPendingQuestionCard, - [showActiveAssistantSurface, messages], + scrollRef, hasPendingQuestion, pendingQuestionEl, ) - // The resume card: only the tail resumable note renders `.chat__resume` - // (MsgContent gates the button on isLastMsg), so observing that button is - // enough to know the card's visibility; a tap on the nudge scrolls it in. - const findResumeCard = () => - [...(scrollRef.current?.querySelectorAll('.chat__resume') ?? [])].pop() + // The resume card publishes the same way, from the TAIL resumable note only + // — the same block tailResumableBlock arms the cue on. (MsgContent renders a + // Resume button on every resumable block of the last message, so the tail + // gate lives at the publication site; one shared ref can only hold one + // node.) A tap on the nudge scrolls that node in. + const [resumeCardEl, resumeCardRef] = useNudgeTargetRef() const resumeCardOffscreen = useOffscreenNudge( - scrollRef, hasPendingResume, findResumeCard, - [showActiveAssistantSurface, messages], + scrollRef, hasPendingResume, resumeCardEl, ) // The ONE active <li> carries this data-key for both DB and live payloads. @@ -3846,6 +3847,8 @@ export default function ChatView({ isLastMsg={isLastMsg} liveQuestionId={liveQuestionId} suppressedQuestionKeys={streamItemQuestionKeys} + pendingQuestionRef={pendingQuestionRef} + resumeCardRef={resumeCardRef} /> {msg.ts && ownerUserMessage && ( <time className={`chat__ts${visibleTimestampKey === dataKey ? ' chat__ts--visible' : ''}`}> @@ -3879,6 +3882,11 @@ export default function ChatView({ onAutoResumeChange={handleAutoResumeChange} submissionBlocked={providerSwitching} liveQuestionId={liveQuestionId} + // Same publication channel as the durable rows above: while the + // turn is live THIS surface owns the pending question card, so + // the offscreen observer follows the handoff automatically. + pendingQuestionRef={pendingQuestionRef} + resumeCardRef={resumeCardRef} // Liveness for the ACTIVE surface follows the TURN, not the // payload source: when a richer DB partial wins source selection // (useDbActivePayload, e.g. through the reconnect catch-up diff --git a/frontend/src/components/ChatView/MsgContent.jsx b/frontend/src/components/ChatView/MsgContent.jsx index 4abd666ef..62f0175f6 100644 --- a/frontend/src/components/ChatView/MsgContent.jsx +++ b/frontend/src/components/ChatView/MsgContent.jsx @@ -56,6 +56,15 @@ function MsgContentInner({ // fresh function reference every render and defeat it. isLastMsg, liveQuestionId, + // Callback refs (ChatView's useNudgeTargetRef) that publish the DOM node of + // the card each footer nudge watches. This renderer serves BOTH surfaces of + // the active answer — the durable message row and the live streaming <li> — + // so a mid-turn surface handoff reaches the observer as a plain node swap + // instead of leaving it bound to a node React has detached. Only the card + // that actually blocks the turn publishes: the answerable tail question and + // the resumable tail note. + pendingQuestionRef, + resumeCardRef, // Active answers always use this renderer for both their DB partial and // live SSE payload. isStreaming only enables the cursor, aria-live, and the // active thinking timer; it never selects a different component tree. @@ -209,6 +218,7 @@ function MsgContentInner({ answeredMap={answers} onAnswer={answerable ? onQuestionAnswer : undefined} disabled={!answerable && !answers} + pendingCardRef={answerable ? pendingQuestionRef : undefined} /> </div> ) @@ -268,6 +278,18 @@ function MsgContentInner({ <button type="button" className="chat__resume" + // Publish ONLY the tail note's button. `resumable` gates on + // isLastMsg, not on tail position, so a last message holding + // two resumable error blocks renders two buttons — and one + // shared callback ref has ONE slot: the later mount wins, then + // an unmount of the OTHER button calls the ref with null and + // the cue goes dark while the real target is still on screen + // and offscreen. The tail is also exactly what arms the cue + // (ChatView's tailResumableBlock), so gating here keeps the + // observed node and the `active` flag talking about the same + // block. The old querySelectorAll lookup took `.pop()`, which + // is what this reproduces. + ref={i === lastEntryIdx ? resumeCardRef : undefined} onClick={() => onResume('continue')} disabled={submissionBlocked} title={submissionBlocked @@ -387,6 +409,15 @@ export default memo(MsgContentInner, (prev, next) => { && prev.submissionBlocked === next.submissionBlocked && prev.isLastMsg === next.isLastMsg && prev.liveQuestionId === next.liveQuestionId + // The nudge refs are props, so an exhaustive comparator has to include + // them: a ref whose identity changed must reach the DOM node, or React + // never re-invokes it and the observer keeps the old channel. In practice + // they never change (useNudgeTargetRef memoizes both with []), which is + // why these two lines cost nothing. Skipping a re-render does NOT + // unpublish anything — React only re-runs a callback ref when the ref or + // the host node changes, and a bailed-out render changes neither. + && prev.pendingQuestionRef === next.pendingQuestionRef + && prev.resumeCardRef === next.resumeCardRef && prev.isActiveAnswer === next.isActiveAnswer && prev.isStreaming === next.isStreaming // suppressedQuestionKeys is a Set (new reference each render) or null. diff --git a/frontend/src/components/ChatView/QuestionCard.jsx b/frontend/src/components/ChatView/QuestionCard.jsx index 1330cd99d..dd828d3ac 100644 --- a/frontend/src/components/ChatView/QuestionCard.jsx +++ b/frontend/src/components/ChatView/QuestionCard.jsx @@ -25,6 +25,14 @@ export default function QuestionCard({ answeredMap, onAnswer, disabled, + // Callback ref that publishes this card's node to the "Möbius asked you + // something — tap to answer" offscreen observer. Set only by the surface + // rendering the answerable tail question, and only while the card is still + // unanswered — the cue exists to send the owner back to a card that is + // blocking the turn, and a submitted card no longer is. Because a live→ + // durable surface handoff remounts this component, the observer's target + // has to come from here (the node's own render) rather than a lookup. + pendingCardRef, }) { const draftKey = questionDraftKey(chatId, questionId, questions) const [answers, setAnswers] = useState( @@ -137,6 +145,7 @@ export default function QuestionCard({ return ( <div className={`qcard${answered ? ' qcard--answered' : ''}`} + ref={answered ? null : pendingCardRef} aria-disabled={disabled && !answered ? true : undefined} > {questions.map((q, qi) => { diff --git a/frontend/src/components/ChatView/StreamingMessage.jsx b/frontend/src/components/ChatView/StreamingMessage.jsx index 2324d92f6..337564133 100644 --- a/frontend/src/components/ChatView/StreamingMessage.jsx +++ b/frontend/src/components/ChatView/StreamingMessage.jsx @@ -22,6 +22,8 @@ export default function StreamingMessage({ onAutoResumeChange, submissionBlocked, liveQuestionId, + pendingQuestionRef, + resumeCardRef, isStreaming, }) { return ( @@ -45,6 +47,8 @@ export default function StreamingMessage({ submissionBlocked={submissionBlocked} isLastMsg liveQuestionId={liveQuestionId} + pendingQuestionRef={pendingQuestionRef} + resumeCardRef={resumeCardRef} isActiveAnswer isStreaming={isStreaming} suppressedQuestionKeys={null} diff --git a/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js b/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js index 35371d201..e143506ab 100644 --- a/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js +++ b/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js @@ -10,6 +10,25 @@ const msgContent = readFileSync(new URL('../MsgContent.jsx', import.meta.url), ' const chatView = readFileSync(new URL('../ChatView.jsx', import.meta.url), 'utf8') const css = readFileSync(new URL('../ChatView.css', import.meta.url), 'utf8') +// Slice ONE JSX element's own text, from its tag to the matching close of that +// tag. A source assertion about an element's props is worthless without this: +// with an unbounded `[\s\S]*?` between the tag and the prop, a DIFFERENT +// element's props further down the file satisfy the match, so "both render +// paths pass the ref" can pass with one of them not passing it at all. +function sliceElement(source, openTag) { + const from = source.indexOf(openTag) + assert.ok(from >= 0, `expected to find ${openTag}`) + let depth = 0 + for (let i = from; i < source.length; i++) { + if (source[i] === '<') depth++ + if (source[i] === '>') { + depth-- + if (depth === 0) return source.slice(from, i + 1) + } + } + assert.fail(`unterminated element ${openTag}`) +} + test('MsgContent gates the Resume button on a resumable tail note', () => { assert.match(msgContent, /onResume/, 'MsgContent must accept an onResume prop') @@ -80,6 +99,73 @@ test('ChatView routes both offscreen attention nudges through the controller', ( 'the resume nudge reuses the question-nudge visual style') }) +test('both attention nudges observe a node published by the card, not a lookup', () => { + // The nudges track a card that changes DOM node mid-turn: the live streaming + // surface renders the pending question while the turn runs, the durable + // message row renders it once the turn parks. A querySelector taken when the + // observer binds cannot see that swap, so the observer was left on a detached + // node — and with the turn parked awaiting the answer nothing re-renders, so + // the pill stuck forever. Node identity has to be the hook's input. + assert.doesNotMatch(chatView, /querySelectorAll\('\.qcard|querySelectorAll\('\.chat__resume/, + 'neither nudge may locate its card by query') + assert.match(chatView, /const \[pendingQuestionEl, pendingQuestionRef\] = useNudgeTargetRef\(\)/, + 'the pending question card publishes its node through a callback ref') + assert.match(chatView, /useOffscreenNudge\(\s*scrollRef, hasPendingQuestion, pendingQuestionEl,/, + 'the question nudge observes the published node') + assert.match(chatView, /const \[resumeCardEl, resumeCardRef\] = useNudgeTargetRef\(\)/, + 'the resume card publishes its node the same way — no parallel mechanism') + assert.match(chatView, /useOffscreenNudge\(\s*scrollRef, hasPendingResume, resumeCardEl,/, + 'the resume nudge observes the published node') + + // BOTH render paths must publish through the SAME ref so the live→durable + // handoff reaches the observer as an ordinary node swap. Each element is + // sliced to its OWN text first: an unbounded wildcard between the tag and the + // prop lets one call site satisfy both patterns, which makes deleting the + // refs from the durable row — the reported bug, exactly — undetectable. + for (const [label, openTag] of [ + ['durable message rows', '<MsgContent'], + ['the live active surface', '<ActiveAssistantSurface'], + ]) { + const element = sliceElement(chatView, openTag) + assert.match(element, /pendingQuestionRef=\{pendingQuestionRef\}/, + `${label} must publish the question card through the shared ref`) + assert.match(element, /resumeCardRef=\{resumeCardRef\}/, + `${label} must publish the resume card through the shared ref`) + } + // The live surface reaches MsgContent through two more components, and a hop + // that accepts the prop without forwarding it kills the cue for the whole + // live half of the turn — silently, since the durable half still works. + for (const [file, child] of [ + ['../ActiveAssistantSurface.jsx', '<StreamingMessage'], + ['../StreamingMessage.jsx', '<MsgContent'], + ]) { + const source = readFileSync(new URL(file, import.meta.url), 'utf8') + const element = sliceElement(source, child) + for (const prop of ['pendingQuestionRef', 'resumeCardRef']) { + assert.match(element, new RegExp(`${prop}=\\{${prop}\\}`), + `${file} must forward ${prop} to ${child}`) + } + } + // Only the card that actually blocks the turn registers: an answered question + // or a scrolled-back history card is not somewhere to send the owner back to. + assert.match(msgContent, /pendingCardRef=\{answerable \? pendingQuestionRef : undefined\}/, + 'only the answerable tail question publishes its node') + // ONE publisher per ref. `resumable` gates on isLastMsg, not tail position, so + // a last message with two resumable blocks renders two Resume buttons; a + // shared single-slot ref would then be nulled by whichever unmounts first and + // the cue would go dark with the real button still offscreen. The tail is also + // what arms the cue (tailResumableBlock), so both must name the same block. + const resumeButton = sliceElement(msgContent, '<button') + assert.match(resumeButton, /className="chat__resume"/, + 'the sliced element is the Resume button') + assert.match(resumeButton, /ref=\{i === lastEntryIdx \? resumeCardRef : undefined\}/, + 'only the TAIL resumable note may publish the observed node') + const questionCard = readFileSync(new URL('../QuestionCard.jsx', import.meta.url), 'utf8') + const qcard = sliceElement(questionCard, '<div\n className={`qcard') + assert.match(qcard, /ref=\{answered \? (null|undefined) : pendingCardRef\}/, + 'a submitted card retires itself from the cue') +}) + test('ariaStatus announces the recovery state instead of "Response ready."', () => { assert.match(chatView, /Turn paused — Resume available\./, 'a paused turn announces the recovery state, not readiness') diff --git a/frontend/src/components/ChatView/hooks/__tests__/react-hook-shim.mjs b/frontend/src/components/ChatView/hooks/__tests__/react-hook-shim.mjs index 1729e004c..84af15d1a 100644 --- a/frontend/src/components/ChatView/hooks/__tests__/react-hook-shim.mjs +++ b/frontend/src/components/ChatView/hooks/__tests__/react-hook-shim.mjs @@ -15,7 +15,10 @@ // matching React's "layout effects fire after commit" timing without a // real DOM commit cycle. Dep semantics follow React: Object.is per // element; undefined deps = fire every render; [] = fire once; [a,b] = -// fire when a or b changes by identity. +// fire when a or b changes by identity. A function returned by an effect +// is retained as its cleanup and invoked before that effect fires again, +// like React — a hook that owns an external subscription (an +// IntersectionObserver) cannot be tested honestly without that teardown. // // Why this instead of @testing-library/react-hooks: zero new // devDependencies, fits the Möbius preference for keeping the @@ -80,7 +83,7 @@ export function useCallback(fn, deps) { function _scheduleEffect(fn, deps) { const i = _slotIndex++ if (_slots[i] === undefined) { - _slots[i] = { prevDeps: _UNSET } + _slots[i] = { prevDeps: _UNSET, cleanup: null } } const slot = _slots[i] // Fire on: first render (prevDeps === _UNSET), no dep array @@ -92,7 +95,7 @@ function _scheduleEffect(fn, deps) { deps.length !== slot.prevDeps.length || deps.some((d, idx) => !Object.is(d, slot.prevDeps[idx])) slot.prevDeps = deps === undefined ? _UNSET : deps - if (shouldFire) _pendingEffects.push(fn) + if (shouldFire) _pendingEffects.push({ slot, fn }) } // useLayoutEffect and useEffect collapse to the same scheduling here: @@ -110,28 +113,44 @@ export function useEffect(fn, deps) { function _flushEffects() { // Drain in registration order. Effects that call setState would // re-trigger _rerender → run → another flush; the hooks tested here - // only mutate refs inside effects, so the recursion concern is - // theoretical. If you hit it, gate _flushEffects behind a depth - // counter or move setState callers to useEffect-with-deferred-flush. + // only mutate refs or re-set the same state inside effects, so the + // recursion concern is theoretical. If you hit it, gate _flushEffects + // behind a depth counter or move setState callers to + // useEffect-with-deferred-flush. const toRun = _pendingEffects.splice(0) - for (const fn of toRun) fn() + for (const { slot, fn } of toRun) { + // Tear down the previous subscription before re-establishing it, so a + // re-fire cannot leave two live observers behind (React's order too). + if (typeof slot.cleanup === 'function') slot.cleanup() + const cleanup = fn() + slot.cleanup = typeof cleanup === 'function' ? cleanup : null + } } /** * Run a hook function as if React were mounting it. Returns a - * { result, rerender } pair; `result.current` reflects the latest - * return value, and `rerender(...args)` re-invokes the hook with - * fresh arguments while preserving slot state. + * { result, rerender, unmount } triple; `result.current` reflects the + * latest return value, `rerender(...args)` re-invokes the hook with + * fresh arguments while preserving slot state, and `unmount()` runs + * every retained effect cleanup like React's teardown. * * Effects (useLayoutEffect / useEffect) registered during the hook * call are flushed synchronously after hookFn returns, so callers * can assert on ref values that effects set without an `act` wrapper. + * + * `unmount` exists because a hook that owns an external subscription + * (an IntersectionObserver) has a teardown path that no dep change can + * reach: without it, "the observer is disconnected when the component + * goes away" is untestable and a leaked observer looks identical to a + * released one. */ export function renderHook(hookFn, ...initialArgs) { __reset() const result = { current: undefined } let currentArgs = initialArgs + let unmounted = false function run() { + if (unmounted) return _slotIndex = 0 result.current = hookFn(...currentArgs) _flushEffects() @@ -144,5 +163,16 @@ export function renderHook(hookFn, ...initialArgs) { currentArgs = nextArgs.length > 0 ? nextArgs : currentArgs run() }, + unmount: () => { + if (unmounted) return + unmounted = true + // React tears effects down in the order they were registered. + for (const slot of _slots) { + if (slot && typeof slot.cleanup === 'function') { + slot.cleanup() + slot.cleanup = null + } + } + }, } } diff --git a/frontend/src/components/ChatView/hooks/__tests__/useOffscreenNudge.test.js b/frontend/src/components/ChatView/hooks/__tests__/useOffscreenNudge.test.js index 1d8f78769..ae54d05c4 100644 --- a/frontend/src/components/ChatView/hooks/__tests__/useOffscreenNudge.test.js +++ b/frontend/src/components/ChatView/hooks/__tests__/useOffscreenNudge.test.js @@ -7,6 +7,7 @@ import { renderHook } from './react-hook-shim.mjs' import useOffscreenNudge, { isElementOffscreen, isIntersectionOffscreen, + useNudgeTargetRef, } from '../useOffscreenNudge.js' @@ -19,6 +20,47 @@ function elementAt(top, bottom) { } +// Records every observer the hook creates so a test can prove WHICH node is +// actually being watched. The defect class here is an observer left on a node +// React has detached — invisible if you only read the returned boolean. +function withObserverSpy(run) { + const original = globalThis.IntersectionObserver + const created = [] + class SpyObserver { + constructor(callback) { + this.callback = callback + this.observed = [] + this.disconnected = false + created.push(this) + } + + observe(element) { this.observed.push(element) } + + disconnect() { this.disconnected = true } + + // Drive the async half of the contract: the browser invokes this after + // paint whenever the observed node crosses the root's bounds. + report(isIntersecting) { this.callback([{ isIntersecting }]) } + } + globalThis.IntersectionObserver = SpyObserver + try { + return run(created) + } finally { + globalThis.IntersectionObserver = original + } +} + + +// The real wiring: the card publishes its own node through a callback ref and +// the hook watches whatever is currently published. Mirrors ChatView, which +// hands the SAME ref to the live streaming surface and the durable message row. +function useNudgedCard(scrollRef, active) { + const [element, cardRef] = useNudgeTargetRef() + const offscreen = useOffscreenNudge(scrollRef, active, element) + return { offscreen, cardRef } +} + + test('visible targets are not offscreen, including partial visibility', () => { const viewport = elementAt(100, 500) assert.equal(isElementOffscreen(viewport, elementAt(150, 450)), false) @@ -48,36 +90,184 @@ test('observer delivery requires positive visible area', () => { }) test('mount computes committed geometry before observer delivery', () => { - const originalObserver = globalThis.IntersectionObserver - class IdleObserver { - observe() {} - disconnect() {} - } - globalThis.IntersectionObserver = IdleObserver - - try { + withObserverSpy(() => { const scrollRef = { current: elementAt(100, 500) } const visible = elementAt(200, 400) const { result, rerender } = renderHook( - useOffscreenNudge, - scrollRef, - true, - () => visible, - ['messages-v1'], + useOffscreenNudge, scrollRef, true, visible, ) assert.equal(result.current, false, 'a visible question must not wait for IntersectionObserver to hide the cue') const hiddenBelow = elementAt(600, 800) - rerender( - scrollRef, - true, - () => hiddenBelow, - ['messages-v2'], - ) + rerender(scrollRef, true, hiddenBelow) assert.equal(result.current, true, - 'a newly rebound offscreen question is exposed in the same layout pass') - } finally { - globalThis.IntersectionObserver = originalObserver - } + 'a newly bound offscreen question is exposed in the same layout pass') + }) +}) + +test('a card publishing through the target ref is observed once it mounts', () => { + withObserverSpy(created => { + const scrollRef = { current: elementAt(100, 500) } + const { result } = renderHook(useNudgedCard, scrollRef, true) + assert.equal(result.current.offscreen, false, + 'no card has mounted yet, so there is nowhere to nudge the owner toward') + assert.equal(created.length, 0) + + const card = elementAt(600, 800) + result.current.cardRef(card) + assert.equal(result.current.offscreen, true) + assert.equal(created.length, 1) + assert.deepEqual(created[0].observed, [card]) + }) +}) + +test('a question card that re-mounts onto a new DOM node keeps the offscreen cue truthful', () => { + withObserverSpy(created => { + // The turn is parked on the question, so nothing but the card's own node + // changes in this commit — exactly the case the old find-at-bind contract + // missed. The observer stayed on the detached streaming node, and with no + // further renders coming the pill could never clear again. + const scrollRef = { current: elementAt(100, 500) } + const { result } = renderHook(useNudgedCard, scrollRef, true) + + const liveCard = elementAt(600, 800) + result.current.cardRef(liveCard) + assert.equal(created.length, 1) + + // Live streaming surface → durable message row: React detaches the removed + // node's ref, then attaches the new row's. + result.current.cardRef(null) + const durableCard = elementAt(620, 820) + result.current.cardRef(durableCard) + + assert.equal(created[0].disconnected, true, + 'the observer on the detached streaming node must be torn down') + const bound = created[created.length - 1] + assert.deepEqual(bound.observed, [durableCard], + 'the node currently rendering the card is the node being observed') + assert.equal(bound.disconnected, false) + assert.equal(result.current.offscreen, true, + 'the card is still out of view, so the cue must still be showing') + }) +}) + +test('the cue clears when the card scrolls into view after a live-to-durable handoff', () => { + withObserverSpy(created => { + const scrollRef = { current: elementAt(100, 500) } + const { result } = renderHook(useNudgedCard, scrollRef, true) + + result.current.cardRef(elementAt(600, 800)) + result.current.cardRef(null) + result.current.cardRef(elementAt(620, 820)) + assert.equal(result.current.offscreen, true) + + // Post-handoff scroll. Only an observer bound to the CURRENT node can + // report this, so a stale binding surfaces here as a stuck cue. + created[created.length - 1].report(true) + assert.equal(result.current.offscreen, false) + }) +}) + +test('a handoff onto an already-visible row clears the cue without waiting for the observer', () => { + withObserverSpy(() => { + const scrollRef = { current: elementAt(100, 500) } + const { result } = renderHook(useNudgedCard, scrollRef, true) + + result.current.cardRef(elementAt(600, 800)) + assert.equal(result.current.offscreen, true) + + // The durable row renders where the owner is already looking. The + // synchronous pre-paint recompute owns this: IntersectionObserver does not + // deliver until after the next paint, which would leave a frame of cue + // pointing at a card that is on screen. + result.current.cardRef(null) + result.current.cardRef(elementAt(200, 400)) + assert.equal(result.current.offscreen, false) + }) +}) + +test('an unmounting card clears the cue and releases its observer', () => { + withObserverSpy(created => { + const scrollRef = { current: elementAt(100, 500) } + const { result } = renderHook(useNudgedCard, scrollRef, true) + + result.current.cardRef(elementAt(600, 800)) + assert.equal(result.current.offscreen, true) + + result.current.cardRef(null) + assert.equal(result.current.offscreen, false, + 'there is no card left to send the owner back to') + assert.equal(created[0].disconnected, true) + }) +}) + +test('the cue retires when the pending state ends even with the card still mounted', () => { + withObserverSpy(created => { + const scrollRef = { current: elementAt(100, 500) } + const offscreenCard = elementAt(600, 800) + const { result, rerender } = renderHook( + useOffscreenNudge, scrollRef, true, offscreenCard, + ) + assert.equal(result.current, true) + + rerender(scrollRef, false, offscreenCard) + assert.equal(result.current, false, + 'an answered question retires the cue regardless of geometry') + assert.equal(created[0].disconnected, true) + }) +}) + +test('the target ref is stable across renders so memoized rows keep publishing', () => { + const { result } = renderHook(useNudgeTargetRef) + const [, first] = result.current + const node = elementAt(0, 10) + first(node) + const [element, second] = result.current + assert.equal(element, node) + assert.equal(second, first, + 'a fresh ref identity every render would churn every card it is passed to') +}) + +test('leaving the chat disconnects the observer', () => { + // The teardown no dep change can reach. Without it a chat switch leaves a + // live IntersectionObserver rooted at the old scroll container for every + // parked question the owner ever scrolled away from. + withObserverSpy(created => { + const scrollRef = { current: elementAt(100, 500) } + const { result, unmount } = renderHook(useNudgedCard, scrollRef, true) + + result.current.cardRef(elementAt(600, 800)) + assert.equal(created.length, 1) + assert.equal(created[0].disconnected, false) + + unmount() + assert.equal(created[0].disconnected, true, + 'the observer must be released with the component, not leaked') + }) +}) + +test('a batched live-to-durable handoff never blanks the cue', () => { + // React commits the retiring surface's `ref(null)` and the arriving + // surface's `ref(node)` in ONE pass, so the intermediate "no element" state + // is never rendered. The hook must therefore reach the new node's geometry + // from a single effect run — a handoff that only works because it was + // observed as two commits would flicker the cue for a frame in the browser. + withObserverSpy(created => { + const scrollRef = { current: elementAt(100, 500) } + const offscreenLive = elementAt(600, 800) + const offscreenDurable = elementAt(700, 900) + const { result, rerender } = renderHook( + useOffscreenNudge, scrollRef, true, offscreenLive, + ) + assert.equal(result.current, true) + assert.deepEqual(created[0].observed, [offscreenLive]) + + // One render, one new node: exactly what a batched handoff commits. + rerender(scrollRef, true, offscreenDurable) + assert.equal(result.current, true, 'the cue stays up across the handoff') + assert.equal(created.length, 2, 'the observer re-binds to the new node') + assert.deepEqual(created[1].observed, [offscreenDurable]) + assert.equal(created[0].disconnected, true, 'the old observer is released') + }) }) diff --git a/frontend/src/components/ChatView/hooks/useOffscreenNudge.js b/frontend/src/components/ChatView/hooks/useOffscreenNudge.js index b041aa140..b2b31ecf3 100644 --- a/frontend/src/components/ChatView/hooks/useOffscreenNudge.js +++ b/frontend/src/components/ChatView/hooks/useOffscreenNudge.js @@ -1,7 +1,7 @@ /* Tracks whether a footer attention target is outside the chat viewport. */ import { + useCallback, useLayoutEffect, - useRef, useState, } from 'react' @@ -22,19 +22,38 @@ export function isIntersectionOffscreen(entry) { } -// `findElement` is a fresh closure every render (it reads the live scroll -// ref), so keep it out of the dependency list. Rebinding on every streamed -// token would replace the observer continuously; callers instead provide the -// rendering-surface values that can replace the target node. -export default function useOffscreenNudge( - scrollRef, - active, - findElement, - rebindDeps, -) { +/** + * Publishes the DOM node a nudge watches, as a render input. + * + * The watched card is NOT one durable node. A pending question is rendered by + * the live streaming <li> while the turn runs and by the DURABLE message row + * once the turn parks (streamItemQuestionKeys suppresses whichever copy is not + * current), so React genuinely mounts a NEW element mid-turn. Node identity is + * therefore the only honest dependency for the observer below, and it must + * arrive through STATE: a `useRef` mutation does not re-render, so the effect + * would never re-run and the observer would stay bound to the detached node. + * With the turn parked awaiting an answer nothing else re-renders, so that + * observer is the only signal that could ever clear the cue. + * + * Returns `[element, ref]`. Attach `ref` to the element that IS the target, + * and only while it is the pending one; every render path for that card must + * publish through this same channel so a surface handoff is just a node swap. + */ +export function useNudgeTargetRef() { + const [element, setElement] = useState(null) + // Stable identity: this ref is passed as a prop across memoized component + // boundaries (MsgContent, ActiveAssistantSurface), which compare by identity. + const ref = useCallback(node => setElement(node ?? null), []) + return [element, ref] +} + + +// `element` is the node to watch — the hook is not given a way to FIND it. +// Handing it the node makes a stale observer unrepresentable: any swap of the +// rendering surface changes this identity, so the effect re-binds by +// construction instead of relying on a caller to enumerate rebind triggers. +export default function useOffscreenNudge(scrollRef, active, element) { const [offscreen, setOffscreen] = useState(false) - const findElementRef = useRef(findElement) - findElementRef.current = findElement useLayoutEffect(() => { if (!active) { @@ -43,7 +62,6 @@ export default function useOffscreenNudge( } const scrollEl = scrollRef.current - const element = findElementRef.current() if (!scrollEl || !element) { setOffscreen(false) return undefined @@ -60,8 +78,7 @@ export default function useOffscreenNudge( }, { root: scrollEl, threshold: 0 }) observer.observe(element) return () => observer.disconnect() - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [active, scrollRef, ...rebindDeps]) + }, [active, scrollRef, element]) return offscreen } From 585efc9d876a07ee366154b717eb733e4b24935d Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:36:46 +0000 Subject: [PATCH 06/15] Merge platform updates as one reviewed change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/platform_update.py | 199 ++++++++++++++----------- backend/scripts/entrypoint.sh | 14 +- backend/scripts/seed-skills/theming.md | 3 +- backend/tests/test_platform_update.py | 134 ++++++++++++----- 4 files changed, 218 insertions(+), 132 deletions(-) diff --git a/backend/app/platform_update.py b/backend/app/platform_update.py index 70fd4a785..4ea194f1a 100644 --- a/backend/app/platform_update.py +++ b/backend/app/platform_update.py @@ -1,12 +1,12 @@ -"""Platform self-update — clone-native ``git fetch`` + rebase reconcile. +"""Platform self-update — clone-native ``git fetch`` + merge reconcile. ``/data/platform`` is a real ``git clone`` of the canonical repo; uvicorn serves its backend directly (``cd /data/platform/backend && uvicorn app.main:app``). Local ``main`` carries the agent's edits; the ``upstream`` branch records the commit the clone was last reconciled to (set to HEAD at clone time). A deploy ships a new image AND advances canonical ``origin/main``; this module makes that -deploy actually REACH a running instance by fetching origin and replaying the -local edits onto the new upstream — on boot (before uvicorn imports the code, so +deploy actually REACH a running instance by fetching origin and merging it with +the local edits — on boot (before uvicorn imports the code, so the update goes live automatically) and on owner-triggered Apply. Owner Apply pins the exact target returned by the review plan even if its fetch observes a newer remote head; backend changes then need a restart to load. @@ -14,18 +14,18 @@ The reconcile is built to be non-destructive above all else: 1. ``/data/platform`` holds the SERVED backend, so a reconcile must never leave a - half-applied tree. A rebase conflict is aborted back to the pre-reconcile + half-applied tree. A merge conflict is aborted back to the pre-reconcile commit (the old, working code keeps serving) and surfaced as a conflict; a - crash mid-rebase is detected on the next boot (``.git/rebase-merge``) and - aborted before anything else runs. + crash mid-merge is detected on the next boot and aborted before anything + else runs. Legacy interrupted rebases are still cleaned up too. 2. Local edits are NEVER lost. Uncommitted working-tree edits are committed onto - ``main`` before any fast-forward/rebase, so a fast-forward ``reset --hard`` or - a rebase can only ever replay them, never discard them. A conflict or an + ``main`` before any fast-forward/merge, so either operation starts from a + durable local tip. A conflict or an import-broken result rolls the served tree back to exactly those local edits. -3. A text-clean rebase can still produce a tree that fails to import (e.g. - upstream deleted a module a local edit still imports). A post-rebase import +3. A text-clean merge can still produce a tree that fails to import (e.g. + upstream deleted a module a local edit still imports). A post-merge import probe catches that and rolls back to the previous served commit rather than serving a broken tree. @@ -35,7 +35,9 @@ module reuses ``app_git``'s isolated git env and ``commit_local`` engine; it does NOT carry forward the old baked-floor machinery (recording a baked tree onto ``upstream``), which fought the clone model — a real ``git fetch origin`` plus a -rebase against real ancestry replaces it entirely. +merge against real ancestry replaces it entirely. Diverged histories merge as +one net change instead of replaying every local commit separately, so all +conflicts surface together and local commit identities remain intact. """ from __future__ import annotations @@ -80,20 +82,20 @@ # on-disk clone after an agent edits /data/platform. SERVING_SOURCE_FILE = Path("/tmp/serving-source") SERVING_SHA_FILE = Path("/tmp/serving-sha") -# Persist a conflict so Settings keeps showing it across reloads (the rebase is +# Persist a conflict so Settings keeps showing it across reloads (the merge is # aborted, so no git state alone can signal it). Records the target sha + paths. CONFLICT_FLAG = Path("/data/.platform-conflict") # Persist that the last reconcile could not refresh origin. Deploy verification # treats this as an explicit exemption from the freshness assertion; the next # successful fetch clears it. OFFLINE_FLAG = Path("/data/.platform-offline") -# A text-clean rebase whose result failed the import probe was rolled back to the +# A text-clean merge whose result failed the import probe was rolled back to the # previous served commit. Records the target sha + the import error so Settings # can show "rolled back — needs repair" rather than silently staying "up to # date". ROLLED_BACK_FLAG = Path("/data/.platform-rolled-back") # Transient crash-safety marker written immediately before reconcile mutates the -# served tree. If the boot subprocess is SIGKILLed mid-rebase/probe/rollback, the +# served tree. If the boot subprocess is SIGKILLed mid-merge/probe/rollback, the # post-timeout boot guard uses this sha to restore the last committed served tip # before uvicorn imports anything. RECONCILE_PRE_FLAG = Path("/data/.platform-reconcile-pre") @@ -116,7 +118,7 @@ # this is wedged, not busy. Fetch gets its own (network-bound) budget. _GIT_TIMEOUT = 120 _FETCH_TIMEOUT = 120 -# The post-rebase import probe. A module-level infinite loop or a blocking call +# The post-merge import probe. A module-level infinite loop or a blocking call # in agent-edited code would otherwise wedge boot forever; a timeout-kill counts # as probe-fail -> roll back. _PROBE_TIMEOUT = 60 @@ -195,7 +197,7 @@ class PlatformUpdateState(str, Enum): AVAILABLE = "available" CONFLICT = "conflict" RESTART_NEEDED = "restart_needed" - # A text-clean rebase failed the import probe and was rolled back to the + # A text-clean merge failed the import probe and was rolled back to the # previous served commit; the update needs a repair pass before it can land. ROLLED_BACK = "rolled_back" @@ -210,7 +212,7 @@ class PlatformStatus(TypedDict): recorded_upstream_sha: str | None # Latest fetched origin/main commit that is already contained in local main. # Unlike recorded_upstream_sha, this remains correct after a manual/agent - # rebase that did not run the updater's marker-maintenance path. + # merge that did not run the updater's marker-maintenance path. contained_upstream_sha: str | None seed_required: bool conflict_paths: list[str] @@ -293,9 +295,9 @@ class ReconcileResult: """Outcome of a single :func:`reconcile_clone` pass. ``status`` is one of ``up_to_date`` (origin already integrated), ``updated`` - (fast-forward or rebase applied and the import probe passed), ``conflict`` - (rebase conflicted, aborted, serving the pre sha), ``rolled_back`` (text-clean - rebase failed the import probe, reset to the pre sha), ``offline`` (fetch + (fast-forward or merge applied and the import probe passed), ``conflict`` + (merge conflicted, aborted, serving the pre sha), ``rolled_back`` (text-clean + merge failed the import probe, reset to the pre sha), ``offline`` (fetch failed — kept serving unchanged), ``skipped`` (not a reconcilable clone), or ``error`` (an unexpected git failure was caught and the served tree reset to the pre sha). @@ -312,7 +314,7 @@ class ReconcileResult: error: str | None = None # Exact reviewed release/upstream commit captured while RECONCILE_LOCK is # still held. Hook refresh reads every allowlisted blob from this immutable - # generation rather than trusting replayed local HEAD or a moving ref. + # generation rather than trusting a locally merged HEAD or a moving ref. hook_source_sha: str | None = None @@ -427,7 +429,7 @@ def _git( ) -> subprocess.CompletedProcess: """Run ``git -C repo <args>`` in text mode under the scrubbed, ceiling-pinned env. ``check=False`` lets callers read a non-zero return (a merge-base miss, a - rebase conflict) instead of raising.""" + merge conflict) instead of raising.""" return subprocess.run( ["git", "-C", str(repo), *args], capture_output=True, text=True, timeout=timeout, check=check, @@ -470,7 +472,7 @@ def _head_detached(repo: Path = PLATFORM_REPO) -> bool: def _reattach_detached_head(repo: Path, local: str) -> None: """Move the working branch to the current detached HEAD, preserving the worktree. This makes the subsequent ``commit_local`` land on the branch the - reconcile will actually fast-forward/rebase.""" + reconcile will actually fast-forward/merge.""" if _head_detached(repo): _git("checkout", "-B", local, "HEAD", repo=repo) @@ -498,18 +500,24 @@ def _unmerged_paths(repo: Path = PLATFORM_REPO) -> list[str]: def _rebase_in_progress(repo: Path = PLATFORM_REPO) -> bool: + """Legacy sequencer state left by an updater or resolver from an older build.""" git_dir = repo / ".git" return (git_dir / "rebase-merge").exists() or (git_dir / "rebase-apply").exists() +def _merge_in_progress(repo: Path = PLATFORM_REPO) -> bool: + return bool(_rev(repo, "MERGE_HEAD")) + + +def _reconcile_in_progress(repo: Path = PLATFORM_REPO) -> bool: + return _rebase_in_progress(repo) or _merge_in_progress(repo) + + def _abort_interrupted(repo: Path = PLATFORM_REPO) -> None: - """Crash-safety: abort a rebase/merge left half-finished by a prior crash so - the reconcile starts from a clean, committed ``main`` (the pre-crash tip). A - mid-rebase SIGKILL leaves ``.git/rebase-merge``; a stray merge leaves - ``MERGE_HEAD``. Aborting each restores the branch to its state before the op.""" + """Abort a current merge or a legacy rebase left half-finished by a crash.""" if _rebase_in_progress(repo): _git("rebase", "--abort", repo=repo, check=False) - if (repo / ".git" / "MERGE_HEAD").exists(): + if _merge_in_progress(repo): _git("merge", "--abort", repo=repo, check=False) @@ -542,7 +550,7 @@ def boot_guard_clean_served_tree(repo: Path = PLATFORM_REPO) -> str: return "boot_guard[skipped] no_git" local = _local_branch(repo) pre = _read_reconcile_pre() - interrupted = _rebase_in_progress(repo) or (repo / ".git" / "MERGE_HEAD").exists() + interrupted = _reconcile_in_progress(repo) _abort_interrupted(repo) if pre and _rev(repo, pre): _reset_hard_to(repo, local, pre) @@ -569,8 +577,8 @@ def _fetch(repo: Path = PLATFORM_REPO) -> bool: def _fetch_unshallow(repo: Path = PLATFORM_REPO) -> None: - """Deepen a shallow clone so a rebase can find a real merge base. Best-effort: - an offline/timeout failure leaves the clone shallow and the caller's rebase + """Deepen a shallow clone so a merge can find a real merge base. Best-effort: + an offline/timeout failure leaves the clone shallow and the caller's merge either still succeeds (the base was inside the shallow window) or reports a conflict, which fails closed to serve-old — never a hard reset.""" try: @@ -580,37 +588,34 @@ def _fetch_unshallow(repo: Path = PLATFORM_REPO) -> None: pass -def _rebase_onto(repo: Path, target: str, local: str) -> int: - """Rebase the local commits (``main`` beyond the shared base) onto ``target``. - - ``git rebase target local`` replays the commits in ``local`` that are not in - ``target`` on top of ``target`` — i.e. the agent's local edits onto the new - upstream. The Mobius identity is injected per-invocation (``-c user.*``) so a - replay commit never depends on repo/global git config being set — the rebase - writes new commits and would otherwise fail "committer identity unknown" on a - clone with no configured user. The editor is disabled so a replay never blocks - on an interactive editor, and the whole op is bounded by a timeout. Returns the - git return code (0 clean, non-zero on conflict/error).""" - env = { - **_scrubbed_git_env(repo), - "GIT_EDITOR": "true", - "GIT_SEQUENCE_EDITOR": "true", - } +def _merge_target(repo: Path, target: str) -> int: + """Merge one reviewed upstream target into the checked-out local branch. + + A single merge compares the net local and upstream trees from their shared + base. Unlike a rebase, it neither rewrites every local commit nor makes the + resolver discover conflicts one historical commit at a time. ``--no-ff`` is + deliberate: this helper is called only for diverged histories, and the merge + commit preserves both the reviewed upstream target and the complete local + history as explicit parents. Returns 0 on a clean committed merge and nonzero + on conflict/error; the caller owns abort + serve-old recovery. + """ + env = _scrubbed_git_env(repo) try: proc = subprocess.run( [ "git", "-c", f"user.name={app_git._GIT_NAME}", "-c", f"user.email={app_git._GIT_EMAIL}", - "-C", str(repo), "rebase", target, local, + "-C", str(repo), "merge", "--no-ff", "-m", + f"platform: merge upstream {target[:12]}", target, ], capture_output=True, text=True, timeout=_GIT_TIMEOUT, check=False, env=env, ) return proc.returncode except subprocess.TimeoutExpired: - # A wedged rebase must not leave a half-rebased tree: abort so the caller's + # A wedged merge must not leave a half-merged tree: abort so the caller's # serve-old path is honoured. - _git("rebase", "--abort", repo=repo, check=False) + _git("merge", "--abort", repo=repo, check=False) return 1 @@ -639,7 +644,7 @@ def _clear_upstream(repo: Path) -> None: def _import_probe(repo: Path = PLATFORM_REPO, timeout: int = _PROBE_TIMEOUT): """Run ``import app.main`` as a fresh subprocess with cwd the served backend. - Single-source probe for both boot and post-rebase: it MUST be a subprocess (not + Single-source probe for both boot and post-merge: it MUST be a subprocess (not an in-process import) so the reconcile process — which already imported the OLD ``app.platform_update`` — validates the NEW on-disk tree without corrupting its own interpreter, and so cwd/env exactly mirror the uvicorn exec. The env scrubs @@ -1087,7 +1092,7 @@ def _rebuild_frontend_after_update_if_needed( ) -> None: """Rebuild served frontend assets after a clean update that changed them. - The live edit watcher sees ordinary file saves, but git checkout/rebase during + The live edit watcher sees ordinary file saves, but git checkout/merge during the Settings update flow can move frontend files without a reliable watcher event. Without this explicit rebuild, ``/data/platform/frontend/src`` advances while ``dist`` keeps serving the old bundle. @@ -1163,7 +1168,7 @@ def reconcile_clone( return ReconcileResult("skipped", None, None, None, error="no_git") local = _local_branch(repo) - # Crash-safety FIRST: a mid-rebase crash must be aborted before anything reads + # Crash-safety FIRST: a mid-reconcile crash must be aborted before anything reads # the tree, so we reconcile from the committed pre-crash tip. _abort_interrupted(repo) pre = _rev(repo, local) @@ -1207,7 +1212,7 @@ def reconcile_clone( if progress: progress(PlatformUpdatePhase.RECONCILING) # A deploy advanced origin beyond committed main. Commit any uncommitted edits - # FIRST so neither the fast-forward reset nor the rebase can discard them. + # FIRST so neither the fast-forward reset nor the merge can discard them. _reattach_detached_head(repo, local) app_git.commit_local(repo, "platform: local edits before reconcile") pre = _rev(repo, local) # now includes the just-committed edits @@ -1224,7 +1229,7 @@ def reconcile_clone( # behind. The normal fetch has already transferred the new first-parent # chain, so Git can prove the overwhelmingly common fast-forward directly. # Only a shallow clone whose ancestry is still ambiguous needs the expensive - # full-history fallback before we choose between reset and rebase. + # full-history fallback before we choose between reset and merge. fast_forward = bool(pre) and _is_ancestor(repo, pre, target) if _is_shallow(repo) and not fast_forward: if progress: @@ -1241,22 +1246,23 @@ def reconcile_clone( # discard committed local edits. _git("reset", "--hard", target, repo=repo) else: - # main has commits not in target (diverged): REBASE local edits onto the new - # upstream so BOTH survive. - rc = _rebase_onto(repo, target, local) + # Main and target diverged: merge the reviewed upstream tree ONCE. This + # preserves local commit identities and makes one resolver pass see every + # net conflict instead of stopping once per historical local commit. + rc = _merge_target(repo, target) if rc != 0: - # Conflict: NEVER leave a half-rebased tree. Abort back to PRE (the old, + # Conflict: NEVER leave a half-merged tree. Abort back to PRE (the old, # working code keeps serving), record the conflict, clear any stale # rollback flag, and let the caller open a resolver chat. paths = _unmerged_paths(repo) - _git("rebase", "--abort", repo=repo, check=False) + _git("merge", "--abort", repo=repo, check=False) _reset_hard_to(repo, local, pre) # belt-and-braces: ensure main == PRE _write_conflict_flag(target, paths) ROLLED_BACK_FLAG.unlink(missing_ok=True) _clear_reconcile_pre() return ReconcileResult("conflict", pre, pre, target, conflict_paths=paths) - # Post-reconcile import probe: a text-clean ff/rebase can still produce a + # Post-reconcile import probe: a text-clean ff/merge can still produce a # tree that fails to import (upstream dropped a module a local edit imports; # a bad deploy). Roll back to the previous served commit rather than serve it # broken. Skip the ~60s throwaway boot when the reconcile touched NO served @@ -1280,7 +1286,7 @@ def reconcile_clone( _clear_reconcile_pre() return ReconcileResult("error", pre, pre, target, error=repr(exc)) - # Success: main now carries the update plus any replayed local edits. Advance + # Success: main now carries the update plus all local edits. Advance # the upstream marker and clear conflict/rollback flags. At boot the fresh # uvicorn imports this directly (clear the restart flag — the boot IS the # restart the flag would ask for); an owner Apply marks a restart via the @@ -1330,7 +1336,7 @@ def _reconcile_under_lock( at_boot=at_boot, # A reviewed Apply already proved the immutable object exists. Fetching a # moving remote again adds latency and was the original TOCTOU bug; boot - # keeps the normal refresh path. A shallow rebase may still deepen below. + # keeps the normal refresh path. A shallow merge may still deepen below. fetch_remote=plan_id is None, progress=progress, ) @@ -1354,7 +1360,7 @@ def _reconcile_under_lock( ) # `upstream` is moved only by a successful/contained reconcile to the # fetched release target. Capture its immutable oid before releasing the - # cross-process lock; local replay commits on main are intentionally not a + # cross-process lock; local commits on main are intentionally not a # hook trust transition. return replace( result, @@ -1416,7 +1422,7 @@ def platform_status(repo: Path = PLATFORM_REPO) -> PlatformStatus: """ image_sha = current_build_sha() upstream_sha = recorded_upstream_sha(repo) - conflict = CONFLICT_FLAG.exists() or _rebase_in_progress(repo) + conflict = CONFLICT_FLAG.exists() or _reconcile_in_progress(repo) rolled_back = ROLLED_BACK_FLAG.exists() restart_needed = RESTART_NEEDED_FLAG.exists() or _platform_tree_needs_restart(repo) local = _local_branch(repo) @@ -1777,7 +1783,7 @@ async def create_platform_conflict_resolver_chat( from app import models flag = _read_conflict_flag() or {} - if not (CONFLICT_FLAG.exists() or _rebase_in_progress(repo)): + if not (CONFLICT_FLAG.exists() or _reconcile_in_progress(repo)): raise PlatformUpdateError("No unresolved platform update conflict.") existing_chat_id = flag.get("chat_id") @@ -1795,24 +1801,55 @@ async def create_platform_conflict_resolver_chat( ) conflict_paths = flag.get("paths") or _unmerged_paths(repo) - result = await spawn_platform_conflict_chat(db, conflict_paths) + target_sha = flag.get("upstream") or _rev(repo, DEFAULT_TARGET_REF) + if not target_sha: + raise PlatformUpdateError("Platform conflict target is unavailable.") + result = await spawn_platform_conflict_chat(db, conflict_paths, target_sha) if result is None: raise PlatformUpdateError("Could not open resolver chat.") _write_conflict_flag( - flag.get("upstream") or _rev(repo, DEFAULT_TARGET_REF), + target_sha, conflict_paths, result["chat_id"], ) return result +def _platform_conflict_resolver_message( + target_sha: str, + conflict_paths: list[str], +) -> str: + """Instructions bound to the exact release the owner reviewed and applied.""" + files = ", ".join(conflict_paths) if conflict_paths else "some files" + return ( + "A platform update is ready but conflicts with local edits — the new " + "version and the local changes both touched the same lines, so they can't " + "merge cleanly.\n\n" + "The clone at `/data/platform` is a real git checkout of the platform repo. " + f"The exact reviewed version is commit `{target_sha}`; local edits are on " + "the checked-out working branch. " + f"Reconcile these conflicting files by hand: {files}.\n\n" + "Resolve it with ordinary git: `git -C /data/platform merge --no-ff " + f"{target_sha}` compares the complete local and reviewed upstream trees " + "once and stops with every conflicting file marked; combine the intent of " + "the local version and upstream's, save each file, then `git add` it and " + "`git merge --continue`. When the merge finishes, the working branch " + "carries both histories.\n\n" + "When the reconcile is committed, clear the flag " + "(`rm -f /data/.platform-conflict`) and tell the owner to **restart the " + "server** from Settings to finish. To back out instead, `git -C " + "/data/platform merge --abort`, `rm -f /data/.platform-conflict`, and tell " + "the owner the update was skipped." + ) + + async def spawn_platform_conflict_chat( - db: Session, conflict_paths: list[str], + db: Session, conflict_paths: list[str], target_sha: str, ) -> PlatformConflictResolverChatOut | None: """Open a visible agent chat to reconcile the new platform version into - ``main`` — the platform analogue of a per-app update-conflict resolver chat. - Dedupes on a running resolver.""" + the checked-out working branch — the platform analogue of a per-app + update-conflict resolver chat. Dedupes on a running resolver.""" import time import uuid @@ -1845,25 +1882,7 @@ async def spawn_platform_conflict_chat( get_settings().data_dir, owner.provider, ) - files = ", ".join(conflict_paths) if conflict_paths else "some files" - content = ( - "A platform update is ready but conflicts with local edits — the new " - "version and the local changes both touched the same lines, so it can't " - "rebase cleanly.\n\n" - "The clone at `/data/platform` is a real git checkout of the platform repo. " - "The new version is on the fetched `origin/main`; local edits are on `main`. " - f"Reconcile these conflicting files by hand: {files}.\n\n" - "Resolve it with ordinary git: `git -C /data/platform rebase origin/main` " - "replays the local edits onto the new version and stops on the conflicting " - "files with conflict markers; for each, combine the intent of the local " - "version and origin's, save it, then `git add` it and `git rebase " - "--continue`. When the rebase finishes, `main` carries both.\n\n" - "When the reconcile is committed, clear the flag " - "(`rm -f /data/.platform-conflict`) and tell the owner to **restart the " - "server** from Settings to finish. To back out instead, `git -C " - "/data/platform rebase --abort`, `rm -f /data/.platform-conflict`, and tell " - "the owner the update was skipped." - ) + content = _platform_conflict_resolver_message(target_sha, conflict_paths) chat_id = str(uuid.uuid4()) chat = models.Chat( diff --git a/backend/scripts/entrypoint.sh b/backend/scripts/entrypoint.sh index 2d246f2fc..8526bf976 100755 --- a/backend/scripts/entrypoint.sh +++ b/backend/scripts/entrypoint.sh @@ -85,7 +85,7 @@ if [ "$_boot_counter" -ge 3 ] && [ -f /data/.last-successful-boot ]; then # TIMESTAMPED /data/platform.crashloop-prev.<ts> for inspection/recovery, not # deleted. A one-slot .crashloop-prev would let a SECOND crash-loop delete the # first preserved tree before the owner could inspect it, so we timestamp each - # quarantine and keep only the newest few. (slice B's deploy=rebase + # quarantine and keep only the newest few. (slice B's deploy=merge # reconciliation will refine this.) _cl_ts=$(date -u +%Y%m%dT%H%M%SZ) if [ -e /data/platform ] && [ -n "$(ls -A /data/platform 2>/dev/null)" ] && @@ -736,9 +736,9 @@ printf '%s\n' "$_served_sha" > /tmp/serving-sha chmod 644 /tmp/serving-source /tmp/serving-sha 2>/dev/null || true if [ "$_use_platform" -eq 1 ] && [ "${MOBIUS_TEST_RUNTIME:-0}" != "1" ]; then - # Slice B deploy=rebase reconcile. A deploy ships a new image AND advances - # canonical origin/main; fetch origin and replay the local edits onto the new - # version NOW, before uvicorn imports the code, so the update goes live this + # Slice B deploy=merge reconcile. A deploy ships a new image AND advances + # canonical origin/main; fetch origin and merge the new version once with the + # local edits NOW, before uvicorn imports the code, so the update goes live this # boot with no restart. Runs as mobius (writes /data; root would poison /data # ownership + hit git "dubious ownership"), cwd the served backend so `app` # imports resolve from the clone, under the IDENTICAL GIT_*/PYTHONPATH scrub @@ -746,12 +746,12 @@ if [ "$_use_platform" -eq 1 ] && [ "${MOBIUS_TEST_RUNTIME:-0}" != "1" ]; then # `|| true` guards the shell, so a reconcile failure never bricks boot; a # conflict/rollback leaves the pre-reconcile code on disk (aborted/reset) and # sets a flag Settings surfaces. The outer `timeout` is a last-resort bound set - # ABOVE the reconcile's bounded operations: fetch 120 + unshallow 120 + rebase + # ABOVE the reconcile's bounded operations: fetch 120 + unshallow 120 + merge # 120 + probe 60 = 420, plus commit_local's own bounded git calls. Keep this # comfortably higher so internal timeouts fire FIRST; the post-timeout guard # below still cleans the tree if the outer kill ever wins. recoveryd remains # the outer floor. - echo "Platform layer: reconciling /data/platform with origin (slice B deploy=rebase)..." >&2 + echo "Platform layer: reconciling /data/platform with origin (slice B deploy=merge)..." >&2 su -s /bin/sh mobius -c \ "cd /data/platform/backend && $_env_scrub timeout 900 python3 -c \ 'from app import platform_update; print(platform_update.reconcile_clone_sync())'" \ @@ -767,7 +767,7 @@ if [ "$_use_platform" -eq 1 ] && [ "${MOBIUS_TEST_RUNTIME:-0}" != "1" ]; then echo "Platform layer: boot guard failed; refusing to serve the platform tree." >&2 exit 1 fi - # A fast-forward / rebase advanced main, so the served sha the /api/version and + # A fast-forward / merge advanced main, so the served sha the /api/version and # /api/debug/serving routes report (written to /tmp/serving-sha above) must # reflect the reconciled HEAD, not the pre-reconcile clone tip. _served_sha=$(su -s /bin/sh mobius -c \ diff --git a/backend/scripts/seed-skills/theming.md b/backend/scripts/seed-skills/theming.md index 8ece1b402..f7a5d79c4 100644 --- a/backend/scripts/seed-skills/theming.md +++ b/backend/scripts/seed-skills/theming.md @@ -89,7 +89,8 @@ git diff -- frontend/src frontend/public | head -80 touch /data/platform/frontend/src/path/to/changed-file.jsx ``` -Pick up the changes through the platform-apply flow (rebase onto `origin/main`) rather than hand-copying files — see `contributing.md`. +Pick up the changes through the platform-apply flow (merge the reviewed +`origin/main` target) rather than hand-copying files — see `contributing.md`. --- diff --git a/backend/tests/test_platform_update.py b/backend/tests/test_platform_update.py index 8d6073cf9..f52cc30b2 100644 --- a/backend/tests/test_platform_update.py +++ b/backend/tests/test_platform_update.py @@ -1,20 +1,21 @@ """Clone-native platform reconcile — the git plumbing that fetches origin and -rebases the local edits onto the new version without ever losing them or serving -a broken tree. +merges it once with local edits without ever losing them or serving a broken +tree. These drive ``platform_update.reconcile_clone`` against throwaway repos in ``tmp_path``: a bare ``origin`` repo, a ``platform`` clone of it (mirroring the entrypoint bootstrap: local ``main`` + an ``upstream`` marker branch at HEAD), and the module's ``/data`` flag paths monkeypatched into ``tmp_path`` so no real platform tree is touched. Each platform tree carries a trivially-importable -``backend/app`` package so the post-rebase import probe (a real ``import +``backend/app`` package so the post-merge import probe (a real ``import app.main`` subprocess) exercises for real. -The load-bearing cases: a clean fast-forward advances the served tree; a disjoint -local edit is preserved by a rebase; a same-line conflict aborts and serves the -OLD code; a text-clean rebase whose result fails to import rolls back to the old -code; an offline fetch keeps serving unchanged; and a crash-interrupted rebase is -aborted on the next pass. +The load-bearing cases: a clean fast-forward advances the served tree; a +disjoint local edit is preserved with its original commit identity; all net +conflicts surface in one merge; a text-clean merge whose result fails to import +rolls back to the old code; an offline fetch keeps serving unchanged; and a +crash-interrupted merge is aborted on the next pass. A legacy interrupted rebase +is still cleaned up because an update can cross this implementation boundary. """ import subprocess @@ -38,7 +39,7 @@ def _git(cwd: Path, *args: str, check: bool = True) -> subprocess.CompletedProce # A trivially-importable backend so the import probe (`import app.main` with cwd # repo/backend) runs for real. `main.py` imports the sibling `foo` module so a -# test can delete `foo` upstream to make a text-clean rebase import-broken. +# test can delete `foo` upstream to make a text-clean merge import-broken. _MAIN_PY = "import app.foo\n\nVALUE = app.foo.VALUE\nLINE_A = 1\nLINE_B = 2\nLINE_C = 3\n" _FOO_PY = "VALUE = 'foo'\n" @@ -182,13 +183,13 @@ def fail_unshallow(_repo): assert _served_sha(platform) == new -# --- V-B2: disjoint local edit preserved via rebase ------------------------- +# --- V-B2: disjoint local edit preserved via one merge ---------------------- def test_local_edit_preserved_across_update(clone_env): origin, platform = clone_env - _local_commit(platform, edits={"backend/app/main.py": + local = _local_commit(platform, edits={"backend/app/main.py": _MAIN_PY.replace("LINE_A = 1", "LINE_A = 111")}) - _advance_origin(origin, edits={"backend/app/main.py": + target = _advance_origin(origin, edits={"backend/app/main.py": _MAIN_PY.replace("LINE_C = 3", "LINE_C = 333")}) res = pu.reconcile_clone(platform, at_boot=True) @@ -197,11 +198,23 @@ def test_local_edit_preserved_across_update(clone_env): served = (platform / "backend/app/main.py").read_text() assert "LINE_A = 111" in served # local edit assert "LINE_C = 333" in served # upstream edit + # A single merge preserves the original local commit instead of rewriting it + # through a per-commit rebase. Both complete histories are explicit parents. + parents = _git( + platform, "show", "-s", "--format=%P", res.new_sha, + ).stdout.split() + assert parents == [local, target] + assert _git( + platform, "merge-base", "--is-ancestor", local, res.new_sha, + ).returncode == 0 + assert _git( + platform, "merge-base", "--is-ancestor", target, res.new_sha, + ).returncode == 0 assert not pu.CONFLICT_FLAG.exists() # --- regression: a drifted upstream marker never triggers a data-losing -# fast-forward. The ff-vs-rebase choice is decided by ANCESTRY, not the upstream +# fast-forward. The ff-vs-merge choice is decided by ANCESTRY, not the upstream # marker, so committed local edits survive even when the marker is set to the # exact value that would have made the old marker-gated `reset --hard target` # discard them. This is the headline data-safety invariant of the fix. --------- @@ -221,7 +234,7 @@ def test_drifted_upstream_marker_never_discards_local_commits(clone_env): res = pu.reconcile_clone(platform, at_boot=True) - # local is NOT an ancestor of target, so ancestry forces a REBASE (not a reset) + # local is NOT an ancestor of target, so ancestry forces a MERGE (not a reset) # and BOTH survive — the local commit is not discarded despite the bad marker. assert res.status == "updated" served = (platform / "backend/app/main.py").read_text() @@ -244,10 +257,10 @@ def test_conflict_serves_old_and_flags(clone_env): res = pu.reconcile_clone(platform, at_boot=True) assert res.status == "conflict" - # Served tree is the pre-reconcile local code, intact; no half-rebase left. + # Served tree is the pre-reconcile local code, intact; no half-merge left. assert _served_sha(platform) == pre assert "LINE_A = 'LOCAL'" in (platform / "backend/app/main.py").read_text() - assert not pu._rebase_in_progress(platform) + assert not pu._reconcile_in_progress(platform) assert pu.CONFLICT_FLAG.exists() assert any("main.py" in p for p in res.conflict_paths) status = pu.platform_status(platform) @@ -255,11 +268,34 @@ def test_conflict_serves_old_and_flags(clone_env): assert any("main.py" in p for p in status["conflict_paths"]) -# --- V-B4: import-broken text-clean rebase -> rollback ---------------------- +def test_diverged_update_surfaces_all_net_conflicts_together(clone_env): + origin, platform = clone_env + _local_commit(platform, edits={"backend/app/main.py": + _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'LOCAL'")}) + _local_commit(platform, edits={"backend/app/foo.py": "VALUE = 'LOCAL'\n"}) + _advance_origin(origin, edits={ + "backend/app/main.py": _MAIN_PY.replace( + "LINE_A = 1", "LINE_A = 'UPSTREAM'", + ), + "backend/app/foo.py": "VALUE = 'UPSTREAM'\n", + }) + + res = pu.reconcile_clone(platform, at_boot=True) + + # A per-commit rebase stopped on main.py, then made the resolver discover the + # foo.py conflict only after continuing. The net merge reports both at once. + assert res.status == "conflict" + assert set(res.conflict_paths) == { + "backend/app/main.py", "backend/app/foo.py", + } + assert not pu._reconcile_in_progress(platform) + + +# --- V-B4: import-broken text-clean merge -> rollback ----------------------- -def test_import_broken_rebase_rolls_back(clone_env): +def test_import_broken_merge_rolls_back(clone_env): origin, platform = clone_env - # A disjoint local edit (so the rebase is text-clean), while upstream DELETES + # A disjoint local edit (so the merge is text-clean), while upstream DELETES # foo.py — which main.py still imports. Textually clean, import-broken. pre = _local_commit(platform, edits={"backend/app/main.py": _MAIN_PY + "LOCAL = 'kept'\n"}) @@ -335,29 +371,29 @@ def test_detached_head_uncommitted_edit_survives_reconcile(clone_env): assert "LINE_C = 1001" in served -# --- crash-safety: a stale in-progress rebase is aborted -------------------- +# --- crash-safety: interrupted merge + legacy rebase are aborted ------------ -def test_stale_rebase_aborted_on_next_pass(clone_env): +def test_stale_merge_aborted_on_next_pass(clone_env): origin, platform = clone_env - # Force a real conflict and leave the rebase in progress (no abort), mirroring - # a crash mid-rebase. + # Force a real conflict and leave the merge in progress (no abort), mirroring + # a crash mid-merge. pre = _local_commit(platform, edits={"backend/app/main.py": _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'LOCAL'")}) new = _advance_origin(origin, edits={"backend/app/main.py": _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'UPSTREAM'")}) _git(platform, "fetch", "-q", "origin") - rc = _git(platform, "rebase", new, "main", check=False).returncode - assert rc != 0 and pu._rebase_in_progress(platform) # left mid-rebase + rc = _git(platform, "merge", "--no-ff", new, check=False).returncode + assert rc != 0 and pu._merge_in_progress(platform) # left mid-merge - # Next reconcile must abort the stale rebase FIRST, then reconcile cleanly + # Next reconcile must abort the stale merge FIRST, then reconcile cleanly # (here: re-conflict and serve old — the point is it does not wedge or corrupt). res = pu.reconcile_clone(platform, at_boot=True) assert res.status == "conflict" assert _served_sha(platform) == pre - assert not pu._rebase_in_progress(platform) + assert not pu._reconcile_in_progress(platform) -def test_boot_guard_aborts_interrupted_rebase_before_serving(clone_env): +def test_boot_guard_aborts_interrupted_merge_before_serving(clone_env): origin, platform = clone_env pre = _local_commit(platform, edits={"backend/app/main.py": _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'LOCAL'")}) @@ -365,21 +401,38 @@ def test_boot_guard_aborts_interrupted_rebase_before_serving(clone_env): _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'UPSTREAM'")}) _git(platform, "fetch", "-q", "origin") pu._write_reconcile_pre(pre) - rc = _git(platform, "rebase", new, "main", check=False).returncode - assert rc != 0 and pu._rebase_in_progress(platform) + rc = _git(platform, "merge", "--no-ff", new, check=False).returncode + assert rc != 0 and pu._merge_in_progress(platform) assert "<<<<<<<" in (platform / "backend/app/main.py").read_text() summary = pu.boot_guard_clean_served_tree(platform) assert summary.startswith("boot_guard[reset]") assert _served_sha(platform) == pre - assert not pu._rebase_in_progress(platform) + assert not pu._reconcile_in_progress(platform) assert "<<<<<<<" not in (platform / "backend/app/main.py").read_text() ok, err = pu._import_probe(platform) assert ok, err assert not pu.RECONCILE_PRE_FLAG.exists() +def test_legacy_stale_rebase_aborted_on_next_pass(clone_env): + origin, platform = clone_env + pre = _local_commit(platform, edits={"backend/app/main.py": + _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'LOCAL'")}) + new = _advance_origin(origin, edits={"backend/app/main.py": + _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'UPSTREAM'")}) + _git(platform, "fetch", "-q", "origin") + rc = _git(platform, "rebase", new, "main", check=False).returncode + assert rc != 0 and pu._rebase_in_progress(platform) + + res = pu.reconcile_clone(platform, at_boot=True) + + assert res.status == "conflict" + assert _served_sha(platform) == pre + assert not pu._reconcile_in_progress(platform) + + def test_boot_guard_sync_propagates_failure(monkeypatch): """The final boot gate must fail closed; callers need a non-zero process, not an error-looking success string that the shell can accidentally ignore.""" @@ -855,8 +908,8 @@ async def test_platform_conflict_resolver_chat_is_click_gated( pu._write_conflict_flag(target, ["backend/app/main.py"]) calls = [] - async def fake_spawn(db, paths): - calls.append((db, paths)) + async def fake_spawn(db, paths, target_sha): + calls.append((db, paths, target_sha)) return { "chat_id": "resolver-chat", "created": True, @@ -873,13 +926,26 @@ async def fake_spawn(db, paths): "created": True, "started": True, } - assert calls == [(db, ["backend/app/main.py"])] + assert calls == [(db, ["backend/app/main.py"], target)] flag = pu._read_conflict_flag() assert flag["upstream"] == target assert flag["paths"] == ["backend/app/main.py"] assert flag["chat_id"] == "resolver-chat" +def test_platform_conflict_resolver_message_pins_reviewed_target(): + target = "a" * 40 + + content = pu._platform_conflict_resolver_message( + target, + ["backend/app/main.py", "frontend/src/App.jsx"], + ) + + assert f"merge --no-ff {target}" in content + assert "merge --no-ff origin/main" not in content + assert "backend/app/main.py, frontend/src/App.jsx" in content + + def test_status_restart_needed_when_disk_head_changed_after_boot(clone_env): origin, platform = clone_env served = _served_sha(platform) From 93807d7bcc481a96e2fbb6094a91e98aab1d2787 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:55:17 +0000 Subject: [PATCH 07/15] Make Memory recall outcomes provider-neutral MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/chat.py | 37 ++-- backend/app/chat_transcript.py | 24 ++- backend/app/claude_sdk_runner.py | 1 + backend/app/codex_sdk_runner.py | 10 +- backend/app/events.py | 22 +- backend/app/memory_recall.py | 197 ++++++++---------- backend/tests/test_claude_sdk_runner.py | 5 +- backend/tests/test_codex_sdk_runner.py | 18 +- backend/tests/test_memory_recall.py | 177 ++++++++++++---- backend/tests/test_tool_output_excerpt.py | 7 + .../components/ChatView/MessageSources.jsx | 24 +-- .../ChatView/__tests__/groupBlocks.test.js | 10 + .../ChatView/__tests__/memoryRecall.test.js | 6 +- .../ChatView/__tests__/streamReducers.test.js | 21 ++ .../src/components/ChatView/memoryRecall.js | 3 + .../src/components/ChatView/streamReducers.js | 19 +- .../components/ChatView/toolActivityLabel.js | 3 +- .../ChatView/useStreamConnection.js | 19 +- 18 files changed, 383 insertions(+), 220 deletions(-) diff --git a/backend/app/chat.py b/backend/app/chat.py index 4af9f2973..cec2b3c65 100644 --- a/backend/app/chat.py +++ b/backend/app/chat.py @@ -76,9 +76,10 @@ excerpt_tool_output, finalize_blocks, process_event, + tool_output_exit_code, undo_question_scrub, ) -from app.memory_recall import recall_from_command, recall_from_output +from app.memory_recall import recall_from_command, recall_from_result from app.providers import effective_agent_settings, get_provider, get_skill_path from app.runner_registry import registry from app.runtime_types import ChatEvent @@ -414,19 +415,23 @@ def _tool_was_memory_recall(self, tool_use_id) -> bool: def _stamp_memory_recall(self, event: ChatEvent) -> None: """Name a Memory-app recall on the event, in two lifecycle phases. - At input time the command identifies the lookup, so the live turn can say - it is remembering while the search still runs. At output time that same - tool's stdout is parsed into the notes it actually returned — including the - honest "found nothing" outcome, which is otherwise indistinguishable from - never having looked. + The documented simple command identifies the lookup, so the live turn can + say it is remembering while the search runs. Only the final output event + settles it from the Memory app's structured result; streaming deltas cannot + prematurely claim success, emptiness, or failure. """ - if event.get("type") == "tool_input": + if event.get("type") in ("tool_start", "tool_input"): + if event.get("type") == "tool_start" and event.get("tool") != "Bash": + return recall = recall_from_command(event.get("input")) if recall is not None: event["recall"] = recall return - if self._tool_was_memory_recall(event.get("tool_use_id")): - event["recall"] = recall_from_output(event.get("content")) + if (event.get("output_complete") + and self._tool_was_memory_recall(event.get("tool_use_id"))): + event["recall"] = recall_from_result( + event.get("content"), event.get("output_exit_code"), + ) def _reduce_tool_output(self, event: ChatEvent) -> None: """Move a large tool_output's full text OFF the wire (contract rule 6). @@ -545,12 +550,14 @@ def publish(self, event: ChatEvent) -> bool: # and before the broadcast below, so the rewritten event is the single # source feeding the persisted block, the live wire, and the catch-up log. # - # Memory recall is stamped just BEFORE that reduction, for the same reason - # and on the same funnel: a full lookup's stdout exceeds the inline - # threshold, so parsing after rule-6 carving would silently lose the note - # titles that make a citation readable. One stamp here reaches the persisted - # block, the live wire, and the catch-up log alike, for both runners. - if event_type in ("tool_input", "tool_output"): + # Memory recall is stamped just BEFORE that reduction on the same funnel. + # The app prints its bounded structured result last, so one validated stamp + # reaches the persisted block, live wire, and catch-up log for both runners. + if event_type == "tool_output" and event.get("output_exit_code") is None: + exit_code = tool_output_exit_code(event.get("content")) + if exit_code is not None: + event["output_exit_code"] = exit_code + if event_type in ("tool_start", "tool_input", "tool_output"): self._stamp_memory_recall(event) if event_type == "tool_output": self._reduce_tool_output(event) diff --git a/backend/app/chat_transcript.py b/backend/app/chat_transcript.py index b2f953840..a44432131 100644 --- a/backend/app/chat_transcript.py +++ b/backend/app/chat_transcript.py @@ -4,7 +4,13 @@ import re -from app.memory_recall import RECALL_HIT, merge_recall_notes +from app.memory_recall import ( + RECALL_EMPTY, + RECALL_FAILED, + RECALL_HIT, + RECALL_SEARCHING, + merge_recall_notes, +) _QUESTION_TOOLS = {"AskUserQuestion", "request_user_input"} @@ -263,14 +269,22 @@ def _compact_activity_run( recall_notes: list[dict] = [] seen_recall_paths: set[str] = set() recall_status = "" + recall_rank = { + RECALL_SEARCHING: 0, + RECALL_FAILED: 1, + RECALL_EMPTY: 2, + RECALL_HIT: 3, + } for _, block in blocks: recall = block.get("recall") if not isinstance(recall, dict): continue - # Any lookup that returned something outranks one that came back empty: - # the turn did remember, even if another probe found nothing. - if recall_status != RECALL_HIT: - recall_status = recall.get("status") or recall_status + # A real hit outranks an empty search, which outranks a failed probe. This + # preserves useful evidence without letting one failure erase a successful + # result elsewhere in the same folded run. + status = recall.get("status") + if recall_rank.get(status, -1) > recall_rank.get(recall_status, -1): + recall_status = status merge_recall_notes(recall_notes, seen_recall_paths, recall) start = blocks[0][0] diff --git a/backend/app/claude_sdk_runner.py b/backend/app/claude_sdk_runner.py index 1e61436b5..792a50446 100644 --- a/backend/app/claude_sdk_runner.py +++ b/backend/app/claude_sdk_runner.py @@ -1022,6 +1022,7 @@ def dispatch_sdk_message( "type": "tool_output", "content": output, "tool_use_id": block.tool_use_id, + "output_complete": True, }) if output.startswith("Web search results for query"): sources = sources_from_websearch_text(output) diff --git a/backend/app/codex_sdk_runner.py b/backend/app/codex_sdk_runner.py index 9bd90b298..75f3b6a6e 100644 --- a/backend/app/codex_sdk_runner.py +++ b/backend/app/codex_sdk_runner.py @@ -1193,9 +1193,13 @@ def _tool_completed_events(item: Any, sdk: dict[str, Any]) -> list[dict[str, Any if isinstance(item, sdk["CommandExecutionThreadItem"]): output = (item.aggregated_output or "").strip() - events: list[dict[str, Any]] = [] - if output: - events.append({"type": "tool_output", "content": output}) + exit_code = getattr(item, "exit_code", None) + events: list[dict[str, Any]] = [{ + "type": "tool_output", + "content": output, + "output_complete": True, + **({"output_exit_code": exit_code} if isinstance(exit_code, int) else {}), + }] events.append({"type": "tool_end"}) return events diff --git a/backend/app/events.py b/backend/app/events.py index ab69bdf04..c789b45cc 100644 --- a/backend/app/events.py +++ b/backend/app/events.py @@ -334,6 +334,20 @@ def _tool_output_exit_code(content: str, parsed): return int(m.group(1)) if m else None +def tool_output_exit_code(content: object): + """Return a typed command exit code from any sized tool output.""" + if not isinstance(content, str): + return None + parsed = None + stripped = content.lstrip() + if stripped[:1] in ("{", "["): + try: + parsed = json.loads(content) + except (ValueError, TypeError): + parsed = None + return _tool_output_exit_code(content, parsed) + + def excerpt_tool_output(content: str): """Reduce a large tool_output string to (excerpt, full_len, exit_code). @@ -574,6 +588,8 @@ def process_event(event: dict, assistant_blocks: list) -> bool: tool_use_id = event.get("tool_use_id") if tool_use_id: block["tool_use_id"] = tool_use_id + if isinstance(event.get("recall"), dict): + block["recall"] = event["recall"] assistant_blocks.append(block) return True @@ -624,12 +640,12 @@ def _apply_tool_input(blk: dict) -> None: # the full text and read a failure exit code from a field, not a parse # of the possibly-carved excerpt. Absent fields leave the block shape # unchanged (a small, un-reduced output). + exit_code = event.get("output_exit_code") + if exit_code is not None: + blk["output_exit_code"] = exit_code if event.get("output_truncated"): blk["output_truncated"] = True blk["output_full_len"] = event.get("output_full_len") - exit_code = event.get("output_exit_code") - if exit_code is not None: - blk["output_exit_code"] = exit_code # Settle a Memory lookup from "searching" to what it actually recalled. # The sink parsed this from the FULL output, before the carving above. if isinstance(event.get("recall"), dict): diff --git a/backend/app/memory_recall.py b/backend/app/memory_recall.py index f00ab9a70..c55d16e02 100644 --- a/backend/app/memory_recall.py +++ b/backend/app/memory_recall.py @@ -7,48 +7,29 @@ "it ran housekeeping" — nor, more importantly, "it looked and found nothing" from "it never looked". -Detection is deliberately two-phase and keyed off the tool's own lifecycle -rather than the shape of its output: - -* ``recall_from_command`` matches the *command being run*. This is the only - positive identification; nothing here ever concludes "this was a memory - lookup" from output text alone, so an unrelated command that happens to - print ``FILES:`` can never mint a citation. -* ``recall_from_output`` is then trusted to parse that command's stdout, and is - only ever called for a tool already identified by the first phase. - -The stdout contract belongs to the Memory app (``memory_search.py``'s -``retrieve``/``run``) and is stable:: - - Relevant memories: - - <title>: <excerpt> [<relative/path.md>] - - <title>: <excerpt> [<relative/path.md>] - FILES: notes/a.md, notes/b.md - -or, for a lookup that matched nothing:: - - No relevant memories. - -``FILES:`` is authoritative: those paths were opened by confined Python after -the graph commit was pinned, whereas the ``- title: excerpt [path]`` lines are -presentation. So paths come from ``FILES:`` and the section lines only enrich -them with a title and excerpt. That split also makes the parse robust to the -head+tail carving a large tool output receives (``events.py`` -``excerpt_tool_output``): both markers sit at the very start and very end of -the stream, so a carved middle costs some titles but never the citation set. - -Every failure mode degrades to *less* metadata, never to a wrong citation: -an unparseable body still yields a bounded "hit" with path-derived titles, and -a missing command summary yields no recall at all. +Detection is deliberately two-phase and keyed off the tool's own lifecycle: + +* ``recall_from_command`` accepts only the simple absolute invocation documented + by the Memory skill. It deliberately rejects shell composition rather than + trying to partially parse Bash. +* ``recall_from_result`` reads the Memory app's bounded structured result line + and is only called for a tool already identified by the first phase. + +The structured line is printed last, so head+tail carving preserves it. Human +prose and the legacy ``FILES:`` line remain useful to the agent, but neither is +parsed for product state. Missing, malformed, or contradictory result metadata +is an explicit failed lookup, never a successful note-less recall. """ from __future__ import annotations +import json import re +import shlex # Recall metadata rides inline on the SSE event, the persisted tool block, and # the compacted activity summary — the same budget the web-source citations -# live within. memory_search.py itself returns at most 6 files with 900-char +# live within. memory_search.py itself returns at most 4 files with 900-char # excerpts; these ceilings leave headroom without letting a malformed or # hostile stdout inflate every transcript read. MAX_RECALL_NOTES = 12 @@ -61,28 +42,24 @@ RECALL_SEARCHING = "searching" RECALL_HIT = "hit" RECALL_EMPTY = "empty" +RECALL_FAILED = "failed" -# The command summary the backend builds for Bash is the verbatim command -# string, so identification means answering "did this command RUN the search -# script?" — not "does this command mention it?". Substring matching gets that -# wrong in the most ordinary way possible: `grep -rn memory_search.py …` and -# `cat memory_search.py` both name the script while doing something else -# entirely, and either would mint a citation from unrelated output. -# -# So the command is split into segments and each segment's HEAD is inspected: -# a lookup is either the script executed directly or an interpreter invoked on -# it. Anything where the script is a mere argument is correctly ignored. +# The command summary is the verbatim Bash command. Accept only Memory's +# documented simple absolute invocation. This is intentionally a narrow +# protocol, not a growing shell grammar: composition, redirection, substitution, +# and relative scripts all yield no observability marker while the command +# itself continues to run normally. _MAX_COMMAND_SCAN_CHARS = 8192 -_SEGMENT_RE = re.compile(r"[;&|\n]+") _ENV_ASSIGN_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") _INTERPRETER_RE = re.compile(r"^(?:.*/)?python[0-9.]*$") -_SCRIPT_RE = re.compile(r"^(?:.*/)?memory_search\.py$") +_SCRIPT_RE = re.compile(r"^/data/apps/memory(?:-[0-9]+)?/memory_search\.py$") +_CONTROL_TOKEN_RE = re.compile(r"^[;&|()<>]+$") -_EMPTY_RE = re.compile(r"^No relevant memories\.\s*$", re.MULTILINE) -_FILES_RE = re.compile(r"^FILES:[ \t]*(.+)$", re.MULTILINE) -# "- <title>: <excerpt> [<path>]" — the excerpt is greedy-free and the path is -# anchored to the end of the line, matching how memory_search.py builds it. -_SECTION_RE = re.compile(r"^- (.*?): (.*) \[([^\]]+)\]$", re.MULTILINE) +_RESULT_PREFIX = "MOBIUS_MEMORY_RESULT_V1:" +_RESULT_RE = re.compile( + rf"^{re.escape(_RESULT_PREFIX)}(?P<payload>\{{.*\}})[ \t]*$", + re.MULTILINE, +) # A citation path is only ever a repository-relative markdown pointer. Refusing # anything else keeps traversal, absolute paths, and control characters out of @@ -121,12 +98,24 @@ def _safe_path(value: str) -> str: return candidate if _PATH_RE.match(candidate) else "" -def _segment_runs_search(segment: str) -> bool: - """Whether one command segment EXECUTES the memory search script.""" - tokens = [token.strip("'\"") for token in segment.split()] - tokens = [token for token in tokens if token] - # `FOO=bar python3 script.py` — leading environment assignments are not the - # command being run. +def _simple_command_tokens(command: str) -> list[str] | None: + try: + lexer = shlex.shlex(command, posix=True, punctuation_chars=True) + lexer.whitespace_split = True + lexer.commenters = "" + tokens = list(lexer) + except ValueError: + return None + if not tokens or any(_CONTROL_TOKEN_RE.fullmatch(token) for token in tokens): + return None + # Reject substitutions/backticks conservatively. They are not part of the + # documented call and would turn this recognizer back into a shell parser. + if any("`" in token or "$(" in token for token in tokens): + return None + return tokens + + +def _tokens_run_search(tokens: list[str]) -> bool: index = 0 while index < len(tokens) and _ENV_ASSIGN_RE.match(tokens[index]): index += 1 @@ -134,15 +123,13 @@ def _segment_runs_search(segment: str) -> bool: return False head = tokens[index] if _SCRIPT_RE.match(head): - return True + return len(tokens) > index + 2 if not _INTERPRETER_RE.match(head): return False - # `python3 -u script.py` — interpreter flags may precede the script, but the - # first non-flag token is the thing actually being run. - for token in tokens[index + 1:]: + for script_index, token in enumerate(tokens[index + 1:], start=index + 1): if token.startswith("-"): continue - return bool(_SCRIPT_RE.match(token)) + return bool(_SCRIPT_RE.match(token) and len(tokens) > script_index + 2) return False @@ -162,59 +149,49 @@ def recall_from_command(command: object) -> dict | None: # not memory lookups and should cost one substring scan. if "memory_search.py" not in command: return None - for segment in _SEGMENT_RE.split(command): - if _segment_runs_search(segment): - return {"status": RECALL_SEARCHING} - return None + tokens = _simple_command_tokens(command) + return {"status": RECALL_SEARCHING} if tokens and _tokens_run_search(tokens) else None -def recall_from_output(text: object) -> dict: - """Parse a known memory lookup's stdout into a bounded citation set. - - Only ever called for a tool ``recall_from_command`` already identified, so - an unrecognized body is a lookup whose output we could not read — not a - reason to claim nothing was found. That case returns a note-less ``hit``, - which renders as a plain "recalled from Memory" beat rather than the far - stronger (and possibly false) "nothing relevant" claim. - """ +def recall_from_result(text: object, exit_code: object = None) -> dict: + """Validate a known Memory command's final structured result.""" + if isinstance(exit_code, bool): + exit_code = None + if isinstance(exit_code, int) and exit_code != 0: + return {"status": RECALL_FAILED} if not isinstance(text, str) or not text.strip(): - # No output at all: the command produced nothing readable. Keep the beat, - # drop the claim. - return {"status": RECALL_HIT, "notes": []} - - body = text[:_MAX_OUTPUT_SCAN_CHARS] - - files_match = None - for files_match in _FILES_RE.finditer(body): - # The last FILES: line wins — a head+tail carve keeps the tail, and the - # real one is always the final line memory_search.py prints. - pass - - if files_match is None: - if _EMPTY_RE.search(body): - return {"status": RECALL_EMPTY} - return {"status": RECALL_HIT, "notes": []} - - titles: dict[str, tuple[str, str]] = {} - for index, section in enumerate(_SECTION_RE.finditer(body)): - if index >= _MAX_SECTION_LINES_SCANNED: - break - raw_title, raw_excerpt, raw_path = section.groups() - path = _safe_path(raw_path) - if path and path not in titles: - titles[path] = ( - _clean(raw_title, MAX_RECALL_TITLE_CHARS), - _clean(raw_excerpt, MAX_RECALL_EXCERPT_CHARS), - ) + return {"status": RECALL_FAILED} + + body = text[-_MAX_OUTPUT_SCAN_CHARS:] + matches = list(_RESULT_RE.finditer(body)) + if not matches: + return {"status": RECALL_FAILED} + try: + payload = json.loads(matches[-1].group("payload")) + except (TypeError, ValueError, json.JSONDecodeError): + return {"status": RECALL_FAILED} + if not isinstance(payload, dict): + return {"status": RECALL_FAILED} + + status = payload.get("status") + if status == RECALL_FAILED: + return {"status": RECALL_FAILED} + if status == RECALL_EMPTY: + return {"status": RECALL_EMPTY} + if status != RECALL_HIT or not isinstance(payload.get("notes"), list): + return {"status": RECALL_FAILED} notes: list[dict[str, str]] = [] seen: set[str] = set() - for raw_path in files_match.group(1).split(","): - path = _safe_path(raw_path) + for raw_note in payload["notes"][:_MAX_SECTION_LINES_SCANNED]: + if not isinstance(raw_note, dict): + continue + path = _safe_path(raw_note.get("path")) if not path or path in seen: continue seen.add(path) - title, excerpt = titles.get(path, ("", "")) + title = _clean(raw_note.get("title"), MAX_RECALL_TITLE_CHARS) + excerpt = _clean(raw_note.get("excerpt"), MAX_RECALL_EXCERPT_CHARS) note = { "id": _note_id(path), "path": path, @@ -225,10 +202,10 @@ def recall_from_output(text: object) -> dict: notes.append(note) if len(notes) >= MAX_RECALL_NOTES: break - - # A FILES: line whose every entry failed validation is a malformed citation - # set, not an empty lookup — same conservative fallback as an unreadable body. - return {"status": RECALL_HIT, "notes": notes} + return ( + {"status": RECALL_HIT, "notes": notes} + if notes else {"status": RECALL_FAILED} + ) def merge_recall_notes( diff --git a/backend/tests/test_claude_sdk_runner.py b/backend/tests/test_claude_sdk_runner.py index 7b7fcae34..acdd19d28 100644 --- a/backend/tests/test_claude_sdk_runner.py +++ b/backend/tests/test_claude_sdk_runner.py @@ -1074,7 +1074,10 @@ def test_dispatch_client_web_search_tool_result_emits_sources(): {"type": "tool_start", "tool": "WebSearch", "input": "", "tool_use_id": "t1"}, {"type": "tool_input", "tool": "WebSearch", "input": "mobius docs", "tool_use_id": "t1"}, - {"type": "tool_output", "content": result_text, "tool_use_id": "t1"}, + { + "type": "tool_output", "content": result_text, "tool_use_id": "t1", + "output_complete": True, + }, # tool_use_id binds these sources to the search that produced them, so a # batch of parallel WebSearch calls does not collapse onto one block. {"type": "tool_sources", "tool_use_id": "t1", "sources": [ diff --git a/backend/tests/test_codex_sdk_runner.py b/backend/tests/test_codex_sdk_runner.py index 0404459c4..68dbb1b21 100644 --- a/backend/tests/test_codex_sdk_runner.py +++ b/backend/tests/test_codex_sdk_runner.py @@ -205,8 +205,9 @@ def test_stamp_tool_use_id_uses_stable_item_id(): def test_tool_completed_events_emit_output_before_end(): class CommandExecutionThreadItem: - def __init__(self, output: str): + def __init__(self, output: str, exit_code: int = 0): self.aggregated_output = output + self.exit_code = exit_code sdk = {"CommandExecutionThreadItem": CommandExecutionThreadItem} sdk.update({ @@ -222,7 +223,20 @@ def __init__(self, output: str): ) assert events == [ - {"type": "tool_output", "content": "hello"}, + { + "type": "tool_output", "content": "hello", + "output_complete": True, "output_exit_code": 0, + }, + {"type": "tool_end"}, + ] + + assert codex_sdk_runner._tool_completed_events( + CommandExecutionThreadItem("", exit_code=7), sdk, + ) == [ + { + "type": "tool_output", "content": "", + "output_complete": True, "output_exit_code": 7, + }, {"type": "tool_end"}, ] diff --git a/backend/tests/test_memory_recall.py b/backend/tests/test_memory_recall.py index 25d9c0962..680ab6dcd 100644 --- a/backend/tests/test_memory_recall.py +++ b/backend/tests/test_memory_recall.py @@ -6,19 +6,23 @@ much about what is NOT stamped as about what is. """ +import json + from app.chat_transcript import ( _compact_activity_item, _compact_activity_run, _distinctive_activity, ) +from app.chat import _ChatEventSink from app.events import process_event from app.memory_recall import ( MAX_RECALL_NOTES, RECALL_EMPTY, + RECALL_FAILED, RECALL_HIT, RECALL_SEARCHING, recall_from_command, - recall_from_output, + recall_from_result, ) MEMORY_CMD = 'python3 /data/apps/memory/memory_search.py "what does he prefer" "chat-1"' @@ -28,7 +32,12 @@ HIT_OUTPUT = """Relevant memories: - Apps render in a sandboxed frame: Each mini-app runs isolated. [notes/apps-render-in-a-sandboxed-frame.md] - Theme variables are shared: Colors come from one stylesheet. [notes/theme-variables-are-shared.md] -FILES: notes/apps-render-in-a-sandboxed-frame.md, notes/theme-variables-are-shared.md""" +FILES: notes/apps-render-in-a-sandboxed-frame.md, notes/theme-variables-are-shared.md +MOBIUS_MEMORY_RESULT_V1:{"status":"hit","notes":[{"id":"apps-render-in-a-sandboxed-frame","path":"notes/apps-render-in-a-sandboxed-frame.md","title":"Apps render in a sandboxed frame","excerpt":"Each mini-app runs isolated."},{"id":"theme-variables-are-shared","path":"notes/theme-variables-are-shared.md","title":"Theme variables are shared","excerpt":"Colors come from one stylesheet."}]}""" +EMPTY_OUTPUT = """No relevant memories. +MOBIUS_MEMORY_RESULT_V1:{"status":"empty"}""" +FAILED_OUTPUT = """Memory lookup failed. +MOBIUS_MEMORY_RESULT_V1:{"status":"failed"}""" # --- identification ------------------------------------------------------- @@ -54,18 +63,32 @@ def test_a_command_merely_mentioning_memory_search_is_not_a_lookup(): assert recall_from_command(None) is None -def test_the_script_is_recognized_however_it_is_invoked(): - assert recall_from_command("memory_search.py 'q'") is not None - assert recall_from_command('cd /x && python3 ./memory_search.py "q"') is not None - assert recall_from_command('python3 -u "/a/b/memory_search.py" "q"') is not None - assert recall_from_command('MEMORY_READER_PROVIDER=none python3 /a/memory_search.py "q"') is not None - assert recall_from_command('/usr/bin/python3.12 /a/memory_search.py "q"') is not None +def test_the_documented_simple_invocation_is_recognized(): + assert recall_from_command(MEMORY_CMD) is not None + assert recall_from_command( + 'MEMORY_READER_PROVIDER=none python3 -u ' + '/data/apps/memory-2/memory_search.py "q" "chat-1"' + ) is not None + + +def test_shell_composition_and_non_memory_paths_are_rejected_conservatively(): + assert recall_from_command( + 'python3 /data/apps/memory/memory_search.py "q"' + ) is None + assert recall_from_command( + 'cd /x && python3 /data/apps/memory/memory_search.py "q"' + ) is None + assert recall_from_command('python3 ./memory_search.py "q"') is None + assert recall_from_command('python3 /a/b/memory_search.py "q"') is None + assert recall_from_command( + 'python3 /data/apps/memory/memory_search.py "q" > /tmp/result' + ) is None # --- parsing -------------------------------------------------------------- def test_a_successful_lookup_cites_the_notes_it_opened(): - recall = recall_from_output(HIT_OUTPUT) + recall = recall_from_result(HIT_OUTPUT, 0) assert recall["status"] == RECALL_HIT assert [note["id"] for note in recall["notes"]] == [ "apps-render-in-a-sandboxed-frame", "theme-variables-are-shared", @@ -75,59 +98,67 @@ def test_a_successful_lookup_cites_the_notes_it_opened(): def test_a_lookup_that_found_nothing_says_so(): - assert recall_from_output("No relevant memories.") == {"status": RECALL_EMPTY} + assert recall_from_result(EMPTY_OUTPUT, 0) == {"status": RECALL_EMPTY} -def test_a_carved_output_keeps_every_citation_and_falls_back_on_titles(): - # A full lookup exceeds the inline threshold and is reduced to head+tail, so - # the titled section lines in the middle can be lost. The FILES: line is - # authoritative and sits in the tail, so the citation SET must survive whole. - carved = "Relevant memories:\n- A: b [notes/a.md]\n…\nFILES: notes/a.md, notes/z.md" - recall = recall_from_output(carved) - assert [note["id"] for note in recall["notes"]] == ["a", "z"] - assert recall["notes"][1]["title"] == "z", "a title-less note still reads" +def test_a_carved_output_keeps_the_structured_tail_result(): + carved = "presentation head\n…[large middle carved]…\n" + HIT_OUTPUT.splitlines()[-1] + recall = recall_from_result(carved, 0) + assert [note["id"] for note in recall["notes"]] == [ + "apps-render-in-a-sandboxed-frame", "theme-variables-are-shared", + ] -def test_an_unreadable_body_never_claims_nothing_was_found(): - # "Nothing relevant" is a strong claim about the owner's memory. Only the - # app's own marker may make it; anything else degrades to a note-less beat. +def test_unreadable_or_failed_results_are_explicit_failures(): for body in ("", " ", "some unrelated text", "Traceback (most recent call last):"): - recall = recall_from_output(body) - assert recall["status"] == RECALL_HIT - assert recall["notes"] == [] + assert recall_from_result(body, 0) == {"status": RECALL_FAILED} + assert recall_from_result(FAILED_OUTPUT, 1) == {"status": RECALL_FAILED} + assert recall_from_result(HIT_OUTPUT, 1) == {"status": RECALL_FAILED} def test_a_citation_path_may_not_escape_the_graph(): - recall = recall_from_output( - "FILES: ../../etc/passwd, /abs/x.md, notes/../secret.md, notes/ok.md" + recall = recall_from_result( + 'MOBIUS_MEMORY_RESULT_V1:{"status":"hit","notes":[' + '{"path":"../../etc/passwd"},{"path":"/abs/x.md"},' + '{"path":"notes/../secret.md"},{"path":"notes/ok.md"}]}', + 0, ) assert [note["path"] for note in recall["notes"]] == ["notes/ok.md"] def test_repeated_and_excessive_citations_are_bounded(): - paths = ", ".join(f"notes/n{i}.md" for i in range(40)) - recall = recall_from_output(f"FILES: notes/dup.md, notes/dup.md, {paths}") + notes = [{"path": "notes/dup.md"}, {"path": "notes/dup.md"}] + [ + {"path": f"notes/n{i}.md"} for i in range(40) + ] + recall = recall_from_result( + "MOBIUS_MEMORY_RESULT_V1:" + json.dumps({"status": "hit", "notes": notes}), + 0, + ) assert len(recall["notes"]) == MAX_RECALL_NOTES assert recall["notes"][0]["path"] == "notes/dup.md" assert len({note["path"] for note in recall["notes"]}) == len(recall["notes"]) -def test_the_last_files_line_wins_after_a_head_tail_carve(): - # A carve keeps the head, so a stale FILES: from the head could linger above - # the real one. memory_search.py always prints it last. - recall = recall_from_output( - "FILES: notes/stale.md\n…carved…\nRelevant memories:\nFILES: notes/real.md" +def test_the_last_structured_result_line_wins(): + recall = recall_from_result( + 'MOBIUS_MEMORY_RESULT_V1:{"status":"hit","notes":[{"path":"notes/stale.md"}]}\n' + 'MOBIUS_MEMORY_RESULT_V1:{"status":"hit","notes":[{"path":"notes/real.md"}]}', + 0, ) assert [note["path"] for note in recall["notes"]] == ["notes/real.md"] # --- the block carries it through persistence ------------------------------ -def _tool_blocks(recall_in, recall_out, output=HIT_OUTPUT): +def _tool_blocks(recall_in, recall_out, output=HIT_OUTPUT, recall_on_start=False): blocks: list = [] - process_event({"type": "tool_start", "tool": "Bash", "tool_use_id": "t1"}, blocks) + start = {"type": "tool_start", "tool": "Bash", "input": MEMORY_CMD, + "tool_use_id": "t1"} + if recall_on_start and recall_in is not None: + start["recall"] = recall_in + process_event(start, blocks) event_in = {"type": "tool_input", "tool_use_id": "t1", "input": MEMORY_CMD} - if recall_in is not None: + if not recall_on_start and recall_in is not None: event_in["recall"] = recall_in process_event(event_in, blocks) event_out = {"type": "tool_output", "tool_use_id": "t1", "content": output} @@ -146,6 +177,63 @@ def test_the_lookup_marker_reaches_the_persisted_block_and_then_settles(): assert blocks[0]["recall"]["notes"][0]["id"] == "a" +def test_codex_tool_start_carries_the_lookup_marker_without_tool_input(): + blocks = _tool_blocks( + {"status": RECALL_SEARCHING}, + {"status": RECALL_HIT, "notes": [{"id": "a", "path": "notes/a.md"}]}, + recall_on_start=True, + ) + assert blocks[0]["recall"]["status"] == RECALL_HIT + + +def test_partial_output_does_not_settle_the_lookup_before_completion(): + blocks: list = [] + process_event({ + "type": "tool_start", "tool": "Bash", "input": MEMORY_CMD, + "tool_use_id": "t1", "recall": {"status": RECALL_SEARCHING}, + }, blocks) + process_event({"type": "tool_output", "tool_use_id": "t1", "content": "partial"}, blocks) + assert blocks[0]["recall"]["status"] == RECALL_SEARCHING + process_event({ + "type": "tool_output", "tool_use_id": "t1", "content": HIT_OUTPUT, + "recall": {"status": RECALL_HIT, "notes": [{"id": "a", "path": "notes/a.md"}]}, + }, blocks) + assert blocks[0]["recall"]["status"] == RECALL_HIT + + +def _sink_lifecycle(events): + sink = object.__new__(_ChatEventSink) + sink.assistant_blocks = [] + for event in events: + sink._stamp_memory_recall(event) + process_event(event, sink.assistant_blocks) + return sink.assistant_blocks[0]["recall"] + + +def test_claude_and_codex_lifecycles_settle_to_identical_recall_metadata(): + final = { + "type": "tool_output", "tool_use_id": "t1", "content": HIT_OUTPUT, + "output_complete": True, "output_exit_code": 0, + } + codex = _sink_lifecycle([ + {"type": "tool_start", "tool": "Bash", "input": MEMORY_CMD, + "tool_use_id": "t1"}, + {"type": "tool_output", "tool_use_id": "t1", "content": "partial"}, + dict(final), + ]) + claude = _sink_lifecycle([ + {"type": "tool_start", "tool": "Bash", "input": "", + "tool_use_id": "t1"}, + {"type": "tool_input", "input": MEMORY_CMD, "tool_use_id": "t1"}, + dict(final), + ]) + assert codex == claude + assert codex["status"] == RECALL_HIT + assert [note["id"] for note in codex["notes"]] == [ + "apps-render-in-a-sandboxed-frame", "theme-variables-are-shared", + ] + + def test_an_ordinary_command_gains_no_recall_field(): blocks = _tool_blocks(None, None, output="total 0\n") assert "recall" not in blocks[0] @@ -159,6 +247,12 @@ def test_consulting_memory_is_its_own_activity_beat(): assert not _distinctive_activity({"type": "tool", "tool": "Bash"}) +def test_a_failed_lookup_remains_an_activity_without_citations(): + assert _distinctive_activity({ + "type": "tool", "tool": "Bash", "recall": {"status": RECALL_FAILED}, + }) + + def test_the_compacted_line_still_knows_what_it_recalled(): # Without this the beat renders live and reverts to "Ran a command" on the # next chat load, which is worse for trust than never having shown it. @@ -207,3 +301,14 @@ def test_an_all_empty_run_still_reports_that_it_looked(): "recall": {"status": RECALL_EMPTY}})] run = _compact_activity_run(blocks, message_index=0) assert run["recall"] == {"status": RECALL_EMPTY, "notes": []} + + +def test_a_successful_empty_lookup_outranks_a_failed_probe_in_the_same_run(): + blocks = [ + (0, {"type": "tool", "tool": "Bash", "status": "done", + "recall": {"status": RECALL_FAILED}}), + (1, {"type": "tool", "tool": "Bash", "status": "done", + "recall": {"status": RECALL_EMPTY}}), + ] + run = _compact_activity_run(blocks, message_index=0) + assert run["recall"] == {"status": RECALL_EMPTY, "notes": []} diff --git a/backend/tests/test_tool_output_excerpt.py b/backend/tests/test_tool_output_excerpt.py index 58c263144..bee70ab60 100644 --- a/backend/tests/test_tool_output_excerpt.py +++ b/backend/tests/test_tool_output_excerpt.py @@ -9,6 +9,7 @@ TOOL_OUTPUT_HEAD, TOOL_OUTPUT_INLINE_THRESHOLD, excerpt_tool_output, + tool_output_exit_code, ) @@ -38,6 +39,12 @@ def test_bash_failure_head_and_exit_code_survive_truncation(): assert full_len == len(content) +def test_small_tool_outputs_expose_the_same_typed_exit_code(): + assert tool_output_exit_code("Exit code 7\nfailed") == 7 + assert tool_output_exit_code(json.dumps({"stderr": "x", "exit_code": 2})) == 2 + assert tool_output_exit_code("success") is None + + def test_json_envelope_stays_valid_json_with_exit_code_intact(): envelope = { "stdout": "S" * 200000, diff --git a/frontend/src/components/ChatView/MessageSources.jsx b/frontend/src/components/ChatView/MessageSources.jsx index d32f0535d..911c0d379 100644 --- a/frontend/src/components/ChatView/MessageSources.jsx +++ b/frontend/src/components/ChatView/MessageSources.jsx @@ -1,3 +1,4 @@ +import BrainCircuit from 'lucide-react/dist/esm/icons/brain-circuit.mjs' import { messageSources, sourceHost, sourceLabel } from './messageSources.js' import { messageRecall, noteHref, noteLabel } from './memoryRecall.js' @@ -6,29 +7,8 @@ function sourceMark(host) { return displayHost.match(/[a-z0-9]/i)?.[0]?.toUpperCase() || '•' } -// A recalled note is not a website, so it gets a mark of its own rather than a -// domain letter. Local and inline for the same reason the web chip's mark is: -// nothing about viewing an answer should contact a remote server. function MemoryMark() { - return ( - <svg - className="chat__source-glyph" - viewBox="0 0 16 16" - aria-hidden="true" - focusable="false" - > - <path - d="M8 2.2c-1.5 0-2.6 1-2.8 2.3-1.1.3-1.9 1.2-1.9 2.4 0 .5.2 1 .4 1.4-.3.4-.5.9-.5 1.5 0 1.4 1.2 2.5 2.7 2.5.5 0 1-.1 1.4-.4.2.5.4.9.7 1.2V2.2z" - fill="currentColor" - opacity="0.85" - /> - <path - d="M8 2.2c1.5 0 2.6 1 2.8 2.3 1.1.3 1.9 1.2 1.9 2.4 0 .5-.2 1-.4 1.4.3.4.5.9.5 1.5 0 1.4-1.2 2.5-2.7 2.5-.5 0-1-.1-1.4-.4-.2.5-.4.9-.7 1.2V2.2z" - fill="currentColor" - opacity="0.55" - /> - </svg> - ) + return <BrainCircuit className="chat__source-glyph" aria-hidden="true" /> } // Everything that informed an answer, surfaced ONCE at the end of the message: diff --git a/frontend/src/components/ChatView/__tests__/groupBlocks.test.js b/frontend/src/components/ChatView/__tests__/groupBlocks.test.js index 754eabdbc..109940aae 100644 --- a/frontend/src/components/ChatView/__tests__/groupBlocks.test.js +++ b/frontend/src/components/ChatView/__tests__/groupBlocks.test.js @@ -11,6 +11,7 @@ import { toolCallLabel, effectiveToolName, isDistinctiveActivityTool, + memoryRecallLabel, } from '../toolActivityLabel.js' const e = item => ({ item }) @@ -223,6 +224,15 @@ test('toolCallLabel names the concrete nested step in progressive and past tense ) }) +test('failed Memory activity is honest and remains distinctive', () => { + const failed = { + type: 'tool', tool: 'Bash', status: 'done', + recall: { status: 'failed' }, + } + assert.equal(memoryRecallLabel(failed), 'Memory lookup failed') + assert.equal(isDistinctiveActivityTool(failed), true) +}) + const think = (content, duration_ms = 0) => ({ type: 'thinking', content, duration_ms }) diff --git a/frontend/src/components/ChatView/__tests__/memoryRecall.test.js b/frontend/src/components/ChatView/__tests__/memoryRecall.test.js index 53ad7eddd..2d1c0f96c 100644 --- a/frontend/src/components/ChatView/__tests__/memoryRecall.test.js +++ b/frontend/src/components/ChatView/__tests__/memoryRecall.test.js @@ -70,10 +70,8 @@ test('compacted activity carries citations so they survive a reload', () => { assert.deepEqual(recall.notes.map(n => n.id), ['alpha']) }) -test('a lookup whose output could not be parsed still counts as looking', () => { - const recall = messageRecall([toolBlock({ status: 'hit', notes: [] })]) - assert.deepEqual(recall, { notes: [], empty: false }, - 'no notes to cite, but no false "nothing relevant" claim either') +test('a failed lookup creates no citation section or empty-memory claim', () => { + assert.equal(messageRecall([toolBlock({ status: 'failed' })]), null) }) test('citations are bounded so one turn cannot flood the transcript', () => { diff --git a/frontend/src/components/ChatView/__tests__/streamReducers.test.js b/frontend/src/components/ChatView/__tests__/streamReducers.test.js index c35d4c811..7dc1647e1 100644 --- a/frontend/src/components/ChatView/__tests__/streamReducers.test.js +++ b/frontend/src/components/ChatView/__tests__/streamReducers.test.js @@ -30,6 +30,7 @@ import { appendTextItem, repairInterleavedQuestionText, replaceTextItem, + startToolLifecycle, } from '../streamReducers.js' import { questionKey } from '../questionKey.js' @@ -46,6 +47,17 @@ function questionEvent(id, text) { } } +test('Codex tool start preserves provider-neutral Memory recall metadata', () => { + const items = startToolLifecycle([], { + tool: 'Bash', + input: 'python3 /data/apps/memory/memory_search.py "q"', + tool_use_id: 'cmd-1', + recall: { status: 'searching' }, + }) + assert.deepEqual(items[0].recall, { status: 'searching' }) + assert.equal(items[0].tool_use_id, 'cmd-1') +}) + test('text item identity keeps late deltas before an interleaved question', () => { let items = appendTextItem([], 'Build it', { textItemId: 'msg-1' }) items = upsertQuestionItem(items, questionEvent('q1', 'Proceed?')) @@ -259,6 +271,15 @@ test('large live tool output keeps the metadata needed for lazy full fetch', () }) }) +test('small completed command output keeps its typed exit code', () => { + const prev = [toolItem('Bash', { input: 'false' })] + const next = attachToolOutput(prev, 'failed', { + tool_use_id: 'toolu_small_failure', + output_exit_code: 1, + }) + assert.equal(next[0].output_exit_code, 1) +}) + test('live output metadata cannot replace a runner-assigned tool identity', () => { const prev = [toolItem('Bash', { tool_use_id: 'toolu_original' })] const next = attachToolOutput(prev, 'bounded excerpt', { diff --git a/frontend/src/components/ChatView/memoryRecall.js b/frontend/src/components/ChatView/memoryRecall.js index 077efae08..26b978a83 100644 --- a/frontend/src/components/ChatView/memoryRecall.js +++ b/frontend/src/components/ChatView/memoryRecall.js @@ -82,6 +82,9 @@ export function messageRecall(blocks) { if (!recall || typeof recall !== 'object') continue // A lookup still in flight is a live activity beat, not yet a citation. if (recall.status === 'searching') continue + // A failed lookup remains visible in its activity row, but it did not + // successfully consult the graph and must not mint a source section. + if (recall.status === 'failed') continue looked = true if (recall.status === 'empty') empty = true if (!Array.isArray(recall.notes)) continue diff --git a/frontend/src/components/ChatView/streamReducers.js b/frontend/src/components/ChatView/streamReducers.js index 4d98962c1..232543810 100644 --- a/frontend/src/components/ChatView/streamReducers.js +++ b/frontend/src/components/ChatView/streamReducers.js @@ -38,6 +38,19 @@ import { // Mirrors backend/app/tool_summaries.py's question-tool branch. const QUESTION_TOOLS = new Set(['AskUserQuestion', 'request_user_input']) +/** Build the one live tool-block shape shared by every provider. */ +export function startToolLifecycle(prev, event) { + return [...prev, { + type: 'tool', + tool: event?.tool, + input: event?.input || '', + output: '', + status: 'running', + ...(event?.recall ? { recall: event.recall } : {}), + ...(event?.tool_use_id ? { tool_use_id: event.tool_use_id } : {}), + }] +} + /** * Append a streamed text delta to its provider message item. * @@ -342,12 +355,12 @@ export function attachToolOutput(prev, content, event = null) { if (event?.recall) { block.recall = event.recall } + if (event?.output_exit_code != null) { + block.output_exit_code = event.output_exit_code + } if (event?.output_truncated) { block.output_truncated = true block.output_full_len = event.output_full_len - if (event.output_exit_code != null) { - block.output_exit_code = event.output_exit_code - } } updated[i] = block return updated diff --git a/frontend/src/components/ChatView/toolActivityLabel.js b/frontend/src/components/ChatView/toolActivityLabel.js index c2dc3997c..dd3d0d71f 100644 --- a/frontend/src/components/ChatView/toolActivityLabel.js +++ b/frontend/src/components/ChatView/toolActivityLabel.js @@ -212,7 +212,7 @@ export function effectiveToolName(tool) { // swallowed into a chip, not a tool block). const DISTINCTIVE_ACTIVITIES = new Set(['ViewImage', 'MemoryRecall']) -// The one-line story of a memory lookup, in the three states that matter. +// The one-line story of a memory lookup, including honest operational failure. // Reading the count from the citation set the backend already parsed keeps the // label and the pills under the answer from ever disagreeing. export function memoryRecallLabel(tool) { @@ -221,6 +221,7 @@ export function memoryRecallLabel(tool) { return 'Searching Memory' } if (recall?.status === 'empty') return 'Searched Memory — nothing relevant' + if (recall?.status === 'failed') return 'Memory lookup failed' const count = Array.isArray(recall?.notes) ? recall.notes.length : 0 if (count === 0) return 'Recalled from Memory' return `Recalled ${count} note${count === 1 ? '' : 's'} from Memory` diff --git a/frontend/src/components/ChatView/useStreamConnection.js b/frontend/src/components/ChatView/useStreamConnection.js index 08361df67..9b4942555 100644 --- a/frontend/src/components/ChatView/useStreamConnection.js +++ b/frontend/src/components/ChatView/useStreamConnection.js @@ -17,6 +17,7 @@ import { applyTaskEvent, appendTextItem, replaceTextItem, + startToolLifecycle, } from './streamReducers.js' import { readStoredStreamSnapshot, @@ -874,21 +875,9 @@ export default function useStreamConnection(chatId, { } } else if (event.type === 'tool_start') { flushBuffer() - applyStreamItems(prev => [...prev, { - type: 'tool', - tool: event.tool, - input: event.input || '', - output: '', - status: 'running', - // Carry the real per-tool identity from the wire (lever 2a). It - // keys the tool block (StreamingMessage/MsgContent) and lets a - // catch-up commit reconcile onto it by identity instead of - // remounting the heaviest block (expanded state, <img>, lazy - // fullOutput). streamItemToBlock spreads ...item, so it flows to - // the promoted message too. Undefined on a legacy/tokenless wire, - // where the ordinal fallback key still applies. - tool_use_id: event.tool_use_id, - }]) + // Codex identifies Memory here; Claude may add the same marker on + // a later tool_input. One reducer owns the shared live block shape. + applyStreamItems(prev => startToolLifecycle(prev, event)) } else if (event.type === 'tool_input') { // Backfill by stable identity. Older id-less events retain their // earliest-input-less fallback; a late id may be adopted only when From d3137388f1a1be465faa439b2dd2c084036a03a2 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:34:42 +0000 Subject: [PATCH 08/15] fix(drawer): give activity dots a role and a non-color visual channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- .../__tests__/framePipelineQuiescence.test.js | 6 +++++- frontend/src/components/Drawer/Drawer.css | 19 +++++++++++-------- frontend/src/components/Drawer/Drawer.jsx | 2 ++ 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js b/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js index a273f326c..d751b88b7 100644 --- a/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js +++ b/frontend/src/components/ChatView/__tests__/framePipelineQuiescence.test.js @@ -128,6 +128,10 @@ test('streaming and attention rows stay tellable apart without motion', () => { const streaming = ruleBody('.drawer__streaming-dot') const attention = ruleBody('.drawer__attention-dot') assert.notEqual(streaming, attention, 'the two drawer row states render identically') + // Streaming is a filled disc in --accent; the finished/attention dot is a + // hollow ring (transparent fill + coloured border) so the two stay tellable + // apart on a non-colour channel for colour-vision-deficient users. assert.match(streaming, /background:\s*var\(--accent\)/) - assert.match(attention, /background:\s*var\(--green\)/) + assert.match(attention, /background:\s*transparent/) + assert.match(attention, /border:[^;]*var\(--green\)/) }) diff --git a/frontend/src/components/Drawer/Drawer.css b/frontend/src/components/Drawer/Drawer.css index 9063886f0..3b828f444 100644 --- a/frontend/src/components/Drawer/Drawer.css +++ b/frontend/src/components/Drawer/Drawer.css @@ -661,20 +661,23 @@ /* "The latest background run FINISHED while you were elsewhere" — a different row state from the streaming dot above, and it has to stay - legible as one now that motion no longer separates them. Colour is the - channel that survives at 6px (a hollow ring or a one-pixel size delta - does not), and --green is already the palette's finished/healthy token - — SettingsView uses it for a connected provider — against --accent for - work still in flight. Colour is not the only channel: both dots carry - an aria-label and a title (Drawer.jsx), which is what a colour-blind or - screen-reader user reads. */ + legible as one now that motion no longer separates them. Colour alone + is not enough for colour-vision-deficient users, so this dot adds a + non-colour channel: it renders as a hollow ring (transparent fill + + coloured border) while the streaming dot stays a filled disc. --green + is the palette's finished/healthy token — SettingsView uses it for a + connected provider — against --accent for work still in flight. Both + dots also carry role="img" + aria-label + title (Drawer.jsx) so the + accessible name is reliable for screen-reader users. */ .drawer__attention-dot { flex-shrink: 0; + box-sizing: border-box; width: 6px; height: 6px; margin-right: 2px; border-radius: 50%; - background: var(--green); + background: transparent; + border: 1.5px solid var(--green); } /* Respect prefers-reduced-motion. The streaming dot no longer needs an diff --git a/frontend/src/components/Drawer/Drawer.jsx b/frontend/src/components/Drawer/Drawer.jsx index 3e7a8b41b..4a947eedc 100644 --- a/frontend/src/components/Drawer/Drawer.jsx +++ b/frontend/src/components/Drawer/Drawer.jsx @@ -994,12 +994,14 @@ const DrawerRow = memo(function DrawerRow({ ) : building ? ( <span className="drawer__streaming-dot" + role="img" aria-label="Building" title="Building…" /> ) : attention ? ( <span className="drawer__attention-dot" + role="img" aria-label="New activity" title="New activity" /> From b7e2d8dcc27464f6dd03658f06696f6752f5eea9 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:35:07 +0000 Subject: [PATCH 09/15] test(chat): make resume-affordance element slicing tag-close aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- .../__tests__/resumeAffordance.test.js | 76 +++++++++++++++++-- 1 file changed, 71 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js b/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js index e143506ab..27b66dc96 100644 --- a/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js +++ b/frontend/src/components/ChatView/__tests__/resumeAffordance.test.js @@ -15,15 +15,81 @@ const css = readFileSync(new URL('../ChatView.css', import.meta.url), 'utf8') // with an unbounded `[\s\S]*?` between the tag and the prop, a DIFFERENT // element's props further down the file satisfy the match, so "both render // paths pass the ref" can pass with one of them not passing it at all. +// Scan the opening tag that begins at `start` (source[start] === '<'), skipping +// over quoted strings ('...', "...", `...`) and balanced { } JSX expression +// containers so that a '>' inside an arrow prop (onClick={() => ...}), a +// comparison, or a comment does NOT prematurely end the tag. Returns the index +// of the tag-closing '>' and whether the tag is self-closing ('/>'). +function scanOpeningTag(source, start) { + for (let i = start + 1, brace = 0; i < source.length; i++) { + const c = source[i] + if (c === '"' || c === "'" || c === '`') { + i++ + while (i < source.length && source[i] !== c) i++ + continue + } + if (c === '{') { brace++; continue } + if (c === '}') { brace--; continue } + if (brace > 0) continue + if (c === '/' && source[i + 1] === '>') return { gt: i + 1, selfClosing: true } + if (c === '>') return { gt: i, selfClosing: false } + } + return { gt: -1, selfClosing: false } +} + function sliceElement(source, openTag) { const from = source.indexOf(openTag) assert.ok(from >= 0, `expected to find ${openTag}`) - let depth = 0 - for (let i = from; i < source.length; i++) { - if (source[i] === '<') depth++ - if (source[i] === '>') { + // Recover the tag name from the opening tag (e.g. '<button' -> 'button', + // '<div\n className=...' -> 'div') so we can find its real close. + const nameMatch = /^<\s*([A-Za-z][\w.]*)/.exec(openTag) + assert.ok(nameMatch, `openTag must start with a tag name: ${openTag}`) + const tagName = nameMatch[1] + + const opening = scanOpeningTag(source, from) + assert.notEqual(opening.gt, -1, `unterminated opening tag ${openTag}`) + + if (opening.selfClosing) { + const slice = source.slice(from, opening.gt + 1) + assert.ok(slice.endsWith('/>'), + `self-closing slice for ${openTag} must end with '/>'`) + return slice + } + + // Not self-closing: walk forward, quote/brace aware, tracking nesting of + // same-named tags, until the matching '</tagName>' close. + const close = `</${tagName}` + const nestedOpen = new RegExp(`^<\\s*${tagName}[\\s/>]`) + let depth = 1 + for (let i = opening.gt + 1, brace = 0; i < source.length; i++) { + const c = source[i] + if (c === '"' || c === "'" || c === '`') { + i++ + while (i < source.length && source[i] !== c) i++ + continue + } + if (c === '{') { brace++; continue } + if (c === '}') { brace--; continue } + if (brace > 0) continue + if (c !== '<') continue + if (source.startsWith(close, i)) { depth-- - if (depth === 0) return source.slice(from, i + 1) + if (depth === 0) { + const gt = source.indexOf('>', i) + assert.notEqual(gt, -1, `unterminated close for ${openTag}`) + const slice = source.slice(from, gt + 1) + const tail = slice.replace(/\s+/g, ' ') + assert.ok(tail.endsWith(`${close}>`) || tail.endsWith(`${close} >`), + `slice for ${openTag} must end with ${close}>`) + return slice + } + continue + } + // A nested opening tag of the same name deepens the nesting — unless it is + // self-closing, which needs no matching close. + if (nestedOpen.test(source.slice(i))) { + const nested = scanOpeningTag(source, i) + if (nested.gt !== -1 && !nested.selfClosing) depth++ } } assert.fail(`unterminated element ${openTag}`) From 0751bf298c8b55a705ed60a2a47e480545ac75cf Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:35:58 +0000 Subject: [PATCH 10/15] Cover Codex stop usage accounting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/codex_sdk_runner.py | 6 +-- backend/tests/test_codex_sdk_runner.py | 52 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/backend/app/codex_sdk_runner.py b/backend/app/codex_sdk_runner.py index d81a901db..c78a5b9c6 100644 --- a/backend/app/codex_sdk_runner.py +++ b/backend/app/codex_sdk_runner.py @@ -571,7 +571,6 @@ def _sdk_imports() -> dict[str, Any]: from openai_codex.client import CodexConfig from openai_codex.errors import ( CodexRpcError, - InvalidParamsError, TransportClosedError, ) from openai_codex.types import ReasoningEffort, ReasoningSummary @@ -650,7 +649,6 @@ def _sdk_imports() -> dict[str, Any]: "ErrorNotification": ErrorNotification, "FileChangePatchUpdatedNotification": FileChangePatchUpdatedNotification, "FileChangeThreadItem": FileChangeThreadItem, - "InvalidParamsError": InvalidParamsError, "ReasoningEffort": ReasoningEffort, "ReasoningSummary": ReasoningSummary, "Sandbox": Sandbox, @@ -1500,7 +1498,9 @@ def _is_transport_death(exc: BaseException) -> bool: return False if isinstance(exc, sdk["TransportClosedError"]): return True - if isinstance(exc, (sdk["InvalidParamsError"], sdk["CodexRpcError"])): + # InvalidParamsError is a subclass of CodexRpcError, so matching the base + # class alone already covers it. + if isinstance(exc, sdk["CodexRpcError"]): text = str(exc).lower() return "closed" in text or "not running" in text or "broken pipe" in text return False diff --git a/backend/tests/test_codex_sdk_runner.py b/backend/tests/test_codex_sdk_runner.py index 0cdf0dcd6..a1337659b 100644 --- a/backend/tests/test_codex_sdk_runner.py +++ b/backend/tests/test_codex_sdk_runner.py @@ -1449,6 +1449,58 @@ def test_run_codex_sdk_turn_reports_self_requested_kill_as_interrupted( assert registry.get_handle("chat-1", RunnerKind.CODEX_SDK) is None +def test_run_codex_sdk_turn_self_requested_kill_still_reports_usage( + monkeypatch, +): + """A stop we asked for is a clean interrupt, but the tokens still count. + + Usage delivered before the self-kill must survive the transport-death + reclassification: the interrupted (error=None) exit routes through + with_usage just like every other exit, so a stopped turn is not free in + the budget ledger. + """ + class TokenUsageUpdated: + def __init__(self, token_usage): + self.token_usage = token_usage + + class Usage: + def __init__(self, total_tokens): + self.last = SimpleNamespace( + input_tokens=200, cached_input_tokens=100, output_tokens=100, + reasoning_output_tokens=50, total_tokens=300, + ) + self.total = SimpleNamespace( + input_tokens=1_000, cached_input_tokens=400, output_tokens=100, + reasoning_output_tokens=50, total_tokens=total_tokens, + ) + self.model_context_window = 200_000 + + def model_dump(self, **_kwargs): + return { + "last": vars(self.last), + "total": vars(self.total), + "modelContextWindow": self.model_context_window, + } + + result, _bc = _run_turn_whose_stream_dies( + monkeypatch, + _KilledTransportError("Codex process closed stdout. stderr_tail="), + on_register=_mark_interrupted, + notifications=[ + SimpleNamespace( + method="thread/tokenUsage/updated", + payload=TokenUsageUpdated(Usage(1_100)), + ), + ], + sdk_patch={"ThreadTokenUsageUpdatedNotification": TokenUsageUpdated}, + ) + + assert result["error"] is None + assert result["terminal_status"] == "interrupted" + assert result["usage"]["total"]["total_tokens"] == 1_100 + assert "usage_metrics" in result + + def test_run_codex_sdk_turn_unrequested_transport_death_stays_an_error( monkeypatch, ): From ad98c12a7c9b414f0a59c02216cd2e39cd2ec391 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 02:17:18 +0000 Subject: [PATCH 11/15] Harden Memory recall provenance and tool outcomes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/chat.py | 74 +++++++++++++------ backend/app/memory_recall.py | 36 ++++++--- backend/tests/test_memory_recall.py | 33 ++++++++- backend/tests/test_tool_output_stash.py | 44 +++++++++++ .../ChatView/__tests__/memoryRecall.test.js | 9 +++ .../src/components/ChatView/memoryRecall.js | 17 ++++- 6 files changed, 177 insertions(+), 36 deletions(-) diff --git a/backend/app/chat.py b/backend/app/chat.py index cec2b3c65..d6be4adc2 100644 --- a/backend/app/chat.py +++ b/backend/app/chat.py @@ -391,8 +391,8 @@ def _log_if_failed(fut, _kind=type(cmd).__name__, _cid=self.chat_id): ack.add_done_callback(_log_if_failed) - def _tool_was_memory_recall(self, tool_use_id) -> bool: - """Whether this tool was identified as a Memory lookup at input time. + def _memory_recall_for_tool(self, tool_use_id) -> dict | None: + """Return the input-time Memory marker for this tool, if there is one. Read-only by design: resolving the block is a question, not the place to adopt a legacy id (`process_event` still owns that a moment later). Only a @@ -404,13 +404,18 @@ def _tool_was_memory_recall(self, tool_use_id) -> bool: continue if tool_use_id: if blk.get("tool_use_id") == tool_use_id: - return isinstance(blk.get("recall"), dict) + recall = blk.get("recall") + return recall if isinstance(recall, dict) else None continue # Legacy events without an id: the newest still-open tool is the only # safe candidate, matching `_tool_block_for_event`'s fallback. if blk.get("status") != "done": - return isinstance(blk.get("recall"), dict) - return False + recall = blk.get("recall") + return recall if isinstance(recall, dict) else None + return None + + def _tool_was_memory_recall(self, tool_use_id) -> bool: + return self._memory_recall_for_tool(tool_use_id) is not None def _stamp_memory_recall(self, event: ChatEvent) -> None: """Name a Memory-app recall on the event, in two lifecycle phases. @@ -423,17 +428,34 @@ def _stamp_memory_recall(self, event: ChatEvent) -> None: if event.get("type") in ("tool_start", "tool_input"): if event.get("type") == "tool_start" and event.get("tool") != "Bash": return + # Both a tool_start AND a tool_input can arrive for one tool call on the + # Claude runner. Stamp the command-derived marker exactly once per block: + # if the block for this tool_use_id already carries a recall marker, leave + # it settled and skip. (Codex has no tool_input; Claude's tool_start input + # is empty, so in practice only one phase produces a marker — this keeps a + # future runner that populates both from double-stamping.) + if self._tool_was_memory_recall(event.get("tool_use_id")): + return recall = recall_from_command(event.get("input")) if recall is not None: event["recall"] = recall return - if (event.get("output_complete") - and self._tool_was_memory_recall(event.get("tool_use_id"))): - event["recall"] = recall_from_result( + pending = self._memory_recall_for_tool(event.get("tool_use_id")) + if event.get("output_complete") and pending is not None: + settled = recall_from_result( event.get("content"), event.get("output_exit_code"), ) - - def _reduce_tool_output(self, event: ChatEvent) -> None: + # The command path is the authoritative installed-app identity. Stamp it + # onto each successful note so deep links keep working when the official + # system app had to install as memory-2 (or another numeric suffix). + app_slug = pending.get("app_slug") + if settled.get("status") == "hit" and isinstance(app_slug, str): + settled["notes"] = [ + {**note, "app_slug": app_slug} for note in settled.get("notes", []) + ] + event["recall"] = settled + + def _reduce_tool_output(self, event: ChatEvent) -> bool: """Move a large tool_output's full text OFF the wire (contract rule 6). This is the single funnel where the live SSE push, the catch-up event_log, @@ -460,11 +482,11 @@ def _reduce_tool_output(self, event: ChatEvent) -> None: content = event.get("content") if (not isinstance(content, str) or len(content) <= TOOL_OUTPUT_INLINE_THRESHOLD): - return + return False if not self.chat_id: # No chat to key a stash by (a detached/synthetic sink — chat_id is always # set on the live path). Can't move the text off-wire safely, so leave it. - return + return False tool_use_id = event.get("tool_use_id") if not tool_use_id: # Unexpected post-card-221: mint a stash id and stamp it on the event so @@ -476,16 +498,21 @@ def _reduce_tool_output(self, event: ChatEvent) -> None: "minted stash id %s", self.chat_id, tool_use_id, ) full = content - excerpt, full_len, exit_code = excerpt_tool_output(full) + excerpt, full_len, parsed_exit_code = excerpt_tool_output(full) event["content"] = excerpt event["output_truncated"] = True event["output_full_len"] = full_len - event["output_exit_code"] = exit_code + # Codex can supply a typed exit code independently of its display text. + # That runner-owned fact outranks best-effort parsing of the excerpt. + typed_exit_code = event.get("output_exit_code") + if not isinstance(typed_exit_code, int) or isinstance(typed_exit_code, bool): + event["output_exit_code"] = parsed_exit_code self._submit_fire_and_forget( StashToolOutput( chat_id=self.chat_id, tool_use_id=tool_use_id, output=full, ) ) + return True def record_lifecycle(self, event: dict) -> None: """Queue private lifecycle metadata without broadcasting it. @@ -550,17 +577,18 @@ def publish(self, event: ChatEvent) -> bool: # and before the broadcast below, so the rewritten event is the single # source feeding the persisted block, the live wire, and the catch-up log. # - # Memory recall is stamped just BEFORE that reduction on the same funnel. - # The app prints its bounded structured result last, so one validated stamp - # reaches the persisted block, live wire, and catch-up log for both runners. - if event_type == "tool_output" and event.get("output_exit_code") is None: - exit_code = tool_output_exit_code(event.get("content")) - if exit_code is not None: - event["output_exit_code"] = exit_code + # Reduce first so a large JSON envelope is parsed only once. The app prints + # its bounded structured Memory result last, so the carved tail still + # contains the line that settles a recognized lookup. + output_reduced = False + if event_type == "tool_output": + output_reduced = self._reduce_tool_output(event) + if not output_reduced and event.get("output_exit_code") is None: + exit_code = tool_output_exit_code(event.get("content")) + if exit_code is not None: + event["output_exit_code"] = exit_code if event_type in ("tool_start", "tool_input", "tool_output"): self._stamp_memory_recall(event) - if event_type == "tool_output": - self._reduce_tool_output(event) if event_type == "thinking": self._prepare_thinking_event(event) diff --git a/backend/app/memory_recall.py b/backend/app/memory_recall.py index c55d16e02..c66041943 100644 --- a/backend/app/memory_recall.py +++ b/backend/app/memory_recall.py @@ -52,7 +52,9 @@ _MAX_COMMAND_SCAN_CHARS = 8192 _ENV_ASSIGN_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") _INTERPRETER_RE = re.compile(r"^(?:.*/)?python[0-9.]*$") -_SCRIPT_RE = re.compile(r"^/data/apps/memory(?:-[0-9]+)?/memory_search\.py$") +_SCRIPT_RE = re.compile( + r"^/data/apps/(?P<app_slug>memory(?:-[0-9]+)?)/memory_search\.py$" +) _CONTROL_TOKEN_RE = re.compile(r"^[;&|()<>]+$") _RESULT_PREFIX = "MOBIUS_MEMORY_RESULT_V1:" @@ -115,22 +117,34 @@ def _simple_command_tokens(command: str) -> list[str] | None: return tokens -def _tokens_run_search(tokens: list[str]) -> bool: +def _tokens_search_slug(tokens: list[str]) -> str | None: + """Return the invoked Memory app slug for the exact documented command. + + Exact arity is a security boundary, not mere tidiness: ``shlex`` treats a + newline as whitespace, so accepting arbitrary trailing tokens would also + accept a second shell command whose output could forge the structured result + line. The supported command is env assignments + Python flags + script + + query + chat id, and then it must end. + """ index = 0 while index < len(tokens) and _ENV_ASSIGN_RE.match(tokens[index]): index += 1 if index >= len(tokens): - return False + return None head = tokens[index] - if _SCRIPT_RE.match(head): - return len(tokens) > index + 2 + direct = _SCRIPT_RE.fullmatch(head) + if direct: + return direct.group("app_slug") if len(tokens) == index + 3 else None if not _INTERPRETER_RE.match(head): - return False + return None for script_index, token in enumerate(tokens[index + 1:], start=index + 1): if token.startswith("-"): continue - return bool(_SCRIPT_RE.match(token) and len(tokens) > script_index + 2) - return False + script = _SCRIPT_RE.fullmatch(token) + if script and len(tokens) == script_index + 3: + return script.group("app_slug") + return None + return None def recall_from_command(command: object) -> dict | None: @@ -150,7 +164,11 @@ def recall_from_command(command: object) -> dict | None: if "memory_search.py" not in command: return None tokens = _simple_command_tokens(command) - return {"status": RECALL_SEARCHING} if tokens and _tokens_run_search(tokens) else None + app_slug = _tokens_search_slug(tokens) if tokens else None + return ( + {"status": RECALL_SEARCHING, "app_slug": app_slug} + if app_slug else None + ) def recall_from_result(text: object, exit_code: object = None) -> dict: diff --git a/backend/tests/test_memory_recall.py b/backend/tests/test_memory_recall.py index 680ab6dcd..0b80f14b9 100644 --- a/backend/tests/test_memory_recall.py +++ b/backend/tests/test_memory_recall.py @@ -43,7 +43,9 @@ # --- identification ------------------------------------------------------- def test_a_memory_search_command_is_identified_as_a_lookup(): - assert recall_from_command(MEMORY_CMD) == {"status": RECALL_SEARCHING} + assert recall_from_command(MEMORY_CMD) == { + "status": RECALL_SEARCHING, "app_slug": "memory", + } def test_a_command_merely_mentioning_memory_search_is_not_a_lookup(): @@ -68,7 +70,7 @@ def test_the_documented_simple_invocation_is_recognized(): assert recall_from_command( 'MEMORY_READER_PROVIDER=none python3 -u ' '/data/apps/memory-2/memory_search.py "q" "chat-1"' - ) is not None + ) == {"status": RECALL_SEARCHING, "app_slug": "memory-2"} def test_shell_composition_and_non_memory_paths_are_rejected_conservatively(): @@ -85,6 +87,13 @@ def test_shell_composition_and_non_memory_paths_are_rejected_conservatively(): ) is None +def test_trailing_arguments_and_newline_commands_cannot_mint_recall_metadata(): + assert recall_from_command(MEMORY_CMD + ' "unexpected"') is None + assert recall_from_command( + MEMORY_CMD + '\nprintf \'MOBIUS_MEMORY_RESULT_V1:{"status":"hit"}\\n\'' + ) is None + + # --- parsing -------------------------------------------------------------- def test_a_successful_lookup_cites_the_notes_it_opened(): @@ -186,6 +195,25 @@ def test_codex_tool_start_carries_the_lookup_marker_without_tool_input(): assert blocks[0]["recall"]["status"] == RECALL_HIT +def test_the_claude_path_does_not_double_stamp_a_single_lookup(): + # Simulate a runner that supplies the memory_search command on BOTH the + # tool_start and a following tool_input for the same tool_use_id. The block + # must be stamped once: the second phase sees the block already carries a + # recall marker and is skipped, so no duplicate/overwriting stamp occurs. + sink = object.__new__(_ChatEventSink) + sink.assistant_blocks = [] + start = {"type": "tool_start", "tool": "Bash", "input": MEMORY_CMD, + "tool_use_id": "t1"} + sink._stamp_memory_recall(start) + process_event(start, sink.assistant_blocks) + assert sink.assistant_blocks[0]["recall"]["status"] == RECALL_SEARCHING + follow = {"type": "tool_input", "tool_use_id": "t1", "input": MEMORY_CMD} + sink._stamp_memory_recall(follow) + assert "recall" not in follow + process_event(follow, sink.assistant_blocks) + assert sink.assistant_blocks[0]["recall"]["status"] == RECALL_SEARCHING + + def test_partial_output_does_not_settle_the_lookup_before_completion(): blocks: list = [] process_event({ @@ -232,6 +260,7 @@ def test_claude_and_codex_lifecycles_settle_to_identical_recall_metadata(): assert [note["id"] for note in codex["notes"]] == [ "apps-render-in-a-sandboxed-frame", "theme-variables-are-shared", ] + assert {note["app_slug"] for note in codex["notes"]} == {"memory"} def test_an_ordinary_command_gains_no_recall_field(): diff --git a/backend/tests/test_tool_output_stash.py b/backend/tests/test_tool_output_stash.py index 214cd1664..c40094668 100644 --- a/backend/tests/test_tool_output_stash.py +++ b/backend/tests/test_tool_output_stash.py @@ -4,6 +4,7 @@ GET /tool-output/{tool_use_id} endpoint serves a bounded expansion preview and the exact text on explicit copy. Also covers the reducer carrying tool identity + truncation metadata onto the persisted block.""" +import json import uuid from sqlalchemy import event as sqlalchemy_event @@ -424,6 +425,49 @@ def test_sink_reduces_large_tagged_output_and_stashes_full(db): assert row is not None and row.output == big +def test_sink_keeps_a_runner_supplied_exit_code_on_large_plain_output(db): + sink = _sink() + sink.publish({ + "type": "tool_start", "tool": "Bash", "input": "run", + "tool_use_id": "tu_typed", + }) + big = "plain output\n" + ("x" * (TOOL_OUTPUT_INLINE_THRESHOLD + 100)) + event = { + "type": "tool_output", "content": big, "tool_use_id": "tu_typed", + "output_exit_code": 7, + } + + sink.publish(event) + + assert event["output_truncated"] is True + assert event["output_exit_code"] == 7 + assert sink.assistant_blocks[-1]["output_exit_code"] == 7 + + +def test_sink_parses_a_large_json_envelope_once(monkeypatch, db): + import app.events as events + + sink = _sink() + big = json.dumps({ + "stdout": "x" * (TOOL_OUTPUT_INLINE_THRESHOLD + 100), + "exit_code": 3, + }) + loads = events.json.loads + calls = 0 + + def counting_loads(value): + nonlocal calls + calls += 1 + return loads(value) + + monkeypatch.setattr(events.json, "loads", counting_loads) + event = {"type": "tool_output", "content": big, "tool_use_id": "tu_json"} + sink.publish(event) + + assert calls == 1 + assert event["output_exit_code"] == 3 + + def test_sink_passes_through_small_output(db): sink = _sink() small = "ok" diff --git a/frontend/src/components/ChatView/__tests__/memoryRecall.test.js b/frontend/src/components/ChatView/__tests__/memoryRecall.test.js index 2d1c0f96c..777840839 100644 --- a/frontend/src/components/ChatView/__tests__/memoryRecall.test.js +++ b/frontend/src/components/ChatView/__tests__/memoryRecall.test.js @@ -6,6 +6,7 @@ import { messageRecall, noteHref, noteLabel, + safeMemoryAppSlug, safeNoteId, } from '../memoryRecall.js' @@ -97,6 +98,14 @@ test('a note links into the Memory app through the shell intent contract', () => ) assert.equal(noteHref({ id: '../evil' }), '', 'an unsafe id yields no link rather than an unsafe one') + assert.equal( + noteHref({ id: 'alpha', app_slug: 'memory-2' }), + '/shell/?app=memory-2&intent=note%3Aalpha', + 'a suffixed official install links to the app that performed the recall', + ) + assert.equal(safeMemoryAppSlug('memory-12'), 'memory-12') + assert.equal(noteHref({ id: 'alpha', app_slug: '../memory' }), '', + 'a present but invalid app slug fails closed') }) test('a note without a title still reads as words, never blank', () => { diff --git a/frontend/src/components/ChatView/memoryRecall.js b/frontend/src/components/ChatView/memoryRecall.js index 26b978a83..72927aa06 100644 --- a/frontend/src/components/ChatView/memoryRecall.js +++ b/frontend/src/components/ChatView/memoryRecall.js @@ -25,6 +25,7 @@ const MAX_RECALL_ROWS_SCANNED = 256 // path, but this value builds a URL that navigates the shell, so re-check here // rather than trusting an upstream call site to stay correct forever. const SAFE_NOTE_ID = /^[A-Za-z0-9][A-Za-z0-9._-]*$/ +const SAFE_MEMORY_APP_SLUG = /^memory(?:-[0-9]+)?$/ export function safeNoteId(value) { if (typeof value !== 'string') return '' @@ -33,13 +34,25 @@ export function safeNoteId(value) { return SAFE_NOTE_ID.test(candidate) ? candidate : '' } +export function safeMemoryAppSlug(value) { + if (typeof value !== 'string') return '' + const candidate = value.trim() + return SAFE_MEMORY_APP_SLUG.test(candidate) ? candidate : '' +} + // Where a pill points: the Memory app, asked to open this note. `?app=<slug>& // intent=<text>` is the shell's existing internal-nav contract (the same one // artifact links use), so this adds no new navigation mechanism. export function noteHref(note) { const id = safeNoteId(note?.id) - return id - ? `/shell/?app=memory&intent=${encodeURIComponent(`note:${id}`)}` + if (!id) return '' + // Old persisted citations predate app_slug and necessarily came from the + // original unsuffixed install. New citations carry the slug validated from + // the command path; a present-but-invalid value fails closed. + const hasAppSlug = typeof note?.app_slug === 'string' + const appSlug = hasAppSlug ? safeMemoryAppSlug(note.app_slug) : 'memory' + return appSlug + ? `/shell/?app=${appSlug}&intent=${encodeURIComponent(`note:${id}`)}` : '' } From 544126cbe84324642e98df17b36ba758210ecabd Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 02:37:24 +0000 Subject: [PATCH 12/15] fix(codex): retain warning-level stop forensics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/codex_sdk_runner.py | 9 +++++---- backend/tests/test_codex_sdk_runner.py | 8 +++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/backend/app/codex_sdk_runner.py b/backend/app/codex_sdk_runner.py index c78a5b9c6..0a3840efe 100644 --- a/backend/app/codex_sdk_runner.py +++ b/backend/app/codex_sdk_runner.py @@ -2385,10 +2385,11 @@ def aborted_result() -> RunnerResult: # the new event omits — taking the note's one-tap Resume with it and # leaving the owner an unexplained error and no way back. # - # Expected, so INFO — but the transport's dying words are the only - # forensics a kill leaves behind, and nothing downstream logs them - # once `error` is None. - log.info( + # Usually expected after escalation, but WARNING is deliberate: a real + # app-server crash can coincide with a requested stop and has the same + # transport shape. The dying words are the only forensic evidence left + # once the owner-facing error is suppressed. + log.warning( "Codex transport closed by our own stop chat_id=%s: %s", chat_id, exc, ) return with_usage({ diff --git a/backend/tests/test_codex_sdk_runner.py b/backend/tests/test_codex_sdk_runner.py index a1337659b..deb412874 100644 --- a/backend/tests/test_codex_sdk_runner.py +++ b/backend/tests/test_codex_sdk_runner.py @@ -1426,7 +1426,7 @@ def _mark_interrupted(handle): def test_run_codex_sdk_turn_reports_self_requested_kill_as_interrupted( - monkeypatch, + monkeypatch, caplog, ): """A stop we asked for must not read as a provider failure. @@ -1447,6 +1447,12 @@ def test_run_codex_sdk_turn_reports_self_requested_kill_as_interrupted( assert result["terminal_status"] == "interrupted" assert [e for e in bc.events if e.get("type") == "error"] == [] assert registry.get_handle("chat-1", RunnerKind.CODEX_SDK) is None + assert any( + record.levelname == "WARNING" + and "Codex transport closed by our own stop" in record.message + and "closed stdout" in record.message + for record in caplog.records + ) def test_run_codex_sdk_turn_self_requested_kill_still_reports_usage( From b7e4e4b203f652e430aa32672a74d370f3377b24 Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:35:51 +0000 Subject: [PATCH 13/15] test(chat): keep the steer cut when the writer dedupes its row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/tests/test_chats_stream_steer.py | 44 ++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/backend/tests/test_chats_stream_steer.py b/backend/tests/test_chats_stream_steer.py index 92b53f63f..feae3e9be 100644 --- a/backend/tests/test_chats_stream_steer.py +++ b/backend/tests/test_chats_stream_steer.py @@ -611,6 +611,50 @@ async def split_for_steer(self, rows, consume): asyncio.run(_run()) +def test_writer_dedup_still_publishes_the_committed_cut(): + """An empty stored-row echo does not undo the split that just committed. + + `split_for_steer` seals A1 and resets the sink BEFORE the writer appends the + steered row. `stored_messages: []` means only that cid dedup found the row + already in the durable transcript; it does not mean the A1/A2 boundary was + skipped. Suppressing the cut here would leave the client appending A2 to the + segment the server has already sealed. The handed row still supplies the cid + that retires its tray entry and identifies the already-durable user turn. + """ + from app.broadcast import create_broadcast + from app.claude_sdk_runner import _seal_steer_split + + chat_id = "sealdedup" + handle = _make_active_claude_client(chat_id) + turn_bc = create_broadcast(chat_id) + + async def _run(): + row = {"role": "user", "content": "Q2", "ts": 10, "cid": "c-q2"} + handle._steer_user_msgs = [row] + handle._steer_consume_cids = ["c-q2"] + + class _SinkLike: + def __init__(self, bc): + self.bc = bc + + async def split_for_steer(self, rows, consume): + assert rows == [row] + assert consume == ["c-q2"] + # The durable writer already has this cid, but the sink-side split still + # sealed A1 and reset its accumulator for A2. + return {"stored_messages": []} + + await _seal_steer_split(_SinkLike(turn_bc), handle, chat_id) + + cuts = [e for e in turn_bc.event_log if e.get("type") == "steered_into_turn"] + assert len(cuts) == 1 + assert cuts[0]["messages"][0]["cid"] == "c-q2" + assert handle._steer_user_msgs == [] + assert handle._steer_consume_cids == [] + + asyncio.run(_run()) + + def test_a_failing_publisher_cannot_escape_the_seal(): """Announcing the cut must never raise out of `_seal_steer_split`. From ece06a940814208c8cd4064e65ec88ba3317964e Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:35:27 +0000 Subject: [PATCH 14/15] fix(update): finish reconcile merge non-interactively; classify a wedged merge as error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/platform_update.py | 40 +++++++++++++++++++++------ backend/tests/test_platform_update.py | 39 ++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/backend/app/platform_update.py b/backend/app/platform_update.py index 4ea194f1a..bacad3bc0 100644 --- a/backend/app/platform_update.py +++ b/backend/app/platform_update.py @@ -118,6 +118,11 @@ # this is wedged, not busy. Fetch gets its own (network-bound) budget. _GIT_TIMEOUT = 120 _FETCH_TIMEOUT = 120 +# Distinct sentinel returned by ``_merge_target`` when the merge wedged and was +# aborted (as opposed to a positive git returncode from a content conflict). +# After the abort no unmerged paths survive, so the caller must treat this as an +# error/serve-old outcome rather than fabricating a zero-path content conflict. +_MERGE_TIMEOUT = -1 # The post-merge import probe. A module-level infinite loop or a blocking call # in agent-edited code would otherwise wedge boot forever; a timeout-kill counts # as probe-fail -> roll back. @@ -596,8 +601,12 @@ def _merge_target(repo: Path, target: str) -> int: resolver discover conflicts one historical commit at a time. ``--no-ff`` is deliberate: this helper is called only for diverged histories, and the merge commit preserves both the reviewed upstream target and the complete local - history as explicit parents. Returns 0 on a clean committed merge and nonzero - on conflict/error; the caller owns abort + serve-old recovery. + history as explicit parents. Returns 0 on a clean committed merge and a + positive returncode on a content conflict/error; returns the distinct + sentinel :data:`_MERGE_TIMEOUT` when the merge wedged and was aborted (no + unmerged paths survive the abort, so the caller must classify it as an error/ + serve-old outcome rather than a content conflict). The caller owns abort + + serve-old recovery. """ env = _scrubbed_git_env(repo) try: @@ -614,9 +623,10 @@ def _merge_target(repo: Path, target: str) -> int: return proc.returncode except subprocess.TimeoutExpired: # A wedged merge must not leave a half-merged tree: abort so the caller's - # serve-old path is honoured. + # serve-old path is honoured. Return a distinct sentinel so the caller does + # not read the (now empty) unmerged paths and fabricate a zero-path conflict. _git("merge", "--abort", repo=repo, check=False) - return 1 + return _MERGE_TIMEOUT def _reset_hard_to(repo: Path, local: str, sha: str) -> None: @@ -1251,12 +1261,25 @@ def reconcile_clone( # net conflict instead of stopping once per historical local commit. rc = _merge_target(repo, target) if rc != 0: - # Conflict: NEVER leave a half-merged tree. Abort back to PRE (the old, - # working code keeps serving), record the conflict, clear any stale - # rollback flag, and let the caller open a resolver chat. + # NEVER leave a half-merged tree. Read the unmerged paths BEFORE the + # abort clears them. paths = _unmerged_paths(repo) _git("merge", "--abort", repo=repo, check=False) _reset_hard_to(repo, local, pre) # belt-and-braces: ensure main == PRE + # A wedged merge (timeout sentinel) — or, defensively, ANY nonzero + # result that produced no unmerged paths — is NOT a reviewable content + # conflict: ``_merge_target`` already aborted it, so there is nothing for + # a resolver chat to reconcile. Classify it as an error/serve-old outcome + # (reset to PRE, no conflict flag, no resolver chat) rather than + # fabricating a zero-path conflict. + if rc == _MERGE_TIMEOUT or not paths: + CONFLICT_FLAG.unlink(missing_ok=True) + ROLLED_BACK_FLAG.unlink(missing_ok=True) + _clear_reconcile_pre() + err = "merge_timeout" if rc == _MERGE_TIMEOUT else "merge_failed" + return ReconcileResult("error", pre, pre, target, error=err) + # Content conflict: record it, clear any stale rollback flag, and let the + # caller open a resolver chat. _write_conflict_flag(target, paths) ROLLED_BACK_FLAG.unlink(missing_ok=True) _clear_reconcile_pre() @@ -1834,7 +1857,8 @@ def _platform_conflict_resolver_message( f"{target_sha}` compares the complete local and reviewed upstream trees " "once and stops with every conflicting file marked; combine the intent of " "the local version and upstream's, save each file, then `git add` it and " - "`git merge --continue`. When the merge finishes, the working branch " + "`git commit --no-edit` (this finishes the merge non-interactively from the " + "prepared merge message). When the merge finishes, the working branch " "carries both histories.\n\n" "When the reconcile is committed, clear the flag " "(`rm -f /data/.platform-conflict`) and tell the owner to **restart the " diff --git a/backend/tests/test_platform_update.py b/backend/tests/test_platform_update.py index f52cc30b2..81e34d61d 100644 --- a/backend/tests/test_platform_update.py +++ b/backend/tests/test_platform_update.py @@ -291,6 +291,41 @@ def test_diverged_update_surfaces_all_net_conflicts_together(clone_env): assert not pu._reconcile_in_progress(platform) +@pytest.mark.parametrize( + ("merge_rc", "expected_error"), + [ + (pu._MERGE_TIMEOUT, "merge_timeout"), + (2, "merge_failed"), + ], +) +def test_non_conflict_merge_failure_serves_old_without_a_resolver_flag( + clone_env, monkeypatch, merge_rc, expected_error, +): + origin, platform = clone_env + pre = _local_commit(platform, edits={"backend/app/main.py": + _MAIN_PY.replace("LINE_A = 1", "LINE_A = 'LOCAL'")}) + target = _advance_origin(origin, edits={"backend/app/main.py": + _MAIN_PY.replace("LINE_C = 3", "LINE_C = 'UPSTREAM'")}) + # A previous attempt may have left either durable review state behind. A + # timeout or git failure with no unmerged paths has nothing a resolver can + # act on, so returning "error" while preserving one of these flags would make + # the next status read lie about what just happened. + pu._write_conflict_flag("b" * 40, ["backend/app/old.py"]) + pu._write_rolled_back_flag("c" * 40, "old import failure") + monkeypatch.setattr(pu, "_merge_target", lambda _repo, _target: merge_rc) + + res = pu.reconcile_clone(platform, at_boot=True) + + assert res.status == "error" + assert res.error == expected_error + assert res.target_sha == target + assert _served_sha(platform) == pre + assert not pu._reconcile_in_progress(platform) + assert not pu.CONFLICT_FLAG.exists() + assert not pu.ROLLED_BACK_FLAG.exists() + assert not pu.RECONCILE_PRE_FLAG.exists() + + # --- V-B4: import-broken text-clean merge -> rollback ----------------------- def test_import_broken_merge_rolls_back(clone_env): @@ -944,6 +979,10 @@ def test_platform_conflict_resolver_message_pins_reviewed_target(): assert f"merge --no-ff {target}" in content assert "merge --no-ff origin/main" not in content assert "backend/app/main.py, frontend/src/App.jsx" in content + # The resolver runs in a headless shell where `git merge --continue` opens an + # editor and hangs/errors; it must finish non-interactively instead. + assert "commit --no-edit" in content + assert "merge --continue" not in content def test_status_restart_needed_when_disk_head_changed_after_boot(clone_env): From bf7a9994f6dab825d8ac016cea38af5ff9509f2d Mon Sep 17 00:00:00 2001 From: hamzamerzic <10846014+hamzamerzic@users.noreply.github.com> Date: Mon, 27 Jul 2026 02:37:25 +0000 Subject: [PATCH 15/15] fix(memory): preserve citation node identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Möbius Agent <mobius-agent@users.noreply.github.com> --- backend/app/events.py | 3 +- backend/app/memory_recall.py | 22 ++++++++++++++- backend/tests/test_memory_recall.py | 28 +++++++++++++++++++ .../src/components/ChatView/streamReducers.js | 3 +- 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/backend/app/events.py b/backend/app/events.py index c789b45cc..74900765f 100644 --- a/backend/app/events.py +++ b/backend/app/events.py @@ -647,7 +647,8 @@ def _apply_tool_input(blk: dict) -> None: blk["output_truncated"] = True blk["output_full_len"] = event.get("output_full_len") # Settle a Memory lookup from "searching" to what it actually recalled. - # The sink parsed this from the FULL output, before the carving above. + # The app prints its bounded result last, so it survives any head+tail + # carving the sink performed before parsing. if isinstance(event.get("recall"), dict): blk["recall"] = event["recall"] return True diff --git a/backend/app/memory_recall.py b/backend/app/memory_recall.py index c66041943..62f450ad4 100644 --- a/backend/app/memory_recall.py +++ b/backend/app/memory_recall.py @@ -67,6 +67,8 @@ # anything else keeps traversal, absolute paths, and control characters out of # a value the client turns into a deep link. _PATH_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]*\.md$") +_NOTE_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") +_MAX_NOTE_ID_CHARS = 128 def _clean(value: str, limit: int) -> str: @@ -84,6 +86,24 @@ def _note_id(path: str) -> str: return tail[:-3] if tail.endswith(".md") else tail +def _safe_note_id(value: object, path: str) -> str: + """Keep the graph's real node id, with a path-stem fallback for old apps. + + A graph id is not required to equal its markdown filename. The Memory app + opens nodes by id, so replacing a valid structured id with the path stem + makes a well-formed citation navigate to nowhere whenever those differ. + """ + if isinstance(value, str): + candidate = value.strip() + if ( + candidate + and len(candidate) <= _MAX_NOTE_ID_CHARS + and _NOTE_ID_RE.fullmatch(candidate) + ): + return candidate + return _note_id(path) + + def _title_from_path(path: str) -> str: """A readable fallback when the titled section line was carved away.""" return _note_id(path).replace("-", " ").replace("_", " ").strip() @@ -211,7 +231,7 @@ def recall_from_result(text: object, exit_code: object = None) -> dict: title = _clean(raw_note.get("title"), MAX_RECALL_TITLE_CHARS) excerpt = _clean(raw_note.get("excerpt"), MAX_RECALL_EXCERPT_CHARS) note = { - "id": _note_id(path), + "id": _safe_note_id(raw_note.get("id"), path), "path": path, "title": title or _title_from_path(path) or path, } diff --git a/backend/tests/test_memory_recall.py b/backend/tests/test_memory_recall.py index 0b80f14b9..18444ac5d 100644 --- a/backend/tests/test_memory_recall.py +++ b/backend/tests/test_memory_recall.py @@ -106,6 +106,34 @@ def test_a_successful_lookup_cites_the_notes_it_opened(): assert recall["notes"][0]["excerpt"] == "Each mini-app runs isolated." +def test_a_citation_keeps_the_graph_node_id_when_it_differs_from_the_file(): + recall = recall_from_result( + 'MOBIUS_MEMORY_RESULT_V1:{"status":"hit","notes":[' + '{"id":"canonical-node","path":"notes/readable-filename.md",' + '"title":"Canonical node"}]}', + 0, + ) + + assert recall["notes"] == [{ + "id": "canonical-node", + "path": "notes/readable-filename.md", + "title": "Canonical node", + }] + + +def test_an_unsafe_or_missing_graph_node_id_falls_back_to_the_file_stem(): + recall = recall_from_result( + 'MOBIUS_MEMORY_RESULT_V1:{"status":"hit","notes":[' + '{"id":"../escape","path":"notes/safe-fallback.md"},' + '{"path":"notes/legacy-note.md"}]}', + 0, + ) + + assert [note["id"] for note in recall["notes"]] == [ + "safe-fallback", "legacy-note", + ] + + def test_a_lookup_that_found_nothing_says_so(): assert recall_from_result(EMPTY_OUTPUT, 0) == {"status": RECALL_EMPTY} diff --git a/frontend/src/components/ChatView/streamReducers.js b/frontend/src/components/ChatView/streamReducers.js index 232543810..153755185 100644 --- a/frontend/src/components/ChatView/streamReducers.js +++ b/frontend/src/components/ChatView/streamReducers.js @@ -351,7 +351,8 @@ export function attachToolOutput(prev, content, event = null) { block.tool_use_id = event.tool_use_id } // Settle a Memory lookup from "searching" to the notes it actually returned. - // The backend parsed these from the full output, before the reduction below. + // The backend stamps this from the bounded structured tail that survives any + // large-output carving. if (event?.recall) { block.recall = event.recall }