Repository navigation
feat(server): restore stream liveness on v2 - #184
Conversation
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: unavailable · PR result: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
lukemaj
left a comment
There was a problem hiding this comment.
What: Source review of the pinned 14-file #172 slice, cd6ad61ade9041311738afc3de49961a292a15d4 to 77d952dd8f55bbf596d2a83c657949687472e3d2, found no blocking or optional findings.
Why: Stream observation and stale-turn behavior are implemented in fork-owned modules, with only small upstream integration hooks.
So what: This review is for exact head 77d952dd8f55bbf596d2a83c657949687472e3d2; #184 remains draft. A fresh exact-head review is still required after #183 merges and #172 rebases.
Review
The routed provider stream reaches the clock after route ownership is established. Filtered events are observed in memory, delivered events are observed by the ingestor decorator, and the captured attempt ID fences observation and cleanup. The decorator also handles successful turn.terminal events whose normalization returns no stored events. The clock/store cover multiple provider turns, terminal outcomes, tool gaps and idempotent persistence. The detector reports silence only; open tools, host suspension, pending input and retirement suppress notices. The monitor queues one parent notice with queue_after_active; existing V2/Prism failure and recovery paths remain in control.
Production source composition wires the same layerPrismStreamClockProvided into the decorated ingestor, RunExecutionService hooks and production monitor; server.ts uses RuntimeLayer.layerProduction.
Modularity
| Upstream file in the #172 slice | Added | Removed | Review |
|---|---|---|---|
apps/server/src/orchestration-v2/RunExecutionService.ts |
10 | 0 | Original ingestion block is unchanged. Added hooks call fork helpers for attempt identity and filtered events, provide the attempt context once on the stream, and end the captured attempt. |
apps/server/src/orchestration-v2/runtimeLayer.ts |
21 | 1 | Layer, decorator and monitor registration only. Largest added hunk is 9 lines. |
This satisfies /tmp/166-assess/MODULAR.md: helper behavior and identity construction stay in streamClock.ts. The feature map gives RunExecutionService to prism-stream-clock; runtimeLayer.ts has its upstream owner under #169 prism-toolkit and is shared by #172. The allowlist likewise keeps one owner entry for each upstream edit. The PR body includes both files' #172-slice counts and the hunk-size justification.
Elon record
Requirements and who asked: User-approved #172 stream timing, idempotent per-turn storage, exact attempt fencing and silence-only parent notice; user requires fork modules with small upstream hooks.
Deleted: Inline RunExecutionService identity/event construction and ingestion-block restructuring; no feature cuts.
Bottleneck: #183 merge, then #172 rebase and final exact-head proof/review.
Checked myself: Read the full immutable 14-file diff and relevant consumers; checked production composition, feature ownership, allowlist and hunk/line counts. No proof commands were run.
Limits
This is a source-only review; author-reported tests and scoped checks were not rerun. The live fork/v2 ref is 8fc40da4fd603f343f13d6e51676b6f1e091b555; the PR API still reports base OID 6627c1afe97378cd0c2e4528dec20e8177acd133. This verdict covers only the explicitly pinned cd6…77d slice, not the future post-merge rebase.
77d952d to
57a59b4
Compare
Agent work on this PREstimated cost unknown · 0 responses · 45 sessions · 7.0 h wall time
Flags: 3 human corrections · 85 large tool outputs · 87 repeated commands · 10 repeated failures · 99 repeated reads · 12 repeated skill loads · 44 sessions with usage bound to no task · 1 session without usage records Details: snapshot, prices, coverage, counters
Token counters by model (native counter semantics; never added across semantics): Selected rates (USD per million tokens). These rates value the report at the selected schedule date; they do not establish historical prices or subscription spending.
Other output and reasoning are priced without double counting inclusive native output. Missing rates remain unknown. Local measurement from native records; usage totals are not billing. Updated in place by |
There was a problem hiding this comment.
What: Final independent source review of #172 at exact head 57a59b4fa1d26abe842a12827e776428dc2f2b9f found no source findings.
Why: The 14-file change keeps stream-clock behavior in fork modules and limits upstream edits to runtime hooks.
So what: Current CI passes after the same-head Server 4 retry; this review records source success on the exact SHA and makes no readiness or merge change.
Verdict and scope
Reviewed base 7963bda523bbec5e5366c44e4a6ea34a5b10039e to head 57a59b4fa1d26abe842a12827e776428dc2f2b9f, the complete 14-file #172 integration slice. I found no blocking or optional source issues. Range-diff against cd6ad61ade9041311738afc3de49961a292a15d4..77d952dd8f55bbf596d2a83c657949687472e3d2 shows the #172 source patch is unchanged; only the additive allowlist tail was resolved to retain both #169 and #172 entries. No dependency commits were replayed.
Modularity
/tmp/166-assess/MODULAR.md calls for fork behavior in fork modules and small upstream hooks. The final diff follows that boundary:
| Upstream file | Insertions | Deletions | Review |
|---|---|---|---|
apps/server/src/orchestration-v2/RunExecutionService.ts |
10 | 0 | Imports/gets the hook, starts and ends the captured attempt, observes filtered events, and provides attempt context before runDrain. The original ingestion block remains byte-for-byte unchanged. |
apps/server/src/orchestration-v2/runtimeLayer.ts |
21 | 1 | Composes the fork-owned stats, clock, ingestor decorator, and stale monitor in the production layer. The largest added hunk is 9 lines. |
Behavior and identity construction remain in apps/server/src/prism/*; there are no feature cuts or direction, client, version, or ACP edits. The feature inventory assigns the RunExecutionService hook to #172's prism-stream-clock; the shared runtime composition remains #169's owner, with one allowlist entry per upstream edit. Production composition reaches layerProduction, including the stale monitor and shared clock layer.
Behavior reviewed
I checked the filtered-event observation after route ownership, once-only delivered-event observation through the ingestor decorator, exact run/attempt fencing and cleanup, successful empty-terminal finalization, first-token timing across shortened paragraph snapshots and turn mode, silence-only detection, tool/input/sleep/retirement suppression, and parent queueing with the stored model/effort. V2/Prism remain owners of terminal failure delivery, and D28 is unchanged.
CI and proof
The first exact-head Test Server 4 attempt failed; the user-triggered second attempt on the same SHA passed. Per the user's decision, record this as a flake. Fresh gh pr checks showed every current check passing or skipped. The aggregate check passed. No parent CI rerun was performed.
The PR records 84 focused tests, 2 selected production-composition tests, scoped server typecheck/lint, and the 19-feature inventory as passing on this head. Those proof results are reported from the PR record; this review ran no tests or other proof commands.
Requirements and who asked: The user confirmed #172 liveness, SQLite persistence, filtered-event observation, attempt fencing, D6′/D28 behavior, and the MODULAR upstream boundary.
Deleted: Client changes, v1 sample import, per-event SQL clocks, duplicate terminal notices, and unrelated ACP fixture work.
Bottleneck: The exact-head independent source review was the remaining review gate; final CI now passes after the user-directed retry.
Checked myself: Live PR base/head and checks, the complete final diff and consumers, production composition and feature ownership, upstream insertion/deletion counts, and range-diff from the pre-rebase candidate.
Source review status applies only to this exact SHA. This review does not mark the PR ready or merge it.
What: Restore server-owned stream clocks and one parent notice for a silent child on v2.
Why: V2 reports terminal results but has no silence detector, and presentation buffering can hide healthy provider activity.
So what: This #172-only PR is ready for human review with exact-head proof, independent review and final CI passing; the user decides the merge.
Canonical decisions, topology and proof.
The ingestor decorator observes delivered events, including successful terminals that normalize to no stored events. The approved RunExecutionService hook observes filtered text in memory and fences observation and cleanup to the captured attempt. A fork-owned SQLite table stores one idempotent sample per finished run/provider turn and reloads healthy thresholds across restarts. It imports no v1 samples and writes no per-event clocks.
The silence monitor excludes host suspension, open tools, pending input and retired work. It queues one advisory parent message with the parent's stored model/effort. V2 and Prism retain terminal failure delivery and recovery. No client or version changes.
Final topology and upstream edit scope
Head
57a59b4fa1d26abe842a12827e776428dc2f2b9f, base7963bda523bbec5e5366c44e4a6ea34a5b10039e(fork/v2, includes merged #183). Only the two #172 commits were rebased after the temporary cd6 boundary. The additive allowlist conflict retained every entry from both sides. The final diff contains 14 #172 files, with no dependency replay, direction edits or ACP changes.Applied
/tmp/166-assess/MODULAR.md: algorithms and input construction stay in fork modules. The original ingestion block is byte-for-byte unchanged, with attempt context provided once on Stream. No added upstream hunk exceeds 9 lines; runtimeLayer's largest hunk only registers the shared layers. Inventory and allowlist retain their owners.Exact-head affected proof
All checks below ran on
57a59b4fa1d26abe842a12827e776428dc2f2b9fafter the final rebase:pnpm --filter t3 exec tsc --noEmitand targeted lint of all ten new TS files plus both upstream hooks passed.bash scripts/fork-check.sh --base 12069eefd707f78eafc27812027c994eea0613cf: 19 features and allowlist passed; diff whitespace passed.Node 24.13.1, docs/fork.md neutral environment, one test worker, no file parallelism, fresh load below 8 before each heavy command, all proof serial. No browser/live-provider/desktop-build verification or repo-wide local checks. Exact commands and results are in the canonical handoff.
Prior controlled regressions failed for the intended causes: empty terminals became aborted samples; missing filtered observation delayed/lost original first tokens. The candidate was restored and the final affected tests pass. Prior exact77d review was clean; Final exact-head independent source review passed, with no source findings and review/independent=success on57a. The user-triggered final CI rerun passed. This PR is ready and remains unmerged.
CI attribution and decision
#169-owned unused exports, settings restore fixtures and delegation/replay guidance fixes entered only through dependency merge.
The earlier ACP process-tree failure belongs to the unchanged upstream fixture: source/test equal the upstream pin and merged base. Hardcoded synthetic PIDs can collide with the worker; retained1004/11004 matches that mechanism, though the old log did not expose the actual worker PID. Upstream #16633, inspected head
c8175050793c3bc46995479f97ed0c6af2259ad0, fixes that fixture only and is not adopted here.Final exact57a attempt1 Server4 failed the unchanged fixture with retained1111/11111. The user reran that job once. Attempt2 Server4 passed on the same head:1251 tests passed/14 skipped,99 files passed/2 skipped. Current aggregate Check and the overall attempt2 run passed. All current CI checks and the exact-head independent review pass/skip.
Per explicit user decision this is recorded as a flake with both job links. The parent triggered no rerun; source stayed unchanged. No unrelated fixture port, second PR, altered policy or waiver. The upstream fixture candidate remains outside #172.
Agent cost record published. Sync failed with a locked ledger; the available snapshot reports unknown cost and incomplete usage attribution, not zero consumption.
Requirements and who asked: User-approved #56/#65 liveness through #166/#172; SQLite persistence and filtered observation explicitly approved; D6′/D28, modular hooks and one #172-only PR required.
Deleted: Client stream-clock hunks, duplicate provider dead/error notices, v1 sample import, superseded dependency composition, ingestion reindentation and unrelated ACP fix.
Bottleneck: Human review/merge decision; dependency rebase, proof, exact-head review and CI reconciliation are complete.
Checked myself: Live merged base and published head, two-commit rebase/range-diff, complete final integration diff, actual buffering/empty terminals, 84 focused and 2 composition test outputs, scoped types/lint and inventory, upstream line counts and CI source attribution.
Closes #172
Model/harness: GPT-6.1-Sol through Codex in T3 Code; SQLite slice by Claude Opus 5.5 through Claude Code in a bounded visible T3 worker. Independent review by GPT-6-Luna through Codex in a bounded visible T3 reviewer.