Repository navigation
Conversation
|
Windows compatibility review — PR #1133 @ Scope: full base Result: no Windows-specific issue. The changes are protocol and ledger logic: message text selection, dict slots, numeric normalisation. No path, filesystem, subprocess, signal, encoding, or platform primitive is added. Verification (native Windows 11, CPython 3.12.13, isolated worktrees The failing-ID sets are identical (diffed — no additions, no removals): the known Windows ACP/spawn fixtures present on main. Limitations: no real |
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 full github/main...HEAD diff and the relevant ACP collector/refusal paths, remote resume path, RPC runner/sink lifecycle, cron and session-wake callers, shared usage normalization, session marker readers/writers/clearers, and the #1121 history. I also checked AGENTS.md, CLAUDE.md, and CONTEXT-MAP.md/CONTEXT.md; backward compatibility; the additive test changes and stub behavior; and found no weakened tests. The change keeps wire and stored field names stable and the behavioral fixes match their callers.
Verification: uv run pytest over the 13 focused ACP, RPC, agent-loop, session, gateway, and wire-shape files passed (1222 passed). The first attempt did not reach tests because a fresh partial uv environment lacked the workspace raven_everos package; after uv sync --all-packages --all-groups, the identical test command passed. Ruff check, Ruff format check, source-language check, and git diff --check also passed on the changed files/diff.
The refusal check read the turn's whole text, every agent message joined. Copilot's refusals are matched by their opening and a short length, so a turn that narrated before the refused call never matched, and the refusal came back as the run's result. The check now reads the last message, the one handed to the caller, here and on the remote pick-up path; the empty-turn check still reads the whole turn.
A message.complete reports the lane's usage slot. A cron or wake turn never wrote it, and a turn that does not own the lane never clears it, so a wake after a sub-agent result resent that result's usage and cost. An ACP client then added the same cost to the session's running total twice, and every other client was shown the stale numbers. The cron turn now passes a usage sink like any other turn and stashes what it spent.
The turn is booked before the empty-turn and refusal checks fail it, and only a comment held that order. The stub's empty prompts now report the usage they were given, so both failures run through a real backend and the ledger is read back.
output_limit_turn_at and budget_stop_turn_at were spelled out by the loop that writes them, the ACP server that reads them and the session that clears them. They are now constants in raven.session.manager; the stored names are unchanged, and the session tests still spell them out.
The ACP client's _counted and _turn_cost and the server's _usd re-decided what a reported amount and a token count are. They now use reported_cost and token_count. The difference was the finite check: an inf amount passed all three and a nan passed the client's, and either one as a session's spend baseline left every later turn of that session unpriced.
|
Merge conflict: @LivXue, this PR no longer merges onto the current
The earlier A on this PR was graded on a merge onto |
|
Blocking: rebase onto current main and resolve the turn_path.py conflict before merge. I independently confirmed the new mergeability report: Baseline verification on the existing head still passes: |
bfda01b to
433cba8
Compare
|
Rebased onto main (959cd0a) to resolve the one conflict with #1092 in |
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.
The prior merge-conflict block is resolved. I reviewed the rebase delta and conflict resolution against current main (959cd0a9), checked git range-diff across all five commits, and verified the merged tree is clean. Four commits are patch-identical; the marker refactor preserves main's narrower budget_stopped condition and changes only the metadata-key spelling to BUDGET_STOP_TURN_AT. I also rechecked the affected callers, compatibility of the stored keys, the unchanged/additive tests, and the repository rules and architecture context covered in the earlier review.
Verification: the 13-file focused suite completed with 1,644 passed and one pre-existing timing-sensitive ACP cancellation test failure; that untouched test passed immediately in isolation (1 passed). Ruff check, Ruff format check, source-language check, and git diff --check passed. GitHub reports this head mergeable.
|
Windows compatibility re-review — PR #1133 @ Scope: the branch is rebased onto current main; the PR's own diff at this head was taken against its new merge-base Result: no Windows-specific defect introduced. The change set is ACP usage/refusal accounting — Copilot-refusal reading, wake-turn usage, cost/count rules centralised in Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun; these files use the ACP stub rather than live agent binaries. |
Summary
Follow-up to #1121, which merged before its last review round landed. The acceptance pass on #1121 raised five points; this takes all five.
collector.reply, the last message, which is what the caller receives, on the local and the remote pick-up path. The empty-turn check still reads the whole turn.message.completereports the lane's usage slot in the RPC spine. A cron or wake turn never wrote that slot, and a turn that does not own the lane, such as a sub-agent result re-entering the session, never clears it, so the next wake resent that turn's usage and cost. An ACP client of Raven added the same cost to the session's running total a second time, and the TUI and web footers were shown the stale numbers. The CRON branch ofRpcTurnRunner.runnow passes ausage_sinklike every other turn and stashes what it spent. Deduplicating by turn id in the ACP translator would not have caught this: the wake'smessage.completecarries the wake's own turn id._countedand_turn_costand the server's_usdre-decided what a reported amount and a token count are; they now usereported_costandtoken_countfromraven/providers/usage.py. That closes a gap: aninfamount passed all three, and ananpassed the client's check becauseamount < 0is false for nan. Either one as a session's spend baseline left every later turn of that session unpriced.raven.session.manager(OUTPUT_LIMIT_TURN_AT,BUDGET_STOP_TURN_AT,TURN_MARKERS) and used by the loop that writes them, the ACP server that reads them and the session that clears them. The stored names are unchanged, and the session tests still spell them out.Known gap, deliberately not fixed here: the running cost counts completed turns. A cancelled or failed turn, and a priced call outside the turn's decide step, reach the usage record but not the running total until the next
session/loadorsession/resumereads it again.Type
Verification
Test first. On this branch's base, with the source changes reverted and the tests kept, six of the new cases fail: the two refusal cases (local dispatch and remote pick-up), the wake usage case and three non-finite cost cases. The other six pass by design: the two guards that a refused or empty turn still books its spend, the case that a turn run without a stream fills its usage sink, and three amounts that were already turned away.
Mutants, each killed: booking skipped for an empty or refused turn (both guard cases fail), the wake turn's usage write dropped (the wake case fails, from the runner through the sink to the
message.completepayload).Full suite,
python -m pytest -q -n 4with the branch's own venv (lockfile identical to main), on the head rebased onto 959cd0a: 36892 passed, 9 failed, 255 skipped. None of the 9 is this change's: 8 (provider proxy redaction, the node-dir resolver of the install script, a legacy Excel workbook, the node runtime reader) fail with the same IDs on unmodified main; andtest_an_empty_message_file_is_a_usage_errorfails or passes with the length of the temp path its error panel wraps, and passes under a short--basetempon this branch and on main.Focused:
tests/test_subagent_github_copilot.py,test_remote_resume.py,test_rpc_spine.py,test_agent_loop_usage_sink.py,test_subagent_acp.py,test_acp_updates.py,test_acp_methods.py,test_session_manager.py,test_agent_loop_max_iter_synthesis.py,test_agent_loop_empty_recovery.py,test_gateway_spine.py,test_rpc_turn_send.py,test_turn_wire_shape_conformance.pypass.ruff check,ruff format --checkandty checkon the changed files (ty reports 5 diagnostics, all on lines oftests/test_subagent_acp.pythis branch does not touch);lint-imports(10 contracts kept);scripts/check_commit_messages.py,scripts/check_large_files.pyandscripts/check_source_language.pyover the branch: all pass.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
User-visible changes: a Copilot run whose last message is its provider's refusal now fails even when it narrated first; after a wake turn, every RPC client is sent that turn's own usage rather than the turn before's, so a wake whose provider reported no usage shows zeros, as any other such turn does; an ACP client's running cost no longer counts the turn before a wake twice; a reported
infornancost is read as unknown. Rollback is reverting the squash commit; nothing is stored in a new shape.Related Issues
#1121