fix: address audit Bundle B — error-handling discipline

[PaulDotterer]

Summary

Second bundle from the 2026-05-07 server tech-debt audit. Closes
F006, F007, F022, F023. F046 was already met (no deviant slog keys
in the codebase).

Builds on PR #141 (the gofmt sweep + lint CI). Targets the user's
explicit "always log errors and never ignore them" rule.

• F022 — `taskqueue.Client.Close` swallowed inspector close
  errors with a bare `_ = err`. Now logs via `slog.Warn`.

• F023 — `Store` had no logger handle, so `fireListeners`'
  panic recovery wrote directly to `os.Stderr`. Add
  `Store.SetLogger`; `cmd/control` and `cmd/indexer` plumb the
  boot logger. Logger snapshot is taken under the same RLock as
  the listener snapshot to avoid a race with a future
  `SetLogger` after AppendEvent traffic starts (CR catch).

• F006 — Five dispatch fan-out RPCs in `action_handler.go`
  silently dropped per-element errors. All sites now `slog.Warn`
  with rpc + source + group_id + device_id + action_id context.

• F007 — Handler enrichment lookups stopped silently dropping
  errors across `device_handler.go` (5 sites + inventory),
  `assignment_handler.go`, `user_selection_handler.go` (3
  sites), `internal_handler.go`, and `sso_handler.go` (3
  sites — pre-auth path keeps the existing info-leak guard;
  non-NotFound errors log without including raw user.ID per CR
  catch). Also fixed two name-uniqueness checks in
  `role_handler.go` + `user_group_handler.go` that didn't
  distinguish NotFound from DB error (silent "name available"
  on a flaky DB → concurrent duplicate-Create race).

  New helper `logEnrichmentErr` in `internal/api/helpers.go`
  consolidates the shape so future enrichment loops get the
  same `operation`/`<id_key>`/`error` slog shape for free.

• F046 (skipped) — Audit flagged inconsistent slog keys
  (`device_id`/`deviceID`, `error`/`err`). A grep across
  internal/ + cmd/ found zero deviant keys; snake_case +
  `error` are already the consistent convention.

Filed as separate issues (incidental CR catches)

• Auth: Logout + RenewCertificate are PublicProcedures with no rate-limit #142 — `Logout` + `RenewCertificate` are `PublicProcedures`
  with no rate-limit. Logout-flooding can invalidate sessions
  arbitrarily; RenewCertificate-flooding could exhaust the CA
  signer.
• errors_parity_test regex matches commented-out TS exports #143 — `errors_parity_test` regex matches commented-out TS
  exports. Test passes by luck today.
• (Comment on Auth interceptor: all ValidateToken errors classified as token-expired #139) — Authorization header token isn't trimmed;
  whitespace-only tokens hit `ValidateToken("")` and produce
  misleading JWT errors. Suggested fix folded into the existing
  JWT-classification issue scope.

Test plan

• Watch `Lint` (PR fix: address audit Bundle A — security + CI hardening #141 CI) green
• Watch `Tests` green
• Spot-check log output: trigger an enrichment failure (e.g.
  delete an action that's referenced from an LPS row, then
  hit `GetDeviceLpsPasswords`) — should see one
  `enrichment lookup failed` warn per failed lookup, no
  response degradation

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved error handling for name uniqueness checks in role and user group creation under database failures.
  • Enhanced authentication enforcement for user group evaluation operations.
  • Better error context logging when data enrichment lookups fail.

• Improvements

  • Added comprehensive error logging throughout API operations for improved diagnostics and troubleshooting.
  • Enhanced store logger configuration for structured logging.
  • Improved error reporting in task queue operations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 70e5ae9a-7d12-4dcb-bfdb-2a824358c3b8

📥 Commits

Reviewing files that changed from the base of the PR and between 937b5f2 and 39ab1e5.

📒 Files selected for processing (1)

• internal/api/action_handler.go

---

📝 Walkthrough

Walkthrough

This PR strengthens observability and error handling throughout the server by adding structured logging for transient failures and error conditions. The Store's listener panic recovery now logs via a configured slog.Logger, API handlers log enrichment failures through a new helper function, database uniqueness checks distinguish transient errors from logic errors, and dispatch/task operations log per-item failures instead of silently continuing.

Changes

Logging and Error Handling Improvements

Layer / File(s)	Summary
Store Logger Infrastructure internal/store/store.go	Store gains logger field and SetLogger() method. fireListeners snapshots logger under read lock and routes listener panic recovery through structured logging or falls back to stderr.
Enrichment Error Logging Helper internal/api/helpers.go	New logEnrichmentErr() function emits structured warn logs with operation, identifier key/value, and error for consistent enrichment-failure logging across handlers.
Application Startup Wiring cmd/control/main.go, cmd/indexer/main.go	Both applications explicitly wire configured logger into store via st.SetLogger(logger) after store initialization. Minor import and struct formatting updates in control and indexer main files.
Database Error Handling internal/api/role_handler.go, internal/api/user_group_handler.go	Uniqueness checks now distinguish pgx.ErrNoRows from other errors, returning Internal for transient failures instead of treating them as "name available." EvaluateDynamicUserGroup now requires authentication context.
Enrichment Logging internal/api/assignment_handler.go, internal/api/device_handler.go, internal/api/sso_handler.go, internal/api/internal_handler.go, internal/api/user_selection_handler.go	Compliance policies, device inventory, action/device metadata, and SSO provider lookups now log failures via logEnrichmentErr instead of silently skipping enrichment.
Action Dispatch Logging internal/api/action_handler.go	DispatchAction removes redundant signature loading. Fan-out dispatches (DispatchAssignedActions, DispatchActionSet, DispatchDefinition, DispatchToGroup) now log per-device/per-action failures. GetExecution and ListExecutions log action-name enrichment failures.
Task Queue Logging internal/taskqueue/client.go	Client.Close() now logs warnings when inspector close fails instead of silently suppressing errors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• manchtools/power-manage-server#83: Both PRs modify internal/store/store.go (Store, fireListeners, listener dispatch) and cmd/control/main.go wiring, making them directly related at the code level.
• manchtools/power-manage-server#130: Both PRs touch the same internal/api/device_handler.go functions (GetDeviceLpsPasswords/GetDeviceLuksKeys) and modify error/logging handling there.

Poem

A rabbit hops through logs so bright, 🐰
Where panics now have sight and light,
Each error whispers, enrichment calls,
Dispatch and task logs through the halls.
No more silent failures in the night! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.74% 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 title accurately describes the main focus of the PR: implementing error-handling improvements across multiple components to address audit findings related to error discipline.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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 fix/error-handling-discipline

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (6)

internal/config/config.go (1)

176-176: 💤 Low value

Consider clarifying the MTLSHost default documentation.

The comment states MTLSHost = GATEWAY_DOMAIN (not Traefik-prefixed — one name per thing), which could mislead operators into thinking GATEWAY_TRAEFIK_MTLS_HOST is not supported. However, the implementation at line 204 explicitly prioritizes GATEWAY_TRAEFIK_MTLS_HOST before falling back to GATEWAY_DOMAIN.

Consider updating the comment to reflect the fallback chain more accurately, e.g., MTLSHost = GATEWAY_TRAEFIK_MTLS_HOST → GATEWAY_DOMAIN.

🤖 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/config/config.go` at line 176, Update the comment for MTLSHost to
accurately describe the fallback chain used in the code: indicate that MTLSHost
prefers GATEWAY_TRAEFIK_MTLS_HOST and falls back to GATEWAY_DOMAIN (i.e.,
"MTLSHost = GATEWAY_TRAEFIK_MTLS_HOST → GATEWAY_DOMAIN"), so readers understand
the implemented priority logic referenced by MTLSHost, GATEWAY_TRAEFIK_MTLS_HOST
and GATEWAY_DOMAIN in config.go.

.github/workflows/lint.yml (1)

43-44: 💤 Low value

Consider pinning linter tool versions for reproducibility.

Using @latest for staticcheck and govulncheck could cause non-reproducible CI builds or unexpected failures when new versions are released. Consider pinning to specific versions (e.g., staticcheck@2024.1.1) and updating them deliberately.

That said, using @latest for linters is a common pattern to get the latest rules and fixes, so this is acceptable if the team prefers staying current.

Also applies to: 48-48

🤖 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 @.github/workflows/lint.yml around lines 43 - 44, Replace the use of `@latest`
in the go install lines with explicit, pinned versions to ensure reproducible CI
runs: update the "go install honnef.co/go/tools/cmd/staticcheck@latest"
invocation to use a specific release (e.g., staticcheck@2024.1.1) and likewise
pin the "go install golang.org/x/vuln/cmd/govulncheck@latest" invocation to a
specific govulncheck release; change those exact command strings in the workflow
to the chosen version tags and document/update them deliberately when you want
to upgrade.

internal/projectors/identity_provider.go (1)

55-55: ⚡ Quick win

Replace curly quotes with straight ASCII quotes in technical documentation.

The comments describing COALESCE/NULLIF semantics now use curly quotes (") instead of straight ASCII quotes (""). This creates the same searchability and copy-paste issues noted in role.go.

📝 Proposed fix to use straight quotes

-// `COALESCE(NULLIF(payload, "), existing)` (preserve on missing OR
+// `COALESCE(NULLIF(payload, ""), existing)` (preserve on missing OR

-// PL/pgSQL projector's `COALESCE(NULLIF(payload, "), existing)`.
+// PL/pgSQL projector's `COALESCE(NULLIF(payload, ""), existing)`.

Also applies to: 223-223

🤖 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/projectors/identity_provider.go` at line 55, The comment in
identity_provider.go that shows SQL semantics uses curly quotes around the empty
string (e.g., the `COALESCE(NULLIF(payload, ”), existing)` fragment); replace
the curly/typographic quotes with straight ASCII double quotes so the comment
reads `COALESCE(NULLIF(payload, ""), existing)` (also check the similar instance
at the other occurrence referenced and mirror the same change as done in
role.go) to restore correct copy/paste and searchability.

internal/projectors/role_test.go (1)

20-20: ⚡ Quick win

Replace curly quotes with straight ASCII quotes in test comments.

The test documentation now uses curly quotes when describing the empty-string default (") instead of straight quotes (""). This creates the same issues noted in the production code files.

📝 Proposed fix

-// The PL/pgSQL projector defaulted description to ", permissions to
+// The PL/pgSQL projector defaulted description to "", permissions to

-//   - name uses `COALESCE(NULLIF(payload, "), existing)` — empty
+//   - name uses `COALESCE(NULLIF(payload, ""), existing)` — empty

Also applies to: 90-90

🤖 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/projectors/role_test.go` at line 20, In the role_test.go test
comments replace curly/typographic quotes around the empty-string default with
straight ASCII double quotes: locate the comment text in
internal/projectors/role_test.go (and the similar occurrence around lines 90-90)
and change the curly opening/closing quotes to plain ASCII " characters so the
comment reads ...defaulted description to "", permissions to.... Ensure only the
comment text is edited and no other test logic is changed.

internal/projectors/role.go (1)

12-12: ⚡ Quick win

Replace curly quotes with straight ASCII quotes in technical documentation.

The comments now use curly/smart quotes (") instead of straight ASCII quotes (") when describing string literals and SQL syntax. In Go source code comments, especially when documenting string values, COALESCE semantics, or SQL literals, straight ASCII quotes are strongly preferred because:

• Searchability: Developers searching for literal "" won't find ""
• Copy-paste safety: Examples copied from comments with curly quotes could confuse readers
• Consistency: Go idioms and technical documentation conventionally use straight quotes

📝 Proposed fix to use straight quotes

-// for description ("), permissions ('{}'), and is_system (FALSE);
+// for description (""), permissions ('{}'), and is_system (FALSE);

-//     keep existing (PL/pgSQL `COALESCE(NULLIF(payload, "), existing)`).
+//     keep existing (PL/pgSQL `COALESCE(NULLIF(payload, ""), existing)`).

-// Defaults match the PL/pgSQL projector: missing description → ";
+// Defaults match the PL/pgSQL projector: missing description → "";

-// PL/pgSQL `NULLIF(name, ")` semantics — a UI that sends "" for
+// PL/pgSQL `NULLIF(name, "")` semantics — a UI that sends "" for

Also applies to: 29-29, 59-59, 109-109

🤖 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/projectors/role.go` at line 12, Replace all curly/smart quotes in
the comments of internal/projectors/role.go with straight ASCII quotes: change
occurrences like the comment fragment for description (”), permissions ('{}'),
and is_system (FALSE) to use "description", "'{}'", and "is_system" with
straight double/single quotes; ensure the same replacement is applied at the
other mentioned comment locations referencing description, permissions, and
is_system so examples and SQL literals are copy-paste safe and searchable.

internal/api/helpers.go (1)

104-109: ⚡ Quick win

logEnrichmentErr uses the global slog rather than a caller-provided *slog.Logger, dropping per-handler context.

Every other helper in this file (appendEvent, enqueueSearchReindex) accepts a *slog.Logger. logEnrichmentErr is the sole exception, which means:

• The "component" attribute injected via logger.With("component", "user_selection_handler") (and equivalent in each other handler) is absent from every warning this emits.
• The request ID (extractable via middleware.RequestIDFromContext(ctx)) is unavailable.
• In tests that don't call slog.SetDefault, output goes to the default text handler.

♻️ Proposed fix — accept a *slog.Logger parameter

-func logEnrichmentErr(operation, idKey, idValue string, err error) {
-	slog.Warn("enrichment lookup failed",
+func logEnrichmentErr(logger *slog.Logger, operation, idKey, idValue string, err error) {
+	logger.Warn("enrichment lookup failed",
 		"operation", operation,
 		idKey, idValue,
 		"error", err)
 }

All call sites then become e.g.:

logEnrichmentErr(h.logger, "GetActionByID", "action_id", asn.SourceID, 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/api/helpers.go` around lines 104 - 109, The helper logEnrichmentErr
currently uses the global slog instead of a caller-provided *slog.Logger; change
its signature to accept a *slog.Logger (e.g., logEnrichmentErr(logger
*slog.Logger, operation, idKey, idValue string, err error)) and replace the
global slog usage inside the function with the passed logger so per-handler
context (component, request ID) is preserved; update all call sites (where
appendEvent/enqueueSearchReindex/handlers call logEnrichmentErr) to pass the
handler logger (e.g., h.logger) as the first argument.

🤖 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/action_handler.go`:
- Around line 867-874: The pre-assignments to signature and paramsCanonical are
ineffectual because they are always overwritten later by the re-sign logic (the
later assignments to signature and paramsCanonical around the re-sign call),
which triggers the ineffassign lint; remove the initial assignments to signature
and paramsCanonical (or conditionally set them only when they won't be
overwritten) in the block that currently contains the //lint:ignore comments so
the variables are only assigned where they are actually used.

---

Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 43-44: Replace the use of `@latest` in the go install lines with
explicit, pinned versions to ensure reproducible CI runs: update the "go install
honnef.co/go/tools/cmd/staticcheck@latest" invocation to use a specific release
(e.g., staticcheck@2024.1.1) and likewise pin the "go install
golang.org/x/vuln/cmd/govulncheck@latest" invocation to a specific govulncheck
release; change those exact command strings in the workflow to the chosen
version tags and document/update them deliberately when you want to upgrade.

In `@internal/api/helpers.go`:
- Around line 104-109: The helper logEnrichmentErr currently uses the global
slog instead of a caller-provided *slog.Logger; change its signature to accept a
*slog.Logger (e.g., logEnrichmentErr(logger *slog.Logger, operation, idKey,
idValue string, err error)) and replace the global slog usage inside the
function with the passed logger so per-handler context (component, request ID)
is preserved; update all call sites (where
appendEvent/enqueueSearchReindex/handlers call logEnrichmentErr) to pass the
handler logger (e.g., h.logger) as the first argument.

In `@internal/config/config.go`:
- Line 176: Update the comment for MTLSHost to accurately describe the fallback
chain used in the code: indicate that MTLSHost prefers GATEWAY_TRAEFIK_MTLS_HOST
and falls back to GATEWAY_DOMAIN (i.e., "MTLSHost = GATEWAY_TRAEFIK_MTLS_HOST →
GATEWAY_DOMAIN"), so readers understand the implemented priority logic
referenced by MTLSHost, GATEWAY_TRAEFIK_MTLS_HOST and GATEWAY_DOMAIN in
config.go.

In `@internal/projectors/identity_provider.go`:
- Line 55: The comment in identity_provider.go that shows SQL semantics uses
curly quotes around the empty string (e.g., the `COALESCE(NULLIF(payload, ”),
existing)` fragment); replace the curly/typographic quotes with straight ASCII
double quotes so the comment reads `COALESCE(NULLIF(payload, ""), existing)`
(also check the similar instance at the other occurrence referenced and mirror
the same change as done in role.go) to restore correct copy/paste and
searchability.

In `@internal/projectors/role_test.go`:
- Line 20: In the role_test.go test comments replace curly/typographic quotes
around the empty-string default with straight ASCII double quotes: locate the
comment text in internal/projectors/role_test.go (and the similar occurrence
around lines 90-90) and change the curly opening/closing quotes to plain ASCII "
characters so the comment reads ...defaulted description to "", permissions
to.... Ensure only the comment text is edited and no other test logic is
changed.

In `@internal/projectors/role.go`:
- Line 12: Replace all curly/smart quotes in the comments of
internal/projectors/role.go with straight ASCII quotes: change occurrences like
the comment fragment for description (”), permissions ('{}'), and is_system
(FALSE) to use "description", "'{}'", and "is_system" with straight
double/single quotes; ensure the same replacement is applied at the other
mentioned comment locations referencing description, permissions, and is_system
so examples and SQL literals are copy-paste safe and searchable.

🪄 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: c76f87ce-f5dd-4c64-baff-0d6836a2989f

📥 Commits

Reviewing files that changed from the base of the PR and between 44ef974 and 653bf19.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (69)

• .github/workflows/lint.yml
• cmd/control/main.go
• cmd/indexer/main.go
• go.mod
• internal/api/action_handler.go
• internal/api/action_set_handler.go
• internal/api/action_set_handler_test.go
• internal/api/assignment_handler.go
• internal/api/audit_handler.go
• internal/api/compliance_policy_handler.go
• internal/api/definition_handler.go
• internal/api/device_group_handler.go
• internal/api/device_handler.go
• internal/api/errors.go
• internal/api/errors_parity_test.go
• internal/api/helpers.go
• internal/api/internal_handler.go
• internal/api/role_handler.go
• internal/api/search_handler.go
• internal/api/search_listener_test.go
• internal/api/service.go
• internal/api/sso_handler.go
• internal/api/system_actions_listener_test.go
• internal/api/terminal_handler.go
• internal/api/terminal_handler_test.go
• internal/api/user_group_handler.go
• internal/api/user_handler.go
• internal/api/user_selection_handler.go
• internal/api/validator_test.go
• internal/auth/interceptor.go
• internal/auth/interceptor_test.go
• internal/auth/jwt.go
• internal/auth/reconcile_test.go
• internal/config/config.go
• internal/connection/manager.go
• internal/gateway/registry/registry.go
• internal/gateway/task_handlers.go
• internal/gateway/task_handlers_test.go
• internal/handler/terminal_bridge.go
• internal/idp/linker.go
• internal/idp/linker_test.go
• internal/idp/oidc.go
• internal/middleware/middleware_test.go
• internal/mtls/mtls.go
• internal/mtls/peer_class_test.go
• internal/projectors/identity_provider.go
• internal/projectors/identity_provider_test.go
• internal/projectors/luks_key_listener.go
• internal/projectors/luks_key_test.go
• internal/projectors/role.go
• internal/projectors/role_test.go
• internal/projectors/scim_group_mapping.go
• internal/projectors/scim_group_mapping_test.go
• internal/projectors/security_alert_test.go
• internal/projectors/token.go
• internal/projectors/token_test.go
• internal/projectors/user_role_test.go
• internal/projectors/user_selection_listener.go
• internal/resolution/resolve_tree_test.go
• internal/scim/discovery.go
• internal/scim/groups.go
• internal/scim/handler.go
• internal/scim/scim.go
• internal/search/index.go
• internal/store/store.go
• internal/taskqueue/client.go
• internal/taskqueue/payloads.go
• internal/taskqueue/types_test.go
• internal/terminal/store_test.go

💤 Files with no reviewable changes (10)

• internal/api/definition_handler.go
• internal/gateway/task_handlers_test.go
• internal/projectors/user_selection_listener.go
• internal/api/compliance_policy_handler.go
• internal/auth/reconcile_test.go
• internal/gateway/task_handlers.go
• internal/mtls/mtls.go
• internal/api/action_set_handler.go
• internal/resolution/resolve_tree_test.go
• internal/api/device_group_handler.go

[PaulDotterer]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.