refactor(store): wave A — ErrNotFound sentinel + scrub pgx.ErrNoRows (#243)

[PaulDotterer]

Summary

First wave of the storage-abstraction tracker (#242). Stops handler/projector/scim/idp/control packages from reaching for pgx.ErrNoRows directly; centralizes not-found recognition in store.IsNotFound(err).

Why this matters: when a second backend (libSQL/Turso as the illustrative example, NoSQL as the architectural ceiling) lands later, its no-rows error gets registered inside store.IsNotFound — not in every caller.

Changes

• New server/internal/store/notfound.go — defines store.ErrNotFound sentinel + store.IsNotFound(err) bool recognizer that today knows both pgx.ErrNoRows and store.ErrNotFound.
• Updated internal/api/helpers.go::handleGetError to use store.IsNotFound. Drops pgx import.
• Swept 33 call sites: errors.Is(err, pgx.ErrNoRows) → store.IsNotFound(err) across:
  • internal/api/ — 18 files
  • internal/scim/ — 8 files
  • internal/idp/ — 2 files (incl. test mock returning store.ErrNotFound instead of pgx.ErrNoRows)
  • internal/projectors/ — 4 files (incl. comment cleanup)
  • internal/control/inbox_worker.go
• Removed now-unused pgx imports (33 files; goimports clean).
• Bridge test TestHandleGetError_NotFound now exercises both pgx_native and store_abstr sentinels through handleGetError to lock the recognizer contract.

Non-goals

• No interface refactor (Wave B).
• No backend swap.
• No proto / schema / event changes.
• No user-visible behaviour change.

Acceptance

• pgx.ErrNoRows outside internal/store/ + generated/ is now confined to:
  • 1 line in helpers_test.go (the bridge test fixture — intentional).
  • 0 production call sites.
• go vet ./..., gofmt -l server/, go build ./server/...: clean.
• Local CodeRabbit review: no findings.
• Targeted tests pass: TestHandleGetError_NotFound/{pgx_native,store_abstr}, sample handler integration suite, all internal/projectors, internal/scim, internal/idp, internal/control, internal/store.
• Full internal/api suite hangs locally on TestProxyStoreLuksKey_MissingFields testcontainer setup (~30 min) — pre-existing environmental issue on this host, NOT a regression from this PR. Pushing to let CI verify with proper Docker.

Test plan

• CI green on all checks
• CR full review confirms local-CR plain finding (none)
• No behaviour regressions in handler not-found mapping (covered by existing handler tests)

Part of #242. Closes #243.

Summary by CodeRabbit

• Refactor
  • Improved internal database error handling abstraction to enhance code maintainability and reduce direct driver-level dependencies across the system.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR centralizes "not found" error detection across the codebase by introducing a store-level abstraction (store.ErrNotFound and store.IsNotFound()) and removing direct PostgreSQL driver imports from 34 handler, SCIM, IDP, projector, and control files.

Changes

Centralize "not found" error detection behind store abstraction

Layer / File(s)	Summary
Store-level not-found abstraction definition internal/store/notfound.go	Introduces store.ErrNotFound sentinel and store.IsNotFound(err) helper that recognizes both the new sentinel and pgx.ErrNoRows for backward compatibility, enabling future backend abstraction without changing all callers.
API helpers and handler error classification internal/api/helpers.go, internal/api/helpers_test.go, internal/api/action_crud.go, internal/api/action_dispatch.go, internal/api/assignment_handler.go, internal/api/auth_handler.go, internal/api/certificate_handler.go, internal/api/device_group_handler.go, internal/api/device_handler.go, internal/api/idp_handler.go, internal/api/registration_handler.go, internal/api/role_handler.go, internal/api/search_listener.go, internal/api/sso_handler.go, internal/api/terminal_handler.go, internal/api/totp_handler.go, internal/api/user_group_handler.go, internal/api/user_handler.go	Updates 19 handler files and the common error-mapping helper to use store.IsNotFound() for resource-not-found detection during CRUD operations, validation, and lookups; removes pgx and errors imports from each.
SCIM and IDP handler integration internal/scim/auth.go, internal/scim/groups.go, internal/scim/groups_create.go, internal/scim/groups_helpers.go, internal/scim/groups_mutate.go, internal/scim/users.go, internal/scim/users_create.go, internal/scim/users_mutate.go, internal/idp/linker.go, internal/idp/linker_test.go	Updates SCIM user/group handlers and IDP identity linker to detect not-found errors via store.IsNotFound() during user/group/provider lookups and link verification; removes pgx imports and aligns test mocks to use store.ErrNotFound.
Control worker and projector cascade guards internal/control/inbox_worker.go, internal/projectors/assignment_listener.go, internal/projectors/device_group_listener.go, internal/projectors/user_group_listener.go	Updates control inbox worker and 3 event projector files to use store.IsNotFound() for stale-replay detection and action/execution/user-group existence checks that gate event cascade logic; removes pgx imports.
Test mock and assertion updates internal/api/search_listener_e2e_test.go, internal/projectors/assignment_test.go, internal/idp/linker_test.go	Aligns test files to return store.ErrNotFound from mock database methods and updates assertion/comment text to reference the new not-found abstraction instead of pgx.ErrNoRows.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• manchtools/power-manage-server#29: Overlaps on internal/api/helpers.go refactoring of handleGetError and error-mapping logic to centralize "not found" classification.
• manchtools/power-manage-server#76: Both modify internal/control/inbox_worker.go stream-id fallback logic; main PR switches to store.IsNotFound(err) while other uses pgx error check for same branch.

Poem

🐰 From pgx chains we hop away,
A store abstraction sees the day,
No rows? One sentine! Clean and bright,
Future backends? Now in sight! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly summarizes the main change: introducing a store.ErrNotFound sentinel and removing pgx.ErrNoRows references from non-store packages, which aligns with the changeset.
Linked Issues check	✅ Passed	All objectives from issue #243 are met: store.ErrNotFound sentinel added, store.IsNotFound() helper implemented, internal/api/helpers.go updated, pgx.ErrNoRows replaced across 33 call sites in specified packages, and unused pgx imports removed.
Out of Scope Changes check	✅ Passed	All changes are within scope—no interface refactors, backend swaps, or proto/schema changes. The scope is limited to adding the store abstraction layer and sweeping error-detection call sites as planned.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/243-store-errnotfound

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/idp/linker.go`:
- Around line 142-143: The current error branch returns whenever
store.IsNotFound(err) is false, but since err can be nil after a successful link
lookup this causes an early return and blocks the stale-link fallback; update
the conditional around the lookup error (the check using store.IsNotFound and
the error wrap that returns "lookup identity link: %w") to only return on a real
error (i.e. require err != nil && !store.IsNotFound(err)), and ensure the
subsequent logic that handles a found link with a missing user continues into
the auto-link/auto-create fallback paths (inspect the lookup code and follow-up
branches around the identity link lookup to confirm flow).

🪄 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: 6377cbb0-e6b3-429a-8c74-a23f8ca4fb93

📥 Commits

Reviewing files that changed from the base of the PR and between 8d12458 and ef67463.

📒 Files selected for processing (35)

• internal/api/action_crud.go
• internal/api/action_dispatch.go
• internal/api/assignment_handler.go
• internal/api/auth_handler.go
• internal/api/certificate_handler.go
• internal/api/device_group_handler.go
• internal/api/device_handler.go
• internal/api/helpers.go
• internal/api/helpers_test.go
• internal/api/idp_handler.go
• internal/api/registration_handler.go
• internal/api/role_handler.go
• internal/api/search_listener.go
• internal/api/search_listener_e2e_test.go
• internal/api/sso_handler.go
• internal/api/terminal_handler.go
• internal/api/totp_handler.go
• internal/api/user_group_handler.go
• internal/api/user_handler.go
• internal/control/inbox_worker.go
• internal/idp/linker.go
• internal/idp/linker_test.go
• internal/projectors/assignment_listener.go
• internal/projectors/assignment_test.go
• internal/projectors/device_group_listener.go
• internal/projectors/user_group_listener.go
• internal/scim/auth.go
• internal/scim/groups.go
• internal/scim/groups_create.go
• internal/scim/groups_helpers.go
• internal/scim/groups_mutate.go
• internal/scim/users.go
• internal/scim/users_create.go
• internal/scim/users_mutate.go
• internal/store/notfound.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stale-link fallback exits before Step 2/3.

When the initial link lookup succeeds and the linked user is missing, err is still nil here, so this branch returns early and prevents the intended fallback flow (auto-link/auto-create).

Suggested fix

-	if !store.IsNotFound(err) {
+	if err != nil && !store.IsNotFound(err) {
 		return nil, fmt.Errorf("lookup identity link: %w", err)
 	}

📝 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 !store.IsNotFound(err) {
	return nil, fmt.Errorf("lookup identity link: %w", err)
	if err != nil && !store.IsNotFound(err) {
	return nil, fmt.Errorf("lookup identity link: %w", err)
	}

🤖 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/idp/linker.go` around lines 142 - 143, The current error branch
returns whenever store.IsNotFound(err) is false, but since err can be nil after
a successful link lookup this causes an early return and blocks the stale-link
fallback; update the conditional around the lookup error (the check using
store.IsNotFound and the error wrap that returns "lookup identity link: %w") to
only return on a real error (i.e. require err != nil && !store.IsNotFound(err)),
and ensure the subsequent logic that handles a found link with a missing user
continues into the auto-link/auto-create fallback paths (inspect the lookup code
and follow-up branches around the identity link lookup to confirm flow).