Repository navigation
fix(runtime): preserve Agent Graph results across session revisions - #5512
Conversation
9261617 to
e5a7877
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current seven-file Agent Graph revision change. Copying preserves external child-session references as static snapshots, and Host admission validates child/parent graph, run/turn terminal event, and Artifact ownership. I found no substantiated P0–P3 issue in the inspected copy/admission paths; current-head CI test and merge checks pass. I did not run a real multi-session concurrency/restart, cross-tenant end-to-end, or complete local test suite. Maintainers should validate the higher-impact copy semantics before merging.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
e5a7877 to
32cbb7c
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the current commit 32cbb7c0. The change recognizes persisted agent_output results (including diagnostic views and archived results), validates retained child Run/Graph/Artifact provenance, and copies independent conversations as static snapshots while preserving validated revision references. I did not find a substantiated P0–P3 defect in the reviewed diff. The focused Agent Graph/copy tests passed; one unrelated local managed-Bash sandbox integration test failed in this Linux environment, while the current-head hosted test check passed.\n\nThis PR is not merge-ready against current main (c0787020): the merge tree has content conflicts in packages/runtime-host/src/server/session-revision-graph-references.ts and packages/runtime/src/conversation-copy.ts, where #5757 changed the same linked-child copy mechanism. The resolution needs a fresh review and tests of the combined behavior. I did not run a real Desktop/Electron copy flow.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Fixes edit-and-resend after Agent Graph completion: revision preflight collected linked-child references only from typed subagent/agent_swarm tool results, while real agent_output calls persist JSON envelopes that were never recognized. The issue is real — base conversationCopyLinkedChildReferences (packages/runtime/src/conversation-copy.ts:1008-1038) returns [] for kind === 'json', and the session-revision coordinator (packages/runtime-host/src/server/session-revision-coordinator.ts:404) depends on that collection. The direction is right: a strict envelope recognizer producing reference claims in packages/runtime, validated against retained authority (headers, run ledger, graph identity, terminal event, artifacts) in packages/runtime-host, with Side Conversations receiving identity-stripped snapshots. I verified the recognizer gates against the real producer (readChildAgentOutput, packages/runtime/src/session-manager.ts:3846-3930): view result emits empty side arrays, diagnostic views omit result — both match.
Findings
- [P3] packages/runtime/src/conversation-copy-agent-output.ts:108-115 — the recognizer rejects envelopes where
execution.currentRunId !== invocation.runId. Reading a superseded run viaagent_outputwith an explicit olderrun_idyields such an envelope; it is then silently treated as opaque JSON, so references are missed and the pre-fixoperation_unavailable/lost-linkage behavior persists on that narrow path. Consider accepting the mismatch (the claim is still validated host-side) or documenting the bound. - [P3] packages/runtime-host/src/tests/execution-model-composition.test.ts (new block) — the revision-create retry loop tolerates up to 3
source_revision_conflicts to absorb post-terminal metadata advances. Bounded and mirroring the Desktop client, but it would also mask a regression that always conflicts. A counter assertion (e.g., ≤1 retry in the fixture) would make it mutation-tight. - Test quality otherwise strong: the 1060-line conversation-copy-agent-output suite exercises flipped-condition mutations (malformed identities, wrong graph ids, cross-session artifacts), and the poll-accept rule ("running poll only with a retained terminal result for the same child/run/turn", packages/runtime-host/src/server/session-revision-graph-references.ts:183-250) is covered both accepting and rejecting. The legacy-archive→ledger migration in
cloneModelProjectionTransition(packages/runtime/src/conversation-copy.ts:1400-1460) correctly guards chain adjacency and aliases repeated wrappers.
Verdict
merge-ready — real correctness fix at the right layer, heavily mutation-tested; only conservative-recognizer edge notes. Recommend merging, then tracking the superseded-run read edge as a follow-up.
Recognize child references in persisted JSON agent_output responses, including diagnostic views and archived results, so editing a follow-up after Graph completion can create a revision without losing child results. Validate retained child ownership, Graph and terminal Run identities, and allow historical polls only alongside a terminal result for the same Run. Preserve validated references within the revision family. Copy independent Side Conversations as static snapshots with rewritten Artifact links and reconstructible ledger archives, including subsequent copies. Cover the complete Graph completion, follow-up, edit-and-resend workflow through the production Host, along with reference and archive regressions. Fixes apache#5331 Generated-by: OpenAI Codex
32cbb7c to
ad38a32
Compare
|
Thanks for the reviews. I rebased this PR onto Apache main at b393304 and pushed ad38a32.
|
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head ad38a328a06a816a8e145cf01f1e154bb7fb9bd0. I found no substantiated P0–P3 issue in the changed paths.
The change recognizes persisted JSON agent_output envelopes (including bounded diagnostic views) as child-run reference claims in packages/runtime/src/conversation-copy-agent-output.ts:98-260, then validates retained parent turn, Graph identity, terminal run/event and Artifact lineage before sharing them across revisions in packages/runtime-host/src/server/session-revision-graph-references.ts:143-245. Independent Side Conversations instead receive identity-stripped snapshots with rewritten Artifact references in packages/runtime/src/conversation-copy.ts:1807-1959. The copy path also rebuilds legacy archive transitions and Read addresses for repeated copies (conversation-copy.ts:1294-1377). The current tests cover malformed claims, poll/terminal pairing, ownership rejection, repeated snapshot/archive copies, and a production Host edit-and-resend flow.
Node 24 clean npm ci, build:test, and focused Runtime/Host tests (63/63) pass. The exact-head hosted test check is green; git diff --check and static merge-tree against current main 6e21e611 are clean. I did not run native Electron, manually inspect real provider wire outside the included Host fixture, or reproduce an actual multi-user race. The PR remains GitHub BLOCKED; the technical review is not an approval or a merge decision.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head ad38a328a06a816a8e145cf01f1e154bb7fb9bd0 as an independent second pass. No P0–P2 findings.
Trust boundary holds. Any tool (including MCP/provider tools) can emit envelope-shaped JSON, so the parsed agent_output is correctly treated only as a claim; the Host then requires the child to belong to the source revision family and be spawned in a retained turn, any claimed Graph identity to match the child record, the named run to exist in the child's own history with a matching terminal event/status/lineage (and all child runs finished), and each Artifact to be owned by the child within that lineage. I wrote seven forged-claim tests framed as arbitrary MCP tool results — another family's child, a turn-id collision, an invented run, a foreign Artifact, a running poll paired with a fake terminal claim, a nonexistent child — and all were rejected; a forged snapshot's Artifact ids can only reach the source Session's own Artifacts. Turn ids survive copies, so revising a revision works.
Three P3s inline: a guard that can never fire, a denial-of-copy path from arbitrary tool output, and parser checks with no test coverage (mutation-verified; the Host-side checks are well covered, so impact is low).
Checks run locally: git diff --check, ASF headers, Windows inventory (current), runtime 46/46, runtime-host session-revision 26/26, provider-wire Agent Graph integration 1/1. Not run: full suite, lint/typecheck, Desktop UI.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| request.graph.workId !== parent.graph?.workId || | ||
| request.graph.operatorId !== parent.graph?.operatorId)) || | ||
| (parent.graph !== undefined && | ||
| !referencedGraphs.get(parent.parentSessionId)?.has(parent.graph.graphId)) || |
There was a problem hiding this comment.
P3: referencedGraphs is built from the same claims being validated here, so this condition is always satisfied — replacing it with false leaves all 17 tests passing. Either drop it or replace it with the check that was intended (e.g. against an independently derived set of Graphs referenced by retained turns).
| content: ToolResultContent, | ||
| ): readonly ConversationCopyLinkedChildReference[] { | ||
| if (content.kind === 'json') { | ||
| const output = conversationCopyAgentOutput(content.value); |
There was a problem hiding this comment.
P3 (availability only): any tool result shaped like an agent_output envelope is now treated as a linked-child claim. If it names a nonexistent child, every revision, branch and Side Conversation of that session fails with operation_unavailable permanently (reproduced with an MCP-tool fixture). Deleted-subagent results already fail the same way, so this is not new in kind, but only treating envelopes produced by the agent_output tool as claims would close it.
| execution.kind !== 'child_session' || | ||
| !nonempty(execution.sessionId) || | ||
| !isRecord(invocation) || | ||
| invocation.sessionId !== execution.sessionId || |
There was a problem hiding this comment.
P3: several parser checks are untested — removing any of the invocation/execution session match (here), the terminal-event identity check, the terminal.partial check, events.every(sameRun) (~130-134), or the result view's empty-diagnostics check (~163) still passes all 46 runtime tests. The Host re-validates what matters, so security impact is low, but a couple of targeted cases would keep these from silently regressing.
Resolve agent_output JSON through retained tool-call metadata before collecting child claims or rewriting snapshots. Preserve unrelated tool payloads across transcript, ledger, projection, and repeated archive copies. Remove the redundant Graph membership guard and add isolated parser and copy regressions covering the review findings. Generated-by: OpenAI Codex
|
Thanks for the detailed review. I’ve prepared fixes for all three points:
|
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current four-file increment. Conversation copying now associates transcript and retained ledger tool results with a (turnId, callId) tool name, and treats missing or conflicting provenance as opaque rather than interpreting external JSON as agent_output child references (packages/runtime/src/conversation-copy.ts:1103). The same provenance gates result, snapshot, Artifact, and projection rewrites. The Host still validates child family, Graph identity, and retained spawn Turn; it no longer requires an independent Graph-reference set entry for a child already represented by a verified result (packages/runtime-host/src/server/session-revision-graph-references.ts:150). In the inspected paths, I found no substantiated P0-P3 issue.
Node 24 core/storage/runtime/runtime-host builds and 50 focused copy/Graph tests pass, including external-tool lookalike and successive-copy cases. The current-head hosted test check is green; fresh-main merge-tree and diff check are clean. I did not run a real provider tool stream, packaged Desktop, multi-user race, or the full local suite. This comment is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Summary
Fixes #5331
Editing and resending a follow-up after Agent Graph completion could fail with
operation_unavailable: revision preflight recognizedsubagentandagent_swarmresults, but missed the child references in persisted JSONagent_outputresponses. The revision now retains those references so the edited question can run in a new version while the original version remains available.agent_outputenvelope across result and diagnostic views, including historical JSON in messages, RuntimeEvents, and legacy archive bodies. Validate child ownership, retained parent turns, Graph identity, terminal Run/event identity, lineage, and Artifact ownership. An earlier running poll is accepted only when retained history also contains a terminal result for the same child, Run, and Turn.Verification
Passed
npm run lint,npm run format:check,npm run build, andnpm run typecheck.Passed
npx --no-install knip --workspace apps/desktopandnpx --no-install knip --workspace packages/ui.Passed 72 tests across
conversation-copy-agent-output,conversation-copy,session-revision-graph-references,session-revision-diagnostics, andsession-revision-protocolafter rebuilding.Passed the targeted production Host integration test:
A baseline comparison using the same historical JSON fixture collected 0 child references at
4dc6a47f9and 1 with this fix.Not run: full repository
npm testand manual Desktop UI reproduction. The integration test uses a local provider fixture, not a live external model.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex — code review, regression verification, commit preparation, and this PR description. The commit includes
Generated-by: OpenAI Codex.Checklist
Does this PR entail a change in behavior?