reduce event union b removing unused members#1379
Open
juliusmarminge wants to merge 10 commits intomainfrom
Open
reduce event union b removing unused members#1379juliusmarminge wants to merge 10 commits intomainfrom
juliusmarminge wants to merge 10 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}
apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
Outdated
Show resolved
Hide resolved
![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);
+}
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.
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
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 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({
- 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 <[email protected]>
- Update live Claude adapter tests to match the current startup event sequence - Fix stream drain expectations after removing the configured event
Co-authored-by: codex <[email protected]>
juliusmarminge
force-pushed
the
t3code/context-window-meter
branch
from
0458f4e to
e95b0de
Compare
Contributor
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This is a major schema refactoring that restructures OrchestrationThreadActivity into a discriminated union with 27 typed variants, removes 20+ event types from the provider runtime event union, and introduces new canonical types for tool lifecycle data. Despite being framed as removing unused members, this fundamentally changes shared contracts and warrants careful review of the breaking changes and compatibility implications. You can customize Macroscope's approvability policy. Learn more. |
Co-authored-by: codex <[email protected]>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Ternary branches produce identical activity objects
- Replaced the duplicated ternary branches with a single object literal using destructuring and spread, eliminating the redundancy while preserving discriminated-union type safety.
Or push these changes by commenting:
@cursor push c93fcf98f8
Preview (c93fcf98f8)
diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts
--- a/apps/server/src/ws.ts
+++ b/apps/server/src/ws.ts
@@ -161,33 +161,20 @@
readonly tone: "error";
}
),
- ) =>
- orchestrationEngine.dispatch({
+ ) => {
+ const { threadId, ...rest } = input;
+ return orchestrationEngine.dispatch({
type: "thread.activity.append",
commandId: serverCommandId("setup-script-activity"),
- threadId: input.threadId,
- activity:
- input.kind === "setup-script.failed"
- ? {
- id: EventId.make(crypto.randomUUID()),
- tone: input.tone,
- kind: input.kind,
- summary: input.summary,
- payload: input.payload,
- turnId: null,
- createdAt: input.createdAt,
- }
- : {
- id: EventId.make(crypto.randomUUID()),
- tone: input.tone,
- kind: input.kind,
- summary: input.summary,
- payload: input.payload,
- turnId: null,
- createdAt: input.createdAt,
- },
+ threadId,
+ activity: {
+ id: EventId.make(crypto.randomUUID()),
+ ...rest,
+ turnId: null,
+ },
createdAt: input.createdAt,
});
+ };
const toDispatchCommandError = (cause: unknown, fallbackMessage: string) =>
Schema.is(OrchestrationDispatchCommandError)(cause)You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 58f45db. Configure here.
| payload: input.payload, | ||
| turnId: null, | ||
| createdAt: input.createdAt, | ||
| }, |
Contributor
There was a problem hiding this comment.
Ternary branches produce identical activity objects
Low Severity
The ternary input.kind === "setup-script.failed" ? { ... } : { ... } at the activity field produces two structurally identical object literals — same keys (id, tone, kind, summary, payload, turnId, createdAt) populated from the same input properties. The branching adds complexity with no behavioral difference. The discriminated union input type already provides type safety, so the runtime branch is unnecessary.
Reviewed by Cursor Bugbot for commit 58f45db. Configure here.
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 and removes multiple provider runtime event variants; any producer/consumer still emitting the removed shapes will now fail validation and/or break at runtime. It also changes DI wiring for
ProviderRuntimeIngestionLive, requiring callers to explicitly provideProjectionTurnRepository.Overview
Tightens the contracts and runtime event surface area by pruning unused
ProviderRuntimeEventvariants (e.g.session.configured,turn.aborted, hook/tool/account/realtime events) and turningOrchestrationThreadActivityinto a discriminated union with typed payloads.Canonicalizes runtime payload shapes: introduces new contract modules for
CanonicalJsonValue,ThreadTokenUsageSnapshot,CanonicalToolLifecycleData, and typedProviderUserInputAnswers, then updates Claude/Codex adapters and ingestion to emit/normalize these structured payloads (including safer JSON normalization for warning/compaction details).Propagates the schema changes through server/web: updates snapshot hydration to decode activities via schema, adjusts work-log/session logic to read the new tool lifecycle
datashape (command/file lists), and fixes Layer wiring soProjectionTurnRepositoryLiveis provided by the server/test harnesses instead of implicitly byProviderRuntimeIngestionLive.Written by Cursor Bugbot for commit 4bf483d. This will update automatically on new commits. Configure here.
Note
Remove unused event types from
ProviderRuntimeEventV2union and tighten payload schemasProviderRuntimeEventV2includingsession.configured,turn.aborted,hook.*,tool.progress,tool.summary,auth.status,account.*,mcp.*,model.rerouted,config.warning,deprecation.notice,files.persisted, andthread.realtime.*; adapters no longer emit these events.CanonicalToolLifecycleData(command_execution / file_change / generic),ThreadTokenUsageSnapshot,UserInputQuestion, andProviderUserInputAnswers.datawith akinddiscriminator derived from tool input/result, replacing raw provider payloads.TurnCompletedPayloadnow uses a typedThreadTokenUsageSnapshotforusageand dropsmodelUsage; task progress/completed payloads are similarly normalized.OrchestrationThreadActivityis now a discriminated union with per-kind typed payloads, rejecting arbitrary kinds and payload shapes.Macroscope summarized 58f45db.