LiteShip 0.5.0 — make-it-loud + DAG compaction + SSE hardening#90
Conversation
Single source of truth: new pnpm.overrides.effect=4.0.0-beta.32 (mirrors pnpm.overrides.astro — physically pins one resolved version). Bump the 13 unbounded @CZap peer ranges >=4.0.0-beta.0 -> >=4.0.0-beta.32 <5 so every published package advertises the same bounded library range. The create-liteship template now ships effect ^4.0.0-beta.32, fixing the unmet peer a fresh `npm create liteship` hit from @czap/core + @czap/astro. Gate: tests/unit/devops/effect-version-sync.test.ts — clones the astro-version-sync drift guard with a 3-way role-derived expected (lib peers/deps=R, dev pin=V, example/app=V, template=^V), derived from the override; folds the supply-chain-policy prose pin so it can't go stale. First items of the 0.5.0 make-it-loud program (plan: declarative-plotting-grove). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
A client:gpu host with no layout at boot (clientWidth/clientHeight = 0) sized its canvas to the 300x150 HTML default and rendered the shader into a tiny offscreen buffer with no console error — the silent-boot family the #77 directive-collision warn already addresses. Add a shared warnIfHostUnsized helper (Diagnostics.warnOnce 'canvas-default-size', source czap/astro.gpu) called in both shader paths; it covers the wrapper-element path AND a directly-passed <canvas> whose own layout is 0 (the render loops fall back per-frame there too — the coverage gap Wave-2 flagged). Gate: tests/unit/astro/gpu-canvas-default-size.test.ts — unsized wrapper + direct canvas + WGSL branch each warn once; sized host stays quiet; the fallback dims in the warn detail are imported from @czap/core, not literals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
Wave-2 found the existing consumer-mode suite already proves error===0 on clean fixtures and that unknown-internal-package errors survive — so #4 needs no engine change, only the one uncovered class: a real LAYERING violation between two INSTALLED packages (@acme/core importing @acme/app against an empty allowedInternalImports) must stay a hard package-topology error in consumer mode. Locks the surgical-suppress decision so a future "just suppress structure in consumer mode" change reds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
genui silently dropped any registered on* handler that wasn't onClick (and a
non-string onClick) at render — author intent vanished with no signal. Make it
LOUD via the existing validation channel: a shared /^on[A-Z]/ predicate
(new packages/genui/src/interaction.ts — narrow, so data props like
online/once are NOT escalated) + a usage-gated reject in validate.ts through
the existing closed genui/invalid-prop code, which makes the silent render-side
continue provably unreachable. renderFromCatalog now returns a tagged
RenderFromCatalogResult (not a bare boolean) so a rejection surfaces WHY; the
astro llm-session maps it back to the boolean host contract and emits
Diagnostics.warnOnce('genui-render-rejected'). genui stays @czap/core-free.
Gates: render.test.ts reject tests (were "silently ignores"); validate.test.ts
source-of-truth coverage (validate rejects iff the predicate+wire rule says so,
so validate and the renderer can't drift). genui+astro `tsc --build` clean.
Surface change (renderFromCatalog return type) — api-surface snapshot regen +
coordinated 0.5.0 bump are batched into the release step with A3/A5.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
Recognized signal inputs with no live producer on a directive (pointer.*, time.*, media:*, custom:*, audio.sample/normalized) silently froze; Wave-2 only half-caught this by MISLABELING recognized vocabulary as a typo. Add classifySignalServing + warnIfSignalUnserved to boundary.ts (two disjoint codes: signal-input-unknown for typos, signal-input-unserved-here for frozen) and call once at the six directive setup sites (satellite, worker, svg, graph-runtime, scene-bridge, uniform-signal). The uniform-signal mislabel block is replaced, keeping typo detection. Gate derives expected from classifySignalServing.
Add DAG.checkpoint/spliceCheckpoint (drop-only reclamation that rebuilds from survivors -- never re-points a content-addressed parent), a genesis-shaped checkpoint envelope returned out-of-band, and Receipt.ChainValidationOptions (base/checkpoint) so a compacted tail validates against its watermark with a typed checkpoint_invalid variant. Wire auto-compaction into the astro LLM receipt tracker off the hot path (rememberEnvelope/replayGap, debounced, below last-ack minus a retention margin). Mirror the surface into _spine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
…ix (A3)
Harden the SSE receive primitive with a selective overflow policy and an
edge stream, and fix the latent heartbeat watchdog that wedged the stream
in `error` instead of reconnecting.
- types: add `OverflowPolicy` ('drop-newest' | 'drop-oldest' |
'coalesce-by-id'; 'block' intentionally absent — onmessage cannot
suspend), `SSEConfig.overflow` (partial-over-default), and enrich
`BackpressureHint` with `policy`/`droppedCount`/`coalescedCount`.
- sse-pure: `defaultOverflowPolicy` (coalesce-by-id), pure `extractCoalesceKey`
(only id-bearing `patch` is keyed; ambiguity -> null so ordered/keyless
messages like LLM tokens never coalesce) and pure `applyOverflow`
(FIFO backbone + in-place same-id supersede; saturation evicts the
oldest keyed idempotent patch before any token).
- sse: thread the policy through a drain -> applyOverflow -> rebuild pass
over the bounded Queue using sync ops (takeUnsafe/offerUnsafe/sizeUnsafe);
retire the hand-tracked bufferSize + Stream.tap (Queue.sizeUnsafe is the
backpressure source of truth); warnOnce 'sse-buffer-saturated' on first
saturation; add `stateChanges` (deduped transition edge stream); funnel a
heartbeat timeout through the SAME reconnect path as onerror.
- _spine/web.d.ts: mirror the new public surface.
Gates: tests/property/sse-overflow.test.ts (fast-check over interleaved
token+keyed-patch sequences: no token drop/reorder/merge while a patch is
evictable; same-id coalesces to newest; bound <= SSE_BUFFER_SIZE; BITE
warnOnce) + component test updates (heartbeat-reconnect, stateChanges edges).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
…imitive (A3b) Make A3's overflow/coalesce/backpressure + unified reconnect/resumption LIVE in the framework (the directives previously hand-rolled raw EventSource and bypassed SSE.create): - client:stream — FULL migration to SSE.create via the scene/runtime.ts Scope bridge (Scope.make/use + a top-level forked drain over client.messages / client.stateChanges; Scope.close on czap:teardown disposes EventSource + Queue + fibers together). Preserves the createStreamScheduler rAF batching, the message-type branch, saveResumptionState/reconcileResumption on the reconnect->connected stateChanges edge, and the lifecycle CustomEvents. Default overflow coalesce-by-id (patches are data-czap-id-addressed). Documents the forkScoped-inside-runSync pumping subtlety. - client:llm — adopts the Scope-bridge lifecycle but KEEPS its raw EventSource + its own already-guarded decodeLLMEventData over raw frames. Security-locked (owner's call): the LLM token protocol uses bare-string payloads that SSE.create's MANDATORY parseMessage preflight drops, so the llm path deliberately does NOT route through SSE.create.messages and adds NO public decode bypass to the primitive (sse.ts untouched) — the preflight invariant holds by construction. Gate: tests/unit/astro/stream-llm-lifecycle.test.ts (jsdom EventSourceMock) — dispose closes the connection, VT-swap single-boot survival, heartbeat reconnect. tsc --build packages/web packages/astro clean. Completes the SSE work; api-surface regen + 0.5.0 bump are the release step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
… gap) Wave/self-audit caught a plan item I dropped: the effect-version-sync guard swept the manifests + policy prose but NOT the test files that embed the effect floor/range as fixtures (supply-chain-lib.test.ts, feedback-integrity, codebase-audit, satellite-scan). On a future floor bump those literals would rot silently. Pin them so a bump reds them for a coordinated update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
#3 (High, soundness): Receipt.validateChainDetailed widened the index-0 genesis predicate on `base` ALONE — a caller could validate a TRUNCATED chain by passing base = tail[0].previous with no proof the omitted prefix was ever checkpointed. Now reject base without a verified checkpoint attestation (checkpoint_invalid). No production caller passed base alone; the dag-compaction property test is updated to the corrected contract (base+checkpoint required). #4 (race): receipt-chain.compactBelow computed the dropped set from the pre-mint DAG, then spliced the CURRENT DAG by it — a concurrent ingest of a late child of a dropped node would orphan it. Snapshot the DAG identity before the async mint and abort if it changed (compaction is best-effort; the next trigger retries). #2 (High, leak): client:llm terminal paths (server error chunk + 'done') closed only the EventSource, leaking the scoped queue + drain fiber until a later teardown/reinit. Fully closeScope() on terminal frames. Codex #1 (the A3b flaky-hang from the global Effect runtime + fire-and-forget teardown) remains — it needs the disposable per-directive runtime fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
The A4 renderFromCatalog union edit left a format:check violation that the --no-verify commits hid (surfaced by Codex's pre-commit run). lint + format:check now pass clean across packages/tests/scripts on the integrated branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
…addendum, CHANGELOG 0.5.0 Owner-approved (SSE as an ADR-0005 addendum, not a standalone ADR). Index updated; doc-registry test green; lint (incl. tsdoc) green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
…doc notExported) docs:check's typedoc build (treatWarningsAsErrors) failed because new exports reference types that weren't themselves re-exported from the package barrel — typedoc's default notExported validation. renderFromCatalog returns RenderFromCatalogResult (genui), DAG.checkpoint returns CheckpointResult and Receipt.validateChain takes ChainValidationOptions (core); all three were standalone exports in their modules but missing from index.ts. Re-export them. Diagnosed statically (typedoc needs ~8GB to load the 24-pkg graph; this machine has 6.5GB, so a full docs:build can't run/verify here — a 4GB-capped run OOMs on load before emitting warnings). Final typedoc-clean verification is CI/RAM-bound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
Coordinated 0.5.0 minor across all 26 manifests + the create-liteship template @czap/* ranges (^0.5.0); lockfile refreshed. api-surface snapshot regenerated for the new public exports (DAG.checkpoint/spliceCheckpoint + CheckpointResult, Receipt ChainValidationOptions, SSE OverflowPolicy/stateChanges/BackpressureHint fields, genui RenderFromCatalogResult) — the 0.4.1->0.5.0 minor satisfies the api-surface semver gate. Local-safe gates green: version-coupled (scaffold drift, rosters, doc-registry, effect-sync), api-surface, and meta/spine/genui/dag sweeps (694 tests). The heavy gates (full suite incl. capsule-verify, docs:build/typedoc + docs/api regen) are CI/RAM-bound and run on the PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 012ad409bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| drainFiber = nextRuntime.runFork( | ||
| Stream.runForEach(Stream.fromQueue(rawQueue), (raw) => Effect.sync(() => handleFrame(raw))), | ||
| { scheduler }, |
There was a problem hiding this comment.
When an LLM EventSource fires onerror, handleDisconnect still only closes the source and never calls closeScope. With this newly-created raw queue/drain fiber/runtime, a transient transport failure leaves the fiber blocked on Stream.fromQueue and the Scope/ManagedRuntime alive until a later teardown or reinit. Pages with failed LLM streams that stay mounted leak one runtime per failed connection; close the scope on the disconnect path after replay bookkeeping.
Useful? React with 👍 / 👎.
… from A3b) A3b migrated client:stream/client:llm onto an async drain — stream consumed SSE.create.messages on a forked fiber, llm drained a sliding queue on a fiber — which slipped message processing a macrotask behind each onmessage. That broke the directives' synchronous morph/token contract that ~30 existing directive tests (node + browser) and the live render timing depend on: a patch/snapshot/ token simulated then asserted within one microtask saw empty output. CI caught it (browser-e2e + macos/windows-smoke); the worktree agents could not run the browser/directive suites locally, so it shipped unverified. NOT a revert — the async Stream model is fundamentally wrong for these synchronous consumers. Fix: SSE.create gains optional synchronous onMessage/onStateChange callbacks, invoked inside onmessage/setStatus after the mandatory parseMessage preflight, bypassing the async Stream + overflow buffer (a synchronous consumer holds no buffer, so there is nothing to overflow). client:stream uses them and keeps a Scope only for connection teardown (no drain fibers); client:llm processes handleFrame synchronously in onmessage and drops the queue/fiber/runtime machinery entirely (raw EventSource was always there; the parseMessage-bypass security rationale for the LLM bare-string path is unchanged). The buffered messages/stateChanges Streams + overflow stay for genuine async consumers (sse-overflow.test.ts still green). - Rewrote the 2 A3b lifecycle tests to assert the corrected behavior (sync delivery bypasses the buffer; a token flood is processed in order, nothing dropped). Updated one astro-directives reconnect assertion: SSE.create's heartbeat watchdog legitimately clears+resets its own timer per message, so the stale pre-heartbeat `clearTimeout`-never expectation no longer holds. - Corrected the ADR-0005 SSE addendum + CHANGELOG (they described the superseded async ManagedRuntime-drain design) to the shipped synchronous design. - Mirrored onMessage/onStateChange into _spine/web.d.ts (ADR-0010). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3196059ca3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| return { | ||
| dag: spliceCheckpoint(dag, dropSet), |
There was a problem hiding this comment.
When a checkpoint is taken below a watermark W that already has retained children, this drops W from the compacted DAG while those children still name W in previous. If a remote envelope from the same actor later arrives with previous === W, checkForkRule can no longer inspect W's child list because the parent node is gone, so DAG.merge accepts a sibling fork that the un-compacted DAG would have rejected. This affects replicas that continue merging receipts after compaction below a watermark with retained children; keep enough frontier/child metadata or reject new children of compacted parents.
Useful? React with 👍 / 👎.
| // Synchronous consumer: deliver in-turn and skip the async buffer/Stream | ||
| // entirely. A synchronous consumer holds no buffer, so there is nothing | ||
| // to overflow; `parseMessage` already gated this message upstream. | ||
| config.onMessage(message); |
There was a problem hiding this comment.
When onMessage is configured, a message queued from an EventSource that has just been closed during client.close/scope finalization or a VT reinit still invokes this synchronous sink because the close paths only call close() and leave the handlers installed with no source-identity guard. The old buffered path would hit a shut-down queue, but this bypass can still mutate the DOM after czap:teardown or let an old generation race the new one; clear handlers or ignore events whose source is no longer machine.source before calling the sink.
Useful? React with 👍 / 👎.
…econnect test Same stale expectation as the node astro-directives reconnect test: this browser test asserted clearTimeout was never called, but the directive now runs on SSE.create whose heartbeat watchdog legitimately clears+resets its own timer per message and on teardown. The meaningful assertions (source.closed on reinit, lastEventId resumption cursor, 11 disconnects + max-reconnect-attempts error) all pass; only the pre-heartbeat clearTimeout-never check was stale. This was the last of the 4 browser stream/llm tests broken by A3b's async drain — the other 3 were fixed by the synchronous-processing change (3196059). Codex review P2 (tear down LLM runtime after a transport error) is resolved by that same rewrite: client:llm no longer owns a runtime/scope, so onerror -> handleDisconnect (close source + reconnect) leaks nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…sion One batch addressing the reinit-cursor regression the browser e2e caught plus all four inline review findings (Codex/Greptile), verified locally so CI runs once. - Reinit cursor (browser e2e): SSE.create tracks lastEventId per-connection, so a czap:reinit opened a fresh connection that restarted the tail from the top. The directive now carries `lastCursor` across connections and re-seeds the replacement, resuming where the swapped-out connection left off. Node-tested. - Codex P1 (dag.ts) — anti-fork rule must survive compaction: dropping watermark W with retained children left a later `previous === W` fork undetectable (checkForkRule bailed once W's node was gone). It now falls back to scanning the retained nodes that still name a missing parent, so the fork is still rejected — with NO extra DAG state, preserving spliced==fresh-reload. Pinned by test (H). - Codex P2 (sse.ts) — stale message after teardown invoked the synchronous onMessage sink: new `detachSource()` clears the EventSource handlers BEFORE close on the intentional teardown/reinit paths (NOT the internal reconnect), so a queued frame can't morph into a newer generation. - Greptile P1 (stream.ts) — old source live during the reinit window: closeClient now closes the Scope SYNCHRONOUSLY (its finalizer runs detachSource before the replacement opens), so the old generation can't deliver a stale frame. - Codex P2 (llm.ts) — LLM runtime leak on transport error: already resolved by the synchronous rewrite (3196059) that removed the runtime/scope entirely. The broad `machine.source !== source` guard was tried and rejected: it also blocked the reconnect-gap messages ~6 resumption tests legitimately depend on. The targeted handler-clear is correct without that collateral. Verified: 1418-test astro/component/meta sweep + dag/sse suites green; lint/format/api-surface/ check-invariants clean. ADR-0026 updated for the fork-survives-compaction property. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR releases v0.5.0 across the monorepo: adds receipt-DAG checkpoint compaction (core + Astro auto-compaction), hardens SSE with overflow policies, backpressure metrics, and a state-change stream, tightens GenUI to a single onClick interaction contract, adds unserved-signal/GPU-canvas diagnostics, pins the effect dependency, and updates docs/tests/snapshots accordingly. ChangesReceipt-DAG Checkpoint Compaction
SSE Overflow and Directive Hardening
GenUI Single-Interaction Contract
Unserved-Signal and GPU Canvas Diagnostics
Effect Version Pin and Release Housekeeping
Cell-DOM Value Boundary Guard
Allocation-Gate Measurement Robustness
Sequence Diagram(s)sequenceDiagram
participant Directive as "client:stream directive"
participant SSE as "SSE.create client"
participant EventSource
participant Scheduler as "StreamScheduler"
Directive->>SSE: openClient(url, lastCursor)
SSE->>EventSource: new EventSource
EventSource-->>SSE: onmessage(frame)
SSE->>Directive: onMessage(message)
Directive->>Directive: applyOverflow(buffer, message, policy)
Directive->>Scheduler: enqueue patch
EventSource-->>SSE: onerror
SSE->>SSE: handleConnectionLoss
SSE->>Directive: onStateChange('reconnecting')
Directive->>Scheduler: beginReconnect
SSE->>EventSource: reopen after backoff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3ddc32706
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (let i = 0; i < buffer.length; i++) { | ||
| if (extractCoalesceKey(buffer[i]!) === key) { | ||
| // Supersede in place: keyless neighbors keep their order. | ||
| buffer[i] = message; |
There was a problem hiding this comment.
For async SSE.create consumers, coalescing a later same-id patch by overwriting the earlier buffer slot reorders it ahead of any intervening order-significant messages. A sequence like patch A v1, signal S, patch A v2 is delivered as patch A v2, signal S, so S observes a future patch even though signal messages are documented as keyless/FIFO. Delete the old patch and append the replacement after intervening keyless entries, or only coalesce when no ordered message sits between the old and new patch.
Useful? React with 👍 / 👎.
| // an out-of-order parent simply has no children yet, so no false fork. | ||
| for (const node of dag.nodes.values()) { | ||
| if ( | ||
| node.envelope.previous === prevHash && |
There was a problem hiding this comment.
Fresh evidence for the compaction anti-fork path: the missing-parent scan only matches retained children whose previous is the string watermark, so a retained merge child with previous containing the compacted watermark is invisible. Before compaction, that merge child is in the watermark node's children list and a same-actor sibling from the watermark is rejected; after compaction the same fork is accepted. Check array parents here as well so merge receipts do not bypass the anti-fork rule.
Useful? React with 👍 / 👎.
| (base !== undefined && | ||
| (firstPrev === base || (Array.isArray(firstPrev) && (firstPrev as readonly string[]).includes(base)))); |
There was a problem hiding this comment.
When base is supplied, the first retained envelope is accepted as the new index 0, but the loop only enforces HLC monotonicity for i > 0. In a compacted linear tail where tail[0].previous === base, a self-consistent envelope with an HLC equal to or older than the checkpoint/base now validates, whereas the full chain would fail at the prefix boundary. Compare the first tail timestamp with the verified checkpoint timestamp before accepting the compacted tail.
Useful? React with 👍 / 👎.
…browser red) The browser reconnect test asserted czap:stream-disconnected fired 11 times — once per failed attempt, the old hand-rolled directive's behavior. The SSE.create model coalesces consecutive `reconnecting` transitions, so disconnection is surfaced per episode and the retry budget terminates in a single max-reconnect-attempts error (verified: no error spam). - New NODE gate (astro-directives) reproduces the reinit + exhaust-retries flow in jsdom + MockEventSource, firing the freshly-scheduled RECONNECT timer (not the heartbeat leftovers the stubbed clearTimeout leaves in the queue): asserts loss is surfaced AND exactly one terminal error. This is the local coverage the browser-only test never had — the root reason these kept slipping to CI. - Browser test updated to that contract: disconnects > 0 + errors contains the terminal max-reconnect-attempts error (robust to browser timer interleaving I can't reproduce locally). Last assertion in the last A3b-affected browser test; the cursor fix (d3ddc32) already cleared the assertion before it. Node suite green (19). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/stream/sse.ts (1)
274-291: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard EventSource handlers against stale sources.
After
handleConnectionLoss()orreconnect()closes an old source, its queuedonmessage/onerrorcan still run because handlers are left attached. Since the handlers do not checkmachine.source === source, stale frames can be ingested and stale errors can close the replacement connection.Proposed fix
source.onmessage = (event: MessageEvent) => { + if (machine.source !== source) { + return; + } const message = parseMessage(event); if (message) { if (event.lastEventId) { machine.lastEventId = event.lastEventId; } @@ }; source.onerror = () => { + if (machine.source !== source) { + return; + } handleConnectionLoss(); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/stream/sse.ts` around lines 274 - 291, The EventSource handlers in the SSE connection setup can still fire after a stale source has been closed and replaced, so guard both the onmessage and onerror callbacks with a check that machine.source still matches the captured source before calling ingest, handleConnectionLoss, or updating state. Update the handlers in the stream logic around source.onmessage and source.onerror so only the active EventSource instance can process messages or trigger reconnect/error handling.
🧹 Nitpick comments (8)
tests/unit/meta/cell-dom-boundary.test.ts (3)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DOM_SINKtoken list omits several common write/bind sinks.The sink list catches
querySelector/getElementById/createElement/innerHTML/.textContent =/setAttribute/appendChild/.append(, but notouterHTML,insertAdjacentHTML,replaceWith(,prepend(,before(,after(,classList.add/remove, or direct style/attribute-property assignment — all legitimate value→DOM write paths a futuresignal.ts/zap.tschange could introduce undetected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/meta/cell-dom-boundary.test.ts` around lines 51 - 52, Expand the DOM_SINK pattern in the cell DOM boundary test so it also matches additional write/bind sinks beyond the current set. Update the token list in DOM_SINK to cover symbols like outerHTML, insertAdjacentHTML, replaceWith, prepend, before, after, classList.add/remove, and direct style/attribute-property assignments, so future changes in signal.ts or zap.ts that write to the DOM are detected by the test.
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing word boundaries on
document/windowcreate false-positive failure risk.
\bElement\bis explicitly boundary-guarded, butdocumentandwindoware bare substrings in the same alternation. A future identifier likedocumentId,windowSize, orwindowedAverageincell.ts/store.ts/derived.ts/live-cell.tswould trip the STRICT tier despite having nothing to do with the DOM, causing an unrelated build break.♻️ Proposed fix for consistency
-const STRICT_DOM = - /document|window|querySelector|getElementById|createElement|innerHTML|textContent|setAttribute|appendChild|HTMLElement|\bElement\b/; +const STRICT_DOM = + /\bdocument\b|\bwindow\b|querySelector|getElementById|createElement|innerHTML|textContent|setAttribute|appendChild|HTMLElement|\bElement\b/;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/meta/cell-dom-boundary.test.ts` around lines 45 - 46, The STRICT_DOM matcher is too broad because `document` and `window` are bare substrings, so unrelated identifiers can falsely trigger the DOM check. Update the regex in `STRICT_DOM` to use word boundaries for `document` and `window`, matching the existing handling used for `Element`, so `cell.ts`/`store.ts`/`derived.ts`/`live-cell.ts` identifiers like `documentId` or `windowSize` won’t be misclassified.
37-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNaive comment stripper is unaware of string/template literals.
codeOnlystrips//...based on the preceding character not being:, but doesn't track whether the//occurs inside a string or template literal. A line like"const path = '/usr//double-slash'"would be misread as having a comment starting mid-string, truncating everything after it from the "code" view — potentially hiding a realDOM_SINKmatch later on the same line and producing a false negative in the very guard meant to catch regressions.Consider a small lexer-aware stripper (or a well-tested library) instead of the regex heuristic, at least for cases where DOM-sink tokens could appear after an in-string
//.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/meta/cell-dom-boundary.test.ts` around lines 37 - 41, The codeOnly comment stripper is too naive because it removes // text even when it appears inside string or template literals. Update codeOnly to use a lexer-aware approach (or a proven parser utility) instead of the current regex heuristic, so it only strips real comments and preserves code after in-string // sequences; focus the fix in codeOnly and the DOM sink scanning path that depends on it.tests/unit/devops/effect-version-sync.test.ts (1)
127-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded literal-file list is a maintenance trip-wire.
The four test files in this list must be manually kept in sync any time the
effectfloor changes; a new file embedding the literal version string elsewhere in the repo won't be caught by this guard. Since the comment already acknowledges this ("the manifest sweep above does NOT cover them"), consider grepping the test tree for the literal pattern instead of maintaining an explicit allowlist, so newly added literal-bearing files are automatically covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/devops/effect-version-sync.test.ts` around lines 127 - 141, The guard in effect-version-sync.test.ts is relying on a hardcoded allowlist of literal-bearing test files, so new files that embed the effect floor can slip through unnoticed. Update the check in the literal-bearing test case to discover matching test files dynamically by scanning the test tree for the embedded version pattern, and then assert each match contains the current floor value. Keep the existing manifest sweep and reuse the current helper logic around readFileSync, join, and the R/V constants, but remove the static file array so the coverage stays automatic.tests/unit/astro/signal-not-served.test.ts (2)
81-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDedup test only covers repeated calls with the same input.
This pins same-input dedup across repeated setup reads, but doesn't verify behavior when two different unserved/typo inputs share the same
source+codepair (relevant givensvg.tscallswarnIfSignalUnservedonce per entity in a loop — see comment onpackages/astro/src/runtime/svg.tsline 215). IfwarnOnce's dedup key excludes the input/message, a second distinct misconfiguration on the same directive surface would be silently swallowed. Consider adding a case with two different unserved inputs to confirm both warnings surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/astro/signal-not-served.test.ts` around lines 81 - 85, The current dedup test in signal-not-served only checks repeated calls with the same input, so extend it to cover two different unserved/typo inputs that still share the same source and UNSERVED_CODE. Update the test around warnIfSignalUnserved and warnOnce to assert both warnings are emitted, ensuring the dedup key does not collapse distinct inputs into one event.
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTautological assertion —
c === UNKNOWN_CODE && c === UNSERVED_CODEis always false for any singlec.This filter predicate can never match (a single value can't equal two different constants simultaneously), so the assertion always trivially passes regardless of actual behavior. Line 77 already covers the real disjointness check via
includes. Consider dropping line 76 or rewriting it to express the intended check.Suggested cleanup
- // The two failure codes are disjoint by construction — never both. - expect(codes.filter((c) => c === UNKNOWN_CODE && c === UNSERVED_CODE)).toHaveLength(0); + // The two failure codes are disjoint by construction — never both. expect(codes.includes(UNKNOWN_CODE) && codes.includes(UNSERVED_CODE)).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/astro/signal-not-served.test.ts` at line 76, The assertion in signal-not-served.test.ts is tautological because the filter in the test references both UNKNOWN_CODE and UNSERVED_CODE on the same value, so it can never match; update the test around the codes assertion to remove this redundant check or rewrite it to express the intended expectation using the existing UNKNOWN_CODE and UNSERVED_CODE values, keeping the disjointness validation in the test readable and meaningful.packages/_spine/core.d.ts (1)
900-907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated
checkpointdeclaration.Line 905 declares the same public method twice; keep a single signature so the spine and generated docs don’t expose duplicate overloads.
Proposed cleanup
*/ checkpoint(dag: ReceiptDAG, options: { readonly below: string }): Effect.Effect<CheckpointResult>; - checkpoint(dag: ReceiptDAG, options: { readonly below: string }): Effect.Effect<CheckpointResult>; /** Rebuild the DAG from its survivors after dropping a checkpoint region (pure). */ spliceCheckpoint(dag: ReceiptDAG, dropSet: ReadonlySet<string>): ReceiptDAG;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/_spine/core.d.ts` around lines 900 - 907, The public API in the core type declarations has a duplicated checkpoint signature, which causes the same method to be exposed twice in the spine and generated docs. In the ReceiptDAG-facing declarations, keep only one checkpoint declaration and remove the extra duplicate overload, leaving spliceCheckpoint unchanged so the API surface remains a single, clean signature.tests/component/sse-client.test.ts (1)
275-289: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify the scheduled reconnect was cancelled.
The title claims manual
reconnect()"supersedes the scheduled one," but the assertions only check that a newEventSourcewas created — they don't advance time past the original heartbeat-scheduled delay to confirm no extra instance later appears if that timer wasn't cancelled.♻️ Suggested strengthening
yield* client.reconnect(); expect(MockEventSource.instances.length).toBe(countBefore + 1); expect(yield* client.state).toBe('connecting'); + + // Confirm the original heartbeat-scheduled reconnect was actually + // cancelled, not just raced — no extra instance should appear later. + vi.advanceTimersByTime(5000); + expect(MockEventSource.instances.length).toBe(countBefore + 1); }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/component/sse-client.test.ts` around lines 275 - 289, The test manual reconnect after a heartbeat timeout supersedes the scheduled one does not prove the original reconnect timer was cancelled. Update this test in tests/component/sse-client.test.ts to use the existing SSE.create, client.reconnect, and MockEventSource.instances checks, then advance the fake timers past the original heartbeat reconnect delay after the manual reconnect. Assert that no additional EventSource instance is created beyond the manual reconnect and that client.state remains stable, so the test verifies the scheduled reconnect was actually superseded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/astro/src/runtime/llm-receipt-tracker.ts`:
- Around line 66-74: The _compacting flag in llm-receipt-tracker should not be
cleared by stale async completions from an earlier compaction after reset()
starts a new chain. Update the compaction flow around the
chain.compactBelow(watermark) promise so each run captures a unique epoch/token
and only sets _compacting = false if it is still the latest generation. Apply
the same guard in the reset-related compaction path as well, using the existing
_compacting state and the compaction trigger points to prevent overlapping
compactions.
In `@packages/core/src/dag.ts`:
- Around line 180-199: The missing-parent branch in dag.ts’s anti-fork check
only scans nodes where envelope.previous equals prevHash, so it misses retained
merge children after compaction. Update the same-actor fork detection in the dag
anti-fork logic to mirror the normal parent-present path by checking retained
children whose previous includes the parent hash, including merge receipts,
before allowing a new child from the same actor.
In `@packages/core/src/receipt.ts`:
- Around line 273-285: The checkpoint validation in receipt.ts is missing a
receipt-kind check, so a genesis-shaped attestation with the same subject id can
slip through. Update the validation logic in the checkpoint receipt path to
explicitly require the attestation to be a checkpoint receipt, and only then
continue with the existing hash/genesis/subject id checks. Use the existing
symbols like DAG.checkpoint, checkpoint, and isGenesis to locate the validation
block and add the kind/type guard alongside the current checkpoint_invalid
failures.
- Around line 238-240: The empty-chain fast path in receipt validation currently
returns before the base/checkpoint checks, so `validateChainDetailed` can accept
an empty tail without verifying authorization. In `receipt.ts`, update
`Effect.gen` in `validateChainDetailed` so the `chain.length === 0` return
happens only after `base` is bound and the checkpoint/authorization verification
has completed, keeping the empty-chain optimization but not bypassing
validation.
In `@packages/web/src/stream/sse.ts`:
- Around line 241-253: The reconnect handler in handleConnectionLoss can leave
an older timeout active if another loss signal arrives before the first retry
fires. Before assigning a new machine.reconnectHandle in setupSource scheduling,
clear any existing pending reconnect timer and reset the handle so only one
reconnect attempt is active at a time. Use the handleConnectionLoss logic and
machine.reconnectHandle as the key places to update.
In `@tests/property/sse-overflow.test.ts`:
- Around line 86-106: Strengthen the token-preservation check in the SSE
overflow test by asserting not just that keyless count never drops, but that any
change to the keyless set preserves the exact incoming token sequence. In the
test around the buffer/keylessAfter logic and survivingTokenNs extraction,
verify that the surviving token values remain a strict ordered subsequence of
insertedTokenOrder and that no previously buffered token disappears when a new
token is appended, even if the total keyless count stays the same.
In `@tests/unit/astro/stream-llm-lifecycle.test.ts`:
- Around line 1-21: Update the docstring in the `stream-llm-lifecycle.test.ts`
header so point 4 matches the actual lifecycle contract and ADR-0005: remove the
claim that `client:llm` uses `drop-oldest` and emits a dropped-count diagnostic,
and instead describe overflow behavior only for the buffered `Stream` path. Keep
the wording aligned with the tests and the `client:stream` / `client:llm`
synchronous bridge semantics so the comment no longer contradicts the assertions
below.
---
Outside diff comments:
In `@packages/web/src/stream/sse.ts`:
- Around line 274-291: The EventSource handlers in the SSE connection setup can
still fire after a stale source has been closed and replaced, so guard both the
onmessage and onerror callbacks with a check that machine.source still matches
the captured source before calling ingest, handleConnectionLoss, or updating
state. Update the handlers in the stream logic around source.onmessage and
source.onerror so only the active EventSource instance can process messages or
trigger reconnect/error handling.
---
Nitpick comments:
In `@packages/_spine/core.d.ts`:
- Around line 900-907: The public API in the core type declarations has a
duplicated checkpoint signature, which causes the same method to be exposed
twice in the spine and generated docs. In the ReceiptDAG-facing declarations,
keep only one checkpoint declaration and remove the extra duplicate overload,
leaving spliceCheckpoint unchanged so the API surface remains a single, clean
signature.
In `@tests/component/sse-client.test.ts`:
- Around line 275-289: The test manual reconnect after a heartbeat timeout
supersedes the scheduled one does not prove the original reconnect timer was
cancelled. Update this test in tests/component/sse-client.test.ts to use the
existing SSE.create, client.reconnect, and MockEventSource.instances checks,
then advance the fake timers past the original heartbeat reconnect delay after
the manual reconnect. Assert that no additional EventSource instance is created
beyond the manual reconnect and that client.state remains stable, so the test
verifies the scheduled reconnect was actually superseded.
In `@tests/unit/astro/signal-not-served.test.ts`:
- Around line 81-85: The current dedup test in signal-not-served only checks
repeated calls with the same input, so extend it to cover two different
unserved/typo inputs that still share the same source and UNSERVED_CODE. Update
the test around warnIfSignalUnserved and warnOnce to assert both warnings are
emitted, ensuring the dedup key does not collapse distinct inputs into one
event.
- Line 76: The assertion in signal-not-served.test.ts is tautological because
the filter in the test references both UNKNOWN_CODE and UNSERVED_CODE on the
same value, so it can never match; update the test around the codes assertion to
remove this redundant check or rewrite it to express the intended expectation
using the existing UNKNOWN_CODE and UNSERVED_CODE values, keeping the
disjointness validation in the test readable and meaningful.
In `@tests/unit/devops/effect-version-sync.test.ts`:
- Around line 127-141: The guard in effect-version-sync.test.ts is relying on a
hardcoded allowlist of literal-bearing test files, so new files that embed the
effect floor can slip through unnoticed. Update the check in the literal-bearing
test case to discover matching test files dynamically by scanning the test tree
for the embedded version pattern, and then assert each match contains the
current floor value. Keep the existing manifest sweep and reuse the current
helper logic around readFileSync, join, and the R/V constants, but remove the
static file array so the coverage stays automatic.
In `@tests/unit/meta/cell-dom-boundary.test.ts`:
- Around line 51-52: Expand the DOM_SINK pattern in the cell DOM boundary test
so it also matches additional write/bind sinks beyond the current set. Update
the token list in DOM_SINK to cover symbols like outerHTML, insertAdjacentHTML,
replaceWith, prepend, before, after, classList.add/remove, and direct
style/attribute-property assignments, so future changes in signal.ts or zap.ts
that write to the DOM are detected by the test.
- Around line 45-46: The STRICT_DOM matcher is too broad because `document` and
`window` are bare substrings, so unrelated identifiers can falsely trigger the
DOM check. Update the regex in `STRICT_DOM` to use word boundaries for
`document` and `window`, matching the existing handling used for `Element`, so
`cell.ts`/`store.ts`/`derived.ts`/`live-cell.ts` identifiers like `documentId`
or `windowSize` won’t be misclassified.
- Around line 37-41: The codeOnly comment stripper is too naive because it
removes // text even when it appears inside string or template literals. Update
codeOnly to use a lexer-aware approach (or a proven parser utility) instead of
the current regex heuristic, so it only strips real comments and preserves code
after in-string // sequences; focus the fix in codeOnly and the DOM sink
scanning path that depends on it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 350568ac-fd20-407b-9104-123c9424f738
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (75)
CHANGELOG.mddocs/adr/0005-effect-boundary.mddocs/adr/0026-dag-compaction.mddocs/adr/0027-cell-value-dom-boundary.mddocs/adr/README.mdpackage.jsonpackages/_spine/core.d.tspackages/_spine/package.jsonpackages/_spine/web.d.tspackages/assets/package.jsonpackages/astro/package.jsonpackages/astro/src/runtime/boundary.tspackages/astro/src/runtime/effect-scheduler.tspackages/astro/src/runtime/gpu.tspackages/astro/src/runtime/graph-runtime.tspackages/astro/src/runtime/llm-receipt-tracker.tspackages/astro/src/runtime/llm-session.tspackages/astro/src/runtime/llm.tspackages/astro/src/runtime/receipt-chain.tspackages/astro/src/runtime/satellite.tspackages/astro/src/runtime/scene-bridge.tspackages/astro/src/runtime/stream.tspackages/astro/src/runtime/svg.tspackages/astro/src/runtime/uniform-signal.tspackages/astro/src/runtime/worker.tspackages/audit/package.jsonpackages/canonical/package.jsonpackages/cli/package.jsonpackages/cli/src/lib/supply-chain-policy.tspackages/cloudflare/package.jsonpackages/command/package.jsonpackages/compiler/package.jsonpackages/core/package.jsonpackages/core/src/dag.tspackages/core/src/index.tspackages/core/src/receipt.tspackages/create-liteship/package.jsonpackages/create-liteship/templates/default/package.jsonpackages/detect/package.jsonpackages/edge/package.jsonpackages/error/package.jsonpackages/gauntlet/package.jsonpackages/genui/package.jsonpackages/genui/src/index.tspackages/genui/src/interaction.tspackages/genui/src/render.tspackages/genui/src/validate.tspackages/liteship/package.jsonpackages/mcp-server/package.jsonpackages/quantizer/package.jsonpackages/remotion/package.jsonpackages/scene/package.jsonpackages/stage/package.jsonpackages/vite/package.jsonpackages/web/package.jsonpackages/web/src/index.tspackages/web/src/stream/sse-pure.tspackages/web/src/stream/sse.tspackages/web/src/types.tspackages/worker/package.jsontests/browser/astro-stream-llm.test.tstests/component/sse-client.test.tstests/fixtures/api-surface-snapshot.jsontests/property/dag-compaction.prop.test.tstests/property/sse-overflow.test.tstests/unit/astro/astro-directives.test.tstests/unit/astro/gpu-canvas-default-size.test.tstests/unit/astro/signal-not-served.test.tstests/unit/astro/stream-llm-lifecycle.test.tstests/unit/create-liteship/scaffold.test.tstests/unit/devops/audit-consumer-mode.test.tstests/unit/devops/effect-version-sync.test.tstests/unit/genui/render.test.tstests/unit/genui/validate.test.tstests/unit/meta/cell-dom-boundary.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bf4207035
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currentSource = machine.source; | ||
| machine.source = null; | ||
| currentSource?.close(); |
There was a problem hiding this comment.
When an old EventSource's queued onerror or heartbeat timeout fires after a reconnect/manual client.reconnect() has already installed a replacement, this path reads machine.source instead of the source that raised the error, so the stale callback closes and nulls the new live connection and schedules another retry. The fresh evidence is that the new internal loss path still closes machine.source without detaching handlers or checking source identity, even though teardown now uses detachSource; capture/pass the source into the loss handler and ignore callbacks that are no longer current.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/astro/astro-directives.test.ts (1)
287-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile reliance on array-position ordering for timer disambiguation.
scheduled.pop()assumes the most-recently-pushed callback is always the reconnect timer rather than a heartbeat-reset timer; the comment at lines 289-291 already acknowledges this risk. SinceclearTimeoutis a no-op stub here, any future change to scheduling order (e.g., heartbeat reset scheduled after the reconnect timer) would silently corrupt this test instead of failing clearly.Consider tagging scheduled callbacks with a purpose/kind when pushed (e.g., wrap as
{ kind: 'reconnect' | 'heartbeat', cb }) so the test can deterministically select the reconnect timer instead of relying on push order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/astro/astro-directives.test.ts` around lines 287 - 293, The test in astro-directives.test.ts is selecting the reconnect timer by relying on the last entry in the scheduled callbacks array, which is fragile if timer ordering changes. Update the test around the MockEventSource/scheduled timer handling to tag each scheduled callback with its purpose (for example, reconnect vs heartbeat) and then explicitly invoke the reconnect callback by kind instead of using pop(). Keep the fix localized to the loop that calls simulateError() and advances the timer queue so the intent stays clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/astro/astro-directives.test.ts`:
- Around line 287-293: The test in astro-directives.test.ts is selecting the
reconnect timer by relying on the last entry in the scheduled callbacks array,
which is fragile if timer ordering changes. Update the test around the
MockEventSource/scheduled timer handling to tag each scheduled callback with its
purpose (for example, reconnect vs heartbeat) and then explicitly invoke the
reconnect callback by kind instead of using pop(). Keep the fix localized to the
loop that calls simulateError() and advances the timer queue so the intent stays
clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63132eb2-d7ca-4fd5-9f36-ba383bb029be
📒 Files selected for processing (2)
tests/browser/astro-stream-llm.test.tstests/unit/astro/astro-directives.test.ts
…rowser e2e The bots re-reviewed d3ddc32 and found real follow-ups on the A5/A3 primitives — plus this push fixes the ACTUAL reason browser-e2e stayed red. Browser e2e (the real blocker): the reconnect test fired `scheduled.shift()` — the OLDEST timer, a leftover heartbeat — so the reconnect timers never fired, the retry budget never exhausted, and the terminal max-reconnect-attempts error never dispatched (errors === []). Fire the freshly-scheduled reconnect with `pop()` (matching the faithful node gate added in astro-directives). Review findings (all real): - A5 checkpoint validation (receipt.ts): an empty chain no longer bypasses checkpoint authorization (fast-path moved after auth); the attestation must carry kind:"checkpoint" (a different genesis-shaped receipt with the same subject id can no longer authorize); a compacted tail must advance the HLC beyond the checkpoint (a stale-HLC envelope naming `base` is rejected). Gated. - A5 anti-fork (dag.ts): the missing-parent fork scan now also matches retained MERGE children (array `previous` containing the watermark), so a merge receipt can't bypass the anti-fork rule after compaction (Codex/Greptile P1). - A3 overflow (sse-pure.ts): coalesce drops the stale patch and re-appends the replacement at the TAIL instead of overwriting in place, so a later patch no longer jumps ahead of an intervening ordered signal/token. Gated. - A3 reconnect (sse.ts): handleConnectionLoss clears any pending reconnect timer first, so duplicate loss signals can't double-open a source. - llm-receipt-tracker: epoch-guard `_compacting` so a stale async compaction completion can't unlock a new generation. - Strengthened the sse-overflow token assertion (identity, not count); corrected a stale lifecycle docstring that contradicted the synchronous directives + the ADR. Verified: tsc + lint + format + check-invariants clean; ~370 tests across the affected dag/receipt/sse/directive suites green, incl. the new gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f9f9576fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * patch. Built from the canonical {@link SEMANTIC_ID_ATTR} constant so the | ||
| * coalesce key tracks the same attribute Morph uses for node identity. | ||
| */ | ||
| const COALESCE_ID_ATTR_PATTERN = new RegExp(`${SEMANTIC_ID_ATTR}="([^"]+)"`); |
There was a problem hiding this comment.
When an incoming patch is valid HTML that uses data-czap-id='hero', this extractor returns null, so the default coalesce-by-id policy treats an addressed/idempotent patch as keyless. Under saturation, a buffer full of these single-quoted patches has no keyed entries to evict or coalesce, so the fallback can drop ordered/keyless messages instead of shedding the addressed patches; match both quote styles (or parse the attribute) before classifying the patch as unkeyed.
Useful? React with 👍 / 👎.
The relative zero-alloc gate flaked on a loaded Blacksmith runner: measureLiveBytesPerOp took a SINGLE before/after heapUsed delta, which a stray background GC or steady-state scratch resize perturbs by several B/op run-to-run. The token-buffer + compositor candidates AND the retaining reference all measure through it, so the noise hit the candidate/reference ratio (1 of 7041 tests; passed locally + on every prior run — a textbook load flake, not a regression). Fix: take the MEDIAN of N short windows instead of one long single-shot window — the SAME anti-fragile method measureTransientBytesPerOp already uses across its 25 windows. Spread across the SAME total batches, so it is cost-neutral. The threshold is unchanged: a REAL per-op survivor is consistent across every rep, so the median still trips it; only transient GC-timing luck is filtered — never a false pass for a true leak. Verified green + cost-neutral locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03ceedd1b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const expectedSubjectId = `czap/checkpoint:${base}`; | ||
| if (checkpoint.subject.id !== expectedSubjectId) { | ||
| return yield* Effect.fail({ | ||
| type: 'checkpoint_invalid' as const, | ||
| reason: `checkpoint subject "${checkpoint.subject.id}" does not commit base watermark (expected "${expectedSubjectId}")`, | ||
| }); |
There was a problem hiding this comment.
When compacted tails are validated from persisted or remote inputs, these checks accept any genesis-shaped kind: "checkpoint" receipt whose subject.id matches the base, regardless of whether it has the checkpoint-summary payload (or even the run subject type) that DAG.checkpoint mints. A caller can create a validly hashed checkpoint envelope for an arbitrary base with an older timestamp and arbitrary payload, and the widened first-link predicate will authorize a truncated tail without evidence that this checkpoint came from the compaction path; reject checkpoints that do not match the minted checkpoint shape/provenance before accepting previous === base.
Useful? React with 👍 / 👎.
Codex + Greptile re-reviewed 5f9f957/03ceedd1 and found 3 real follow-ups on the A3/A5 primitives; this also regenerates docs/api for 0.5.0 (the truth-linux blocker). - Codex P2 (sse-pure): extractCoalesceKey only matched double-quoted data-czap-id, so a valid single-quoted addressed patch was classified keyless — under saturation the fallback could shed ordered messages instead of the idempotent patch. Match both quote styles. Gated. - Greptile P1 (sse.ts): a wakeup token can outlive its message (coalesce supersedes, or a saturating drop evicts, an already-signaled entry), so the async `messages` Stream could shift() undefined. The Stream now emits nothing for a stale token (self-heals; the next ingest re-signals). Only the buffered Stream path is affected — the synchronous directives bypass it via onMessage. - Codex P2 (receipt.ts): the kind:"checkpoint" check wasn't enough — a forged genesis-shaped receipt with the right subject.id but arbitrary payload/older timestamp could authorize a truncated tail. Bind to the MINTED shape: subject.type "run" + the czap/checkpoint-summary payload schema (shared constant so mint + verifier can't drift). Gated (a wrong-subject.type forgery is rejected). docs/api regenerated (typedoc @ 6GB) for the 0.5.0 surface + the line-anchor shifts these fixes caused — resolves truth-linux's docs:check drift. Verified: tsc + lint + format + api-surface + check-invariants clean; 212 tests across the dag/receipt/sse/directive suites green, incl. the 2 new gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ec94c08b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reason: `checkpoint subject.type is "${checkpoint.subject.type}" (expected "run")`, | ||
| }); | ||
| } | ||
| if (checkpoint.payload.schema_hash !== CHECKPOINT_ATTESTATION_SCHEMA) { |
There was a problem hiding this comment.
When validating compacted tails from persisted or remote state, this still accepts any self-hashed kind: "checkpoint" envelope whose subject names the base and whose payload merely advertises the checkpoint schema; the content_hash is never tied to the summary that DAG.checkpoint minted for that watermark. A forged checkpoint with arbitrary payload and a timestamp just below the first retained envelope can therefore authorize a previous === base tail without evidence that the omitted prefix was actually checkpointed. Fresh evidence is that the current fix checks subject.type and schema_hash, but leaves the payload content arbitrary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api/core/src/variables/Receipt.md (1)
387-404: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
checkpoint_invalidin the failure taxonomy.Line 393 still enumerates only the old four
ChainValidationErrorvariants, so this section now contradicts the updated type alias docs and understates whatvalidateChainDetailedcan return.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api/core/src/variables/Receipt.md` around lines 387 - 404, Update the validateChainDetailed documentation in Receipt.md to include checkpoint_invalid in the failure taxonomy. Keep the description aligned with the ChainValidationError type alias and adjust the listed discriminated union variants so validateChainDetailed’s possible failures are fully represented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/stream/sse-pure.ts`:
- Around line 25-32: The coalesce key extraction is using a raw substring regex
that can match data-czap-id inside text or another attribute value, which can
assign a false key. Update extractCoalesceKey to inspect only real start-tag
attributes rather than searching the entire serialized patch string, and keep
the logic anchored to the existing SEMANTIC_ID_ATTR/COALESCE_ID_ATTR_PATTERN
symbols so single-quoted and double-quoted values are still handled correctly.
In `@scripts/alloc-gate.ts`:
- Around line 166-201: The median-sampling loop in alloc-gate.ts is over-running
the requested workload because batchesPerRep uses Math.ceil(batches / REPS), so
the total executed batches can exceed batches and the reported
totalOps/liveDeltaBytes only reflect one rep. Update the sampling logic around
the REPS loop and the liveBytesPerOp calculation so the requested batches are
split exactly across min(REPS, batches) reps, distributing the remainder evenly,
and make totalOps reflect the actual total work executed before returning the
result object.
---
Outside diff comments:
In `@docs/api/core/src/variables/Receipt.md`:
- Around line 387-404: Update the validateChainDetailed documentation in
Receipt.md to include checkpoint_invalid in the failure taxonomy. Keep the
description aligned with the ChainValidationError type alias and adjust the
listed discriminated union variants so validateChainDetailed’s possible failures
are fully represented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69cb6938-e918-46c3-8daf-c7d122611456
📒 Files selected for processing (48)
docs/api/core/src/README.mddocs/api/core/src/interfaces/ChainValidationOptions.mddocs/api/core/src/interfaces/CheckpointResult.mddocs/api/core/src/interfaces/DAGNode.mddocs/api/core/src/interfaces/ForkViolation.mddocs/api/core/src/interfaces/MergeResult.mddocs/api/core/src/interfaces/ReceiptDAG.mddocs/api/core/src/namespaces/DAG/README.mddocs/api/core/src/namespaces/DAG/type-aliases/Checkpoint.mddocs/api/core/src/namespaces/DAG/type-aliases/CompactResult.mddocs/api/core/src/namespaces/DAG/type-aliases/Fork.mddocs/api/core/src/namespaces/DAG/type-aliases/Graph.mddocs/api/core/src/namespaces/DAG/type-aliases/Merge.mddocs/api/core/src/namespaces/DAG/type-aliases/Node.mddocs/api/core/src/namespaces/Receipt/type-aliases/ChainError.mddocs/api/core/src/namespaces/Receipt/type-aliases/Envelope.mddocs/api/core/src/namespaces/Receipt/type-aliases/Subject.mddocs/api/core/src/type-aliases/ChainValidationError.mddocs/api/core/src/variables/DAG.mddocs/api/core/src/variables/Receipt.mddocs/api/genui/src/README.mddocs/api/genui/src/functions/renderFromCatalog.mddocs/api/genui/src/functions/validateGeneratedUITree.mddocs/api/genui/src/interfaces/RenderFromCatalogOptions.mddocs/api/genui/src/type-aliases/RenderFromCatalogResult.mddocs/api/genui/src/type-aliases/ValidateGeneratedUIResult.mddocs/api/web/src/README.mddocs/api/web/src/interfaces/BackpressureHint.mddocs/api/web/src/interfaces/MatchResult.mddocs/api/web/src/interfaces/ReconnectConfig.mddocs/api/web/src/interfaces/ResumptionConfig.mddocs/api/web/src/interfaces/ResumptionState.mddocs/api/web/src/interfaces/SSEClient.mddocs/api/web/src/interfaces/SSEConfig.mddocs/api/web/src/type-aliases/MatchPriority.mddocs/api/web/src/type-aliases/OverflowPolicy.mddocs/api/web/src/type-aliases/ResumeResponse.mddocs/api/web/src/type-aliases/ResumptionStateInput.mddocs/api/web/src/type-aliases/SSEMessage.mddocs/api/web/src/variables/Physical.mddocs/api/web/src/variables/SSE.mdpackages/core/src/dag.tspackages/core/src/receipt.tspackages/web/src/stream/sse-pure.tspackages/web/src/stream/sse.tsscripts/alloc-gate.tstests/property/dag-compaction.prop.test.tstests/property/sse-overflow.test.ts
✅ Files skipped from review due to trivial changes (34)
- docs/api/core/src/namespaces/Receipt/type-aliases/Subject.md
- docs/api/core/src/namespaces/Receipt/type-aliases/Envelope.md
- docs/api/core/src/namespaces/DAG/type-aliases/CompactResult.md
- docs/api/genui/src/type-aliases/ValidateGeneratedUIResult.md
- docs/api/core/src/namespaces/DAG/README.md
- docs/api/core/src/namespaces/DAG/type-aliases/Node.md
- docs/api/core/src/namespaces/DAG/type-aliases/Fork.md
- docs/api/core/src/namespaces/DAG/type-aliases/Graph.md
- docs/api/web/src/type-aliases/ResumeResponse.md
- docs/api/core/src/namespaces/DAG/type-aliases/Merge.md
- docs/api/web/src/type-aliases/SSEMessage.md
- docs/api/web/src/README.md
- docs/api/core/src/namespaces/Receipt/type-aliases/ChainError.md
- docs/api/core/src/namespaces/DAG/type-aliases/Checkpoint.md
- docs/api/genui/src/functions/validateGeneratedUITree.md
- docs/api/web/src/type-aliases/MatchPriority.md
- docs/api/web/src/type-aliases/ResumptionStateInput.md
- docs/api/genui/src/type-aliases/RenderFromCatalogResult.md
- docs/api/genui/src/README.md
- docs/api/web/src/type-aliases/OverflowPolicy.md
- docs/api/web/src/variables/Physical.md
- docs/api/core/src/README.md
- docs/api/core/src/interfaces/DAGNode.md
- docs/api/core/src/interfaces/CheckpointResult.md
- docs/api/web/src/interfaces/ReconnectConfig.md
- docs/api/core/src/interfaces/ReceiptDAG.md
- docs/api/web/src/interfaces/ResumptionConfig.md
- docs/api/genui/src/interfaces/RenderFromCatalogOptions.md
- docs/api/web/src/interfaces/MatchResult.md
- docs/api/core/src/interfaces/ForkViolation.md
- docs/api/core/src/interfaces/MergeResult.md
- docs/api/web/src/variables/SSE.md
- docs/api/core/src/interfaces/ChainValidationOptions.md
- docs/api/web/src/interfaces/ResumptionState.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/property/sse-overflow.test.ts
- packages/core/src/receipt.ts
- tests/property/dag-compaction.prop.test.ts
- packages/core/src/dag.ts
- packages/web/src/stream/sse.ts
truth-linux ran the FULL gauntlet (phases 1-28 green) and failed only at phase 29 (coverage:merge): packages/astro/src/runtime/effect-scheduler.ts sat at 0% line coverage. The synchronous-directive rewrite removed both its usages (stream.ts + llm.ts), so it is dead code — deleted (not waived). ADR-0005's evidence line + docs/api no longer reference it. Latest bot review (4 findings): - Greptile P1 (sse-pure): a saturated all-ordered buffer no longer sheds an ordered message for an incoming keyed patch — the idempotent patch is rejected instead (the selective policy sheds keyed first). Gated. - CodeRabbit (alloc-gate): median-of-N now distributes the caller's batches EXACTLY across min(REPS, batches) reps and accumulates the real totalOps, so the work + report match the request (no ceil()-driven overshoot). - CodeRabbit (sse-pure): extractCoalesceKey parses a real start-tag attribute (tokenizer) instead of a raw substring, so `data-czap-id` inside text or another attribute's value can't forge a coalesce key. Gated. - Codex P2 (receipt checkpoint provenance): a genuine architectural limit — validateChain lacks the dropped set, so it can't tie the summary content_hash to the mint; full adversarial provenance needs a SIGNED checkpoint. Documented in ADR-0026 (structural vs cryptographic; signature = future work) rather than fake-closed. Verified: tsc + lint + format + check-invariants clean; 107 sse/directive tests + the alloc property tests green, incl. the 2 new gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12b6477bb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| saveResumptionState(artifactId, currentEventId); | ||
| if (recoveryPending && artifactId) { | ||
| recoveryPending = false; | ||
| void reconcileResumption(currentEventId); |
There was a problem hiding this comment.
When recoveryPending is true after a reconnect, this writes the newly received event id into sessionStorage before calling reconcileResumption(currentEventId). Resumption.resume loads that saved state to compute the previous sequence, so it now sees the current event as already persisted and returns an empty replay (gap <= 0), skipping any patches missed between the pre-disconnect cursor and the first post-reconnect event. Keep the previous cursor/state until after resume completes, or pass the pre-disconnect cursor into the resume path.
Useful? React with 👍 / 👎.
…venance seam Fresh adversarial round on 12b6477 (Codex + Greptile T-Rex + CodeRabbit). Verified each against HEAD; several were already fixed on earlier commits (DAG merge-child array-match, coalesce FIFO/tokenizer/saturation, alloc-gate distribution, the HLC checkpoint-advance check). Three were genuinely open: 1. SSE stale-source callbacks (Greptile P1, T-Rex repro; Codex P2 ×2). A frame or error queued on an EventSource that was replaced (reconnect / manual reconnect) or closed (teardown) still fired against the CURRENT machine.source — tearing down the healthy replacement and scheduling a spurious reconnect, or morphing a stale frame. Fix: per-source IDENTITY guard on onmessage/onerror (ignore when machine.source !== source) + detach the dying source's handlers in handleConnectionLoss. Gate: sse-client "a stale error from a replaced source does not tear down the live replacement" (proven red without the guard). 2. Resumption gap collapse (Codex P1). On the first post-reconnect frame the directive persisted the new cursor BEFORE reconciling — and Resumption.resume loads the persisted cursor (loadState is Effect.sync) to size the replay gap, so persist-first set lastSequence to the current seq, collapsed the gap to <= 0, and silently dropped every patch missed while disconnected. Fix: reconcile BEFORE persist. Gate: stream-llm-lifecycle "reconciles the gap against the PRE-disconnect cursor" (proven red under persist-first). Aligned two astro-directives resume tests to fire the reconnect timer (pop, like their siblings) instead of the boot heartbeat, since the guard now correctly ignores a frame from the dead generation. 3. Checkpoint provenance (Greptile P1 T-Rex + Codex P2 ×2) — owner chose the verifier seam. validateChain's structural checks prove a checkpoint is well-formed but not that it attests to the real dropped set (it lacks that set), so a forged genesis-shaped kind:"checkpoint" can authorize a truncated tail. Added an injected ChainValidationOptions.verifyCheckpoint capability (engine owns the seam, host owns the key): absent, the structural floor stands (sound for trusted self-compaction); present, an adversarial caller closes the forgery (e.g. a macEnvelope signature check). Mirrored in _spine; documented in ADR-0026. Gate: dag-compaction "verifyCheckpoint seam rejects a forged checkpoint the structural floor accepts" (self-differential — proves both the residual limit and the seam). docs/api regenerated (SSE + ChainValidationOptions + Receipt anchor shifts). api-surface unaffected (optional field). Verified: tsc + lint + format + check-invariants clean; 91 tests across the 6 affected files green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 495dbe2c32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!chain || _lastAckReceiptId === null) return; | ||
|
|
||
| const hashes = chain.linearizedHashes(); | ||
| const ackIndex = hashes.indexOf(_lastAckReceiptId); |
There was a problem hiding this comment.
In normal LLM rendering, _lastAckReceiptId is the UIFrame.receiptId minted by GenFrame from frame content, while chain.linearizedHashes() returns the SHA-256 hashes of ingested receipt envelopes. For long sessions that record frames and ingest receipt envelopes, this indexOf therefore stays -1, so the margin/threshold check always returns and the new auto-compaction path never reclaims the DAG or frame history. Keep a mapping from acknowledged frames to their envelope hash, or choose the watermark from receipt-chain state rather than comparing across the two id spaces.
Useful? React with 👍 / 👎.
| * false coalesce key. Both quote styles are valid HTML and are handled. | ||
| */ | ||
| const firstStartTagAttr = (html: string, attr: string): string | null => { | ||
| const tag = /<[a-zA-Z][^>]*>/.exec(html)?.[0]; |
There was a problem hiding this comment.
When a valid patch's first tag has a quoted attribute containing > before data-czap-id (for example a title/ARIA value), this regex truncates the tag at that character and the extractor returns null. Under the default coalesce-by-id policy, that addressed patch is then treated as keyless, so a saturated buffer cannot coalesce or evict it as an idempotent patch and may shed ordered/keyless messages instead. Use a quote-aware scan or a real HTML parser for the first start tag.
Useful? React with 👍 / 👎.
… tests The SSE stale-source identity guard (495dbe2) exposed a latent flake in astro-directive-branches.test.ts: 7 resumption tests fire the reconnect via advanceTimersByTimeAsync(1000), but the backoff is 750 + 500*Math.random() ms (jitter, unmocked), so the 1000ms advance only fired the reconnect ~half the time. Previously the stale pre-reconnect EventSource stayed live, so messaging it "worked" regardless; now SSE.create correctly ignores a frame from the dead generation, so a not-yet-fired reconnect left the recovery frame on a dead source and resume() was never called. Fix: mock Math.random -> 0.5 (zero jitter -> deterministic 1000ms backoff) in each of the 7 tests, the idiom sse-client already uses. These tests were latently ~50% flaky; the guard just surfaced it. Verified with a full SSE/stream/reconnect/resumption/worker sweep (tests/component + tests/unit/web + tests/unit/astro + SSE/DAG property tests): 70 files, 1140 tests, all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
| if (recoveryPending && artifactId) { | ||
| recoveryPending = false; | ||
| void reconcileResumption(currentEventId); | ||
| } | ||
| saveResumptionState(artifactId, currentEventId); |
There was a problem hiding this comment.
reconcileResumption(currentEventId) starts without gating the current frame, then the same message continues to enqueueHtml on lines 364-366. When the first post-reconnect patch arrives, it can render before the replay or snapshot for the missed gap, so the DOM sees sequence N before N-k..N-1 and then applies older recovery content afterward. Gate current message processing until recovery reconciliation finishes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/astro/src/runtime/stream.ts
Line: 343-347
Comment:
**Preserve replay ordering**
`reconcileResumption(currentEventId)` starts without gating the current frame, then the same message continues to `enqueueHtml` on lines 364-366. When the first post-reconnect patch arrives, it can render before the replay or snapshot for the missed gap, so the DOM sees sequence `N` before `N-k..N-1` and then applies older recovery content afterward. Gate current message processing until recovery reconciliation finishes.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4e40b275
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Unbounded edge queue: every status transition is offered here and | ||
| // surfaced as `client.stateChanges`. Unbounded so a transition is never | ||
| // itself dropped under backpressure (edges are control-plane, not data). | ||
| const transitionQueue = yield* Queue.unbounded<SSEState>(); |
There was a problem hiding this comment.
When a client uses only client.state or the synchronous onStateChange callback, client.stateChanges is never drained, but every reconnect/connected/error edge is still enqueued into this unbounded queue. A flaky long-lived EventSource will accumulate one entry per status transition indefinitely, turning network churn into a heap leak; use a bounded/sliding queue or avoid allocating/offering to the queue unless the stream is actually consumed.
Useful? React with 👍 / 👎.
…sion One batch addressing the reinit-cursor regression the browser e2e caught plus all four inline review findings (Codex/Greptile), verified locally so CI runs once. - Reinit cursor (browser e2e): SSE.create tracks lastEventId per-connection, so a czap:reinit opened a fresh connection that restarted the tail from the top. The directive now carries `lastCursor` across connections and re-seeds the replacement, resuming where the swapped-out connection left off. Node-tested. - Codex P1 (dag.ts) — anti-fork rule must survive compaction: dropping watermark W with retained children left a later `previous === W` fork undetectable (checkForkRule bailed once W's node was gone). It now falls back to scanning the retained nodes that still name a missing parent, so the fork is still rejected — with NO extra DAG state, preserving spliced==fresh-reload. Pinned by test (H). - Codex P2 (sse.ts) — stale message after teardown invoked the synchronous onMessage sink: new `detachSource()` clears the EventSource handlers BEFORE close on the intentional teardown/reinit paths (NOT the internal reconnect), so a queued frame can't morph into a newer generation. - Greptile P1 (stream.ts) — old source live during the reinit window: closeClient now closes the Scope SYNCHRONOUSLY (its finalizer runs detachSource before the replacement opens), so the old generation can't deliver a stale frame. - Codex P2 (llm.ts) — LLM runtime leak on transport error: already resolved by the synchronous rewrite (3196059) that removed the runtime/scope entirely. The broad `machine.source !== source` guard was tried and rejected: it also blocked the reconnect-gap messages ~6 resumption tests legitimately depend on. The targeted handler-clear is correct without that collateral. Verified: 1418-test astro/component/meta sweep + dag/sse suites green; lint/format/api-surface/ check-invariants clean. ADR-0026 updated for the fork-survives-compaction property. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
…rowser e2e The bots re-reviewed d3ddc32 and found real follow-ups on the A5/A3 primitives — plus this push fixes the ACTUAL reason browser-e2e stayed red. Browser e2e (the real blocker): the reconnect test fired `scheduled.shift()` — the OLDEST timer, a leftover heartbeat — so the reconnect timers never fired, the retry budget never exhausted, and the terminal max-reconnect-attempts error never dispatched (errors === []). Fire the freshly-scheduled reconnect with `pop()` (matching the faithful node gate added in astro-directives). Review findings (all real): - A5 checkpoint validation (receipt.ts): an empty chain no longer bypasses checkpoint authorization (fast-path moved after auth); the attestation must carry kind:"checkpoint" (a different genesis-shaped receipt with the same subject id can no longer authorize); a compacted tail must advance the HLC beyond the checkpoint (a stale-HLC envelope naming `base` is rejected). Gated. - A5 anti-fork (dag.ts): the missing-parent fork scan now also matches retained MERGE children (array `previous` containing the watermark), so a merge receipt can't bypass the anti-fork rule after compaction (Codex/Greptile P1). - A3 overflow (sse-pure.ts): coalesce drops the stale patch and re-appends the replacement at the TAIL instead of overwriting in place, so a later patch no longer jumps ahead of an intervening ordered signal/token. Gated. - A3 reconnect (sse.ts): handleConnectionLoss clears any pending reconnect timer first, so duplicate loss signals can't double-open a source. - llm-receipt-tracker: epoch-guard `_compacting` so a stale async compaction completion can't unlock a new generation. - Strengthened the sse-overflow token assertion (identity, not count); corrected a stale lifecycle docstring that contradicted the synchronous directives + the ADR. Verified: tsc + lint + format + check-invariants clean; ~370 tests across the affected dag/receipt/sse/directive suites green, incl. the new gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KxU3Y8XueHqfteVGA4KdEh
1 participant
LiteShip 0.5.0 — "make-it-loud"
Every fix turns a silent degradation into a loud diagnostic through the existing
Diagnostics.warnOnce/validation channel, plus a packaging hardening and one feature (DAG compaction). Surfaced by dogfooding the framework on real apps; hardened by an adversarial review pass.What's in it
🔊 Make-it-loud
client:gpuwarns (canvas-default-size) when its host has no layout at boot (was a silent 300×150 tiny render).signal-input-unserved-here) when a boundary input is recognized but has no live producer on that surface (was a silent freeze); theuniform-signal"typo" mislabel is split into two disjoint codes.@czap/genuirejects unsupported handler props (on*≠onClick, non-stringonClick) at validation instead of silently dropping them at render.renderFromCatalogreturns a taggedRenderFromCatalogResult— use.ok.OverflowPolicy(defaultcoalesce-by-id, selective so keyless/ordered messages keep FIFO) + enrichedBackpressureHint+stateChanges, and a heartbeat-watchdog reconnect fix (no longer silently drop-newests).🏗️ Feature — A5 drop-only receipt-DAG compaction:
DAG.checkpoint/spliceCheckpointreclaim a long-lived DAG's dominated prefix into a content-addressed checkpoint attestation (the spliced DAG equals a fresh reload).validateChaingains a checkpoint-authorized horizon. See ADR-0026.🔌 Directive wiring — A3b
client:stream/client:llmnow consume the hardenedSSE.createvia an owned disposableManagedRuntimewith deterministic teardown (dispose / VT-swap single-boot).client:llmkeeps its raw EventSource + own decoder to preserve the non-bypassableparseMessagesecurity preflight (no public decode bypass).📦 Packaging —
effectpeer capped>=4.0.0-beta.32 <5across all@czap/*(was unbounded), pinned bypnpm.overrides.effect+ a drift guard.create-liteshipnow scaffoldseffect.📐 Guard — A6 Cell↔DOM boundary committed as a guarded law (ADR-0027). Audit consumer-mode disallowed-edge coverage (#4).
Adversarial review
Designs went through multi-wave adversarial verification (caught 3 ship-blockers before implementation). The implementation was then reviewed by an external oracle (Codex), which confirmed + closed 5 findings: the directive teardown leak (→ the disposable-runtime fix), an LLM terminal-path leak, a
validateChainsoundness hole (abasewatermark alone could validate a truncated chain — now requires a verified checkpoint), and acompactBelowconcurrent-ingest race. Two Effect-beta bugs were fixed in passing (Scope.usefinalizing early; an overflow-buffer CPU spin).Docs
ADR-0026 (DAG compaction), ADR-0027 (Cell↔DOM boundary), ADR-0005 SSE addendum, CHANGELOG
[0.5.0].Gates
docs:build/typedoc-clean (typedoc needs ~8 GB), and thedocs/apiregeneration. The three typedocnotExportedcauses (new exports missing from package barrels) were found + fixed statically; if CI surfaces another it's a one-line barrel re-export.🤖 Generated with Claude Code
Summary by CodeRabbit
stateChanges, and improved cursor-based resumption.client:gpuhosts.client:stream/client:llmteardown, reconnection/heartbeat behavior, and prevented silent SSE message loss; GenUI now rejects unsupported interaction props during validation.renderFromCatalognow returns a tagged{ ok }result (use.ok).effectpeer compatibility is tightened to a newer beta range.Greptile Summary
This PR hardens LiteShip diagnostics and streaming behavior while adding DAG compaction support. The main changes are:
client:gpuhosts and unserved signal inputs.renderFromCatalogresult.effectpeer ranges and scaffold/package drift guards.Confidence Score: 3/5
The stream resumption path needs attention before merging because reconnect recovery can apply updates out of order.
The changed areas are broad and include runtime streaming, lifecycle, validation, and DAG logic; the review identified one concrete ordering issue in the reconnect path while the rest of the changes appear internally guarded by focused tests and diagnostics.
packages/astro/src/runtime/stream.ts
What T-Rex did
Comments Outside Diff (5)
General comment
pnpm.overrides.effectpins the resolved Effect version, but running pnpm 10.32.1 on head prints that thepnpmfield inpackage.jsonis no longer read and thatpnpm.overrideswas ignored.pnpm config get overridesreturnsundefined, so the package manager does not enforce the declaredeffect: 4.0.0-beta.32override even thoughpackage.jsoncontains it.pnpmobject inpackage.jsoninstead of a pnpm-10-recognized settings location such aspnpm-workspace.yaml/.npmrcaccording to pnpm's warning.effect: 4.0.0-beta.32, into the pnpm-10-supported workspace settings location and update the drift guard to validate pnpm's effective configuration rather than only readingpackage.json#pnpm.overrides. Re-run the lockfile generation afterward.General comment
packages/web/src/stream/sse.ts, line 279-296 (link)handleConnectionLoss()closes the oldEventSourcebut leaves its handlers attached, and these closures do not check thatsourceis stillmachine.source. If a queued frame or error from the closed source fires aftersetupSource()installs a replacement, it can updatelastEventId/deliver stale patches or close the new live source throughhandleConnectionLoss(). Guard both callbacks against stale sources before mutating state.Prompt To Fix With AI
General comment
SSE.createscenario against base and the checked-out head files produced identical output. On head,client.backpressurestill returned only{bufferSize,maxBufferSize,percentFull,dropping}instead of enriched policy/droppedCount/coalescedCount fields,stateChanges-streamandonStateChange-callbacksremained empty, and the heartbeat watchdog scenario still ended atheartbeat-state: "error"witheventSourceInstances: 1rather than reconnecting. The overflow delivery also still showed bothapatch versions in the stream boundary sample rather than demonstrating selective coalescing counters.packages/web/src/index.ts.packages/web/src/stream/sse.tsactually implements and exports the head contract through the publicSSE.createpath: use the coalesce-by-id overflow implementation for buffered messages, populateBackpressureHint.policy,droppedCount, andcoalescedCount, emit state transition edges and invokeonStateChange, and route heartbeat timeout through reconnect scheduling instead of latchingerror. Add a targeted runtime test equivalent to the captured script so base/head behavior cannot remain identical.General comment
Prompt To Fix All With AI
Reviews (8): Last reviewed commit: "test(astro): deterministic reconnect bac..." | Re-trigger Greptile