refactor: give cross-package boundary shapes one home in @crm/validation (CMP-82) - #148
refactor: give cross-package boundary shapes one home in @crm/validation (CMP-82)#148ripgrim wants to merge 1 commit into
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.
|
@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. |
There was a problem hiding this comment.
2 issues found across 17 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="apps/agent/agent/lib/custom-agent-dispatch.ts">
<violation number="1" location="apps/agent/agent/lib/custom-agent-dispatch.ts:283">
P3: When a queued event task's `data` is not a JSON object (array, string, or null), `crmEventTask.safeParse` still succeeds because `data` uses `z.record(...).catch({})`, so the run is queued with `data` silently replaced by `{}` instead of surfacing the malformed payload. Since `data` is persisted into `agentRun.input`, malformed event data is lost without any error signal. If dropping the data is not intended, validate `data` explicitly (or drop the `.catch` on a non-optional data field) before writing the run input.</violation>
</file>
<file name="apps/api/src/conversations/conversations.service.ts">
<violation number="1" location="apps/api/src/conversations/conversations.service.ts:400">
P2: The `builderById`/`byId` responses now replace the stored `manifest` JSON (which contained `resources`, `dataScope.mode`, action `provider`/`activityTypes`, and trigger `config` objects) with the reduced `AgentManifestSummary` object. All in-repo consumers were updated to match, so nothing breaks today, but this is a wire-format contract change: any consumer outside this PR that reads `createdVersions[].manifest` or `reviewVersion.manifest` for more than `name`/`description`/`summary` will silently receive a stripped object instead of the raw manifest. Consider noting this as a produced-for-client-only representation (or returning the full manifest server-side and letting clients parse the shared summary) so the on-the-wire shape change is explicit rather than implicit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| : null, | ||
| createdVersions: row.createdVersions.map((version) => ({ | ||
| ...version, | ||
| manifest: readAgentManifestSummary(version.manifest), |
There was a problem hiding this comment.
P2: The builderById/byId responses now replace the stored manifest JSON (which contained resources, dataScope.mode, action provider/activityTypes, and trigger config objects) with the reduced AgentManifestSummary object. All in-repo consumers were updated to match, so nothing breaks today, but this is a wire-format contract change: any consumer outside this PR that reads createdVersions[].manifest or reviewVersion.manifest for more than name/description/summary will silently receive a stripped object instead of the raw manifest. Consider noting this as a produced-for-client-only representation (or returning the full manifest server-side and letting clients parse the shared summary) so the on-the-wire shape change is explicit rather than implicit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/conversations/conversations.service.ts, line 400:
<comment>The `builderById`/`byId` responses now replace the stored `manifest` JSON (which contained `resources`, `dataScope.mode`, action `provider`/`activityTypes`, and trigger `config` objects) with the reduced `AgentManifestSummary` object. All in-repo consumers were updated to match, so nothing breaks today, but this is a wire-format contract change: any consumer outside this PR that reads `createdVersions[].manifest` or `reviewVersion.manifest` for more than `name`/`description`/`summary` will silently receive a stripped object instead of the raw manifest. Consider noting this as a produced-for-client-only representation (or returning the full manifest server-side and letting clients parse the shared summary) so the on-the-wire shape change is explicit rather than implicit.</comment>
<file context>
@@ -396,6 +397,7 @@ export class ConversationsService {
: null,
createdVersions: row.createdVersions.map((version) => ({
...version,
+ manifest: readAgentManifestSummary(version.manifest),
createdAt: version.createdAt.toISOString(),
})),
</file context>
| throw new Error("The queued agent event is invalid."); | ||
| } | ||
|
|
||
| const { type: eventType, occurredAt, data } = parsed.data; |
There was a problem hiding this comment.
P3: When a queued event task's data is not a JSON object (array, string, or null), crmEventTask.safeParse still succeeds because data uses z.record(...).catch({}), so the run is queued with data silently replaced by {} instead of surfacing the malformed payload. Since data is persisted into agentRun.input, malformed event data is lost without any error signal. If dropping the data is not intended, validate data explicitly (or drop the .catch on a non-optional data field) before writing the run input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/agent/agent/lib/custom-agent-dispatch.ts, line 283:
<comment>When a queued event task's `data` is not a JSON object (array, string, or null), `crmEventTask.safeParse` still succeeds because `data` uses `z.record(...).catch({})`, so the run is queued with `data` silently replaced by `{}` instead of surfacing the malformed payload. Since `data` is persisted into `agentRun.input`, malformed event data is lost without any error signal. If dropping the data is not intended, validate `data` explicitly (or drop the `.catch` on a non-optional data field) before writing the run input.</comment>
<file context>
@@ -271,29 +275,22 @@ export async function queueEventAgentRuns(
+ throw new Error("The queued agent event is invalid.");
+ }
+
+ const { type: eventType, occurredAt, data } = parsed.data;
+ const recordKind = CRM_EVENT_CATALOG[eventType].recordKind;
+ const recordId = parsed.data.record.id;
</file context>
First of a sequence clearing the anti-slop backlog. This one is foundation only — it exists so the module passes that follow have one definition to consume instead of inventing their own.
548 → 524. The count is modest by design; the value here is that four descriptions of the same column became one.
The problem
AgentVersion.manifestis written inapps/agentand read in three packages. Every reader had its own privaterecordOfhelper walking the same JSON, so the stored shape was described four times and agreed on nowhere.AgentTrigger.configand the CRM event payload had the same split — written by the API, read by the agent.What moved
apps/agent/agent/lib/agent-manifest.ts→packages/validation/src/agent-manifest.ts, beside the schemas already there and using theparse()helper that already existed. Newagent-events.tscovers the CRM event payload, deriving the record kind from@crm/db/crm-eventsrather than restating it.Every reader now consumes those. The private helpers are deleted — there is no second way to read a manifest.
Values are imported by subpath (
@crm/validation/agent-manifest) rather than the barrel: a value re-export fromindex.tstripsnoBarrelFile, and the subpath keeps the db and slack schemas out of the client bundle.Existing rows 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.
readAgentTriggerConfigremains byte-identical to the oldintervalOf— bad interval → 1440, clamped to 525600, invalid event → null. Each path was checked against the original with padded strings,Infinity,NaN, fractional intervals and arrays.One contract change worth reviewing
agents.byId.reviewVersion.manifestandconversations.builderById.createdVersions[].manifestnow cross tRPC parsed rather than raw. This was forced: reading that property off the generated output type raises TS2589 (type instantiation is excessively deep), which is exactly why the client had been casting throughunknownto reach it. Parsing server-side removes both casts and renders identically. Nothing else reads those fields.AGENTS.mdgains one line, because the pattern path it cites moved and would otherwise dangle.Known, deliberately not done
schemas.agents.capabilitiesis still a looser second view of the same column, used byrevise(). Tightening it would flip pre-typed-permission manifests to "unreadable" and drop unknown top-level keys on rewrite — that needs a human call, not a refactor.builder-runtime.tsstill writes the manifest as an untyped literal, so the writer isn't checked against the schema its readers now share. Needs the draft trigger config to stop carrying nullable interval/event fields first.Verification
bun run check-types13/13 ·bun run lint9/9apps/agent313 pass / 0 fail ·apps/apiagent-runs, agent-events, conversations, agent-lifecycle, agent-delete 59 pass / 0 fail ·apps/app147 pass / 0 failPushed with
--no-verifyfor the same pre-push stall as #145 and #146.🤖 Generated with Claude Code