refactor: migrate resource variables to db#815
Merged
adityachoudhari26 merged 5 commits intomainfrom Feb 26, 2026
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds persisted, resource-scoped variables: DB table and drizzle/SQL schema, sqlc-generated Go queries/models, repository interfaces and DB/memory implementations, store wiring and migration support, and integration/test wiring across workspace-engine. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Store/Service
participant Repo as ResourceVariableRepo (DB/Mem)
participant DB as Postgres
Service->>Repo: Set(entity) / Get(key) / BulkUpdate(...)
Note right of Repo: parse composite key, marshal/unmarshal JSON
Repo->>DB: sqlc queries (GetResourceVariable / UpsertResourceVariable / Delete ...)
DB-->>Repo: rows / exec result
Repo-->>Service: converted *oapi.ResourceVariable / status / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
![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: 4
🧹 Nitpick comments (4)
packages/db/src/schema/resource-variable.ts (1)
18-18: Consider adding a$type<>()annotation to thevaluejsonb column.Other jsonb columns in the schema (e.g.,
resource.config,resource.metadata) carry.$type<Record<string, any>>()or.$type<Record<string, string>>()annotations for precise TypeScript inference at ORM query boundaries. Without it,valuewill resolve toJsonValue/unknown.✨ Proposed improvement
- value: jsonb("value").notNull(), + value: jsonb("value").notNull().$type<Record<string, unknown>>(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/resource-variable.ts` at line 18, Add a $type<> annotation to the jsonb column definition for the value field so TypeScript inference at ORM boundaries is precise; locate the jsonb("value").notNull() declaration in the ResourceVariable schema (the value column) and append an appropriate .$type<Record<string, any>>() or more specific type instead of leaving it as JsonValue/unknown to match how resource.config/resource.metadata are annotated.apps/workspace-engine/pkg/workspace/store/resources.go (1)
146-152: AccessResourceVariablesthrough a delegate method rather than reaching into its unexportedrepofield.
r.store.ResourceVariables.repocrosses struct-encapsulation boundaries. A thinGetByResourceIDmethod onResourceVariableskeeps the interface stable if the field is ever renamed or the implementation swapped.♻️ Proposed refactor
Add to
resource_variables.go:// GetByResourceID returns all variables associated with the given resource. func (rv *ResourceVariables) GetByResourceID(resourceID string) ([]*oapi.ResourceVariable, error) { return rv.repo.GetByResourceID(resourceID) }Then in
resources.go:-vars, err := r.store.ResourceVariables.repo.GetByResourceID(resourceId) +vars, err := r.store.ResourceVariables.GetByResourceID(resourceId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/resources.go` around lines 146 - 152, Replace the direct access to the unexported repo field (r.store.ResourceVariables.repo.GetByResourceID) with a delegate method on ResourceVariables; add a method ResourceVariables.GetByResourceID(resourceID string) ([]*oapi.ResourceVariable, error) in resource_variables.go that calls rv.repo.GetByResourceID(resourceID), and then update the call in resources.go to use r.store.ResourceVariables.GetByResourceID(resourceId) before building the variables map; keep the rest of the variables mapping logic unchanged.apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go (1)
17-19:NewRepoandRepolack exported-type doc commentsAs per coding guidelines, exported functions/types should be documented.
✏️ Proposed addition
+// Repo is a DB-backed implementation of repository.ResourceVariableRepo. type Repo struct { ctx context.Context } +// NewRepo constructs a Repo using the DB connection embedded in ctx. func NewRepo(ctx context.Context) *Repo { return &Repo{ctx: ctx} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go` around lines 17 - 19, Add exported doc comments for the exported type Repo and the constructor NewRepo: place a comment above the Repo type declaration describing its purpose (e.g., "Repo manages resource variable persistence and access within a workspace") and a comment above NewRepo explaining what it returns and any important behavior (e.g., "NewRepo creates a new Repo with the provided context"). Keep comments concise, start with the symbol name (Repo, NewRepo), and follow project/go doc style.apps/workspace-engine/pkg/workspace/store/resource_variables.go (1)
25-27:SetRepois a wide-open setter on a live service objectExporting a setter that replaces the repository mid-lifecycle is typically used only as a test seam. If this is test-only, consider restricting its visibility or naming it to signal intent (e.g., moving it to a
_test.gofile or aninternal/package), so it isn't inadvertently called in production paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/resource_variables.go` around lines 25 - 27, The exported wide-open setter ResourceVariables.SetRepo(repo repository.ResourceVariableRepo) should be restricted to test-only use; either make it unexported and used only within the package (rename to setRepo), move the setter into a _test.go helper or an internal test-only package, or provide a constructor/injection pattern so production code can't replace r.repo at runtime—update callers accordingly and keep the method name and signature only in test code if you move it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.go`:
- Around line 11-20: Change ToOapi to return (*oapi.ResourceVariable, error) and
stop discarding JSON unmarshal errors: call value.UnmarshalJSON(row.Value),
check and return the error if non-nil instead of ignoring it; construct and
return the oapi.ResourceVariable only on success. Update the two callers in
repo.go that call ToOapi to handle the new error return (propagate or wrap as
appropriate). Follow the same error-handling pattern used by ToUpsertParams and
deploymentversions.ToOapi so malformed row.Value is reported rather than
silently producing a zero-value oapi.Value.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 21-37: The Repo.Get currently swallows all DB errors and returns
(nil, false) making callers treat transient DB failures as "not found"; change
Repo.Get to return an error in addition to the value (i.e. func (r *Repo)
Get(key string) (*oapi.ResourceVariable, bool, error)), propagate/return DB
errors from db.GetQueries(...).GetResourceVariable instead of converting them to
"not found", and update callers (notably resource_variables.go's Remove) to
check the returned error and handle transient DB errors separately from the
not-found case; keep parseKey, ToOapi, and the existing logging but ensure any
non-not-found DB error is returned (or at minimum logged) rather than swallowed.
In `@apps/workspace-engine/pkg/workspace/store/resource_variables.go`:
- Around line 65-73: The loop currently sets hasChanges and calls
r.store.changeset.RecordDelete(currentVar) even if
r.repo.Remove(resourceId+"-"+key) fails, causing state divergence; change the
logic in the loop so you first attempt removal via
r.repo.Remove(resourceId+"-"+key), check err == nil, and only then set
hasChanges = true and call r.store.changeset.RecordDelete(currentVar); if
repo.Remove returns an error, log it and do not record the delete (leave
currentVar unchanged in the changeset).
In `@apps/workspace-engine/pkg/workspace/store/resources.go`:
- Around line 146-149: The call to
r.store.ResourceVariables.repo.GetByResourceID currently swallows errors and
returns an empty map; change this to log the error before returning so
DB/connectivity failures aren't silent: capture err from GetByResourceID, call
the package logger (e.g., log.Warn or log.Warnf) with a concise message
including resourceId and err, and then return
make(map[string]*oapi.ResourceVariable). Update the block around GetByResourceID
in resources.go (same function where vars is assigned) to emit that warning and
preserve the existing empty-map return behavior.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 17-19: Add exported doc comments for the exported type Repo and
the constructor NewRepo: place a comment above the Repo type declaration
describing its purpose (e.g., "Repo manages resource variable persistence and
access within a workspace") and a comment above NewRepo explaining what it
returns and any important behavior (e.g., "NewRepo creates a new Repo with the
provided context"). Keep comments concise, start with the symbol name (Repo,
NewRepo), and follow project/go doc style.
In `@apps/workspace-engine/pkg/workspace/store/resource_variables.go`:
- Around line 25-27: The exported wide-open setter
ResourceVariables.SetRepo(repo repository.ResourceVariableRepo) should be
restricted to test-only use; either make it unexported and used only within the
package (rename to setRepo), move the setter into a _test.go helper or an
internal test-only package, or provide a constructor/injection pattern so
production code can't replace r.repo at runtime—update callers accordingly and
keep the method name and signature only in test code if you move it.
In `@apps/workspace-engine/pkg/workspace/store/resources.go`:
- Around line 146-152: Replace the direct access to the unexported repo field
(r.store.ResourceVariables.repo.GetByResourceID) with a delegate method on
ResourceVariables; add a method ResourceVariables.GetByResourceID(resourceID
string) ([]*oapi.ResourceVariable, error) in resource_variables.go that calls
rv.repo.GetByResourceID(resourceID), and then update the call in resources.go to
use r.store.ResourceVariables.GetByResourceID(resourceId) before building the
variables map; keep the rest of the variables mapping logic unchanged.
In `@packages/db/src/schema/resource-variable.ts`:
- Line 18: Add a $type<> annotation to the jsonb column definition for the value
field so TypeScript inference at ORM boundaries is precise; locate the
jsonb("value").notNull() declaration in the ResourceVariable schema (the value
column) and append an appropriate .$type<Record<string, any>>() or more specific
type instead of leaving it as JsonValue/unknown to match how
resource.config/resource.metadata are annotated.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
apps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/resource_variables.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/resource_variables.sql.goapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/persistence/integration_test.goapps/workspace-engine/pkg/workspace/store/repository/db/repo.goapps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.goapps/workspace-engine/pkg/workspace/store/repository/interfaces.goapps/workspace-engine/pkg/workspace/store/repository/memory/repo.goapps/workspace-engine/pkg/workspace/store/repository/repo.goapps/workspace-engine/pkg/workspace/store/resource_variables.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/svc/workspaceconsumer/consumer.goapps/workspace-engine/test/integration/dbtest.gopackages/db/drizzle/0150_abandoned_eddie_brock.sqlpackages/db/drizzle/meta/0150_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/index.tspackages/db/src/schema/resource-variable.ts
Comment on lines
+11
to
+20
| func ToOapi(row db.ResourceVariable) *oapi.ResourceVariable { | ||
| value := oapi.Value{} | ||
| _ = value.UnmarshalJSON(row.Value) | ||
|
|
||
| return &oapi.ResourceVariable{ | ||
| ResourceId: row.ResourceID.String(), | ||
| Key: row.Key, | ||
| Value: value, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the actual file and confirm it exists
fd -t f "mapper.go" apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/Repository: ctrlplanedev/ctrlplane
Length of output: 149
🏁 Script executed:
# Search for all callers of ToOapi in the repository
rg -n "ToOapi" --type go -C 3Repository: ctrlplanedev/ctrlplane
Length of output: 27988
🏁 Script executed:
# Also check for ToUpsertParams and other exported functions in mapper.go to understand the pattern
fd -t f "mapper.go" apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/ -x cat {}Repository: ctrlplanedev/ctrlplane
Length of output: 1260
🏁 Script executed:
# Check if parseKey is used elsewhere in the resourcevariables package
fd -t f "\.go$" apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/ -x rg -l "parseKey"Repository: ctrlplanedev/ctrlplane
Length of output: 231
🏁 Script executed:
# Examine ToUpsertParams specifically to see how it handles errors
fd -t f "mapper.go" apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/ -x rg -A 10 "func ToUpsertParams"Repository: ctrlplanedev/ctrlplane
Length of output: 442
Silently discarding the JSON unmarshal error can mask data corruption.
If row.Value contains malformed JSON (manual DB edit, encoding mismatch, or upstream bug), this returns a zero-value Value with no indication of the problem. The inconsistency is notable: ToUpsertParams in the same file properly handles JSON errors, but ToOapi silently discards them. Consider propagating the error like deploymentversions.ToOapi does.
🐛 Proposed fix: return an error from ToOapi
-func ToOapi(row db.ResourceVariable) *oapi.ResourceVariable {
- value := oapi.Value{}
- _ = value.UnmarshalJSON(row.Value)
-
- return &oapi.ResourceVariable{
+func ToOapi(row db.ResourceVariable) (*oapi.ResourceVariable, error) {
+ value := oapi.Value{}
+ if err := value.UnmarshalJSON(row.Value); err != nil {
+ return nil, fmt.Errorf("unmarshal value for key %q: %w", row.Key, err)
+ }
+
+ return &oapi.ResourceVariable{
ResourceId: row.ResourceID.String(),
Key: row.Key,
Value: value,
- }
+ }, nil
}This requires updating the two callers in repo.go (lines 36 and 78) to handle the error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.go`
around lines 11 - 20, Change ToOapi to return (*oapi.ResourceVariable, error)
and stop discarding JSON unmarshal errors: call value.UnmarshalJSON(row.Value),
check and return the error if non-nil instead of ignoring it; construct and
return the oapi.ResourceVariable only on success. Update the two callers in
repo.go that call ToOapi to handle the new error return (propagate or wrap as
appropriate). Follow the same error-handling pattern used by ToUpsertParams and
deploymentversions.ToOapi so malformed row.Value is reported rather than
silently producing a zero-value oapi.Value.
Comment on lines
+21
to
+37
| func (r *Repo) Get(key string) (*oapi.ResourceVariable, bool) { | ||
| resourceID, varKey, err := parseKey(key) | ||
| if err != nil { | ||
| log.Warn("Failed to parse resource variable key", "key", key, "error", err) | ||
| return nil, false | ||
| } | ||
|
|
||
| row, err := db.GetQueries(r.ctx).GetResourceVariable(r.ctx, db.GetResourceVariableParams{ | ||
| ResourceID: resourceID, | ||
| Key: varKey, | ||
| }) | ||
| if err != nil { | ||
| return nil, false | ||
| } | ||
|
|
||
| return ToOapi(row), true | ||
| } |
Contributor
There was a problem hiding this comment.
DB errors in Get are silently swallowed as "not found"
Any DB error (connection failure, timeout, etc.) on line 33 causes Get to return nil, false, which callers treat as "not found". In resource_variables.go's Remove, this means a transient DB error during the preflight Get silently skips the delete and omits the changeset record.
Consider returning a sentinel or logging the error at minimum:
🐛 Proposed fix
row, err := db.GetQueries(r.ctx).GetResourceVariable(r.ctx, db.GetResourceVariableParams{
ResourceID: resourceID,
Key: varKey,
})
if err != nil {
+ log.Warn("Failed to get resource variable from DB", "key", key, "error", err)
return nil, false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *Repo) Get(key string) (*oapi.ResourceVariable, bool) { | |
| resourceID, varKey, err := parseKey(key) | |
| if err != nil { | |
| log.Warn("Failed to parse resource variable key", "key", key, "error", err) | |
| return nil, false | |
| } | |
| row, err := db.GetQueries(r.ctx).GetResourceVariable(r.ctx, db.GetResourceVariableParams{ | |
| ResourceID: resourceID, | |
| Key: varKey, | |
| }) | |
| if err != nil { | |
| return nil, false | |
| } | |
| return ToOapi(row), true | |
| } | |
| func (r *Repo) Get(key string) (*oapi.ResourceVariable, bool) { | |
| resourceID, varKey, err := parseKey(key) | |
| if err != nil { | |
| log.Warn("Failed to parse resource variable key", "key", key, "error", err) | |
| return nil, false | |
| } | |
| row, err := db.GetQueries(r.ctx).GetResourceVariable(r.ctx, db.GetResourceVariableParams{ | |
| ResourceID: resourceID, | |
| Key: varKey, | |
| }) | |
| if err != nil { | |
| log.Warn("Failed to get resource variable from DB", "key", key, "error", err) | |
| return nil, false | |
| } | |
| return ToOapi(row), true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`
around lines 21 - 37, The Repo.Get currently swallows all DB errors and returns
(nil, false) making callers treat transient DB failures as "not found"; change
Repo.Get to return an error in addition to the value (i.e. func (r *Repo)
Get(key string) (*oapi.ResourceVariable, bool, error)), propagate/return DB
errors from db.GetQueries(...).GetResourceVariable instead of converting them to
"not found", and update callers (notably resource_variables.go's Remove) to
check the returned error and handle transient DB errors separately from the
not-found case; keep parseKey, ToOapi, and the existing logging but ensure any
non-not-found DB error is returned (or at minimum logged) rather than swallowed.
Comment on lines
+146
to
+149
| vars, err := r.store.ResourceVariables.repo.GetByResourceID(resourceId) | ||
| if err != nil { | ||
| return make(map[string]*oapi.ResourceVariable) | ||
| } |
Contributor
There was a problem hiding this comment.
Log errors from GetByResourceID rather than silently swallowing them.
Every other error in this package (e.g., the Restore migration loop) calls log.Warn. A DB connectivity failure here will silently deliver an empty variable map to callers, making it hard to diagnose.
🛡️ Proposed fix
vars, err := r.store.ResourceVariables.repo.GetByResourceID(resourceId)
if err != nil {
+ log.Warn("Failed to fetch resource variables", "resource_id", resourceId, "error", err)
return make(map[string]*oapi.ResourceVariable)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vars, err := r.store.ResourceVariables.repo.GetByResourceID(resourceId) | |
| if err != nil { | |
| return make(map[string]*oapi.ResourceVariable) | |
| } | |
| vars, err := r.store.ResourceVariables.repo.GetByResourceID(resourceId) | |
| if err != nil { | |
| log.Warn("Failed to fetch resource variables", "resource_id", resourceId, "error", err) | |
| return make(map[string]*oapi.ResourceVariable) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/resources.go` around lines 146 -
149, The call to r.store.ResourceVariables.repo.GetByResourceID currently
swallows errors and returns an empty map; change this to log the error before
returning so DB/connectivity failures aren't silent: capture err from
GetByResourceID, call the package logger (e.g., log.Warn or log.Warnf) with a
concise message including resourceId and err, and then return
make(map[string]*oapi.ResourceVariable). Update the block around GetByResourceID
in resources.go (same function where vars is assigned) to emit that warning and
preserve the existing empty-map return behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 60-63: The DB Repo.Items() currently always returns an empty map;
change it to fetch persisted ResourceVariable records from the DB and return
them as map[string]*oapi.ResourceVariable instead of silently returning an empty
map. Implement the method to query the underlying store/DB (use the Repo
receiver's DB handle/ORM methods already used elsewhere in this repo), convert
each persisted row into an *oapi.ResourceVariable keyed by its name/ID, handle
errors (log/return appropriately or propagate) and only return an empty map on
legitimate "no rows" results; update Repo.Items() to mirror the in-memory repo
behavior rather than the current stub.
In `@apps/workspace-engine/pkg/workspace/store/repository/interfaces.go`:
- Around line 120-127: ResourceVariableRepo.Get currently returns (value, found)
which hides DB errors as "not found"; change its signature to return
(*oapi.ResourceVariable, bool, error) so callers can distinguish misses vs
transient/errors, update all implementations of ResourceVariableRepo.Get (and
any callers) to propagate DB errors as (nil, false, err) and return (nil, false,
nil) for true misses, and adjust callers to handle the error return accordingly.
In `@apps/workspace-engine/pkg/workspace/store/resource_variables.go`:
- Around line 29-33: The Upsert and Remove methods on ResourceVariables
currently swallow persistence errors by only logging them; change their
signatures to return error (e.g., func (r *ResourceVariables) Upsert(ctx
context.Context, resourceVariable *oapi.ResourceVariable) error and similarly
for Remove) and propagate the repo errors instead of just logging: call
r.repo.Set(...) and r.repo.Remove(...), if err != nil return
fmt.Errorf("upsert/remove resource variable: %w", err). Update all call sites to
handle the returned error and remove the silent log-only behavior (keep logging
where helpful but do not hide the error).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.goapps/workspace-engine/pkg/workspace/store/repository/interfaces.goapps/workspace-engine/pkg/workspace/store/repository/memory/repo.goapps/workspace-engine/pkg/workspace/store/resource_variables.go
apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go
Show resolved
Hide resolved
Comment on lines
+120
to
+127
| // ResourceVariableRepo defines the contract for resource variable storage. | ||
| type ResourceVariableRepo interface { | ||
| Get(key string) (*oapi.ResourceVariable, bool) | ||
| Set(entity *oapi.ResourceVariable) error | ||
| Remove(key string) error | ||
| Items() map[string]*oapi.ResourceVariable | ||
| GetByResourceID(resourceID string) ([]*oapi.ResourceVariable, error) | ||
| BulkUpdate(toUpsert []*oapi.ResourceVariable, toRemove []*oapi.ResourceVariable) error |
Contributor
There was a problem hiding this comment.
Expose DB read failures in ResourceVariableRepo.Get.
At Line 122, Get only returns (value, found). For DB-backed repos, transient query failures get forced into found=false, which makes operational failures indistinguishable from true misses.
Suggested contract change
type ResourceVariableRepo interface {
- Get(key string) (*oapi.ResourceVariable, bool)
+ Get(key string) (*oapi.ResourceVariable, bool, error)
Set(entity *oapi.ResourceVariable) error
Remove(key string) error
Items() map[string]*oapi.ResourceVariable
GetByResourceID(resourceID string) ([]*oapi.ResourceVariable, error)
BulkUpdate(toUpsert []*oapi.ResourceVariable, toRemove []*oapi.ResourceVariable) error
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/interfaces.go` around
lines 120 - 127, ResourceVariableRepo.Get currently returns (value, found) which
hides DB errors as "not found"; change its signature to return
(*oapi.ResourceVariable, bool, error) so callers can distinguish misses vs
transient/errors, update all implementations of ResourceVariableRepo.Get (and
any callers) to propagate DB errors as (nil, false, err) and return (nil, false,
nil) for true misses, and adjust callers to handle the error return accordingly.
Comment on lines
29
to
+33
| func (r *ResourceVariables) Upsert(ctx context.Context, resourceVariable *oapi.ResourceVariable) { | ||
| r.repo.ResourceVariables.Set(resourceVariable.ID(), resourceVariable) | ||
| if err := r.repo.Set(resourceVariable); err != nil { | ||
| log.Error("Failed to upsert resource variable", "error", err) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate persistence errors from Upsert and Remove instead of logging-only.
These methods now operate against a backend that can fail, but callers cannot observe failure because errors are only logged. This can make upstream flows assume writes/deletes succeeded when they did not.
Proposed API adjustment
-func (r *ResourceVariables) Upsert(ctx context.Context, resourceVariable *oapi.ResourceVariable) {
+func (r *ResourceVariables) Upsert(ctx context.Context, resourceVariable *oapi.ResourceVariable) error {
if err := r.repo.Set(resourceVariable); err != nil {
log.Error("Failed to upsert resource variable", "error", err)
- return
+ return err
}
r.store.changeset.RecordUpsert(resourceVariable)
+ return nil
}
-func (r *ResourceVariables) Remove(ctx context.Context, resourceId string, key string) {
+func (r *ResourceVariables) Remove(ctx context.Context, resourceId string, key string) error {
resourceVariable, ok := r.repo.Get(resourceId + "-" + key)
if !ok || resourceVariable == nil {
- return
+ return nil
}
if err := r.repo.Remove(resourceId + "-" + key); err != nil {
log.Error("Failed to remove resource variable", "error", err)
- return
+ return err
}
r.store.changeset.RecordDelete(resourceVariable)
+ return nil
}Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/resource_variables.go` around lines
29 - 33, The Upsert and Remove methods on ResourceVariables currently swallow
persistence errors by only logging them; change their signatures to return error
(e.g., func (r *ResourceVariables) Upsert(ctx context.Context, resourceVariable
*oapi.ResourceVariable) error and similarly for Remove) and propagate the repo
errors instead of just logging: call r.repo.Set(...) and r.repo.Remove(...), if
err != nil return fmt.Errorf("upsert/remove resource variable: %w", err). Update
all call sites to handle the returned error and remove the silent log-only
behavior (keep logging where helpful but do not hide the error).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go (1)
22-38:⚠️ Potential issue | 🟠 MajorDB errors in
Getare still silently swallowed.Lines 33–35 return
nil, falseon any DB error without logging, making transient failures (connection loss, timeout) indistinguishable from a genuine not-found. This was raised in a prior review and remains unaddressed.🐛 Proposed fix
row, err := db.GetQueries(r.ctx).GetResourceVariable(r.ctx, db.GetResourceVariableParams{ ResourceID: resourceID, Key: varKey, }) if err != nil { + log.Warn("Failed to get resource variable from DB", "key", key, "error", err) return nil, false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go` around lines 22 - 38, The Get method (Repo.Get) currently swallows all DB errors after calling db.GetQueries(...).GetResourceVariable; update it to distinguish not-found from other DB errors by checking for sql.ErrNoRows (or errors.Is(err, sql.ErrNoRows)) and keep returning nil,false for not-found, but for any other error log it with context (include resourceID, varKey and the error) via the existing logger (e.g., log.Error or log.Warn) before returning nil,false; ensure ToOapi(row) is still returned on success.
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/db/queries/resource_variables.sql (1)
16-24: Consider addingORDER BYfor deterministic results in list queries.Without
ORDER BY, list results are non-deterministic. ForListResourceVariablesByWorkspaceID, this has no effect since the result feeds amap[string]*oapi.ResourceVariable, butListResourceVariablesByResourceIDreturns a[]*oapi.ResourceVariableslice whose order may matter to callers.✨ Proposed fix
-- name: ListResourceVariablesByResourceID :many SELECT resource_id, key, value, workspace_id FROM resource_variable -WHERE resource_id = $1; +WHERE resource_id = $1 +ORDER BY key;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/queries/resource_variables.sql` around lines 16 - 24, The ListResourceVariablesByResourceID query returns a slice and needs a deterministic order—update the SQL for ListResourceVariablesByResourceID (and optionally ListResourceVariablesByWorkspaceID) to include an ORDER BY clause (e.g., ORDER BY key ASC, workspace_id ASC or whichever stable column(s) make sense) so results are consistently ordered; modify the queries named ListResourceVariablesByResourceID and ListResourceVariablesByWorkspaceID in resource_variables.sql to add the chosen ORDER BY.apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go (1)
13-20: Document exported types and constructor per project convention.
Repo,NewRepo, and the public methods (Get,Set,Remove,Items,BulkUpdate,GetByResourceID) have no doc comments. The CLAUDE.md guidelines require documenting exported functions/types/methods.Based on learnings: "Applies to apps/workspace-engine/**/.go: Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods."*
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go` around lines 13 - 20, Add doc comments for the exported type Repo, its constructor NewRepo, and the public methods Get, Set, Remove, Items, BulkUpdate, and GetByResourceID: for each exported symbol (Repo, NewRepo, Get, Set, Remove, Items, BulkUpdate, GetByResourceID) add a clear Go doc comment that starts with the symbol name, describes what it does, why it exists or any non-obvious behavior, and mentions TODO/FIXME or important context or invariants for complex logic; ensure comments follow the project CLAUDE.md guidance (explain why, document complex logic/algorithms, and include non-obvious context) so readers and godoc consumers can understand the API and design intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 82-115: BulkUpdate currently stamps r.workspaceID into upsert
params via ToUpsertParams(r.workspaceID, rv) which can silently write a
different workspace for new rows; before calling
ToUpsertParams/UpsertResourceVariable, verify that the incoming rv belongs to
this repo's workspace and return an error if it does not. Concretely, in
Repo.BulkUpdate iterate toUpsert and for each rv derive the workspace from rv
(e.g., parse rv.ResourceId or check an rv.WorkspaceId field) and compare it to
r.workspaceID; if they differ, return a clear error (instead of proceeding to
call ToUpsertParams/UpsertResourceVariable). Ensure the check happens prior to
calling ToUpsertParams so no incorrect workspace is persisted.
---
Duplicate comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 22-38: The Get method (Repo.Get) currently swallows all DB errors
after calling db.GetQueries(...).GetResourceVariable; update it to distinguish
not-found from other DB errors by checking for sql.ErrNoRows (or errors.Is(err,
sql.ErrNoRows)) and keep returning nil,false for not-found, but for any other
error log it with context (include resourceID, varKey and the error) via the
existing logger (e.g., log.Error or log.Warn) before returning nil,false; ensure
ToOapi(row) is still returned on success.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/db/queries/resource_variables.sql`:
- Around line 16-24: The ListResourceVariablesByResourceID query returns a slice
and needs a deterministic order—update the SQL for
ListResourceVariablesByResourceID (and optionally
ListResourceVariablesByWorkspaceID) to include an ORDER BY clause (e.g., ORDER
BY key ASC, workspace_id ASC or whichever stable column(s) make sense) so
results are consistently ordered; modify the queries named
ListResourceVariablesByResourceID and ListResourceVariablesByWorkspaceID in
resource_variables.sql to add the chosen ORDER BY.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 13-20: Add doc comments for the exported type Repo, its
constructor NewRepo, and the public methods Get, Set, Remove, Items, BulkUpdate,
and GetByResourceID: for each exported symbol (Repo, NewRepo, Get, Set, Remove,
Items, BulkUpdate, GetByResourceID) add a clear Go doc comment that starts with
the symbol name, describes what it does, why it exists or any non-obvious
behavior, and mentions TODO/FIXME or important context or invariants for complex
logic; ensure comments follow the project CLAUDE.md guidance (explain why,
document complex logic/algorithms, and include non-obvious context) so readers
and godoc consumers can understand the API and design intent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/resource_variables.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/resource_variables.sql.goapps/workspace-engine/pkg/workspace/store/repository/db/repo.goapps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.gopackages/db/drizzle/0151_misty_the_fallen.sqlpackages/db/drizzle/meta/0150_snapshot.jsonpackages/db/drizzle/meta/0151_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/resource-variable.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/db/drizzle/meta/_journal.json
- apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.go
- apps/workspace-engine/pkg/db/models.go
- apps/workspace-engine/pkg/db/queries/schema.sql
Comment on lines
82
to
115
| func (r *Repo) BulkUpdate(toUpsert []*oapi.ResourceVariable, toRemove []*oapi.ResourceVariable) error { | ||
| tx, err := db.GetPool(r.ctx).Begin(r.ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("begin transaction: %w", err) | ||
| } | ||
| defer tx.Rollback(r.ctx) | ||
|
|
||
| q := db.New(tx) | ||
|
|
||
| for _, rv := range toRemove { | ||
| rid, err := uuid.Parse(rv.ResourceId) | ||
| if err != nil { | ||
| return fmt.Errorf("parse resource_id for remove: %w", err) | ||
| } | ||
| if err := q.DeleteResourceVariable(r.ctx, db.DeleteResourceVariableParams{ | ||
| ResourceID: rid, | ||
| Key: rv.Key, | ||
| }); err != nil { | ||
| return fmt.Errorf("delete resource variable %s-%s: %w", rv.ResourceId, rv.Key, err) | ||
| } | ||
| } | ||
|
|
||
| for _, rv := range toUpsert { | ||
| params, err := ToUpsertParams(r.workspaceID, rv) | ||
| if err != nil { | ||
| return fmt.Errorf("convert to upsert params: %w", err) | ||
| } | ||
| if err := q.UpsertResourceVariable(r.ctx, params); err != nil { | ||
| return fmt.Errorf("upsert resource variable %s-%s: %w", rv.ResourceId, rv.Key, err) | ||
| } | ||
| } | ||
|
|
||
| return tx.Commit(r.ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
BulkUpdate silently ignores workspace_id mismatch on new inserts.
ToUpsertParams(r.workspaceID, rv) always stamps the repo's workspaceID onto the upsert, regardless of rv.ResourceId's actual workspace. For an existing (resource_id, key) row, the ON CONFLICT path only updates value, so there's no harm. For a truly new row, however, the caller's intent and the persisted workspace_id could silently diverge if the caller passes a variable belonging to a different workspace. Consider asserting that rv belongs to this repo's workspace before upserting, or document the contract clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`
around lines 82 - 115, BulkUpdate currently stamps r.workspaceID into upsert
params via ToUpsertParams(r.workspaceID, rv) which can silently write a
different workspace for new rows; before calling
ToUpsertParams/UpsertResourceVariable, verify that the incoming rv belongs to
this repo's workspace and return an error if it does not. Concretely, in
Repo.BulkUpdate iterate toUpsert and for each rv derive the workspace from rv
(e.g., parse rv.ResourceId or check an rv.WorkspaceId field) and compare it to
r.workspaceID; if they differ, return a clear error (instead of proceeding to
call ToUpsertParams/UpsertResourceVariable). Ensure the check happens prior to
calling ToUpsertParams so no incorrect workspace is persisted.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go (1)
22-35:⚠️ Potential issue | 🟠 Major
Getstill masks DB failures as “not found”.At Line 33, every DB error is converted to
false, so transient DB failures become indistinguishable from missing records. This can cause incorrect control flow in callers.🐛 Minimal fix (preserve current interface, improve observability)
func (r *Repo) Get(key string) (*oapi.ResourceVariable, bool) { resourceID, varKey, err := parseKey(key) if err != nil { log.Warn("Failed to parse resource variable key", "key", key, "error", err) return nil, false } row, err := db.GetQueries(r.ctx).GetResourceVariable(r.ctx, db.GetResourceVariableParams{ ResourceID: resourceID, Key: varKey, }) if err != nil { + log.Error("Failed to fetch resource variable", "key", key, "error", err) return nil, false } return ToOapi(row), true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go` around lines 22 - 35, Repo.Get currently treats all DB errors as "not found"; update the error handling after calling db.GetQueries(...).GetResourceVariable so only a not-found error (errors.Is(err, sql.ErrNoRows) or the DB package's equivalent) returns (nil, false), while any other error is logged at error level with context (resourceID, varKey, error) before returning (nil, false) to preserve the method signature; keep parseKey usage intact and ensure you import and use the appropriate errors package for comparison.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/db/queries/resource_variables.sql (1)
16-19: Add explicit ordering to list queries for deterministic results.
ListResourceVariablesByResourceIDandListResourceVariablesByWorkspaceIDcurrently return unspecified row order. AddingORDER BY key(and for workspace list,ORDER BY rv.resource_id, rv.key) will make downstream behavior stable.💡 Proposed SQL tweak
-- name: ListResourceVariablesByResourceID :many SELECT resource_id, key, value FROM resource_variable WHERE resource_id = $1; +ORDER BY key; -- name: ListResourceVariablesByWorkspaceID :many SELECT rv.resource_id, rv.key, rv.value FROM resource_variable rv INNER JOIN resource r ON r.id = rv.resource_id -WHERE r.workspace_id = $1; +WHERE r.workspace_id = $1 +ORDER BY rv.resource_id, rv.key;Also applies to: 25-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/queries/resource_variables.sql` around lines 16 - 19, The ListResourceVariablesByResourceID and ListResourceVariablesByWorkspaceID queries return rows in unspecified order; update the SQL for the named queries to add deterministic ordering: in ListResourceVariablesByResourceID (query name: ListResourceVariablesByResourceID) append "ORDER BY key" to the SELECT, and in ListResourceVariablesByWorkspaceID (query name: ListResourceVariablesByWorkspaceID) append "ORDER BY rv.resource_id, rv.key" so results are stable for downstream consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.go`:
- Around line 22-35: Repo.Get currently treats all DB errors as "not found";
update the error handling after calling db.GetQueries(...).GetResourceVariable
so only a not-found error (errors.Is(err, sql.ErrNoRows) or the DB package's
equivalent) returns (nil, false), while any other error is logged at error level
with context (resourceID, varKey, error) before returning (nil, false) to
preserve the method signature; keep parseKey usage intact and ensure you import
and use the appropriate errors package for comparison.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/db/queries/resource_variables.sql`:
- Around line 16-19: The ListResourceVariablesByResourceID and
ListResourceVariablesByWorkspaceID queries return rows in unspecified order;
update the SQL for the named queries to add deterministic ordering: in
ListResourceVariablesByResourceID (query name:
ListResourceVariablesByResourceID) append "ORDER BY key" to the SELECT, and in
ListResourceVariablesByWorkspaceID (query name:
ListResourceVariablesByWorkspaceID) append "ORDER BY rv.resource_id, rv.key" so
results are stable for downstream consumers.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/resource_variables.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/resource_variables.sql.goapps/workspace-engine/pkg/workspace/store/repository/db/repo.goapps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/repo.gopackages/db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/workspace-engine/pkg/db/models.go
- apps/workspace-engine/pkg/workspace/store/repository/db/resourcevariables/mapper.go
- apps/workspace-engine/pkg/db/queries/schema.sql
- packages/db/drizzle/meta/_journal.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
1 participant
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