Skip to content

feat(desktop): show Agent Graph output previews - #4751

Open
testikun wants to merge 28 commits into
apache:mainfrom
testikun:codex/issue-3714-agent-graph-v2
Open

testikun wants to merge 28 commits into
apache:mainfrom
testikun:codex/issue-3714-agent-graph-v2

Conversation

@testikun

@testikun testikun commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Project a bounded 280-code-point output tail and usage-derived token rate from existing child Session RuntimeEvents and read models.
  • Carry optional fields through the strict Runtime Host decoder and Desktop Agent Graph panel; current protocol epoch moves from main's 169 to 170.
  • Preserve Open child task and existing graph scheduling, admission, execution, recovery, and child-session semantics.

Fixes #3714

Latest Verification

Head 8508fdfd0 passed hosted CI on 2026-09-21, including all 37 Desktop E2E cases. Verification for the current feature tree also covers the coordinator/read-model suites, Agent Graph panel tests, workspace typechecks, renderer architecture, protocol guard, and changed-file Biome.

The preview prefixes its bounded tail with an ellipsis. Throughput copy explicitly labels average output tokens/s. A 100 ms window coalesces pending text per operator; the integrated FakeBackend regression reduced output projection commits from 103 to 35 while retaining the final durable preview. Removing the window makes that regression fail at 103 commits.

Screenshot Provenance

The previously supplied image shows the real Electron renderer using synthetic fixture data. It does not demonstrate live-provider measured TPS. No new visual verification was performed in this repair pass.

Electron renderer with synthetic Agent Graph data

Review Focus

RuntimeEvents remain the output and usage authority. The bounded projection is presentation-only. Pending output text is bounded and coalesced per operator; durable events preserve the final result. The displayed rate is an average across the output sample window, including intervening tool execution, rather than instantaneous model generation speed.

AI Use

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

OpenAI Codex implemented and repaired this change on behalf of testikun. This update is not an independent human review. Commits include Generated-by: OpenAI Codex.

Behavior Change

Yes: Agent Graph displays bounded output previews and usage-derived token rate.

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 4, 2026
@testikun
testikun force-pushed the codex/issue-3714-agent-graph-v2 branch 6 times, most recently from 24b07e8 to c2124a0 Compare September 8, 2026 03:01

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

Synthesis: GO, no P0–P2 (with 1 credibility note + 3 P3 observations)

This conclusion comes from @Ox-Qronos's independent review. I did not read this diff myself; I verified the current head has not drifted and the exact-head CI state.

What this diff actually does (16 files, three layers): per-operator bounded output previews (280 code points + 2KiB dual cap) with usage-derived TPS for Agent Graph projection; strict allowlist/decoder carriage of the optional output fields with compatibility epoch 133→134; Desktop panel rendering of live/result labels, TPS, and previews.

Credibility note on the PR body (not a code defect, but the author should know): the body claims the screenshot shows real output with measured TPS from a local build at ff866d713. Verification shows that commit only touches a coordinator test file (+35/-5) and contains no panel rendering (added 4 days later); the screenshot content matches storybook fixture data verbatim (session name, title, tokensPerSecond:42/18, mirrored story copy). So: real app window with synthetic data. Rendering itself matches the code path exactly, and feature validity is independently proven by local tests and hosted CI.

P3 observations:

  1. Write amplification tradeoff: every text delta now materializes one bounded projection commit plus invalidation (the old design explicitly asserted zero projection writes on deltas). Payloads are bounded and semantics correct, but commit volume on high-frequency streaming sessions is worth knowing. The author already declared this tradeoff in the review focus.
  2. Display semantic difference: durable rebuild truncates from the head while live streaming truncates from the tail, so the preview window flips from "start" to "end" on reconnect. Display layer only, authoritative data unaffected.
  3. Dead code: the ?? textEvents[0]!.ts fallback near projection.ts:346 is unreachable (latestText is guaranteed non-empty). Harmless.

Not covered: real-provider streaming TPS behavior (no live streaming environment here); whether the author validated with real data beyond fixtures (only proven inconsistent with the claimed head, intent not proven); lint/format/knip not run locally (hosted CI green governs).


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@testikun

testikun commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. The unreachable textEvents[0]!.ts fallback has been removed; sampleStartedAt now uses the already-validated model text event directly. The fix is pushed in commit 153a20f5e. Local npm run build:test passes under Node 24, and the hosted CI check is running on the updated head.

@testikun
testikun force-pushed the codex/issue-3714-agent-graph-v2 branch from 153a20f to e3995ab Compare September 9, 2026 02:45
testikun and others added 5 commits September 9, 2026 10:48
Project bounded child output and usage-derived throughput from existing RuntimeEvent facts into the Agent Graph read model, then render streaming and completed previews without changing graph execution semantics.

Generated-by: OpenAI Codex
Replace the obsolete no-delta-write assertion with bounded streaming projection and one-invalidation-per-commit coverage.

Generated-by: OpenAI Codex
@testikun
testikun force-pushed the codex/issue-3714-agent-graph-v2 branch from e3995ab to 44366b7 Compare September 9, 2026 02:48

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

Re-verification on new head: GO stands, P3③ fixed

This follows up the review above. The PR has since moved to a new head with exactly one commit (fix(runtime): remove unreachable output timestamp fallback), which removes the dead-code fallback this review flagged as P3③. This conclusion comes from @Ox-Qronos's re-verification. I did not read this diff myself; I verified the head binding and the exact-head CI state below.

  • The removal is behavior-preserving (the preceding guard guarantees a non-empty model text event, so the fallback was unreachable).
  • Prior GO verdict (no P0–P2), the credibility note, and the remaining two P3 observations carry over unchanged.
  • Exact-head test is green on this head.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

Merge current main, advance the strict protocol epoch to 136, and consistently retain the bounded output tail. Remove the now-unnecessary head/tail option and cover streamed completion and immutable rebuild.

Generated-by: OpenAI Codex
@testikun

Copy link
Copy Markdown
Contributor Author

Automated follow-up by OpenAI Codex on behalf of testikun (not an independent human review).

Pushed 15d7f89, merging current main and advancing compatibility from 135 to 136. Live, completed, and rebuilt previews now consistently retain the bounded tail; removed the unnecessary head/tail selector and added regressions. The per-text-delta projection-write tradeoff remains explicit: this patch does not introduce a second throttling lifecycle or change graph execution authority.

Validation: full build:test and typecheck; 55 focused projection/read-model/protocol/Host/Desktop tests and the graph boot/recovery integration case passed. diff against main passes whitespace checks.

Correction to earlier evidence: the existing screenshot demonstrates a real Electron renderer with synthetic fixture data, not independently measured live-provider TPS. The old ff866d7 attribution does not prove the later panel implementation. No real-provider TPS claim is made by this verification.

Merge latest main and retain its epoch 136 contract; graph output extends epoch 137.

Generated-by: OpenAI Codex
@testikun

Copy link
Copy Markdown
Contributor Author

Automated follow-up by OpenAI Codex on behalf of testikun. Main advanced during this repair and #5117 now owns epoch 136. Resolved the resulting conflict against main f091198: retain upstream epoch history and move the Agent Graph output extension to 137. Full build:test, 62 focused Graph projection/read-model/Desktop/protocol tests and the protocol epoch guard (136 -> 137) pass. Latest head 0dae287 is pushed. The earlier CI success belongs to the previous head; this update needs a fresh run and human review.

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

Re-verification on new head: GO stands

This follows up the reviews above. The PR has since moved to a new head containing two main merges plus three author-side changes. This conclusion comes from @Ox-Qronos's re-verification, rebuilt and re-run locally (5 suites green). I did not read this diff myself; I verified the head binding and the exact-head CI state below.

  • Unified tail truncation (both durable and live paths now take the last 280 code points without trim) resolves the previously noted P3② display inconsistency, in the right direction and covered by tests.
  • Panel visibility call updated for main's refactor; no behavior concern.
  • Exact-head test is green on this head.

One action needed before merge: the declared compatibility epoch (137) is already taken on main (now at 141), so another rebase with a fresh epoch stamp will be required — the gate will enforce it, do not let it linger.

  • Prior GO verdict (no P0–P2) and remaining observations carry over.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

Preserve main protocol history and reserve epoch 153 for graph output shapes. Remove single-use arithmetic wrappers after focused regression verification.

Generated-by: OpenAI Codex
@testikun

Copy link
Copy Markdown
Contributor Author

Updated on 2026-09-15: the compatibility epoch now advances from the current main baseline as part of 586c7c5. Validation passed locally: npm run build:test, strict renderer-architecture check, and the focused Runtime Host/desktop Agent Graph protocol, reader, coordinator, panel, copy, visibility, and refresh tests.

…eview

# Conflicts:
#	packages/runtime-host/src/protocol/agent-graph.ts
#	packages/runtime-host/src/protocol/index.ts
@testikun

Copy link
Copy Markdown
Contributor Author

Synced this branch with current apache/main and resolved the Runtime Host protocol conflicts in 3d7a4b1dd.

  • Preserved the current shared requireOpaqueIdentity codec boundary.
  • Rebased the Agent Graph output projection onto compatibility epoch 157 and assigned epoch 158 (protocol-epoch-check: 157 -> 158).
  • Reviewed the effective diff against current main; no additional P0-P2 findings.

Local verification on Node v24.20.0:

  • npm ci
  • npm run build:test
  • focused Agent Graph tests: 69 passed
  • Runtime: 3496 passed, 14 skipped, 0 failed
  • Runtime Host: 1949 passed, 12 skipped, 0 failed
  • Desktop: 2681 passed, 0 failed
  • Desktop typecheck (preload/main/renderer/Storybook)
  • renderer architecture check
  • E2E budget check
  • git diff --check upstream/main...HEAD

The first Runtime run was executed concurrently with three other full suites and hit two unrelated timing-sensitive failures in code-mode.test and openai-responses-websocket.test. Both files then passed three isolated runs each, and the final standalone Runtime suite passed in full.

56qi

This comment was marked as outdated.

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

Review — head 3d7a4b1dd

Reviewed the effective diff against main (4410c3a2d) by reading the branch locally, line by line. I did not execute the test suites, so runtime-behaviour claims below are marked with confidence and should be confirmed by profiling before you act on the severity.

Scope read in full: stream-graph-projection.ts, stream-graph-read-model.ts, stream-graph-coordinator.ts, protocol/agent-graph.ts, protocol/index.ts, server/agent-graph-coordinator.ts, agent-graph-panel.tsx, agent-graph-copy.ts, agent-graph.css, the story, and all six touched test files.

The shape of this change is right: output previews are derived from existing RuntimeEvents, RuntimeEvents stay authoritative, the protocol boundary is allowlisted and byte-capped, and the epoch bump is correctly justified in the comment. The findings below are mostly about the presentation layer, which is where the test coverage thins out.


P2-1 — The ellipsis is rendered on the wrong side, so the UI states the opposite of the truth

The projection deliberately keeps the tail (packages/runtime/src/stream-graph-projection.ts, boundOutputPreview):

const visible = codePoints.slice(-AGENT_GRAPH_OUTPUT_PREVIEW_MAX_CODE_POINTS);
return { text: visible.join(''), truncated: true };

But the renderer appends at the end (apps/desktop/src/renderer/agent-graph-panel.tsx):

{operator.output.preview}
{operator.output.previewTruncated ? '…' : ''}

Tail truncation means the beginning was dropped, so the ellipsis must lead (…{preview}). As written a long answer renders as "…the last 280 code points…", which reads as "more follows" when in fact the opening was cut. The existing instructionPreview in the same codebase is head-truncated with a trailing (${work.instruction.slice(0, MAX_INSTRUCTION_PREVIEW_CHARS)}…) — that one is correct, and this change applies the same visual convention to the opposite truncation direction.

Three independent signals that this is a real defect and not my misreading:

  1. agent-graph-panel.test.ts asserts assert.match(textContent, /Inspecting the renderer projection…/) while the fixture sets previewTruncated: truethe test freezes the incorrect rendering, so any fix must update that assertion too.
  2. The OutputPreviews story hand-writes preview: 'Comparing the three provider adapters and their retry boundaries…' together with previewTruncated: false — the author was not settled on who owns the ellipsis.
  3. Every truncation test (Array.from(text).slice(-280) in stream-graph-read-model.test.ts, the projection tests) asserts the projection string only. Nothing asserts the rendered ellipsis position.

P2-2 — "tokens/s" is whole-turn output ÷ whole-turn wall clock, and it disagrees with itself across paths

Batch/rebuild path (projectOperatorOutput):

const sampleStartedAt = orderedEvents.find(
  (event) => event.role === 'model' && event.content?.kind === 'text',
)!.ts;
const usageEndedAt = latestUsage?.ts;
const sampleDurationMs = Math.max(0, usageEndedAt - sampleStartedAt);

token_usage is emitted once per send, after the send finishes (ai-sdk-turn.ts), and its own comment states the value "spans every Runtime loop step and retry". So the numerator is the sum of output tokens across all steps of that send, while the denominator is first-token → end-of-turn — including every tool execution, thinking block, retry, and loop step. For a graph operator doing "Inspect the repository" work, the number will sit well below true generation speed, and it falls as the operator does more work. Rendered as 21.0 token/s it reads as generation throughput; it is not.

More concretely, the two paths disagree on the start of the sample window:

  • incremental path: existing?.sampleStartedAt ?? event.ts (first delta this projection happened to observe)
  • rebuild path: first text event in the stream

So the same operator's TPS changes across a projection repair (#repairClientProjectionBestEffort). Either unify the definition, or label it honestly (e.g. avg) — and if the metric is only meaningful for single-step runs, consider suppressing it otherwise. The PR description only acknowledges the per-delta write tradeoff, not this.

Minor related risk (medium confidence): the code takes usageEvents.at(-1), while the compaction path in runtime-kernel.ts emits a token_usage with output: 0. If that lands last in the stream, the outputTokens > 0 guard makes TPS disappear entirely rather than fall back to the previous sample.

P2-3 — Every text delta is now a full SQLite transaction plus an invalidation, with no bound on the backlog

Before this change text_delta returned undefined from projectClientSessionEvent, so advanceMaterializedAgentGraphClientProjection early-returned and wrote nothing. That is exactly the behaviour the deleted assertion protected:

'partial text deltas must not cause projection writes or invalidations'

Now every delta produces an output, so it runs the full commitAgentGraphClientProjection — a SQLite transaction that re-validates the version, writes an applied record, and rewrites the whole snapshot payload JSON and operator payload — and emits one runtime_activity invalidation.

#queueClientProjectionUpdate only chains onto the previous task; it neither coalesces nor drops:

const previous = driver.clientProjectionTask ?? Promise.resolve();
const task = previous.catch(() => {}).then(async () => { await operation(); });

At a few hundred deltas per second this serial queue grows without bound, each item costing O(snapshot). #waitForClientProjectionUpdates awaits the whole chain, so any repair path also waits out the entire backlog. The comment thread acknowledges "per-text-delta projection-write tradeoff remains explicit", but does not bound it. Since this is purely presentational data, dropping intermediate frames is safe — coalescing on a time window (say ≤100 ms) or keeping only the newest pending frame would remove the risk without changing semantics.


P3 observations

  • activity silently became optional. AdvancedAgentGraphClientProjection.activity?: AgentGraphClientActivity is an exported type of packages/runtime. The single in-repo consumer (the coordinator) handles it, but this is a breaking type change for out-of-tree consumers and deserves a mention in the description/CHANGELOG.
  • A re-run operator transiently loses its preview. projectOperatorOutputs keeps only the stream with the newest openedAt, and projectOperatorOutput returns undefined until that activation emits text — so a previously visible "Result preview" disappears rather than falling back to the last settled activation. Worth deciding deliberately.
  • latestEventTime semantics widened. In the output-only branch, snapshot.latestEventTime = Math.max(..., runtime.event.ts) lets a partial delta's ts advance a field that previously only reflected committed non-partial records. I grepped apps/desktop/src and found no consumer, so it is currently harmless, but the field no longer means "last committed fact".

What is done well

  • The startOffset guard in foldClientOutputDelta is correct and non-obvious: startOffset is a UTF-16 offset (events.ts), and once the preview has been cut to a tail, offset arithmetic is meaningless. The author correctly avoids mixing the two.
  • The ! assertion on sampleStartedAt is genuinely safe — latestText comes from the non-partial subset and orderedEvents is a superset, so find must hit. I verified 44366b7d4: the "unreachable fallback" the comment thread says was removed really was removed, and the assertion really is sound.
  • Protocol boundary is solid: allowlisted requireShapedRecord, requireNonNegativeFiniteNumber rejecting negative TPS with a matching assertion, and the privateOutput leak test in agent-graph-coordinator.test.ts.
  • The 280-code-point producer cap versus the 2 KiB consumer byte cap is a real double guard, not duplication — 280 four-byte emoji is 1120 B, comfortably under 2048 B.

Verdict

Direction and layering are right, and the protocol work is careful. I would not merge before P2-1 is fixed — it is user-visible misinformation, and the current test freezes it. P2-2 needs at minimum an honest label or a unified definition across both paths. P2-3 needs a bound on the queue. P3 items are the author's call.

@testikun

Copy link
Copy Markdown
Contributor Author

Addressed the review items on the current head 7ef35d23f (rebased through current apache/main in 3390a9e06).

  • P2-1: Tail-truncated previews now render the ellipsis before the retained tail (…{preview}), and the panel regression covers the rendered live/completed output.
  • P2-2: The UI now labels the metric as average output tokens/s. It is intentionally the bounded operator-output average over the recorded sample window, rather than a claim about instantaneous generation speed.
  • P2-3: Text deltas are coalesced per operator in 100 ms windows. The coordinator now permits only one queued output-flush worker per graph; deltas arriving during a slow SQLite projection replace the pending value and schedule at most the next flush after the active worker completes. The projection wait loop follows that replacement task, so reconcile/read/close do not return before the pending flush chain settles.

Validation on Node 24.19.0: clean npm ci, npm run build:test, Runtime Agent Graph projection/read-model/coordinator tests (35), Runtime Host Agent Graph protocol/coordinator/reader/two-client tests (14), Desktop Agent Graph panel/copy/visibility/refresh tests (43), direct Biome check of the changed coordinator, and git diff --check upstream/main...HEAD. The repository pre-commit hook itself was interrupted after its Biome stdin invocation blocked without activity; the same file was checked directly and no formatting changes were required.

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

The authority story is clean: previews are derived from existing immutable RuntimeEvents with no new durable facts, the 280-code-point bound counts code points correctly (Array.from, surrogate-pair safe), the token rate is honestly labeled as a window average including tool time, the strict decoder whitelists keys and rejects unknown shapes, and the epoch bump is necessary since old strict decoders would reject the new output field outright. The 100ms coalescing window preserves ordering (same runId+messageId+contiguous offset only) and terminal-state correctness.

One P1 inline, one P2 and P3s in body.

P2 — the coalescing regression is not actually pinned (normal path). The test asserts outputProjectionCommits < 60, but the FakeBackend emits ~18 deltas in that scenario (9 chars / 45ms apart), so removing the 100ms window still passes. Either inject >60 deltas or tighten the assertion to the delta count.

P3s:

  • A stale-activation late delta can overwrite a new run's output (no event.ts < previewUpdatedAt guard when existing mismatches activationId) — transient, self-heals on rebuild. ②
  • Incremental vs replay derivation diverge slightly: token_usage arriving before any text is dropped by advanceClientOperatorOutput but kept by projectOperatorOutput; sampleStartedAt differs when the first event is text_complete; pre-activation output retention differs. Same state renders differently across paths; converges eventually.
  • The 280 bound is defined twice (AGENT_GRAPH_OUTPUT_PREVIEW_MAX_CODE_POINTS vs MAX_OUTPUT_PREVIEW_CODE_POINTS) — one constant should own it.
  • isMaterializedGraphClientEvent dropping 'text_delta' is unreachable (the only call site is gated earlier); if the predicate is reused later it bypasses coalescing.
  • A non-contiguous startOffset gap replaces the preview without marking it truncated — shows a fragment with no ellipsis.
  • projectOperatorOutputs sorts all events per run on the observe hot path.
  • Renderer nits from the panel side: preview lacks <bdi> isolation (a precedent exists in TailVisibleLabel); the literal is not aria-hidden; a failed operator can keep showing "Live output" until the next durable rebuild; unrelated model-picker-internals.tsx whitespace churn should be split or dropped.

Comment thread packages/runtime/src/stream-graph-coordinator.ts
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 18, 2026
@testikun

testikun commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Review follow-up (cac39fe): the inline P1 is fixed and resolved. For the P2 test concern, I checked the integrated fixture on Node 24: it makes 35 output-only projection commits with coalescing, while the PR’s recorded no-window ablation made 103. The existing <60 assertion distinguishes those cases; tightening it to the ~18-delta estimate failed on the healthy implementation, so I kept the original threshold. The other P3 observations remain non-blocking scope/UX follow-ups, not claimed fixed here. The latest hosted run failed the unchanged Runtime Host PTY integrity test after a 60s control-close wait, also seen on #5475; the graph-focused build and integration test pass locally. This account lacks Actions rerun permission.

Generated-by: OpenAI Codex

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

Reviewed current head f7b1dbe. Agent Graph output previews and average throughput are bounded, protocol-guarded, and covered by the focused Runtime/Host checks. No P0–P2 issue found.

…eview

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
Generated-by: OpenAI Codex
…eview

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@testikun

Copy link
Copy Markdown
Contributor Author

Resolved the current-main protocol conflict in 07fa70d on September 20, 2026. Agent Graph output previews now advance the latest main compatibility epoch 166 to 167 while retaining the complete intervening epoch history. GitHub reports MERGEABLE; the prior approval remains and fresh CI is running. diff --check and the protocol epoch guard pass.

@testikun

Copy link
Copy Markdown
Contributor Author

Follow-up on the remaining P3 review notes, pushed in e2cb92a and d20a3d7 on September 20, 2026:\n\n- Ignore late output deltas from an older activation once the operator has moved to a newer run.\n- Mark a non-contiguous startOffset fragment as truncated, so the leading ellipsis truthfully signals missing output.\n- Consolidate the duplicated 280-code-point limit into one Runtime authority shared by replay, incremental projection, and coalescing.\n- Wrap preview text in <bdi> and hide the visual ellipsis from accessibility output.\n- Removed the unrelated model-picker-internals.tsx whitespace hunk.\n\nRegressions cover the stale-activation and offset-gap paths. Validation: Biome; Runtime build; 37 projection/read-model/coordinator tests; Desktop main build and 24 AgentGraphPanel tests.\n\nI did not combine the remaining replay/incremental edge-case normalization or per-run event sorting into this follow-up: those require a broader projection/performance design and are not correctness blockers on the approved bounded/coalesced path. activity remains optional intentionally because output-only projection commits do not create immutable activity records.

Merge apache/maka main at 0dc1142. Preserve the removal of turn.regenerate and assign Agent Graph output snapshots compatibility epoch 168; re-pin the compatible cwd declaration.

Generated-by: OpenAI Codex

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

Re-verification on new head 08bbe4eed9aefb76b4593f427ccc3e895f299ec3: GO stands

This follows up the reviews above. The PR has since re-absorbed current main (merge 08bbe4ee, resolving the protocol epoch conflict) and added three small hardening changes. This conclusion comes from @Ox-Qronos's independent re-verification: diff against the previously reviewed head (eaa07934), epoch ledger check, CI state, and a local build with targeted suites re-run at this exact SHA. I did not read this diff's earlier thread reviews before re-verifying.

Epoch ledger. main is at 166 (167 was consumed by the regenerate removal that this merge absorbs); the branch now claims 168 with both documented in the comment ledger, and the external-session-cwd-limit-authority.json exemption is re-pinned to 168 consistently.

What the increment does beyond the merge:

  • Stale-activation hardening (stream-graph-read-model.ts): materializing the client projection now passes the operator's current activation id into advanceClientOperatorOutput, and events from a superseded activation no longer produce output. A settled operator (no current activation) keeps its terminal output — the guard is falsy-through, which preserves the completed view.
  • Discontinuous deltas are labeled, not silently joined (stream-graph-read-model.ts): an append whose startOffset skips past the existing preview now sets previewTruncated instead of presenting a gapped tail as contiguous text.
  • The 280-code-point bound now has one owner: AGENT_GRAPH_OUTPUT_PREVIEW_MAX_CODE_POINTS is exported from stream-graph-projection.ts and shared by the coordinator and read model (value unchanged). This also settles the earlier "bound defined twice" observation.
  • Panel (agent-graph-panel.tsx): the literal marker is now aria-hidden, and the preview text is wrapped in <bdi> so RTL previews cannot flip the surrounding layout. These settle the earlier renderer observations.

Verification. Hosted CI test passed at this SHA (run 35498148229, 20m54s). Locally: six workspaces typecheck/build clean, and the targeted suites re-run green — runtime 37, runtime-host 13, desktop agent-graph 43 (93/93).

Carry-over minor notes (non-blocking). The reconnect head/tail flip of the preview window (display-only, authoritative data unaffected) remains as previously noted, and the PR body still describes the pre-merge epoch wording.

Not covered here: real-provider streaming behavior; browser-specific rendering of <bdi>/mask fades across themes; lint/knip run locally (hosted CI governs).

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@testikun

testikun commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han @me2seeks Review follow-up on current head 4ea4a1c9c (September 21, 2026):

  • Merged the latest apache/main; the only conflict was the Runtime Host compatibility epoch. Agent Graph output previews now advance main's epoch 169 to 170 while retaining the complete upstream ledger.
  • The previously reviewed Agent Graph implementation and hardening changes are unchanged. The sole inline P1 remains fixed and resolved, and the PR remains within the already approved scope.
  • Ablation removed the stale re-pin of the already-landed external-session compatible-change declaration. It now matches main's historical epoch 165 and no longer appears in the PR diff.

Validation passed on Node 24: protocol epoch guard (169 -> 170) and its 17 tests; full workspace typecheck; strict renderer architecture against upstream/main (112 fixture tests plus the source check); Runtime Agent Graph suites (37 tests); Runtime Host Agent Graph suites (14 tests); Desktop AgentGraphPanel suite (24 tests); Biome on the resolved protocol file; git diff --check. GitHub should now recalculate this approved PR as conflict-free and run fresh CI.

CI follow-up on September 21, 2026: the first hosted rerun reached Desktop E2E and exposed the same stale Skill draft test contract seen on #5482. After a Skill is disabled, the composer intentionally uses the first Enter to dismiss the now-empty slash trigger; the test now waits for and activates the real submit control instead. This test-only change is ca7d9afea; a fresh production Desktop build and both Skill draft Electron cases pass locally. Agent Graph production code is unchanged.

The hosted rerun 35551761443 then exposed a pointer-activation flake in the second Skill draft case: Playwright completed the send-button click after about four seconds, but no form submission event reached the renderer. Since this test covers draft lifecycle rather than pointer behavior, 8508fdfd0 keeps the readiness gate and invokes the same native form submission with requestSubmit(). A fresh production build and both Electron cases pass locally.

Final hosted CI run 35552969256 completed successfully on head 8508fdfd0, including all 37 Desktop E2E cases.

Astro-Han’s latest minor is addressed in 643231283: external-session-cwd-limit-authority.json is re-pinned to epoch 170. The PR body is also current for head/epoch verification.

Hosted CI run 35556120549 completed successfully on head 643231283.

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

Re-verification on new head 8508fdfd02b25fb31e13614ddf5155ff7b284d87: GO stands

This follows up the review above. Since 08bbe4ee the branch re-absorbed current main (through #5536/#5532/#5518/#5541/#5542/#5530/#5527/#5535/#5543/#5544/#5070/#5073/#5471) and added two test-only commits to apps/desktop/e2e/skill-draft-lifecycle.spec.ts (submit via form instead of a keypress on a disabled composer). The feature surface — coordinator, projection, read model, panel, protocol files — is unchanged from the previously verified tree, so the earlier findings carry over verbatim. I re-checked rather than assumed: diff against 08bbe4ee restricted to the feature files is empty apart from the epoch ledger.

Epoch ledger. main is at 169 (consumed by #5536); the branch now claims 170 with 169 documented in the comment ledger — coherent.

One new minor (for the author). external-session-cwd-limit-authority.json was not re-pinned this time and still says "epoch": 165 while the branch claims 170. Previous rounds kept it in sync. The compatibility guard only validates newly added declarations, so nothing is granted or blocked by the stale value, but the ledger consistency break is worth a follow-up pin to 170 (or removal if the exemption is no longer needed).

CI narrative. One intermediate red: run 35551761443 at ca7d9afe — the skill e2e timed out on Skill 调用失败,消息未发送, the exact assertion the in-flight test commits were reworking, right after main absorbed composer refactors. Attribution between "test change interacting with absorbed refactors" and "continued e2e flakiness" is genuinely uncertain from here; the author resolved it in-chain (form submission) and the final head is green: run 35552969256 passed (19m28s) at 8508fdfd.

Verification scope. No local rebuild this round: zero feature diff means the prior local evidence (six workspaces build clean; runtime 37 + runtime-host 13 + desktop agent-graph 43, all green at 08bbe4ee) carries by tree identity; hosted CI green at this SHA governs the gate.

Carry-over minor notes (non-blocking). The reconnect head/tail preview flip and the stale PR-body epoch wording remain as previously noted.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. 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.

Re-verification on new head 6432312838c90a67529e65e2115d8988ebb1b443: GO stands; ledger pin closed

This follows up the review above. Exactly one commit since 8508fdfd: 643231283 chore(runtime-host): repin external session declaration, which re-pins external-session-cwd-limit-authority.json from 165 to 170 — closing the ledger-consistency note from the previous review. The feature files are unchanged; the findings and local evidence from the earlier rounds carry over by tree identity, and I verified the pin value and head directly rather than assuming.

Verification. Hosted CI test passed at this SHA (run 35556120549, 18m32s); head unchanged between check and post.

Remaining minor notes (non-blocking, unchanged). The reconnect head/tail preview flip and the stale PR-body epoch wording.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

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.

feat(desktop): Agent Graph 节点 TPS 与输出 preview

4 participants