Repository navigation
Conversation
gloryfromca
left a comment
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 full github/main...HEAD diff and found no plain error worth raising. The eight follow-ups preserve one coherent silence protocol: the rule is selected from the prompt the model actually reads, cut-answer continuations retain already-streamed work, tool-delivered replies survive live and replay views, and clients retract only the exact suffix named by the runtime. I specifically tried to refute the new behavior against continuation exceptions, silent wrap-ups, failed message sends, file deliveries, runtime/user turn boundaries, partial client joins, and persisted session/ACP replay.
Coverage included AGENTS.md and the Runtime/TUI/Web context contracts, the diff, affected callers, the #1092 review history and this branch's commit history, backward compatibility of the RPC notice and transcript shapes, test changes for weakening, and the kernel/layer architecture. git merge-tree --write-tree HEAD github/main is clean and GitHub reports the revision mergeable.
Verification at 3f5805d94446: 1,523 changed Python tests passed; focused web tests passed 283/283 and focused TUI tests passed 56/56. Python Ruff and targeted type checks, both UI type checks, generated RPC checks, changed-file ESLint, all 10 import contracts, commit-message/source-language/large-file gates, and git diff --check passed. A plain Python environment first failed setup because the EverOS dev plugin was absent; rerunning with the repository's dev extra produced the 1,523 passing result. An attempted --all-extras environment could not build the unrelated optional datrie segmenter because this host has no gcc, so no all-extras test result is claimed.
|
Windows compatibility review — PR #1146 @ Scope: the full PR diff at this head against base Result: the one directly Windows-facing change in this diff is correct in direction.
Verification on native Windows 11, detached worktree
Coverage gaps: the ui-tui/ui-web test files in this PR were not run — the shared |
|
Not a blocker - 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, all non-blocking:
Tests here on the merged tree: the changed Python test files via the board runner gave 1523 passed, and |
The scheduler read no_output with plain attribute access while filing a turn's failure, right below full_detail_of, which reads the instance dictionary because a foreign getter can throw. A runner's AnswerlessTurnError subclass that skipped the base __init__, or guarded no_output with a getter that throws, made the failure branch raise itself: no TurnFailed went out and the lane's worker died. no_output_of reads it the way full_detail_of reads its field, and anything but True means no. A test files a failure for both subclass shapes.
The session list read every transcript as plain UTF-8. A file a Windows editor saved with a byte order mark put the mark in front of the metadata line, which then failed to parse and took the session off the list; one saved in a Windows code page raised UnicodeDecodeError out of the scan and failed the listing as a whole. The scan now reads utf-8-sig, and leaves an undecodable transcript out as it already leaves out an unreadable one. The ACP replay tests resolve their working directory under tmp_path, so the paths they expect are the ones the replay builds on Windows too.
The session list and the ACP replay take back whole a turn the runtime opened and the model closed with a Silence Marker the message tool did not answer. A question that turn could not put to anyone was still filed as a notice after the marker, and it outlived the take-back: an orphan question row in the replay, and a session list moved by a turn it otherwise hides. Such a turn now files none, by the rule those two readers apply: the last user row before the marker carries an origin. A person's silent turn, which both keep, still files its notice.
A cut answer's continuation can fail by raising rather than by answering with an error: LiteLLMProvider.chat_stream raises its HTTP errors once the stream's own reconnects are spent. The loop let that escape and failed the whole turn, though the reader already held the answer so far. It now ends the turn on the partial with PARTIAL_STREAM_NOTE, as interrupted, as it does for a continuation that answered with an error. A call that raises with no cut behind it still fails the turn, and a cancel still propagates.
On a proactive turn the silence gate holds the whole call, so a cut partial was never shown. A continuation that was the marker alone was still dropped as one that adds nothing, which left the held note to be delivered as the turn's answer. That drop rests on the reader already holding what streamed, which is true only of a turn that showed it. Where it did not, the marker is read with what came before it, on its own line, as the same reply uncut would be.
MessageTool counted a send as answering the turn only when it named the turn's own channel and chat. The send callback never sees a target, though: it hands every message to the turn's own reader. A send that named another chat reached the reader and was still read as no answer, and a silent runtime turn whose reply was a file delivery looked unanswered too, so the session list and the ACP replay took back turns whose reply the reader had seen. sent_in_turn now reports any send that went out, and a turn whose tools delivered files is stamped replied_via_tool as well. A send that failed still answers nobody. The wire description of replied_via_tool says so, and the generated clients are regenerated.
Every proactive turn read the loose marker rule, but only the cron reminder and a sub-agent's unprompted report teach the model the marker. A heartbeat task, a relayed sub-agent result and a sentinel notice never name it, so a reply that was, or ended on, "No reply" or "Silent" was swallowed there. The rule is now picked by the origin and by whether the prompt the model read carries SILENCE_INSTRUCTION (teaches_silence). A taught proactive turn reads the loose rule and holds its whole call; every other turn honours only the bracketed [SILENT] and streams as a person's does. The lesson is read off the prompt rather than a flag beside it, so an entry that teaches the marker cannot forget to say so.
A silent turn's notice told a client to take back "the reply", and the web page found it at the step's last tool row. A call whose only tool was a message that went nowhere draws no tool row, so an exhausted turn's wrap-up erased the step's narration with it. The TUI cleared its whole stream buffer, so narration that no tool call had flushed went too. The loop now counts what the silence gates put in front of the reader in the step drawn now -- one call and its cut answer's continuations, or the wrap-up alone -- and the SILENT_TURN notice carries it as its detail (LoopOutcome.shown). The web page and the TUI take back exactly that suffix of the step's text and keep everything before it, and the line they draw no longer shows the detail. ACP writes nothing for the notice, as before.
Catching UnicodeDecodeError inside _scan_file kept a transcript saved in a Windows code page out of the session list, but the save path and append_metadata_patch read through the same scan. A turn on such a transcript then took the file for an empty one: the save appended UTF-8 lines no reader could open, so the turn was lost and nothing said so, and append_metadata_patch reported nothing to patch. Before the catch the save raised and wrote nothing. The catch moves to the two readers that only list, the session list and find_most_recent_chat_id, through _scan_listed. A save or a metadata patch onto such a file raises again and leaves it as it was.
The session list reads a transcript as utf-8-sig, but the loader (read_session_file) and _stored_key still read plain UTF-8. A file a Windows editor saved with a byte order mark was listed with its title and archive flag, opened without them because its metadata line did not parse, and the first save then wrote over both. _stored_key found no key in it, so session_path would hand the file to a session of any channel with the same chat id. Both now read utf-8-sig, as the list does. A file with no mark reads as before.
A turn whose tools delivered files is stamped replied_via_tool, so
the session list and the ACP replay keep it as answered. Live, a turn
that delivered files and then ended on the silence marker was still a
Silent Turn: a SILENT_TURN notice went out and TurnOutcome.silent was
true. Cron therefore left it out of silent_fire_limit and the
sentinel ledger, as if it had sent the user nothing.
Such a turn is now answered live as well: no SILENT_TURN notice,
TurnOutcome.silent false, and explicit_reply true, which _run_turn
learns from the silence sink ("delivered_files"), so cron counts it
and does not hand it to heartbeat as a run that said nothing. The
files already went out with the call that delivered them, so a turn
that delivers and then answers in words still sends the words.
CONTEXT.md put a continuation that raises in the same sentence as one that answers with an error, which ends the turn "with no wait". A raising continuation ends on the partial too, but only after stream_llm_call has spent its reconnects and, on a failure it still reads as transient, waited out llm_error_retry_delays (15, 30 and 60 seconds by default), with no retry notice shown. The entry now says so in a sentence of its own.
Three readers decided which turns are taken back whole: the session list over stored rows, the ACP replay over wire rows, and the live turn, which skips the unanswered-question notice after a marker that both stored readers drop. Each re-derived the rule, and they already disagreed on one shape: the replay closed a turn at its marker, while the session list and the live turn kept it open, so a second marker after a row in between took that row back from the list but not from the replay. The rule is now TakeBack in session/turn_shape.py, which reads a transcript row by row and says where a turn that can be taken back opens and where one is taken back. All three readers use it; the live turn hands it the rows as they will be stored, without the recovery nudges that never are. A marker closes its turn for all three. A test feeds the same turns to the three readers, with and without a nudge and with the opening row filed by an earlier write, and holds their answers equal.
3f5805d to
755e2e8
Compare
|
@0xKT thanks for the acceptance pass. All five are addressed on this
Verification on the new head: the full Python suite 37195 passed, with Could you take another look at the new head, and approve it if it |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the new revision after its rebase. git range-diff shows the eight previously assessed commits replayed unchanged; I reviewed the five added commits and tried to refute each fix against its callers and stored/live shapes. The code-page catch is now confined to list-only readers, BOM handling is consistent across metadata lookup and session loading, file delivery produces the same answered verdict live and on disk, the continuation-wait documentation matches the provider ladder, and TakeBack gives the live save, session list, and ACP replay one closing rule. I found no concrete regression.
Coverage included the repository rules and Runtime context, the delta and affected callers, rebase/commit history, backward compatibility of transcript and turn outcomes, test changes for weakening, and the layer architecture. Verification at 755e2e868010: 774 focused session/replay/agent-loop tests passed; Ruff, formatting, targeted Python types, all 10 import contracts, commit-message/source-language/large-file gates, git diff --check, and merge-tree passed. GitHub reports the current head mergeable. No review threads from my account need resolution.
|
Windows compatibility re-review — PR #1146 @ Scope: the branch is rebased onto current main (which carries #1141); the delta since the previously reviewed head
Windows angle: the pair is right for this platform. BOM handling is now uniform (every read path uses Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun, and this PR touches no UI. |
Summary
Follow-up to #1092, which merged before its last review round was
answered. This settles the seven non-blocking findings of that round's
delta acceptance on the merged head, the Windows note from the Windows
review, and the five non-blocking findings of this PR's own acceptance
review, one commit per finding:
no_outputoff the instance. A runner'sAnswerlessTurnErrorsubclass that skips the base__init__, orguards
no_outputwith a property that raises, made the scheduler'sfailure branch raise itself, so no
TurnFailedwent out.no_output_ofreads the instance dictionary the wayfull_detail_ofalready does.cron reminder and a sub-agent's unprompted report teach
SILENCE_INSTRUCTION; a heartbeat task, a relayed sub-agent resultand a sentinel notice never do, yet swallowed a reply that was, or
ended on,
No replyorSilent. The rule is now picked by theorigin and by whether the prompt the model read carries the
instruction (
teaches_silence). Every other turn honours only thebracketed
[SILENT]and streams as a person's turn does.proactive turn nothing of a cut partial was shown, so a continuation
that is the marker alone is read with what came before it, on its own
line, as the uncut reply would be, instead of leaving the note to be
delivered.
PARTIAL_STREAM_NOTE, as one that answers with an error already did.A call that raises with no cut behind it still fails the turn. It
ends there once the stream's own retries are spent: its reconnects
and, on a failure it reads as transient,
llm_error_retry_delays(15, 30 and 60 seconds by default), which
CONTEXT.mdnow spellsout.
tool's send callback never sees a target: it hands every message to
the turn's own reader. So
sent_in_turnnow reports any send thatwent out, and a turn whose tools delivered files is stamped
replied_via_tooltoo. The session list and the ACP replay keep both.Live, a turn that delivered files and then ended on the marker is no
Silent Turn either: no
silent_turnnotice,TurnOutcome.silentfalse and
explicit_replytrue, so cron counts it towardsilent_fire_limit, records it for the sentinel, and does not handit to heartbeat as a run that said nothing.
TakeBackinsession/turn_shape.py, read by the session list, the ACP replay andthe live save; a test feeds the same turns to all three and holds
their answers equal. A turn it takes back files no
unanswered-question notice, so no orphan question row is left behind
it, and a person's silent turn still files its notice. Sharing the
rule ends one disagreement: the replay closed a turn at its marker
and the list did not, so a row between that marker and a second one
was drawn by the replay and hidden from the list. Both now keep it.
silent_turnnotice'sdetailnow carries the text the reply readas the marker had already streamed (a think block it inlined ahead of
it), counted over the step the reader draws it in. The web page and
the TUI strip exactly that suffix and keep the narration before it;
the web page used to cut at the step's last tool row, which a
message-only call does not draw, and the TUI cleared its whole stream
buffer.
session list, the loader and
_stored_keyreadutf-8-sig, so a BOMtranscript is listed, opens with its title and archive flag, and
claims its key. The list leaves an undecodable file out instead of
failing; a save or a metadata patch onto such a file still raises and
writes nothing, rather than appending UTF-8 lines no reader could
open. The ACP replay tests build their expected paths under
tmp_path.Existing tests whose meaning changed, each with its reason in the diff:
the loose-rule mechanics tests now run a taught turn; the TUI and web
take-back tests now send the detail the server would send; a send to
another chat is now an answer; a refused continuation now ends on its
words (the broken-turn test was split into its cancel and refusal
cases); the code-page listing test now also asks
find_most_recent_chat_id.Not changed:
{channel}:{chat_id}" for a send whose callback ignores the target.
That predates feat(*): align empty-reply recovery with Hermes and let a turn end in silence #1092 and is outside these findings.
unanswered-question notice, so the live web page and TUI, which draw
that turn until a reload, and the next turn's model do not get the
question either. Filing it and dropping it with the turn in the two
stored readers would keep it for them; that was left out.
the sentinel's
daily_analysis.pyandcurrently_focused.pyand thebehaviors extractor, still open a transcript as plain UTF-8: a BOM
costs them the metadata key, which they then take from the path, and
a code-page file raises out of the read. That predates feat(*): align empty-reply recovery with Hermes and let a turn end in silence #1092 and is
outside these findings.
Interplay with the open #1099, which routes a send naming another chat
to that chat: with it, such a send returns before the callback, so it
still does not count as the turn's answer, and the two compose. They
touch the same lines of
MessageTool.execute, and whichever landssecond also drops "whatever chat the call names" from the
sent_in_turndocstring and theCONTEXT.mdentry.Type
Verification
On a 32-core Linux box, at the head commit rebased onto
mainat7c12b93feunless a line says otherwise:uv run --frozen --all-extras pytest -q -n 4: 37195 passed, 8failed, the same 8 as before the rebase, all local to this box and
none in a module this diff changes: five
test_config_update_providers.pytests read the box proxy (the filepasses 200/200 with the proxy variables unset),
test_install_script.pyfinds an npm on PATH,test_subagent_node_runtime.pyruns as root, andtest_knowledge_parser_excel.pygets no.xlsfrom the localLibreOffice.
The 12 changed Python test files under the CI coverage flags with
--idle-ceiling-strict, serially: 1551 passed, no idle-ceiling hit;every executable line the last five commits add is covered.
Each commit with the tests its files reach. The first eight, alone,
before the rebase onto
3ba6179fb(whichgit range-diffshowsreplayed every commit unchanged, as it does onto
7c12b93fe): 54,199, 504, 475, 477, 1172, 974 passed, and the eighth was the head
then. The last five, each on top of the ones before it: 426, 594,
863, 4 and 907 passed.
ui-web:
npx vitest run4520 passed (262 files);npx vitest run --no-file-parallelism scripts/gates257 passed;npm run type-checkandeslinton the changed files clean.ui-tui:
npm test2275 passed, 2 failed inmodelPicker.test.tsxand
newInstancePicker.test.tsx, both 38/38 when rerun alone(full-run load);
npm run type-checkandeslinton the changedfiles clean.
node scripts/gen-rpc-client.mjs --check(ui-web) andnode scripts/gen-rpc-types.mjs --check(ui-tui): in sync.make lint-typesclean;make lint-imports10 kept; ruff check andformat clean.
scripts/check_commit_messages.py origin/main..HEAD,scripts/check_large_files.pyandscripts/check_source_language.pyover
origin/main...HEAD: clean (the last beside a control that addsone CJK comment to a
.pyfile and fails).Every fix was watched failing before its change, and each new guard
was mutated after it: 33 mutants for the first eight commits across
the loop, the web page and the TUI, and 16 for the last five, each
killed by at least one test.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
A heartbeat task, a relayed sub-agent result or a sentinel notice that
answers
No reply.now reaches the reader, and its stream is nolonger held until the call ends. Only an exact
[SILENT]keeps itsilent.
A message-tool send that names another chat now counts as the turn's
answer; it always reached the turn's own reader.
The
silent_turnnotice'sdetailwas always empty and now carriestext. The bundled web page and TUI do not draw it; a third-party
client that draws every notice's detail under its label would show
the think block there.
teaches_silencereads the whole prompt, so a relayed result thatquotes
SILENCE_INSTRUCTIONmakes that turn read the loose rule. Thatcan only withhold a reply the model shaped as a marker, which text in
the prompt could already ask the model for.
A cron run that delivers files and then ends on
[SILENT]nowcounts toward
silent_fire_limitand is recorded for the sentinel,as one that sent a message already was.
A save or a metadata patch onto a transcript saved in a Windows code
page raises, as it does on
main, rather than losing the turn.Each finding is its own commit, so any one can be reverted alone.
Related Issues
N/A