Closed
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdded per-feature Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTP as "Features Handler"
participant Conv as "Converter"
participant Conn as "Feature Connector"
participant LLM as "LLM Cost Service"
participant Resp as "API Response"
Client->>HTTP: GET /features/:id
HTTP->>HTTP: resolve namespace
HTTP->>Conn: GetFeature(namespace, idOrKey)
Conn-->>HTTP: domain Feature (may include UnitCost)
HTTP->>Conv: convertFeatureToAPI(domain Feature)
Conv-->>HTTP: api.Feature
alt unit_cost.type == "llm"
HTTP->>LLM: ResolvePrice(provider, model)
LLM-->>HTTP: FeatureLLMUnitCostPricing
HTTP->>Conv: enrichFeatureResponseWithPricing(api.Feature, pricing)
Conv-->>HTTP: enriched api.Feature
end
HTTP->>Resp: write JSON 200 OK
Resp-->>Client: HTTP 200 + feature payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/features/convert.go`:
- Around line 19-28: The API is advertising a Description field but the
converters drop it: when building api.Feature you set Description: nil and
CreateFeatureRequest.description isn't propagated into the internal feature
model. Fix by wiring the description through both directions: in the request ->
internal conversion copy CreateFeatureRequest.Description into the internal
feature struct, and in this response construction set api.Feature.Description to
the internal value (e.g., Description: f.Description). Update any other similar
blocks (the other occurrence around lines 56-62) to mirror this change so
create/get/list round-trip descriptions.
- Around line 30-43: The handler only sets resp.Meter.Key from f.MeterSlug and
drops any provided meter ID; update the mapping to also set resp.Meter.Id when
f.MeterSlug contains an ID (i.e., copy the ID into resp.Meter.Id alongside Key
and Filters) so create requests that reference a meter by id are preserved and
reads can return meter.id; apply the same fix to the second similar block (the
one around lines 64-72) and ensure the source struct (f.MeterSlug) and helper
convertFiltersToAPI usage remain unchanged.
🪄 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: 3571384f-9a2f-414d-bba8-f28105716559
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (13)
api/spec/packages/aip/src/features/feature.tspapi/spec/packages/aip/src/features/index.tspapi/spec/packages/aip/src/features/unitcost.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/create.goapi/v3/handlers/features/delete.goapi/v3/handlers/features/error_encoder.goapi/v3/handlers/features/get.goapi/v3/handlers/features/handler.goapi/v3/handlers/features/list.goapi/v3/server/routes.goapi/v3/server/server.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/v3/handlers/features/convert_test.go (2)
497-520: Filter conversion coverage is a bit narrow here.This test set checks Eq/In/Ne/Exists but skips
Nin(which is mapped) and doesn’t assert behavior for unsupported operators, so silent conversion loss can slip through unnoticed.As per coding guidelines "
**/*_test.go: Make sure the tests are comprehensive and cover the changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert_test.go` around lines 497 - 520, The tests for TestConvertFilterStringToAPIMapItem are missing coverage for the Nin operator and for behavior when unsupported operators are provided; update the test to add a subtest that constructs filter.FilterString{Nin: &[]string{"x","y"}} and assert the returned result.Nin matches, and add another subtest that passes an unsupported/empty operator (or a struct with an unexpected field) and asserts the conversion either yields nil/zero values or returns the expected error/value according to convertFilterStringToAPIMapItem’s contract; reference convertFilterStringToAPIMapItem and TestConvertFilterStringToAPIMapItem when adding these assertions so silent conversion loss is caught.
97-150: Please add error-path tests forconvertUnitCostFromAPI.This block only verifies happy paths. We should also cover malformed/unknown unit-cost payloads so parser/union regressions are caught early.
Suggested test additions
func TestConvertUnitCostFromAPI(t *testing.T) { + t.Run("unknown discriminator returns error", func(t *testing.T) { + var apiUC api.FeatureUnitCost + err := apiUC.FromFeatureUnknownUnitCostType(map[string]any{"type": "unknown"}) + require.NoError(t, err) + + _, err = convertUnitCostFromAPI(&apiUC) + require.Error(t, err) + }) + + t.Run("manual unit cost with invalid amount returns error", func(t *testing.T) { + var apiUC api.FeatureUnitCost + err := apiUC.FromFeatureManualUnitCost(api.FeatureManualUnitCost{ + Amount: "not-a-number", + }) + require.NoError(t, err) + + _, err = convertUnitCostFromAPI(&apiUC) + require.Error(t, err) + }) + t.Run("manual unit cost", func(t *testing.T) {As per coding guidelines "
**/*_test.go: Make sure the tests are comprehensive and cover the changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert_test.go` around lines 97 - 150, Add negative tests in convert_test.go that call convertUnitCostFromAPI and assert errors for malformed/unknown payloads: (1) a subtest that passes an empty api.FeatureUnitCost (no union set) and expects a non-nil error; (2) a subtest that uses apiUC.FromFeatureManualUnitCost with Amount set to an invalid string (e.g. "not-a-number") and expects convertUnitCostFromAPI to return an error; (3) a subtest that constructs an api.FeatureLLMUnitCost with both static fields and corresponding Property fields set (e.g. Provider and ProviderProperty) and expects convertUnitCostFromAPI to error on the conflicting LLM union. Reference convertUnitCostFromAPI and the api.FeatureUnitCost/FromFeatureManualUnitCost/FromFeatureLLMUnitCost helpers to locate where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/features/convert_test.go`:
- Around line 417-424: The test currently ignores errors from
uc.FromFeatureLLMUnitCost and resp.UnitCost.AsFeatureLLMUnitCost which can hide
setup/decoding failures; update the subtest to check and fail on those errors
(i.e., capture the returned error from FeatureUnitCost.FromFeatureLLMUnitCost
and from AsFeatureLLMUnitCost and call t.Fatalf or require.NoError if non-nil)
before calling enrichFeatureResponseWithPricing and asserting llm.Pricing,
referencing the FeatureUnitCost.FromFeatureLLMUnitCost, resp.UnitCost,
enrichFeatureResponseWithPricing, and resp.UnitCost.AsFeatureLLMUnitCost
symbols.
---
Nitpick comments:
In `@api/v3/handlers/features/convert_test.go`:
- Around line 497-520: The tests for TestConvertFilterStringToAPIMapItem are
missing coverage for the Nin operator and for behavior when unsupported
operators are provided; update the test to add a subtest that constructs
filter.FilterString{Nin: &[]string{"x","y"}} and assert the returned result.Nin
matches, and add another subtest that passes an unsupported/empty operator (or a
struct with an unexpected field) and asserts the conversion either yields
nil/zero values or returns the expected error/value according to
convertFilterStringToAPIMapItem’s contract; reference
convertFilterStringToAPIMapItem and TestConvertFilterStringToAPIMapItem when
adding these assertions so silent conversion loss is caught.
- Around line 97-150: Add negative tests in convert_test.go that call
convertUnitCostFromAPI and assert errors for malformed/unknown payloads: (1) a
subtest that passes an empty api.FeatureUnitCost (no union set) and expects a
non-nil error; (2) a subtest that uses apiUC.FromFeatureManualUnitCost with
Amount set to an invalid string (e.g. "not-a-number") and expects
convertUnitCostFromAPI to return an error; (3) a subtest that constructs an
api.FeatureLLMUnitCost with both static fields and corresponding Property fields
set (e.g. Provider and ProviderProperty) and expects convertUnitCostFromAPI to
error on the conflicting LLM union. Reference convertUnitCostFromAPI and the
api.FeatureUnitCost/FromFeatureManualUnitCost/FromFeatureLLMUnitCost helpers to
locate where to add these tests.
🪄 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: 095598ad-5ba3-4c04-b5a5-24278de44355
📒 Files selected for processing (2)
api/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.go
✅ Files skipped from review due to trivial changes (1)
- api/v3/handlers/features/convert.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/handlers/features/convert_test.go (1)
286-356: Please cover the LLM create-request path too.
convertCreateRequestToDomainonly gets manualunit_costcoverage here, even though this helper is whatCreateFeatureuses for the newllmrequest shapes. I’d add at least one property-ref case and one static-value case so both supported forms stay protected in CI.As per coding guidelines "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert_test.go` around lines 286 - 356, Add tests exercising the LLM unit_cost conversion in convertCreateRequestToDomain (the same helper used by CreateFeature): add one subtest where api.CreateFeatureRequest contains an LLM unit_cost using a property-ref form and one subtest with a static-value LLM form, then assert the returned feature.UnitCost.Type and the parsed values (e.g., that result.UnitCost.Type equals the LLM-specific enum and the property reference or static amount is populated correctly). Locate convertCreateRequestToDomain and the TestConvertCreateRequestToDomain function to add the new t.Run cases and reuse api.FeatureUnitCost/FromFeature... helpers to construct the request shapes so CI covers both supported LLM unit_cost variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/api.gen.go`:
- Around line 2177-2180: The generated comment for the UnitCost field is too
narrow; update the source API spec (the schema/description for FeatureUnitCost /
the UnitCost field) to state that "llm" pricing supports both dynamic
meter-based lookups and static forms using provider, model, and token_type, then
regenerate the API client to update api.gen.go (do not edit the generated file
directly). Ensure you change the FeatureUnitCost/UnitCost description in the
authoritative spec and re-run the codegen so all occurrences (the other
generated doc strings for UnitCost/FeatureUnitCost) are updated.
In `@api/v3/handlers/features/convert_test.go`:
- Around line 358-427: Add a test case that ensures
enrichFeatureResponseWithPricing is a no-op for non-LLM/manual unit costs:
create a Feature with a UnitCost that represents a manual/non-LLM pricing
variant (not an LLM unit cost), record its value, call
enrichFeatureResponseWithPricing(resp, somePricing) and assert the UnitCost
remains unchanged (e.g., deep-equal to the original or that converting to an LLM
unit cost does not expose Pricing). Update the helper
enrichFeatureResponseWithPricing to first check the unit-cost discriminator
(only proceed when the unit cost is an LLM type) before mutating resp.UnitCost
so non-LLM/manual unit costs are left untouched.
---
Nitpick comments:
In `@api/v3/handlers/features/convert_test.go`:
- Around line 286-356: Add tests exercising the LLM unit_cost conversion in
convertCreateRequestToDomain (the same helper used by CreateFeature): add one
subtest where api.CreateFeatureRequest contains an LLM unit_cost using a
property-ref form and one subtest with a static-value LLM form, then assert the
returned feature.UnitCost.Type and the parsed values (e.g., that
result.UnitCost.Type equals the LLM-specific enum and the property reference or
static amount is populated correctly). Locate convertCreateRequestToDomain and
the TestConvertCreateRequestToDomain function to add the new t.Run cases and
reuse api.FeatureUnitCost/FromFeature... helpers to construct the request shapes
so CI covers both supported LLM unit_cost variants.
🪄 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: ce476628-bf4b-45e2-9619-5f99225a3ae8
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (4)
api/spec/packages/aip/src/features/feature.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.go
✅ Files skipped from review due to trivial changes (1)
- api/v3/handlers/features/convert.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/packages/aip/src/features/feature.tsp
…fixes - Reference meters by required ID instead of key in v3 Feature API - Remove unsupported description field from Feature model - Resolve meter ID to slug in CreateFeature handler via meter service - Fix unnecessary type conversions flagged by linter - Fix test to check error returns instead of discarding them Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The FeatureLLMUnitCost and pricing field descriptions implied that only meter group-by properties were supported. Updated to accurately describe that both static values and property references work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/v3/api.gen.go (1)
2177-2180:⚠️ Potential issue | 🟡 MinorPlease widen the LLM unit-cost docs before regenerating.
These comments still read like
llmpricing only works through meter-property lookups, but the schema now also allows staticprovider,model, andtoken_typevalues. Please fix the source TypeSpec/OpenAPI description and regenerate so the generated Go docs and embedded spec stay aligned. As per coding guidelines "Never manually edit generated files marked with '// Code generated by X, DO NOT EDIT.' headers"Also applies to: 2309-2312, 2356-2358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 2177 - 2180, The comment on the generated UnitCost docs is outdated: update the source TypeSpec/OpenAPI description for the FeatureUnitCost/UnitCost property to reflect that "llm" pricing can derive cost either from meter-property lookups or from static provider/model/token_type values (and not only meter lookups), then regenerate the OpenAPI spec and re-run codegen so api.gen.go's UnitCost comment (for FeatureUnitCost) and the embedded spec remain synchronized; locate the TypeSpec that defines UnitCost/FeatureUnitCost and expand its description to mention both meter-property based lookup and the optional static provider/model/token_type fields before regenerating.
🧹 Nitpick comments (3)
api/v3/handlers/features/create.go (1)
35-46: Good meter resolution flow, but the error fromGetMeterByIDOrSlugcould use context.The meter ID-to-slug resolution is correctly implemented. One small thing: when
GetMeterByIDOrSlugfails, the raw error is returned. While the error encoder will handleMeterNotFoundError, wrapping it with context (e.g., "failed to resolve meter") would make debugging easier when other error types occur.Optional: Add context to meter resolution error
m, err := h.meterService.GetMeterByIDOrSlug(ctx, meter.GetMeterInput{ Namespace: ns, IDOrSlug: body.Meter.Id, }) if err != nil { - return CreateFeatureRequest{}, err + return CreateFeatureRequest{}, fmt.Errorf("failed to resolve meter: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/create.go` around lines 35 - 46, When GetMeterByIDOrSlug (via h.meterService.GetMeterByIDOrSlug) returns an error, wrap that error with contextual text before returning so callers can see "failed to resolve meter" plus the original error; update the error return in the meter resolution block (the branch that sets meterSlug) to wrap the err (e.g., using fmt.Errorf("failed to resolve meter %s: %w", body.Meter.Id, err) or errors.Wrap) and return the wrapped error instead of the raw err.api/v3/handlers/features/convert.go (2)
196-198: Silently ignoring the error fromFromFeatureLLMUnitCost.If
FromFeatureLLMUnitCostfails, the response'sUnitCostremains in an inconsistent state (the pricing was built but not applied). At minimum, consider logging this error for debugging purposes.Optional: Log conversion error
llmCost.Pricing = &apiPricing - _ = resp.UnitCost.FromFeatureLLMUnitCost(llmCost) + if err := resp.UnitCost.FromFeatureLLMUnitCost(llmCost); err != nil { + // Log but don't fail - pricing enrichment is best-effort + slog.Debug("failed to enrich feature with pricing", "error", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert.go` around lines 196 - 198, The call to FromFeatureLLMUnitCost is currently ignoring its error which can leave resp.UnitCost inconsistent after llmCost.Pricing = &apiPricing; change the code to capture the returned error from resp.UnitCost.FromFeatureLLMUnitCost(llmCost), log that error with context (including the feature/price info and any request identifiers) and handle it (return the error or set a safe fallback UnitCost) instead of discarding it; update the call site where FromFeatureLLMUnitCost and resp.UnitCost are used to handle the error path explicitly.
200-234: Silent error suppression inresolveLLMPricinghides potential issues.When
svc.ResolvePricefails (line 229), the function returnsnilwithout any indication of why. This is fine for "not found" cases, but database errors or validation failures are also swallowed. Consider logging at debug level to aid troubleshooting, especially since the context snippet showsResolvePricecan return both not-found errors and database errors.Optional: Add debug logging for pricing resolution failures
price, err := svc.ResolvePrice(ctx, llmcost.ResolvePriceInput{ Namespace: feat.Namespace, Provider: llmcost.Provider(provider), ModelID: model, }) if err != nil { + // Log at debug level - pricing lookup failures are not critical + slog.DebugContext(ctx, "failed to resolve LLM pricing", + "provider", provider, + "model", model, + "error", err, + ) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert.go` around lines 200 - 234, resolveLLMPricing currently swallows errors from svc.ResolvePrice; when ResolvePrice returns an error you should log the failure (at debug level) with the error and contextual info (feat.Namespace, provider, model) before returning nil. Locate resolveLLMPricing and the svc.ResolvePrice call and add a debug log statement that extracts/uses the request context logger (e.g. logger from ctx or the project's logging helper) to emit a message like "failed to resolve LLM price" including err, feat.Namespace, provider and model, then keep returning nil for not-found flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/v3/api.gen.go`:
- Around line 2177-2180: The comment on the generated UnitCost docs is outdated:
update the source TypeSpec/OpenAPI description for the FeatureUnitCost/UnitCost
property to reflect that "llm" pricing can derive cost either from
meter-property lookups or from static provider/model/token_type values (and not
only meter lookups), then regenerate the OpenAPI spec and re-run codegen so
api.gen.go's UnitCost comment (for FeatureUnitCost) and the embedded spec remain
synchronized; locate the TypeSpec that defines UnitCost/FeatureUnitCost and
expand its description to mention both meter-property based lookup and the
optional static provider/model/token_type fields before regenerating.
---
Nitpick comments:
In `@api/v3/handlers/features/convert.go`:
- Around line 196-198: The call to FromFeatureLLMUnitCost is currently ignoring
its error which can leave resp.UnitCost inconsistent after llmCost.Pricing =
&apiPricing; change the code to capture the returned error from
resp.UnitCost.FromFeatureLLMUnitCost(llmCost), log that error with context
(including the feature/price info and any request identifiers) and handle it
(return the error or set a safe fallback UnitCost) instead of discarding it;
update the call site where FromFeatureLLMUnitCost and resp.UnitCost are used to
handle the error path explicitly.
- Around line 200-234: resolveLLMPricing currently swallows errors from
svc.ResolvePrice; when ResolvePrice returns an error you should log the failure
(at debug level) with the error and contextual info (feat.Namespace, provider,
model) before returning nil. Locate resolveLLMPricing and the svc.ResolvePrice
call and add a debug log statement that extracts/uses the request context logger
(e.g. logger from ctx or the project's logging helper) to emit a message like
"failed to resolve LLM price" including err, feat.Namespace, provider and model,
then keep returning nil for not-found flows.
In `@api/v3/handlers/features/create.go`:
- Around line 35-46: When GetMeterByIDOrSlug (via
h.meterService.GetMeterByIDOrSlug) returns an error, wrap that error with
contextual text before returning so callers can see "failed to resolve meter"
plus the original error; update the error return in the meter resolution block
(the branch that sets meterSlug) to wrap the err (e.g., using fmt.Errorf("failed
to resolve meter %s: %w", body.Meter.Id, err) or errors.Wrap) and return the
wrapped error instead of the raw err.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73799da5-a448-4da3-8a09-6f76a0431a92
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (7)
api/spec/packages/aip/src/features/feature.tspapi/v3/api.gen.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/features/create.goapi/v3/handlers/features/handler.goapi/v3/server/server.go
… costs AsFeatureLLMUnitCost() silently succeeds on manual union variants, causing FromFeatureLLMUnitCost() to overwrite the manual cost data. Add discriminator check before attempting the LLM cast. Added test for the manual unit cost no-op case that exposed the bug. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/v3/api.gen.go (1)
2177-2180:⚠️ Potential issue | 🟡 MinorBroaden the
unit_costdocs to include static LLM values.The wording still reads like
llmonly works through meter properties, butFeatureLLMUnitCostalready supports staticprovider,model, andtoken_typevalues. Please update the source OpenAPI description and regenerate so client docs and SDKs don’t miss that form.As per coding guidelines "Never manually edit generated files marked with '// Code generated by X, DO NOT EDIT.' headers", update the source OpenAPI spec and regenerate.
Also applies to: 2309-2312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 2177 - 2180, Update the OpenAPI source description for the UnitCost/unit_cost property (the schema backing FeatureUnitCost and FeatureLLMUnitCost) to state that when using "llm" it supports either dynamic lookup via meter group-by properties or static LLM configuration by specifying provider, model, and token_type fields in FeatureLLMUnitCost; then regenerate the API client code so the generated file (containing UnitCost, FeatureUnitCost, FeatureLLMUnitCost) and downstream SDK docs reflect the expanded wording instead of editing the generated api.gen.go directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/api.gen.go`:
- Around line 2168-2169: The feature flow is carrying the meter slug into the
generated API (meter.id) because create handler normalizes to m.Key and convert
uses f.MeterSlug; fix by plumbing the actual meter ULID through the domain
objects: in the create path (api/v3/handlers/features/create.go) resolve the
meter (using GetMeterByIDOrSlug or existing lookup used today), set a
new/existing Feature.MeterID (ULID) field on the Feature domain object before
persisting, ensure the persistence/save logic stores that MeterID, and update
the convert routine (api/v3/handlers/features/convert.go) to populate
resp.Meter.Id from f.MeterID (not f.MeterSlug); this keeps generated api.gen.go
correct without editing generated files.
---
Duplicate comments:
In `@api/v3/api.gen.go`:
- Around line 2177-2180: Update the OpenAPI source description for the
UnitCost/unit_cost property (the schema backing FeatureUnitCost and
FeatureLLMUnitCost) to state that when using "llm" it supports either dynamic
lookup via meter group-by properties or static LLM configuration by specifying
provider, model, and token_type fields in FeatureLLMUnitCost; then regenerate
the API client code so the generated file (containing UnitCost, FeatureUnitCost,
FeatureLLMUnitCost) and downstream SDK docs reflect the expanded wording instead
of editing the generated api.gen.go directly.
🪄 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: c649c985-e4f0-46ac-8ba6-fdad08dd6917
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (2)
api/spec/packages/aip/src/features/unitcost.tspapi/v3/api.gen.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/spec/packages/aip/src/features/unitcost.tsp
Comment on lines
+2168
to
+2169
| // Id The ID of the meter to associate with this feature. | ||
| Id ULID `json:"id"` |
Contributor
There was a problem hiding this comment.
meter.id still looks wired to the meter slug.
This contract is ID-based now, but api/v3/handlers/features/create.go (Lines 35-46) still normalizes the request to m.Key, and api/v3/handlers/features/convert.go (Lines 18-42) writes f.MeterSlug back into resp.Meter.Id. Unless the domain model now carries the real meter ID too, responses will end up serializing a slug/key under meter.id. I’d carry or preload the actual meter ID through the feature flow instead of fixing this with per-feature lookups later.
Quick read-only check to confirm the drift. Expected result: the create path stores m.Key, and the convert path later feeds that slug/key into resp.Meter.Id.
#!/bin/bash
set -euo pipefail
printf '\n== api/v3/handlers/features/create.go ==\n'
sed -n '1,120p' api/v3/handlers/features/create.go
printf '\n== api/v3/handlers/features/convert.go ==\n'
sed -n '1,120p' api/v3/handlers/features/convert.go
printf '\n== feature meter fields/usages outside generated code ==\n'
rg -n -C3 --type go -g '!api/v3/api.gen.go' '\btype Feature struct\b|\bMeterSlug\b|\bMeterID\b|\bGetMeterByIDOrSlug\b'As per coding guidelines "Never manually edit generated files marked with '// Code generated by X, DO NOT EDIT.' headers", fix this in the source spec and/or non-generated handlers rather than patching api.gen.go directly.
Also applies to: 2300-2301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/api.gen.go` around lines 2168 - 2169, The feature flow is carrying the
meter slug into the generated API (meter.id) because create handler normalizes
to m.Key and convert uses f.MeterSlug; fix by plumbing the actual meter ULID
through the domain objects: in the create path
(api/v3/handlers/features/create.go) resolve the meter (using GetMeterByIDOrSlug
or existing lookup used today), set a new/existing Feature.MeterID (ULID) field
on the Feature domain object before persisting, ensure the persistence/save
logic stores that MeterID, and update the convert routine
(api/v3/handlers/features/convert.go) to populate resp.Meter.Id from f.MeterID
(not f.MeterSlug); this keeps generated api.gen.go correct without editing
generated files.
Validate filter dimension keys against the meter's GroupBy and reject unknown filter operators at the API layer before reaching the domain. Fix remaining unnecessary type conversions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing in favor of: #3986 |
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
3 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.
Changes
Todo
V3 Feature API - Test Results
GET /api/v3/openmeter/featuresGET /api/v3/openmeter/features/:idPOST /api/v3/openmeter/features"amount": "0.005")POST /api/v3/openmeter/featuresPOST /api/v3/openmeter/featuresPOST /api/v3/openmeter/featuresGET /api/v3/openmeter/featuresGET /api/v3/openmeter/features/:idDELETE /api/v3/openmeter/features/:idGET /api/v3/openmeter/features/:idPOST /api/v3/openmeter/features"type": "invalid")POST /api/v3/openmeter/featuresPOST /api/v3/openmeter/featuresGET /api/v3/openmeter/features/:idPOST /api/v3/openmeter/featuresSummary by CodeRabbit