feat: compaction indicator in token bar by dimakis · Pull Request #428 · dimakis/mitzo

dimakis · 2026-07-02T00:25:36Z

Summary

Detect SDK compaction content blocks (type: 'compaction') and emit compaction_status events to the frontend
TokenBar replaces normal display with a pulsing amber COMPACTING label during active compaction
Crash-safe: compaction_delta events are explicitly skipped, compaction blocks don't create phantom snapshot state
Safety reset: if compaction flag is still active when the query ends, it resets with a warning log

Files changed

server/query-loop.ts — detect compaction blocks, emit status events, safety reset
frontend/src/components/TokenBar.tsx — compacting state renders amber indicator
frontend/src/styles/global.css — pulsing amber animation
frontend/src/types/ws-messages.ts — CompactionStatusMsg type
packages/client/src/slices/tokens.ts — compacting: boolean in TokensState
packages/client/src/protocol-parser.ts — parse compaction_status
server/__tests__/token-update.test.ts — full compaction block lifecycle coverage
packages/client/__tests__/protocol-parser.test.ts — compaction_status parsing

Test plan

Server: token-update tests pass (7/7) — includes compaction block start/delta/stop + status events
Client: protocol-parser tests pass (57/57) — includes compaction_status flag setting
Manual: trigger compaction in a long session and verify amber indicator appears

🤖 Generated with Claude Code

Telos items from the Python script were never ingested into workloadStore, so the promote endpoint always returned 404. The frontend now sends item data (title, contextHints, sources) as fallback fields in the promote request body, and the endpoint uses them when workloadStore.get() misses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Remove misleading `as string` cast on taskHint, use nullish coalescing - Make sources author/snippet optional in schema for real-world data - Omit `item` from response when not in workloadStore (cleaner contract) - Remove comment coupling schema to specific caller - Add tests for fallback promote path and no-broadcast behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Detect SDK compaction content blocks and emit compaction_status events so the frontend can show a pulsing amber COMPACTING label instead of appearing to hang. Includes safety reset on query end and crash-safe handling of compaction_delta events. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

TokensState now has a compacting boolean — update all test fixtures that construct the type directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dimakis

Centaur Review

Found 4 issue(s) (1 warning).

`server/tests/workload-routes.test.ts`

Well-structured feature with good test coverage for the happy path. The main concern is the broadcast-interception test for the promote fallback path, which passes vacuously due to monkey-patching a non-existent property; the compaction safety-reset path also lacks test coverage.

🟡 missing_tests (L548): This test is ineffective — it monkey-patches (app as any)._workloadBroadcast, but that property does not exist on the app object. The actual broadcast callback is the module-level closure onWorkloadBroadcast (set via setWorkloadBroadcast() in index.ts, never called in the test harness). Since onWorkloadBroadcast is always null in tests, the assertion expect(workloadBroadcasts).toHaveLength(0) passes vacuously for every promote call, not just the fallback path. The test doesn't verify what it claims. [fixable]

`server/tests/token-update.test.ts`

Well-structured feature with good test coverage for the happy path. The main concern is the broadcast-interception test for the promote fallback path, which passes vacuously due to monkey-patching a non-existent property; the compaction safety-reset path also lacks test coverage.

🔵 missing_tests (L449): 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. [fixable]

`server/query-loop.ts`

Well-structured feature with good test coverage for the happy path. The main concern is the broadcast-interception test for the promote fallback path, which passes vacuously due to monkey-patching a non-existent property; the compaction safety-reset path also lacks test coverage.

🔵 style (L970): Compaction content_block_start still runs the generic registration at lines 900-902 (blockIdByIndex.set + openBlockCount++) before reaching the compaction branch. The blockId and index mapping are unused — at content_block_stop, the code finds the blockId but then no snapshot block exists, so block_end is silently skipped. The openBlockCount stays balanced (incremented then decremented), so this is harmless, but a brief comment noting this intentional fall-through would prevent future confusion. [fixable]

`packages/client/src/protocol-parser.ts`

Well-structured feature with good test coverage for the happy path. The main concern is the broadcast-interception test for the promote fallback path, which passes vacuously due to monkey-patching a non-existent property; the compaction safety-reset path also lacks test coverage.

🔵 style (L525): msg.active as boolean is 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 a Boolean(msg.active) coercion would be more defensive against unexpected payloads. [fixable]

dimakis · 2026-07-04T01:17:52Z

+    expect(res.body.item).toBeUndefined();
+  });
+
+  it('POST /api/workload/items/:id/promote — fallback does not broadcast workload update', async () => {


🟡 missing_tests: This test is ineffective — it monkey-patches (app as any)._workloadBroadcast, but that property does not exist on the app object. The actual broadcast callback is the module-level closure onWorkloadBroadcast (set via setWorkloadBroadcast() in index.ts, never called in the test harness). Since onWorkloadBroadcast is always null in tests, the assertion expect(workloadBroadcasts).toHaveLength(0) passes vacuously for every promote call, not just the fallback path. The test doesn't verify what it claims. [fixable]

dimakis · 2026-07-04T01:17:52Z

@@ -429,6 +447,12 @@ describe('token_update emission', () => {
    // The final token_update should include numCompactions: 1
    const last = tokenUpdates[tokenUpdates.length - 1];
    expect(last).toMatchObject({ numCompactions: 1 });


🔵 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. [fixable]

dimakis · 2026-07-04T01:17:52Z

                  blockType: 'text',
                }),
              );
+            } else if (blockType === 'compaction') {


🔵 style: Compaction content_block_start still runs the generic registration at lines 900-902 (blockIdByIndex.set + openBlockCount++) before reaching the compaction branch. The blockId and index mapping are unused — at content_block_stop, the code finds the blockId but then no snapshot block exists, so block_end is silently skipped. The openBlockCount stays balanced (incremented then decremented), so this is harmless, but a brief comment noting this intentional fall-through would prevent future confusion. [fixable]

dimakis · 2026-07-04T01:17:52Z

      break;
    }

+    case 'compaction_status':


🔵 style: msg.active as boolean is 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 a Boolean(msg.active) coercion would be more defensive against unexpected payloads. [fixable]

- Add comment documenting intentional blockId fall-through for compaction blocks - Use Boolean() coercion instead of bare type assertion in parser - Add test for safety reset path (compaction started, success signal missing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dimakis

Centaur Review

Found 4 issue(s) (1 warning).

`server/tests/workload-routes.test.ts`

Clean two-feature PR (compaction indicator + promote fallback). The compaction lifecycle is well-structured with proper safety nets. One test (fallback does not broadcast) has a broken interception mechanism that makes it pass vacuously rather than verifying the guard logic.

🟡 missing_tests (L548): The 'fallback does not broadcast workload update' test intercepts (app as any)._workloadBroadcast, but that property doesn't exist on the Express app. The actual broadcast function is the module-level onWorkloadBroadcast closure, set via setWorkloadBroadcast(). Since onWorkloadBroadcast is never set in this test suite, it's always null, so the test passes vacuously — it would pass even if the if (updatedItem) guard were removed. The assertion verifies the correct outcome but doesn't actually test the code path. [fixable]
🔵 style (L512): The renamed test description 'returns 404 for missing item with no body title' is slightly awkward — it reads as though the body has 'no body title' rather than 'no title in body'. Consider 'returns 404 when item not found and body has no title'. [fixable]

`server/app.ts`

Clean two-feature PR (compaction indicator + promote fallback). The compaction lifecycle is well-structured with proper safety nets. One test (fallback does not broadcast) has a broken interception mechanism that makes it pass vacuously rather than verifying the guard logic.

🔵 style (L1929): When the item isn't found AND no title is provided, a 404 is returned with 'Item not found and no title provided'. A 404 is semantically correct for a missing resource, but when the item is intentionally absent (Telos fallback path) and the real issue is the missing title field, a 400 would more accurately convey that the request body is incomplete. Minor — current behavior is workable since the only caller always provides a title. [fixable]

`server/tests/token-update.test.ts`

Clean two-feature PR (compaction indicator + promote fallback). The compaction lifecycle is well-structured with proper safety nets. One test (fallback does not broadcast) has a broken interception mechanism that makes it pass vacuously rather than verifying the guard logic.

🔵 missing_tests (L447): No test covers the safety-net path where the result event arrives while compacting is still true (i.e., SDK crashed or skipped the compact_result: 'success' system message). This would verify the warn log and the fallback compaction_status: active: false emission. The logic is straightforward but it's a defensive branch worth covering. [fixable]

dimakis · 2026-07-04T09:43:07Z

+    expect(res.body.item).toBeUndefined();
+  });
+
+  it('POST /api/workload/items/:id/promote — fallback does not broadcast workload update', async () => {


🟡 missing_tests: The 'fallback does not broadcast workload update' test intercepts (app as any)._workloadBroadcast, but that property doesn't exist on the Express app. The actual broadcast function is the module-level onWorkloadBroadcast closure, set via setWorkloadBroadcast(). Since onWorkloadBroadcast is never set in this test suite, it's always null, so the test passes vacuously — it would pass even if the if (updatedItem) guard were removed. The assertion verifies the correct outcome but doesn't actually test the code path. [fixable]

dimakis · 2026-07-04T09:43:07Z

  });

-  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 () => {


🔵 style: The renamed test description 'returns 404 for missing item with no body title' is slightly awkward — it reads as though the body has 'no body title' rather than 'no title in body'. Consider 'returns 404 when item not found and body has no title'. [fixable]

dimakis · 2026-07-04T09:43:07Z


+  const hints = item?.contextHints ?? body.data.contextHints;
+  const taskHint = hints?.taskHint ?? undefined;
+


🔵 style: When the item isn't found AND no title is provided, a 404 is returned with 'Item not found and no title provided'. A 404 is semantically correct for a missing resource, but when the item is intentionally absent (Telos fallback path) and the real issue is the missing title field, a 400 would more accurately convey that the request body is incomplete. Minor — current behavior is workable since the only caller always provides a title. [fixable]

dimakis · 2026-07-04T09:43:08Z

@@ -429,6 +447,12 @@ describe('token_update emission', () => {
    // The final token_update should include numCompactions: 1


🔵 missing_tests: No test covers the safety-net path where the result event arrives while compacting is still true (i.e., SDK crashed or skipped the compact_result: 'success' system message). This would verify the warn log and the fallback compaction_status: active: false emission. The logic is straightforward but it's a defensive branch worth covering. [fixable]

dimakis

Centaur Review

Found 3 issue(s) (1 warning).

`server/tests/workload-routes.test.ts`

Compaction indicator is well-implemented with proper state lifecycle (including defensive reset on SDK crash). Promote fallback logic is sound, but the 'no broadcast' test is broken — it monkey-patches a non-existent app property instead of the module-scoped callback, passing vacuously.

🟡 bugs (L550): Test 'fallback does not broadcast workload update' monkey-patches (app as any)._workloadBroadcast, but the actual broadcast callback is a module-scoped closure variable onWorkloadBroadcast set via setWorkloadBroadcast(). The test creates a property on the app object that nothing reads, so the assertion passes vacuously — the broadcast is never intercepted. Furthermore, setWorkloadBroadcast is never called in the test setup, so onWorkloadBroadcast is null and no broadcast fires at all. This test proves nothing about the fallback path's broadcast behavior. [fixable]
🔵 style (L512): The test description was renamed to 'returns 404 for missing item with no body title' but the test body only sends { description: '' } — consider also adding a test that sends neither title nor description (empty body) to cover the pure 404 case. [fixable]

`server/app.ts`

Compaction indicator is well-implemented with proper state lifecycle (including defensive reset on SDK crash). Promote fallback logic is sound, but the 'no broadcast' test is broken — it monkey-patches a non-existent app property instead of the module-scoped callback, passing vacuously.

🔵 style (L1922): When the item doesn't exist and no body.data.title is provided, the error response is 404 with 'Item not found and no title provided'. A 404 is debatable when the real issue is a missing required field — 400 would be more accurate for the 'no title provided' case. Consider splitting: 404 when !item && !body.data.title, vs keeping 404 for a true 'not found'. Minor since the frontend always sends a title for Telos items. [fixable]

dimakis · 2026-07-04T09:46:01Z

+
+  it('POST /api/workload/items/:id/promote — fallback does not broadcast workload update', async () => {
+    const broadcasts: unknown[] = [];
+    const origBroadcast = (app as any)._workloadBroadcast;


🟡 bugs: Test 'fallback does not broadcast workload update' monkey-patches (app as any)._workloadBroadcast, but the actual broadcast callback is a module-scoped closure variable onWorkloadBroadcast set via setWorkloadBroadcast(). The test creates a property on the app object that nothing reads, so the assertion passes vacuously — the broadcast is never intercepted. Furthermore, setWorkloadBroadcast is never called in the test setup, so onWorkloadBroadcast is null and no broadcast fires at all. This test proves nothing about the fallback path's broadcast behavior. [fixable]

dimakis · 2026-07-04T09:46:01Z

  });

-  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 () => {


🔵 style: The test description was renamed to 'returns 404 for missing item with no body title' but the test body only sends { description: '' } — consider also adding a test that sends neither title nor description (empty body) to cover the pure 404 case. [fixable]

dimakis · 2026-07-04T09:46:01Z

+
+  // Resolve title and context from workloadStore item or fallback body data (Telos items)
+  const title = item?.title ?? body.data.title;
+  if (!title) {


🔵 style: When the item doesn't exist and no body.data.title is provided, the error response is 404 with 'Item not found and no title provided'. A 404 is debatable when the real issue is a missing required field — 400 would be more accurate for the 'no title provided' case. Consider splitting: 404 when !item && !body.data.title, vs keeping 404 for a true 'not found'. Minor since the frontend always sends a title for Telos items. [fixable]

dimakis and others added 4 commits June 28, 2026 21:45

fix: add compacting field to test fixtures

acd97db

TokensState now has a compacting boolean — update all test fixtures that construct the type directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dimakis commented Jul 4, 2026

View reviewed changes


		const hints = item?.contextHints ?? body.data.contextHints;
		const taskHint = hints?.taskHint ?? undefined;

		@@ -429,6 +447,12 @@ describe('token_update emission', () => {
		// The final token_update should include numCompactions: 1

Conversation

dimakis commented Jul 2, 2026

Summary

Files changed

Test plan

Uh oh!

dimakis left a comment

Choose a reason for hiding this comment

Centaur Review

server/__tests__/workload-routes.test.ts

server/__tests__/token-update.test.ts

server/query-loop.ts

packages/client/src/protocol-parser.ts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimakis left a comment

Choose a reason for hiding this comment

Centaur Review

server/__tests__/workload-routes.test.ts

server/app.ts

server/__tests__/token-update.test.ts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimakis left a comment

Choose a reason for hiding this comment

Centaur Review

server/__tests__/workload-routes.test.ts

server/app.ts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`server/tests/workload-routes.test.ts`

`server/tests/token-update.test.ts`

`server/query-loop.ts`

`packages/client/src/protocol-parser.ts`

`server/tests/workload-routes.test.ts`

`server/app.ts`

`server/tests/token-update.test.ts`

`server/tests/workload-routes.test.ts`

`server/app.ts`