Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds annotations support to tax codes across DB, model, adapter, service, and API conversion; blocks updates/deletes for system-managed tax codes; includes tests and test utilities for new behavior. (<=50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Handler"
participant Service
participant Adapter
participant DB
Client->>API: Create/Update/Delete TaxCode (may include annotations)
API->>Service: Convert request → input (some mappings ignore Annotations)
Service->>Adapter: Create -> persist Metadata + Annotations
Adapter->>DB: INSERT/UPDATE tax_codes (annotations jsonb)
Service->>Adapter: Update/Delete -> fetch existing by NamespacedID
Adapter->>DB: SELECT tax_code
DB-->>Adapter: existing tax code (includes annotations)
Adapter-->>Service: mapped TaxCode (Annotations populated)
Service->>Service: check IsManagedBySystem()
alt system-managed
Service-->>Client: return ErrTaxCodeManagedBySystem (409)
else not system-managed
Service->>Adapter: proceed with Update/Delete
Adapter->>DB: UPDATE/DELETE
DB-->>Adapter: result
Adapter-->>Service: success
Service-->>Client: success response
end
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)
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 |
borbelyr-kong
added
the
release-note/ignore
![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
🧹 Nitpick comments (3)
openmeter/taxcode/adapter/mapping.go (1)
33-33: Consider copying annotations defensively before returning the domain object.Line 33 assigns the map by reference, which can create accidental shared mutations. Copying it (like metadata handling does) is safer.
♻️ Suggested refactor
func MapTaxCodeFromEntity(entity *db.TaxCode) (taxcode.TaxCode, error) { if entity == nil { return taxcode.TaxCode{}, errors.New("entity is required") } + var annotations models.Annotations + if entity.Annotations != nil { + annotations = make(models.Annotations, len(entity.Annotations)) + for k, v := range entity.Annotations { + annotations[k] = v + } + } + return taxcode.TaxCode{ NamespacedID: models.NamespacedID{ Namespace: entity.Namespace, ID: entity.ID, }, @@ AppMappings: lo.FromPtr(entity.AppMappings), Metadata: models.NewMetadata(entity.Metadata), - Annotations: entity.Annotations, + Annotations: annotations, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/adapter/mapping.go` at line 33, The Annotations map is being assigned by reference (Annotations: entity.Annotations) which risks shared mutations; change the mapper (in mapping.go) to defensively copy entity.Annotations into a new map before assigning to the domain object's Annotations field (similar to how metadata is handled): create an empty map when entity.Annotations is non-nil, copy each key/value, and assign that new map (handle nil case by leaving domain Annotations nil or empty consistently).openmeter/taxcode/service.go (1)
39-39: Recommend guarding system-reserved annotations at the service boundary.Since this input now accepts annotations, adding validation for reserved/system-managed keys would help prevent accidental externalization of internal control flags later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/service.go` at line 39, The service now accepts Annotations (models.Annotations) but doesn't guard system-reserved keys; add validation in the service boundary (inside the methods in service.go that accept models.Annotations, e.g., Create/Update handlers or the service struct methods) to either reject requests containing reserved keys or strip them before persisting; implement a small reservedKeys set (e.g., "_system", "managed_by", "internal_*") and check incoming annotations against it, returning a clear validation error (bad request) or removing those keys before calling lower layers.openmeter/taxcode/service/taxcode_test.go (1)
33-54: Assert the persisted state after each operation.These subtests mostly stop at the returned error/value. A refetch after blocked update/delete, a refetch after successful update, and a not-found check after successful delete would lock down the real contract here: the DB row stayed unchanged when blocked and actually changed when allowed.
As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.Also applies to: 67-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/service/taxcode_test.go` around lines 33 - 54, After asserting errors for UpdateTaxCode and DeleteTaxCode, refetch the persisted taxcode via the service (e.g., env.Service.GetTaxCode or the equivalent read method) and assert the DB state: for the blocked UpdateIsBlocked and DeleteIsBlocked subtests confirm the taxcode row is unchanged (same Name and other fields) and still present; for the successful update subtest refetch and assert the Name changed to "updated name"; for the successful delete subtest refetch and assert the taxcode is not found (expect a not-found error). Use the same NamespacedID (models.NamespacedID{Namespace: ns, ID: tc.ID}) when refetching to verify persistence after calling UpdateTaxCode and DeleteTaxCode.
🤖 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/taxcodes/convert.go`:
- Around line 23-24: Reject any incoming label keys that start with the reserved
prefix "openmeter.io/" when mapping client Labels into Metadata (the code around
the goverter mapping comment "goverter:map Labels Metadata" and input handlers
such as CreateTaxCodeHandler / UpsertTaxCodeHandler or the converter function
that maps Labels->Metadata); implement a validation check on create/upsert input
that returns a ValidationIssue (per API handler conventions) listing the
offending keys rather than copying them into Metadata, and add the same check
wherever Labels are accepted so responses' openmeter.io/ annotations cannot be
spoofed by client input.
In `@openmeter/taxcode/service/taxcode.go`:
- Around line 47-49: Existing legacy tax_code rows have NULL annotations so
UpdateTaxCode and DeleteTaxCode guards (which call existing.IsManagedBySystem())
incorrectly allow edits/deletes; fix by either adding a migration backfill that
UPDATEs annotation=managed_by=system for rows that were created via
GetOrCreateByAppMapping, or modify IsManagedBySystem (or add a helper used by
UpdateTaxCode/DeleteTaxCode) to treat NULL annotation as system-managed for
records whose creation context matches GetOrCreateByAppMapping; ensure the
change references the existing functions GetOrCreateByAppMapping, UpdateTaxCode,
DeleteTaxCode and the annotations column so legacy rows are protected.
---
Nitpick comments:
In `@openmeter/taxcode/adapter/mapping.go`:
- Line 33: The Annotations map is being assigned by reference (Annotations:
entity.Annotations) which risks shared mutations; change the mapper (in
mapping.go) to defensively copy entity.Annotations into a new map before
assigning to the domain object's Annotations field (similar to how metadata is
handled): create an empty map when entity.Annotations is non-nil, copy each
key/value, and assign that new map (handle nil case by leaving domain
Annotations nil or empty consistently).
In `@openmeter/taxcode/service.go`:
- Line 39: The service now accepts Annotations (models.Annotations) but doesn't
guard system-reserved keys; add validation in the service boundary (inside the
methods in service.go that accept models.Annotations, e.g., Create/Update
handlers or the service struct methods) to either reject requests containing
reserved keys or strip them before persisting; implement a small reservedKeys
set (e.g., "_system", "managed_by", "internal_*") and check incoming annotations
against it, returning a clear validation error (bad request) or removing those
keys before calling lower layers.
In `@openmeter/taxcode/service/taxcode_test.go`:
- Around line 33-54: After asserting errors for UpdateTaxCode and DeleteTaxCode,
refetch the persisted taxcode via the service (e.g., env.Service.GetTaxCode or
the equivalent read method) and assert the DB state: for the blocked
UpdateIsBlocked and DeleteIsBlocked subtests confirm the taxcode row is
unchanged (same Name and other fields) and still present; for the successful
update subtest refetch and assert the Name changed to "updated name"; for the
successful delete subtest refetch and assert the taxcode is not found (expect a
not-found error). Use the same NamespacedID (models.NamespacedID{Namespace: ns,
ID: tc.ID}) when refetching to verify persistence after calling UpdateTaxCode
and DeleteTaxCode.
🪄 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: b48a85f0-8c70-4967-b971-8fefb0b35b51
⛔ Files ignored due to path filters (10)
openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode/taxcode.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode/where.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode_create.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode_update.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
api/v3/handlers/taxcodes/convert.gen.goapi/v3/handlers/taxcodes/convert.goopenmeter/ent/schema/taxcode.goopenmeter/taxcode/adapter/mapping.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/annotations.goopenmeter/taxcode/errors.goopenmeter/taxcode/service.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/service/taxcode_test.goopenmeter/taxcode/taxcode.goopenmeter/taxcode/testutils/env.gotools/migrate/migrations/20260402082013_add_annotations_to_taxcode.down.sqltools/migrate/migrations/20260402082013_add_annotations_to_taxcode.up.sql
Comment on lines
23
to
+24
| // goverter:map Labels Metadata | ||
| // goverter:ignore Annotations |
Contributor
There was a problem hiding this comment.
Reserve the openmeter.io/ label namespace on write.
Create/upsert still copy client Labels straight into Metadata, while responses now emit annotations under the same openmeter.io/ prefix. That lets a caller send openmeter.io/managed_by=system on a user-managed tax code and get back a label that looks identical to a real system annotation. Please reject that prefix with a ValidationIssue, or otherwise reserve it on input, so the annotation namespace stays trustworthy.
As per coding guidelines, api/**/*.go: Follow HTTP handler patterns and ValidationIssue conventions as documented in the /api skill.
Also applies to: 29-32, 86-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/handlers/taxcodes/convert.go` around lines 23 - 24, Reject any
incoming label keys that start with the reserved prefix "openmeter.io/" when
mapping client Labels into Metadata (the code around the goverter mapping
comment "goverter:map Labels Metadata" and input handlers such as
CreateTaxCodeHandler / UpsertTaxCodeHandler or the converter function that maps
Labels->Metadata); implement a validation check on create/upsert input that
returns a ValidationIssue (per API handler conventions) listing the offending
keys rather than copying them into Metadata, and add the same check wherever
Labels are accepted so responses' openmeter.io/ annotations cannot be spoofed by
client input.
Comment on lines
+47
to
+49
| if existing.IsManagedBySystem() { | ||
| return taxcode.TaxCode{}, taxcode.ErrTaxCodeManagedBySystem | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for migrations/backfills or compatibility logic related to legacy taxcode annotations.
rg -n -C3 'AnnotationKeyManagedBy|managed_by|GetOrCreateByAppMapping|SetAnnotations|UPDATE .*tax.?code|ALTER TABLE .*tax.?code|annotations' \
--glob '*.go' --glob '*.sql' --glob '*.yaml' --glob '*.yml'Repository: openmeterio/openmeter
Length of output: 50379
🏁 Script executed:
cat tools/migrate/migrations/20260402082013_add_annotations_to_taxcode.up.sqlRepository: openmeterio/openmeter
Length of output: 290
🏁 Script executed:
cat tools/migrate/migrations/20260402082013_add_annotations_to_taxcode.down.sqlRepository: openmeterio/openmeter
Length of output: 255
🏁 Script executed:
head -200 openmeter/taxcode/service/taxcode.goRepository: openmeterio/openmeter
Length of output: 4647
Legacy tax codes need a backfill to enforce the new system-managed protection.
The migration adds the annotations column but doesn't backfill existing rows with managed_by=system. Since GetOrCreateByAppMapping only stamps that annotation on new creates, any tax codes that were auto-created before this rollout will have a NULL annotation. The new guards in UpdateTaxCode (line 47) and DeleteTaxCode (line 141) check existing.IsManagedBySystem(), which will return false for NULL values, letting legacy rows slip through editable/deletable.
Either add an UPDATE statement to the migration to backfill existing rows created by GetOrCreateByAppMapping, or implement a compatibility check that treats missing annotations as system-managed based on creation context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/taxcode/service/taxcode.go` around lines 47 - 49, Existing legacy
tax_code rows have NULL annotations so UpdateTaxCode and DeleteTaxCode guards
(which call existing.IsManagedBySystem()) incorrectly allow edits/deletes; fix
by either adding a migration backfill that UPDATEs annotation=managed_by=system
for rows that were created via GetOrCreateByAppMapping, or modify
IsManagedBySystem (or add a helper used by UpdateTaxCode/DeleteTaxCode) to treat
NULL annotation as system-managed for records whose creation context matches
GetOrCreateByAppMapping; ensure the change references the existing functions
GetOrCreateByAppMapping, UpdateTaxCode, DeleteTaxCode and the annotations column
so legacy rows are protected.
Contributor
Author
There was a problem hiding this comment.
GetOrCreateByAppMapping is no longer sets annotations and also we don't want to backfill.
Contributor
There was a problem hiding this comment.
@borbelyr-kong, got it — thanks for the clarification! Since GetOrCreateByAppMapping doesn't stamp the managed_by=system annotation, the IsManagedBySystem() guard is intentionally selective, only protecting records that are explicitly annotated as system-managed through some other path. The legacy rows were never meant to be protected, so no backfill is needed. My concern was off-base here. 🙏
( ˆ◡ˆ)っ✎
scribbling notes carefully
✏️ Learnings added
Learnt from: borbelyr-kong
Repo: openmeterio/openmeter PR: 4061
File: openmeter/taxcode/service/taxcode.go:47-49
Timestamp: 2026-04-08T08:10:19.698Z
Learning: In `openmeter/taxcode/service/taxcode.go`, `GetOrCreateByAppMapping` does NOT set the `managed_by=system` annotation on auto-created tax codes. Only records explicitly annotated elsewhere carry that annotation. Therefore, the `IsManagedBySystem()` guards in `UpdateTaxCode` and `DeleteTaxCode` are intentionally selective — auto-created (legacy) rows without annotations are NOT treated as system-managed, and there is no need for a backfill migration. Do not flag missing backfill or legacy-row compatibility in this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: borbelyr-kong
Repo: openmeterio/openmeter PR: 4034
File: openmeter/billing/service/profile.go:85-87
Timestamp: 2026-03-31T11:55:04.046Z
Learning: In `openmeter/billing/service/profile.go`, the `demoteDefaultProfile` function does NOT need an explicit call to `resolveDefaultTaxCode` before writing. When it calls `GetDefaultProfile`, the adapter uses `WithWorkflowConfig(workflowConfigWithTaxCode)` which eager-loads the `TaxCode` edge; `mapWorkflowConfigFromDB` then calls `BackfillTaxConfig` to stamp `DefaultTaxConfig.TaxCodeID` from the DB row. Since `resolveDefaultTaxCode` is idempotent (no-op when `TaxCodeID != nil`), `TaxCodeID` is already populated by the time `adapter.UpdateProfile` is called. Do not flag the absence of `resolveDefaultTaxCode` in `demoteDefaultProfile` as a missing dual-write guard.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4077
File: openmeter/billing/charges/usagebased/charge.go:93-114
Timestamp: 2026-04-03T12:22:50.169Z
Learning: In the openmeterio/openmeter codebase, the `charge_usage_based`, `charge_usage_based_runs`, and `charge_flat_fees` tables had no pre-existing rows when the `feature_id` column (NOT NULL for usage-based) was introduced in migration `20260403090155_charges_feature_ids`. There is no legacy data that lacks `feature_id`, so no backfill strategy is needed and `State.Validate()` requiring a non-empty `FeatureID` is safe.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4023
File: openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go:112-127
Timestamp: 2026-03-28T18:44:25.086Z
Learning: In `openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go`, the `AddShrink` and `AddExtend` methods do NOT need to clear `DeletedAt` on the split line group (`updatedGroup.DeletedAt = nil`) when reviving managed child lines. Whole group delete/restore is not a scenario that occurs in this system, so this is intentional and not a missing guard. Do not flag the absence of group-level `DeletedAt` revival alongside managed child reactivation in this file.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4053
File: openmeter/billing/charges/usagebased/service/creditsonly.go:125-147
Timestamp: 2026-04-01T15:37:04.838Z
Learning: In `openmeter/billing/charges/usagebased/service/creditsonly.go`, the `DeleteCharge` method on `CreditsOnlyStateMachine` (including all `CreateRunCreditRealization` and `Adapter.DeleteCharge` calls within it) runs inside a `transaction.New`, so all persistence operations are atomic from an external point of view. Do not flag the sequential CreateRunCreditRealization + DeleteCharge calls as non-atomic — they are wrapped in a single DB transaction.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4031
File: openmeter/billing/charges/flatfee/adapter/charge.go:81-86
Timestamp: 2026-04-01T14:14:56.558Z
Learning: In `openmeter/billing/charges/flatfee/adapter/charge.go`, `DeleteCharge` sets `charge.DeletedAt` and `charge.Status` on the in-memory struct before calling `chargemeta.Update`. This is correct: `chargemeta.Update` (in `openmeter/billing/charges/models/chargemeta/mixin.go`) calls `SetOrClearDeletedAt(convert.TimePtrIn(in.DeletedAt, time.UTC))` and `SetStatus(in.Status)`, so both fields are persisted to the DB via the ent mutation. Do not flag the absence of an explicit `SetDeletedAt` call on the ent UpdateOneID builder in `DeleteCharge` as a bug — `chargemeta.Update` already handles this.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4053
File: openmeter/billing/charges/usagebased/adapter/charge.go:117-121
Timestamp: 2026-04-01T15:31:02.589Z
Learning: In this codebase, database transactions are propagated through `context.Context` using `entutils.TransactingRepo` / `entutils.TransactingRepoWithNoValue` together with `SetDriverOnContext` / `GetDriverFromContext`. When a nested call also uses these helpers and the same `ctx` is threaded through, it should automatically join/reuse the existing transaction (including when an adapter field such as `metaAdapter` is not explicitly wrapped with the tx). During code review, do not flag a call as “outside the transaction” solely because a specific adapter instance isn’t wrapped, as long as the transaction driver is obtainable from the provided `ctx` via these helpers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/handlers/taxcodes/convert.go (1)
105-108: Silent drop of non-string annotation values is worth documenting.The type assertion
v.(string)silently skips any annotation values that aren't strings. Currently this is fine since all annotations use string values (peropenmeter/taxcode/annotations.go), but a brief comment here would help future maintainers understand this is intentional rather than an oversight.📝 Suggested documentation
func ConvertTaxCodeToLabels(source taxcode.TaxCode) *api.Labels { labels := ConvertMetadataToLabels(source.Metadata) for k, v := range source.Annotations { + // Only string-valued annotations are surfaced as labels; + // non-string values are intentionally skipped. if s, ok := v.(string); ok { (*labels)[annotationLabelPrefix+k] = s } } return labels }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/taxcodes/convert.go` around lines 105 - 108, In convert.go where the loop over source.Annotations performs a type assertion `v.(string)` and assigns into `(*labels)[annotationLabelPrefix+k]`, add a concise inline comment noting that non-string annotation values are intentionally ignored because all annotations are defined as strings (see openmeter/taxcode/annotations.go), so the silent skip via the `ok` check is deliberate and not a bug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/taxcode/service/taxcode.go`:
- Around line 42-49: The service checks IsManagedBySystem() before calling the
adapter, but the adapter mutations (UpdateTaxCode and DeleteTaxCode) must
enforce the same invariant to avoid race conditions; modify the adapter
implementations for UpdateTaxCode and DeleteTaxCode to include a predicate that
excludes rows with annotations->>'managed_by' = 'system' (i.e., add a WHERE
clause that annotations->>'managed_by' != 'system' or NULL) so the DB will
refuse updates/deletes on system-managed tax codes, and return a not-allowed
error when zero rows are affected; alternatively you can lock the row in
GetTaxCode with FOR UPDATE (used by the service's pre-check) if you prefer
row-locking instead of the WHERE predicate.
In `@openmeter/taxcode/testutils/env.go`:
- Around line 56-79: Create and register a t.Cleanup early in NewTestEnv so
resources are always torn down on test failure: after creating logger, db and
client (from testutils.InitPostgresDB and db.EntDriver.Client) construct a
partial TestEnv value (setting Logger, Client, db and close sync.Once) and call
t.Cleanup(func(){ env.Close() }) before calling taxcodeadapter.New and
require.NoErrorf; then finish initializing adapter and Service and return the
same env. Ensure TestEnv.Close is idempotent (uses the existing sync.Once) so
the cleanup is safe to call even if setup later fails.
---
Nitpick comments:
In `@api/v3/handlers/taxcodes/convert.go`:
- Around line 105-108: In convert.go where the loop over source.Annotations
performs a type assertion `v.(string)` and assigns into
`(*labels)[annotationLabelPrefix+k]`, add a concise inline comment noting that
non-string annotation values are intentionally ignored because all annotations
are defined as strings (see openmeter/taxcode/annotations.go), so the silent
skip via the `ok` check is deliberate and not a bug.
🪄 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: 624c9b71-fbcd-4d81-9f29-53ecc12aefb1
📒 Files selected for processing (6)
api/v3/handlers/taxcodes/convert.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/service.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/service/taxcode_test.goopenmeter/taxcode/testutils/env.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/taxcode/service/taxcode_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/taxcode/adapter/taxcode.go
- openmeter/taxcode/service.go
Comment on lines
+42
to
+49
| existing, err := s.adapter.GetTaxCode(ctx, taxcode.GetTaxCodeInput{NamespacedID: input.NamespacedID}) | ||
| if err != nil { | ||
| return taxcode.TaxCode{}, err | ||
| } | ||
|
|
||
| if existing.IsManagedBySystem() { | ||
| return taxcode.TaxCode{}, taxcode.ErrTaxCodeManagedBySystem | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== transaction helpers =="
fd '.go$' pkg/framework/transaction --exec sed -n '1,220p' {}
echo
echo "== taxcode adapter write paths =="
rg -n -C8 '^func .*UpdateTaxCode\(|^func .*DeleteTaxCode\(' openmeter/taxcode/adapter/taxcode.go
echo
echo "== managed_by predicates / row locks =="
rg -n -C4 'FOR UPDATE|managed_by|AnnotationKeyManagedBy|IsManagedBySystem' openmeter/taxcode/adapter/taxcode.go pkg/framework/transactionRepository: openmeterio/openmeter
Length of output: 4821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== UpdateTaxCode full implementation ==="
rg -n -A 20 '^func \(a \*adapter\) UpdateTaxCode\(' openmeter/taxcode/adapter/taxcode.go
echo
echo "=== DeleteTaxCode full implementation ==="
rg -n -A 20 '^func \(a \*adapter\) DeleteTaxCode\(' openmeter/taxcode/adapter/taxcode.go
echo
echo "=== TransactingRepo implementation ==="
fd 'entutils' --type f -name '*.go' -path '*openmeter*' | head -5 | xargs rg -l 'TransactingRepo' | head -1 | xargs cat | head -100
echo
echo "=== Isolation level configuration ==="
rg -r 'isolation|ReadCommitted|RepeatableRead|Serializable' pkg/framework --type go | head -20Repository: openmeterio/openmeter
Length of output: 5155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== TaxCode schema / migrations ==="
fd 'taxcode' pkg/framework/ent migrations -name '*.go' -o -name '*.sql' | head -10 | xargs -I {} sh -c 'echo "FILE: {}"; head -50 {}'
echo
echo "=== entutils.TransactingRepo implementation ==="
fd 'entutils' openmeter pkg --type f -name '*.go' | xargs grep -l 'TransactingRepo' | head -1 | xargs sed -n '1,250p'
echo
echo "=== Isolation level / tx config ==="
rg 'isolation|ReadCommitted|RepeatableRead|Serializable|SetIsolation' pkg/framework --type go --type rust -C 3 | head -50
echo
echo "=== managed_by column definition ==="
rg 'managed_by|ManagedBy|managed.by' openmeter/taxcode -C 2 | head -40Repository: openmeterio/openmeter
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find ent schema files ==="
find . -path ./node_modules -prune -o -type f -name '*.go' -print | xargs grep -l 'managed_by\|ManagedBy' 2>/dev/null | head -10
echo
echo "=== entutils TransactingRepo implementation ==="
find . -path ./node_modules -prune -o -type f -path '*/entutils*' -name '*.go' -print | xargs cat 2>/dev/null | head -200
echo
echo "=== PostgreSQL isolation config ==="
find . -path ./node_modules -prune -o -type f -name '*.go' -print | xargs grep -i 'isolation\|read.*commit\|repeatable.*read\|serializable' 2>/dev/null | grep -v '.pb.go' | head -30
echo
echo "=== TaxCode ent schema (look for managed_by) ==="
find . -path ./node_modules -prune -o -type f -name 'taxcode.go' -path '*/ent/db/schema/*' -print | xargs cat 2>/dev/nullRepository: openmeterio/openmeter
Length of output: 5627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== TransactingRepo definition ==="
find . -type f -name '*.go' | xargs grep -l 'func TransactingRepo' | head -5
echo
echo "=== TaxCode schema/entity ==="
find . -type f -name 'taxcode.go' | grep -E 'ent|schema' | head -5 | xargs cat 2>/dev/null
echo
echo "=== managed_by field in TaxCode ==="
find . -type f -name '*.go' | xargs grep -A 5 -B 5 'Field.*managed.*\|managed.*Field' 2>/dev/null | grep -i taxcode -A 10 -B 5 | head -40
echo
echo "=== Database driver and isolation setup ==="
find . -type f -name '*.go' | xargs grep -l 'pgx\|postgres\|isolation' | xargs grep -A 3 'isolation\|ReadCommitted\|Serializable' 2>/dev/null | head -40Repository: openmeterio/openmeter
Length of output: 34471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== IsManagedBySystem implementation ==="
find . -type f -name '*.go' | xargs grep -n 'func.*IsManagedBySystem\|ManagedBySystem' | head -20
echo
echo "=== taxcode.TaxCode domain struct ==="
find . -type f -path '*/taxcode/*.go' -name '*.go' ! -path '*/ent/*' ! -path '*/db/*' | xargs grep -l 'type TaxCode struct' | head -1 | xargs cat | head -100
echo
echo "=== Search for managed annotation/key ==="
find . -type f -path '*/taxcode/*.go' | xargs grep -i 'managed\|system' | grep -v '//.*managed' | head -30Repository: openmeterio/openmeter
Length of output: 7186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== entutils/transaction.go full implementation ==="
cat pkg/framework/entutils/transaction.go
echo
echo "=== Check for SERIALIZABLE or FOR UPDATE in entutils ==="
grep -r 'SERIALIZABLE\|FOR UPDATE\|SetIsolation\|isolation' pkg/framework/entutils/ --include='*.go'
echo
echo "=== Database initialization and isolation config ==="
find . -type f -name '*.go' -path '*/db/*' ! -path '*/ent/*' | xargs grep -l 'isolation\|pgx' | head -5 | xargs grep -A 5 -B 5 'isolation\|Serializable' | head -50Repository: openmeterio/openmeter
Length of output: 8712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== TaxCode adapter.Tx() implementation ==="
grep -n 'func.*Tx\|Tx(ctx' openmeter/taxcode/adapter/taxcode.go | head -20
echo
echo "=== Database Client creation / transaction setup ==="
find . -type f -name '*.go' -path '*/db/*' ! -path '*/ent/db/*' ! -path '*/framework/*' | xargs grep -l 'Tx.*sql.TxOptions\|isolation\|ReadCommitted' | head -5
echo
echo "=== Check if any code explicitly sets isolation ==="
grep -r 'TxOptions\|Isolation' openmeter --include='*.go' | grep -v test | head -20
echo
echo "=== Look for TxHijacker implementation in adapter ==="
grep -r 'HijackTx\|TxOptions' openmeter/taxcode --include='*.go'Repository: openmeterio/openmeter
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full TaxCode adapter definition ==="
head -50 openmeter/taxcode/adapter/taxcode.go
echo
echo "=== Search for Tx method in any adapter ==="
find openmeter -type f -name '*.go' -path '*/adapter/*' | xargs grep -l 'func.*Tx(' | head -3 | xargs grep -A 10 'func.*Tx('
echo
echo "=== Check TxCreator implementation ==="
grep -r 'func.*Tx.*context.Context' openmeter --include='*.go' | grep -v test | grep -v '\.pb\.' | head -10
echo
echo "=== Transaction setup in main/db init ==="
find . -type f -name 'main.go' -o -name 'db.go' -o -name 'setup.go' | xargs grep -l postgres 2>/dev/null | head -3 | xargs grep -A 20 'sql.Open\|pgx\|postgres'Repository: openmeterio/openmeter
Length of output: 7585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check sql.TxOptions and default isolation ==="
grep -r 'sql.TxOptions' openmeter --include='*.go' -B 2 -A 5 | head -60
echo
echo "=== Look for any row locking (FOR UPDATE) in taxcode adapter ==="
grep -r 'FOR UPDATE\|for update\|ForUpdate' openmeter/taxcode --include='*.go'
echo
echo "=== Check if service reads with lock before adapter call ==="
sed -n '40,95p' openmeter/taxcode/service/taxcode.go
echo
echo "=== Check if adapter UpdateTaxCode has WHERE managed_by predicate ==="
sed -n '50,75p' openmeter/taxcode/adapter/taxcode.goRepository: openmeterio/openmeter
Length of output: 4498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Exact UpdateTaxCode mutation in adapter ==="
sed -n '50,80p' openmeter/taxcode/adapter/taxcode.go
echo
echo "=== Exact DeleteTaxCode mutation in adapter ==="
sed -n '163,200p' openmeter/taxcode/adapter/taxcode.go
echo
echo "=== Confirm: sql.TxOptions default isolation is READ COMMITTED ==="
echo "PostgreSQL default isolation level is READ COMMITTED when not specified in sql.TxOptions"
echo "The code uses sql.TxOptions{ReadOnly: false} which does NOT set Isolation"
echo "This means READ COMMITTED isolation is in effect"Repository: openmeterio/openmeter
Length of output: 2263
Add managed_by predicate to adapter writes to prevent race conditions.
The service-layer checks at lines 47–48 and 138–140 (verifying IsManagedBySystem()) run only before calling the adapter, but the adapter itself doesn't re-enforce this invariant. Since annotations[managed_by] is stored in a JSONB column (not a separate DB column), a concurrent transaction can flip the status between the service check and the adapter mutation. With PostgreSQL's default READ COMMITTED isolation and no row locking, another request can successfully modify or delete a tax code after it's been marked as system-managed.
To fix:
- Add a WHERE clause in
UpdateTaxCode(line 56) andDeleteTaxCode(line 182) that excludes rows whereannotations->>'managed_by' = 'system', or - Use
FOR UPDATEwhen reading to lock the row before the check, or - Bump isolation to SERIALIZABLE
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/taxcode/service/taxcode.go` around lines 42 - 49, The service
checks IsManagedBySystem() before calling the adapter, but the adapter mutations
(UpdateTaxCode and DeleteTaxCode) must enforce the same invariant to avoid race
conditions; modify the adapter implementations for UpdateTaxCode and
DeleteTaxCode to include a predicate that excludes rows with
annotations->>'managed_by' = 'system' (i.e., add a WHERE clause that
annotations->>'managed_by' != 'system' or NULL) so the DB will refuse
updates/deletes on system-managed tax codes, and return a not-allowed error when
zero rows are affected; alternatively you can lock the row in GetTaxCode with
FOR UPDATE (used by the service's pre-check) if you prefer row-locking instead
of the WHERE predicate.
065d44d to
7f413dd
Compare
borbelyr-kong
force-pushed
the
feat/tax-codes-add-annotations
branch
from
7f413dd to
01ae1bb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/taxcode/testutils/env.go (1)
25-32: Minor inconsistency: consider reusinge.Client.Line 30 creates a fresh client via
e.db.EntDriver.Client(), bute.Clientalready holds the same client reference. Usinge.Client.Schema.Create(...)would be more consistent and make it clearer you're working with the same client throughout.💡 Suggested tweak
func (e *TestEnv) DBSchemaMigrate(t *testing.T) { t.Helper() require.NotNilf(t, e.db, "database must be initialized") - err := e.db.EntDriver.Client().Schema.Create(t.Context()) + err := e.Client.Schema.Create(t.Context()) require.NoErrorf(t, err, "schema migration must not fail") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/testutils/env.go` around lines 25 - 32, In DBSchemaMigrate on TestEnv, switch from creating a fresh client via e.db.EntDriver.Client() to reusing the existing e.Client (use e.Client.Schema.Create(...)) to keep client usage consistent; also ensure you assert e.Client is not nil (similar to the existing require.NotNilf for e.db) before calling Schema.Create to avoid nil dereference. This touches the DBSchemaMigrate method and the TestEnv fields e.Client and e.db.EntDriver.Client().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/taxcode/testutils/env.go`:
- Around line 34-54: In TestEnv.Close (the e.close.Do block) change the resource
shutdown order to close e.Client first, then e.db.EntDriver, then e.db.PGDriver:
move the e.Client nil-check and Client.Close() call before the driver closes,
preserving the existing error handling (t.Errorf) and the use of e.close.Do to
ensure idempotence; ensure you reference the same symbols (e.Client,
e.db.EntDriver.Close, e.db.PGDriver.Close) and keep behavior otherwise
identical.
---
Nitpick comments:
In `@openmeter/taxcode/testutils/env.go`:
- Around line 25-32: In DBSchemaMigrate on TestEnv, switch from creating a fresh
client via e.db.EntDriver.Client() to reusing the existing e.Client (use
e.Client.Schema.Create(...)) to keep client usage consistent; also ensure you
assert e.Client is not nil (similar to the existing require.NotNilf for e.db)
before calling Schema.Create to avoid nil dereference. This touches the
DBSchemaMigrate method and the TestEnv fields e.Client and
e.db.EntDriver.Client().
🪄 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: 28345bcf-6fc8-44ec-8a3e-e081e68b6eb8
⛔ Files ignored due to path filters (10)
openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode/taxcode.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode/where.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode_create.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode_update.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
api/v3/handlers/taxcodes/convert.gen.goapi/v3/handlers/taxcodes/convert.goopenmeter/ent/schema/taxcode.goopenmeter/taxcode/adapter/mapping.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/annotations.goopenmeter/taxcode/errors.goopenmeter/taxcode/service.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/service/taxcode_test.goopenmeter/taxcode/taxcode.goopenmeter/taxcode/testutils/env.gotools/migrate/migrations/20260408082246_add_annotations_to_taxcode.down.sqltools/migrate/migrations/20260408082246_add_annotations_to_taxcode.up.sql
✅ Files skipped from review due to trivial changes (6)
- tools/migrate/migrations/20260408082246_add_annotations_to_taxcode.down.sql
- tools/migrate/migrations/20260408082246_add_annotations_to_taxcode.up.sql
- openmeter/taxcode/errors.go
- openmeter/ent/schema/taxcode.go
- openmeter/taxcode/annotations.go
- openmeter/taxcode/service/taxcode_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- openmeter/taxcode/adapter/mapping.go
- api/v3/handlers/taxcodes/convert.gen.go
- openmeter/taxcode/service.go
- openmeter/taxcode/adapter/taxcode.go
- openmeter/taxcode/taxcode.go
- openmeter/taxcode/service/taxcode.go
chrisgacsal
previously approved these changes
Apr 9, 2026
chrisgacsal
approved these changes
Apr 9, 2026
This was referenced Apr 10, 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
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.
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Database