feat(server): add specific 4xx error responses to REST OpenAPI spec

[ymc9]

Summary

• Replace generic "Error" descriptions with specific messages for 400, 403, 404, and 422 responses in the REST OpenAPI spec
• Add respectAccessPolicies option to OpenApiSpecOptions that inspects @@allow/@@deny rules and conditionally includes 403 responses on mutation operations (create, update, delete, relationship changes) when access is not unconditionally allowed
• Relationship mutation paths (PUT/PATCH/POST) also receive conditional 403 based on the parent model's update policy

Test plan

• All 32 existing REST OpenAPI tests pass
• 8 new tests for respectAccessPolicies covering:
  • Off by default (no 403 without the flag)
  • Non-constant-allow policies → 403
  • Constant @@allow('all', true) → no 403
  • @@deny rule overrides constant allow → 403
  • No rules at all (default-deny) → 403
  • Per-operation granularity (mix of constant and non-constant)
  • Relationship mutations get 403 when parent update may be denied
  • Relationship mutations have no 403 when parent update is constant-allow

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • OpenAPI specs can optionally include HTTP 403 responses to indicate operations that may be denied by access policies.

• Improvements

  • Error response documentation expanded and clarified with specific status codes (400, 403, 404, 422) and clearer descriptions across endpoints.

• Tests

  • Added comprehensive tests validating OpenAPI output includes or omits 403 responses according to policy conditions.

[claude]

Claude finished @ymc9's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed this PR that adds specific 4xx error responses to the REST OpenAPI spec. Here's my comprehensive analysis:

Review Tasks:

• Analyze core implementation changes in openapi.ts
• Review new type definitions in types.ts
• Examine test coverage and quality
• Assess for potential bugs and security issues
• Evaluate performance considerations
• Review baseline changes for correctness

✅ Strengths

1. Well-Designed API Enhancement

• Clear separation of concerns: The new respectAccessPolicies option is opt-in and doesn't affect existing functionality
• Specific error messages: Replaces generic "Error" with meaningful descriptions:
  • 400: "Error occurred while processing the request"
  • 403: "Forbidden: insufficient permissions to perform this operation"
  • 404: "Resource not found"
  • 422: "Operation is unprocessable due to validation errors"

2. Robust Policy Analysis Logic

The mayDenyAccess() method at lines 1048-1084 is well-implemented:

• ✅ Correctly handles default-deny when no policies exist
• ✅ Properly analyzes @@allow and @@deny rules
• ✅ Distinguishes between constant true conditions and dynamic expressions
• ✅ Handles operation-specific and 'all' operation matching

3. Comprehensive Test Coverage

The new tests (lines 666-855) cover all critical scenarios:

• ✅ Feature off by default
• ✅ Non-constant policies → 403 responses
• ✅ Constant allow (@@allow('all', true)) → no 403
• ✅ Deny rules override constant allow → 403
• ✅ Default-deny behavior → 403
• ✅ Per-operation granularity
• ✅ Relationship mutation policies based on parent model's update policy

4. Clean Implementation Details

• Consistent error constant usage: ERROR_400, ERROR_403, etc. defined once and reused
• Proper conditional inclusion: Uses spread operator pattern ...(condition && { '403': ERROR_403 })
• Maintains backwards compatibility: All existing tests still pass

🔍 Areas for Consideration

1. Minor: Error Message Consistency ⚠️

The 400 error message "Error occurred while processing the request" is quite generic. Consider if it could be more specific, though this might be intentional for security reasons.

2. Documentation Enhancement 📖

The new respectAccessPolicies option has good inline documentation, but consider adding examples to help users understand when to enable it.

3. Performance Impact 🚀

The policy analysis in mayDenyAccess() runs for every operation when respectAccessPolicies is enabled. This is acceptable since:

• It's opt-in behavior
• The analysis is lightweight (just parsing model attributes)
• It runs at spec generation time, not request time

🔧 Code Quality Assessment

Type Safety ✅

• Proper TypeScript usage throughout
• New OpenApiSpecOptions interface correctly typed
• Attribute analysis uses proper type guards

Error Handling ✅

• Graceful handling of missing/malformed policy attributes
• Safe access patterns with optional chaining and nullish coalescing

Security ✅

• No sensitive information exposed in error responses
• Policy analysis respects the schema's access control design
• Feature is opt-in, preventing unintended exposure

Architecture ✅

• Changes follow existing patterns in the codebase
• Clean separation between policy analysis and OpenAPI generation
• Consistent with JSON:API error response format

📊 Testing Quality

The test coverage is exemplary:

• 8 new focused tests for the new feature
• All 32 existing tests still pass
• Edge cases covered: no policies, mixed policies, relationship mutations
• Realistic scenarios: Tests use actual ZModel schemas with policy rules

✅ Overall Assessment

This is a high-quality PR that:

• ✅ Adds valuable functionality for API documentation accuracy
• ✅ Maintains full backwards compatibility
• ✅ Includes comprehensive test coverage
• ✅ Follows established code patterns and conventions
• ✅ Provides clear, actionable error messages for API consumers

The implementation is well-thought-out, thoroughly tested, and ready for merge. The respectAccessPolicies feature will help users generate more accurate OpenAPI specs that reflect their actual access control policies.

Recommendation: ✅ Approve and merge

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 808d6ff9-cbb3-4b9e-872d-afdd41fbbfa0

📥 Commits

Reviewing files that changed from the base of the PR and between 3579df5 and d2d3615.

📒 Files selected for processing (2)

• packages/server/src/api/rest/openapi.ts
• packages/server/test/openapi/rest-openapi.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/server/test/openapi/rest-openapi.test.ts

---

📝 Walkthrough

Walkthrough

Adds an optional respectAccessPolicies option to OpenAPI spec generation. The generator now records spec options, refactors error-response handling into status-specific constants, evaluates model-level access policies (via ModelDef) and conditionally includes 403 responses for operations that may be denied.

Changes

Cohort / File(s)	Summary
Type Configuration packages/server/src/api/common/types.ts	Added optional respectAccessPolicies?: boolean to OpenApiSpecOptions.
OpenAPI Generator Core packages/server/src/api/rest/openapi.ts	Replaced generic error constant with status-specific error response helpers; added specOptions state; changed path builder/operation signatures to accept ModelDef; added mayDenyAccess() to conditionally include 403 for create/update/delete and relationship-mutating operations; updated responses to include appropriate 400/404/422 mappings.
OpenAPI Specification Baseline packages/server/test/openapi/baseline/rest.baseline.yaml	Updated error response descriptions and added explicit 422 responses and refined 400/404/403 response entries with JSON:API content and _errorResponse schema where applicable.
Access Policy Tests packages/server/test/openapi/rest-openapi.test.ts	Added respectAccessPolicies test suite covering many policy scenarios: absence of option, non-constant @@allow, constant-true @@allow, @@deny precedence, literal @@deny(..., false) as no-op, default-deny behavior, per-operation allow granularity, and relationship mutation endpoints receiving 403 when applicable.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

🐇 I hopped through specs both near and far,

I added checks for when policies spar.
403s glitter where access may fail,
Responses now tidy, precise as a trail.
A rabbit-approved change — hop, ship, and sail!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding specific 4xx error responses to the REST OpenAPI spec, including the new 403 response handling.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/rest-openapi-error-responses

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/server/test/openapi/rest-openapi.test.ts (1)

733-751: Add one regression case for constant-false deny

Please add a case like @@allow('all', true) + @@deny('all', false) asserting no 403, to lock in unconditional-allow behavior when deny is statically impossible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/server/test/openapi/rest-openapi.test.ts` around lines 733 - 751,
Add a regression test that verifies a statically-impossible deny does not
produce a 403: create a policySchema with @@allow('all', true) and @@deny('all',
false), use createTestClient(...) to get the schema, instantiate
RestApiHandler({ schema: client.$schema, endpoint: 'http://localhost/api' }),
call await h.generateSpec({ respectAccessPolicies: true }) and assert that the
generated spec does not include a 403 response (i.e., expect
s.paths?.['/item']?.post?.responses?.['403'] toBeUndefined()). This mirrors the
existing test that checks deny overrides, but ensures constant-false deny is
ignored when allow is unconditional.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/server/src/api/rest/openapi.ts`:
- Around line 1068-1072: The code currently treats any matching @@deny attribute
as a deny regardless of its condition; update the logic in mayDenyAccess to
ignore @@deny entries whose condition is a literal false and only treat a deny
as effective when its condition evaluates truthy (or when there are no
args/unconditional). Concretely: replace the simple relevantDeny.length>0 check
with iterating relevantDeny, skip entries where attr.args is a literal boolean
false, and for the rest evaluate the condition using the same
evaluation/resolution used elsewhere (the same mechanism as matchesOperation or
the policy condition evaluator used for allows); return true only if any
evaluated deny condition yields true (or if an entry has no args), otherwise
return false.

---

Nitpick comments:
In `@packages/server/test/openapi/rest-openapi.test.ts`:
- Around line 733-751: Add a regression test that verifies a
statically-impossible deny does not produce a 403: create a policySchema with
@@allow('all', true) and @@deny('all', false), use createTestClient(...) to get
the schema, instantiate RestApiHandler({ schema: client.$schema, endpoint:
'http://localhost/api' }), call await h.generateSpec({ respectAccessPolicies:
true }) and assert that the generated spec does not include a 403 response
(i.e., expect s.paths?.['/item']?.post?.responses?.['403'] toBeUndefined()).
This mirrors the existing test that checks deny overrides, but ensures
constant-false deny is ignored when allow is unconditional.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 745cef07-df2e-4fb2-81b8-b900e689163f

📥 Commits

Reviewing files that changed from the base of the PR and between 41ea0c9 and 3579df5.

📒 Files selected for processing (4)

• packages/server/src/api/common/types.ts
• packages/server/src/api/rest/openapi.ts
• packages/server/test/openapi/baseline/rest.baseline.yaml
• packages/server/test/openapi/rest-openapi.test.ts

[claude]

Claude finished @ymc9's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed this PR that adds specific 4xx error responses to the REST OpenAPI spec. Here's my comprehensive analysis:

Review Tasks:

• Analyze core implementation changes in openapi.ts
• Review new type definitions in types.ts
• Examine test coverage and quality
• Assess CodeRabbit's finding about @@deny rule handling
• Evaluate performance considerations
• Review security implications
• Check baseline changes for correctness

✅ Strengths

1. Well-Designed API Enhancement

• Clear separation of concerns: The new respectAccessPolicies option is opt-in and doesn't affect existing functionality
• Specific error messages: Replaces generic "Error" with meaningful descriptions:
  • 400: "Error occurred while processing the request"
  • 403: "Forbidden: insufficient permissions to perform this operation"
  • 404: "Resource not found"
  • 422: "Operation is unprocessable due to validation errors"

2. Robust Policy Analysis Logic

The mayDenyAccess() method at lines 1048-1087 is well-implemented:

• ✅ Correctly handles default-deny when no policies exist
• ✅ Properly analyzes @@allow and @@deny rules
• ✅ Distinguishes between constant true conditions and dynamic expressions
• ✅ FIXED: CodeRabbit's finding is already addressed - The code correctly ignores @@deny rules with literal false conditions (lines 1071-1072)
• ✅ Handles operation-specific and 'all' operation matching

3. Comprehensive Test Coverage

The new tests (lines 666-875) cover all critical scenarios:

• ✅ Feature off by default
• ✅ Non-constant policies → 403 responses
• ✅ Constant allow (@@allow('all', true)) → no 403
• ✅ Deny rules override constant allow → 403
• ✅ Constant-false deny is properly handled - Test at lines 753-771 verifies @@deny('create', false) doesn't add 403
• ✅ Default-deny behavior → 403
• ✅ Per-operation granularity
• ✅ Relationship mutation policies based on parent model's update policy

4. Clean Implementation Details

• Consistent error constant usage: ERROR_400, ERROR_403, etc. defined once and reused
• Proper conditional inclusion: Uses spread operator pattern ...(condition && { '403': ERROR_403 })
• Maintains backwards compatibility: All existing tests still pass

🔍 Areas for Consideration

1. Minor: Error Message Consistency ⚠️

The 400 error message "Error occurred while processing the request" is quite generic. Consider if it could be more specific, though this might be intentional for security reasons.

2. Documentation Enhancement 📖

The new respectAccessPolicies option has good inline documentation, but consider adding examples to help users understand when to enable it.

3. Performance Impact 🚀

The policy analysis in mayDenyAccess() runs for every operation when respectAccessPolicies is enabled. This is acceptable since:

• It's opt-in behavior
• The analysis is lightweight (just parsing model attributes)
• It runs at spec generation time, not request time

🔧 Code Quality Assessment

Type Safety ✅

• Proper TypeScript usage throughout
• New OpenApiSpecOptions interface correctly typed
• Attribute analysis uses proper type guards

Error Handling ✅

• Graceful handling of missing/malformed policy attributes
• Safe access patterns with optional chaining and nullish coalescing

Security ✅

• No sensitive information exposed in error responses
• Policy analysis respects the schema's access control design
• Feature is opt-in, preventing unintended exposure

Architecture ✅

• Changes follow existing patterns in the codebase
• Clean separation between policy analysis and OpenAPI generation
• Consistent with JSON:API error response format

📊 Testing Quality

The test coverage is exemplary:

• 8 new focused tests for the new feature
• All 32 existing tests still pass
• Edge cases covered: no policies, mixed policies, relationship mutations
• Realistic scenarios: Tests use actual ZModel schemas with policy rules

✅ Overall Assessment

This is a high-quality PR that:

• ✅ Adds valuable functionality for API documentation accuracy
• ✅ Maintains full backwards compatibility
• ✅ Includes comprehensive test coverage
• ✅ Follows established code patterns and conventions
• ✅ Provides clear, actionable error messages for API consumers

The implementation is well-thought-out, thoroughly tested, and ready for merge. The respectAccessPolicies feature will help users generate more accurate OpenAPI specs that reflect their actual access control policies.

Recommendation: ✅ Approve and merge

---