Repository navigation
feat(*): align empty-reply recovery with Hermes and let a turn end in silence - #1092
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: session.list must preserve activity from a proactive turn that already delivered through the message tool.
I found one merge-blocking cross-surface mismatch, marked inline. I tried to refute it against the intended silence semantics: an ordinary runtime-origin Silent Turn should disappear from activity/replay, but this revision explicitly treats a turn answered by the message tool as answered, and ACP preserves that exact case. The session scanner does not, and a direct stored-row reproduction returns the previous turn's timestamp.
Coverage: I reviewed the repository rules and canonical Runtime/TUI/Web boundaries, the full diff, core recovery and provider paths, silence propagation through gateway/RPC/session/ACP/cron, both UI consumers, relevant callers, commit history, backward-compatibility seams, and test changes for weakening.
Verification: the focused Python slice passed (834 passed); web passed (4,281 tests, type-check, generated RPC check); TUI passed (2,266 passed, 13 skipped, type-check, RPC/i18n generation checks); commit-message, source-language, and large-file checks passed. The full Python run finished with 34,625 passed, 187 skipped, 1 xfailed, 15 failed, and 2 collection errors: fourteen failures/errors require unavailable optional datrie/nltk dependencies (datrie could not build because this image has no gcc), and one failure is an untouched CLI-theme assertion.
7269126 to
f4fb262
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Re-reviewed the current head after its rebase and the blocker fix. The range-diff shows the prior 97-patch series is unchanged; the only new patch centralizes message-tool turn detection and makes the session scanner preserve delivered runtime activity, matching ACP replay. The regression exercises the concrete stale-session failure, and the helper tests cover both stored provider calls and flattened wire calls. No tests were weakened.
Covered the repository rules and context terminology, the current diff, affected session-list and replay callers, patch history across the rebase, compatibility with both transcript shapes, and the shared-rule architecture. Verification: 249 focused session/replay tests passed; git diff --check and the source-language, large-file, and commit-message gates also passed. The prior thread is resolved.
|
Windows compatibility review at head What was reviewed. All 115 changed files were tiered by Windows risk and the whole diff was read. Tier 1 (read fully and verified carefully): 20 files - the session and export readers, Verdict: this PR introduces no Windows compatibility regression. The changed lines contain no new filesystem, subprocess, signal, path-join, encoding or Windows-only API calls at all. The only new persistent artifact is a JSON key ( Inherited issue (present at the base commit too, not introduced here). Inherited coverage note. The Native Windows verification (Python 3.13.12, UTF-8; Node 25.9.0, npm 11.12.1; Windows 11 10.0.26300 x64):
Exact verification commands and results$root = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024'
$env:UV_PROJECT_ENVIRONMENT = "$root\venv"
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
# batch 1 at the head; exit 1
$head = "$root\pr1092\head"
$env:PYTHONPATH = $head
uv run --frozen --no-sync --project $head pytest -c "$head\pyproject.toml" --rootdir $head `
"$head\tests\test_spine_silence.py" "$head\tests\test_acp_replay.py" `
"$head\tests\test_rpc_session.py" "$head\tests\test_session_export.py" "$head\tests\test_config_schema.py" `
-p no:randomly -p no:cacheprovider -n 0 --tb=no -q --basetemp "$root\scratch\pr1092\head-batch1"
# 317 passed, 2 failed - both test_acp_replay.py TestToolCalls cases, KeyError: 'locations'
# batch 1 at the base commit (test_spine_silence.py does not exist there); exit 1
$base = "$root\pr1092\base"
$env:PYTHONPATH = $base
uv run --frozen --no-sync --project $base pytest -c "$base\pyproject.toml" --rootdir $base `
"$base\tests\test_acp_replay.py" "$base\tests\test_rpc_session.py" `
"$base\tests\test_session_export.py" "$base\tests\test_config_schema.py" `
-p no:randomly -p no:cacheprovider -n 0 --tb=no -q --basetemp "$root\scratch\pr1092\base-batch1"
# 2 failed, 260 passed in 32.40s - the same two
# test_acp_replay.py alone, both sides; exit 1 on each
uv run --frozen --no-sync --project $head pytest -c "$head\pyproject.toml" --rootdir $head `
"$head\tests\test_acp_replay.py" -p no:randomly -p no:cacheprovider -n 0 --tb=no -q --basetemp "$root\scratch\pr1092\head-replay"
# 2 failed, 50 passed in 12.92s# batch 2: 33 changed test files, head then base; exit 1 on each
# (the file list is the 33 changed test files of this PR)
$env:PYTHONPATH = $head
uv run --frozen --no-sync --project $head pytest -c "$head\pyproject.toml" --rootdir $head <33 files> `
-p no:randomly -p no:cacheprovider -n 0 --tb=no -q --basetemp "$root\scratch\pr1092\head-batch2"
# 28 failed, 2039 passed, 1 skipped, 26 warnings in 542.58s (0:09:02)
$env:PYTHONPATH = $base
uv run --frozen --no-sync --project $base pytest -c "$base\pyproject.toml" --rootdir $base <33 files> `
-p no:randomly -p no:cacheprovider -n 0 --tb=no -q --basetemp "$root\scratch\pr1092\base-batch2"
# 28 failed, 1668 passed, 1 skipped, 26 warnings in 391.21s (0:06:31)
# the two sets of 28 failing test ids are identical# the touched ui-web suites and the type check; exit 0
npm.cmd ci --prefix "$root\pr1092\head\ui-web" --no-audit --no-fund
npm.cmd test --prefix "$root\pr1092\head\ui-web" -- src/features/transcript/TranscriptPage.test.tsx src/state/session/pipeline.events.test.ts src/state/session/pipeline.notify.test.ts src/state/session/runtime.cancel.test.ts --no-file-parallelism
# Test Files 4 passed (4); Tests 297 passed (297)
npm.cmd run type-check --prefix "$root\pr1092\head\ui-web"
# exit 0# the touched ui-tui suites and the generated-catalog check; exit 1 (the 7 inherited), then exit 0
npm.cmd ci --prefix "$root\pr1092\head\ui-tui" --no-audit --no-fund
npm.cmd ci --prefix "$root\pr1092\head\ui-tui\packages\hermes-ink" --no-audit --no-fund
npm.cmd test --prefix "$root\pr1092\head\ui-tui" -- src/__tests__/createGatewayEventHandler.test.ts src/__tests__/messages.test.ts src/__tests__/turn-subscribe.test.ts
# Test Files 1 failed | 2 passed (3); Tests 7 failed | 108 passed (115)
node "$root\pr1092\head\ui-tui\scripts\gen-i18n.mjs" --check
# i18n: generated catalogue is up to date (exit 0)
# the base checkout's messages.test.ts fails with the same 7 names
npm.cmd test --prefix "$root\pr1092\base\ui-tui" -- src/__tests__/messages.test.ts
# Test Files 1 failed (1); Tests 7 failed | 51 passed (58)node --version # v25.9.0
uv --version # uv 0.13.0 (4ecd0eff2 2026-10-09 x86_64-pc-windows-msvc)Coverage limits: no live model call was made, no Windows ACP stdio or process-tree scenario was exercised, and the full test suite was not run. The |
|
Windows compatibility re-review at head The previous review was at head Inherited issues only. These are already present at base
Native Windows verification (Python 3.13.12, UTF-8): at head Exact verification commands and results$env:UV_PROJECT_ENVIRONMENT = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\venv'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
uv run --frozen --no-sync --project 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head' pytest -c 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head\pyproject.toml' --rootdir 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head\tests\test_session_turn_shape.py' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head\tests\test_rpc_session.py' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head\tests\test_acp_replay.py' -p no:randomly -p no:cacheprovider -n 0 --tb=short -q --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\pytest'exit code 1 - 2 failed, 247 passed (both failures inherited, item 3 above) $env:UV_PROJECT_ENVIRONMENT = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\venv'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
uv run --frozen --no-sync --project 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head' pytest -c 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head\pyproject.toml' --rootdir 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head\tests\test_session_turn_shape.py' -p no:randomly -p no:cacheprovider -n 0 --tb=short -q --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\pytest-newtests'exit code 0 - 7 passed (new file) and, together with the new $env:UV_PROJECT_ENVIRONMENT = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\venv'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\base'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
uv run --frozen --no-sync --project 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\base' pytest -c 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\base\pyproject.toml' --rootdir 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\base' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\base\tests\test_rpc_session.py' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\base\tests\test_acp_replay.py' -p no:randomly -p no:cacheprovider -n 0 --tb=short -q --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\pytest-base'exit code 1 - 2 failed, 231 passed (base, same two failures: inherited) git -C 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\pr1092\new\head' archive --format=zip 72691268c5a6ad47397163a900fb0d37be75a845 -o 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268.zip'
Expand-Archive -Path 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268.zip' -DestinationPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268' -Forceexit code 0 - previously reviewed head extracted into the scratch dir, unmodified repo $env:UV_PROJECT_ENVIRONMENT = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\venv'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
uv run --frozen --no-sync --project 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268' pytest -c 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268\pyproject.toml' --rootdir 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268\tests\test_rpc_session.py' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\prev72691268\tests\test_acp_replay.py' -p no:randomly -p no:cacheprovider -n 0 --tb=short -q --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\pytest-prev'exit code 1 - 2 failed, 239 passed (previously reviewed head, same two failures: inherited) $env:UV_PROJECT_ENVIRONMENT = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\venv'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
uv run --frozen --no-sync --project '<checkout>' python 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\probe_bom.py' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\bom-<base|prev|head>'
uv run --frozen --no-sync --project '<checkout>' python 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\probe_codepage.py' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024\scratch\pr1092delta\cp-<base|prev|head>'exit code 0 for all three checkouts - "BOM hides the session: True" at base, at the previously reviewed head and at head; a cp936 transcript raises Scope covered: the delta only, on native Windows, for the session transcript read path, the ACP replay path, the new |
154f8ea to
67357ab
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the revision delta through the live head 67357abfffc3. The rewritten fix retains the previously verified message-tool exception, adds a meaningful cross-turn regression for resetting answered at each user row, and the final commit only formats that test. The call-based behavior remains shared with ACP replay and compatible with both stored and flattened tool-call shapes; no tests were weakened.
Covered the applicable repository rules and context terminology, the delta and affected callers, history across the rewritten commit and formatting tip, backward compatibility, and the shared-rule architecture. Verification at the live head: 389 focused session/replay tests passed; Ruff formatting, git diff --check, source-language, large-file, and commit-message gates passed. My thread is resolved.
|
Windows compatibility re-review at head The previous review was at head The production change adds no Windows surface. It imports The new test file is written for Windows rather than against it. That is the part worth recording, because a test file for the session manager is exactly where a POSIX-only assumption would land. It takes the multiprocessing start method explicitly - Native Windows verification (Python 3.13.12, UTF-8; Windows 11 10.0.26300 x64): the new Coverage limits: the delta was exercised through the new test file and the reading path it covers; the full session suite, the ACP launcher and the Node side were not run, and the inherited BOM and code-page findings were not re-measured in this round because the reader they concern is unchanged. This is a scoped compatibility report, not a merge verdict. Exact verification commands and results$root = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024'
$ck = "$root\pr1092\new2\head"
$env:UV_PROJECT_ENVIRONMENT = "$root\venv"
$env:PYTHONPATH = $ck
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
# the new test file at the current head; exit 0
uv run --frozen --no-sync --project $ck pytest -c "$ck\pyproject.toml" --rootdir $ck `
"$ck\tests\test_session_manager.py" -p no:randomly -p no:cacheprovider -n 0 --tb=short -q `
--basetemp "$root\scratch\pr1092\sessmgr"
# 140 passed in 14.29s (exit 0)
# the delta itself
git -C "$root\repo" diff --no-color "e8332e5d4c..67357abfffc3940663a66ca4f217a9dcfdc06936" -- raven/session/manager.py
# the spawn context the new test takes
Select-String -Path "$ck\tests\test_session_manager.py" -Pattern 'multiprocessing|get_context' |
|
Merge conflict: @LivXue this PR no longer merges cleanly into Measured just now with |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: rebase onto current main and resolve the four merge conflicts before merge.
I reproduced the report with git merge-tree --write-tree using current main dacfc64c and this head 67357abf. It conflicts in raven/agent/loop/turn_path.py, raven/agent/window/compaction.py, ui-web/src/state/session/runtime.ts, and ui-web/src/state/session/stages.ts. This meets the late-round blocker bar: this branch's overlapping changes produce the conflict, it is reached by the ordinary merge path, and there is no working merge path until the branch is refreshed and the conflicts are resolved.
The unchanged branch itself still passes the focused session/replay suite (389 passed), but that cannot validate the unresolved integration with current main.
67357ab to
a71f068
Compare
|
@gloryfromca @0xKT Rebased onto main
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The rebase resolves the four conflicts against main at 471842132; git merge-tree --write-tree now succeeds. I reviewed the range-diff and the substantive resolutions: continuation requests are capped to their remaining context room without overriding hook-owned ceilings, and a held cancel now drops its stale silent mark with the late end so the following turn is not misclassified. The added sub-agent and turn-navigation controls preserve the merged behavior. I found no concrete regression, and the previous merge-conflict blocker is cleared.
Covered the repository rules and context terminology, the current diff and conflict-resolution history, affected callers, compatibility with the merged main behaviors, test strength, and the shared runtime/UI architecture. Verification: 1,011 focused Python tests passed; the targeted web rerun passed 96/96 after one load-related timeout in the initial parallel run; the complete web suite passed 4,425/4,425; web type-check, generated-client check, diff check, source-language, large-file, and commit-message gates passed. My review thread remains resolved.
|
Windows compatibility review — PR #1092 @ Scope: full base Result: no Windows-specific code added and no new Windows test failures. What I checked:
Verification (native Windows 11, CPython 3.12.13, isolated worktrees The failing-ID sets are identical (diffed as sets — no additions, no removals): the 20 Limitations: the UI/TUI portions of the diff were not run (generated clients only read); the full 90-file suite was not run; no live provider or gateway was exercised. Only the six platform-sensitive suites above were measured. |
|
Merge conflict: @LivXue this PR no longer merges cleanly into Measured just now with After a rebase onto current main I re-check what moved before the grade comes back. |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: rebase onto current main and resolve the turn_path.py conflict before merge.
I reproduced the report with git merge-tree --write-tree using current main 26352da0 and this head a71f0688; raven/agent/loop/turn_path.py conflicts after #1119 changed the same call-window bookkeeping. This meets the late-round blocker bar: the branch's overlapping changes produce it, it is reached on the ordinary merge path, and there is no working merge until the branch is refreshed and the resolution is validated.
The unchanged head still passes the directly affected agent-loop suites (365 passed), but those tests cannot validate an unresolved integration tree.
a71f068 to
d5c19e0
Compare
|
@gloryfromca @0xKT Rebased onto main
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The rebase onto 26352da0 resolves the outstanding conflict cleanly. I checked the rewritten series and the turn_path.py integration: the four non-identical range-diff entries retain both this PR's silent-turn behavior and main's context-window stamp. The added tip change also correctly stamps a reply assembled from a cut answer with the usage from the call it represents, while preserving the established unknown-usage behavior.
This re-review covered the repository rules and domain context, the changed diff and conflict resolution, the affected callers and history, backward compatibility, test strength, and the context-stamping architecture. The new regression exercises the integration rather than weakening prior coverage.
Verification at d5c19e07d8e3:
git merge-tree --write-tree HEAD github/main: clean- affected recovery/context tests: 432 passed
- Ruff, diff, source-language, large-file, and commit-message checks: passed
|
Windows compatibility re-review — PR #1092 @ Scope: the head moved from the previously reviewed Result: no Windows-specific issue; the earlier conclusion holds, and the previously noted inherited ref-path failures are gone on main. Verification (native Windows 11, CPython 3.12.13, isolated worktrees The failing-ID sets are identical (diffed — no additions, no removals): 11 Limitations: the UI/TUI portions of the diff were not run; the full suite was not run; no live provider or gateway. |
|
Not a blocker -- behaviour findings from the same acceptance pass, separate from the blocking thread. Most were reproduced by running probes through the PR's own test helpers in an isolated tree, against both this head and base 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.
Ran in an isolated tree (this head merged onto |
Each part of a cut answer had its think blocks taken out on its own, so a cut inside an inline think block left both halves of the block in place: the opening half unclosed in the first part, the closing tag in the continuation. The joined reply delivered the raw reasoning, "<think>abcdef</think>Four." for "Four.", and a marker after the thought was never read as one. On a proactive turn the loose rule read the continuation alone, "def</think>" over "[SILENT]", as a marker, dropped it, and delivered the unclosed "<think>abc" as the report. The parts are now kept as they came, and on every exit that ends on them -- a continuation that completes or comes back empty, one that fails, the spent continuations, a budget that stops the turn, and a continuation that calls a tool -- the reply is the joined text with its think blocks taken out of the whole, read for the marker by the turn's rule the way a reply with no cut is. A continuation inside a block the cut left open is read only that way, never as a marker or as debris of its own. A cut thought that the continuation only closes leaves no answer, and the turn now ends with no text rather than with the reasoning as its reply.
A cut "[SILENT]" whose continuation was only "</think>" ended the turn silent: the loop reads a reply that is nothing but a closing tag with no block open as the debris a reasoning parser leaves, and the continuation added nothing to the marker. The gate read the same tag as text that could not be the marker and let the held "[SILENT]" out with it, so the reader saw the marker of a turn that ended silent. The gate now reads a closing think tag with no block open, in any of the spellings the loop strips, as debris: not a word of the reply, so the hold goes on past it. A reply the loop then reads as text is let out at the end of the call, as any held reply is.
The wire said the flag sits on the turn's closing entry, or on "the entry that made the call" when the turn closed on a reply with no text. Neither holds in general. The stamp goes on the turn's last assistant row with text or tool calls, so a turn that sent a message, then listed a directory, then said nothing has it on the listing's call, not the send; and a notice the runtime files after the turn, for a question nobody answered, comes after the stamped row without the flag. The field description, the schema, both TypeScript mirrors (regenerated), CONTEXT.md and the docstrings that repeat it now say exactly that, and a test pins the shape: the send unflagged, the listing's call flagged, the notice after it unflagged.
…ut one A thought cut in two, "<think>abc" and then "</think>" or "def</think>", joined into a reply with no text once its think block was taken out. The loop then ended the turn as a success with no text: final content empty, no_output set, only the question stored, no notice and no failure, while the same reply uncut took the thinking-only moves and failed when they were spent. The answerless guard only fails a turn whose content is None, and the join turned None into "". Before a reply with no tool call is read, a cut answer whose join with what continued it is nothing but a thought now becomes that joined reply, the cut parts done with: reply_kind and decide read it as they read the uncut one, so it is prefilled, stepped down, sent to a fallback model and failed in the same order. Each part a cut adds leaves the joined text with something to read, so no later exit ends on parts with no text. The test that pinned the turn ending with no text is replaced by tests that run the cut and uncut shapes side by side.
The row a tool call is filed on keeps what the model said beside the call as it came, inline thought included. After a cut, the row took the joined words with their think blocks stripped and trimmed, so a cut inside a thought filed "Let me look." where the same call uncut filed "<think>abc</think>Let me look.", and whether the stream had been cut changed the history. The row now takes the parts and the call's own text joined as they came, which is what one uncut call would have said. The tests run the cut and uncut shapes side by side; the one that read the trimmed partial now reads it with the space at the cut kept.
CONTEXT.md said every exit that ends on a cut answer's parts reads them with their think blocks taken out. Only closed blocks are: a thought the cut left open and nothing closed stays in as text, on the failed-continuation exit and with recovery switched off, just as a reply with no cut and an unclosed think tag is read as text. The entry and the loop's comment now say that, and a test runs the unclosed thought cut and uncut side by side.
When cut parts join into a bare thought, the loop rereads them as the uncut reply and drops the hold the cut carried. Without that drop, a proactive turn let the held thought out together with the answer the prefill drew, and no test noticed. The new test runs the uncut, cut and cut-at-its-close shapes and expects only the answer on screen.
litellm's stream wrapper drops a chunk whose only payload is a refusal, so a refusal an OpenAI-compatible model streams reaches Raven as an empty stop. It was resent three times with backoff, every time as a stream that dropped it again, then switched to the fallbacks and failed in empty-reply words. The same call made without streaming reads the refusal, and a gateway whose streams come back empty while its plain calls work is rescued by the same move. On a streamed turn, the first resend of a plain empty reply, the one decide answers RETRY to, now asks the same model without streaming. It is one of the empty_content_max_retries resends, under the same backoff and notice; later resends stream again, and an empty reply right after tools is resent as it was. The call hands the provider no fallback chain, so a model switch stays the loop's. What comes back is read as any reply: a refusal goes to its one fallback switch or fails with the model's explanation, tool calls run, an empty reply goes on down the ladder. What a stream would have shown is shown once the reply is in, through the same sinks: the thought through the reasoning sink, then the words through the gates a streamed delta passes, so a marker is still held and dropped, a reply reaches the reader once, and the resend opens the step a streamed one would. deterministic_empty is unchanged in meaning: it stops only on a streak of zero-output replies of one model and finish, so mixing a streamed and an unstreamed attempt can only fail open. Tests drive litellm's own OpenAI route over a mock transport through the loop, and the loop's sinks, gates, steps and budget directly.
The unstreamed resend of an empty streamed reply replays what a stream would have shown through the reader's sinks, but only for a call that did not fail: a failed call's content is the account of the failure, not a reply. Nothing pinned that guard, so dropping it left the suite green while an error's text would have reached the reader as words of the answer before the error handling ran. The test resends an empty streamed reply and has the unstreamed call come back as a non-retryable error: none of its text reaches the token sink, and the turn ends as an error does.
d5c19e0 to
f55e814
Compare
|
@0xKT Thanks for the acceptance pass. All ten are answered in
Two more from the rebase and the gates:
Verification at |
|
Windows compatibility re-review — PR #1092 @ Scope: the head moved from the previously reviewed Result: no Windows-specific issue, and no reproducible new Windows failure. Verification (native Windows 11, CPython 3.12.13, isolated worktrees The failing-ID sets are identical between the second head run and main (diffed — no additions, no removals): 11 Limitations: the UI portion was not re-run this pass (unchanged behaviour, main's merge only); no live gateway. |
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 35-commit delta beyond the previously assessed series; range-diff shows the earlier 103 commits replayed unchanged. The frozen-kernel fix is complete, and the acceptance changes consistently cover the per-origin silence rule, live message-tool verdict, cut/continued replies, OpenAI-shaped refusals, merged-message typing cleanup, and transcript rollback without weakening prior behavior. I found no plain error worth raising.
This pass covered the repository rules and domain context, the delta and affected callers, history/range-diff, backward compatibility, test changes, and the kernel/layer architecture.
Verification at f55e814ca299:
- affected Python suites: 1,353 passed, 27 warnings (Pydantic/LiteLLM warnings plus one LiteLLM logging-worker RuntimeWarning)
- affected web suites: 275 passed
- import contracts: 10 kept, 0 broken
- Python and UI type checks, Ruff, RPC generation, source-language, large-file, commit-message, and diff checks: passed
- this host has no
make, so I ran thelint-typestarget's exactuv ... ty checkrecipe directly; it passed - current
github/mainis the merge base; merge-tree is clean and GitHub reports MERGEABLE
The silence gate now reads think tags as it holds a person's reply, and it read every character of a held delta through that reader. A long hold is only ever whitespace before the marker or after the whole of it, which moves no reading, yet each blank delta paid the full pass: eighty thousand of them took 1.72 s in CI's coverage-traced run, past the bound the hold's cost test sets. A whitespace delta that every reading would pass over unchanged is now kept as it is. Under coverage the same run takes about 0.22 s, close to what it took before the tag reader. A new test counts the characters the readers see; a randomized comparison of 20,000 delta sequences against the per-character path found the same output every time.
|
CI on
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the one-commit delta from f55e814ca to fab98f576. The whitespace fast path is behavior-preserving: it applies only when every active marker reading is outside a think block/tag and is either before the marker or after the complete marker, exactly the states where reading whitespace returns true without changing state. It removes the coverage-triggered per-character overhead without weakening the timing bound.
Verification at the new head: the full empty-recovery file passes (446 tests); the timing regression and new reader-count regression pass under coverage with strict idle ceilings; Ruff passes on both changed files. I covered the repository rules, the delta and surrounding state machine, the CI-triggering history, caller/backward behavior, test strength, and architecture impact. The clean-thread sweep found my only review thread already resolved.
|
Windows compatibility re-review — PR #1092 @ Scope: the delta from the previously reviewed head What changed: Equivalence check (code reading): under exactly those preconditions the per-character path returns Verification on native Windows 11, detached worktree
Gaps: the full suite and the coverage-traced shard were not rerun on Windows for this delta; the timing figures in the commit message come from the author's CI run. Within those limits, no Windows compatibility issue was found for this head. |
|
Not a blocker - delta acceptance on 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.
What is left is non-blocking:
Tests here on the merged tree: all 45 changed Python test files via the board runner gave 2972 passed. ui-web, 6 changed test files: 340 passed. ui-tui, 3 changed test files: 116 passed. |
|
Thanks for the pass. #1092 merged before I answered it, so the seven
Verification on the follow-up head: the full Python suite 36967 passed, |
Summary
Supersedes #1052, which was closed unmerged. That PR ended a turn
without a reply whenever the model returned nothing. After a survey of
how other agents handle the same replies, empty and abnormal replies now
follow Hermes Agent, and Raven keeps its reasoning-effort step-down only
for a reply that stopped while still thinking. This also ends the Codex
HTTP 400 "Unsupported value: 'minimal'": the old empty-reply recovery
lowered the effort on every resend, down to
minimal, which Codexrefuses. Neither recovery nor the in-turn compaction summary asks for
minimalany more; a session whose own effort isminimalstill sendsit, and the compacting history strategy's checkpoint summary goes out at
the provider's configured effort.
Silence is now something the model says, not something it omits:
turn a person started honours only the bracketed
[SILENT]theruntime teaches, as the whole reply (whitespace aside): such a turn is
never taught a marker, so
Silent.orNo reply.there is theperson's answer. That is stricter than Hermes, whose strict rule takes
all four spellings. A proactive turn (cron, heartbeat, Sentinel, a
sub-agent report) uses Hermes' rules over all four (
[SILENT],SILENT,NO_REPLY,NO REPLY): the whole reply, at most 64characters, or the marker on the first or last line. An A2A turn,
whose peer waits on the text, gets its reply back as written.
raven/agent/context/silence.py, outsidethe kernel. The kernel carries only the outcome:
TurnOutcome.silentand the
silent_turnnotice.report teach the marker, in Hermes' words.
a silenced reply never flashes on screen. On a person's turn only a
reply that opens with whitespace or
[is held, and an inline thinkblock streams as it arrives while what follows it is held; a
proactive turn holds the whole call. The hold lasts across a stream
cut and its continuation.
silent: trueand the turn ends with asilent_turnnotice; the TUI and the web UI draw one muted line("Raven chose not to reply."). ACP replay, the session list, the
session export and the cron run list do not show it as a reply. A
reply an
after_sendhook composes in place of the marker isdelivered and stored as that reply.
end_turn) when a turn a userstarted, or one a user's message was merged into, ends without a
reply. A silent cron run is a success: it wakes nobody and does not
count toward the silent-fire limit.
An empty reply is recovered by kind, decided by two pure functions
(
reply_kind,decide):prefills when there is thinking to re-feed, then the same step asked
again at
high, thenlow(only the rungs below the turn's effort,each once per turn), then the fallback models, then a failure;
times, or once when the call reported a cost at or above
emptyRetryCostThresholdUsd(default 0.25 USD); two zero-outputreplies from one model stop the resends early. On a streamed turn
the first resend goes out without streaming (see below);
continuation with max_tokens doubled each time (x2, x4, x8), held to
the model's own output ceiling and to the room its prompt leaves;
refusalfield, which litellm moves intoprovider_specific_fields):never resent; one fallback model, whose answer opens a step of its
own, then a failure that quotes the model's explanation;
of the turn.
Failures are worded after Hermes, per kind, and
no_output(onTurnFailedandAnswerlessTurnError) marks a turn whose last replywas empty. A transient
llm_retrynotice says when a resend iswaiting.
A stream that dies after showing text is continued (up to three times,
stitched into the same reply). If it stays cut, or the turn runs out of
iterations or time while a continuation is owed, the partial is
delivered with a note that it is partial, status
interrupted, insteadof failing the turn. A cut is read before any marker, so a fragment that
happens to spell one is continued, and a stream that had already
received a refusal finish keeps it; the reply the turn ends on, the
joined parts with their closed think blocks taken out, is then read for
a marker like any other, and parts that join into nothing but a thought
take the thinking-only path an uncut thought takes. A continuation that
calls a tool keeps the words the reader saw on that call's row, as they
came. Providers now report the finish they actually got:
unknownfor amissing one,
refusal/content_filter, and an error for a 200response that carries one. ACP reports a turn its own budget ended as
max_turn_requests(#1121); a cut stream delivered as a partial spentno budget and stays
end_turn.Decisions beyond the design, so a reviewer does not rediscover them:
it
finish_reason="stream_cut"; a reply that merely lacks a finish isdelivered as is. Continuing on a missing finish would draw three
continuations on every reply of a backend that never sends one.
frame, but say that the cut left nothing and that the model may
answer or make the call the task needs: Raven sends them only for a
cut that produced nothing, which Hermes' wording does not fit.
(
MessageTool.sent_in_turn: its send to the turn's own chat wentout), an empty last reply that a nudge or a prefill does not mend ends
the turn as answered, without the paid resends, step-downs and
fallbacks; a refusal there ends it the same way. A marker after such a
reply does not make the turn silent, so no
silent_turnnotice issent.
same model without streaming, with no provider-side fallback, and its
answer is shown through the same sinks and gates a stream's deltas
pass; later resends stream again. A stream can come back empty where
the plain call does not: the locked litellm 1.85 drops a streamed
chunk that carries only a refusal, so a refusal an OpenAI-compatible
model streams arrives as an empty stop, and the plain call reads it
and hands it to the refusal path. A gateway whose streams fail is
rescued the same way.
above, only the providers' finish reasons reach them (see Risk).
after_sendhooks still run on a silent turn, withoutbound_content=None;FACTORY_LOOP_SURFACE_VERSIONis now 4.How a stored turn is read: the live turn files
replied_via_toolon thelast assistant row a turn the message tool answered stores, and the
session list and the ACP replay read that flag from one place,
raven/session/turn_shape.py, instead of guessing from the calls theycan see. A send that failed or went to another chat answered nobody, so
both surfaces agree with the live view about which turn answered.
Type
Verification
At the PR head, on origin/main 06509f8 (Linux, 32 cores).
The full Python suite: 36878 passed, 9 failed, 255 skipped, 2
xfailed.
The same 9 fail on a clean archive of origin/main 06509f8 on this
machine: five proxy tests in tests/test_config_update_providers.py,
test_install_script, test_knowledge_parser_excel,
test_subagent_node_runtime, and test_cli_agent_commands'
empty-message-file test, whose error box is cut to the terminal
width. They come from the environment, not from this branch.
ui-web
npx vitest run: 260 files, 4461 passed.ui-tui
npx vitest run --maxWorkers=2: 145 files, 2275 passed, 13skipped.
make lint-python lint-imports lint-deps lint-types lint-ui lint-tui check-commits check-large-files check-source-language: all exit 0.ui-tui
npm run lint:i18nandnpm run lint:rpc, and ui-webnpm run gen:check: exit 0.The review round's fixes were each written test first, and each was
checked by removing or inverting it and watching a test go red. An
independent re-review repeated those mutants, probed the stream and
marker cases through the real loop against the previous head, and
fuzzed the stream gate over about 35,000 random two-part cut replies
without finding a silent turn that showed its marker.
The rebase onto 06509f8 replayed every commit unchanged; its one
meeting point with main, fix(*): report a delegated turn's real outcome and cost #1121's budget-stop marker, is described in
the summary. Earlier rebases met feat(context_engine): compacting history strategy with source refs and checkpoints #1031 (compacting history), fix(*): say whether the lane is still busy when a cancel is refused #1078
(refused cancel), fix(*): restore the context ring from the window the last call measured #1119 (the context ring a resume restores), fix(agent): a sub-agent's blank reply is no answer, not the answer #1047
and feat(ui-web): add a turn nav that marks each turn beside the transcript #1114; what each needed is pinned by tests on this branch.
Prompt text, by a paper exercise with fresh model readers, at an
earlier head (no prompt text has changed since): a routine sub-agent
report was answered with
[SILENT]and a finished one with a realreport; a plain one-shot reminder was still delivered; an hourly
watch with nothing new stayed silent. With the text before this
change, the two routine cases produced a message.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
What a user will notice: a reply that is only a silence marker is not
delivered; an empty reply is asked again, stepped down or switched to a
fallback before the turn fails, and fails in Hermes' wording; a cut
stream is continued and, if it stays cut, delivered as partial instead
of failing the turn.
Known gaps, each left as it is here:
or a retryable error that outlasts the stream's own ladder, about
105 s) fails the turn although the reader saw the partial.
errors) is not switched to a fallback model: it is waited out and
fails the turn, as before. A stall before any output, and an error
that comes back as a response, do fall back.
on its own is not known to the loop, so a later empty-reply fallback
can ask that model again.
its own, and only the fallback's answer is stored, so a reload shows
the answer alone. Storing the refused words would need a row kind
shown to the reader but never sent to the model, and none exists.
a reload draws the muted line after the reply; on the web the reply
then sits in the turn's folded steps.
replay with its tool calls, as the session list leaves it out,
unless the message tool replied in it.
result turns that are never taught the marker: such a reply whose
first or last line is a bare "No reply" or "Silent" is not delivered.
job that always stays silent is never paused, and each run adds about
3.5 KB to its session file and curator traces until consolidation
trims them. A job whose model always returns nothing keeps firing,
visible only as lastStatus=error.
before it is handled as a refusal, and only the turn's first plain
empty resend can read it: a fallback model is never resent to, so a
streamed refusal from a fallback still reads as an empty reply. The
unstreamed resend's answer is shown whole rather than as it is
written, has none of a stream's idle watchdogs, and shows only the
retry notice while a long-reasoning model works; a proxy that caps
plain responses turns it into an error, which takes the usual error
path. A test pins what litellm 1.85 passes on, so an upgrade that
starts passing the words is noticed.
usage.cost_usd, so it doesnothing on direct OpenAI, Azure, Codex or the Anthropic API unless a
gateway adds the cost.
the model's thinking in the TUI and web detail; channels get the
bounded 200-character text.
stream_cutshows as the call's finish in the trace viewer.read the providers' honest finish reasons this way: a refusal with
text is the run's answer; a refusal, content filter or missing finish
with no text is a blank reply, which gets the one wrap-up call and
then fails as having no answer; a 200 response carrying an error fails
the run as a failed model call, with no wrap-up. An empty stream whose
prompt was never read is asked once more without streaming in a
sub-agent's direct chat before it fails.
than the room its prompt leaves, as the first call did; a reply cut
there with nothing in it is therefore usually cut again and fails
after its continuations.
to go on) in the window for the rest of the turn, so a continuation
that calls a tool shows the model the partial twice, and a cut that
joins into a bare thought is prefilled, stepped down or sent to a
fallback after them: the re-ask reads as "continue the cut thought",
and a model that goes on mid-thought has the thought's tail delivered.
The stored history has neither row. Taking the rows out lost a hook's
note and left the question autofill reading an old list.
whose continuation is refused and then answered by a fallback shows
the held marker in the refused step; a cut marker prefix whose
continuation starts the marker over (
[SILEN, then[SILENT])delivers the fragment, as before; a hook rollback of the
continuation's tool call drops the partial from that call's row, as
before.
output leaves that chat's typing indicator running until the next
send.
dropped, and with it any silent mark that turn set, so the next turn
is not drawn as silent.
hold are quadratic on very long runs of tags or whitespace.
Compatibility and rollback:
feat(context_engine): compacting history strategy with source refs and checkpoints #1031 (merged) also changes
raven/session/manager.py: it adds acaller of
_scan_fileand changes neither its return shape nor thesilent-turn rule.
after_sendhooks now also run on a silent turn withoutbound_content=None; an out-of-tree hook that assumes a stringmust accept None.
The new fields (
silentandreplied_via_toolon stored rows and onTranscriptMessage,no_outputon failures, thesilent_turnnoticekind) are optional; an older Raven reading a session written by this
one shows a marker row's text as a reply.
New config key
agents.defaults.emptyRetryCostThresholdUsd(default 0.25). Rollback is a revert of the squash commit; there is
no data migration.
Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
Related Issues
N/A. Supersedes #1052 (closed unmerged).