Conversation
236e527 to
888db2b
Compare
|
Reading the full diff at One thing I'd flag before merge — a hot-path perf regression in the redaction helper. Code reference
def _redact_snapshot(snapshot: dict) -> dict:
from typing import cast
from api.helpers import _redact_value
return cast(dict, _redact_value(snapshot))
def _redact_text(text, *, _enabled=None):
if _enabled is None:
from api.config import load_settings
_enabled = bool(load_settings().get("api_redact_enabled", True))So every string in the snapshot triggers its own Why it matters here
RecommendationThread the setting once, mirroring def _redact_snapshot(snapshot: dict) -> dict:
from typing import cast
from api.helpers import _redact_value
from api.config import load_settings
_enabled = bool(load_settings().get("api_redact_enabled", True))
return cast(dict, _redact_value(snapshot, _enabled=_enabled))That's one read per emission instead of one per string, and it keeps the fail-closed behavior intact (a raising Test note
Everything else looks solid — CI is green across the 3.11/3.12/3.13 matrix and the cross-session |
888db2b to
6de7667
Compare
|
Re-read
from api.config import load_settings
from api.helpers import _redact_value
_enabled = bool(load_settings().get("api_redact_enabled", True))
return cast(dict, _redact_value(snapshot, _enabled=_enabled))That's one The new No further concerns from me on the redaction path. LGTM. |
6de7667 to
8b24ade
Compare
|
| Filename | Overview |
|---|---|
| api/models.py | Adds two defensive ValueError guards in all_sessions() after the session live-filter; the post-filter guard can fire on a legitimate cleanup-lag state and break the sessions list endpoint. |
| api/streaming.py | Wires emit_todo_state() into both tool-callback shapes (legacy preview fallback and modern result path); error-swallow policy keeps tool delivery unaffected. |
| api/todo_state.py | Major expansion: adds emit_todo_state, _redact_snapshot, and moves derive_todo_state; logic is solid but _redact_snapshot reads load_settings() on every SSE emission. |
| static/messages.js | Adds todo_state SSE listener with cross-session and timestamp guards; correctly persists to INFLIGHT. Three session-refresh handlers call _hydrateTodosFromSession but omit scheduleTodosRefresh(), leaving the panel potentially stale after compression events. |
| static/panels.js | Refactors loadTodos() to read from S.todos/S.todoStateMeta; adds hash-based short-circuit and RAF coalescing; legacy fallback preserved for old servers. |
| static/sessions.js | Plumbs _hydrateTodosFromSession into loadSession/newSession/delete paths and restores todos from INFLIGHT; newSession omits scheduleTodosRefresh(), leaving the Todos panel stale after session creation if the panel is open. |
| static/ui.js | Adds todos/todoStateMeta to global S state, implements _hydrateTodosFromSession with cold-vs-INFLIGHT timestamp reconciliation, and adds scheduleTodosRefresh RAF coalescing; logic is well-thought-out. |
| tests/test_todo_state_emission.py | New tests cover emit_todo_state happy path, invalid/non-todo payloads, redaction integration, and settings-call count. |
| tests/test_streaming_todo_state_static.py | Static AST-level tests verify that streaming.py imports emit_todo_state, calls it in both callback shapes, and prefers full result over preview. |
| tests/test_todo_live_frontend_static.py | Thorough static and node-executed tests for the frontend SSE listener: validates session filtering, timestamp ordering, INFLIGHT mirroring, and malformed-payload handling. |
| tests/test_todo_panel_cold_load_static.py | Updates existing cold-load static tests to match the new _hydrateTodosFromSession/scheduleTodosRefresh API and S.todoStateMeta-based render path. |
Sequence Diagram
sequenceDiagram
participant Agent
participant streaming as streaming.py
participant todo_state as todo_state.py
participant SSE
participant messages as messages.js
participant ui as ui.js
participant panels as panels.js
Agent->>streaming: todo tool completes
streaming->>todo_state: emit_todo_state(put, name, result, session_id)
todo_state->>todo_state: parse_todo_tool_result()
todo_state->>todo_state: _redact_snapshot()
todo_state->>SSE: put todo_state event
SSE->>messages: addEventListener todo_state fires
messages->>messages: filter by session_id and ts guard
messages->>ui: S.todos and S.todoStateMeta updated
messages->>messages: INFLIGHT persisted
messages->>ui: scheduleTodosRefresh()
ui->>panels: loadTodos() via RAF
Note over messages,ui: On session load or reload
messages->>ui: _hydrateTodosFromSession(session)
ui->>ui: reconcile cold-load vs INFLIGHT by timestamp
ui->>ui: _resetTodosRenderCache()
Comments Outside Diff (2)
-
static/sessions.js, line 650-652 (link)Todos panel not refreshed after
newSession()hydration_hydrateTodosFromSession(S.session)correctly clearsS.todosandS.todoStateMetafor a fresh session, butscheduleTodosRefresh()is never called afterwards.renderMessages()on line 669 re-renders the message thread but does not invokeloadTodos(). If the Todos panel is open while switching to a new session, it will continue to show the previous session's todo list until the user manually triggers a refresh. Compare with_ensureMessagesLoaded(), which explicitly follows_hydrateTodosFromSessionwithscheduleTodosRefresh()— the same pattern is needed here.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
api/todo_state.py, line 362-368 (link)load_settings()called per SSE emission in_redact_snapshot()_redact_snapshot()re-reads settings viaload_settings()on every call — once pertodo_stateSSE event. For an agent invokingtodofrequently, this reads and parses the settings file repeatedly in the hot streaming path. Consider caching the_enabledflag at the call site inemit_todo_stateand passing it as an argument, so settings reads are bounded per-turn rather than per-tool-call.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix(todos): stream live state updates wi..." | Re-trigger Greptile
| @@ -2839,6 +2849,8 @@ def all_sessions(diag=None): | |||
| ) | |||
| ) | |||
| ] | |||
| if not index and _session_dir_has_persisted_session_files(): | |||
| raise ValueError("session index has no live rows while session files exist") | |||
There was a problem hiding this comment.
ValueError can fire in post-cleanup races, breaking the sessions API
_session_dir_has_persisted_session_files() scans SESSION_DIR for any non-underscore-prefixed .json file, while the filter retains only index entries whose session_id appears in in_memory_ids or persisted_ids. These two predicates are not symmetric: a file for session C can make the helper return True while the index contains only stale entries for sessions A and B (just purged from both stores). After the filter removes A and B, the second guard fires and raises ValueError("session index has no live rows while session files exist") — even though the state is entirely consistent (A/B are legitimately gone; C is simply not yet indexed). This ValueError would surface as a 500 on the sessions list endpoint rather than returning an empty list or triggering the index-rebuild fallback below this block.
| // Phase 2: dedicated `todo_state` event carries a full snapshot of | ||
| // the upstream TodoStore. We treat it as the single source of truth |
There was a problem hiding this comment.
_hydrateTodosFromSession calls in stream session-refresh paths lack scheduleTodosRefresh()
The three stream settle / session-refresh handlers added in messages.js (around lines 2136, 2566, and 2664) all call _hydrateTodosFromSession(S.session) but none follow with scheduleTodosRefresh(). If the reconciled snapshot differs from what the panel last rendered (e.g., after a context-compression refresh), the Todos panel stays stale for the remainder of the turn if no further todo calls happen. _ensureMessagesLoaded() sets the expected pattern — all _hydrateTodosFromSession call sites outside the SSE todo_state listener should schedule a refresh.
## Release v0.51.261 — Release IC (stage-r11) Live Todos panel via an explicit `todo_state` SSE contract. ### Fixed | Issue/PR | Author | Fix | |----------|--------|-----| | #3373 follow-up (#3454) | @v2psv | The Todos side panel now tracks `todo` tool state **live during an active run** instead of staying stale until settle / rolling back on a mid-stream reload. A dedicated `todo_state` SSE event sends a full, redacted, idempotent snapshot on todo-tool completion (no more truncated `tool_complete.preview`); the same `api.todo_state` parser feeds live + cold-load; live snapshots persist into INFLIGHT so reload/reattach restores the panel; cold-load vs INFLIGHT reconciled by timestamp (incl. the `coldTs===0` compressed-session edge); legacy reverse-scan kept as fallback for old servers. | ### Gate - Full pytest suite: **7692 passed, 0 failed** - ESLint: CLEAN · ruff: CLEAN · browser-smoke: CLEAN - Codex (regression): **SAFE TO SHIP** — verified the new `todo_state` SSE handler composes with existing dispatch (no double-subscribe), INFLIGHT persistence is cleared on terminal/cancel (composes with discard_session + turn-journal), timestamp reconciliation can't let a stale local snapshot win, redaction holds, the legacy reverse-scan fallback still works with no double-render, and the `models.py` change is todo-scoped (no CLI-classification interaction). Co-authored-by: v2psv <v2psv@users.noreply.github.com>
|
Shipped in v0.51.261 (Release IC) — thank you @v2psv! 🙏 Live Todos panel via the new |
## Release v0.51.261 — Release IC (stage-r11) Live Todos panel via an explicit `todo_state` SSE contract. ### Fixed | Issue/PR | Author | Fix | |----------|--------|-----| | nesquena#3373 follow-up (nesquena#3454) | @v2psv | The Todos side panel now tracks `todo` tool state **live during an active run** instead of staying stale until settle / rolling back on a mid-stream reload. A dedicated `todo_state` SSE event sends a full, redacted, idempotent snapshot on todo-tool completion (no more truncated `tool_complete.preview`); the same `api.todo_state` parser feeds live + cold-load; live snapshots persist into INFLIGHT so reload/reattach restores the panel; cold-load vs INFLIGHT reconciled by timestamp (incl. the `coldTs===0` compressed-session edge); legacy reverse-scan kept as fallback for old servers. | ### Gate - Full pytest suite: **7692 passed, 0 failed** - ESLint: CLEAN · ruff: CLEAN · browser-smoke: CLEAN - Codex (regression): **SAFE TO SHIP** — verified the new `todo_state` SSE handler composes with existing dispatch (no double-subscribe), INFLIGHT persistence is cleared on terminal/cancel (composes with discard_session + turn-journal), timestamp reconciliation can't let a stale local snapshot win, redaction holds, the legacy reverse-scan fallback still works with no double-render, and the `models.py` change is todo-scoped (no CLI-classification interaction). Co-authored-by: v2psv <v2psv@users.noreply.github.com>
Bug Description
Follow-up to the cold-load todo state work from #3373.
The Todos panel can now hydrate correctly after a page refresh, but it still does not consistently track the current
todotool state while an agent run is active.Before this PR:
todotool calls updated the agent-side TodoStore, but the side panel usually stayed stale until the run settled.Root Cause
The panel historically derived todo state by reverse-scanning settled
toolmessages inS.messages.That works after a completed turn, but not during a live run:
toolmessages appear inS.messages;tool_complete.previewis truncated and not a durable structured state contract;Fix
This PR adds an explicit todo-state contract across server, SSE, and frontend state:
todo_stateSSE event when thetodotool completes.tool_complete.preview.api.todo_stateparser / normalizer for live emission and cold-load state.S.todos+S.todoStateMetaas the Todos panel source of truth.coldTs === 0edge case for compressed sessions.session.todo_statesidecars when a freshmessages=1response does not include one.How to Verify
Manual verification path:
todotool multiple times.session.todo_state.Test Plan
Ran locally after rebasing onto
origin/master:Result:
Also ran:
Result:
Additional local hygiene checks:
No whitespace errors or conflict markers found.
Risk Assessment
Low / Medium.
The change is scoped to Todos panel state propagation and recovery. The new SSE event is additive, and the frontend keeps the legacy message-scan fallback for compatibility with older servers.
Risk is mainly in state reconciliation:
out-of-order SSE replay is guarded by timestamp checks;
cross-session events are filtered by session_id;
malformed or non-todo payloads are ignored;
live SSE snapshots are redacted before emission;
cold-load snapshots continue to pass through session response redaction.
The PR intentionally keeps the detector symmetric with the existing agent TodoStore hydration logic so the WebUI panel does not disagree with the agent-side state.