reduce event union b removing unused members#1379
reduce event union b removing unused members#1379juliusmarminge wants to merge 4 commits intomainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XXL
- Normalize provider token-usage events into thread activities - Add a compact context window meter to chat - Handle context compaction markers in the timeline
- Replace timeline marker treatment with normal grouped work rows - Switch context window meter to an SVG ring and show auto-compaction
- Model orchestration activities with typed payloads - Drop obsolete provider runtime event mappings - Improve context window snapshot derivation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| @@ -454,16 +454,18 @@ const makeProjectionSnapshotQuery = Effect.gen(function* () { | |||
| for (const row of activityRows) { | |||
There was a problem hiding this comment.
🟡 Medium Layers/ProjectionSnapshotQuery.ts:454
Schema.decodeUnknownSync(OrchestrationThreadActivity) on line 458 throws synchronously on decode failure, becoming an unhandled defect that bypasses the ProjectionRepositoryError error type in the function signature. Consider using Schema.decodeUnknownEffect with toPersistenceDecodeError mapping instead, consistent with line 574.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts around line 454:
`Schema.decodeUnknownSync(OrchestrationThreadActivity)` on line 458 throws synchronously on decode failure, becoming an unhandled defect that bypasses the `ProjectionRepositoryError` error type in the function signature. Consider using `Schema.decodeUnknownEffect` with `toPersistenceDecodeError` mapping instead, consistent with line 574.
Evidence trail:
apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts lines 458 (uses Schema.decodeUnknownSync), line 45 (defines decodeReadModel using Schema.decodeUnknownEffect), lines 574-576 (shows proper error mapping pattern with toPersistenceDecodeError); apps/server/src/orchestration/Services/ProjectionSnapshotQuery.ts line 24 (function signature declares ProjectionRepositoryError as error type)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
juliusmarminge
changed the title
juliusmarminge
force-pushed
the
t3code/context-window-meter
branch
from
2ab0eb0 to
3f48ad2
Compare
There was a problem hiding this comment.
🔴 Critical
Multiple tests have Stream.take counts that don't match the number of events expected in their assertions, causing them to hang indefinitely waiting for events that never arrive. For example at line 608, Stream.take(adapter.streamEvents, 10) waits for 10 events but the assertion at lines 718-730 only expects 9 events (after removing "session.configured"). Similar mismatches occur at lines 781, 959, 1035, 1224, 1271, 1325, 1391, 1650, and 1718 — each taking one more event than will ever be emitted. Decrement each Stream.take count by one to match the shortened expected event arrays.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/ClaudeAdapter.test.ts around line 608:
Multiple tests have `Stream.take` counts that don't match the number of events expected in their assertions, causing them to hang indefinitely waiting for events that never arrive. For example at line 608, `Stream.take(adapter.streamEvents, 10)` waits for 10 events but the assertion at lines 718-730 only expects 9 events (after removing `"session.configured"`). Similar mismatches occur at lines 781, 959, 1035, 1224, 1271, 1325, 1391, 1650, and 1718 — each taking one more event than will ever be emitted. Decrement each `Stream.take` count by one to match the shortened expected event arrays.
Evidence trail:
1. Line 608 showing `Stream.take(adapter.streamEvents, 10)`: apps/server/src/provider/Layers/ClaudeAdapter.test.ts:608 (REVIEWED_COMMIT)
2. Lines 718-730 showing 9 expected events: apps/server/src/provider/Layers/ClaudeAdapter.test.ts:718-730 (REVIEWED_COMMIT)
3. Line 1035 showing `Stream.take(adapter.streamEvents, 6)`: apps/server/src/provider/Layers/ClaudeAdapter.test.ts:1035 (REVIEWED_COMMIT)
4. Lines 1066-1073 showing 5 expected events: apps/server/src/provider/Layers/ClaudeAdapter.test.ts:1066-1073 (REVIEWED_COMMIT)
5. git_diff of MERGE_BASE to REVIEWED_COMMIT showing `"session.configured"` removed from expected arrays without adjusting Stream.take counts
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Schema rejects failed task activities at read time
- Added a second union member
makeThreadActivitySchema("task.completed", "error", TaskCompletedActivityPayload)so the schema accepts both "info" and "error" tones for task.completed activities, preventing decode failures on failed tasks.
- Added a second union member
Or push these changes by commenting:
@cursor push fac4623d46
Preview (fac4623d46)
diff --git a/packages/contracts/src/orchestration.ts b/packages/contracts/src/orchestration.ts
--- a/packages/contracts/src/orchestration.ts
+++ b/packages/contracts/src/orchestration.ts
@@ -397,6 +397,7 @@
makeThreadActivitySchema("task.started", "info", TaskStartedActivityPayload),
makeThreadActivitySchema("task.progress", "info", TaskProgressActivityPayload),
makeThreadActivitySchema("task.completed", "info", TaskCompletedActivityPayload),
+ makeThreadActivitySchema("task.completed", "error", TaskCompletedActivityPayload),
makeThreadActivitySchema("context-compaction", "info", ContextCompactionActivityPayload),
makeThreadActivitySchema("context-window.updated", "info", ThreadTokenUsageSnapshot),
makeThreadActivitySchema("tool.updated", "tool", ToolUpdatedActivityPayload),| makeThreadActivitySchema("user-input.resolved", "info", UserInputResolvedActivityPayload), | ||
| makeThreadActivitySchema("task.started", "info", TaskStartedActivityPayload), | ||
| makeThreadActivitySchema("task.progress", "info", TaskProgressActivityPayload), | ||
| makeThreadActivitySchema("task.completed", "info", TaskCompletedActivityPayload), |
There was a problem hiding this comment.
Schema rejects failed task activities at read time
High Severity
The OrchestrationThreadActivity union defines task.completed with a hardcoded tone of "info", but runtimeEventToActivities in ProviderRuntimeIngestion.ts produces task.completed activities with tone: "error" when event.payload.status === "failed". Since ProjectionSnapshotQuery.ts now validates activities via Schema.decodeUnknownSync(OrchestrationThreadActivity), any thread containing a failed task will throw a decode error, crashing the snapshot query and making that thread inaccessible.
Additional Locations (1)
- Canonicalize provider runtime tool data and usage snapshots - Update work log extraction and user input contracts for the new schema - Add coverage for the revised lifecycle and ingestion shapes
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Tool lifecycle data computed twice with identical arguments
- Stored the result of canonicalToolLifecycleData/buildCanonicalToolLifecycleData in a local variable at all four call sites (ClaudeAdapter lines 1543, 1747, 1825 and CodexAdapter line 673) to eliminate redundant computation, matching the existing pattern at line 1890.
- ✅ Fixed: Identical utility functions duplicated across adapter files
- Extracted the identical normalizeCommandValue and pushChangedFile functions into a new shared module at apps/server/src/provider/toolDataUtils.ts and updated both adapter files to import from it.
- ✅ Fixed:
toCanonicalJsonValuesilently converts null to undefined- Removed the
?? undefinednullish coalescing and cast the JSON.parse result directly as CanonicalJsonValue, preserving null as a valid return value.
- Removed the
Or push these changes by commenting:
@cursor push 5b3e710759
Preview (5b3e710759)
diff --git a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
--- a/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
+++ b/apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts
@@ -107,10 +107,9 @@
if (value === undefined) {
return undefined;
}
- const normalized = JSON.parse(
+ return JSON.parse(
JSON.stringify(value, (_key, nestedValue) => (nestedValue === undefined ? null : nestedValue)),
- ) as CanonicalJsonValue | null;
- return normalized ?? undefined;
+ ) as CanonicalJsonValue;
}
function buildContextWindowActivityPayload(
diff --git a/apps/server/src/provider/Layers/ClaudeAdapter.ts b/apps/server/src/provider/Layers/ClaudeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeAdapter.ts
@@ -75,6 +75,7 @@
type ProviderAdapterError,
} from "../Errors.ts";
import { ClaudeAdapter, type ClaudeAdapterShape } from "../Services/ClaudeAdapter.ts";
+import { normalizeCommandValue, pushChangedFile } from "../toolDataUtils.ts";
import { type EventNdjsonLogger, makeEventNdjsonLogger } from "./EventNdjsonLogger.ts";
const PROVIDER = "claudeAgent" as const;
@@ -747,32 +748,6 @@
}
}
-function normalizeCommandValue(value: unknown): string | undefined {
- if (typeof value === "string") {
- const trimmed = value.trim();
- return trimmed.length > 0 ? trimmed : undefined;
- }
- if (!Array.isArray(value)) {
- return undefined;
- }
- const parts = value
- .map((entry) => (typeof entry === "string" ? entry.trim() : ""))
- .filter((entry) => entry.length > 0);
- return parts.length > 0 ? parts.join(" ") : undefined;
-}
-
-function pushChangedFile(target: string[], seen: Set<string>, value: unknown) {
- if (typeof value !== "string") {
- return;
- }
- const normalized = value.trim();
- if (normalized.length === 0 || seen.has(normalized)) {
- return;
- }
- seen.add(normalized);
- target.push(normalized);
-}
-
function collectChangedFiles(value: unknown, target: string[], seen: Set<string>, depth: number) {
if (depth > 4 || target.length >= 12) {
return;
@@ -1540,19 +1515,14 @@
status: status === "completed" ? "completed" : "failed",
title: tool.title,
...(tool.detail ? { detail: tool.detail } : {}),
- ...(canonicalToolLifecycleData({
- itemType: tool.itemType,
- toolName: tool.toolName,
- input: tool.input,
- })
- ? {
- data: canonicalToolLifecycleData({
- itemType: tool.itemType,
- toolName: tool.toolName,
- input: tool.input,
- }),
- }
- : {}),
+ ...(() => {
+ const toolData = canonicalToolLifecycleData({
+ itemType: tool.itemType,
+ toolName: tool.toolName,
+ input: tool.input,
+ });
+ return toolData ? { data: toolData } : {};
+ })(),
},
providerRefs: nativeProviderRefs(context, { providerItemId: tool.itemId }),
raw: {
@@ -1744,19 +1714,14 @@
status: "inProgress",
title: nextTool.title,
...(nextTool.detail ? { detail: nextTool.detail } : {}),
- ...(canonicalToolLifecycleData({
- itemType: nextTool.itemType,
- toolName: nextTool.toolName,
- input: nextTool.input,
- })
- ? {
- data: canonicalToolLifecycleData({
- itemType: nextTool.itemType,
- toolName: nextTool.toolName,
- input: nextTool.input,
- }),
- }
- : {}),
+ ...(() => {
+ const toolData = canonicalToolLifecycleData({
+ itemType: nextTool.itemType,
+ toolName: nextTool.toolName,
+ input: nextTool.input,
+ });
+ return toolData ? { data: toolData } : {};
+ })(),
},
providerRefs: nativeProviderRefs(context, { providerItemId: nextTool.itemId }),
raw: {
@@ -1822,19 +1787,14 @@
status: "inProgress",
title: tool.title,
...(tool.detail ? { detail: tool.detail } : {}),
- ...(canonicalToolLifecycleData({
- itemType: tool.itemType,
- toolName: tool.toolName,
- input: toolInput,
- })
- ? {
- data: canonicalToolLifecycleData({
- itemType: tool.itemType,
- toolName: tool.toolName,
- input: toolInput,
- }),
- }
- : {}),
+ ...(() => {
+ const toolData = canonicalToolLifecycleData({
+ itemType: tool.itemType,
+ toolName: tool.toolName,
+ input: toolInput,
+ });
+ return toolData ? { data: toolData } : {};
+ })(),
},
providerRefs: nativeProviderRefs(context, { providerItemId: tool.itemId }),
raw: {
diff --git a/apps/server/src/provider/Layers/CodexAdapter.ts b/apps/server/src/provider/Layers/CodexAdapter.ts
--- a/apps/server/src/provider/Layers/CodexAdapter.ts
+++ b/apps/server/src/provider/Layers/CodexAdapter.ts
@@ -33,6 +33,7 @@
type ProviderAdapterError,
} from "../Errors.ts";
import { CodexAdapter, type CodexAdapterShape } from "../Services/CodexAdapter.ts";
+import { normalizeCommandValue, pushChangedFile } from "../toolDataUtils.ts";
import {
CodexAppServerManager,
type CodexAppServerStartSessionInput,
@@ -111,32 +112,6 @@
return typeof value === "number" && Number.isFinite(value) ? value : undefined;
}
-function normalizeCommandValue(value: unknown): string | undefined {
- if (typeof value === "string") {
- const trimmed = value.trim();
- return trimmed.length > 0 ? trimmed : undefined;
- }
- if (!Array.isArray(value)) {
- return undefined;
- }
- const parts = value
- .map((entry) => (typeof entry === "string" ? entry.trim() : ""))
- .filter((entry) => entry.length > 0);
- return parts.length > 0 ? parts.join(" ") : undefined;
-}
-
-function pushChangedFile(target: string[], seen: Set<string>, value: unknown) {
- if (typeof value !== "string") {
- return;
- }
- const normalized = value.trim();
- if (normalized.length === 0 || seen.has(normalized)) {
- return;
- }
- seen.add(normalized);
- target.push(normalized);
-}
-
function collectChangedFiles(value: unknown, target: string[], seen: Set<string>, depth: number) {
if (depth > 4 || target.length >= 12) {
return;
@@ -670,9 +645,10 @@
...(status ? { status } : {}),
...(itemTitle(itemType) ? { title: itemTitle(itemType) } : {}),
...(detail ? { detail } : {}),
- ...(buildCanonicalToolLifecycleData(itemType, source, payload ?? {})
- ? { data: buildCanonicalToolLifecycleData(itemType, source, payload ?? {}) }
- : {}),
+ ...(() => {
+ const toolData = buildCanonicalToolLifecycleData(itemType, source, payload ?? {});
+ return toolData ? { data: toolData } : {};
+ })(),
},
};
}
diff --git a/apps/server/src/provider/toolDataUtils.ts b/apps/server/src/provider/toolDataUtils.ts
new file mode 100644
--- /dev/null
+++ b/apps/server/src/provider/toolDataUtils.ts
@@ -1,0 +1,25 @@
+export function normalizeCommandValue(value: unknown): string | undefined {
+ if (typeof value === "string") {
+ const trimmed = value.trim();
+ return trimmed.length > 0 ? trimmed : undefined;
+ }
+ if (!Array.isArray(value)) {
+ return undefined;
+ }
+ const parts = value
+ .map((entry) => (typeof entry === "string" ? entry.trim() : ""))
+ .filter((entry) => entry.length > 0);
+ return parts.length > 0 ? parts.join(" ") : undefined;
+}
+
+export function pushChangedFile(target: string[], seen: Set<string>, value: unknown) {
+ if (typeof value !== "string") {
+ return;
+ }
+ const normalized = value.trim();
+ if (normalized.length === 0 || seen.has(normalized)) {
+ return;
+ }
+ seen.add(normalized);
+ target.push(normalized);
+}| input: tool.input, | ||
| }), | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
Tool lifecycle data computed twice with identical arguments
Low Severity
canonicalToolLifecycleData (and buildCanonicalToolLifecycleData in the Codex adapter) is called twice with identical arguments — once for the truthiness check and once to get the value. This function performs non-trivial work including recursive collectChangedFiles traversal. The correct pattern of storing the result in a variable is already used at line 1890 of the same file, making this inconsistency easy to spot.
Additional Locations (2)
| } | ||
| seen.add(normalized); | ||
| target.push(normalized); | ||
| } |
There was a problem hiding this comment.
Identical utility functions duplicated across adapter files
Low Severity
normalizeCommandValue and pushChangedFile are character-for-character identical between ClaudeAdapter.ts and CodexAdapter.ts. Both are newly added in this PR. Extracting them into a shared module would reduce duplication and the risk of future divergence when fixing bugs in one copy but not the other.
Additional Locations (1)
| JSON.stringify(value, (_key, nestedValue) => (nestedValue === undefined ? null : nestedValue)), | ||
| ) as CanonicalJsonValue | null; | ||
| return normalized ?? undefined; | ||
| } |
There was a problem hiding this comment.
toCanonicalJsonValue silently converts null to undefined
Low Severity
toCanonicalJsonValue uses normalized ?? undefined on the final line, which converts a top-level null input (a valid CanonicalJsonValue) into undefined. This causes null detail values to be silently dropped from runtime.warning and context-compaction activity payloads instead of being preserved.
There was a problem hiding this comment.
🟢 Low
When event.payload.detail is null, the runtime warning payload calls toCanonicalJsonValue(null), which returns undefined via null ?? undefined. This causes detail: undefined to be stored, which is then dropped entirely during JSON serialization. Downstream consumers that distinguish between null and a missing detail will see the key disappear instead of receiving null. Consider preserving null values directly rather than normalizing them through toCanonicalJsonValue.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts around line 1693:
When `event.payload.detail` is `null`, the runtime warning payload calls `toCanonicalJsonValue(null)`, which returns `undefined` via `null ?? undefined`. This causes `detail: undefined` to be stored, which is then dropped entirely during JSON serialization. Downstream consumers that distinguish between `null` and a missing `detail` will see the key disappear instead of receiving `null`. Consider preserving `null` values directly rather than normalizing them through `toCanonicalJsonValue`.
Evidence trail:
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts lines 106-116 (toCanonicalJsonValue function), line 298 (usage with event.payload.detail), line 450 (similar usage). The `null ?? undefined` expression at line 115 returns `undefined` when normalized is `null`, and the spread at line 298 creates `{ detail: undefined }` which is dropped during JSON serialization.
1 participant
Summary
Testing
bun fmt,bun lint,bun typecheck, andbun run test.Note
Medium Risk
Medium risk because it changes shared contract schemas and removes multiple runtime event types; any remaining producers/consumers of the pruned events or old payload shapes may now fail validation or break downstream UI expectations.
Overview
Shrinks and hardens the shared event contracts by replacing
OrchestrationThreadActivitywith a typed union (OrchestrationThreadActivityKind) and pruning many unusedProviderRuntimeEventvariants (e.g.session.configured,turn.aborted,hook.*,tool.progress, various account/mcp events). New contract modules (jsonValue,toolLifecycle,threadUsage,userInput) are added and re-exported to standardize payload shapes.Normalizes provider emissions to the new schemas.
ClaudeAdapterandCodexAdapterstop emitting removed events, normalize turnusageintoThreadTokenUsageSnapshot, and emit structuredCanonicalToolLifecycleDatafor tool lifecycle events (including derivedcommandstrings andchangedFiles). Runtime-warning/compaction details are coerced into canonical JSON values.Updates server/web consumers and tests to validate/decode activities via the new schemas (
ProjectionSnapshotQuerynow decodesOrchestrationThreadActivity), adjusts UI logic to read tool metadata from canonicaldata, and types user-input answers asProviderUserInputAnswers.Written by Cursor Bugbot for commit ff581d6. This will update automatically on new commits. Configure here.
Note
Remove unused event types from the provider runtime event union and add structured payload schemas
ProviderRuntimeEventTypeand theProviderRuntimeEventV2union inproviderRuntime.ts, includingsession.configured,turn.aborted,hook.*,tool.progress/summary,auth.status,account.*,mcp.*,model.rerouted,config.warning,deprecation.notice,files.persisted, and allthread.realtime.*events.Schema.Unknownpayload fields with structured schemas:TurnCompletedPayload.usageusesThreadTokenUsageSnapshot,ItemLifecyclePayload.datausesCanonicalToolLifecycleData, and user input answers useProviderUserInputAnswers.toolLifecycle.ts,threadUsage.ts,userInput.ts, andjsonValue.tsto centralize these schemas.datafields using new canonical builder helpers.OrchestrationThreadActivityinorchestration.tsis replaced with a discriminated union keyed by a finite set ofOrchestrationThreadActivityKindliterals, with typed payloads per kind.Macroscope summarized ff581d6.