Conversation
24b07e8 to
c2124a0
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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:
- 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.
- 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.
- Dead code: the
?? textEvents[0]!.tsfallback nearprojection.ts:346is unreachable (latestTextis 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.
|
Thanks for the review. The unreachable |
153a20f to
e3995ab
Compare
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
e3995ab to
44366b7
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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
testis 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
|
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
|
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
left a comment
There was a problem hiding this comment.
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
testis 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
|
Updated on 2026-09-15: the compatibility epoch now advances from the current main baseline as part of 586c7c5. Validation passed locally: |
…eview # Conflicts: # packages/runtime-host/src/protocol/agent-graph.ts # packages/runtime-host/src/protocol/index.ts
|
Synced this branch with current
Local verification on Node
The first Runtime run was executed concurrently with three other full suites and hit two unrelated timing-sensitive failures in |
me2seeks
left a comment
There was a problem hiding this comment.
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:
agent-graph-panel.test.tsassertsassert.match(textContent, /Inspecting the renderer projection…/)while the fixture setspreviewTruncated: true— the test freezes the incorrect rendering, so any fix must update that assertion too.- The
OutputPreviewsstory hand-writespreview: 'Comparing the three provider adapters and their retry boundaries…'together withpreviewTruncated: false— the author was not settled on who owns the ellipsis. - Every truncation test (
Array.from(text).slice(-280)instream-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
activitysilently became optional.AdvancedAgentGraphClientProjection.activity?: AgentGraphClientActivityis an exported type ofpackages/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.
projectOperatorOutputskeeps only the stream with the newestopenedAt, andprojectOperatorOutputreturnsundefineduntil that activation emits text — so a previously visible "Result preview" disappears rather than falling back to the last settled activation. Worth deciding deliberately. latestEventTimesemantics widened. In the output-only branch,snapshot.latestEventTime = Math.max(..., runtime.event.ts)lets a partial delta'stsadvance a field that previously only reflected committed non-partial records. I greppedapps/desktop/srcand found no consumer, so it is currently harmless, but the field no longer means "last committed fact".
What is done well
- The
startOffsetguard infoldClientOutputDeltais correct and non-obvious:startOffsetis 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 onsampleStartedAtis genuinely safe —latestTextcomes from the non-partial subset andorderedEventsis a superset, sofindmust hit. I verified44366b7d4: the "unreachable fallback" the comment thread says was removed really was removed, and the assertion really is sound. - Protocol boundary is solid: allowlisted
requireShapedRecord,requireNonNegativeFiniteNumberrejecting negative TPS with a matching assertion, and theprivateOutputleak test inagent-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.
|
@Astro-Han @me2seeks Review follow-up on current head
Validation on Node 24.20.0: workspace |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-verification on new head 19673421135ebfe478a55f36915f56c4edfdc548: GO stands; epoch ledger re-checked against main
This follows up the review above. Since 25e0ee3b the branch re-absorbed 33 commits from main (#5707, #5687, #5706, #5592, #5629, #5638, #5684, #5697, #5517, #5510, #5678, #5682, #5686, #5683, #5679, #5675, #5681, #5667, #5666, #5643, #5639, #5676, #5672, #5660, #5610, #5658, #5659, #5628, #5598, #5482, #5636 and others). The four feature files (coordinator, projection, read model, panel) are unchanged — the diff against the previously verified tree is empty apart from the epoch ledger, so the earlier findings carry by tree identity. Re-checked directly, not assumed.
Epoch ledger verified coherent. main is now at epoch 187 (185: bounded next-prompt suggestion operation via #5629; 186: form_interaction transcript messages; 187: transcript_changed subscription closing with bounded-tail reseeding — each documented). This branch claims 188, re-stating this PR's feature (Agent Graph operator snapshots with bounded output previews and metrics), and re-attributes 184 to the unified custom provider type (#5684, now on the main side). Main 187 → branch 188, every number accounted for, no gaps.
Gate: green. Hosted CI test passed at this SHA (run 36085283878, 22m0s).
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.
|
Review follow-up after rebasing onto current main (head
Validation: full |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 006b98052a0e4eb1188cc559c4e0a75ae35987f2.
Current-head result: no P0-P3 findings.
This update re-absorbs current main and advances the Runtime Host compatibility ledger to epoch 197; the Agent Graph output-preview implementation itself is unchanged from the previously reviewed head. I re-checked the effective feature surface rather than carrying the old review forward: bounded/coalesced output projection, activation-scoped read-model updates, strict protocol decoding, and Desktop panel rendering remain intact. The only adjacent test delta is comment-only.
Validation on Node 24.18.1: clean npm ci, build:test, full workspace typecheck, git diff --check, Runtime graph coordinator/projection/read-model suites (38/38), Runtime Host graph coordinator/protocol suites (10/10), and Desktop Agent Graph panel tests (24/24). Hosted test passed on this exact head. Against fresh main 0fd7540831ebc4c38f6af9b96c8a510d7d087f1, the PR is 36 commits ahead and 4 behind; merge-tree 3922fdeaad11ee608a6d2e2611d48557e475b12b is conflict-free.
The PR description's “Current head 196734211” and epoch-188 verification text are stale and should be refreshed, but the checked code and current gate are coherent. I did not exercise a real provider stream, packaged Electron, native Windows/macOS rendering, or manual reconnect/visual behavior.
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.
# Conflicts: # packages/runtime-host/src/protocol/index.ts
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 443f135d0d99f0446b9b80d296aa62ccea98ed7f.
Current-head result: no P0-P3 findings.
This update is a merge of current main into the previously reviewed head. The Agent Graph production implementation is unchanged; the only feature-adjacent resolution is the compatibility ledger in packages/runtime-host/src/protocol/index.ts:107-111. Main now owns epoch 197 for revision-fenced queue reorder requests, and this PR coherently advances Agent Graph output snapshots to epoch 198. I re-checked the effective path: per-operator delta coalescing and serialized projection flushes remain in packages/runtime/src/stream-graph-coordinator.ts:1177-1242, activation-scoped output materialization remains in packages/runtime/src/stream-graph-read-model.ts:366-419, and the Desktop panel keeps the bounded preview and bidi isolation in apps/desktop/src/renderer/agent-graph-panel.tsx:379-394.
Validation on Node 24.18.1: clean npm ci, build:test, full workspace typecheck, lint, format check, ASF headers, and git diff --check passed. Focused Runtime projection/coordinator/read-model tests passed 38/38; Runtime Host protocol/coordinator plus Desktop panel tests passed 34/34. Hosted test is green on this exact head. Fresh main 8b104db12b70814b33f25b111021e2e8ec325d1f is the merge parent, so the PR is 37 commits ahead / 0 behind; merge-tree c1363cadb2f2b9800462947e1137865511147f55 is conflict-free.
The PR description still names head 196734211 and epoch 188, so its verification narrative is stale even though the current code and gates are coherent. I did not exercise a real provider stream, packaged Electron, native Windows/macOS rendering, or manual reconnect and visual behavior.
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.
A second, independent review of head 443f135d (Claude lineage), alongside the hqhq1025 review 5334369656. The PR's Generated-by trailers name OpenAI Codex; no Grok. The epoch moves 197 → 198 against main at 197, but #5458 and #5732 also claim 198, so this depends on merge order.
P2: output streaming can permanently supersede supervisor wakes. I traced this path through the code; it has not been reproduced end to end.
clientSnapshotVersion(stream-graph-read-model.ts:1569-1575) hashes the whole bounded snapshot, and the snapshot now includes each operator's output preview (:408-413). Every output flush, about every 100ms (OUTPUT_DELTA_PROJECTION_INTERVAL_MS), therefore produces a newsnapshotVersion. Before this PR, partial text deltas deliberately caused no projection write. The test asserting that ("partial text deltas must not cause projection writes or invalidations") was deleted instream-graph-coordinator.test.ts.- A supervisor wake is current only while
${graphId}:${snapshotVersion} === wake.wakeId(agent-graph-supervisor-wake.ts:681-690). agent-graph-execution-coordinator.ts:94returnssupersededas soon asisCurrent()fails, and supersession is terminal.- The snapshot read goes through
#readOrRebuildClientProjection, which first waits for the pending output flush (stream-graph-coordinator.ts:940-948).
As a result, while any operator is streaming, a wake raised for a failed or blocked item is likely to be superseded before admission. In swarm mode, the supervisor is then not woken again until the whole graph settles.
Suggested fix: keep output previews out of the version hash, for example as a separate field excluded from clientSnapshotVersion, or key wake currency on a structural version. Please also restore a test that streaming does not change the supervisor-facing version.
P2: the newest output is never visible in the panel. I checked this in the CSS but did not render it in Electron. The rule at agent-graph.css:190-196 applies to the nested spans, so the preview is forced onto a single line and cut off on the right. The backend keeps the last 280 characters, but the panel shows only the start of that window. While streaming, the newest text is hidden, and a finished result shows a middle fragment. The PR screenshot uses short text, so it doesn't show this.
P3 (the first two reproduced with a scratch test):
- The ending can appear twice. A late text delta with the same timestamp as
text_completeis appended to an already truncated preview (read-model.ts:680-686,733-744). - A fragment can be shown as complete. After a projection rebuild mid-stream, the next delta is displayed as the full output with no ellipsis (
read-model.ts:690-701). - The token rate is misleading. It divides total output tokens, including reasoning and tool-call tokens, by the time since the first text. A zero-token compaction shows "avg 0.0" live but nothing after a rebuild.
- Screen readers aren't told the preview is truncated. The ellipsis is
aria-hidden. - Child-agent output is now persisted. Up to 280 characters of it, which could include secrets, are stored in the control store and sent to clients. Worth stating in the PR.
Checked and fine: truncation counts whole code points, and pending memory is capped per operator. <bdi> isolation is in place, the protocol decoder is strict, and late output from an older activation is dropped.
Tests: runtime stream-graph 38/38 and runtime-host agent-graph 55/55 pass. Not run: the Desktop panel tests, Storybook, Electron rendering, and an end-to-end repro of the wake P2.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; the wake-supersession path was checked against the code, but please verify before acting.
hqhq1025
left a comment
There was a problem hiding this comment.
Correction to my earlier review on this head: I withdraw the "no P0-P3 findings" conclusion for 443f135d. I independently confirmed the wake-currency path described in review 5334430789; please treat that review's findings as the current result for this head.
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.
|
Review follow-up on current head
Validation: full test build; 26 focused Runtime read-model/coordinator tests; 24 Desktop panel tests; Biome; strict renderer architecture; narrow-width Chromium layout; |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 495dafe94ac5df341bd8d6478ddf9769eb9bd572. The two previously reported P2 issues are fixed: presentation-only output is excluded from snapshotVersion while durable activity still advances it, and the preview now wraps instead of clipping the newest retained tail. A Chromium Storybook check rendered a 281-character preview across multiple lines with no horizontal overflow and the NEWEST-END suffix visible.
I found one P3 in the output folding path, reported inline. No P0-P2 findings remain on this head.
Validation completed on Node 24.18.1: clean install, build:test, full typecheck, lint, format, ASF headers, git diff --check, focused Runtime 59/59, Runtime Host/root-turn 94/94, Desktop panel 24/24, and renderer architecture 112/112. Hosted test is green. A synthetic merge with current main de4fc5ff95b1f8034ca00b448984b31ca711dce1 is conflict-free and passed build:test, focused Runtime 51/51, and Runtime Host 94/94. I did not run a real provider, packaged Electron, or native Windows/macOS flows. Epoch 198 also remains subject to merge ordering because other open PRs claim the same epoch.
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.
A second, independent review of head 495dafe9 (Claude lineage), alongside the hqhq1025 review 5335227774. Both P2s we raised on the previous head are fixed, and we found nothing at P0–P2.
Supervisor wakes are no longer superseded by streaming output.
clientSnapshotVersionnow dropsoutputbefore hashing (stream-graph-read-model.ts:1568-1579), and output-only commits no longer advancelatestEventTime(:415-419).- A temporary probe in the coordinator integration test confirmed that none of 35 output commits changed the version.
- Clients still receive preview updates:
runtime_activityis still emitted after each output commit (stream-graph-coordinator.ts:1330), and the panel re-reads the snapshot every time. - Nothing in apps, ui or cli uses
snapshotVersionas an ETag or cache key.
The newest output is visible. The more specific preview rule switches to pre-wrap with overflow-wrap: anywhere, so the 280-character tail wraps instead of being clipped. We checked this from the CSS only; the committed stories use short previews.
P3:
- The duplicated tail is still reproducible (
:879-884,:931-941). A late delta that shares thetext_completetimestamp produces…TAIL-END…TAIL-END. snapshotVersionno longer covers the full payload. A repair rebuild that runs outside the serialized queue (stream-graph-coordinator.ts:940-961,1253-1262) can briefly overwrite a streaming preview. The next delta restores it. A code comment warning against using the version as an ETag would help.- The preview has no line cap. Output that is mostly newlines can fill the panel, though the panel is capped at 300px and scrolls.
- The token rate is still misleading. The ellipsis is now readable text, but nothing announces that the preview is truncated.
- There is no test that a wake is delivered while output is streaming.
Epoch: this head uses 198, the same as #5709, #5458 and #5732, so the order they merge in matters.
Tests: runtime 59/59, runtime-host 55/55, desktop panel 24/24.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
|
Reviewed the new exact-head feedback and fixed the reproducible duplicated-tail case in The remaining P3 notes do not identify a correctness regression on this head: |
hqhq1025
left a comment
There was a problem hiding this comment.
I found no P0-P3 issues at exact head 3b97e2e4371fdfacba4463ab0f9d79c1eaad10af.
Since the previously reviewed 495dafe94ac5df341bd8d6478ddf9769eb9bd572, the only substantive change is the completed-output guard in packages/runtime/src/stream-graph-read-model.ts:885-889 plus its regression in packages/runtime/src/__tests__/stream-graph-read-model.test.ts:468-533; the following two CI retry commits have no file changes. The guard rejects a text_delta only when the same activation/message output is already completed, while different messages and activations retain their existing paths. Removing the guard and rebuilding made the new regression fail with the prior duplicated ...YZYZ tail, so the test distinguishes the fix from the old behavior. The earlier wake-currency and visible-tail fixes remain intact.
Verification: clean Node 24.18.1 npm ci and build:test; focused Agent Graph Runtime tests 60/60; full typecheck, lint, format, ASF-header, and diff checks; hosted test green. A synthetic merge with current main de4fc5ff95b1f8034ca00b448984b31ca711dce1 built successfully and passed the focused Runtime suites 65/65; merge tree 15ce46f7b1721251633f6d04bf5898693e492bc8 is conflict-free. The PR is 41 commits ahead and 3 behind main.
Residual scope: I did not run a real provider, packaged Electron, or native Windows/macOS. Compatibility epoch 198 is still also claimed by other open PRs, so the final merger must re-evaluate the next free epoch after merge ordering is known.
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.
Incremental re-review at 3b97e2e4, relative to 495dafe9, which our previous full review found clean. The only substantive change is the 3-line guard at stream-graph-read-model.ts:887-889 plus its regression test. The two later CI-retry commits change no files.
Verdict: no P0–P3 in the delta.
phasebecomes'completed'only viaoperatorSettledor an already-completedexisting, not viatext_completealone. The guard therefore drops only latetext_deltareplays for the same activation and message after the operator settled. This is exactly the...YZYZduplicate-tail case.- New messages (different
messageId), new activations (handled earlier byexisting/currentActivationId) andtext_completeevents still pass through. sameMessageis also true when bothmessageIds areundefined. This is acceptable, because no new message is expected in a settled activation.stream-graph-read-model.test.ts:468-533pins the behavior.
The earlier wake-currency and newest-tail conclusions carry over. Note: protocol epoch 198 is also claimed by other open PRs (#5458, #5732, #5709), so whichever merges later must renumber.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
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
Shows Agent Graph output previews in the panel: operators get an output preview surface with dedicated copy and styles, backed by a protocol field in protocol/agent-graph.ts (with the agent-graph coordinator and protocol tests updated) and the execution-composition test pinning the wire shape. Stories, the renderer panel test, and the Windows test inventory move in step. test lane passes.
Findings
- [P3] Output previews surface child results in the panel — confirm the preview text passes through the same redaction path as other model-visible surfaces before render (the panel test exists, but redaction of preview content specifically should have one assertion; verification, not a proven defect).
Verdict
merge-ready — useful observability with the protocol surface tested; request the redaction assertion as a follow-up if not present.
# Conflicts: # packages/runtime-host/src/protocol/index.ts
|
Review/conflict follow-up on current head
|
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 7edccea. Relative to the previously reviewed 3b97e2e head, this merge commit preserves the Agent Graph preview behavior in all 17 PR-changed files; the substantive integration change advances the Runtime Host compatibility epoch from 198 to 199 after the new bounded operator-output field. I checked the bounded/coalesced projection, strict protocol shape, client read model, and Desktop rendering paths. I found no substantiated P0–P3 issue in the reviewed code.
Node 24 clean installation and test build passed, as did 119 focused Desktop/Runtime/Host tests, ASF headers, diff-check, and the current-head hosted test. However, the PR cannot merge against current main 0323714: merge-tree reports content conflicts in three Agent Graph test files. Please resolve these and re-run the gates on the resulting head. I did not test a live provider, packaged Electron, or native Windows/macOS. No schema migration is included. This is not a 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.
# Conflicts: # apps/desktop/src/main/__tests__/agent-graph-panel.test.ts # packages/runtime-host/src/__tests__/agent-graph-coordinator.test.ts # packages/runtime-host/src/__tests__/agent-graph-protocol.test.ts
|
@hqhq1025 Conflict follow-up on current head
|
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit c5ec253. This merge brings current main into the Agent Graph preview branch and resolves the three previously conflicting Agent Graph test files. The bounded output preview, metrics, strict wire shape, and compatibility epoch 199 remain in place. Current main is at epoch 198, so this PR has a distinct epoch against the present base. I found no new P0–P3 issue on this head.
A clean Node 24 installation and build:test, 57 focused Agent Graph/stream tests, renderer architecture check, Astryx inventory check, diff-check, current hosted test, and a merge-tree against current main pass. I did not exercise a live provider, packaged Electron, or native Windows/macOS. Another open PR also proposes epoch 199; merge order must be reconciled if that PR lands first. This is not a 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 #3714
Latest Verification
Current head
495dafe94is based on current main8b104db12. Epoch 198 advances main's epoch 197 exactly once for the Agent Graph operator output/metrics shape.Local validation on Node 24 passes: full test build, 26 focused Runtime read-model/coordinator tests, 24 Desktop panel tests, Biome on all changed files, strict renderer architecture against current main, and
git diff --check. The wake-currency ablation fails when output is put back intosnapshotVersion, proving the regression detects the issue. A real Chromium layout check at 240 px confirms the bounded preview wraps without horizontal overflow and keeps the newest tail visible. Hosted CI passed on this exact head.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.
Review Follow-up
snapshotVersionor structurallatestEventTime; durable runtime activity still advances the version.startOffsetafter projection rebuild is now marked as truncated.Screenshot Provenance
The supplied image shows the real Electron renderer using synthetic fixture data. It does not demonstrate live-provider measured TPS. No new packaged Electron screenshot was produced in this repair pass; the narrow-width browser layout was checked directly.
Review Focus
RuntimeEvents remain the output and usage authority. The bounded projection is presentation-only and excluded from supervisor wake currency. 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
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.