Conversation
added 30 commits
April 4, 2026 18:20
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Replace errors.Is with core.Is in tenant_test.go to remove the banned "errors" import. Add usage-example comments to all internal helper functions across cache.go, client.go, entitlement.go, package.go, scope.go, tenant.go, usage.go, user.go, and workspace.go per AX principle 2 (comments as usage examples). Fix fmt.Printf reference in UsageSummaryItem comment to use core.Println instead. Co-Authored-By: Virgil <virgil@lethean.io>
- Bump dappco.re/go/* deps to v0.8.0-alpha.1 in go.mod (any forge.lthn.ai/core/* paths migrated to canonical dappco.re/go/* form) - Update Go source imports across 2 .go files - Add tests/cli/tenant/Taskfile.yaml AX-10 scaffold (build/vet/test under default deps), per RFC-CORE-008-AGENT-EXPERIENCE.md §10 Co-Authored-By: Athena <athena@lthn.ai>
bash /tmp/v090/audit.sh . → verdict: COMPLIANT (all 7 dimensions zero). Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughThis PR substantially enhances the tenant subsystem with Core framework integration, persistent caching via go-store, refactored error handling using core.E utilities, expanded entitlement limit computation with feature code normalisation, and comprehensive compliance and unit test coverage. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scope.go (1)
136-145:⚠️ Potential issue | 🟡 Minor
parseInt64returns 0 for empty string and lacks overflow protection.Two edge cases:
- Empty string
""returns(0, nil)which may be unintended since workspace ID 0 is likely invalid.- Very large numeric strings could cause integer overflow without detection.
🛡️ Proposed fix to handle edge cases
func parseInt64(value string) (int64, error) { + if value == "" { + return 0, ErrNoWorkspaceContext + } var result int64 for _, r := range value { if r < '0' || r > '9' { return 0, ErrNoWorkspaceContext } + next := result*10 + int64(r-'0') + if next < result { + return 0, ErrNoWorkspaceContext // overflow + } - result = result*10 + int64(r-'0') + result = next } return result, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scope.go` around lines 136 - 145, The parseInt64 function currently accepts empty strings and doesn't detect overflow; update parseInt64 to first return ErrNoWorkspaceContext for empty value, then during the loop check before result = result*10 + digit that result <= (math.MaxInt64-int64(digit))/10 (or equivalent) to detect overflow and return ErrNoWorkspaceContext on overflow or non-digit, keeping the same error symbol ErrNoWorkspaceContext for all invalid cases; ensure imports are added if using math.MaxInt64 and keep the function signature parseInt64(value string) (int64, error).
🧹 Nitpick comments (12)
workspace.go (1)
114-130: Shallow copy limitation for nested Settings values.Similar to
cloneMetadatainusage.go, the Settings map copy is shallow. Nested structures within Settings values would share references with the original. Consider documenting this limitation or implementing deep copy if full isolation is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspace.go` around lines 114 - 130, The cloneWorkspace function currently performs a shallow copy of the Settings map which means nested objects in Settings will remain shared with the original; update cloneWorkspace to either call the deep-copy logic used by cloneMetadata (from usage.go) or introduce a deepCopyValue helper to recursively copy map/slice/struct-like values when populating clone.Settings so nested structures are duplicated, or alternatively add a clear comment above cloneWorkspace documenting the shallow-copy limitation and when a full deep copy is required.go.mod (1)
40-40: Alpha dependency in production code.Using
v0.8.0-alpha.1fordappco.re/go/storeintroduces potential instability. Consider whether this alpha version has been sufficiently tested or if a stable release is available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 40, The go.mod lists an alpha dependency "dappco.re/go/store v0.8.0-alpha.1" which is unsafe for production; update the module version in go.mod to a stable release (e.g., the latest non-alpha tag such as v0.8.0 or newer) or, if no stable exists, document and lock the dependency in go.sum and add a comment why the alpha is required, and run `go mod tidy` and `go test ./...` to verify nothing breaks; locate the dappco.re/go/store entry in go.mod to perform this change.usage.go (1)
77-89: Shallow copy may not fully isolate nested structures.
cloneMetadataperforms a shallow copy, so nested maps or slices withinmetadatavalues would still share references with the original. If deep isolation is required, consider a recursive deep-copy or document this limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usage.go` around lines 77 - 89, The current cloneMetadata function does only a shallow copy, leaving nested maps/slices shared; update cloneMetadata to perform a deep copy of common composite types (handle map[string]any recursively and []any slices by allocating new containers and deep-copying their elements, while copying primitives by value) so nested structures are isolated, or alternatively add a clear comment on cloneMetadata documenting that it intentionally performs only a shallow copy; reference the cloneMetadata function name and ensure the implementation preserves nil handling and original map keys/types.package.go (1)
31-39: Consider pre-normalizing feature codes to avoid repeated string operations.Both
GetFeatureLimitandincludesFeaturecallnormalizedFeatureCodeon eachf.FeatureCodeduring iteration. If features are stored with normalised codes, this redundant processing could be avoided. However, the current defensive approach ensures correctness regardless of data source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.go` around lines 31 - 39, The code repeatedly calls normalizedFeatureCode during lookups; to avoid redundant string ops, normalize feature codes once when features are ingested/stored (e.g., in the Package constructor/feature setter or AddFeature routine) so Package.Features contains normalized FeatureCode values, and then update GetFeatureLimit and includesFeature to compare f.FeatureCode directly without calling normalizedFeatureCode inside the loop; alternatively (quick fix), normalize each f.FeatureCode to a local variable once before the comparison in Package.GetFeatureLimit and includesFeature to avoid double normalization per iteration.scope.go (1)
94-98: Usecore.Isfor error comparison to align with codebase conventions.Direct equality comparison at line 95 will not work with wrapped errors. The test suite consistently uses
core.Is(err, ErrWorkspaceNotFound)for this error check.♻️ Proposed fix
workspace, err := s.tenant.GetWorkspace(ctx, slug) if err != nil { - if err == ErrWorkspaceNotFound { + if core.Is(err, ErrWorkspaceNotFound) { return ErrNoWorkspaceContext } return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scope.go` around lines 94 - 98, Replace the direct equality check against ErrWorkspaceNotFound with the project's error comparison helper: change the condition in the error handling block (the branch that currently reads `if err == ErrWorkspaceNotFound`) to use core.Is(err, ErrWorkspaceNotFound) so wrapped errors are detected; keep the same return behavior (return ErrNoWorkspaceContext) when the check matches. Reference: the error handling block in scope.go that returns ErrNoWorkspaceContext on workspace-not-found.client.go (2)
93-96: Timeout detection relies on fragile string matching.The check
core.Contains(err.Error(), "timeout")is locale-dependent and could match unrelated errors containing "timeout" in their message. Consider usingerrors.Iswithcontext.DeadlineExceededor checking fornet.ErrorwithTimeout()method:var netErr net.Error if errors.Is(err, context.DeadlineExceeded) || (errors.As(err, &netErr) && netErr.Timeout()) { return nil, 0, ErrClientTimeout }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 93 - 96, Replace the fragile string-match timeout check around the error handling (the block that currently checks err == context.DeadlineExceeded || core.Contains(err.Error(), context.DeadlineExceeded.Error()) || core.Contains(err.Error(), "timeout")) with a robust check using errors.Is to detect context.DeadlineExceeded and errors.As to detect net.Error and call its Timeout() method; update the branch that returns ErrClientTimeout to use this new check and leave the final return as core.E("tenant", "api request failed", err).
146-156: Re-marshalling nested data is inefficient.The pattern of marshalling
nestedback to JSON (line 147-151) and then unmarshalling (line 152) adds overhead. Consider usingmapstructureor direct JSON raw message handling if performance is critical.For a network-bound API client, this overhead is likely negligible compared to HTTP latency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 146 - 156, The current code double-serializes by calling core.JSONMarshal on envelope["data"] and then core.JSONUnmarshalString into target; replace that re-marshalling with a direct decode from the in-memory structure: detect that nested is a map[string]interface{} (or []interface{} for arrays) and use a decoder like mapstructure.Decode(nested, target) (or json.RawMessage handling if you prefer), removing the core.JSONMarshal/core.JSONUnmarshalString pair; ensure to import and use mapstructure (or the chosen decoder) and keep the same error returns when decode fails so the surrounding logic (envelope, nested, target) remains unchanged.tenant.go (1)
130-131: Float64 duration conversion truncates fractional seconds.
time.Duration(typed) * time.Secondtruncates the fractional part (e.g.,1.5becomes1snot1.5s). If sub-second timeout precision is needed, consider:return time.Duration(typed * float64(time.Second))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tenant.go` around lines 130 - 131, The switch branch handling the float64 duration (the "case float64" branch) currently converts by multiplying a time.Duration by time.Second which truncates fractional seconds; change the conversion to first multiply the float64 value by float64(time.Second) and then cast that result to time.Duration so sub-second precision is preserved (update the float64 case where durations are parsed/converted).cache.go (3)
263-264: Type assertion silently ignores failures.The pattern
workspace, _ := entry.value.(*Workspace)discards the boolean indicating whether the assertion succeeded. If a bug causes the wrong type to be stored, this would silently returnnil(appearing as a cache miss) rather than surfacing the error.This is acceptable for cache resilience but consider logging type mismatches during development to catch bugs early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache.go` around lines 263 - 264, The type assertion for entry.value into *Workspace should check the boolean result instead of discarding it; modify the code around entry.value, the cloneWorkspace call, and Workspace typing to capture ok (e.g., workspace, ok := entry.value.(*Workspace)), and if ok is false log a warning or debug message about the unexpected type (during development) and return nil, false to avoid silently treating a type-mismatch as a cache hit; when ok is true continue to return cloneWorkspace(workspace), true.
245-265: TOCTOU race in lock upgrade pattern.The pattern of RLock → read → RUnlock → Lock → delete (lines 245-253) has a time-of-check to time-of-use race. Between releasing the read lock and acquiring the write lock, another goroutine could call
SetWorkspacewith the same UUID. The subsequentdeleteon line 251 would then remove freshly-set data.For a cache, the consequence is merely an unnecessary cache miss on the next read, which is acceptable. However, if stricter consistency is desired, consider using a single write lock for the entire operation when expiry is detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache.go` around lines 245 - 265, The TOCTOU comes from releasing c.lock RLock then acquiring c.lock Lock before deleting from c.workspacesByUUID; change the flow so when an entry is present but expired you acquire the write lock and re-check the map/entry before deleting to avoid removing a freshly SetWorkspace: i.e. in the branch where entry.expired(c.now()) release the RLock, immediately acquire c.lock (write), re-read c.workspacesByUUID[uuid] and its expired() status, then delete only if it still exists and is still expired; keep the rest of the logic (readJSON, SetWorkspace, cloneWorkspace) unchanged.
201-216: Partial cleanup on store deletion error leaves inconsistent state.If
deleteStoreGroupfails partway through the loop (e.g., lines 204-206 or 212-214), the in-memory maps have already been modified but the function returns early. This leaves the cache in a partially-cleaned state where some old mappings are removed from memory but not from the store.Consider collecting all deletions and attempting them after the in-memory cleanup, or accepting that cache inconsistency is tolerable here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache.go` around lines 201 - 216, Collect workspace IDs and slugs to remove first (e.g., build slices idsToDelete and slugsToDelete by iterating c.workspaceIDs and c.workspaceSlugs and matching ws.UUID), then remove those keys from the in-memory maps, and only after that loop call c.deleteStoreGroup for each workspaceIDGroup(id) and workspaceSlugGroup(slug); if any deleteStoreGroup call returns an error, restore the removed entries back into c.workspaceIDs/c.workspaceSlugs (rollback) before returning the error to avoid leaving the cache partially cleaned. Ensure you reference c.workspaceIDs, c.workspaceSlugs, deleteStoreGroup, workspaceIDGroup, workspaceSlugGroup and ws.UUID in the changes.entitlement.go (1)
61-70:AsError()no longer includes denial reason.The change from wrapping
Reasonto returning bareErrEntitlementDeniedmeans callers cannot extract the denial reason from the error alone. They must inspectEntitlementResult.Reasonbefore callingAsError():result := svc.Can(ctx, ws, "pages", 1) if result.IsDenied() { log.Printf("denied: %s", result.Reason) // Must access before AsError() return result.AsError() }This is acceptable if documented, but could be surprising for callers expecting error message to contain the reason.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@entitlement.go` around lines 61 - 70, The AsError method on EntitlementResult currently returns only ErrEntitlementDenied and drops the denial Reason; change AsError to include the Reason in the returned error (but preserve sentinel behavior) by returning ErrEntitlementDenied wrapped with the reason when r.Allowed is false (e.g. use fmt.Errorf("%w: %s", ErrEntitlementDenied, r.Reason) or similar), and fall back to returning ErrEntitlementDenied unwrapped when Reason is empty so callers can still use errors.Is(err, ErrEntitlementDenied) while getting human-readable reason text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@errors.go`:
- Around line 9-13: Update the doc comments for the sentinel errors (e.g.,
ErrNoWorkspaceContext and the other ErrXxx variables defined via core.E()) to
show the correct comparison pattern using core.Is(err, tenant.ErrXxx) instead of
direct equality (err == tenant.ErrXxx), and modify the equality check in
scope.go where it currently does if err == ErrWorkspaceNotFound to use
core.Is(err, ErrWorkspaceNotFound) so sentinel comparisons work with errors
created by core.E().
In `@tenant.go`:
- Around line 333-347: The code returns early when
tenantService.cache.InvalidateWorkspace(wsUUID) errors, which skips cleaning
tenantService.alertState; change the error handling in the InvalidateWorkspace
call inside the function containing tenantService.cache.InvalidateWorkspace to
not return immediately—either log the error (using the existing logger) and
continue to the alertState cleanup or collect/propagate the error after cleanup;
ensure tenantService.lock, tenantService.alertState, and the
hasAlertPrefix(wsUUID) loop still run (acquire lock/unlock) even if
InvalidateWorkspace failed.
In `@usage.go`:
- Around line 24-27: newUsageRecord currently silently coerces non-positive
quantity to 1 which can hide caller bugs; change newUsageRecord to validate
input and return an error instead of mutating the value: update the signature to
func newUsageRecord(workspaceID int64, featureCode string, quantity int, userID
*int64, metadata map[string]any) (UsageRecord, error), check if quantity <= 0
and return a clear error (e.g., fmt.Errorf("invalid quantity: %d", quantity)),
and propagate the error to callers (update any call sites to handle the error)
so invalid inputs are surfaced rather than silently corrected.
---
Outside diff comments:
In `@scope.go`:
- Around line 136-145: The parseInt64 function currently accepts empty strings
and doesn't detect overflow; update parseInt64 to first return
ErrNoWorkspaceContext for empty value, then during the loop check before result
= result*10 + digit that result <= (math.MaxInt64-int64(digit))/10 (or
equivalent) to detect overflow and return ErrNoWorkspaceContext on overflow or
non-digit, keeping the same error symbol ErrNoWorkspaceContext for all invalid
cases; ensure imports are added if using math.MaxInt64 and keep the function
signature parseInt64(value string) (int64, error).
---
Nitpick comments:
In `@cache.go`:
- Around line 263-264: The type assertion for entry.value into *Workspace should
check the boolean result instead of discarding it; modify the code around
entry.value, the cloneWorkspace call, and Workspace typing to capture ok (e.g.,
workspace, ok := entry.value.(*Workspace)), and if ok is false log a warning or
debug message about the unexpected type (during development) and return nil,
false to avoid silently treating a type-mismatch as a cache hit; when ok is true
continue to return cloneWorkspace(workspace), true.
- Around line 245-265: The TOCTOU comes from releasing c.lock RLock then
acquiring c.lock Lock before deleting from c.workspacesByUUID; change the flow
so when an entry is present but expired you acquire the write lock and re-check
the map/entry before deleting to avoid removing a freshly SetWorkspace: i.e. in
the branch where entry.expired(c.now()) release the RLock, immediately acquire
c.lock (write), re-read c.workspacesByUUID[uuid] and its expired() status, then
delete only if it still exists and is still expired; keep the rest of the logic
(readJSON, SetWorkspace, cloneWorkspace) unchanged.
- Around line 201-216: Collect workspace IDs and slugs to remove first (e.g.,
build slices idsToDelete and slugsToDelete by iterating c.workspaceIDs and
c.workspaceSlugs and matching ws.UUID), then remove those keys from the
in-memory maps, and only after that loop call c.deleteStoreGroup for each
workspaceIDGroup(id) and workspaceSlugGroup(slug); if any deleteStoreGroup call
returns an error, restore the removed entries back into
c.workspaceIDs/c.workspaceSlugs (rollback) before returning the error to avoid
leaving the cache partially cleaned. Ensure you reference c.workspaceIDs,
c.workspaceSlugs, deleteStoreGroup, workspaceIDGroup, workspaceSlugGroup and
ws.UUID in the changes.
In `@client.go`:
- Around line 93-96: Replace the fragile string-match timeout check around the
error handling (the block that currently checks err == context.DeadlineExceeded
|| core.Contains(err.Error(), context.DeadlineExceeded.Error()) ||
core.Contains(err.Error(), "timeout")) with a robust check using errors.Is to
detect context.DeadlineExceeded and errors.As to detect net.Error and call its
Timeout() method; update the branch that returns ErrClientTimeout to use this
new check and leave the final return as core.E("tenant", "api request failed",
err).
- Around line 146-156: The current code double-serializes by calling
core.JSONMarshal on envelope["data"] and then core.JSONUnmarshalString into
target; replace that re-marshalling with a direct decode from the in-memory
structure: detect that nested is a map[string]interface{} (or []interface{} for
arrays) and use a decoder like mapstructure.Decode(nested, target) (or
json.RawMessage handling if you prefer), removing the
core.JSONMarshal/core.JSONUnmarshalString pair; ensure to import and use
mapstructure (or the chosen decoder) and keep the same error returns when decode
fails so the surrounding logic (envelope, nested, target) remains unchanged.
In `@entitlement.go`:
- Around line 61-70: The AsError method on EntitlementResult currently returns
only ErrEntitlementDenied and drops the denial Reason; change AsError to include
the Reason in the returned error (but preserve sentinel behavior) by returning
ErrEntitlementDenied wrapped with the reason when r.Allowed is false (e.g. use
fmt.Errorf("%w: %s", ErrEntitlementDenied, r.Reason) or similar), and fall back
to returning ErrEntitlementDenied unwrapped when Reason is empty so callers can
still use errors.Is(err, ErrEntitlementDenied) while getting human-readable
reason text.
In `@go.mod`:
- Line 40: The go.mod lists an alpha dependency "dappco.re/go/store
v0.8.0-alpha.1" which is unsafe for production; update the module version in
go.mod to a stable release (e.g., the latest non-alpha tag such as v0.8.0 or
newer) or, if no stable exists, document and lock the dependency in go.sum and
add a comment why the alpha is required, and run `go mod tidy` and `go test
./...` to verify nothing breaks; locate the dappco.re/go/store entry in go.mod
to perform this change.
In `@package.go`:
- Around line 31-39: The code repeatedly calls normalizedFeatureCode during
lookups; to avoid redundant string ops, normalize feature codes once when
features are ingested/stored (e.g., in the Package constructor/feature setter or
AddFeature routine) so Package.Features contains normalized FeatureCode values,
and then update GetFeatureLimit and includesFeature to compare f.FeatureCode
directly without calling normalizedFeatureCode inside the loop; alternatively
(quick fix), normalize each f.FeatureCode to a local variable once before the
comparison in Package.GetFeatureLimit and includesFeature to avoid double
normalization per iteration.
In `@scope.go`:
- Around line 94-98: Replace the direct equality check against
ErrWorkspaceNotFound with the project's error comparison helper: change the
condition in the error handling block (the branch that currently reads `if err
== ErrWorkspaceNotFound`) to use core.Is(err, ErrWorkspaceNotFound) so wrapped
errors are detected; keep the same return behavior (return
ErrNoWorkspaceContext) when the check matches. Reference: the error handling
block in scope.go that returns ErrNoWorkspaceContext on workspace-not-found.
In `@tenant.go`:
- Around line 130-131: The switch branch handling the float64 duration (the
"case float64" branch) currently converts by multiplying a time.Duration by
time.Second which truncates fractional seconds; change the conversion to first
multiply the float64 value by float64(time.Second) and then cast that result to
time.Duration so sub-second precision is preserved (update the float64 case
where durations are parsed/converted).
In `@usage.go`:
- Around line 77-89: The current cloneMetadata function does only a shallow
copy, leaving nested maps/slices shared; update cloneMetadata to perform a deep
copy of common composite types (handle map[string]any recursively and []any
slices by allocating new containers and deep-copying their elements, while
copying primitives by value) so nested structures are isolated, or alternatively
add a clear comment on cloneMetadata documenting that it intentionally performs
only a shallow copy; reference the cloneMetadata function name and ensure the
implementation preserves nil handling and original map keys/types.
In `@workspace.go`:
- Around line 114-130: The cloneWorkspace function currently performs a shallow
copy of the Settings map which means nested objects in Settings will remain
shared with the original; update cloneWorkspace to either call the deep-copy
logic used by cloneMetadata (from usage.go) or introduce a deepCopyValue helper
to recursively copy map/slice/struct-like values when populating clone.Settings
so nested structures are duplicated, or alternatively add a clear comment above
cloneWorkspace documenting the shallow-copy limitation and when a full deep copy
is required.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d5b04c2-ce1e-4671-8305-3dfc1b361239
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.gitignorealert.goax7_compliance_test.goboost.gocache.goclient.goentitlement.goerrors.gofeature.gogo.modpackage.goscope.gotenant.gotenant_test.gotests/cli/tenant/Taskfile.yamlusage.gouser.goworkspace.go
Comment on lines
9
to
+13
| // ErrNoWorkspaceContext is returned when context carries no workspace. | ||
| // | ||
| // ws, err := tenant.WorkspaceFromCtx(ctx) | ||
| // // err == tenant.ErrNoWorkspaceContext when no workspace was injected | ||
| var ErrNoWorkspaceContext = errors.New("tenant: no workspace in context") | ||
| var ErrNoWorkspaceContext = core.E("tenant", "no workspace in context", nil) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for direct equality comparisons with tenant sentinel errors
rg -n --type=go '==\s*(Err(NoWorkspaceContext|NoUserContext|WorkspaceNotFound|FeatureNotFound|EntitlementDenied|ClientTimeout)|tenant\.Err)'
rg -n --type=go '(Err(NoWorkspaceContext|NoUserContext|WorkspaceNotFound|FeatureNotFound|EntitlementDenied|ClientTimeout)|tenant\.Err)\s*=='Repository: dAppCore/go-tenant
Length of output: 437
🏁 Script executed:
cat -n errors.goRepository: dAppCore/go-tenant
Length of output: 1929
🏁 Script executed:
# Find the core.E implementation or import to understand error wrapping
rg -n 'core\.E|import.*core' --type=go errors.goRepository: dAppCore/go-tenant
Length of output: 502
🏁 Script executed:
# Check scope.go around line 95 for context
sed -n '90,100p' scope.goRepository: dAppCore/go-tenant
Length of output: 309
🏁 Script executed:
# Search for core.Is usage patterns to understand correct comparison
rg -n 'core\.Is' --type=go -A 1 -B 1Repository: dAppCore/go-tenant
Length of output: 3689
Doc comments mislead on error comparison and scope.go:95 will break after this change.
The doc comments on lines 12, 18, 24, 30, and 41 show direct equality comparison (err == tenant.ErrXxx), but errors created with core.E() require core.Is() for sentinel comparison. The test suite consistently uses core.Is() (tenant_test.go lines 67, 225, 232, 257, 260, 263, 584, 782, 789, 814, 924, 1173, 1189, 1207, 1245, 1248), confirming this is the correct pattern.
Additionally, scope.go:95 contains a direct equality comparison (if err == ErrWorkspaceNotFound {) that will silently fail to match after this change.
Fix the doc comments to show core.Is(err, tenant.ErrXxx) instead of err == tenant.ErrXxx, and update scope.go:95 to use core.Is() for the comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@errors.go` around lines 9 - 13, Update the doc comments for the sentinel
errors (e.g., ErrNoWorkspaceContext and the other ErrXxx variables defined via
core.E()) to show the correct comparison pattern using core.Is(err,
tenant.ErrXxx) instead of direct equality (err == tenant.ErrXxx), and modify the
equality check in scope.go where it currently does if err ==
ErrWorkspaceNotFound to use core.Is(err, ErrWorkspaceNotFound) so sentinel
comparisons work with errors created by core.E().
Comment on lines
+333
to
347
| if tenantService.cache != nil { | ||
| if err := tenantService.cache.InvalidateWorkspace(wsUUID); err != nil { | ||
| return | ||
| } | ||
| } | ||
| tenantService.lock.Lock() | ||
| if len(tenantService.alertState) > 0 { | ||
| for key := range tenantService.alertState { | ||
| if hasAlertPrefix(key, wsUUID) { | ||
| delete(tenantService.alertState, key) | ||
| } | ||
| } | ||
| } | ||
| tenantService.lock.Unlock() | ||
| } |
There was a problem hiding this comment.
Cache invalidation error silently skips alert state cleanup.
On line 335, if InvalidateWorkspace returns an error, the function returns early without cleaning up the corresponding alert state (lines 338-346). This could leave stale alert tracking data that affects subsequent threshold firing.
Consider logging the error and continuing with alert state cleanup, or propagating the error if full cleanup is required.
Suggested fix
if tenantService.cache != nil {
- if err := tenantService.cache.InvalidateWorkspace(wsUUID); err != nil {
- return
- }
+ _ = tenantService.cache.InvalidateWorkspace(wsUUID) // Log error if needed
}
tenantService.lock.Lock()📝 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
| if tenantService.cache != nil { | |
| if err := tenantService.cache.InvalidateWorkspace(wsUUID); err != nil { | |
| return | |
| } | |
| } | |
| tenantService.lock.Lock() | |
| if len(tenantService.alertState) > 0 { | |
| for key := range tenantService.alertState { | |
| if hasAlertPrefix(key, wsUUID) { | |
| delete(tenantService.alertState, key) | |
| } | |
| } | |
| } | |
| tenantService.lock.Unlock() | |
| } | |
| if tenantService.cache != nil { | |
| _ = tenantService.cache.InvalidateWorkspace(wsUUID) // Log error if needed | |
| } | |
| tenantService.lock.Lock() | |
| if len(tenantService.alertState) > 0 { | |
| for key := range tenantService.alertState { | |
| if hasAlertPrefix(key, wsUUID) { | |
| delete(tenantService.alertState, key) | |
| } | |
| } | |
| } | |
| tenantService.lock.Unlock() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tenant.go` around lines 333 - 347, The code returns early when
tenantService.cache.InvalidateWorkspace(wsUUID) errors, which skips cleaning
tenantService.alertState; change the error handling in the InvalidateWorkspace
call inside the function containing tenantService.cache.InvalidateWorkspace to
not return immediately—either log the error (using the existing logger) and
continue to the alertState cleanup or collect/propagate the error after cleanup;
ensure tenantService.lock, tenantService.alertState, and the
hasAlertPrefix(wsUUID) loop still run (acquire lock/unlock) even if
InvalidateWorkspace failed.
Comment on lines
+24
to
+27
| func newUsageRecord(workspaceID int64, featureCode string, quantity int, userID *int64, metadata map[string]any) UsageRecord { | ||
| if quantity <= 0 { | ||
| quantity = 1 | ||
| } |
There was a problem hiding this comment.
Silent coercion of non-positive quantity may mask caller bugs.
When quantity <= 0, the function silently coerces it to 1. This defensive behaviour could hide bugs in calling code that mistakenly passes zero or negative values. Consider logging a warning or returning an error for invalid input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usage.go` around lines 24 - 27, newUsageRecord currently silently coerces
non-positive quantity to 1 which can hide caller bugs; change newUsageRecord to
validate input and return an error instead of mutating the value: update the
signature to func newUsageRecord(workspaceID int64, featureCode string, quantity
int, userID *int64, metadata map[string]any) (UsageRecord, error), check if
quantity <= 0 and return a clear error (e.g., fmt.Errorf("invalid quantity: %d",
quantity)), and propagate the error to callers (update any call sites to handle
the error) so invalid inputs are surfaced rather than silently corrected.
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.
Brings this repo to verdict: COMPLIANT against the v0.9.0 audit.
🤖 Generated with Claude Code + Codex
Co-Authored-By: Codex noreply@openai.com
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests