fix(agent-runtime,thread-view): stop timeline 500s from session-scoped ACP fs-write ids - #1224
Conversation
agustif
left a comment
There was a problem hiding this comment.
Note: earlier empty/test APPROVE was accidental from a scout dry-run of the gh CLI APIs — please ignore. Not a real review of this PR.
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, tests, and architecture. I will post the combined findings after the parallel checks finish. |
| // restart their synthetic id counters). Merge only rows from a compatible | ||
| // scope and leave foreign-scope rows untouched, so each scope keeps its own | ||
| // file-edit message instead of failing the whole projection. | ||
| const compatibleRows: EventProjectionFileEditMessage[] = []; |
There was a problem hiding this comment.
🚨 slopcop/review — High: scope whole-item windowing by turn
This fallback handles reused IDs only after the server selects a timeline window. The server still groups item spans and lifecycle rows by raw itemId. A legacy ID reused across turns becomes one thread-wide item.
With 39 events and a budget of 10, the latest page backfilled both turns. The next older page returned neither file row. Larger histories can exceed the 1,500-event budget and restore the expensive projection path.
Please use (scopeKind, turnId, itemId) through whole-item closure and its database queries. Add a server test with the reused ID on opposite sides of the event cut.
| id: `acp-fs-write-${state.fsWriteCounter}`, | ||
| // Include the turn id: resumed sessions restart the counter, so a | ||
| // bare counter would reuse ids already persisted in earlier turns. | ||
| id: `acp-fs-write-${turnId}-${state.fsWriteCounter}`, |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: update the daemon protocol for this wire change
This changes an event item ID that the host daemon sends to the server. HOST_DAEMON_PROTOCOL_VERSION remains 86, so enrolled daemons will not update. They will continue to emit the legacy IDs that this PR tries to stop.
Please increment the protocol version and update its contract expectation.
| } | ||
| const foreignMessageIds = new Set(foreignRows.map((row) => row.id)); | ||
| const stdoutBuffer = | ||
| state.fileEditStdoutBuffersByCallId.get(partial.callId) ?? |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: scope the file-edit output buffer too
The new row logic separates reused call IDs by turn, but this buffer still uses only callId. Two pending file-edit output streams with the same ID therefore share text across turns. The flush path then writes that combined text into every row for the ID.
A focused reproduction made both rows contain one-outputtwo-output. Please key buffers by the scoped call identity. Add an output-delta regression test that keeps each turn’s output separate.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: The app gave two file changes the same name. This patch separates them. Some shelves still use the old name, so files can mix or disappear.
I found three actionable problems.
- High: whole-item timeline closure ignores the turn scope. Legacy IDs can exceed the event budget and make older rows disappear.
- Medium: the host-daemon wire behavior changed without a protocol update. Enrolled daemons will continue to emit legacy IDs.
- Medium: file-edit output buffers still use only the call ID. Reused IDs can combine output from different turns.
The security review found no issue. The architecture review recommends one scoped item identity for closure queries, projection maps, and output buffers.
The focused suite passed all 123 tests. Both affected package type checks passed. The complete thread-view suite passed all 349 tests.
The agent-runtime suite passed 838 of 839 tests. The unrelated stderr-tail test failed twice, while GitHub package checks passed.
A browser test was not practical for this historical ACP session path. An in-memory server test used the real database and timeline service.
The GPT-5.6 review gate confirmed all three findings. I posted this review as comment-only, as required.
…ndows Address the three SlopCop findings on get-bb#1224. Whole-item window closure keyed items by raw item_id, so a reused id looked like one item spanning every turn between its two uses. The newest page then backfilled the oldest turn's lifecycle rows, and every older page disowned the item, so the earlier file changes vanished. The closure and its three queries now key on (scope_kind, turn_id, item_id). The file-edit stdout buffer was also keyed by call id alone, so two pending output streams sharing a reused id merged their text into every row for that id. Buffers are now keyed by scoped call identity, and the flush path resolves each row's buffer from that row's own scope. Bump HOST_DAEMON_PROTOCOL_VERSION to 87: the ACP adapter now sends turn-qualified fileChange item ids, and an enrolled daemon on an older build keeps emitting the colliding session-scoped counters. Tests: a server timeline test with one file-change item id reused across turns on both sides of an event-budget cut, and a thread-view test that keeps each turn's file-change output separate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ndows Address the three SlopCop findings on get-bb#1224. Whole-item window closure keyed items by raw item_id, so a reused id looked like one item spanning every turn between its two uses. The newest page then backfilled the oldest turn's lifecycle rows, and every older page disowned the item, so the earlier file changes vanished. The closure and its three queries now key on (scope_kind, turn_id, item_id). The file-edit stdout buffer was also keyed by call id alone, so two pending output streams sharing a reused id merged their text into every row for that id. Buffers are now keyed by scoped call identity, and the flush path resolves each row's buffer from that row's own scope. Bump HOST_DAEMON_PROTOCOL_VERSION to 88: the ACP adapter now sends turn-qualified fileChange item ids, and an enrolled daemon on an older build keeps emitting the colliding session-scoped counters. Version 87 already shipped on main for the moved-thread session handoff, so this needs its own bump to force those daemons to update. Tests: a server timeline test with one file-change item id reused across turns on both sides of an event-budget cut, and a thread-view test that keeps each turn's file-change output separate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5e1dcd1 to
b96fbdf
Compare
…d ACP fs-write ids The ACP adapter minted fileChange item ids from a per-session counter (acp-fs-write-N). Resumed ACP sessions (e.g. acp-kimi after a restart) restart the counter, so a later turn reuses an item id already persisted in an earlier turn. The timeline projection merges file-edit rows by call id and threw 'Cannot merge file-edit messages with different scopes', failing the whole timeline request and taking down the thread's chat with a 500. Two layers: - Adapter: mint acp-fs-write-<turnId>-<counter>. Turn ids carry a per-adapter-instance random prefix, so ids are unique across sessions. - Projection: upsertFileEdit partitions existing rows by scope, merges only compatible-scope rows, preserves foreign-scope rows as their own messages, and scope-qualifies message keys on collision. Any reused call id (including rows already persisted by older builds) now degrades to separate file cards instead of failing the projection. Tests: cross-session fs-write id uniqueness in the ACP adapter, and a timeline regression test covering two turns that reuse the same fileChange item id.
…ndows Address the three SlopCop findings on get-bb#1224. Whole-item window closure keyed items by raw item_id, so a reused id looked like one item spanning every turn between its two uses. The newest page then backfilled the oldest turn's lifecycle rows, and every older page disowned the item, so the earlier file changes vanished. The closure and its three queries now key on (scope_kind, turn_id, item_id). The file-edit stdout buffer was also keyed by call id alone, so two pending output streams sharing a reused id merged their text into every row for that id. Buffers are now keyed by scoped call identity, and the flush path resolves each row's buffer from that row's own scope. Bump HOST_DAEMON_PROTOCOL_VERSION to 89: the ACP adapter now sends turn-qualified fileChange item ids, and an enrolled daemon on an older build keeps emitting the colliding session-scoped counters. Tests: a server timeline test with one file-change item id reused across turns on both sides of an event-budget cut, and a thread-view test that keeps each turn's file-change output separate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b96fbdf to
087df27
Compare
|
Sorry about that @vburojevic I just asked the review bot to fix and test my b! |
Problem
Threads on ACP providers (reproduced with acp-kimi) intermittently fail
GET /api/v1/threads/:id/timelinewith a 500, taking down the whole chat view (Cannot merge file-edit messages with different scopes).Root cause: the ACP adapter minted
fileChangeitem ids from a per-session counter (acp-fs-write-N,packages/agent-runtime/src/acp/adapter.ts). Resumed ACP sessions restart the counter, so a later turn reuses an item id already persisted in an earlier turn. The timeline projection merges file-edit rows by call id (upsertFileEditinpackages/thread-view/src/operation-projection.ts) and threw on the cross-turn scope mismatch, 500ing every timeline request for the thread from then on. Every resumed session that wrote files created a fresh collision, so affected threads kept re-breaking.Fix (two layers)
acp-fs-write-<turnId>-<counter>. Turn ids carry a per-adapter-instance random prefix, so ids are unique across sessions for good.upsertFileEditnow partitions existing rows by scope — it merges only compatible-scope rows, preserves foreign-scope rows as their own messages, and scope-qualifies message keys on collision. Any reused call id (including rows already persisted by older builds) degrades to separate file cards instead of failing the projection. The strict throw inupdateFileEditMessagestays as an invariant guard, now unreachable fromupsertFileEdit.Tests
acp/adapter.test.ts: two adapter instances (simulating a resumed session) mint distinct fs-write ids.build-thread-timeline.test.ts: two turns reusing the samefileChangeitem id produce two distinct file-change rows, each keeping its own diff.Verified: full
@bb/agent-runtimeand@bb/thread-viewvitest suites green,tsc --noEmitclean in both packages. Also repaired the affected production threads in a livebb.dbwith this recipe (kept earliest turn, suffixed later-turn duplicates initem_idanddata->'$.item.id'); all previously-500ing threads load again.Release note
After merge, cut 0.36.1 per the usual flow:
node scripts/bump-version.mjs --patch→ "Prepare bb-app 0.36.1" PR → dispatchpublish-bb-app.ymlwithnpm_tag=latest,dry_run=false.