fix: promote REST route contracts to controller decorators - #1008
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running โ you're only billed for reviews past your plan's rate limits ($0.25/file). โ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. ๐ฆ How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. โน๏ธ Review infoโ๏ธ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ๐ Files selected for processing (13)
๐ WalkthroughWalkthrough
ChangesContract-first REST ๋ผ์ฐํธ ๊ตฌํ
Sequence Diagram(s)sequenceDiagram
participant Dev as ๊ฐ๋ฐ์
participant RouteContract as defineRouteContract
participant Decorator as `@Get/`\n@Param/@Body
participant Controller as `@Controller`
participant ExtractRouteIR as extractRouteIR
participant ContractGraph as buildContractGraph
participant Diagnostic as ContractDiagnostic
Dev->>RouteContract: defineRouteContract({ method, path, params, body, response })
RouteContract-->>Dev: typed contract ๊ฐ์ฒด ๋ฐํ
Dev->>Decorator: `@Get`(contract), `@Param`(contract, "id"), `@Body`(contract)
Decorator->>Decorator: isRouteContractSpec ํ๋ณ โ ์คํค๋ง ์ถ์ถ
Dev->>Controller: `@Controller`("/users")
Controller->>Controller: normalizeContractRoutePaths โ contract.path๋ฅผ ์๋ ๊ฒฝ๋ก๋ก ์ ๊ทํ
ExtractRouteIR->>ExtractRouteIR: extractRouteContract โ RouteContractIR ๊ตฌ์ฑ
ExtractRouteIR->>ExtractRouteIR: mergeContractInputSchemas โ body/params/query ๋ณํฉ
ExtractRouteIR-->>ContractGraph: RouteIR (routeContract ํฌํจ)
ContractGraph->>ContractGraph: validateRouteContract โ method/path/params/body/response ๊ฒ์ฌ
ContractGraph-->>Diagnostic: ๋ถ์ผ์น ์ contractId/sourceLocation ํฌํจ ์ง๋จ ์์ฑ
Estimated code review effort๐ฏ 4 (Complex) | โฑ๏ธ ~60 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-21T07:00:52.387Z ยท Commit: 3f302f7 |
58f20fa to
8f8c93c
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
f190766 to
c62672d
Compare
c62672d to
9c810da
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
๐ค 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/docs/src/content/docs/api/protocols-rest/src/functions/Body.md`:
- Around line 8-44: The type constraint for TContract in the Body function
documentation is incorrect and doesn't match the actual implementation. Update
line 18 where TContract is defined to change the constraint from
RouteContractSpec & object to RouteContractSpec & { body: z.ZodType } to
accurately reflect what the implementation requires. Additionally, resolve the
markdown lint MD024 violation by renaming the duplicate "## Call Signature"
headings at lines 8 and 30 to descriptive names like "## Overload with Contract"
for the first occurrence and "## Overload with Schema" for the second occurrence
to clearly distinguish between the two function overloads.
In
`@packages/docs/src/content/docs/api/protocols-rest/src/functions/isRouteContractSpec.md`:
- Around line 1-18: The return type for the isRouteContractSpec function is
overly complex with fully expanded generic parameters making it hard to read.
Simplify both the return type in the function signature and in the Returns
section by replacing the lengthy parameterized type with a more concise form
like `value is RouteContractSpec`. Additionally, add a new Description or
Example section to explain the function's purpose, runtime behavior, and
demonstrate how the type guard works with example code.
In `@packages/docs/src/content/docs/api/protocols-rest/src/functions/Param.md`:
- Around line 8-56: Update the Param function documentation to accurately
reflect the actual type constraints from the implementation in
packages/protocols-rest/src/libs/decorators/Params.ts. Change the TContract type
constraint from `RouteContractSpec & object` to `RouteContractSpec & { params:
AnyZodObject }`, and update the Name type constraint from simply `string` to the
complete constraint `RoutePathParamName<TContract["path"]> & keyof
RoutePathParams<TContract> & string`. Additionally, resolve the markdown linting
violation (MD024) by removing the duplicate "## Call Signature" heading that
appears in the second overload documentation and replace it with a different
heading structure or use a different approach to distinguish the two function
signatures.
In `@packages/docs/src/content/docs/api/protocols-rest/src/functions/Query.md`:
- Around line 8-56: The documentation for the Query function contains three
issues that need to be corrected. First, update the TContract type parameter
constraint from `RouteContractSpec & object` to `RouteContractSpec & { query:
AnyZodObject }` to match the actual implementation. Second, update the Name type
parameter constraint from simply `string` to `keyof RouteQuery<TContract> &
string` to accurately reflect the constraint used in the implementation. Third,
remove the duplicate "## Call Signature" heading that appears at the second
function overload definition and replace it with a unique heading (such as "##
Call Signature (Schema-based)") to comply with markdown lint rule MD024 which
prohibits duplicate headings.
In
`@packages/docs/src/content/docs/api/protocols-rest/src/functions/ResponseSchema.md`:
- Around line 8-40: Update the ResponseSchema documentation to match its actual
implementation and add missing Korean descriptions. First, change the TContract
type parameter constraint from `RouteContractSpec & object` to
`RouteContractSpec & { response: z.ZodType }` to accurately reflect the
implementation in packages/protocols-rest/src/libs/decorators/ResponseSchema.ts.
Second, add Korean descriptions after each Call Signature section (after the
Returns statements) by including the text "์๋ต ์คํค๋ง๋ฅผ ๋ฉ์๋์ ๋ฐ์ธ๋ฉํฉ๋๋ค" for both
overloads, following the same pattern used in the Body and Param decorator
documentation. Third, fix the markdown linting MD024 violation by changing the
second "## Call Signature" heading to a different heading level such as "###" or
providing a unique title to eliminate the duplicate heading.
In
`@packages/docs/src/content/docs/api/protocols-rest/src/functions/routeParamSchema.md`:
- Around line 1-32: The type parameter constraints documented for
routeParamSchema, routeQueryParamSchema, routeBodySchema, and routeQuerySchema
functions do not match their actual TypeScript implementation. The documentation
shows overly generic constraints while the actual implementations have more
specific constraints including specific type extensions like params properties
and keyof restrictions. Review and update the documentation generation logic or
the source documentation to accurately reflect the precise type constraints from
the TypeScript source code, ensuring that TContract and Name parameters include
all the specific type bounds and constraints that exist in the actual
implementation.
In
`@packages/docs/src/content/docs/api/protocols-rest/src/functions/routeQueryParamSchema.md`:
- Around line 1-32: The generated API documentation for the
routeQueryParamSchema function contains inaccurate type parameter constraints
that diverge from the actual implementation. The TContract type parameter
constraint is documented as extending RouteContractSpec & object, but should
extend RouteContractSpec & { query: AnyZodObject } to match the implementation.
The Name type parameter is documented as extending just string, but should
extend keyof RouteQuery<TContract> & string to accurately reflect the actual
constraint. Since this same pattern occurs across multiple files
(routeParamSchema.md, Param.md, Query.md), the fix needs to be applied to the
API documentation generation logic itself rather than individual markdown files,
ensuring all generic type constraints are extracted and represented with
complete accuracy.
In `@packages/protocols-core/src/libs/extractRouteIR.ts`:
- Around line 75-96: The id field assignment in the extractRouteContract
function currently uses operationId as a fallback value when id is missing,
which creates confusion between two independent fields and causes potential
duplication. According to the test expectations and the field definitions, id
and operationId should remain separate. Remove the fallback to
contract.operationId from the id field assignment on line 83 so that id only
uses contract.id directly, ensuring both fields maintain their distinct purposes
and avoid duplication when operationId exists but id does not.
In `@packages/protocols-core/src/libs/RouteIR.ts`:
- Line 9: The routeContract field in the RouteIR type definition uses both the
optional property modifier (?) and null in the union type, allowing three
possible states (undefined, null, or RouteContractIR). However, the runtime
implementation in extractRouteIR.ts always assigns either a RouteContractIR
object or null to this field, never leaving it undefined. Remove the optional
property modifier from routeContract so the type is defined as simply a union of
RouteContractIR or null, without the question mark, to accurately reflect that
the field is always explicitly assigned and will never be undefined.
In `@packages/protocols-core/src/tests/extractRouteIR.spec.ts`:
- Line 235: Remove the explicit `any` type cast `z.ZodObject<any>` used
throughout the test file (there are 6 occurrences total) and replace all
instances with `z.AnyZodObject` instead. This aligns with the codebase coding
guidelines that forbid explicit `any` types in TypeScript files and matches the
pattern already established in sharedTypes.ts.
In `@packages/protocols-rest/src/libs/types/RouteContract.ts`:
- Around line 115-123: The isRouteContractSpec type guard function is too
permissive as it only checks if method and path are strings, which can cause
false positives when used in conditional branches for different overload types.
This leads to objects that are not actual RouteContractSpec instances being
incorrectly classified as such, resulting in runtime errors like
getObjectShape(undefined). Enhance the type guard by adding checks for all
required properties and constraints that define a complete RouteContractSpec
(beyond just method and path), ensuring that only objects matching the full
contract specification return true from this function.
๐ช 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: daeb15b1-3383-4ae5-a3dd-bf188c9dfa54
๐ Files selected for processing (46)
.changeset/contract-first-rest-routes.mdpackages/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/apps/api-server/src/controllers/UserController.tspackages/create-croco-app/templates/spa-be-split/apps/api-server/src/controllers/userSchemas.tspackages/docs/src/content/docs/api/protocols-rest/src/functions/Body.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/Param.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/Query.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/ResponseSchema.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/defineRouteContract.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/isRouteContractSpec.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/routeParamSchema.mdpackages/docs/src/content/docs/api/protocols-rest/src/functions/routeQueryParamSchema.mdpackages/docs/src/content/docs/api/protocols-rest/src/interfaces/RouteMetadata.mdpackages/docs/src/content/docs/api/protocols-rest/src/type-aliases/RouteContractSourceLocation.mdpackages/docs/src/content/docs/api/protocols-rest/src/type-aliases/RouteContractSpec.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/All.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Delete.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Get.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Head.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Options.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Patch.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Post.mdpackages/docs/src/content/docs/api/protocols-rest/src/variables/Put.mdpackages/docs/src/content/docs/en/guides/schema-source-of-truth.mdxpackages/openapi-spec/src/tests/emitOpenAPI.spec.tspackages/protocols-core/src/index.tspackages/protocols-core/src/libs/ContractGraph.tspackages/protocols-core/src/libs/ContractGraphSnapshot.tspackages/protocols-core/src/libs/RouteIR.tspackages/protocols-core/src/libs/extractRouteIR.tspackages/protocols-core/src/libs/sharedTypes.tspackages/protocols-core/src/tests/ContractGraph.spec.tspackages/protocols-core/src/tests/extractRouteIR.spec.tspackages/protocols-rest/README.mdpackages/protocols-rest/src/libs/decorators/Controller.tspackages/protocols-rest/src/libs/decorators/HttpMethod.tspackages/protocols-rest/src/libs/decorators/Params.tspackages/protocols-rest/src/libs/decorators/ResponseSchema.tspackages/protocols-rest/src/libs/types.tspackages/protocols-rest/src/libs/types/RouteContract.tspackages/protocols-rest/src/libs/types/index.tspackages/protocols-rest/src/tests/RouteContractTypes.spec.tspackages/protocols-rest/src/tests/decorators/Route.spec.tspackages/rpc-codegen/src/tests/codegen.spec.tspublic-api-surface.snapshot.json
fdbb95f to
1a01621
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
๐ค 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/projectMap.spec.ts`:
- Around line 1-12: Reorder the imports to follow the correct hierarchy:
external packages like vitest should come first, followed by `@croco/`* package
imports, and finally relative imports from "../commands/projectMap.js".
Additionally, within the relative import statement, separate the type imports
(ProjectMapDirent, ProjectMapIo, ProjectMapPackage) into a dedicated type import
statement, keeping the value imports (createProjectMapManifest, runProjectMap,
stringifyProjectMapManifest) in their own separate import statement.
๐ช 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: 539bb2da-beff-41bc-af26-1ed23ff08b9a
๐ Files selected for processing (5)
packages/cli/src/tests/contractsCheck.spec.tspackages/cli/src/tests/contractsDiff.spec.tspackages/cli/src/tests/projectMap.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/spa-be-split/README.md.hbs
๐ค Files with no reviewable changes (2)
- packages/create-croco-app/src/tests/templates-build.spec.ts
- packages/create-croco-app/templates/spa-be-split/README.md.hbs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
๐ค 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/projectMap.spec.ts`:
- Around line 1-12: Reorder the imports to follow the correct hierarchy:
external packages like vitest should come first, followed by `@croco/`* package
imports, and finally relative imports from "../commands/projectMap.js".
Additionally, within the relative import statement, separate the type imports
(ProjectMapDirent, ProjectMapIo, ProjectMapPackage) into a dedicated type import
statement, keeping the value imports (createProjectMapManifest, runProjectMap,
stringifyProjectMapManifest) in their own separate import statement.
๐ช 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: 539bb2da-beff-41bc-af26-1ed23ff08b9a
๐ Files selected for processing (5)
packages/cli/src/tests/contractsCheck.spec.tspackages/cli/src/tests/contractsDiff.spec.tspackages/cli/src/tests/projectMap.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/spa-be-split/README.md.hbs
๐ค Files with no reviewable changes (2)
- packages/create-croco-app/src/tests/templates-build.spec.ts
- packages/create-croco-app/templates/spa-be-split/README.md.hbs
๐ Comments failed to post (1)
packages/cli/src/tests/projectMap.spec.ts (1)
1-12:
โ ๏ธ Potential issue | ๐ Major๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash # ํ์ผ ์๋จ import ์ ์ธ ํ์ธ sed -n '1,30p' packages/cli/src/tests/projectMap.spec.ts # ๊ฐ import์ inline type specifier๊ฐ ์์ฌ ์๋์ง ํ์ธ rg -nP '^\s*import\s*\{[^}]*\btype\s+[A-Za-z_]\w*' packages/cli/src/tests/projectMap.spec.tsRepository: croco-dev/framework
Length of output: 1322
Import ์ ๋ ฌ ์์์ ํ์ import ๋ถ๋ฆฌ๋ฅผ ์์ ํ์ธ์.
ํ์ฌ
@croco/*ํ์ import๊ฐ ์ธ๋ถ ํจํค์งvitest๋ณด๋ค ๋จผ์ ์ ์ธ๋์ด ์๊ณ , ์๋๊ฒฝ๋ก import์์ ํ์ ๊ณผ ๊ฐ์ด ์์ฌ ์์ต๋๋ค. ์ฝ๋ฉ ๊ฐ์ด๋์ ๋ฐ๋ผ ์ธ๋ถ ํจํค์ง โ@croco/*โ ์๋๊ฒฝ๋ก ์์๋ก ์ ๋ ฌํ๊ณ , ํ์ import๋ฅผ ๋ณ๋ ์ ์ธ์ผ๋ก ๋ถ๋ฆฌํ์ธ์.์์ ์
-import type { PolicyTable, RuntimeCapabilityName } from "`@croco/framework-context`"; -import type { FrameworkManifest } from "`@croco/framework-routes`"; -import type { ContractDiagnostic, ContractGraphSnapshot } from "`@croco/protocols-core`"; import { describe, expect, it } from "vitest"; +import type { PolicyTable, RuntimeCapabilityName } from "`@croco/framework-context`"; +import type { FrameworkManifest } from "`@croco/framework-routes`"; +import type { ContractDiagnostic, ContractGraphSnapshot } from "`@croco/protocols-core`"; +import type { + ProjectMapDirent, + ProjectMapIo, + ProjectMapPackage, +} from "../commands/projectMap.js"; import { createProjectMapManifest, runProjectMap, stringifyProjectMapManifest, - type ProjectMapDirent, - type ProjectMapIo, - type ProjectMapPackage, } from "../commands/projectMap.js";๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { describe, expect, it } from "vitest"; import type { PolicyTable, RuntimeCapabilityName } from "`@croco/framework-context`"; import type { FrameworkManifest } from "`@croco/framework-routes`"; import type { ContractDiagnostic, ContractGraphSnapshot } from "`@croco/protocols-core`"; import type { ProjectMapDirent, ProjectMapIo, ProjectMapPackage, } from "../commands/projectMap.js"; import { createProjectMapManifest, runProjectMap, stringifyProjectMapManifest, } from "../commands/projectMap.js";๐ค 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/cli/src/tests/projectMap.spec.ts` around lines 1 - 12, Reorder the imports to follow the correct hierarchy: external packages like vitest should come first, followed by `@croco/`* package imports, and finally relative imports from "../commands/projectMap.js". Additionally, within the relative import statement, separate the type imports (ProjectMapDirent, ProjectMapIo, ProjectMapPackage) into a dedicated type import statement, keeping the value imports (createProjectMapManifest, runProjectMap, stringifyProjectMapManifest) in their own separate import statement.Source: Coding guidelines
Fixes #974.
Summary
REST route contracts can now drive controller decorators directly: HTTP method decorators accept matching
RouteContractSpecobjects, and@Param,@Query,@Body, and@ResponseSchemacan bind from the same contract.The contract graph preserves route contract id, operation id, source location, and input/output schemas, then reports drift when controller path, method, path/query bindings, body schema, or response metadata diverges from the contract. OpenAPI and RPC generation both handle contract-first routes.
The SPA split REST template now declares typed route contracts as the source of truth for its user routes, with updated guide/API docs, public API snapshots, tests, and a patch changeset.
Verification
pnpm --filter @croco/protocols-rest exec vitest run src/tests/RouteContractTypes.spec.ts src/tests/decorators/Route.spec.ts- passed, 19 tests.pnpm --filter @croco/protocols-core exec vitest run src/tests/extractRouteIR.spec.ts src/tests/ContractGraph.spec.ts- passed, 36 tests.pnpm --filter @croco/openapi-spec exec vitest run src/tests/emitOpenAPI.spec.ts- passed, 19 tests.pnpm --filter @croco/rpc-codegen exec vitest run src/tests/codegen.spec.ts- passed, 33 tests.pnpm --filter create-croco-app exec vitest run src/tests/templates-build.spec.ts src/tests/e2e-generation.spec.ts- passed, 16 tests.pnpm --filter @croco/protocols-rest test- passed, 124 tests.pnpm --filter @croco/protocols-core test- passed, 37 tests.pnpm --filter @croco/openapi-spec test- passed, 28 tests.pnpm --filter @croco/rpc-codegen test- passed, 59 tests.pnpm create-croco-app:smoke- passed; all generated app smoke cases passed.pnpm typecheck- passed, 202/202 Turbo tasks.pnpm test- passed, 203/203 Turbo tasks.pnpm check- passed.pnpm changeset-required:check -- --base origin/trunk --head HEAD- passed.pnpm docs:examples:checkandpnpm docs:catalog:check- passed.git diff --checkandgit diff --cached --check- passed.oxlintandoxfmt.pnpm testpassed with 203/203 Turbo tasks, and fullpnpm typecheckpassed with 202/202 Turbo tasks.Self-review gates
Independent review
An independent review found two documentation drift issues before the PR was opened: a stale
defineRouteSchema()reference in the guide and an incomplete README API list. Both were fixed, and the docs/static checks were rerun successfully.Risk
Low-medium. This adds public contract-first decorator overloads and stricter diagnostics for contract-first routes while preserving the existing loose decorator path for compatibility.
Summary by CodeRabbit
๋ฆด๋ฆฌ์ค ๋ ธํธ