reduce event union b removing unused members#1379
Open
juliusmarminge wants to merge 6 commits intomainfrom
Open
reduce event union b removing unused members#1379juliusmarminge wants to merge 6 commits intomainfrom
juliusmarminge wants to merge 6 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
![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) { | |||
Contributor
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
Contributor
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),
Contributor
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).
Autofix Details
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, | ||
| }), | ||
| } | ||
| : {}), |
Contributor
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)
Contributor
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.
- 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
- 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
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
t3code/context-window-meter
branch
from
ff581d6 to
e02c60c
Compare
Contributor
There was a problem hiding this comment.
🟢 Low
In stopSessionInternal, the pendingUserInputs map is never cleaned up when a session stops, unlike pendingApprovals which is iterated and resolved. If handleAskUserQuestion is blocked on Deferred.await(answersDeferred) when the session stops, and the abort signal doesn't fire, the deferred never resolves and the waiter hangs indefinitely.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/ClaudeAdapter.ts around line 2331:
In `stopSessionInternal`, the `pendingUserInputs` map is never cleaned up when a session stops, unlike `pendingApprovals` which is iterated and resolved. If `handleAskUserQuestion` is blocked on `Deferred.await(answersDeferred)` when the session stops, and the abort signal doesn't fire, the deferred never resolves and the waiter hangs indefinitely.
Evidence trail:
apps/server/src/provider/Layers/ClaudeAdapter.ts lines 2322-2394 (`stopSessionInternal` function - handles `pendingApprovals` at lines 2331-2350, no handling for `pendingUserInputs`); lines 2459-2550 (`handleAskUserQuestion` - creates deferred at line 2493, stores in `pendingUserInputs` at line 2512, abort handler at lines 2515-2521, awaits deferred at line 2524); line 2450 (creates `pendingUserInputs` map); line 2813 (stores `pendingUserInputs` in context). Commit: REVIEWED_COMMIT
- Update live Claude adapter tests to match the current startup event sequence - Fix stream drain expectations after removing the configured event
Contributor
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).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Schema missing fields for tool started and completed
- Added the missing optional
statusanddatafields toToolStartedOrCompletedActivityPayloadso thattool.startedandtool.completedactivities preserve these fields through schema decoding, matching the existing test expectations andToolUpdatedActivityPayloadshape.
- Added the missing optional
Or push these changes by commenting:
@cursor push 8717fe98f1
Preview (8717fe98f1)
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
@@ -362,7 +362,9 @@
const ToolStartedOrCompletedActivityPayload = Schema.Struct({
itemType: TrimmedNonEmptyString,
+ status: Schema.optional(TrimmedNonEmptyString),
detail: Schema.optional(TrimmedNonEmptyString),
+ data: Schema.optional(CanonicalToolLifecycleData),
});
const ToolUpdatedActivityPayload = Schema.Struct({| const ToolStartedOrCompletedActivityPayload = Schema.Struct({ | ||
| itemType: TrimmedNonEmptyString, | ||
| detail: Schema.optional(TrimmedNonEmptyString), | ||
| }); |
Contributor
There was a problem hiding this comment.
Schema missing fields for tool started and completed
Medium Severity
ToolStartedOrCompletedActivityPayload lacks data and status, while ToolUpdatedActivityPayload includes both. The session-logic.ts toDerivedWorkLogEntry function uses toolLifecyclePayloadFromActivity for all three tool kinds and checks "data" in toolPayload to extract command/changed-file info. Since tool.completed activities can't carry data after schema decode, collapsed work log entries that rely on the completion event for final data (e.g. tool results with changed files) will silently lose that information.
Additional Locations (1)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
bun fmt,bun lint,bun typecheck, andbun run test.Note
High Risk
High risk because it changes shared contract schemas (provider runtime events and
OrchestrationThreadActivity) and removes many event variants, so any producer/consumer still emitting or expecting the old shapes will fail decoding at runtime.Overview
Refactors shared event contracts to be stricter and smaller.
ProviderRuntimeEventdrops many previously-supported event types (e.g.session.configured,turn.aborted,hook.*,tool.*,thread.realtime.*, and several account/model/mcp events) and tightens payload typing (e.g. turn completionusageis now aThreadTokenUsageSnapshot, and item lifecycledatais now structuredCanonicalToolLifecycleData).Makes orchestration thread activities schema-validated and canonical.
OrchestrationThreadActivitybecomes a discriminated union with an explicitOrchestrationThreadActivityKindand per-kind payload schemas; server snapshot hydration now decodes activities via the schema, and runtime ingestion normalizesdetailfields into canonical JSON.Updates adapters and UI to match the new canonical shapes. Claude/Codex adapters now emit canonical tool lifecycle metadata (command strings, changed file lists, generic fallback) and normalized usage snapshots, while web logic and tests are updated to read these typed payloads and use
ProviderUserInputAnswers.Written by Cursor Bugbot for commit 0458f4e. This will update automatically on new commits. Configure here.
Note
Remove unused event types from the provider runtime event union
session.configured,turn.aborted, hook events, auth/account/MCP/model/config/deprecation/files events, andthread.realtime.*events fromProviderRuntimeEventV2in providerRuntime.ts; these will now fail schema validation if produced.command_execution,file_change,generic), thread token usage snapshots, and user input answers in new contract modules, replacingSchema.Unknownfields across turn, task, and item lifecycle payloads.ClaudeAdapterandCodexAdapterto stop emitting removed event types and to populate structuredusageanddatafields using the new canonical shapes.modelUsage, rawdataobjects) will fail runtime schema decoding.Macroscope summarized 0458f4e.