feat: align http request schema with group spec proto JSON representation#3190
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughBid-screening schemas across API and provider-inventory apps are updated to validate resource quantities as bigint-transforming strings (supporting both plain unsigned-integer and base64-encoded formats) and make resource attributes optional. Downstream mappers default optional attributes to empty arrays and introduce CPU scaling via a configurable multiplier. The BidScreeningResult type removes region and uptime fields in favor of an isAudited flag, with the service updated accordingly. ChangesBid-screening schema and result flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3190 +/- ##
==========================================
- Coverage 64.00% 63.24% -0.77%
==========================================
Files 1100 1059 -41
Lines 26702 25675 -1027
Branches 6468 6306 -162
==========================================
- Hits 17090 16237 -853
+ Misses 8413 8249 -164
+ Partials 1199 1189 -10
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
apps/provider-inventory/src/http-schemas/bid-screening.schema.ts (1)
99-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResponse schema/type mismatch:
regionanduptime7dfields.Same issue as the API schema. These fields should be removed to match the updated
BidScreeningResulttype that only includesowner,hostUri, andisAudited.🤖 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 `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts` around lines 99 - 105, ProviderResultSchema currently includes extra fields region and uptime7d that no longer exist on the BidScreeningResult type; open the definition of ProviderResultSchema and remove the region and uptime7d properties so the schema only defines owner, hostUri, and isAudited (preserving their existing openapi metadata), and ensure any OpenAPI/exports referencing ProviderResultSchema reflect the reduced shape.apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts (1)
100-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
regionanduptime7dfromProviderResultSchema.The schema declares these fields as nullable, but
BidScreeningResultand the#toResult()method no longer populate them. This mismatch will cause response validation to fail (if enabled) or OpenAPI documentation to misrepresent actual responses.🤖 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 `@apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts` around lines 100 - 106, ProviderResultSchema currently declares region and uptime7d but BidScreeningResult and the BidScreeningResult#toResult() method no longer populate them; remove the region and uptime7d properties from the ProviderResultSchema object (and any openapi metadata for them) so the schema matches the actual responses and OpenAPI output, then run a quick search for ProviderResultSchema usages to ensure no callers expect those fields and update any tests/docs that referenced them.
🧹 Nitpick comments (2)
apps/provider-inventory/src/http-schemas/bid-screening.schema.ts (1)
68-68: 💤 Low valueRedundant double
.optional()Same issue as in the API schema—remove the duplicate.
- endpoints: z.array(z.unknown()).optional().optional() + endpoints: z.array(z.unknown()).optional()🤖 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 `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts` at line 68, The "endpoints" property in the schema is calling .optional() twice (endpoints: z.array(z.unknown()).optional().optional()); remove the redundant second .optional() so it becomes a single optional call on the z.array(z.unknown()) for the endpoints field in the bid-screening.schema (look for the endpoints property in the schema definition).apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts (1)
69-69: 💤 Low valueRedundant double
.optional()The
.optional().optional()chain is redundant—single.optional()suffices.- endpoints: z.array(z.unknown()).optional().optional() + endpoints: z.array(z.unknown()).optional()🤖 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 `@apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts` at line 69, The schema for the "endpoints" property in bid-screening.schema.ts currently chains .optional() twice on the field definition (endpoints: z.array(z.unknown()).optional().optional()); remove the redundant second .optional() so the field reads endpoints: z.array(z.unknown()).optional(), leaving a single .optional() on the array schema to achieve the intended optional behavior while keeping the definition clean.
🤖 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 `@apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts`:
- Around line 100-106: ProviderResultSchema currently declares region and
uptime7d but BidScreeningResult and the BidScreeningResult#toResult() method no
longer populate them; remove the region and uptime7d properties from the
ProviderResultSchema object (and any openapi metadata for them) so the schema
matches the actual responses and OpenAPI output, then run a quick search for
ProviderResultSchema usages to ensure no callers expect those fields and update
any tests/docs that referenced them.
In `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts`:
- Around line 99-105: ProviderResultSchema currently includes extra fields
region and uptime7d that no longer exist on the BidScreeningResult type; open
the definition of ProviderResultSchema and remove the region and uptime7d
properties so the schema only defines owner, hostUri, and isAudited (preserving
their existing openapi metadata), and ensure any OpenAPI/exports referencing
ProviderResultSchema reflect the reduced shape.
---
Nitpick comments:
In `@apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts`:
- Line 69: The schema for the "endpoints" property in bid-screening.schema.ts
currently chains .optional() twice on the field definition (endpoints:
z.array(z.unknown()).optional().optional()); remove the redundant second
.optional() so the field reads endpoints: z.array(z.unknown()).optional(),
leaving a single .optional() on the array schema to achieve the intended
optional behavior while keeping the definition clean.
In `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts`:
- Line 68: The "endpoints" property in the schema is calling .optional() twice
(endpoints: z.array(z.unknown()).optional().optional()); remove the redundant
second .optional() so it becomes a single optional call on the
z.array(z.unknown()) for the endpoints field in the bid-screening.schema (look
for the endpoints property in the schema definition).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08d31ffa-3ca9-4759-a1ec-c0399c9cbad0
📒 Files selected for processing (6)
apps/api/src/bid-screening/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/lib/groupspec-mapper/groupspec-mapper.tsapps/provider-inventory/src/lib/stream-status-mapper/stream-status-mapper.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.tsapps/provider-inventory/src/types/inventory.types.ts
💤 Files with no reviewable changes (1)
- apps/provider-inventory/src/types/inventory.types.ts
ygrishajev
approved these changes
May 18, 2026
stalniy
force-pushed
the
feat/request-schema
branch
from
27424fc to
e922823
Compare
Contributor
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
`@apps/provider-inventory/src/lib/stream-status-mapper/stream-status-mapper.ts`:
- Around line 69-70: The bug is that CPU scaling (multiplier 1000n) is applied
after parseQuantity truncates fractional values in pairFromSdk, losing millicore
precision; fix by moving the scaling into parseQuantity (or adding a scale
parameter) so the exponent shift happens before integer division/truncation.
Update parseQuantity to accept an optional multiplier (e.g., multiplier = 1n)
and apply it before any integer truncation, then change pairFromSdk to call
parseQuantity(pair?.allocatable, 1000n) and parseQuantity(pair?.allocated,
1000n) (and the analogous call at the other site mentioned) so ResourcePair gets
correctly scaled values without losing fractional millicore precision.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 417b8ed7-c3dd-4b4e-9739-81e70bbcbadf
📒 Files selected for processing (6)
apps/api/src/bid-screening/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/lib/groupspec-mapper/groupspec-mapper.tsapps/provider-inventory/src/lib/stream-status-mapper/stream-status-mapper.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.tsapps/provider-inventory/src/types/inventory.types.ts
💤 Files with no reviewable changes (1)
- apps/provider-inventory/src/types/inventory.types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/provider-inventory/src/lib/groupspec-mapper/groupspec-mapper.ts
- apps/provider-inventory/src/services/bid-screening/bid-screening.service.ts
- apps/provider-inventory/src/http-schemas/bid-screening.schema.ts
- apps/api/src/bid-screening/http-schemas/bid-screening.schema.ts
stalniy
force-pushed
the
feat/request-schema
branch
from
e922823 to
8083857
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/provider-inventory/src/http-schemas/bid-screening.schema.ts (2)
68-68: 💤 Low valueRedundant
.optional().optional()chaining.The double
.optional()has no effect beyond the first call.- endpoints: z.array(z.unknown()).optional().optional() + endpoints: z.array(z.unknown()).optional()🤖 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 `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts` at line 68, The schema definition for the "endpoints" field in bid-screening.schema.ts is calling .optional() twice on z.array(z.unknown()) (symbol: endpoints), which is redundant; remove the duplicate .optional() so the field is declared as z.array(z.unknown()).optional() to keep intent the same and eliminate the unnecessary chaining.
9-18: 💤 Low value
NaNas sentinel works but the type predicate is misleading.The transform returns
NaN(anumber) on parse failure, then the refine checkstypeof val === "bigint". This works but the type annotation(val): val is bigintclaims the parameter is already a bigint when it could beNaN. Consider returningnullor throwing during transform for clearer semantics, or adjust the type signature.Cleaner alternative using null sentinel
.transform(str => { if (/^\d+$/.test(str)) return BigInt(str); const parsed = Buffer.from(str, "base64").toString("utf-8"); if (/^\d+$/.test(parsed)) return BigInt(parsed); - return NaN; + return null; }) .refine( - (val): val is bigint => !Number.isFinite(val) && typeof val === "bigint" && val >= 0n, + (val): val is bigint => val !== null && val >= 0n, "Must be a non-negative integer or its protobuf base64-encoded representation" )🤖 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 `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts` around lines 9 - 18, The transform currently returns NaN on parse failure but the refine type predicate (val): val is bigint is misleading because NaN is a number; change the transform to return null (or throw) on failure instead of NaN and update the refine to validate that val is a bigint and non-negative (e.g., (val): val is bigint => typeof val === "bigint" && val >= 0n), keeping the same error message; adjust any downstream code expecting NaN to handle null (or rely on thrown errors) and ensure the .transform and .refine calls in the bid-screening.schema (the BigInt/base64 parsing logic) are updated consistently.
🤖 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.
Nitpick comments:
In `@apps/provider-inventory/src/http-schemas/bid-screening.schema.ts`:
- Line 68: The schema definition for the "endpoints" field in
bid-screening.schema.ts is calling .optional() twice on z.array(z.unknown())
(symbol: endpoints), which is redundant; remove the duplicate .optional() so the
field is declared as z.array(z.unknown()).optional() to keep intent the same and
eliminate the unnecessary chaining.
- Around line 9-18: The transform currently returns NaN on parse failure but the
refine type predicate (val): val is bigint is misleading because NaN is a
number; change the transform to return null (or throw) on failure instead of NaN
and update the refine to validate that val is a bigint and non-negative (e.g.,
(val): val is bigint => typeof val === "bigint" && val >= 0n), keeping the same
error message; adjust any downstream code expecting NaN to handle null (or rely
on thrown errors) and ensure the .transform and .refine calls in the
bid-screening.schema (the BigInt/base64 parsing logic) are updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5456d760-42e5-400e-a539-e798804309e2
📒 Files selected for processing (8)
apps/provider-inventory/package.jsonapps/provider-inventory/src/http-schemas/bid-screening.schema.tsapps/provider-inventory/src/lib/groupspec-mapper/groupspec-mapper.spec.tsapps/provider-inventory/src/lib/groupspec-mapper/groupspec-mapper.tsapps/provider-inventory/src/lib/stream-status-mapper/stream-status-mapper.spec.tsapps/provider-inventory/src/lib/stream-status-mapper/stream-status-mapper.tsapps/provider-inventory/src/services/bid-screening/bid-screening.service.spec.tsapps/provider-inventory/src/types/inventory.types.ts
✅ Files skipped from review due to trivial changes (1)
- apps/provider-inventory/package.json
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
2 participants
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.
Why
Ref CON-347
What
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores