refactor(store): wave B.13 — device repo (#269)#270
Merged
Conversation
|
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 (15)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughAdds a Device read-side abstraction and Postgres implementation, wires it into Repos, and migrates handlers and workers to use store.Repos().Device for device existence, listing, hostname enrichment, deletion checks, and sync-interval reads. ChangesDevice Repository Abstraction & Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant DeviceHandler
participant DeviceRepo
participant ProtoConv
Client->>DeviceHandler: ListDevicesRequest(filter)
DeviceHandler->>DeviceRepo: List(filter) / Count(filter)
DeviceRepo-->>DeviceHandler: []store.Device, int64
DeviceHandler->>ProtoConv: deviceToProtoCtx(store.Device)
ProtoConv-->>DeviceHandler: protobuf device
DeviceHandler-->>Client: ListDevicesResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/user_selection_handler.go (1)
44-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not collapse all repo errors into
device not foundin access checks.These branches should only return
CodeNotFoundforstore.IsNotFound(err); internal lookup failures should returnCodeInternal.Suggested fix (both call sites)
_, err = h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ ID: req.Msg.DeviceId, OwnerScope: userFilterID(ctx, "ListDevices"), }) if err != nil { - return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found") + if store.IsNotFound(err) { + return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found") + } + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to look up device") }As per coding guidelines "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
Also applies to: 113-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/user_selection_handler.go` around lines 44 - 50, The current access-check error handling collapses all Get errors into a "device not found" NotFound response; change both call sites that call h.store.Repos().Device.Get (the one using userFilterID in the access check and the later lookup) to inspect the error with store.IsNotFound(err) and only return apiErrorCtx(..., ErrDeviceNotFound, connect.CodeNotFound, ...) when IsNotFound is true; for any other non-nil err return apiErrorCtx(ctx, err, connect.CodeInternal, "failed to get device") (preserve the original err), keeping references to h.store.Repos().Device.Get, userFilterID, apiErrorCtx, and ErrDeviceNotFound to locate the sites.internal/api/internal_handler.go (1)
68-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferentiate not-found from repository failures.
Line 68 and Line 85 currently map every
Device.Geterror todevice not found or deleted. This collapses transient/store failures into false 404s and hides internal faults.Suggested fix
- _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: deviceID}) - if err != nil { - h.logger.Warn("device verification failed", "device_id", deviceID, "error", err) - return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found or deleted") - } + _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: deviceID}) + if err != nil { + if store.IsNotFound(err) { + h.logger.Warn("device verification failed", "device_id", deviceID, "error", err) + return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found or deleted") + } + h.logger.Error("device verification lookup failed", "device_id", deviceID, "error", err) + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to verify device") + }- if _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: deviceID}); err != nil { - h.logger.Warn("sync actions for unknown/deleted device", "device_id", deviceID) - return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found or deleted") - } + if _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: deviceID}); err != nil { + if store.IsNotFound(err) { + h.logger.Warn("sync actions for unknown/deleted device", "device_id", deviceID) + return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found or deleted") + } + h.logger.Error("sync actions device lookup failed", "device_id", deviceID, "error", err) + return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to verify device") + }As per coding guidelines: "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
Also applies to: 85-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/internal_handler.go` around lines 68 - 72, The current handling of h.store.Repos().Device.Get treats all errors as "device not found or deleted"; change it to distinguish not-found vs other repo failures by checking the returned error (e.g., compare to the repository's NotFound sentinel or use errors.Is). If the error indicates not found, return apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found or deleted"); for any other error, log it with h.logger.Error including the error and deviceID and return apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "internal error") (or the appropriate internal error constant). Apply the same fix to the other Device.Get call at the later block (lines ~85-88) so transient/store failures are not mapped to 404s.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/logs_handler.go`:
- Around line 38-41: The current device lookup in h.store.Repos().Device.Get
maps any error to ErrDeviceNotFound/CodeNotFound; change it to distinguish
not-found vs internal errors by using errors.Is (or equivalent) against the
repository's sentinel not-found error (e.g., store.ErrNotFound) after calling
h.store.Repos().Device.Get, returning apiErrorCtx(ctx, ErrDeviceNotFound,
connect.CodeNotFound, "device not found") only for not-found and returning
apiErrorCtx(ctx, err, connect.CodeInternal, "internal error retrieving device")
for all other errors so internal DB/repo failures yield connect.CodeInternal and
preserve the original error.
In `@internal/api/osquery_handler.go`:
- Around line 51-54: The repo Get call's error is always mapped to CodeNotFound;
change both existence checks (the call to h.store.Repos().Device.Get and the
similar check later) to distinguish a repository "not found" sentinel from other
errors: use errors.Is(err, store.ErrNotFound) (or the repo package's equivalent)
to return apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not
found"), and for any other non-nil err return apiErrorCtx(ctx, err,
connect.CodeInternal, "failed to fetch device") (i.e., map repository/internal
failures to connect.CodeInternal rather than CodeNotFound).
---
Outside diff comments:
In `@internal/api/internal_handler.go`:
- Around line 68-72: The current handling of h.store.Repos().Device.Get treats
all errors as "device not found or deleted"; change it to distinguish not-found
vs other repo failures by checking the returned error (e.g., compare to the
repository's NotFound sentinel or use errors.Is). If the error indicates not
found, return apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device
not found or deleted"); for any other error, log it with h.logger.Error
including the error and deviceID and return apiErrorCtx(ctx, ErrInternal,
connect.CodeInternal, "internal error") (or the appropriate internal error
constant). Apply the same fix to the other Device.Get call at the later block
(lines ~85-88) so transient/store failures are not mapped to 404s.
In `@internal/api/user_selection_handler.go`:
- Around line 44-50: The current access-check error handling collapses all Get
errors into a "device not found" NotFound response; change both call sites that
call h.store.Repos().Device.Get (the one using userFilterID in the access check
and the later lookup) to inspect the error with store.IsNotFound(err) and only
return apiErrorCtx(..., ErrDeviceNotFound, connect.CodeNotFound, ...) when
IsNotFound is true; for any other non-nil err return apiErrorCtx(ctx, err,
connect.CodeInternal, "failed to get device") (preserve the original err),
keeping references to h.store.Repos().Device.Get, userFilterID, apiErrorCtx, and
ErrDeviceNotFound to locate the sites.
🪄 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: e5dc66f4-84db-41cf-93b4-01a73793aa6a
📒 Files selected for processing (15)
internal/api/action_dispatch.gointernal/api/assignment_handler.gointernal/api/certificate_handler.gointernal/api/device_handler.gointernal/api/internal_handler.gointernal/api/logs_handler.gointernal/api/osquery_handler.gointernal/api/terminal_handler.gointernal/api/user_selection_handler.gointernal/control/inbox_worker.gointernal/search/index.gointernal/store/device.gointernal/store/postgres/device.gointernal/store/postgres/wire.gointernal/store/repos.go
Comment on lines
+38
to
41
| _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId}) | ||
| if err != nil { | ||
| return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found") | ||
| } |
There was a problem hiding this comment.
Preserve internal error semantics for device lookup.
This maps all repo errors to CodeNotFound. Internal DB/repo failures should return CodeInternal, otherwise outages are misreported as missing devices.
Suggested fix
- _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId})
- if err != nil {
- return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found")
- }
+ _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId})
+ if err != nil {
+ if store.IsNotFound(err) {
+ return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found")
+ }
+ return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to look up device")
+ }As per coding guidelines "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
📝 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
| _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId}) | |
| if err != nil { | |
| return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found") | |
| } | |
| _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId}) | |
| if err != nil { | |
| if store.IsNotFound(err) { | |
| return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found") | |
| } | |
| return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to look up device") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/logs_handler.go` around lines 38 - 41, The current device lookup
in h.store.Repos().Device.Get maps any error to ErrDeviceNotFound/CodeNotFound;
change it to distinguish not-found vs internal errors by using errors.Is (or
equivalent) against the repository's sentinel not-found error (e.g.,
store.ErrNotFound) after calling h.store.Repos().Device.Get, returning
apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found")
only for not-found and returning apiErrorCtx(ctx, err, connect.CodeInternal,
"internal error retrieving device") for all other errors so internal DB/repo
failures yield connect.CodeInternal and preserve the original error.
Comment on lines
+51
to
54
| _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId}) | ||
| if err != nil { | ||
| return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found") | ||
| } |
There was a problem hiding this comment.
Differentiate not-found from repo/internal failures in both existence checks.
Both checks currently return CodeNotFound for any repository error. This hides backend failures and returns incorrect semantics to clients.
Suggested fix (apply in both methods)
- _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId})
- if err != nil {
- return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found")
- }
+ _, err := h.store.Repos().Device.Get(ctx, store.GetDeviceKey{ID: msg.DeviceId})
+ if err != nil {
+ if store.IsNotFound(err) {
+ return nil, apiErrorCtx(ctx, ErrDeviceNotFound, connect.CodeNotFound, "device not found")
+ }
+ return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to look up device")
+ }As per coding guidelines "Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."
Also applies to: 209-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/osquery_handler.go` around lines 51 - 54, The repo Get call's
error is always mapped to CodeNotFound; change both existence checks (the call
to h.store.Repos().Device.Get and the similar check later) to distinguish a
repository "not found" sentinel from other errors: use errors.Is(err,
store.ErrNotFound) (or the repo package's equivalent) to return apiErrorCtx(ctx,
ErrDeviceNotFound, connect.CodeNotFound, "device not found"), and for any other
non-nil err return apiErrorCtx(ctx, err, connect.CodeInternal, "failed to fetch
device") (i.e., map repository/internal failures to connect.CodeInternal rather
than CodeNotFound).
DeviceRepo — read side of devices_projection:
Get / IsDeleted / List / ListOnline / ListOffline /
Count / CountOnline / CountOffline /
HostnamesByIDs / SyncInterval
The Get and List methods carry OwnerScope to model the :self-scoped
permission shape that previously used FilterUserID directly; nil =
admin view, non-nil = restrict to devices the user has assignment
access to (the underlying query returns ErrNotFound when the scope
excludes the row — handlers can't distinguish, by design).
Labels stays as json.RawMessage at the boundary per the JSONB
normalize plan. Compliance status (Status / CheckedAt / Total /
Passing) is denormalized on the device row by the compliance
projector and surfaced as-is.
Call sites migrated:
- internal/api/device_handler.go (large sweep, 3-way Online/Offline
switch with matching count branches, deviceToProtoCtx +
deviceToProtoWithAssignments signatures shifted to store.Device)
- internal/api/user_selection_handler.go (2 :self-scoped Get sites)
- internal/search/index.go (2 sites: ListDevices + GetDeviceByID
for enrichment)
- internal/control/inbox_worker.go (IsDeviceDeleted, 2 sites)
- internal/api/{logs,osquery,terminal,assignment,compliance,
certificate}_handler.go — GetDeviceByID for verification
Non-goals:
- Per-device cross-domain queries (assignments, executions,
maintenance windows, compliance evaluations) migrate with their
owning domains.
- CountMatchingDevicesForQuery — PL/pgSQL evaluator, Wave C.
- Write-side projector queries — stay on Queries().
Closes #269.
PaulDotterer
force-pushed
the
feat/269-device-repo
branch
from
5e8ddb0 to
6be4d86
Compare
This was referenced May 15, 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
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
`DeviceRepo` — read side of devices_projection. Branches off main directly.
Methods
`Labels` stays as `json.RawMessage` at the boundary. Compliance status fields denormalized on the row.
Call sites migrated
Non-goals
Acceptance
Part of #242. Closes #269.
Summary by CodeRabbit