fix: validate generated REST contracts - #815
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
π WalkthroughWalkthrough
ChangesCanonical Contract Graph ν΅ν©
Sequence Diagram(s)sequenceDiagram
actor Developer
participant CrocoCLI as croco CLI
participant runContractsCheck
participant rpcCodegenCLI as croco-rpc-codegen --check
participant loadContractGraph
participant buildContractGraph
participant assertContractGraphHasNoErrors
Developer->>CrocoCLI: croco contracts check --controllers '...'
CrocoCLI->>runContractsCheck: rawArgs
runContractsCheck->>rpcCodegenCLI: spawn(node, [dist/cli.js, --check, ...args])
rpcCodegenCLI->>loadContractGraph: glob ν¨ν΄
loadContractGraph->>buildContractGraph: controllerConstructors[]
buildContractGraph-->>loadContractGraph: ContractGraph (routes, diagnostics)
loadContractGraph-->>rpcCodegenCLI: ContractGraph
rpcCodegenCLI->>assertContractGraphHasNoErrors: ContractGraph
alt μλ¬ μμ
assertContractGraphHasNoErrors-->>rpcCodegenCLI: ν΅κ³Ό
rpcCodegenCLI-->>Developer: exit 0
else μλ¬ μμ
assertContractGraphHasNoErrors-->>rpcCodegenCLI: ContractGraphDiagnosticError
rpcCodegenCLI-->>Developer: μ§λ¨ μΆλ ₯ + exit 1
end
Estimated code review effortπ― 4 (Complex) | β±οΈ ~65 minutes Possibly related PRs
π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing Touchesπ§ͺ Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
π Benchmark Resultsβ Some benchmarks failed Gate failures
Updated: 2026-06-16T06:49:23.839Z Β· Commit: 325bded |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
packages/openapi-spec/src/libs/emitOpenAPI.ts (1)
301-317:β οΈ Potential issue | π Major | β‘ Quick win
toHttpMethodμμ genericErrorλ₯Ό throw νκ³ μμ΅λλ€.μλ¬ κ²½λ‘(
@All, unsupported method) λͺ¨λ Problem subclassλ‘ ν΅μΌν΄μΌ μμ κ³μΈ΅μ RFC7807 μ²λ¦¬ κ³μ½μ΄ μ μ§λ©λλ€.
As per coding guidelines, "Throw only Problem subclasses for error handling; never throw generic Error instances (RFC 7807 Problem-based error handling)."π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openapi-spec/src/libs/emitOpenAPI.ts` around lines 301 - 317, The toHttpMethod function is throwing generic Error instances instead of Problem subclass instances, which breaks the RFC7807 error handling contract in upper layers. Replace both throw statements in toHttpMethod (the one for the "`@All`" route case and the one for the unsupported HTTP method case) with appropriate Problem subclass instances instead of generic Error, ensuring the descriptive error messages are preserved in the Problem instances.Source: Coding guidelines
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/tests/contractsCheck.spec.ts`:
- Around line 4-63: The test file contractsCheck.spec.ts is missing the required
DI Container reset for test isolation. Add a beforeEach hook inside the describe
block that calls Container.reset() to ensure proper test isolation as per the
project's testing guidelines. Import the Container from the appropriate DI
module and call Container.reset() within a beforeEach() function that wraps all
the it test cases in the describe block.
In `@packages/create-croco-app/src/tests/templates-build.spec.ts`:
- Line 87: The validation for the `contract:check` key is too lenient as it only
checks for the presence of `--check` flag using expect.stringContaining, which
could allow incorrect command strings to pass the test. Strengthen this
assertion by also verifying that the command body `croco-rpc-codegen` is present
in the value, ensuring the complete and correct command is validated. Use a
matcher that checks for both the command name and the flag to prevent false
positives.
In `@packages/create-croco-app/templates/spa-be-split/package.json.hbs`:
- Around line 8-11: The contract:check script is defined but not enforced as a
prerequisite before code generation runs, allowing contract:openapi and
contract:client to be executed directly without validation. Update the codegen
script to chain contract:check before contract:client by modifying its value to
first run the validation step, ensuring validation is always performed before
any code generation occurs at the template level.
In `@packages/openapi-spec/src/tests/emitOpenAPI.spec.ts`:
- Around line 19-22: Add the missing test initialization setup to comply with
coding guidelines. First, update the vitest import statement to include
beforeEach alongside the existing describe, expect, and it imports. Then, add a
beforeEach hook after the imports that calls Container.reset() to ensure the DI
Container is reset before each test runs. This addresses the required testing
utilities import set and the mandatory Container cleanup pattern that must be
applied throughout the test file (lines 23-439).
In `@packages/protocols-core/src/libs/ContractGraph.ts`:
- Around line 41-48: The ContractGraphDiagnosticError class extends Error
instead of Problem, which violates the RFC 7807 Problem-based error handling
policy. Change the base class of ContractGraphDiagnosticError from Error to
Problem, and update the constructor to properly initialize the Problem base
class with appropriate problem details (such as type, title, status, and detail
fields) while maintaining the diagnostics field and its initialization.
- Around line 81-82: The code currently validates routeId uniqueness using
validateUniqueRouteIds but lacks validation for operationId uniqueness, which
can cause naming collisions in OpenAPI client generation. Add a new validation
function (similar to validateUniqueRouteIds) to check for duplicate normalized
operationIds across the graph routes, then call this validation function and
push its results to the diagnostics array alongside the existing
validateUniqueRouteIds call at the anchor location (line 81-82). Also add the
same operationId validation check at the sibling locations (lines 113-121 and
256-279) where similar diagnostics collection occurs, ensuring operationId
uniqueness is validated consistently across all graph validation points.
In `@packages/protocols-core/src/tests/ContractGraph.spec.ts`:
- Around line 2-3: The test file is missing required vitest and DI container
initialization patterns per coding guidelines. Add beforeEach to the vitest
imports at the top of the file, and import the Container class from the DI
container module. Then add a beforeEach() block that calls Container.reset() to
ensure the dependency injection container is reset before each test runs. This
setup applies to the entire test suite in ContractGraph.spec.ts and ensures test
isolation according to the project's testing standards.
In `@packages/rpc-codegen/src/libs/generate.ts`:
- Around line 73-80: The assertGeneratedClientPathParams function is missing a
validation check in one direction: it does not verify that all parameters
present in schemaParamNames also exist in pathParamNames. Currently it only
checks pathβdeclared, pathβschema, and declaredβpath directions, but not
schemaβpath. Add a validation assertion that ensures every parameter name in
schemaParamNames is contained in pathParamNames, blocking cases where
inputSchemas.path contains keys that do not exist in the actual route path. This
will prevent invalid shape influx in the RouteIR validation.
- Around line 63-66: The code is throwing generic Error instances at multiple
locations (lines 63-66, 84-86, 90-92, and 98-100) in
packages/rpc-codegen/src/libs/generate.ts, which violates RFC 7807 Problem-based
error handling guidelines. Replace all instances of new Error with appropriate
Problem subclasses at all four locations. For the error at lines 63-66 that
checks bodyParamCount and calls formatRoute(route), and similarly for the errors
at lines 84-86, 90-92, and 98-100, use the corresponding Problem subclass type
instead of generic Error to maintain consistency with the Problem-based error
contract.
---
Outside diff comments:
In `@packages/openapi-spec/src/libs/emitOpenAPI.ts`:
- Around line 301-317: The toHttpMethod function is throwing generic Error
instances instead of Problem subclass instances, which breaks the RFC7807 error
handling contract in upper layers. Replace both throw statements in toHttpMethod
(the one for the "`@All`" route case and the one for the unsupported HTTP method
case) with appropriate Problem subclass instances instead of generic Error,
ensuring the descriptive error messages are preserved in the Problem instances.
πͺ Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e915f948-09b2-424d-b12f-c083baec448f
π Files selected for processing (24)
.changeset/contract-graph-check.mdpackages/cli/src/bin/croco.tspackages/cli/src/commands/contracts.tspackages/cli/src/commands/contractsCheck.tspackages/cli/src/index.tspackages/cli/src/tests/contractsCheck.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/spa-be-split/README.md.hbspackages/create-croco-app/templates/spa-be-split/package.json.hbspackages/openapi-spec/src/index.tspackages/openapi-spec/src/libs/emitOpenAPI.tspackages/openapi-spec/src/tests/emitOpenAPI.spec.tspackages/protocols-core/src/index.tspackages/protocols-core/src/libs/ContractGraph.tspackages/protocols-core/src/tests/ContractGraph.spec.tspackages/rpc-codegen/src/index.tspackages/rpc-codegen/src/libs/cli.tspackages/rpc-codegen/src/libs/generate.tspackages/rpc-codegen/src/libs/loadRoutes.tspackages/rpc-codegen/src/tests/Cli.spec.tspackages/rpc-codegen/src/tests/ContractCheckCli.spec.tspackages/rpc-codegen/src/tests/codegen.spec.tspackages/rpc-codegen/src/tests/loadRoutes.spec.tsscripts/create-croco-app-generated-smoke.mts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (2)
packages/rpc-codegen/src/libs/generate.ts (2)
552-554:β οΈ Potential issue | π Major | β‘ Quick win
:...paramκ²½λ‘ νλΌλ―Έν° νμ±μ΄ μλͺ»λμ΄ μ μ λΌμ°νΈκ° κ±°λΆλ©λλ€Line 553μμ
:...idλ₯Ό...idλ‘ λ³΄κ΄ν΄@Param('id')/inputSchemas.path.idμ λΆμΌμΉκ° λ°μν©λλ€. catch-all κ²½λ‘κ° μ€νμΌλ‘ μ€ν¨ν©λλ€.μμ μ μ
function getRoutePathParamNames(pathname: string): string[] { - return [...pathname.matchAll(/:([^/]+)/g)].map((match) => match[1]).filter(Boolean); + return [...pathname.matchAll(/:([^/]+)/g)] + .map((match) => match[1].replace(/^\.\.\./, '')) + .filter((name) => name.length > 0); }π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rpc-codegen/src/libs/generate.ts` around lines 552 - 554, The getRoutePathParamNames function is capturing path parameter names including spread operator syntax (e.g., capturing `...id` from `:...id`), but the corresponding decorator names and schema keys only use the base parameter name (e.g., `id`). Modify the function to strip the leading `...` from catch-all parameters after extracting them with the regex, so that both `:id` and `:...id` route definitions return the same parameter name `id` from this function.
626-633:β οΈ Potential issue | π MajorRFC 7807 Problem κΈ°λ° μμΈ μ²λ¦¬ κ³μ½ μλ°
Line 632μμ
throw new Error(...)λ₯Ό μ¬μ©νκ³ μμΌλ©°, μ΄λ μ½λ© κ°μ΄λλΌμΈ "Throw only Problem subclasses for error handling; never throw generic Error instances (RFC 7807 Problem-based error handling)"μ μλ°°λ©λλ€. νμΌ λ΄ λ€λ₯Έ λͺ¨λ μμΈ μ²λ¦¬(lines 63, 71, 91, 97, 105, 113)μμλRpcCodegenContractProblemμ μ¬μ©νλ―λ‘, μ΄ ν¨μλ ν΅μΌν΄μΌ ν©λλ€.μμ μ μ
function assertNoZodImport(content: string): void { if ( content.includes("from 'zod'") || content.includes("import { z }") || content.includes("zod") ) { - throw new Error("Generated client must not import zod."); + throw new RpcCodegenContractProblem('Generated client must not import zod.'); } }π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rpc-codegen/src/libs/generate.ts` around lines 626 - 633, The assertNoZodImport function throws a generic Error instead of RpcCodegenContractProblem, which violates the coding guideline that requires RFC 7807 Problem-based error handling. Replace the throw new Error statement in the assertNoZodImport function with throw new RpcCodegenContractProblem to maintain consistency with all other error handling in the file (as seen at lines 63, 71, 91, 97, 105, 113). Keep the error message content the same, just wrap it in RpcCodegenContractProblem instead of Error.Source: Coding guidelines
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rpc-codegen/src/libs/generate.ts`:
- Around line 552-554: The getRoutePathParamNames function is capturing path
parameter names including spread operator syntax (e.g., capturing `...id` from
`:...id`), but the corresponding decorator names and schema keys only use the
base parameter name (e.g., `id`). Modify the function to strip the leading `...`
from catch-all parameters after extracting them with the regex, so that both
`:id` and `:...id` route definitions return the same parameter name `id` from
this function.
- Around line 626-633: The assertNoZodImport function throws a generic Error
instead of RpcCodegenContractProblem, which violates the coding guideline that
requires RFC 7807 Problem-based error handling. Replace the throw new Error
statement in the assertNoZodImport function with throw new
RpcCodegenContractProblem to maintain consistency with all other error handling
in the file (as seen at lines 63, 71, 91, 97, 105, 113). Keep the error message
content the same, just wrap it in RpcCodegenContractProblem instead of Error.
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58e33135-3381-4d99-ade9-8ce1e470db59
β Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
π Files selected for processing (17)
.changeset/contract-graph-check.mdpackages/cli/package.jsonpackages/cli/src/tests/contractsCheck.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/spa-be-split/package.json.hbspackages/openapi-spec/package.jsonpackages/openapi-spec/src/libs/emitOpenAPI.tspackages/openapi-spec/src/libs/loadControllers.tspackages/openapi-spec/src/tests/emitOpenAPI.spec.tspackages/protocols-core/package.jsonpackages/protocols-core/src/libs/ContractGraph.tspackages/protocols-core/src/tests/ContractGraph.spec.tspackages/rpc-codegen/package.jsonpackages/rpc-codegen/src/libs/generate.tspackages/rpc-codegen/src/libs/loadRoutes.tspackages/rpc-codegen/src/tests/codegen.spec.tsscripts/create-croco-app-generated-smoke.mts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
packages/protocols-core/src/tests/ContractGraph.spec.ts (1)
3-3:β οΈ Potential issue | π Major | β‘ Quick win
vitestνμ import μΈνΈμμviκ° λλ½λμμ΅λλ€.Line 3μμ
viimportκ° λΉ μ Έ μμ΄ ν μ€νΈ κ·μΉκ³Ό λΆμΌμΉν©λλ€.beforeEachμμvi.restoreAllMocks()λ₯Ό ν¨κ» νΈμΆνλ©΄ κ·μΉκ³Ό 격리μ±μ κ°μ΄ λ§μΆ μ μμ΅λλ€.As per coding guidelines, "Test files must import testing utilities from 'vitest' (beforeEach, describe, expect, it, vi)".
π§ μ μ μμ μ
-import { beforeEach, describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; beforeEach(() => { Container.reset(); + vi.restoreAllMocks(); });Also applies to: 23-25
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/protocols-core/src/tests/ContractGraph.spec.ts` at line 3, The import statement is missing `vi` from the vitest utilities, which violates the coding guidelines that require importing `beforeEach, describe, expect, it, vi` from vitest. Add `vi` to the import statement on line 3, then locate the `beforeEach` hook (around lines 23-25) and call `vi.restoreAllMocks()` within it to ensure proper test isolation and mock restoration between test cases.Source: Coding guidelines
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/openapi-spec/src/libs/emitOpenAPI.ts`:
- Around line 305-309: The path token substitution in the `toOpenAPIPath`
function uses a simple string split/join approach that incorrectly matches
parameter name prefixes. When a path contains parameters like `:id` and `:id2`,
the current logic splits on `:id` and corrupts `:id2` to `{id}2`. Replace the
simple split and join in the reduce callback with a boundary-based substitution
method, such as using a regex pattern that matches the parameter token only when
followed by a path delimiter or end of string, to ensure exact parameter
matching without prefix collisions.
In `@packages/rpc-codegen/src/libs/generate.ts`:
- Around line 535-540: The pathExpression assignment in the pathParams.reduce
function has a prefix collision issue where replacing shorter parameter tokens
like `:id` before longer ones like `:id2` corrupts the longer tokens during
string replacement. Fix this by either sorting pathParams in descending order by
token length before processing, or by replacing all path parameters in a single
regex-based operation instead of sequential string replacements, ensuring that
longer token names are processed first and preventing any prefix matching
issues.
---
Outside diff comments:
In `@packages/protocols-core/src/tests/ContractGraph.spec.ts`:
- Line 3: The import statement is missing `vi` from the vitest utilities, which
violates the coding guidelines that require importing `beforeEach, describe,
expect, it, vi` from vitest. Add `vi` to the import statement on line 3, then
locate the `beforeEach` hook (around lines 23-25) and call
`vi.restoreAllMocks()` within it to ensure proper test isolation and mock
restoration between test cases.
πͺ Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0cb04e5-d3dc-4b42-a8e9-edb8762351d4
π Files selected for processing (15)
.changeset/contract-graph-check.mdpackages/openapi-spec/src/libs/emitOpenAPI.tspackages/openapi-spec/src/tests/emitOpenAPI.spec.tspackages/protocols-core/src/index.tspackages/protocols-core/src/libs/ContractGraph.tspackages/protocols-core/src/libs/sharedTypes.tspackages/protocols-core/src/tests/ContractGraph.spec.tspackages/protocols-core/src/tests/helpers/test-decorators.tspackages/rpc-codegen/src/libs/cli.tspackages/rpc-codegen/src/libs/generate.tspackages/rpc-codegen/src/tests/Cli.spec.tspackages/rpc-codegen/src/tests/ContractCheckCli.spec.tspackages/rpc-codegen/src/tests/PublishedCli.spec.tspackages/rpc-codegen/src/tests/codegen.spec.tspackages/rpc-codegen/src/tests/loadRoutes.spec.ts
π€ Files with no reviewable changes (1)
- .changeset/contract-graph-check.md
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
packages/protocols-core/src/libs/ContractGraph.ts (1)
325-341:β οΈ Potential issue | π Majorμ€μ²©λ Zod effectsλ μ§λ¨ λμμΌλ‘ μνν΄ μ£ΌμΈμ.
νμ¬ μ½λλ
getRouteSchemasκ° λ°ννλ μ΅μμ μ€ν€λ§λ§isZodEffectsλ‘ κ²μ¬ν©λλ€.z.object({ name: z.string().transform(...) })μ²λΌ κ°μ²΄ λ΄λΆμ μλ transform/refinementλ κ°μ²΄ μμ²΄κ° ZodEffectsκ° μλλ―λ‘ κ²½κ³ μμ΄ ν΅κ³Όν©λλ€. μ΄ κ²½μ° μμ±λ κ³μ½μ μ¬μ ν μ΄λ° effectsλ₯Ό νννμ§ λͺ»νλλ°, κ²μ¦ λ‘μ§μ΄ μ΄λ₯Ό κ°μ§νμ§ μμ drift κ°λ₯μ±μ΄ μ¨κ²¨μ§λλ€.ZodObjectμ
.shapeμμ±μ ν΅ν΄ μ€μ²©λ νλμ μ κ·Όνκ³ μ¬κ·μ μΌλ‘ effectsλ₯Ό κ²μ¬νλ λ‘μ§μ΄ νμν©λλ€.κ²μ¦μ© ν μ€νΈ μμ
it("should warn when generated contracts unwrap Zod effects", () => { @@ }); + + it("should warn when generated contracts unwrap nested Zod effects", () => { + `@Controller`("/profiles") + class ProfilesController { + `@Post`("/") + createProfile( + `@Body`(z.object({ name: z.string().transform((value) => value.trim()) })) + _body: { name: string }, + ): void {} + } + + const graph = buildContractGraph([ProfilesController]); + + expect(graph.diagnostics).toEqual([ + expect.objectContaining({ + code: "contract-schema-zod-effects-unwrapped", + severity: "warning", + routeId: "ProfilesController.createProfile", + }), + ]); + });π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/protocols-core/src/libs/ContractGraph.ts` around lines 325 - 341, The validateSchemaEffects function currently only checks top-level schemas returned by getRouteSchemas for ZodEffects, but misses nested effects that exist within ZodObject properties (such as z.string().transform(...) nested inside z.object()). Add a recursive helper function that traverses into ZodObject's .shape property to inspect nested schemas, and modify the main loop in validateSchemaEffects to call this helper function for each top-level schema. This recursive traversal should detect ZodEffects at any depth and generate diagnostics for each occurrence, preventing the drift that occurs when generated contracts cannot represent these hidden effects.
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/protocols-core/src/libs/ContractGraph.ts`:
- Around line 477-500: The getMetadataReference function is silently discarding
guard metadata when the function or constructor has an empty name by returning
null, which causes incomplete access metadata. Instead of filtering out guards
based on name.length > 0 in both the function check and the constructor check,
you should handle unnamed guards appropriatelyβeither by creating references
with a generated or placeholder name, or by logging warnings about skipped
guards rather than silently returning null. This ensures that guard metadata is
preserved in the access graph even when names are missing, preventing incomplete
contract metadata and missing drift diagnostics.
In `@packages/protocols-core/src/tests/ContractGraph.spec.ts`:
- Around line 239-240: Remove the definite assignment assertion (!) from the
FirstController and SecondController variable declarations. These are local
variables within a test block that are assigned immediately, not test instance
variables declared in describe blocks. According to the coding guidelines, the
non-null assertion (!) is prohibited for local test variables and should only be
used for test instance variables declared in describe scopes. Simply declare the
variables without the ! operator.
In `@packages/rpc-codegen/src/libs/generate.ts`:
- Around line 535-540: The path parameter accessor in the replace callback of
the pathExpression generation currently uses dot notation (input.path.${name})
which fails for parameter names containing hyphens or other characters that are
not valid JavaScript identifiers. Modify the return statement in the replace
callback to check if the parameter name is a valid JavaScript identifier; if it
is, use dot notation as currently implemented, but if it is not, use bracket
notation with quoted string notation (input.path['${name}']) instead. This
ensures that path parameters like user-id are accessed correctly as
input.path['user-id'] rather than the invalid input.path.user-id.
In `@packages/rpc-codegen/src/tests/codegen.spec.ts`:
- Line 473: Remove the `as any` type assertion from the path schema object in
the test. Since this object is being passed as part of a `RouteIR[]` array, it
has contextual typing that will properly infer the types without needing the
explicit `as any` cast. This aligns with the coding guideline that prohibits
explicit use of the any type. Simply delete the `as any` portion while keeping
the z.object({ id: z.string(), id2: z.string() }) expression as is.
---
Outside diff comments:
In `@packages/protocols-core/src/libs/ContractGraph.ts`:
- Around line 325-341: The validateSchemaEffects function currently only checks
top-level schemas returned by getRouteSchemas for ZodEffects, but misses nested
effects that exist within ZodObject properties (such as
z.string().transform(...) nested inside z.object()). Add a recursive helper
function that traverses into ZodObject's .shape property to inspect nested
schemas, and modify the main loop in validateSchemaEffects to call this helper
function for each top-level schema. This recursive traversal should detect
ZodEffects at any depth and generate diagnostics for each occurrence, preventing
the drift that occurs when generated contracts cannot represent these hidden
effects.
πͺ Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b725239-fbaa-455d-8826-8dce7398be5a
π Files selected for processing (10)
.changeset/contract-graph-check.mdpackages/openapi-spec/src/libs/emitOpenAPI.tspackages/openapi-spec/src/tests/emitOpenAPI.spec.tspackages/protocols-core/src/index.tspackages/protocols-core/src/libs/ContractGraph.tspackages/protocols-core/src/tests/ContractGraph.spec.tspackages/rpc-codegen/src/libs/generate.tspackages/rpc-codegen/src/tests/codegen.spec.tspackages/transports-http/src/libs/RouteCompiler.tspackages/transports-http/src/tests/CrocoApp.spec.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
packages/protocols-core/src/libs/ContractGraph.ts (1)
436-438: π§Ή Nitpick | π΅ Trivial | π€ Low value
constructor.nameκΈ°λ° μ²΄ν¬λ μ½λ λλ ν νκ²½μμ μ·¨μ½ν μ μμ΅λλ€.
isZodEffectsν¨μκ°schema.constructor.name === "ZodEffects"λ₯Ό μ¬μ©νλλ°, μ΄λ minification λλ λ²λ€λ¬ μ€μ μ λ°λΌ ν΄λμ€ μ΄λ¦μ΄ λ³κ²½λ κ²½μ° λμνμ§ μμ μ μμ΅λλ€.μλ² μ¬μ΄λ μ½λλ‘μ μΌλ°μ μΌλ‘ λλ νλμ§ μμΌλ―λ‘ νμ¬ κ΅¬νμ μ€μ©μ μ΄μ§λ§, ν₯ν μ΄μ κ°λ₯μ±μ μΈμ§ν΄ λμκΈ° λ°λλλ€. Zodμ
schema._def.typeNameμ κ·Όλ κ³ λ €ν μ μμΌλ, λ΄λΆ API μμ‘΄μ± μΈ‘λ©΄μμλ νμ¬ λ°©μλ ν©λ¦¬μ μ λλ€.π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/protocols-core/src/libs/ContractGraph.ts` around lines 436 - 438, The isZodEffects function currently relies on schema.constructor.name to detect ZodEffects instances, which can fail if code is minified or obfuscated. Replace the constructor.name check with a more robust approach by accessing Zod's internal _def.typeName property (e.g., checking if schema._def?.typeName === "ZodEffects"), which is more reliable across different bundling and obfuscation scenarios. If using the internal _def API is not preferred, add a comment explaining the current limitation and the assumption that server-side code will not be obfuscated.
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/protocols-core/src/libs/ContractGraph.ts`:
- Around line 436-438: The isZodEffects function currently relies on
schema.constructor.name to detect ZodEffects instances, which can fail if code
is minified or obfuscated. Replace the constructor.name check with a more robust
approach by accessing Zod's internal _def.typeName property (e.g., checking if
schema._def?.typeName === "ZodEffects"), which is more reliable across different
bundling and obfuscation scenarios. If using the internal _def API is not
preferred, add a comment explaining the current limitation and the assumption
that server-side code will not be obfuscated.
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5dbf8ff-7b12-4385-b04f-3c586e21f244
π Files selected for processing (5)
packages/protocols-core/src/libs/ContractGraph.tspackages/protocols-core/src/tests/ContractGraph.spec.tspackages/rpc-codegen/src/libs/generate.tspackages/rpc-codegen/src/tests/codegen.spec.tspackages/rpc-codegen/vitest.config.ts
This was referenced Jun 19, 2026
This was referenced Jun 30, 2026
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
ContractGraphin@croco/protocols-corewith diagnostics for generated-contract metadata drift.croco-rpc-codegen --checkandcroco contracts check.contract:checkbefore OpenAPI/client generation.Closes #715
Verification
pnpm --filter @croco/protocols-core testpnpm --filter @croco/openapi-spec testpnpm --filter @croco/rpc-codegen testpnpm --filter @croco/cli testpnpm --filter create-croco-app testpnpm --filter @croco/protocols-core --filter @croco/openapi-spec --filter @croco/rpc-codegen --filter @croco/cli --filter create-croco-app typecheckpnpm checkpnpm changeset-required:check -- --base origin/trunk --head HEADpnpm create-croco-app:smokepnpm testandpnpm typecheckSelf-review
Risk
generateClientFiles(routes)remains a low-level compatibility API for rawRouteIR[]; CLI and template generation now useContractGraph, and the raw API guards known invalid generated-contract shapes before writing clients.Summary by CodeRabbit
Release Notes
New Features
contracts checkλ‘ REST canonical contract graph(λΌμ°νΈ μ§λ¨ ν¬ν¨) κ²μ¦ λ° κ²°κ³Ό μΆλ ₯μ΄ κ°λ₯ν©λλ€.rpc-codegen --checkλ° contract graph κΈ°λ° OpenAPI/RPC ν΄λΌμ΄μΈνΈ μμ±μΌλ‘ κ²½λ‘/νλΌλ―Έν°/λ°λ κ·μΉ μλ°μ λ μ격ν μ κ²ν©λλ€.spa-be-splitν νλ¦Ώμpnpm contract:checkλ¨κ³λ₯Ό μΆκ°νκ³ OpenAPI/RPC μμ± μμλ₯Ό μ λ ¬νμ΅λλ€.Bug Fixes
Chores
patchλ‘ μ§μ νμ΅λλ€.