Skip to content

fix(runtime): preserve Agent Graph results across session revisions - #5512

Merged
chinawch007 merged 2 commits into
apache:mainfrom
chinawch007:fix/agent-graph-session-revision
Oct 9, 2026
Merged

chinawch007 merged 2 commits into
apache:mainfrom
chinawch007:fix/agent-graph-session-revision

Conversation

@chinawch007

Copy link
Copy Markdown
Contributor

Summary

Fixes #5331

Editing and resending a follow-up after Agent Graph completion could fail with operation_unavailable: revision preflight recognized subagent and agent_swarm results, but missed the child references in persisted JSON agent_output responses. The revision now retains those references so the edited question can run in a new version while the original version remains available.

  • Recognize the defined agent_output envelope 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.
  • Preserve validated child identities within the revision family. Independent Side Conversations instead receive static result snapshots and copied Artifacts. Rewrite attachment links on subsequent copies, and migrate affected legacy archive transitions to ledger-backed snapshots with matching digests and usable pagination.
  • Add regressions for JSON reference collection, invalid references, duplicate reads, polls, diagnostic views, and repeated snapshot/archive copies. Extend the production Host/provider-wire test through Graph completion, an ordinary follow-up, revision creation, and a successful edited turn; assert that child results survive, the child is not rerun, and the original follow-up remains available.

Verification

  • Passed npm run lint, npm run format:check, npm run build, and npm run typecheck.

  • Passed npx --no-install knip --workspace apps/desktop and npx --no-install knip --workspace packages/ui.

  • Passed 72 tests across conversation-copy-agent-output, conversation-copy, session-revision-graph-references, session-revision-diagnostics, and session-revision-protocol after rebuilding.

  • Passed the targeted production Host integration test:

    ✔ production Host executes and durably supervises an Agent Graph over a real provider wire
    ℹ tests 1
    ℹ pass 1
    ℹ fail 0
    
  • A baseline comparison using the same historical JSON fixture collected 0 child references at 4dc6a47f9 and 1 with this fix.

  • Not run: full repository npm test and manual Desktop UI reproduction. The integration test uses a local provider fixture, not a live external model.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex — code review, regression verification, commit preparation, and this PR description. The commit includes Generated-by: OpenAI Codex.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 19, 2026
@chinawch007
chinawch007 force-pushed the fix/agent-graph-session-revision branch from 9261617 to e5a7877 Compare September 21, 2026 17:54

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chinawch007
chinawch007 force-pushed the fix/agent-graph-session-revision branch from e5a7877 to 32cbb7c Compare September 27, 2026 13:28

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [P3] packages/runtime/src/conversation-copy-agent-output.ts:108-115 — the recognizer rejects envelopes where execution.currentRunId !== invocation.runId. Reading a superseded run via agent_output with an explicit older run_id yields such an envelope; it is then silently treated as opaque JSON, so references are missed and the pre-fix operation_unavailable/lost-linkage behavior persists on that narrow path. Consider accepting the mismatch (the claim is still validated host-side) or documenting the bound.
  2. [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.
  3. 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
@chinawch007
chinawch007 force-pushed the fix/agent-graph-session-revision branch from 32cbb7c to ad38a32 Compare September 29, 2026 17:50
@chinawch007

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews. I rebased this PR onto Apache main at b393304 and pushed ad38a32.
The integration follows #5757’s unified copy rules: revisions share validated Graph children, while other retained children become static snapshots. I also fixed archive address mapping for repeated copies.
Regarding the two P3 observations:

  • Superseded-run reads: findChildRunForOutput returns both invocation: selected and execution.currentRunId: selected.runId. Selecting an older run therefore does not produce the ID mismatch described, so I retained the consistency check.
  • Revision retries: after at most three retries, the test explicitly requires committed. I verified that an always-conflicting handler fails that assertion after four total calls. The loop does not mask persistent conflicts.
    Validation passed: 102 relevant tests, including the production Host Graph/edit-and-resend flow, ordinary-child snapshots, archive pagination, and repeated copies. Build, lint, formatting, typecheck, and Desktop/UI knip checks also passed. Full repository tests and manual Desktop verification were not run.
    The updated commit is ready for another review; the new CI result should be checked before merging.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@chinawch007

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I’ve prepared fixes for all three points:

  1. Removed the redundant referencedGraphs membership guard, while retaining the Graph state checks.
  2. Restricted JSON child-reference collection and snapshot rewriting to results identified as coming from agent_output. Transcript results are matched to tool metadata by turn and call ID. External tool payloads remain opaque across transcript, RuntimeEvent, model-projection, and repeated archive copies. Added regressions covering the nonexistent-child case for revisions, branches, and Side Conversations.
  3. Added isolated parser cases for invocation/session mismatch, terminal identity and partial status, diagnostic event identity, and nonempty diagnostics in the result view. The five targeted parser mutations now fail the new tests.
    Validation: 86 focused tests passed, including the production Host Graph/edit-and-resend integration test. Runtime and Host builds, changed-file lint/format checks, and git diff --check also passed. The new provenance regression fails against the previous implementation. Full repository tests and manual Desktop verification were not run.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chinawch007
chinawch007 merged commit badebba into apache:main Oct 9, 2026
1 check passed
@chinawch007
chinawch007 deleted the fix/agent-graph-session-revision branch October 9, 2026 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): Editing and resending a follow-up fails after Agent Graph completion

4 participants