reduce event union b removing unused members#1379
Open
juliusmarminge wants to merge 9 commits intomainfrom
Open
reduce event union b removing unused members#1379juliusmarminge wants to merge 9 commits intomainfrom
juliusmarminge wants to merge 9 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);
+}
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).
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 <codex@users.noreply.github.com>
- 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 <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
t3code/context-window-meter
branch
from
0458f4e to
e95b0de
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
Contributor
ApprovabilityVerdict: Needs human review This PR is a significant schema refactoring that removes multiple runtime event types and changes OrchestrationThreadActivity from a loose schema to a strict discriminated union. While framed as 'removing unused members', the changes fundamentally alter what events flow through the system and could break any code consuming the removed events. The scope warrants human review despite the author's strong ownership of these files. You can customize Macroscope's approvability policy. Learn more. |
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 runtime event types from the provider event union
ProviderRuntimeEventV2includingsession.configured,turn.aborted,tool.progress,hook.*,account.*,model.rerouted,thread.realtime.*, and others; validators will now reject these event shapes.CanonicalToolLifecycleDatawithcommand_execution,file_change, andgenerickinds), thread token usage (ThreadTokenUsageSnapshot), and user input answers (ProviderUserInputAnswers) in new contract modules.dataon item lifecycle events using the new canonical shapes instead of forwarding raw payloads.OrchestrationThreadActivityto a discriminated union of specific kind/payload pairs; arbitrary kinds and unknown payloads are no longer valid.ProviderRuntimeIngestionLivelayer no longer implicitly providesProjectionTurnRepository, which must now be supplied by the caller.Macroscope summarized 4bf483d.