-
Notifications
You must be signed in to change notification settings - Fork 0
feat: compaction indicator in token bar #428
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.
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
Changes from all commits
d072ca5
25f2afc
daf7287
acd97db
32fa705
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -389,6 +389,24 @@ describe('token_update emission', () => { | |
| }, | ||
| { type: 'stream_event', event: { type: 'content_block_stop', index: 0 } }, | ||
| { type: 'assistant', message: { content: [] }, session_id: 'sess-compact' }, | ||
| // SDK compaction content block — start, delta, stop | ||
| { | ||
| type: 'stream_event', | ||
| event: { | ||
| type: 'content_block_start', | ||
| index: 1, | ||
| content_block: { type: 'compaction' }, | ||
| }, | ||
| }, | ||
| { | ||
| type: 'stream_event', | ||
| event: { | ||
| type: 'content_block_delta', | ||
| index: 1, | ||
| delta: { type: 'compaction_delta', content: 'Summary of prior context...' }, | ||
| }, | ||
| }, | ||
| { type: 'stream_event', event: { type: 'content_block_stop', index: 1 } }, | ||
| // SDK system status: compaction completed | ||
| { | ||
| type: 'system', | ||
|
|
@@ -429,6 +447,59 @@ describe('token_update emission', () => { | |
| // The final token_update should include numCompactions: 1 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 missing_tests: No test covers the safety-net path where the |
||
| const last = tokenUpdates[tokenUpdates.length - 1]; | ||
| expect(last).toMatchObject({ numCompactions: 1 }); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 missing_tests: The safety reset at query-loop.ts:573-581 (compaction flag reset on result when the SDK crashes/aborts before sending the system status success signal) has no test coverage. Only the happy path (content_block_start → system status success) is tested. A test with compaction started but result arriving without a preceding system status success would exercise the warn+reset path. |
||
|
|
||
| // compaction_status events should bracket the compaction | ||
| const compactionStatuses = transport.sent.filter((m) => m.type === 'compaction_status'); | ||
| expect(compactionStatuses).toHaveLength(2); | ||
| expect(compactionStatuses[0]).toMatchObject({ type: 'compaction_status', active: true }); | ||
| expect(compactionStatuses[1]).toMatchObject({ type: 'compaction_status', active: false }); | ||
| }); | ||
|
|
||
| it('resets compacting flag on result when success signal is missing', async () => { | ||
| const events: Record<string, unknown>[] = [ | ||
| { | ||
| type: 'stream_event', | ||
| parent_tool_use_id: null, | ||
| event: { | ||
| type: 'message_start', | ||
| message: { id: 'msg-crash', usage: { input_tokens: 180000 } }, | ||
| }, | ||
| }, | ||
| { | ||
| type: 'stream_event', | ||
| event: { type: 'content_block_start', index: 0, content_block: { type: 'text' } }, | ||
| }, | ||
| { type: 'stream_event', event: { type: 'content_block_stop', index: 0 } }, | ||
| { type: 'assistant', message: { content: [] }, session_id: 'sess-crash' }, | ||
| // Compaction starts but SDK never sends compact_result: 'success' | ||
| { | ||
| type: 'stream_event', | ||
| event: { | ||
| type: 'content_block_start', | ||
| index: 1, | ||
| content_block: { type: 'compaction' }, | ||
| }, | ||
| }, | ||
| { type: 'stream_event', event: { type: 'content_block_stop', index: 1 } }, | ||
| // Result arrives directly — no compact_result system message | ||
| { | ||
| type: 'result', | ||
| session_id: 'sess-crash', | ||
| usage: { input_tokens: 180000, output_tokens: 500 }, | ||
| total_cost_usd: 0.01, | ||
| num_turns: 1, | ||
| duration_ms: 3000, | ||
| duration_api_ms: 2000, | ||
| }, | ||
| ]; | ||
|
|
||
| await runQueryLoop(eventStream(events), clientId, registry, abortController); | ||
|
|
||
| const compactionStatuses = transport.sent.filter((m) => m.type === 'compaction_status'); | ||
| // Should have active:true from block_start and active:false from safety reset | ||
| expect(compactionStatuses).toHaveLength(2); | ||
| expect(compactionStatuses[0]).toMatchObject({ active: true }); | ||
| expect(compactionStatuses[1]).toMatchObject({ active: false }); | ||
| }); | ||
|
|
||
| it('handles missing usage on message_start gracefully', async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -509,7 +509,7 @@ describe('workload routes', () => { | |
| expect(res.body.item.goalId).toBe(res.body.task.id); | ||
| }); | ||
|
|
||
| it('POST /api/workload/items/:id/promote — returns 404 for missing item', async () => { | ||
| it('POST /api/workload/items/:id/promote — returns 404 for missing item with no body title', async () => { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The renamed test description
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The test description was renamed to 'returns 404 for missing item with no body title' but the test body only sends |
||
| const res = await request(app) | ||
| .post('/api/workload/items/nonexistent-id/promote') | ||
| .set('Cookie', authCookie) | ||
|
|
@@ -518,6 +518,49 @@ describe('workload routes', () => { | |
| expect(res.status).toBe(404); | ||
| }); | ||
|
|
||
| it('POST /api/workload/items/:id/promote — creates task from body fallback (Telos item)', async () => { | ||
| const res = await request(app) | ||
| .post('/api/workload/items/telos-item-123/promote') | ||
| .set('Cookie', authCookie) | ||
| .send({ | ||
| title: 'Telos task title', | ||
| description: 'Extra context', | ||
| contextHints: { | ||
| repos: ['mitzo'], | ||
| taskHint: 'Check the API layer', | ||
| }, | ||
| sources: [{ type: 'telos', url: 'https://example.com', title: 'Source doc' }], | ||
| }); | ||
|
|
||
| expect(res.status).toBe(201); | ||
| expect(res.body.task).toMatchObject({ | ||
| title: 'Telos task title', | ||
| status: 'pending', | ||
| }); | ||
| expect(res.body.task.description).toContain('Extra context'); | ||
| expect(res.body.task.description).toContain('Check the API layer'); | ||
| expect(res.body.task.annotations).toEqual( | ||
| expect.arrayContaining([expect.stringContaining('Source doc')]), | ||
| ); | ||
| expect(res.body.item).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('POST /api/workload/items/:id/promote — fallback does not broadcast workload update', async () => { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 missing_tests: The 'fallback does not broadcast workload update' test intercepts |
||
| const broadcasts: unknown[] = []; | ||
| const origBroadcast = (app as any)._workloadBroadcast; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 bugs: Test 'fallback does not broadcast workload update' monkey-patches |
||
| (app as any)._workloadBroadcast = (msg: unknown) => broadcasts.push(msg); | ||
|
|
||
| await request(app) | ||
| .post('/api/workload/items/telos-no-broadcast/promote') | ||
| .set('Cookie', authCookie) | ||
| .send({ title: 'No broadcast test' }); | ||
|
|
||
| const workloadBroadcasts = broadcasts.filter((b: any) => b.type === 'workload_item_updated'); | ||
| expect(workloadBroadcasts).toHaveLength(0); | ||
|
|
||
| (app as any)._workloadBroadcast = origBroadcast; | ||
| }); | ||
|
|
||
| it('POST /api/workload/items/:id/promote — broadcasts workload_item_updated', async () => { | ||
| // Create an item first | ||
| const signal = { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 style:
msg.active as booleanis a bare type assertion on an untyped message field. Other cases in this switch use similar casts (e.g.,msg.agentContext as number), so this is consistent with the codebase, but aBoolean(msg.active)coercion would be more defensive against unexpected payloads.[fixable]