Skip to content

Fix wrong session token total after a tab reload #5845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jurgenwerk wants to merge 2 commits into
base: main
Choose a base branch
from
+86 −1
Open

Fix wrong session token total after a tab reload #5845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b004e7
Decode the aggregation bundle's wire data so rebuilt messages keep us…
jurgenwerk Aug 21, 2026
98c3279
Drop the duplicated eventId fallback in addEvent
jurgenwerk Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/host/app/lib/matrix-classes/room.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,22 @@ export default class Room {
event.content.data = JSON.parse(event.content.data);
}
}
eventId = eventId ?? stateKey; // room state may not necessary have an event ID
// Backfilled events carry their latest edit as a server-side aggregation
// bundle, and getAggregatedReplacement substitutes that bundle's content
// for the event's own — so its data needs the same decoding. Without
// this, every rebuilt message whose usage/context arrived on an edit
// reads `data` as a wire string and silently loses those fields (the
// session token total visibly shrank after a reload because pre-reload
// turns' usage vanished this way).
Comment on lines +142 to +148

@habdelra habdelra Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Two rewording asks on this comment — non-blocking, but both cost a future reader real time.

The parenthetical narrates the bug in past tense. "the session token total visibly shrank after a reload because pre-reload turns' usage vanished this way" describes the behaviour before this commit. A reader two years out has no anchor for when "shrank" was true and will read it as a live symptom, then go hunting for a bug that isn't there. evergreen-comments asks for the contract stated timelessly; the mechanism in the first three lines already carries the value, so the parenthetical only needs to name what depends on the decode.

getAggregatedReplacement names two different functions in this repo — packages/host/app/resources/room.ts (the one this comment means) and packages/runtime-common/ai/history.ts (which orders the decode the other way round, per the comment below). The reader has to open one of them to see why the ordering matters here, so it's worth saying which.

Something like:

    // Backfilled events carry their latest edit as a server-side aggregation
    // bundle, and getAggregatedReplacement in app/resources/room.ts
    // substitutes that bundle's content for the event's own — so its data
    // needs the same decoding. Everything read off `data` on rebuild depends
    // on it: token usage, and context.agentId, which gates tool auto-run.

Scope: prose only, no behaviour change.

Generated by Claude Code

let bundledReplace = (event.unsigned as any)?.['m.relations']?.[
'm.replace'
];
if (
bundledReplace?.content?.data &&
typeof bundledReplace.content.data === 'string'
) {
bundledReplace.content.data = JSON.parse(bundledReplace.content.data);
}
Comment on lines +149 to +157

@habdelra habdelra Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The decode is correct, but it establishes an invariant — "the event MessageBuilder sees always has an object data" — that holds only because two files, in two packages, happen to agree. Nothing enforces it, and the field that goes missing when they stop agreeing goes missing silently. Non-blocking, but worth pinning one way or the other.

Background — why the ordering matters. Two subsystems apply the same server-side aggregation, in opposite orders. constructHistory in packages/runtime-common/ai/history.ts maps getAggregatedReplacement over the whole event list first, then calls parseContentData(rawEvent) on the substituted event. Decoding after substitution means that path structurally cannot lose data, however many nesting levels the homeserver bundles. The host inverts it: Room.addEvent decodes at ingest, and RoomResource.getAggregatedReplacement (packages/host/app/resources/room.ts) substitutes later, in loadRoomMessage. That inversion is the bug's actual cause; this hunk compensates for it by pre-decoding the one nesting level the substitution can reach.

Verified against the real path. loadAllTimelineEvents (matrix-service.ts) builds its backfill timeline with 'org.matrix.msc3874.not_rel_types': ['m.replace'], so the edit events themselves never come back on a reload and the bundle under unsigned['m.relations']['m.replace'] is the only carrier of the latest edit — the diagnosis is exact. Those events flow timelineQueue → drainTimeline → buildEventForProcessing (a cloneDeep, so the in-place mutation here can't touch SDK state) → processDecryptedEvent → addRoomEvent → here, which is the single funnel for AI-room timeline events. Downstream, message-builder.ts reads (this.event.content as CardMessageContent)?.data?.usage at two sites on the substituted event, and components/matrix/room.gts sums message.usage into the conversation total — so a string data drops the counts for every pre-reload turn.

The ask. Either match the server's ordering — export parseContentData from runtime-common/ai/history.ts and call it in RoomResource.getAggregatedReplacement right after finalRawEvent = replacedRawEvent, keeping the ingest decode here for non-aggregated events — or, if you'd rather leave the fix where it is, add a line at getAggregatedReplacement recording that its substituted event is only safe because addEvent pre-decodes the bundle. As it stands, whoever next moves this decode or adds a nesting level reproduces the same silent field loss with neither a comment nor a test in the way (see the file-level comment on the new test).

Second, smaller thing on the same lines: neither parse is tolerant. parseContentData wraps its parse in try/catch, logs, and throws a typed HistoryConstructionError; both parses here are bare. A throw out of addEvent propagates through drainTimeline, whose loop is try/finally with no catch and which has already emptied timelineQueue — so one unparseable data drops every remaining event in that batch and rejects the await this.drainTimeline() in loadAllTimelineEvents, i.e. the rest of the room's backfill. The trigger is unlikely, since sendMatrixEvent produces these strings itself, and it's pre-existing for the event's own content.data — but this hunk adds a second site, so it's worth a decision rather than inheritance. One try/catch around both that logs and leaves the string in place degrades to "this message loses its usage" instead of "the timeline stops loading".

Scope: the first point is a missing guard on an otherwise-correct fix (regression class, non-blocking); the second is pre-existing and widened by this hunk (non-blocking).

Generated by Claude Code

if (!eventId) {
throw new Error(
Comment thread
Copilot marked this conversation as resolved.
`bug: event ID is undefined for event ${JSON.stringify(
Expand Down
70 changes: 70 additions & 0 deletions packages/host/tests/unit/room-test.ts
View file
Open in desktop

@habdelra habdelra Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This test pins the decode but not the contract its own comment claims, and nothing else in the host suite covers aggregation bundles — so the regression it guards can come back green. Non-blocking, but the coverage ask is the one I'd most like to see land in this PR.

What the suite covers today. grep -rn "m.relations" packages/host/tests returns one unrelated line in query-fields-test.gts and nothing else; the only aggregation-bundle fixture anywhere in the repo is packages/ai-bot/tests/debug-test.ts. So the host's entire bundle path — RoomResource.getAggregatedReplacement → MessageBuilder → message.usage → the conversation total in components/matrix/room.gts — has no test at all.

What this test does and doesn't reach. It asserts that JSON.parse ran on two objects, and it does fail without the fix, which is the important half. But the comment above it claims the consequences — "token usage, context.agentId, and with it the agent gate for tool auto-execution" — and no assertion here reaches any of them. A change that keeps these two parses while altering the substitution site, or that moves the decode to the substitution site as suggested in the comment on room.ts, leaves this test green while the displayed total goes wrong again. That's the failure mode this PR exists to prevent.

The ask. Add a case one layer up: feed an original plus its bundled edit through the room resource and assert the rebuilt message's usage (and context.agentId, if it's cheap from there). packages/host/tests/unit/message-builder-test.ts is the closest precedent for the setup. Keeping this unit test alongside it is fine — it localises the failure — but on its own it doesn't pin the thing that broke.

Two smaller things. room.addEvent(event as any) means nothing checks that the fixture resembles a real timeline event, which also erases the status: null as EventStatus | null annotation and makes the EventStatus import decorative. The module under test already exports TempEvent — does the fixture type as that? I couldn't check locally (workspace deps aren't installed in my checkout), so treat it as a question rather than a claim; if it does, dropping the cast makes fixture drift a compile error. Separately, the module name is Unit | matrix | room but the file sits at tests/unit/room-test.ts, while tests/unit/matrix/ already exists and holds login-error-text-test.ts under the matching Unit | matrix | … prefix.

Scope: test coverage — the room-resource test is a follow-up at worst, the placement and typing are trivial.

Generated by Claude Code

Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { module, test } from 'qunit';

import Room from '@cardstack/host/lib/matrix-classes/room';

import type { EventStatus } from 'matrix-js-sdk';

module('Unit | matrix | room', function () {
test('addEvent decodes the wire-encoded data of the event and of its aggregation bundle', function (assert) {
// `content.data` is a JSON string on the wire. Backfilled events also
// carry their latest edit as a server-side aggregation bundle under
// unsigned['m.relations']['m.replace'], whose content the room resource
// substitutes for the event's own (getAggregatedReplacement) — so a
// string left undecoded there silently drops every field read off
// `data` on rebuild: token usage, context.agentId, and with it the
// agent gate for tool auto-execution.
let room = new Room('!room1:localhost');
let usage = { promptTokens: 10, completionTokens: 2, costUsd: 0.01 };
let event = {
event_id: '$original',
type: 'm.room.message',
room_id: '!room1:localhost',
origin_server_ts: 1,
status: null as EventStatus | null,
content: {
msgtype: 'app.boxel.message',
body: 'streaming…',
data: JSON.stringify({ context: { agentId: 'agent-1' } }),
},
unsigned: {
age: 0,
'm.relations': {
'm.replace': {
event_id: '$edit',
type: 'm.room.message',
origin_server_ts: 2,
content: {
msgtype: 'app.boxel.message',
body: 'the whole answer',
data: JSON.stringify({ context: { agentId: 'agent-1' }, usage }),
'm.relates_to': {
rel_type: 'm.replace',
event_id: '$original',
},
},
},
},
},
};

room.addEvent(event as any);

let added = room.events[0] as any;
assert.deepEqual(
added.content.data.context,
{ agentId: 'agent-1' },
'the event’s own data is decoded',
);
let bundled = added.unsigned['m.relations']['m.replace'];
assert.deepEqual(
bundled.content.data.usage,
usage,
'the aggregation bundle’s data is decoded too, so a rebuilt message keeps its usage',
);
assert.strictEqual(
bundled.content.data.context.agentId,
'agent-1',
'and keeps its agent id',
);
});
});
Loading