Repository navigation
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the TUI must recover a reassigned node's current agent and handle from completion data.
I found one concrete correctness issue in the TUI live-agent store. I otherwise covered the repository rules and canonical terms, the complete diff, relevant runtime/RPC/UI callers, both commits' history, backward-compatible absence semantics for unreassigned runs, test-strength changes, and the reassignment architecture across admission, staging, handover, persistence, reload, and client event ordering.
Verification: uv run pytest tests/test_subagent_dag_handover.py tests/test_subagent_dag_runner.py tests/test_subagent_dag_control_tools.py tests/test_subagent_instance_registry_reload.py tests/test_rpc_schema_match.py tests/test_unified_playbook.py -x -q (1226 passed); the remaining changed Python test modules (977 passed); changed TUI tests (339 passed) plus TUI type-check; changed web tests (265 passed) plus web type-check. The first Python attempt failed during fixture setup because the fresh environment lacked the dev extra; after uv sync --extra dev, both Python runs passed.
|
Windows compatibility review — PR #1118 @ Scope: full base One Windows finding (medium, test-level; root cause inherited). 7 new tests fail on Windows because the inherited DAG ref-path containment refuses every workdir reference on NTFSFiles: the seven tests in Affected scenario: any DAG node whose prompt template carries Mechanism: Evidence (native Windows 11, CPython 3.12.13): a direct probe of the same tool setup returns Same nine test files, head vs base: The new Scope of the defect: the containment bug is inherited from the base (its ref-path tests are in the base 16 failures and PR #1066, "resolve DAG file references by the host's path rules", is the open fix). This PR does not introduce it, but it adds the first tests that depend on a path-using node running, and they cannot pass on Windows until #1066 lands. Remedy direction: make Checked, no separate Windows defect in the new code: Verification commands and results (isolated worktrees Limitations: the Windows |
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 complete delta from 2bad12f004be to this revision. The completion and list-reconciliation paths now recover a reassigned node's agent and handle with correct null-versus-absent semantics, and focused regressions cover both paths. I also checked the surrounding row merge behavior, backward compatibility for entries without reassignment fields, the unchanged-status reconciliation case, test changes, and the prior finding's concrete reproduction.
Verification: npm test --prefix ui-tui -- --run src/__tests__/liveAgentsStore.test.ts (43 passed) and npm run type-check --prefix ui-tui (passed). The blocker thread is resolved.
|
Windows compatibility re-review — PR #1118 @ Scope: delta from the previously reviewed head Result: the Windows finding from the
The same seven path-gate tests fail, at the same place: Cross-reference: PR #1066 at Limitations: the TUI delta cannot be unit-run in this checkout ( |
There was a problem hiding this comment.
Blocking: the handover puts the source sub-agent's answers into the destination agent's first prompt unfenced, framed as that agent's own earlier conversation.
render_handover builds answers = {i: escape_handover_text(turn.assistant)} (raven/agent/subagent/dag_handover.py:254) and emits them as bare Assistant: ... lines (:216-217). The block is wrapped only in a raven-minted <handover> tag and a trailer telling the destination "you are now picking up where it left off" (:286-295). Those answers are sub-agent-authored by construction: instance-log assistant content (:81-84) or the node's attempt output files (:124-127). Neither dag_handover.py nor dag_runner.py calls wrap_untrusted. escape_handover_text (:41-50) only stops the text from impersonating a role label or a <handover> tag, and the PR's own CONTEXT.md entry says as much: "that escape is a framing guard, not a sanitizer for instructions inside the text".
The repo's rule for this channel already exists:
- CONTEXT.md, Memory record: sub-agent text "must run ... through
wrap_untrusted(raven/security/trust.py) before it reaches another sub-agent's prompt, the same as any other sub-agent-controlled content." - CONTEXT.md, The fence rule: "Another node's output ... is wrapped with
source="subagent"... a node's output is sub-agent-authored by construction" (prompt_render.py:39does this for{{ X.output }}). raven/agent/subagent/direct_chat.py:290-298: an unwrapped prepend is safe "only because every byte here is raven-minted, never sub-agent output ... A future field that echoes a sub-agent's own reply text ... must not be added here."raven/agent/subagent/manager.py:2197-2202fences a sub-agent's result before it reaches the main agent, because it "may have fetched web pages / read files".
Why it matters here: a source agent that was steered by something it read can write instructions that land in the destination's user message with full trust, presented as the destination's own reasoning. The bundled skill tells the main agent to reassign exactly when "the sub-agent lacks what the task needs -- a tool, access to the files" (subagent-dag-orchestration/SKILL.md:177). So the destination is often the more capable agent.
A fix that keeps the design: fence the carried sub-agent answers, and any nested earlier handover carried inside a user row, with wrap_untrusted(..., source="subagent"). Leave the node's task, the person's and the main agent's words, and the raven-minted trailer unfenced. Fencing the whole block would turn the anchor node's task into data. A test in tests/test_subagent_dag_handover.py asserting that a carried answer appears inside the untrusted fence pins it.
Read only; nothing executed.
There was a problem hiding this comment.
Fenced, in d62e293 (rebased onto main 26352da).
Exactly the shape you asked for:
- every carried sub-agent answer is wrapped with
wrap_untrusted(..., source="subagent"), so the destination reads it as data from another sub-agent, not as its own earlier reasoning; - a nested earlier handover travelling inside a user row is fenced too: the move is transitive, and that inner body is sub-agent text as well;
- the node's task, the person's words, the main agent's
messageand the raven-minted trailer stay unfenced, so the anchor task is not turned into data; - the existing marker escaping stays, as the framing guard it is, on top of the fence rather than instead of it;
- the budget reserves the fence length before shortening, so a fitted handover stays inside the limit.
Tests, written first and red against the unfenced implementation: one asserts a carried answer appears inside the untrusted fence, one asserts the task and the main agent's message do not.
Verification at this head: the ten F4 suites 1954 passed across three groups; ruff check and format clean on 34 changed files; the three generator checks exit 0 with no diff; tests/test_agent_loop_token_budget.py 6 passed.
Leaving the thread open for you to verify and resolve.
|
Not a blocker -- other findings from the same pass, separate from the blocking thread. Each was read at this head; none was executed. 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.
|
|
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. |
f2e6df8 to
d62e293
Compare
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 rebased revision by range-diffing it against the previously reviewed head, then reading the two substantive follow-up commits and their callers. The old commits replay cleanly; the new handover path fences carried sub-agent answers and nested handovers while leaving the task and main-agent instruction trusted, and its budget accounts for the fence. I also checked the adjudication, moved-group, backend/MCP, admission-refusal, multi-hop history, assignment-reader, RPC, and TUI identity changes.
Covered the repository rules (AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and the relevant Runtime/TUI context), the diff and callers, commit/range history, backward-compatible reader fallbacks, test changes for weakening, and the documented Worker Table/Charter/trust-boundary constraints. I found no new ordinary-use failure. My earlier TUI thread remains resolved; I did not alter another reviewer's open thread.
Verification: 904 focused Python tests passed; the TUI store suite passed 43 tests and TUI type-check passed; source-language and large-file checks passed via their underlying scripts (make is unavailable here); git diff --check passed; and git merge-tree --write-tree HEAD github/main completed cleanly.
|
Not a blocker items and the merge conflict -- all addressed in
Merge conflict: rebased onto Verification at the rebased head: the changed suites (handover, runner, |
|
Windows compatibility re-review — PR #1118 @ Scope: the head moved from the previously reviewed Result: the Windows finding I reported at The seven path-gate tests that timed out on Windows (valid workdir references refused before dispatch) now pass, because the branch's confinement guard resolves and compares paths with the host's rules: That covers No new Windows-specific code appears in the delta beyond the confinement fix; the TUI changes in the raw diff are main's merges. Limitations: no live gateway or real Telegram/ACP process was run; the TUI delta was not unit-run in this checkout. |
|
Merge conflict: @LivXue, this PR no longer merges onto the current
The judge's verdict on |
|
Blocking: rebase onto current main and resolve the conflict in raven/rpc/methods/tasks.py. I independently confirmed the new integration state: |
A suspended DAG node can now be reassigned: the main agent picks another sub-agent from the roster, the next attempt runs on it, and the run continues instead of being wound down and re-submitted. The node keeps its id, task, dependencies and place in the graph; every attempt is preserved and the continuation budget is shared, not reset. The new agent's first prompt carries a handover block: the instance's visible conversation, or the node's own archived attempts when there is no instance. It excludes tool calls, tool output and private model context, and the message the main agent writes travels as the ordinary follow-up, so the judge still reads the original task plus recorded follow-ups. Admission is explicit: roster membership, not the current agent, local-file capability for every node that moves, a stateful target for an originally shared instance, and an unoccupied destination handle. A confirm run asks again before the move. The scheduler moves pending and suspended nodes, leaves completed ones where they ran, plants the handover once per destination, and keeps the global concurrency slot free while it waits for the source attempt to settle. Records follow the agent that actually ran: the registry row moves atomically at attempt start, readers agree before and after finalization and after a restart, and both UIs show the attempt history. A successful reassigned run captures to a Playbook on the agents that finished it while the audit keeps the factual history. The submitted graph is never modified. Node judgement stays always on; the exception report names the attempt that ended, lists earlier attempts with bounded reasons, and teaches the four recovery choices against the remaining budget.
A partial MCP grant on a reassigned node was dropped: _target_backend built the same dispatch backend as _preflight but discarded the grant's note, so a server that could not be resolved cost the node its tools silently. It now returns the note, the execution helper adds it to the plan's notices, and the installer logs it instead of swallowing it. Moving a group onto a pair that already stands for another group replaced its membership instead of adding to it. A graph may name one handle on two agents, so the target pair can already belong to a group whose members must keep following it through later hops; the write is now a union.
Identity was folded only on dag.node_updated, so a client that missed the moved attempt's frame and received only the run manifest kept the old agent and handle for good. The completion fold now applies the same presence semantics the update branch uses: subagent only when truthy, instance only when the key exists, so a present null clears the handle. The list snapshot had the same gap: it settled a row's status but never repaired its identity. Reconcile now repairs an existing row from the authoritative item on every poll, independently of the status transition.
The carried turns are sub-agent output by construction, and the handover emitted their answers as bare Assistant: lines inside a raven-minted tag. A steered source agent could therefore write instructions that read as the destination's own earlier reasoning. Fence each carried answer with wrap_untrusted(source="subagent"), and fence the body of a handover nested inside a carried user row, which is sub-agent text for the same reason. The task, the person's and the main agent's words, and the raven-minted trailer stay outside the fence; the existing marker escaping stays as the framing guard on top of it. The budget reserves the fence's length before shortening the newest answer, and raven's own "no answer" placeholders are left unfenced.
The handover fence is the blocking item; the remaining review findings on the reassignment flow are closed here: - apply the other answers that land in the same adjudication batch as a reassignment (a sibling's continue or abandon no longer waits for the moved node's next attempt), through one helper shared with the ordinary path. - retarget every graph-named group a move actually takes, and union the destination pair, so a group the move joins keeps following it through later hops. - build the destination backend through the same helper submission uses, and enter the run's MCP scope for the admission dry run, so both admission paths agree and a playbook's own servers resolve. - refuse at admission a move whose handover could never render, instead of committing it and failing every later attempt. - read a moved node's exchanges from its per-attempt archives, and read a second hop's instance log for the attempts that ran on it, so one agent's answer is never shown as another's turn. - resolve the current agent, handle and attempt history through one helper for every reader, and compare instance-card agent names canonically. - document the Worker Table exception and the fence in CONTEXT.md and the DAG skill. Tests cover each finding, including the handover fence and its interaction with a second hop's archived prompt.
The rebase onto main brought #1131's removal of tool_failure_count into the contract and the node fixture builder, but two expectations this branch adds still listed the field, which TaskNode no longer carries.
d62e293 to
0d0f1b7
Compare
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 rebase by range-diffing it against d62e293ab52b and inspecting the resolved raven/rpc/methods/tasks.py callers and the added web-test adjustment. The feature commits otherwise replay unchanged. The resolution keeps the reassignment identity/history reader while adopting current main's removal of tool_failure_count; the follow-up commit removes that obsolete field only from the two branch-added expectations. No tests were weakened beyond matching the new schema.
The prior integration blocker is cleared: github/main is the merge base, git merge-tree --write-tree github/main HEAD succeeds, and git diff --check is clean. Verification passed with 904 focused Python tests, 61 affected web live-state tests, and the web TypeScript type-check. I also checked the applicable repository/context rules, caller compatibility, and my thread sweep; my earlier TUI thread remains resolved, and I did not alter another reviewer's thread.
|
Windows compatibility re-review — PR #1118 @ Scope: the branch was force-updated and rebased onto current main; the PR's own diff at this head is Result: no Windows-specific defect introduced by this head. The 5 failures it shows are the known Windows process/path baseline cases and are identical at the base (details below); the new handover state is carried in run records and instance records, not in new subprocess or filesystem primitives. Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun; multi-process DAG execution was exercised through the test harness, not a long live run; the inherited Windows reaping/log-clock failures above are not attributed to this PR. |
Summary
A suspended DAG node can now be handed to another sub-agent and the run keeps
going. Before this, a node whose agent lacked a capability could only be
continued on the same agent, abandoned, or fixed by winding the whole run down
and submitting another graph.
How it works:
run_subagent_dagnodes gain two fields. A decision node (routes, plusselect: one|many) still uses the existing per-node judge to pick a route; aguard (
when) selects which nodes run at all. Those shipped in F1.reassign:resolve_dag_nodeaccepts
decision: "reassign"with anagent(roster enum, never a workerlabel) and a
message. The node keeps its id, task, dependencies and place inthe graph; the next attempt runs on the new agent; the continuation budget is
shared, not reset.
agent, local-file capability for every node that moves, a stateful target for
an originally shared instance, an unoccupied destination handle, and a
handover that can actually be rendered (a task over the 128000-character
budget is refused before the move is committed). A
confirm: truerun asksagain before moving; with no ask channel it refuses rather than inheriting the
permissive default.
conversation, or the node's own archived attempts when there is no instance.
It excludes tool calls, tool output and private model context; the main
agent's message travels as the ordinary follow-up, so the judge still reads
the original task plus the recorded follow-ups. Each carried answer is
sub-agent output and crosses the same nonce-tagged untrusted fence every other
sub-agent output does; a handover carried inside an earlier user row is fenced
the same way, while the task, the person's and the main agent's words stay
unfenced. The escaping is a framing guard on top of that fence, not a
sanitizer.
they ran, plants the handover once per destination handle, keeps the global
concurrency slot free while it waits for the source attempt to settle, and
gates the file-reference rule again for every node the installer builds. A
sibling's answer decided in the same batch as a reassign is applied at once
rather than stranded behind the moved node's next attempt.
this node now" for
dag.get,tasks.list,subagent.listand the instancehistory; the registry row moves atomically at attempt start; both the TUI and
the web task pane show the attempt history. A moved node's instance history is
read from its per-attempt archives, so one agent's answer is never shown as
another's own turn.
it while the audit keeps the factual history; the submitted graph is never
modified.
ended, lists earlier attempts with bounded reasons, and teaches the four
recovery choices against the remaining budget. On a Worker Table run a
reassign names a roster agent rather than a label, so the node leaves the
table and keeps the Charter of the role it was submitted under; CONTEXT.md and
the DAG skill say so.
This branch is now rebased onto
959cd0a9f(main). The one conflict, in_dag_rowinraven/rpc/methods/tasks.py, was against #1131, which removedthe per-node tool-failure tally. The resolution keeps main's reading
(
call_count = _tool_call_count(account)), produces and declares notool_failure_countfield, and keeps this branch's sharedcurrent_assignmentresolver for the node's agent, handle and attempt history. That resolver falls
back to
entry["subagent"] or graph["subagent"]exactly as main's inline chaindid, so a node that never moved reads the same as before. The automatic merge
left two expectations this branch adds in
ui-web/src/features/tasks/live.test.tsstill listing the removed field; they were dropped. The contract, both
generated clients and both type checks were re-run at the new head.
Type
Verification
Run on the rebased branch head
0d0f1b74b(origin/main=959cd0a9f):pytest -q -n 0over the 16 test files this PR changes -> 2226 passed,exit 0. That set includes both tests that pin the two sides of the rebase:
test_a_reassigned_node_reads_back_its_current_agent_and_attempt_history(the
current_assignmentside) andtest_tool_call_count_reads_only_the_listed_calls(the_tool_call_countside); both pass.
pytest -q -n 0over a wider DAG subset (15 files: adjudication, controltools, core, guard, handover, live, MCP scope, runner, skills, verdict,
instance log, instance registry reload, unified playbook, playbook e2e and
playbook delegate) -> 1553 passed, exit 0.
vitest runin ui-web onlive.test.ts,TasksPage.test.tsx,nodes.test.ts,graph.test.tsandscripts/gates/fixture-shape.test.mjs-> 265 passed, exit 0.
vitest runin ui-tui ondagCommand,dagPanel,dagRun,dagStatusandliveAgentsStore-> 343 passed, exit 0.tsc --noEmitin ui-web and in ui-tui -> exit 0.npm run gen:check(ui-web) ->generated.ts matches the contract (248 methods), exit 0.npm run lint:rpc(ui-tui) ->generated.ts in sync (sha256: 5bdee73ccfb5), exit 0.gen-i18n.mjs --check-> catalogue up todate, exit 0.
ruff checkandruff format --checkon the 34 changed Python files ->exit 0.
ty check raven-> exit 0.lint-imports-> 10 contracts kept,0 broken.
check_commit_messages.py,check_source_language.pyandcheck_large_files.pyoverorigin/main..HEAD, pluscommitlint->exit 0.
Mutation checks at the authored head
d62e293ab(the rebase changes none ofthe sources they target): nine source mutations (no answer fence, no nested
fence, all-attempts completeness, sibling answers left on the desk, no
oversized-task door, anchor-only group retarget, replace-instead-of-union,
dropped partial MCP notice, flat files for a moved node) each turned its
test red; sources restored.
The coverage gate and the full suite were not re-run on this rebase round:
both need the whole suite, and this rebase adds no source line of its own
beyond resolving
_dag_row. CI runs both on the pushed head.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
Security: the handover carries only what the source agent said, never tool calls
or private context, and every carried answer is fenced as untrusted sub-agent
output. Admission checks run before the confirmation question and are re-read in
one synchronous segment at commit time, so a change during the ask refuses the
move instead of applying it.
Compatibility: a run with no
reassignis unchanged. A base-vs-head A/B runproduced byte-identical summary, files, terminal outputs, manifest, events,
graph, prompts and artifacts after normalizing paths and the per-run untrusted
tag. The only deliberate no-reassign text change is the exception report's new
attempt lines; a handover can only appear on a moved node. New wire fields are
optional;
TaskCountsis untouched.The rebase is backward compatible in the same way:
tool_call_countkeeps themeaning and the null rule main gave it in #1131, and the added
attempt_agentsand
instance_autofields remain optional and present only for a node thatmoved.
Known limits, recorded rather than fixed: a multi-hop middle agent whose
attempt ran on a minted handle cannot be found by the instance history (the
archive is addressed by node and attempt, and nothing records which handle that
attempt owned); a node whose handle was dropped and then moved again captures
the handle that actually finished; handover escaping does not cover vertical
tab, form feed or the Unicode line separators.
Rollback: revert the squashed commit. No migration, no config change, no
stored-format change that older builds cannot read.
Related Issues
N/A