refactor: parse every remaining I/O boundary into a domain type (CMP-82) - #151
Merged
Conversation
…validation The agent version manifest is written in one package and read in three. Each reader had its own recordOf helper walking the same JSON column, so the stored shape was described four times and agreed on nowhere. It now lives in packages/validation beside the schemas that were already there, with the parse helper that already existed. Every reader consumes that one definition and the private helpers are gone. Trigger config and the CRM event payload move for the same reason: written by the API, read by the agent. Values are imported by subpath rather than the barrel. A value re-export from index.ts trips noBarrelFile, and the subpath keeps the db and slack schemas out of the client bundle. Rows written before this change still load. Where the old code tolerated bad input it still tolerates it, to the same fallback; where it threw it still throws, with the same message. Each path was checked against the original with padded strings, Infinity, NaN, fractional intervals and arrays. The review-version manifest now crosses tRPC parsed rather than raw. That was forced: reading the property off the generated output type raises TS2589, which is why the client had cast through unknown to reach it. Parsing server-side removes both casts and renders identically.
Every vendor response is now parsed where it arrives, so nothing downstream sees the raw shape. LinkedIn, GitHub, Slack and the Context extract each gain a schema at their fetch, and the private str/int and recordOf helpers that stood in for one are gone. The extract schema was itself a Record<string, unknown>, which is the thing the boundary was supposed to prevent. It is a JsonSchema now, and the team-page payload is parsed by its owner - the caller supplying the schema is the only code that knows the shape. Prisma Json columns are read as Prisma.JsonValue and parsed with a named schema rather than walked with typeof. Behaviour is unchanged. Every vendor schema catches at each level, so a malformed response still degrades to null or an empty list exactly as the typeof guards did; docs/agent.md requires a missing key to remove a capability and never throw. Where the old code threw it still throws, on the same condition and with the same message. Error helpers keep unknown and are renamed to cause. A catch binding is unknown by language rule and cannot be schema'd; cause is the documented exemption and error-formatter.ts already set that precedent. HOST_ALIASES became a Map, which closes a latent prototype-key hole: http://constructor/ would have stringified Object.prototype.constructor into a canonical value.
The transcript in the app and the conversation service in the API read the same eve data - stream events, message parts, tool input and output - and each walked it with its own recordOf helper. The shapes now live in packages/validation beside the manifest, and both consume them. Degradation is preserved exactly: every schema catches, so a malformed part is still ignored rather than throwing, which docs/agent.md requires of a panel read. Empty-string codes and reasons still take the same paths they did.
…he API The exchange-rate feed, the model catalogue, the SSO oidcConfig string column, stored recipients and the browser tracking payload are each parsed where they arrive. Per-row safeParse keeps the old degradation: one bad model or recipient is dropped, not the whole response, and the rates fetcher still returns null and warns rather than throwing, which docs/currency.md depends on. Activity.meta gets a schema in packages/validation because the API, the agent and the app all read that column. The SORTABLE dictionaries in five services become one named contract. translate now throws and returns never instead of returning unknown - every call site already threw its result, so the thrown value, message and status are unchanged. Catch bindings keep unknown and are renamed to cause. TypeScript types them that way by language rule and narrowing one needs a cast.
The channel and audit hook still carried their own copies of the recordOf helper the lib pass deleted. Route bodies, receive targets and eve event data are parsed where they arrive, reusing the eve schemas rather than restating them. The settle paths are byte-identical. docs/agent.md records a token-prefix mistake that silently stopped every task reaching finishedAt, so taskFromToken and the channel handlers were left structurally untouched and are still covered by crm-token and drain. One cast survives in the audit hook, narrowed from object to Prisma.InputJsonObject, and a second one at the event write is gone. It cannot become a parse: eve sets details and error to undefined on the failure events, z.json rejects an undefined property value, so a whole blob parse would catch and silently blank the audit trail for exactly those events.
… casting The components hand-wrote types that duplicated the server's return shape and cast tRPC output into them. The capabilities type had already drifted - it declared dataScope nullable where the success branch always returns it. They are inferred from RouterOutputs now, which deleted the casts with them. Naming a Json column through inferRouterOutputs is a hard TS2589: Serialize maps over Prisma's recursive JsonValue and blows the instantiation limit. That, not drift, is what the double casts were working around. Reading the whole row into a parse is fine, so every Json column here is parsed at the boundary instead. No markup, className or rendered output changed.
…maps The remaining widenings and empty-object spreads sat in the log and custom-field modules. Those maps stay open by design, so only the ordinary findings are fixed: named contracts where inference was being discarded, direct conditional properties where an object was assembled at runtime. The prisma log bridge mattered beyond lint. Its fields map is rendered with inspect in dev, so an explicit undefined would have printed. all-exceptions.filter gains an ErrorBody interface with an index signature, which keeps the body open for whatever Nest's own exception response carried and removed a pre-existing cast on the way. Three areas are scoped off rather than converted, each for a stated reason: the telemetry property bag, the log field map and the user-defined custom field values have no fixed shape to parse into. Skills vendored under .agents are third-party and now ignored, matching what biome already does.
|
@ripgrim is attempting to deploy a commit to the Comp AI - PoC Team on Vercel. A member of the Team first needs to authorize it. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
24 issues found across 177 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/validation/src/agent-manifest.ts">
<violation number="1" location="packages/validation/src/agent-manifest.ts:136">
P2: When persisted trigger config contains a fractional interval such as `1.5`, `readAgentTriggerConfig` accepts it and `queueDueAgentRuns` schedules using fractional-minute intervals. Require integer minutes here to preserve the manifest and builder contract.</violation>
<violation number="2" location="packages/validation/src/agent-manifest.ts:154">
P3: `readAgentTriggerConfig` returns the shared module-level `UNREADABLE_TRIGGER_CONFIG` object by reference on every unreadable input, and `readAgentManifestSummary` returns `UNREADABLE_MANIFEST_SUMMARY` the same way. Callers receive the same mutable instance; if any consumer mutates an `event`/`intervalMinutes` field on an unreadable result (e.g. to patch a degraded value), it corrupts the fallback for every subsequent call. No current caller mutates it, but sharing a single mutable fallback across calls is a latent hazard. Return a freshly allocated fallback each time instead.</violation>
</file>
<file name="apps/api/src/settings/model-catalog.service.ts">
<violation number="1" location="apps/api/src/settings/model-catalog.service.ts:102">
P2: When the gateway returns a 200 JSON `null`, this fallback converts an unavailable response into an empty successful catalog that is cached for 30 minutes. Preserve the top-level-null error path so the caller keeps the unavailable state and retries instead of caching `[]`.</violation>
</file>
<file name="packages/validation/src/slack.ts">
<violation number="1" location="packages/validation/src/slack.ts:35">
P2: When Slack returns a whitespace-only token or profile field, `present` accepts it because it checks length before trimming. Trim before `min(1)` so invalid credentials and profile emails do not reach the OAuth flow.</violation>
</file>
<file name="apps/agent/agent/lib/builder-runtime.ts">
<violation number="1" location="apps/agent/agent/lib/builder-runtime.ts:24">
P3: `taggedResource` duplicates the exact shape of `agentManifestResource`, so future field changes can make builder parsing diverge from the manifest contract. Reuse the shared schema and apply `.nullable().catch(null)` locally.</violation>
</file>
<file name="apps/app/lib/onboarding.ts">
<violation number="1" location="apps/app/lib/onboarding.ts:64">
P2: When the API returns a truthy non-boolean `canRename`, this parser now settles the workspace instead of requiring onboarding. Preserve the previous malformed-response behavior or return `unknown` for invalid permission data, otherwise a malformed response can bypass the onboarding gate.</violation>
</file>
<file name="apps/agent/agent/lib/socials.ts">
<violation number="1" location="apps/agent/agent/lib/socials.ts:21">
P1: When GitHub returns a 2xx JSON value such as `null`, this catch fabricates an account with no profile data. `fetchGithubUser` then defaults it to `User`, so `verifyGithub` can accept the handle alone and write a false GitHub URL. Remove the object-level catch; field-level catches already preserve malformed-field fallback, while non-object payloads must remain rejected.</violation>
</file>
<file name="packages/validation/src/builder-question.ts">
<violation number="1" location="packages/validation/src/builder-question.ts:18">
P2: When a persisted question has a whitespace-only `prompt`, this schema accepts it and the API returns an active question with no visible prompt. Trim the prompt before applying the non-empty check so malformed stored requests are rejected consistently with `agents.inputRequest`.</violation>
</file>
<file name="packages/validation/package.json">
<violation number="1" location="packages/validation/package.json:8">
P3: The two shape modules src/slack.ts and src/agents.ts have no subpath export, so consumers can reach them only through the root `@crm/validation` barrel in index.ts. That conflicts with the stated per-shape subpath convention ("Imported by subpath, not barrel") and leaves Slack OAuth, which the PR lists as centralized, without the same access path as the other modules. Add `./agents` and `./slack` to the exports map for consistency.</violation>
</file>
<file name="apps/agent/agent/lib/linkdapi.ts">
<violation number="1" location="apps/agent/agent/lib/linkdapi.ts:210">
P2: When the vendor returns valid JSON `null`, `envelope.parse` converts it into a missing result instead of an error. Callers now report “No such profile” and suppress the malformed-response/API-error path; reject `null` before parsing or otherwise preserve the non-missing error outcome.</violation>
</file>
<file name="apps/agent/agent/channels/crm.ts">
<violation number="1" location="apps/agent/agent/channels/crm.ts:272">
P2: When `data.message` is non-string or `null`, this parser discards it and reports the generic failure instead of preserving the previous `String(data.message)` behavior. Preserve the prior coercion while normalizing the failure payload.</violation>
</file>
<file name="apps/agent/agent/hooks/telemetry.ts">
<violation number="1" location="apps/agent/agent/hooks/telemetry.ts:11">
P3: This change re-declares two things already defined in apps/agent/agent/lib/session-purpose.ts: the session attributes type (`SessionAttributes = Readonly<Record<string, string | readonly string[]>>`) and the identical `attributeText = z.string().trim().min(1).nullable().catch(null)` schema. This contradicts the PR's stated goal of a single source of truth for session shapes. Extract the schema and attribute type into a shared module (or reuse session-purpose's) and import them here instead of defining local copies, so a shape change is made once.</violation>
</file>
<file name="apps/app/components/agent-builder/team-agent-detail.tsx">
<violation number="1" location="apps/app/components/agent-builder/team-agent-detail.tsx:526">
P3: After this change `capabilities` is typed directly from `RouterOutputs["agents"]["byId"]`, and `byId` always returns a non-optional object from `readCapabilities(...)` (even for unreadable or missing manifests). So the `if (!capabilities) { ... "This agent has no deployed version yet." ... }` block below is now unreachable dead code: capabilities is never falsy, so that message can never render. The draft/no-version state is already gated separately by `deployed = agent.currentVersion !== null`. Consider dropping the dead guard so the component only relies on the real `deployed` check.</violation>
</file>
<file name="apps/agent/agent/hooks/activity.ts">
<violation number="1" location="apps/agent/agent/hooks/activity.ts:24">
P2: The refactor removed the `typeof input !== "object" || input === null` guard that previously protected the `Object.entries(input)` loop. `preview` now relies solely on the compile-time `ActionInput` type. This hook reads raw event data directly (unlike agent-builder-state, which parses through the zod schema), so a malformed action whose `input` is null/undefined or a primitive at runtime will throw in `Object.entries` instead of returning "", contradicting the PR's stated goal that malformed inputs still degrade the same way. An exception here would propagate out of the `actions.requested` hook handler and can break the session it is only meant to log.</violation>
</file>
<file name="apps/app/lib/agent-transcript.ts">
<violation number="1" location="apps/app/lib/agent-transcript.ts:288">
P1: When a malformed dynamic-tool event has a non-string `toolName`, this returns it unchanged and `humanise` later throws on `.replace`. Convert or validate the name before returning it.</violation>
<violation number="2" location="apps/app/lib/agent-transcript.ts:403">
P1: When a tool event contains a non-string `errorText`, `errorTextOf` throws while rendering the transcript. Keep the `typeof text === "string"` guard before calling `.trim()`.</violation>
</file>
<file name=".oxlintrc.json">
<violation number="1" location=".oxlintrc.json:48">
P3: This disables `no-unknown-parameters` for every future module added under packages/validation/src, not just the files that legitimately decode `unknown` at their entry points. A new non-boundary function that accepts `unknown` anywhere in the package will silently pass lint, contradicting the boundary-decoder contract the package exists to enforce. Narrow the override to the specific files that parse `unknown` (agent-manifest.ts, and index.ts's `parse`) instead.</violation>
</file>
<file name="apps/api/src/currency/rates.service.ts">
<violation number="1" location="apps/api/src/currency/rates.service.ts:195">
P3: When the provider returns a non-object JSON body (array/string/null), the outer `.catch(UNREADABLE_FEED)` maps it to result "", so it is logged as "provider refused the request" with a null error-type instead of the previous "Exchange rates unavailable" path. Every field already has its own .catch default and zod fires those on missing keys, so the outer catch only ever fires for non-object bodies. Consider distinguishing a malformed body from a provider refusal to keep the warning meaningful.</violation>
</file>
<file name="apps/app/app/(landing)/grant-access/grant-access.tsx">
<violation number="1" location="apps/app/app/(landing)/grant-access/grant-access.tsx:18">
P3: ProviderGrant (grant-access.tsx) and ProviderChoice (social-sign-in.tsx) declare the same `{ label: string; Logo: FC<SVGProps<SVGSVGElement>> }` shape and both import GoogleLogo/MicrosoftLogo, in the same two landing pages. Extract one shared provider-logo shape (e.g. in the UI package) and reuse it in both registries so the brand-logo contract has a single source of truth.</violation>
</file>
<file name="packages/validation/src/eve-tool.ts">
<violation number="1" location="packages/validation/src/eve-tool.ts:21">
P3: The `link` schema only anchors the regex at the start (`/^https?:\/\//`), and since Zod's `.regex()` uses a partial `test()` match, strings are accepted with no hostname or path validation. Values like `"https://"` or `"https://garbage"` pass and surface as clickable source links in the transcript (`sourcesOf`/`hostOf` render them as-is when `new URL()` throws). Accept only strings that form a valid absolute URL, or keep the prefix check but validate the remainder as a host path.</violation>
</file>
<file name="packages/db/src/json.ts">
<violation number="1" location="packages/db/src/json.ts:19">
P3: Newly added `isJsonText` detects strings via `String(value) === value` instead of `typeof value === "string"`. It is functionally correct for all JsonValue variants (only a string primitive makes `String(value) === value` true), but the coercion/equality trick is non-obvious and easy to regress. Use `typeof value === "string"` for a clearer, equivalent guard that also makes the narrowing intent explicit.</violation>
</file>
<file name="apps/api/src/tracking/tracking.controller.ts">
<violation number="1" location="apps/api/src/tracking/tracking.controller.ts:49">
P3: The `.catch({ body: null })` on `trackingRequest` never triggers because the inner `parsedBody` already ends with `.catch(null)`, so `z.object({ body: parsedBody })` always parses successfully (the request is always an object). This redundant outer fallback is dead code that obscures the schema; simplify it to `z.object({ body: parsedBody })`.</violation>
</file>
<file name="apps/api/src/dashboard/dashboard.service.ts">
<violation number="1" location="apps/api/src/dashboard/dashboard.service.ts:286">
P2: When an activity row's meta is stored as non-object JSON (e.g. a JSON array), this previously passed the value through via the cast, but activityMeta.parse (a z.record(...).nullable().catch(null)) rejects non-objects and silently returns null, dropping the data. This diverges from the prior passthrough cast and from the PR's claim that degradation is preserved. Common object-shaped meta is unaffected.</violation>
</file>
<file name="apps/api/src/activities/activities.service.ts">
<violation number="1" location="apps/api/src/activities/activities.service.ts:278">
P3: When a stored activity `meta` value is a JSON array or scalar (not an object), `activityMeta.parse(entry.meta)` fails and `.catch(null)` replaces it with `null` in the serialized response. The previous line was only a type cast (`entry.meta as Record<string, unknown> | null`) and passed such values through unchanged. If no code path stores arrays/scalars under `meta` this is moot, but the parse introduces a silent data-nullification that the prior guard did not have; confirm activity.meta is always an object or null before relying on it.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Comment on lines
+21
to
+29
| }) | ||
| .catch({ | ||
| login: null, | ||
| name: null, | ||
| company: null, | ||
| blog: null, | ||
| bio: null, | ||
| type: null, | ||
| }); |
Contributor
There was a problem hiding this comment.
P1: When GitHub returns a 2xx JSON value such as null, this catch fabricates an account with no profile data. fetchGithubUser then defaults it to User, so verifyGithub can accept the handle alone and write a false GitHub URL. Remove the object-level catch; field-level catches already preserve malformed-field fallback, while non-object payloads must remain rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/agent/agent/lib/socials.ts, line 21:
<comment>When GitHub returns a 2xx JSON value such as `null`, this catch fabricates an account with no profile data. `fetchGithubUser` then defaults it to `User`, so `verifyGithub` can accept the handle alone and write a false GitHub URL. Remove the object-level catch; field-level catches already preserve malformed-field fallback, while non-object payloads must remain rejected.</comment>
<file context>
@@ -6,6 +7,27 @@ import {
+ blog: text,
+ bio: text,
+ type: rawText,
+ })
+ .catch({
+ login: null,
</file context>
Suggested change
| }) | |
| .catch({ | |
| login: null, | |
| name: null, | |
| company: null, | |
| blog: null, | |
| bio: null, | |
| type: null, | |
| }); | |
| }); |
| if (part.type === "dynamic-tool" && "toolName" in part) { | ||
| return String(part.toolName); | ||
| } | ||
| if (part.type === "dynamic-tool") return part.toolName; |
Contributor
There was a problem hiding this comment.
P1: When a malformed dynamic-tool event has a non-string toolName, this returns it unchanged and humanise later throws on .replace. Convert or validate the name before returning it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/lib/agent-transcript.ts, line 288:
<comment>When a malformed dynamic-tool event has a non-string `toolName`, this returns it unchanged and `humanise` later throws on `.replace`. Convert or validate the name before returning it.</comment>
<file context>
@@ -268,18 +279,13 @@ function partId(
- if (part.type === "dynamic-tool" && "toolName" in part) {
- return String(part.toolName);
- }
+ if (part.type === "dynamic-tool") return part.toolName;
return part.type.replace(/^tool-/, "");
}
</file context>
Suggested change
| if (part.type === "dynamic-tool") return part.toolName; | |
| \tif (part.type === "dynamic-tool") return String(part.toolName); |
| return typeof value === "string" && value ? value : null; | ||
| function errorTextOf(part: EveMessagePart): string | null { | ||
| const text = "errorText" in part ? part.errorText : undefined; | ||
| return text?.trim() ? text : null; |
Contributor
There was a problem hiding this comment.
P1: When a tool event contains a non-string errorText, errorTextOf throws while rendering the transcript. Keep the typeof text === "string" guard before calling .trim().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/lib/agent-transcript.ts, line 403:
<comment>When a tool event contains a non-string `errorText`, `errorTextOf` throws while rendering the transcript. Keep the `typeof text === "string"` guard before calling `.trim()`.</comment>
<file context>
@@ -380,32 +382,25 @@ export function latestTurnFailure(
- return typeof value === "string" && value ? value : null;
+function errorTextOf(part: EveMessagePart): string | null {
+ const text = "errorText" in part ? part.errorText : undefined;
+ return text?.trim() ? text : null;
}
</file context>
Suggested change
| return text?.trim() ? text : null; | |
| \treturn typeof text === "string" && text.trim() ? text : null; |
|
|
||
| export const agentTriggerConfig = z.object({ | ||
| intervalMinutes: z | ||
| .number() |
Contributor
There was a problem hiding this comment.
P2: When persisted trigger config contains a fractional interval such as 1.5, readAgentTriggerConfig accepts it and queueDueAgentRuns schedules using fractional-minute intervals. Require integer minutes here to preserve the manifest and builder contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/validation/src/agent-manifest.ts, line 136:
<comment>When persisted trigger config contains a fractional interval such as `1.5`, `readAgentTriggerConfig` accepts it and `queueDueAgentRuns` schedules using fractional-minute intervals. Require integer minutes here to preserve the manifest and builder contract.</comment>
<file context>
@@ -109,3 +130,56 @@ export function parseAgentManifest(value: unknown): AgentManifest {
+
+export const agentTriggerConfig = z.object({
+ intervalMinutes: z
+ .number()
+ .min(AGENT_TRIGGER_INTERVAL_MINUTES.min)
+ .transform((minutes) =>
</file context>
Suggested change
| .number() | |
| \t\t.number().int() |
| contextWindowTokens: model.context_window as number, | ||
| pricing: input !== null && output !== null ? { input, output } : null, | ||
| }; | ||
| const body = gatewayCatalog.parse(await response.json()); |
Contributor
There was a problem hiding this comment.
P2: When the gateway returns a 200 JSON null, this fallback converts an unavailable response into an empty successful catalog that is cached for 30 minutes. Preserve the top-level-null error path so the caller keeps the unavailable state and retries instead of caching [].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/settings/model-catalog.service.ts, line 102:
<comment>When the gateway returns a 200 JSON `null`, this fallback converts an unavailable response into an empty successful catalog that is cached for 30 minutes. Preserve the top-level-null error path so the caller keeps the unavailable state and retries instead of caching `[]`.</comment>
<file context>
@@ -80,26 +99,13 @@ export class ModelCatalogService {
- contextWindowTokens: model.context_window as number,
- pricing: input !== null && output !== null ? { input, output } : null,
- };
+ const body = gatewayCatalog.parse(await response.json());
+
+ const models = body.data.flatMap((entry) => {
</file context>
Suggested change
| const body = gatewayCatalog.parse(await response.json()); | |
| \t\t\tconst raw = await response.json(); | |
| \t\t\tif (raw === null) throw new Error("Invalid model catalog response"); | |
| \t\t\tconst body = gatewayCatalog.parse(raw); |
|
|
||
| const flag = z.boolean().nullable().catch(null); | ||
|
|
||
| const link = z |
Contributor
There was a problem hiding this comment.
P3: The link schema only anchors the regex at the start (/^https?:\/\//), and since Zod's .regex() uses a partial test() match, strings are accepted with no hostname or path validation. Values like "https://" or "https://garbage" pass and surface as clickable source links in the transcript (sourcesOf/hostOf render them as-is when new URL() throws). Accept only strings that form a valid absolute URL, or keep the prefix check but validate the remainder as a host path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/validation/src/eve-tool.ts, line 21:
<comment>The `link` schema only anchors the regex at the start (`/^https?:\/\//`), and since Zod's `.regex()` uses a partial `test()` match, strings are accepted with no hostname or path validation. Values like `"https://"` or `"https://garbage"` pass and surface as clickable source links in the transcript (`sourcesOf`/`hostOf` render them as-is when `new URL()` throws). Accept only strings that form a valid absolute URL, or keep the prefix check but validate the remainder as a host path.</comment>
<file context>
@@ -0,0 +1,40 @@
+
+const flag = z.boolean().nullable().catch(null);
+
+const link = z
+ .string()
+ .regex(/^https?:\/\//)
</file context>
| return value instanceof Object && !Array.isArray(value); | ||
| } | ||
|
|
||
| function isJsonText(value: JsonValue | undefined): value is string { |
Contributor
There was a problem hiding this comment.
P3: Newly added isJsonText detects strings via String(value) === value instead of typeof value === "string". It is functionally correct for all JsonValue variants (only a string primitive makes String(value) === value true), but the coercion/equality trick is non-obvious and easy to regress. Use typeof value === "string" for a clearer, equivalent guard that also makes the narrowing intent explicit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/src/json.ts, line 19:
<comment>Newly added `isJsonText` detects strings via `String(value) === value` instead of `typeof value === "string"`. It is functionally correct for all JsonValue variants (only a string primitive makes `String(value) === value` true), but the coercion/equality trick is non-obvious and easy to regress. Use `typeof value === "string"` for a clearer, equivalent guard that also makes the narrowing intent explicit.</comment>
<file context>
@@ -1,3 +1,25 @@
+ return value instanceof Object && !Array.isArray(value);
+}
+
+function isJsonText(value: JsonValue | undefined): value is string {
+ return String(value) === value;
+}
</file context>
Suggested change
| function isJsonText(value: JsonValue | undefined): value is string { | |
| function isJsonText(value: JsonValue | undefined): value is string { | |
| return typeof value === "string"; | |
| } |
| .nullable() | ||
| .catch(null); | ||
|
|
||
| const trackingRequest = z.object({ body: parsedBody }).catch({ body: null }); |
Contributor
There was a problem hiding this comment.
P3: The .catch({ body: null }) on trackingRequest never triggers because the inner parsedBody already ends with .catch(null), so z.object({ body: parsedBody }) always parses successfully (the request is always an object). This redundant outer fallback is dead code that obscures the schema; simplify it to z.object({ body: parsedBody }).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/tracking/tracking.controller.ts, line 49:
<comment>The `.catch({ body: null })` on `trackingRequest` never triggers because the inner `parsedBody` already ends with `.catch(null)`, so `z.object({ body: parsedBody })` always parses successfully (the request is always an object). This redundant outer fallback is dead code that obscures the schema; simplify it to `z.object({ body: parsedBody })`.</comment>
<file context>
@@ -35,6 +36,18 @@ const SWEEP_BATCH = 10_000;
+ .nullable()
+ .catch(null);
+
+const trackingRequest = z.object({ body: parsedBody }).catch({ body: null });
+
@Controller("api/t")
</file context>
Suggested change
| const trackingRequest = z.object({ body: parsedBody }).catch({ body: null }); | |
| const trackingRequest = z.object({ body: parsedBody }); |
| completedAt: entry.completedAt?.toISOString() ?? null, | ||
| createdAt: entry.createdAt.toISOString(), | ||
| meta: entry.meta as Record<string, unknown> | null, | ||
| meta: activityMeta.parse(entry.meta), |
Contributor
There was a problem hiding this comment.
P3: When a stored activity meta value is a JSON array or scalar (not an object), activityMeta.parse(entry.meta) fails and .catch(null) replaces it with null in the serialized response. The previous line was only a type cast (entry.meta as Record<string, unknown> | null) and passed such values through unchanged. If no code path stores arrays/scalars under meta this is moot, but the parse introduces a silent data-nullification that the prior guard did not have; confirm activity.meta is always an object or null before relying on it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/activities/activities.service.ts, line 278:
<comment>When a stored activity `meta` value is a JSON array or scalar (not an object), `activityMeta.parse(entry.meta)` fails and `.catch(null)` replaces it with `null` in the serialized response. The previous line was only a type cast (`entry.meta as Record<string, unknown> | null`) and passed such values through unchanged. If no code path stores arrays/scalars under `meta` this is moot, but the parse introduces a silent data-nullification that the prior guard did not have; confirm activity.meta is always an object or null before relying on it.</comment>
<file context>
@@ -273,7 +275,7 @@ function serializeEntry(entry: Entry) {
completedAt: entry.completedAt?.toISOString() ?? null,
createdAt: entry.createdAt.toISOString(),
- meta: entry.meta as Record<string, unknown> | null,
+ meta: activityMeta.parse(entry.meta),
emailThread: entry.emailThread
</file context>
|
|
||
| export function readAgentTriggerConfig(value: unknown): AgentTriggerConfig { | ||
| const parsed = agentTriggerConfig.safeParse(value); | ||
| return parsed.success ? parsed.data : UNREADABLE_TRIGGER_CONFIG; |
Contributor
There was a problem hiding this comment.
P3: readAgentTriggerConfig returns the shared module-level UNREADABLE_TRIGGER_CONFIG object by reference on every unreadable input, and readAgentManifestSummary returns UNREADABLE_MANIFEST_SUMMARY the same way. Callers receive the same mutable instance; if any consumer mutates an event/intervalMinutes field on an unreadable result (e.g. to patch a degraded value), it corrupts the fallback for every subsequent call. No current caller mutates it, but sharing a single mutable fallback across calls is a latent hazard. Return a freshly allocated fallback each time instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/validation/src/agent-manifest.ts, line 154:
<comment>`readAgentTriggerConfig` returns the shared module-level `UNREADABLE_TRIGGER_CONFIG` object by reference on every unreadable input, and `readAgentManifestSummary` returns `UNREADABLE_MANIFEST_SUMMARY` the same way. Callers receive the same mutable instance; if any consumer mutates an `event`/`intervalMinutes` field on an unreadable result (e.g. to patch a degraded value), it corrupts the fallback for every subsequent call. No current caller mutates it, but sharing a single mutable fallback across calls is a latent hazard. Return a freshly allocated fallback each time instead.</comment>
<file context>
@@ -109,3 +130,56 @@ export function parseAgentManifest(value: unknown): AgentManifest {
+
+export function readAgentTriggerConfig(value: unknown): AgentTriggerConfig {
+ const parsed = agentTriggerConfig.safeParse(value);
+ return parsed.success ? parsed.data : UNREADABLE_TRIGGER_CONFIG;
+}
+
</file context>
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.
anti-slop: 548 → 0.
bun run lint:slopnow exits clean.What this is
Every
Record<string, unknown>, everytypeofcheck standing in for a parser, everyunknownparameter with no contract, and everyas unknown asaround a JSON column — replaced by a domain type parsed once, where the data arrives.@crm/validationapps/agent/agent/libapps/apiapps/agent(channels, hooks, tools)apps/appcomponents and routespackages/*The shape of the fix
Nine schema modules now live in
packages/validation/src/, one per shape, imported by subpath: the agent manifest, CRM event payloads, eve stream events, eve tool input/output, builder questions, activity meta, Slack OAuth. Anything read by more than one package is defined exactly once — previously the manifest alone was described four different ways in three apps.Vendor responses are parsed at the
fetch: LinkedIn, GitHub, Slack, the Context extract, the exchange-rate feed, the model catalogue. Nothing downstream ever sees a raw payload. PrismaJsoncolumns are read asPrisma.JsonValueand parsed with a named schema. The app's components infer fromRouterOutputsinstead of re-declaring the server's shapes — one of which had already silently drifted.Degradation is preserved, deliberately
Every vendor and column schema
.catch()es at each level, so malformed input still degrades exactly as the oldtypeofguards did.docs/agent.mdrequires that a missing key removes a capability and never throws;docs/currency.mddepends on the rates fetcher returningnulland warning. Both still hold. Where the old code threw, it throws on the same condition with the same message.Three findings turned out to matter beyond lint:
HOST_ALIASESbecame aMap, closing a prototype-key hole —http://constructor/would have stringifiedObject.prototype.constructorinto a canonical value.inspectin dev, so an explicitundefinedwould have printed.all-exceptions.filterlost a pre-existing cast on the way to a namedErrorBody.What is scoped off rather than converted, and why
Three maps are genuinely open and have no fixed shape to parse into. Each override in
.oxlintrc.jsonis a decision with a reason, not a silenced warning:docs/telemetry.mdtreats them as openpackages/validation's own parser entry points, which takeunknownbecause that is what a parser is, and vendored skills under.agents/, matching what Biome already ignoresVerification
bun run check-types13/13 ·bun run lint9/9 ·bun run lint:slop0apps/agent313 ·apps/app147 ·packages/db115 ·packages/auth43 ·packages/telemetry61 ·packages/env17 ·packages/validation5 — all 0 failapps/api313 pass across 33 specs, run file by fileas unknown asadded anywhere in the branch. No code comments added. NoclassNamechanged inapps/app, so rendered output is provably untouched.Two pre-existing issues confirmed but not fixed here, since neither is caused by this work:
apps/api/test/bulk.spec.tsandtest/fields.spec.tshang on.rejects.toThrow(verified by stashing and reproducing atHEAD) — that is also why the whole-suite run never terminates locally and why these PRs go up with--no-verify. Andtest/auth.e2e.spec.tsflaked once in a sequential loop but passes in isolation.🤖 Generated with Claude Code