Repository navigation
feat(agent): annotate the conversation tail into the episodic log - #924
Conversation
A vendor can accept a tool list and still reject being told it must call one. DeepSeek's thinking mode answers a 400 with "Thinking mode does not support this tool_choice", and the Responses adapter returned that as the call's result, so every caller that forces a choice got an error response with no tool calls in it. Nine call sites force one today: two in the memory consolidator, three in the proactive layer, four in the playbook compiler. Each already handles a model that declines under auto, because that is what auto has always meant. So the adapter now does what its Anthropic sibling already did: when a 400 names tool_choice and the body forces one, relax it to auto and retry once. Measured against the live vendor, three ways: required answered 400, auto and omitted both produced the tool call. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The episodic log had no writer on any surface people use. Its only one, the annotator, runs inside the token-pressure compaction loop, and all three calls into that loop are gated on owns_compaction, which the single shipped context engine hardcodes to true. The annotator survived with one caller: /new typed as message text in a chat channel, which the web and TUI new-session buttons never send. Six readers read the empty file. After a user turn, once memory.annotateEveryTurns user turns have accumulated, the unannotated tail now goes to the same annotator, in the background, under the consolidator's per-session lock so it cannot interleave with /new. Its own cursor lives in the session's metadata and is patched rather than saved, because a task that lands mid-turn must not drop keys the turn wrote. A session is seeded at the length before its first seen turn, so an upgraded install or a fork is never back-filled into one call. Truncation moves the cursor where it happens: clear drops it, undo pulls it back to the cut, beside the same methods' handling of last_consolidated. Only Origin.USER turns count, so the proactive layer is not fed its own output. With a backend wired the prompt names the log again, read-only. That line was removed with the profile path when a backend owns memory; the log is the host's to write either way, and an agent that cannot see it cannot grep it. Set memory.annotateEveryTurns to 0 to turn the step off. Verified on a live gateway with an isolated home: a 12-message session seeded at 12 and wrote nothing; three substantive turns produced three episode lines that the readers' own parse_episode_line accepts, tagged the way project_rhythm ranks by; two turns produced none; the knob at 0 produced none and did not even seed; undo pulled the cursor back to the cut; the cursor survived a restart. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
ruff's N801 fails the lint job on the underscored class name. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: serialize truncation with in-flight annotation before advancing the episodic cursor.
I reviewed the full github/main...HEAD change, the turn and session callers, persistence/consolidation history, AGENTS.md and CONTEXT.md constraints, backward compatibility, test changes, and the relevant architecture boundaries. One concrete cursor race is marked inline.
Verification:
uv run pytest -o addopts='' tests/test_agent_loop_episodic_annotate.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py tests/test_providers_forced_tool_choice.py -q- 252 passed.- Latest-head changed tests in a detached worktree - 23 passed.
- Full
uv run pytest -q- 29,323 passed, 165 skipped, 1 xfailed, 15 failed, 2 collection errors. The failures are outside this diff and include missing optionaldatrie/nltkdependencies plus the existing Rich theme assertion.
The path it named is gitignored, so the reference resolved to nothing for anyone but its author. The contract above it already says what the proxy must do. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Every acceptance run for this step drove a gateway with no backend, while the shipped default wires one. The step reads no backend, so the claim was structural and untested; a later gate on one would have stopped the log on the configuration almost everyone runs, silently. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…ed it Review found three ways the step could swallow a reader's turns, all of them downstream of the same thing: the cursor is written after an await the session can change under. A truncation during the annotator's call. undo and clear are allowed once the foreground turn ends, while the detached task is still waiting, and both move the cursor themselves. The task then wrote arithmetic computed against indices that no longer meant anything, landing inside the turns that replaced the undone ones, which were never annotated. The cursor is now revalidated before it is patched: changed by someone else, the task leaves it alone. clear dropped the cursor instead of stating it. SessionManager's own removal rule says a key a copy merely omits comes back on the next save, and the patch the step writes leaves the file stamp stale, which forces exactly the re-read that resurrects it. A cleared session carried its old cursor and swallowed every turn until the count climbed past it. It now writes zero, as the other removers in that file do. The tail could split a turn and could carry machine turns. It is sliced to the length read when the turn ended, so a task that waited on the lock does not reach into the turn that overtook it; and messages carrying an origin mark are dropped, because two of this file's readers are Sentinel producers and the trigger's origin check only gates whose turn starts the step, not whose messages are in it. Also: the forced-tool-choice relaxation now fires only on a 400, as its Anthropic sibling does. A gateway that echoes the request body into a 5xx would otherwise drop the forced call on a transient fault. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Pins the narrowing in the commit before this one: only a 400 relaxes a forced tool choice. A gateway that puts the request body into a 5xx would otherwise look like a refusal, and the caller would read the missing tool call as the model declining. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
e3c97fb to
26af90e
Compare
|
One thing worth stating rather than leaving for a reviewer to ask: the live-gateway evidence in the description was produced before the five review fixes, so it ran against the earlier tail computation. It still holds, and here is why precisely rather than in general. The tail changed from
So the tail handed to the annotator on that run is byte for byte what the current code would hand it, and the three lines it wrote are evidence for the code as it stands. What the live run does not cover, and what the unit twins carry instead: the truncation-during-the-await case, the lock-contention boundary, and a Sentinel turn inside a reader's session. Each of those has a test that was confirmed red with its guard removed -- including two that were rewritten after the first attempt turned out not to be able to fail. |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: make truncation detection independent of whether the cursor value changes.
I reviewed the e3c97fbcc231...26af90e40826 delta against the full change, including the turn/session callers, persistence and consolidator history, AGENTS.md and CONTEXT.md constraints, backward compatibility, architecture boundaries, and whether tests were weakened. The existing cursor-race thread remains open: the new equality guard fixes a changed-cursor truncation, but the first batch and clear can truncate while leaving the cursor at 0, allowing the same stale write-back. The exact reproduction and missing test case are in the thread reply. I found no separate new finding.
Verification: uv run pytest -o addopts='' tests/test_agent_loop_episodic_annotate.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py tests/test_providers_forced_tool_choice.py -q - 257 passed. A direct unchanged-cursor reproduction still ended with eight messages, cursor 6, and only the original annotation call.
…wrote Both write episodes.md now, and only one of them knew it. The /new path slices from last_consolidated, which moves only inside the token-pressure loop that the shipped context engine owns and so never runs: it is zero on every install. Sliced from that alone, /new handed the annotator the whole session, so every episode the after-turn step had already written was written a second time and every tag counted twice toward the threshold that rewrites the profile section. It starts from whichever cursor ran last. Before this change there was no second writer and nothing to collide with, so this is the interaction the step introduces rather than a defect it uncovers. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: make truncation detection independent of whether the cursor value changes.
I reviewed the 26af90e40826...099652eea9fb delta and its interaction with the full change, callers, persistence history, compatibility, tests, and the documented architecture constraints. The new /new boundary correctly starts from the later consolidation cursor and adds a valid regression test; I found no new issue in that delta. The existing thread still blocks this revision because the unchanged-cursor reproduction is unchanged.
At this fourth review round, the standing blocker meets all three convergence conditions: this change introduces the detached cursor writer; an ordinary /undo or clear immediately after a cadence turn can race the background annotation; and once the stale cursor silently skips replacement turns, neither later cadence runs nor /new backfills them, leaving no normal recovery path.
Verification on 099652eea9fb: focused suite - 258 passed in 17.32s. Direct reproduction - eight messages, cursor 6, one annotation call, confirming the replacement turns remain skipped. The existing thread remains open.
The guard added for the cursor race compared the cursor's value before and after the annotator's await, which misses the case the cursor is blind to. undo_last_turn only pulls a cursor back when the cut is below it, so a session annotated from zero is truncated with its cursor left at zero, and clear now writes zero explicitly. Turns arriving after the cut take the length back past the stale write-back, which then lands inside them: they are behind the cursor and were never annotated. Session counts truncations instead. clear and undo_last_turn each bump it, and the annotation task compares the count rather than the value. The counter is in process only, which is the whole of its life: a detached task does not outlive the process that started it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the 099652eea9fb...8e652948243d delta against the full change, including the turn/session callers, persistence and consolidator interactions, project rules and terminology, backward compatibility, architecture boundaries, and test coverage. The process-local truncation counter fixes the remaining unchanged-cursor race without adding persisted state, and the regression test exercises the exact missed path. I found no new plain error or blocker.
Verification: focused suite - 259 passed in 16.60s. The original reproduction now leaves the cursor at 0 after the stale task, then makes a second annotation call and advances to 8 on the next pass. The only thread I opened was replied to, resolved, and verified resolved.
|
Windows compatibility review: no issue found in the inspected scope. Scope: the complete 14-file PR diff at head Native Windows, Python 3.13.12, isolated detached checkout and user/Raven/temp directories. Dependencies were synced with uv run pytest tests/test_agent_loop_episodic_annotate.py tests/test_providers_forced_tool_choice.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py -n 0 -x --tb=short --basetemp "$R/checks/924-episodic/pytest" --junitxml "$R/checks/924-episodic/junit.xml"
# 259 passed; exit 0.The selection exercises cursor persistence/reload, undo and clear during annotation, queued turn boundaries, origin filtering, backend dispatch, and mocked HTTP 400/500 retry behavior. The previously discussed cursor race is already resolved in its review thread; no duplicate finding is being filed. The consolidator/provider responses are fakes in these checks. No live LLM/EverOS gateway acceptance run, multi-process episode-writer stress or full repository suite was performed; the design's crash-between-write-and-cursor limitation remains. |
|
Blocking: 2 findings need correction before this revision can merge; see the inline notes. Scope: I reviewed only the What I examined: Confirmed findings:
Verification:
Checked and deliberately not reported:
|
LivXue
left a comment
There was a problem hiding this comment.
Requesting changes on the raven/agent/** slice of this PR: 2 blocking notes, 1 nonblocking note and 1 question are filed inline on raven/agent/loop/turn_path.py. The scope statement, the verification log, and what was checked and deliberately not reported are in the PR-level summary comment.
…e boundary Two blocking review findings, both reproduced at the previous head. The origin filter read one message at a time, but the mark rides on the one entry that opens a turn and on nothing else in it: the reply the model writes, its tool calls and their results are all saved unmarked. A Sentinel turn between two of the reader's turns therefore lost its question and kept its answer, and the answer is the half that reaches episodes.md, which two Sentinel producers read. The turn decides now: a user entry opens one and its mark rules until the next opens. The test that claimed to cover this marked the reply by hand as well, a shape _save_turn does not write. The truncation counter was read after the task took the lock, while the boundary it pairs with was read when the turn ended. A task can sit queued behind another annotation on the same session, and a cut landing while it waits had already moved the counter by then, so the check agreed with itself and the stale boundary was used anyway. Both are read together now, describing one moment. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the 8e652948243d...8c61f56b6550 delta against the full change and relevant turn/session callers, history, project rules and terminology, backward compatibility, test coverage, and architecture boundaries. Filtering now follows whole turn boundaries from each opening user entry, preserving a person's unmarked tool/reply records while excluding a machine-opened turn in full. The truncation counter is captured with end before a task can queue on the lock, so a cut during that wait makes the task stand down. I found no new plain error or blocker.
Verification: uv run pytest -o addopts='' tests/test_agent_loop_episodic_annotate.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py tests/test_providers_forced_tool_choice.py -q - 261 passed in 16.74s. My only review thread remains resolved. Threads opened by another reviewer were left untouched for that reviewer to assess.
…ed larger A declined annotation keeps the cursor, which is right, but the tail then came back one turn longer on every following turn with nothing bounding it. A tail refused for its size is refused larger each time, so the log stops being written for that session and each attempt still bills for the input. Review measured 12 turns into 8 calls with the tail growing from 10 to 24 messages, and five tool-heavy turns into a 322k-character prompt, with the retry after it larger again. One pass now sends at most half the context window, the share the token path this replaces consolidated down to, cut where a turn ends so the annotator still sees whole exchanges. The cursor moves to the cut, so what is left over rides the next qualifying turn rather than growing. A single turn larger than the budget still goes out whole: refusing it would stall the cursor on it forever, and it is the caller's to decline once rather than every turn after it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the 8c61f56b6550...c05517c0ec03 delta in the context of the complete change. The new half-window bound preserves whole turns, advances the cursor only through the submitted prefix, and leaves the remainder eligible for a later pass; allowing one oversized first turn also prevents a permanently stuck cursor. I traced the cursor boundary back through persisted message shape and truncation handling and found no plain error.
Coverage this round: repository rules and domain/design documents already established for this change; the delta and surrounding implementation; annotation callers and relevant history; backward compatibility, including legacy cursor seeding; tests for weakened expectations; and the detached-task, truncation, and bounded-context architecture constraints. The ACP-origin question remains an intentional documented behavior and the shared-home concern remains pre-existing in backend dispatch.
Verification: uv run pytest -o addopts='' tests/test_agent_loop_episodic_annotate.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py tests/test_providers_forced_tool_choice.py -q — 263 passed in 18.32s.
The review thread opened in an earlier round is resolved. Other reviewers' threads were left untouched.
…cing it Only one of the three transports carries the relax-and-retry for a refused forced tool choice. On the others a vendor that rejects it -- DeepSeek's thinking mode answers a 400 -- makes that 400 the whole result, so the annotator returns False every time and the episodic log is never written on that transport at all. A model name that is not in the responses list routes to chat, which includes glm. The same 400 is already user-visible there today, on a colder path: /new typed in a chat channel runs the same annotator, and a refusal answers "Memory consolidation failed, session not cleared. Please try again.", which retrying never fixes. Both writers ask for the tool now. The system line already says to call it exactly once, a model that declines is a case both callers have always had -- no tool call, False, the cursor stays and the next pass retries -- and measuring the live vendor three ways showed auto produces the call where required produces a 400. What this gives up is the hard guarantee on vendors that honour required. The adapter relaxation already softened that to a retry; this makes the behaviour the same on every transport instead of only on one. Neither writer is on the conversation path: annotate and refresh_section are reached only through consolidate_messages and maybe_refresh_hot_tags, and a turn passes no tool_choice at all. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the c05517c0ec03...3a5f331aa502 delta and re-read the complete PR comparison. Offering the memory-writer tools enters an already-supported path: both writers return failure when the model declines, and their callers leave the annotation cursor or refresh offset unchanged for a later retry. It also avoids the transport-level 400 on providers that reject a forced choice. I found no plain error in the trade-off.
Coverage this round: AGENTS.md/CLAUDE.md and the runtime vocabulary; the full diff and current delta; both memory-writer callers and provider adapters; relevant commit history; backward behavior for declines and retries; whether tests were weakened; and the cursor, offset, detached-task, and provider-routing architecture constraints. The delta adds tests and weakens no existing assertion.
Verification: uv run pytest -o addopts='' tests/test_providers_forced_tool_choice.py tests/test_consolidator_split.py tests/test_foresight_persistence.py tests/test_agent_loop_episodic_annotate.py tests/test_token_estimation.py -q — 95 passed in 6.76s. git diff --check github/main...HEAD also passed.
The review thread opened in an earlier round remains resolved. Other reviewers' threads were left untouched.
|
Windows compatibility re-review: no issue found in the inspected scope. Reviewed the complete 14-file PR at Native Windows / Python 3.13.12; fresh run-owned environment prepared with $R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7f98f3075ddb453f916c4db0810e091e'
uv run pytest tests/test_agent_loop_episodic_annotate.py tests/test_providers_forced_tool_choice.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py -n 0 -x --tb=short --basetemp "$R\checks\924-episodic\pytest" --junitxml "$R\checks\924-episodic\junit.xml"
# 265 passed.This covers native session writes/reloads, undo/clear and queued-task races, whole machine-turn exclusion, the new input bound, backend dispatch and mocked HTTP retry behavior. Existing origin/truncation/input-bound discussion was read; no duplicate finding is filed. The consolidator/provider responses are fakes. No live LLM/EverOS gateway, multi-process episode-writer stress, real shutdown/restart or full repository suite was run; the documented crash-between-episode-write-and-cursor limitation remains. |
The log has had no writer for the life of the project, so no install is waiting on one. Defaulting the cadence to 5 would bill every user for a side provider call per five user turns to fill a file they may never read; the owner chose cost over reach. Setting annotateEveryTurns turns it on, and the knob already short-circuits before the cursor is seeded, so an install that leaves it at 0 behaves exactly as it did before. The unit suite drives the cadence through its own fixture, so it is unaffected; the shipped default gets one assertion of its own. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the 3a5f331aa502...89d5c0e3db22 delta against the complete change. Shipping annotate_every_turns at 0 is an explicit cost-versus-reach decision: the runtime exits before cursor seeding, so an untouched installation retains its pre-feature behavior, while configured installations still exercise the complete writer path. The acceptance setup supplies its cadence explicitly, and I found no stale default-on dependency.
Coverage this round: repository rules and runtime vocabulary; the diff, affected configuration reader, callers, and history; backward compatibility for existing unconfigured installations; whether tests were weakened; and the cursor-seeding, provider-cost, and boot-time configuration constraints. The delta adds a default assertion and weakens no test.
Verification: uv run pytest -o addopts='' tests/test_config_raven_sections.py tests/test_agent_loop_episodic_annotate.py tests/test_context_engine_factory.py tests/test_segments.py tests/test_session_manager.py -q — 253 passed in 11.05s. git diff --check github/main...HEAD also passed.
The review thread opened in an earlier round remains resolved. Other reviewers' threads were left untouched.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the 89d5c0e3db22...f26e41f654d9 documentation delta against the complete change and its callers. The new ACP participant and open-question entries match the implementation: ACP reaches turn.send, that method stamps Origin.USER, the native annotator consequently admits the turn, and plugin backend storage is independently unconditional. The separate-home deployment distinction and the two Sentinel episodic-log readers are also accurately described. No runtime behavior changes in this revision.
Coverage this round: repository rules and runtime vocabulary; the full change context and documentation delta; ACP, turn-origin, native annotation, backend-store, and Sentinel-reader callers; relevant history and backward compatibility; whether tests were weakened; and the origin/deployment architecture constraint. No test was changed in this delta.
Verification: uv run pytest -o addopts='' tests/test_agent_loop_episodic_annotate.py tests/test_agent_loop_backend_dispatch.py tests/test_acp_methods.py -q — 214 passed in 17.10s. git diff --check github/main...HEAD also passed.
The review thread opened in an earlier round remains resolved. Other reviewers' threads were left untouched.
Review asked whether prompts arriving over ACP should be annotated. They are, because that entrance reaches turn.send as Origin.USER and nothing downstream can tell a parent agent's prompt from a person's. Whether that is right is set by deployment: an ACP Raven with its own RAVEN_HOME writes its own log, which is correct for a separate agent, while one sharing ~/.raven mixes the parent's prompts into the reader's. The reply to that review said this table carried the row; it did not. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
f26e41f to
0db5a7f
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision is tree-identical to the already reviewed f26e41f654d9 revision: both resolve to tree 682115d665567967a7dd963e14eeb63b961515d5, and git diff f26e41f654d9 HEAD is empty. The only change is the commit header from docs(specs): to the repository-compliant unscoped docs: form.
Coverage this round: repository commit rules, revision history, exact full-tree equivalence, and diff cleanliness. Because the implementation, callers, compatibility behavior, tests, and architecture are byte-for-byte unchanged from the preceding reviewed revision, their prior assessment remains applicable; no tests were rerun for this metadata-only rewrite. git diff --check github/main...HEAD passed.
The review thread opened in an earlier round remains resolved. Other reviewers' threads were left untouched.
|
Windows compatibility re-review: no issue found in the inspected scope. Reviewed the complete 15-file PR at Native Windows / Python 3.13.12, isolated source and user/Raven/temp directories, $R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\f78e9e2f41b94d8e90f8a14cdf4769ce'
uv run pytest tests/test_config_raven_sections.py tests/test_agent_loop_episodic_annotate.py tests/test_providers_forced_tool_choice.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py -n 0 -x --tb=short --basetemp "$R\checks\924-episodic-current\pytest" --junitxml "$R\checks\924-episodic-current\junit.xml"
# 303 passed.The selection covers the default-off assertion, enabled annotation path, native session writes/reloads, undo/clear and queued-task cases, origin filtering, input bounds and mocked HTTP retry behavior. The existing annotation discussion was read; no duplicate finding is filed. Limits: consolidator/provider responses are fakes. No live LLM/EverOS gateway, multi-process episode-writer stress, real shutdown/restart, prospective merged-tree test or full repository suite was exercised. The documented crash gap between writing episodes and updating the cursor remains; this is a scoped review, not a full Windows compatibility claim. |
|
No blockers in the reviewed scope. Scope: I reviewed only the This supersedes my earlier summary at What I examined: the delta since Confirmed findings:
Verification:
Checked and deliberately not reported:
|
A turn larger than the whole budget is sent whole, because sending nothing would stall the cursor on it forever. Nothing recorded that it had been sent, so a refusal did not retry the call later under better conditions: every qualifying turn after it rebuilt the same tail and sent the same oversized prompt. Measured by review at 240,095 tokens per call, three times over, with nothing written. That is the self-reinforcing retry the bound was added to stop, in a repeating form. The bound now reports an oversized opening turn, and that is the one refusal the cursor absorbs. An ordinary decline still keeps the cursor, because that one may have been the provider rather than the size. The trimmed prefix is located by identity rather than by dict equality in the same breath. Two messages in a slice can be equal down to the timestamp -- a repeated tool result, or any pair written under a frozen clock -- and a search by value answered with the earlier twin, leaving the cursor short of what was sent and re-annotating the overlap. Each of the three tests was confirmed red with its guard removed. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the 0db5a7f86298...594d3b1e4229 delta against the complete change. Identity-based boundary lookup now advances through the exact capped message even when an equal earlier dict exists. The oversized flag is limited to an opening turn that alone exceeds the budget: its failed call advances without refreshing, while an ordinary failed annotation still keeps the cursor. Both previously reported nonblocking defects are corrected without widening the exemption.
The remaining trade-off is explicit and bounded: a provider failure on an enabled writer's single over-budget turn loses that turn's episode to prevent the same unshrinkable prompt from being retried forever. This requires an opt-in writer, an oversized turn, and a failed call; it is documented and does not meet the late-round blocker bar.
Coverage this round: repository rules and runtime vocabulary; immutable full patch 562c8457c790...594d3b1e4229 (SHA-256 19d8f6be62adf099dd5903f34d9e6b2e8e7688045aa70327bdbcbb3895d02219); the delta, callers, history, compatibility, test strength, persistent cursor state, concurrency, provider failures, and recovery. Patch-risk assessment: merge / human_review_required (moderate impact, low likelihood, partial exact-head protection, managed recovery, moderate confidence).
Verification: uv run pytest -o addopts='' tests/test_agent_loop_episodic_annotate.py tests/test_session_manager.py tests/test_token_estimation.py -q — 146 passed in 8.02s. git diff --check github/main...HEAD passed, and the structured patch-risk assessment validated successfully.
The review thread opened in an earlier round remains resolved. Other reviewers' threads were left untouched.
|
Not a blocker. Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. This contradicts the feature's own stated invariant (turn_path.py:2924-2925, "history is never back-filled, so an upgraded install or a fork does not hand its past to one call"). I confirmed Narrow: needs |
fork copies the parent's messages and its last_consolidated, but not the episodic cursor, so a fresh fork read 0. The after-turn step would have covered that on its own, since it seeds a session it has not seen before. /new does not: it reads the cursor directly, and on a fork it handed every inherited message to the annotator a second time, duplicating in the shared workspace log the episodes the parent had already written. Reported in review, with a reproduction that contrasts a fork against a non-fork control. Narrow in practice -- it needs the step turned on, a fork, and /new as the fork's first action -- but the step's own stated rule is that a session's past is never handed to one call, and that rule was not true here. The test was confirmed red with the seed removed. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
Fixed on
child.metadata["annotated_through"] = len(child.messages)A test in I took it despite the narrowness you measured, because the remedy is one line and the alternative is a stated invariant that holds on one of its two paths. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the three-file delta from 594d3b1e4229 to 1ee83d0d7750. Seeding a fork's annotated_through cursor at its inherited message count is consistent with the existing no-backfill rule and closes the /new path without changing normal first-turn annotation or truncation behavior. The persistence assertion covers reload of the child session, and no test was weakened.
Verification: git diff --check 594d3b1e4229..HEAD; uv run pytest -o addopts='' tests/test_session_manager.py tests/test_agent_loop_episodic_annotate.py -q (125 passed). The live PR head still matches 1ee83d0d7750, the worktree is clean, and no unresolved review threads remain.
Summary
user_memory/episodic/episodes.mdhad no writer on any surface people use.Its only one, the annotator, runs inside the token-pressure compaction loop,
and all three calls into that loop are gated on
owns_compaction, which thesingle shipped context engine hardcodes to
True. The annotator survived aslive code with one caller:
/newtyped as message text in a chat channel,which the web and TUI "new session" buttons never send. Six readers read the
empty file: the prompt line, two attention producers and three predictor
modules.
This gives the log a writer that does not depend on compaction. After a user
turn, once
memory.annotateEveryTurnsuser turns have accumulated since thelast annotation, the unannotated tail goes to the same annotator, in the
background, under the consolidator's per-session lock so it cannot interleave
with
/new.Key decisions, each with the alternative it rejected:
last_consolidated. Theowns_compactiongateexists to keep a second writer off the Curator's pointer, and
undo_last_turnreads that pointer as a floor. The cursor issession.metadata["annotated_through"].sessions.savewith itsown copy of the metadata drops keys the next turn wrote;
append_metadata_patchexists for exactly this writer.annotator chunks bounded by a token budget; this step has no chunking, so an
unseeded long session would go to the provider in one call. Seeding also
covers forks, which copy messages but not metadata. Seeding at the current
length instead would skip the first turn of every new session.
turn leaves the stale index on disk and swallows every turn written after an
undo or a clear until the count climbs past it.
Session.cleardrops thecursor and
Session.undo_last_turnpulls it back to the cut, beside the samemethods' handling of
last_consolidated.Origin.USERturns. Two of the file's readers are Sentinelproducers; writing Sentinel's own turns into it would feed the layer its own
output.
memory.annotateEveryTurnsships at 0, which turns the step off: the log hashad no writer for the life of the project, so no install is waiting on one,
and nobody is billed for a side provider call to fill a file they may never
read. Set it to 5 (or any positive cadence) to turn it on.
The second commit, and why it is here
Driving this on a live gateway showed the annotator could not write at all on
the default model. It sends
tool_choice="required", and DeepSeek's thinkingmode answers
HTTP 400: Thinking mode does not support this tool_choice. TheResponses adapter returned that as the call's result, so the annotator logged
"LLM did not call annotate_conversation" and gave up -- correct behaviour, over
a feature that could never work.
Nine call sites force a tool choice today: two in the memory consolidator,
three in the proactive layer, four in the playbook compiler. Each already
handles a model that declines under
auto. So the adapter now does what itsAnthropic sibling already did: a 400 that names
tool_choicewhile the bodyforces one relaxes it to
autoand retries once. Measured against the livevendor three ways --
requiredanswered 400,autoand omitted both producedthe tool call.
The two commits are separable: the provider repair stands on its own and fixes
eight other latent call sites; without it the first commit is inert on this
model.
Type
Verification
Unit, on this branch:
uv run --frozen --python 3.12 pytest tests/test_agent_loop_episodic_annotate.py tests/test_providers_forced_tool_choice.py tests/test_session_manager.py tests/test_segments.py tests/test_agent_loop_backend_dispatch.py tests/test_context_engine_factory.py tests/test_providers*.py -q-> 344 passedplaybook suites -> 2919 passed, 103 skipped, 1 failed:
test_agent_loop_token_budget.py::test_a_ceiling_as_large_as_the_window_still_leaves_room_for_historyasserts
reserved_tools < 7_500and measures 7557. Pre-existing: a cleangithub/mainworktree at829eb28e1fails it with the same 7557. Thischange touches no tool schema.
make lint-types-> All checks passeduv run --frozen --all-extras lint-imports-> 10 contracts kept, 0 brokenruff check/ruff format --checkon every touched file -> cleanscripts/check_source_language.py github/main...HEAD-> exit 0scripts/check_large_files.py github/main-> exit 0Every new guard was mutation-verified: deleted, the suite run, the matching
test confirmed red on its own symptom, restored, md5-compared. Nine guards,
nine reds -- the knob clause, the origin check, the session lock, the
cursor-on-decline return, the seed, the undo pull-back, the clear drop, and
the provider repair's two halves.
Live gateway, isolated
RAVEN_HOME, real provider,memory.backend = null:episodes.md0 bytesannotate done for 6 messages -> 3 episode(s); three lines carrying a token minted at run timeparse_episode_line#project-<token>the wayproject_rhythmranks byannotateEveryTurns: 0session.undoof two turnsremoved: 4, cursor pulled 26 -> 22 in the record the undo wroteannotate done for 8 messages -> 0 episode(s), cursor advanced -- process-only episodes are dropped by designRisk
User-visible changes:
annotateEveryTurnsat itsdefault of 0: the step short-circuits before the cursor is seeded. An
install that sets a positive cadence runs one annotator LLM call per that
many user turns, plus a profile refresh when a tag crosses the hot
threshold; with a memory backend wired that means two extractors over the
same conversation. The knob is read at boot, so a change needs a gateway
reload.
episodic log.
instead of failing the call. Callers that relied on the error are none: all
nine already handle a declining model.
Rollback: back to
memory.annotateEveryTurns: 0disables the first commit'sbehaviour without reverting it. The provider repair only changes a path that previously
ended in an error response.
Not covered: a crash between the file write and the metadata patch re-annotates
one tail, duplicating lines. Harmless to grep and over-counts a tag by one
batch at most; making it atomic means writing the cursor into the same file
under the same lock.
Related Issues
#122