fix(#404): re-key inert pty-dedup on (generation, offset) + content-hash - #408
Conversation
khaliqgant
added
the
no-agent-relay-review
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughPTY chunk deduplication now keys replay detection by generation, offset, and content hash. Missing offsets remain deliverable but emit rate-limited warnings. Broker wiring, documentation, telemetry, lifecycle behavior, and tests were updated, alongside an additional ChangesPTY replay deduplication
Ignore rule update
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BrokerManager
participant PtyChunkDeduper
participant PTYStream
BrokerManager->>PtyChunkDeduper: Check worker_stream chunk with generation
PtyChunkDeduper->>PtyChunkDeduper: Match generation:offset and content hash
PtyChunkDeduper-->>BrokerManager: Return duplicate decision
BrokerManager->>PTYStream: Deliver accepted chunk
Suggested reviewers: Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
worker_stream PTY chunks never carry seq/event_id — those are ephemeral and
excluded from the daemon replay buffer, so the previous seq/event_id identity
could never match: the layer was structurally inert (it logged "no seq/event_id"
once per stream and suppressed nothing). worker_stream chunks DO carry a
cumulative per-worker byte `offset` (present 5/5 on PTY streams), so re-key
identity to `${generation}:${offset}` where generation is the broker
event-stream generation the delivering listener attached under. This matches
the renderer-side guardrail (pty-buffer-store.ts) exactly, which already keyed
on (generation, offset)+hash.
Invariants preserved (never drop a real byte):
- suppress ONLY on full identity (generation + offset) AND content-hash match
- never drop on content alone (identical chunks are normal terminal traffic)
- never drop on identity alone (repeated offset + different bytes = fresh output)
- generation scopes the offset so a fresh stream's low offset can't collide
with a stale remembered one → no cross-generation false suppression
- offset absent → DELIVER + rate-limited loud console.warn + running counter
(no more silently claiming a protection that isn't running)
Updates AGENTS.md "Duplicate Event Hardening" to describe the real mechanism.
Rewrites pty-dedup.test.ts (suppress/deliver/loud paths) and the broker.test.ts
e2e PTY suite from seq-based to offset-based semantics.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d3878e9 to
b38f8a8
Compare
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: b38f8a8bd9
ℹ️ 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 offset = | ||
| typeof offsetRaw === 'number' && Number.isFinite(offsetRaw) ? offsetRaw : undefined |
There was a problem hiding this comment.
When a legacy or malformed broker uses a finite sentinel such as offset: -1, this treats it as valid correlation metadata. Two legitimate byte-identical chunks carrying that sentinel then share the same generation/offset/hash and the second is dropped, even though the offset cannot establish replay identity; this is especially harmful for repeated terminal echoes or repaint frames. The renderer already treats negative offsets as absent in pty-buffer-store.ts; apply equivalent validation here and deliver/warn when the offset is not a valid nonnegative byte position.
Useful? React with 👍 / 👎.
1 participant
Closes #404.
Problem
Relay
worker_streamPTY chunks never carryseq/event_id— those are ephemeral and excluded from the daemon replay buffer (relaylisten_api.rs~2800).src/main/pty-dedup.tskeyed identity exclusively on them, so the identity could never match: the layer was structurally inert. It logged"… carry no seq/event_id — replay dedup is blind"once per stream and suppressed nothing. AGENTS.md described a protection that did not exist.Fix
Re-key identity to the metadata worker_stream chunks do carry:
offset— cumulative per-worker byte offset, present 5/5 on PTY streams (absent only on headless workers / pre-offset brokers).generation— the broker event-stream generation the delivering listener attached under (BrokerManager.eventStreamGeneration), now passed intoisDuplicatePtyChunk. It scopes the offset so a fresh worker stream's low offset can't collide with a stale remembered one.Identity =
`${generation}:${offset}`. This is exactly what the renderer-side guardrail (pty-buffer-store.ts) already keys on — main was the inert half; the two layers are now consistent.Invariants (CRITICAL — never drop a real byte; when in doubt, DELIVER)
console.warn(once/60s per stream) carrying a running counter — no more silently claiming a protection that isn't running.Review note (worth a look)
worker_streamis excluded from the replay buffer, so PTY chunks are never seq-replayed by the rebind path; a rebind also bumpseventStreamGeneration. Including generation in the key is therefore the safe choice: it makes cross-generation offset-collision false-suppression impossible, at the cost of not suppressing across a generation boundary — which is correct, because a genuine daemon double-emit within one generation still matches, and anything ambiguous is delivered. Net: the layer is now functional and honest rather than inert, and it can never drop a real byte.Verification
npx vitest run src/main/pty-dedup.test.ts→ 13 passed (suppress / deliver / loud paths, incl. deterministic-clock rate-limit test).npx vitest run src/main/broker.test.ts→ 104 passed (e2e PTY suite rewritten seq→offset).npx tsc --noEmit -p tsconfig.node.json→ clean.🤖 Generated with Claude Code