refactor: 2026.06 closing tech-debt audit cleanup (PR G)

[PaulDotterer]

Summary

Closing tech-debt audit (PR G) for the 2026.06 cleanup release. Implements every actionable finding from server/TECH_DEBT_AUDIT.md (80 items) plus the SDK pin bump that picks up the matching SDK PR (manchtools/power-manage-sdk#57).

Eleven thematic commits, organised so each can be reviewed independently:

Theme	Commit
build-fix (SDK enum migration)	7fb3b63 fix: close handler compile breaks for SDK enum migrations
LUKS projector guard	44a1944 fix: mirror LPS stale-replay guard to LUKS keys projector (N007)
dead PL/pgSQL + Containerfiles	7733cc7 refactor: drop dead PL/pgSQL rebuild dispatcher + obsolete Containerfiles (N001 + N027)
Audit redactor	a81b93a feat: schema-aware per-action-type audit-log redactor (N002)
Typed payload sweep	51ff16a feat: typed event payloads + dual-write decommission across remaining streams (N003-N005, N015, N017, N021)
Cleanup cluster	ac27af0 fix: deps + dead writes + CI/docs cluster (N008-N013, N019-N020, N030)
God file split	16ee6f7 refactor: split action_handler.go god file into crud / dispatch / params / schedule / validators (F005)
Config unification	7534a66 refactor: promote env-var + clamp helpers to internal/config (F017 + N022 + N029)
Test coverage	96f1287 test: util / helpers / errors unit coverage in internal/api (F034 partial)
Local CR fixes	8c88141 fix: address local CR review (error code + case-insensitive HTTPS)
SDK pin	fe6a04c chore(sdk): repin SDK to merged PR #57 commit (3bfd7be)

Cross-repo dependency

Pairs with manchtools/power-manage-sdk#57 (already merged). The chore(sdk) commit above swaps the replace target from the previous pseudo-version to the merged SDK SHA. go mod tidy auto-bumped the go directive from 1.25.0 to 1.25.10 to match the SDK's CVE-fix toolchain requirement (GO-2026-4986 / 4977 / 4971 / 4918).

The matching agent PR will land next in the same sequence.

Notes for review

• Audit-log redactor (N002): schema-aware per action type (~6 schemas). Rejects the recursive walk + depth-limit option since over-redacting unrelated nested fields like params.environment.PATH would break operator triage. See internal/api/audit_redactor_internal_test.go for the per-action-type fixtures.
• God file split (F005): internal/api/action_handler.go was 1947 LOC; split into crud.go, dispatch.go, params.go, schedule.go, validators.go in the same package. Public API unchanged.
• Test coverage (F034 partial): covers util / helpers / errors. The remaining files (compliance / system_actions handler suites) need testcontainer wiring and are tracked as F014/F015/F044/F045 — flagged as scope for a follow-up commit IF reviewers want them in this PR window. Per feedback_no_deferrals_in_release_window.md, no out-of-window deferral.
• Cleanup cluster intentionally bundles ~9 small N-numbered findings to keep the diff legible.

Test plan

• go build ./server/... — verified clean locally
• go vet ./server/... — verified clean locally
• go test -short ./server/... — verified locally
• CI green

Summary by CodeRabbit

• New Features

  • Full action CRUD and parameter management; dispatching actions (single/multiple devices, groups, definitions), instant built-in actions, execution read/list/cancel with pagination.

• Bug Fixes

  • Schema-aware audit redaction per event type.
  • LUKS key revocation hardened with projection-version stale-replay protection.

• Improvements

  • Typed event payloads across many subsystems, safer validation/error handling, env-based config overrides, dependency/toolchain bumps.

• Documentation

  • Updated README: actions, indexer env vars, and testing guidance.

• Tests

  • Added unit and wire-contract coverage for payloads and redaction.

[coderabbitai]

Warning

Rate limit exceeded

@PaulDotterer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 51 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dedfdf56-d550-460e-a66d-ee8fe358cb7f

📥 Commits

Reviewing files that changed from the base of the PR and between 863666f and 5c986d4.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (1)

• go.mod

📝 Walkthrough

I'm sorry — I can't reliably generate the required hidden review artifact because it must include every provided rangeId exactly once and the input contains a very large list of rangeIds. Producing that huge, exact, and validated mapping here risks mistakes. If you still want this, please let me:

• accept that I'll produce the full review stack in multiple incremental steps (I will output and you can validate after each step), or
• provide a smaller subset of ranges to focus on (e.g., specific directories or files), or
• permit me to generate a single-cohort artifact that groups ranges by high-level areas and I'll produce the complete mapping in a separate file/attachment.

Which option do you prefer?

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/cleanup-release-pr-g

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

internal/store/rebuild.go (2)

226-230: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale RebuildAll documentation (still references removed PL/pgSQL path).

The comment now contradicts implementation and mentions a non-existent per-target Function field, which can mislead incident operators.

Suggested doc fix

-// The replay invokes the existing project_<stream>_event() PL/pgSQL
-// projector for each event matching the target's stream types. As
-// projectors are ported to Go (`#96`–#106), the per-target Function
-// field gets swapped to a Go dispatcher and this same RebuildAll
-// keeps working without operator-facing changes.
+// The replay dispatches each matching event through the target's
+// registered Go applier (wired via projectors.WireAll ->
+// Store.RegisterRebuildApply). Targets with missing appliers fail
+// fast before any TRUNCATE executes.

🤖 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/store/rebuild.go` around lines 226 - 230, The comment above
RebuildAll is stale: remove references to the removed PL/pgSQL projector path
and the non-existent per-target "Function" field and replace with a concise
description of current behavior — e.g., state that RebuildAll iterates matching
events and invokes the active Go projector dispatcher (or registered Go
projectors) so no operator-facing changes are required; update the comment near
the RebuildAll function to mention the porting to Go and the dispatcher-based
invocation model instead of referencing project_<stream>_event() or a Function
field.

---

299-332: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Avoid unbounded in-memory buffering of all replay events.

dispatchViaGoApplier currently accumulates the full target event set before apply. On large stores this can cause high memory pressure or OOM during emergency rebuilds.

Bound memory with paged replay

 func (s *Store) dispatchViaGoApplier(ctx context.Context, tx pgx.Tx, t rebuildTarget, apply RebuildApply) (int64, error) {
 	q := s.queries.WithTx(tx)
-	rows, err := tx.Query(ctx,
-		`SELECT id, sequence_num, stream_type, stream_id, stream_version,
-		        event_type, data, metadata, actor_type, actor_id, occurred_at
-		   FROM events
-		  WHERE stream_type = ANY($1)
-		  ORDER BY sequence_num`,
-		t.StreamTypes,
-	)
-	if err != nil {
-		return 0, fmt.Errorf("load events for %s: %w", t.Name, err)
-	}
-	events := make([]PersistedEvent, 0, 256)
-	for rows.Next() {
-		var ev PersistedEvent
-		if err := rows.Scan(
-			&ev.ID, &ev.SequenceNum, &ev.StreamType, &ev.StreamID, &ev.StreamVersion,
-			&ev.EventType, &ev.Data, &ev.Metadata, &ev.ActorType, &ev.ActorID, &ev.OccurredAt,
-		); err != nil {
-			rows.Close()
-			return 0, fmt.Errorf("scan event row for %s: %w", t.Name, err)
-		}
-		events = append(events, ev)
-	}
-	rows.Close()
-	if err := rows.Err(); err != nil {
-		return 0, fmt.Errorf("iterate events for %s: %w", t.Name, err)
-	}
-
-	for _, ev := range events {
-		if err := apply(ctx, q, ev); err != nil {
-			return 0, fmt.Errorf("apply event %s for %s: %w", ev.ID, t.Name, err)
-		}
-	}
-	return int64(len(events)), nil
+	const batchSize = 1000
+	var applied int64
+	var lastSeq int64
+
+	for {
+		rows, err := tx.Query(ctx,
+			`SELECT id, sequence_num, stream_type, stream_id, stream_version,
+			        event_type, data, metadata, actor_type, actor_id, occurred_at
+			   FROM events
+			  WHERE stream_type = ANY($1)
+			    AND sequence_num > $2
+			  ORDER BY sequence_num
+			  LIMIT $3`,
+			t.StreamTypes, lastSeq, batchSize,
+		)
+		if err != nil {
+			return 0, fmt.Errorf("load events for %s: %w", t.Name, err)
+		}
+
+		batch := make([]PersistedEvent, 0, batchSize)
+		for rows.Next() {
+			var ev PersistedEvent
+			if err := rows.Scan(
+				&ev.ID, &ev.SequenceNum, &ev.StreamType, &ev.StreamID, &ev.StreamVersion,
+				&ev.EventType, &ev.Data, &ev.Metadata, &ev.ActorType, &ev.ActorID, &ev.OccurredAt,
+			); err != nil {
+				rows.Close()
+				return 0, fmt.Errorf("scan event row for %s: %w", t.Name, err)
+			}
+			batch = append(batch, ev)
+		}
+		rows.Close()
+		if err := rows.Err(); err != nil {
+			return 0, fmt.Errorf("iterate events for %s: %w", t.Name, err)
+		}
+		if len(batch) == 0 {
+			break
+		}
+
+		for _, ev := range batch {
+			if err := apply(ctx, q, ev); err != nil {
+				return 0, fmt.Errorf("apply event %s for %s: %w", ev.ID, t.Name, err)
+			}
+			applied++
+			lastSeq = ev.SequenceNum
+		}
+	}
+	return applied, nil
 }

🤖 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/store/rebuild.go` around lines 299 - 332, The code buffers all
events into memory (events slice) before applying them; change to a
paged/streaming approach by iterating rows and applying each event as you scan
it instead of appending to events: inside the rows.Next() loop, scan into a
PersistedEvent and immediately call apply(ctx, q, ev), handling errors by
closing rows and returning a wrapped error (use ev.ID and t.Name like the
current error message); after the loop call rows.Close() and check rows.Err() as
you already do; keep use of tx.Query(ctx, ..., t.StreamTypes) and the existing
Scan target fields so only the buffering behavior is removed.

internal/handler/agent.go (1)

471-491: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore AgentVersion in the heartbeat payload.

The new DeviceHeartbeatPayload literal on Line 479 only sets DeviceID, but the surrounding comment says the inbox worker still writes payload.AgentVersion into devices_projection. As written, each heartbeat can push an empty version downstream and stale/blank out the projected agent version.

Suggested fix

 func (h *AgentHandler) handleHeartbeat(ctx context.Context, deviceID string, hb *pm.Heartbeat) error {
-	// hb.Uptime / CpuPercent / MemoryPercent / DiskPercent are
+	// hb.Uptime / CpuPercent / MemoryPercent / DiskPercent are
 	// intentionally NOT propagated downstream (audit N008): the inbox
 	// worker terminus only writes the payload's AgentVersion into
 	// devices_projection; the four metrics fields had no consumer and
 	// were dead writes into the event store. Live metrics will need a
 	// dedicated DeviceMetricsPayload + projection if we ever want them.
-	_ = hb
-	payload := taskqueue.DeviceHeartbeatPayload{DeviceID: deviceID}
+	payload := taskqueue.DeviceHeartbeatPayload{
+		DeviceID:     deviceID,
+		AgentVersion: hb.GetAgentVersion(),
+	}

🤖 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/handler/agent.go` around lines 471 - 491, The heartbeat payload
currently only sets DeviceID, which leaves DeviceHeartbeatPayload.AgentVersion
empty and risks blanking the projected agent version; in handleHeartbeat
populate the payload's AgentVersion from hb.AgentVersion (e.g., set
payload.AgentVersion = hb.AgentVersion) before calling
h.aqClient.EnqueueToControl so the inbox worker continues to write the correct
AgentVersion into devices_projection.

internal/api/sso_handler.go (1)

344-363: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail the callback if UserLoggedIn cannot be persisted.

This branch still mints tokens after AppendEvent fails, so a successful SSO login can bypass its audit record. For a handler mutation like this, the safer behavior is to return apiErrorCtx(..., connect.CodeInternal, ...) and abort the login.

Suggested fix

 	if err := h.store.AppendEvent(ctx, store.Event{
 		StreamType: "user",
 		StreamID:   user.ID,
 		EventType:  string(eventtypes.UserLoggedIn),
 		Data: payloads.UserLoggedIn{
 			Provider: provider.Slug,
 		},
 		ActorType: "user",
 		ActorID:   user.ID,
 	}); err != nil {
-		h.logger.Error("AUDIT GAP: failed to append UserLoggedIn event; SSO login proceeded without audit record",
-			"user_id", user.ID, "provider", provider.Slug, "error", err)
+		h.logger.Error("failed to append UserLoggedIn event",
+			"user_id", user.ID, "provider", provider.Slug, "error", err)
+		return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to record login event")
 	} else {
 		h.logger.Debug("event appended",
 			"request_id", middleware.RequestIDFromContext(ctx),
 			"stream_type", "user",
 			"stream_id", user.ID,

As per coding guidelines, "internal/api/*_handler.go: RPC handlers follow Connect-RPC patterns. Every mutation must append an event via store.AppendEvent. Errors must use apiErrorCtx with proper Connect error codes. Never silently ignore errors."

🤖 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/sso_handler.go` around lines 344 - 363, The AppendEvent error
path currently logs and continues, allowing token minting despite failing to
persist the UserLoggedIn event; change this to abort the SSO callback: when
h.store.AppendEvent(ctx, store.Event{...}) returns an error, return
apiErrorCtx(ctx, connect.CodeInternal, "failed to persist UserLoggedIn event")
(including the underlying error details) instead of continuing so the handler
stops and no tokens are minted; update the branch that now calls
h.logger.Error(...) to call apiErrorCtx and return immediately from the SSO
callback handler.

🧹 Nitpick comments (1)

internal/api/errors_test.go (1)

78-84: 💤 Low value

Redundant underscore check in loop condition.

Line 79-81 already continues the loop for underscores, so the r == '_' check on line 82 is unreachable. Consider simplifying:

Proposed fix

 for _, r := range c {
-	if r == '_' {
-		continue
-	}
-	assert.True(t, r == '_' || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9'),
+	assert.True(t, r == '_' || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9'),
 		"error code %q has non-snake_case rune %q", c, r)
 }

🤖 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/errors_test.go` around lines 78 - 84, The loop over runes in the
test (for _, r := range c) currently early-continues on underscores but then the
assertion still redundantly checks r == '_' (variable r and input c); update the
assertion to remove the unreachable r == '_' branch so it only verifies (r >=
'a' && r <= 'z') || (r >= '0' && r <= '9'), keeping the same error message and
leaving the continue for '_' in place—this simplifies the test in errors_test.go
and avoids the redundant condition.

🤖 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 `@cmd/control/main.go`:
- Around line 940-951: The event payload always sets Role: "admin" which
contradicts the intended behavior of emitting no legacy role when the role
lookup failed; change the code that builds payloads.UserCreatedWithRoles before
calling st.AppendEvent so that the legacy Role field is only set when a valid
legacy role was determined (e.g., conditionally assign &role), otherwise pass a
nil/absent Role pointer (or omit it) so Role is not set; adjust the role
variable handling near emailCopy/passwordHashCopy and roleIDs so st.AppendEvent
receives Role == nil on lookup failure.

In `@cmd/indexer/main.go`:
- Around line 196-206: After calling config.EnvInt(&cfg.Concurrency,
"INDEXER_CONCURRENCY") validate that cfg.Concurrency is > 0 and fail fast if
not: add a bounds check (e.g., if cfg.Concurrency <= 0) and return an error from
parseFlags (or call log.Fatal/process exit) with a clear message that
INDEXER_CONCURRENCY must be a positive integer; reference the symbols
cfg.Concurrency and config.EnvInt in your change so the check sits immediately
after the EnvInt override to ensure invalid env values are rejected during
config parsing.

In `@internal/api/action_params.go`:
- Around line 22-25: serializeProtoParams currently treats a nil proto.Message
as a valid empty payload which hides unset/unsupported oneof params; change
serializeProtoParams to return an error when msg is nil instead of returning {}
and update the extractor helpers (e.g., extractCreateActionParamsMsg,
extractUpdateActionParamsMsg, extractDispatchActionParamsMsg or similarly named
functions referenced around lines 22/76/120/164) to return (proto.Message,
error) and return a descriptive error for the default/unsupported case (e.g.,
"unsupported ... params type %T"); propagate that error to callers and map it to
an appropriate invalid-argument/precondition gRPC error so
create/update/dispatch fail fast on missing or unhandled params.

In `@internal/api/action_schedule.go`:
- Around line 18-32: scheduleToMap currently dereferences s and panics if called
with a nil *pm.ActionSchedule; update the function (scheduleToMap) to guard for
s == nil at the top and return an empty map (representing "no schedule") when
nil. Preserve existing behavior for non-nil schedules (populate "cron",
"interval_hours", "run_on_assign", "skip_if_unchanged") and ensure no further
dereferences occur when s is nil so callers can safely pass nil schedules.

In `@internal/api/audit_handler.go`:
- Around line 109-118: Replace the raw string event type keys
"LpsPasswordRotated" and "LuksKeyRotated" with the typed-constant event types
from internal/eventtypes to match the codebase pattern; locate the map entries
for "lps_password" and "luks_key" in audit_handler.go and change the keys to use
string(eventtypes.LpsPasswordRotated) and string(eventtypes.LuksKeyRotated)
respectively so the compiler-checked constants are used consistently.

In `@internal/api/errors_test.go`:
- Around line 63-75: The test currently validates only 11 Err* constants; update
the test to cover every Err* constant (including ErrLuksKeyNotFound and all
other Err... symbols defined in the package) so the snake_case convention is
enforced across the board—either by expanding the cases slice to include all
Err* constants (list every Err... constant symbol) or refactor the test to
programmatically collect all Err* constants from the package (e.g., build a
slice of all package-level Err... variables used by the API and iterate over
them) and assert each matches the snake_case regexp.

In `@internal/api/idp_handler.go`:
- Around line 72-89: The code currently discards the error from json.Marshal of
the group mapping (groupMappingJSON) and blindly sets GroupMapping:
json.RawMessage(groupMappingJSON) in the payloads.IdentityProviderCreated, which
can create malformed events; modify the flow around the json.Marshal(...) that
produces groupMappingJSON to capture and check its error and, if marshal fails,
return/propagate an API error (or validation error) instead of proceeding, and
only set GroupMapping on the payload when the marshal succeeded; update any
surrounding handler function (the IDP creation handler that constructs
payloads.IdentityProviderCreated) to return that error to the caller rather than
swallowing it.

In `@internal/config/durations.go`:
- Around line 41-49: ClampDurationFloor currently assigns def directly when
*target <= 0 which can produce a value below minDur; change the function to
clamp def to minDur before assigning (e.g., compute a local val := def; if val <
minDur { val = minDur } and then set *target = val) so both the zero/negative
branch and the existing < minDur branch respect the minimum; update references
to target, def, and minDur accordingly.

In `@README.md`:
- Around line 491-504: The README's Indexer Environment Variables list has three
incorrect defaults; update the default values to match the implementation in
cmd/indexer/main.go by changing INDEXER_LOG_FORMAT default from `json` to
`text`, INDEXER_CONCURRENCY default from `4` to `5`, and INDEXER_HEALTH_ADDR
default from `:8090` to `:8082` so the documented defaults for
INDEXER_LOG_FORMAT, INDEXER_CONCURRENCY, and INDEXER_HEALTH_ADDR align with the
actual behavior in main.go.
- Around line 133-141: The README lists action types that don't match the
codebase; add ACTION_TYPE_SCRIPT_RUN to the documented action types and clarify
which types are actually implemented vs. planned: scan references to
ACTION_TYPE_SCRIPT_RUN in internal/api/action_validators.go,
internal/api/action_params.go, and internal/actionparams/actionparams.go to
confirm its usage and then update the README paragraph to include SCRIPT_RUN and
a short note that the README reflects canonical proto definitions while
indicating which entries are implemented in code (or mark the others as
planned/future), so documentation matches actual implementation.

---

Outside diff comments:
In `@internal/api/sso_handler.go`:
- Around line 344-363: The AppendEvent error path currently logs and continues,
allowing token minting despite failing to persist the UserLoggedIn event; change
this to abort the SSO callback: when h.store.AppendEvent(ctx, store.Event{...})
returns an error, return apiErrorCtx(ctx, connect.CodeInternal, "failed to
persist UserLoggedIn event") (including the underlying error details) instead of
continuing so the handler stops and no tokens are minted; update the branch that
now calls h.logger.Error(...) to call apiErrorCtx and return immediately from
the SSO callback handler.

In `@internal/handler/agent.go`:
- Around line 471-491: The heartbeat payload currently only sets DeviceID, which
leaves DeviceHeartbeatPayload.AgentVersion empty and risks blanking the
projected agent version; in handleHeartbeat populate the payload's AgentVersion
from hb.AgentVersion (e.g., set payload.AgentVersion = hb.AgentVersion) before
calling h.aqClient.EnqueueToControl so the inbox worker continues to write the
correct AgentVersion into devices_projection.

In `@internal/store/rebuild.go`:
- Around line 226-230: The comment above RebuildAll is stale: remove references
to the removed PL/pgSQL projector path and the non-existent per-target
"Function" field and replace with a concise description of current behavior —
e.g., state that RebuildAll iterates matching events and invokes the active Go
projector dispatcher (or registered Go projectors) so no operator-facing changes
are required; update the comment near the RebuildAll function to mention the
porting to Go and the dispatcher-based invocation model instead of referencing
project_<stream>_event() or a Function field.
- Around line 299-332: The code buffers all events into memory (events slice)
before applying them; change to a paged/streaming approach by iterating rows and
applying each event as you scan it instead of appending to events: inside the
rows.Next() loop, scan into a PersistedEvent and immediately call apply(ctx, q,
ev), handling errors by closing rows and returning a wrapped error (use ev.ID
and t.Name like the current error message); after the loop call rows.Close() and
check rows.Err() as you already do; keep use of tx.Query(ctx, ...,
t.StreamTypes) and the existing Scan target fields so only the buffering
behavior is removed.

---

Nitpick comments:
In `@internal/api/errors_test.go`:
- Around line 78-84: The loop over runes in the test (for _, r := range c)
currently early-continues on underscores but then the assertion still
redundantly checks r == '_' (variable r and input c); update the assertion to
remove the unreachable r == '_' branch so it only verifies (r >= 'a' && r <=
'z') || (r >= '0' && r <= '9'), keeping the same error message and leaving the
continue for '_' in place—this simplifies the test in errors_test.go and avoids
the redundant condition.

🪄 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: 2dd14154-5191-448d-bfa0-14e05a56512d

📥 Commits

Reviewing files that changed from the base of the PR and between c78f36a and 2873b82.

⛔ Files ignored due to path filters (3)

• go.sum is excluded by !**/*.sum
• internal/store/generated/luks.sql.go is excluded by !**/generated/**
• internal/store/generated/models.go is excluded by !**/generated/**

📒 Files selected for processing (73)

• .github/workflows/test.yml
• Containerfile
• Containerfile.control
• README.md
• cmd/control/main.go
• cmd/indexer/main.go
• go.mod
• internal/api/action_crud.go
• internal/api/action_dispatch.go
• internal/api/action_handler.go
• internal/api/action_params.go
• internal/api/action_schedule.go
• internal/api/action_set_handler.go
• internal/api/action_validators.go
• internal/api/assignment_handler.go
• internal/api/audit_handler.go
• internal/api/audit_redactor_internal_test.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_test.go
• internal/api/helpers.go
• internal/api/helpers_test.go
• internal/api/identity_link_handler.go
• internal/api/idp_handler.go
• internal/api/internal_handler.go
• internal/api/registration_handler.go
• internal/api/role_handler.go
• internal/api/search_listener.go
• internal/api/service.go
• internal/api/settings_handler.go
• internal/api/sso_handler.go
• internal/api/system_actions.go
• internal/api/system_actions_listener.go
• internal/api/terminal_handler.go
• internal/api/token_handler.go
• internal/api/totp_handler.go
• internal/api/user_group_handler.go
• internal/api/user_handler.go
• internal/api/user_selection_handler.go
• internal/api/util_test.go
• internal/config/durations.go
• internal/config/env.go
• internal/control/inbox_worker.go
• internal/control/inbox_worker_test.go
• internal/eventtypes/payloads/action_set.go
• internal/eventtypes/payloads/compliance_policy.go
• internal/eventtypes/payloads/definition.go
• internal/eventtypes/payloads/device.go
• internal/eventtypes/payloads/device_group.go
• internal/eventtypes/payloads/execution.go
• internal/eventtypes/payloads/identity_link.go
• internal/eventtypes/payloads/idp.go
• internal/eventtypes/payloads/luks_key.go
• internal/eventtypes/payloads/misc.go
• internal/eventtypes/payloads/payloads_test.go
• internal/eventtypes/payloads/role.go
• internal/eventtypes/payloads/settings.go
• internal/eventtypes/payloads/terminal.go
• internal/eventtypes/payloads/totp.go
• internal/eventtypes/payloads/user.go
• internal/eventtypes/payloads/user_group.go
• internal/eventtypes/types_test.go
• internal/handler/agent.go
• internal/idp/linker.go
• internal/idp/linker_test.go
• internal/projectors/luks_key_listener.go
• internal/store/migrations/042_luks_keys_projection_version.sql
• internal/store/queries/luks.sql
• internal/store/rebuild.go
• internal/taskqueue/payloads.go

💤 Files with no reviewable changes (3)

• Containerfile
• Containerfile.control
• internal/api/service.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t emit an “admin” role when the lookup already failed.

The code path above explicitly says bootstrap should continue with no roles on lookup failure, but this payload still always sets Role: "admin". That makes the event self-contradictory and can mislead any consumer that still honors the legacy singular role field.

Suggested fix

-	emailCopy := email
-	passwordHashCopy := passwordHash
-	role := "admin"
+	emailCopy := email
+	passwordHashCopy := passwordHash
+	var role *string
+	if len(roleIDs) > 0 {
+		r := "admin"
+		role = &r
+	}
 	err = st.AppendEvent(ctx, store.Event{
 		StreamType: "user",
 		StreamID:   id,
 		EventType:  string(eventtypes.UserCreatedWithRoles),
 		Data: payloads.UserCreatedWithRoles{
 			Email:        &emailCopy,
 			PasswordHash: &passwordHashCopy,
-			Role:         &role,
+			Role:         role,
 			RoleIDs:      roleIDs,
 		},

📝 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

	emailCopy := email
	passwordHashCopy := passwordHash
	role := "admin"
	err = st.AppendEvent(ctx, store.Event{
	StreamType: "user",
	StreamID: id,
	EventType: "UserCreatedWithRoles",
	Data: map[string]any{
	"email": email,
	"password_hash": passwordHash,
	"role": "admin",
	"role_ids": roleIDs,
	EventType: string(eventtypes.UserCreatedWithRoles),
	Data: payloads.UserCreatedWithRoles{
	Email: &emailCopy,
	PasswordHash: &passwordHashCopy,
	Role: &role,
	RoleIDs: roleIDs,
	emailCopy := email
	passwordHashCopy := passwordHash
	var role *string
	if len(roleIDs) > 0 {
	r := "admin"
	role = &r
	}
	err = st.AppendEvent(ctx, store.Event{
	StreamType: "user",
	StreamID: id,
	EventType: string(eventtypes.UserCreatedWithRoles),
	Data: payloads.UserCreatedWithRoles{
	Email: &emailCopy,
	PasswordHash: &passwordHashCopy,
	Role: role,
	RoleIDs: roleIDs,

🤖 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 `@cmd/control/main.go` around lines 940 - 951, The event payload always sets
Role: "admin" which contradicts the intended behavior of emitting no legacy role
when the role lookup failed; change the code that builds
payloads.UserCreatedWithRoles before calling st.AppendEvent so that the legacy
Role field is only set when a valid legacy role was determined (e.g.,
conditionally assign &role), otherwise pass a nil/absent Role pointer (or omit
it) so Role is not set; adjust the role variable handling near
emailCopy/passwordHashCopy and roleIDs so st.AppendEvent receives Role == nil on
lookup failure.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject non-positive INDEXER_CONCURRENCY values during config parsing.

This override block now accepts 0 and negative values for cfg.Concurrency, and parseFlags() returns success with an invalid worker config. Please keep the bounds check here so a bad env deploy fails fast with a clear error.

Suggested fix

 	config.EnvDuration(&cfg.ReconcileInterval, "INDEXER_RECONCILE_INTERVAL")
 	config.EnvInt(&cfg.Concurrency, "INDEXER_CONCURRENCY")
 	config.EnvString(&cfg.HealthAddr, "INDEXER_HEALTH_ADDR")
+
+	if cfg.Concurrency < 1 {
+		fmt.Fprintln(os.Stderr, "FATAL: INDEXER_CONCURRENCY (or -concurrency) must be >= 1")
+		os.Exit(1)
+	}

🤖 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 `@cmd/indexer/main.go` around lines 196 - 206, After calling
config.EnvInt(&cfg.Concurrency, "INDEXER_CONCURRENCY") validate that
cfg.Concurrency is > 0 and fail fast if not: add a bounds check (e.g., if
cfg.Concurrency <= 0) and return an error from parseFlags (or call
log.Fatal/process exit) with a clear message that INDEXER_CONCURRENCY must be a
positive integer; reference the symbols cfg.Concurrency and config.EnvInt in
your change so the check sits immediately after the EnvInt override to ensure
invalid env values are rejected during config parsing.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject missing or unhandled action params instead of serializing {}.

The default branches return nil, and serializeProtoParams(nil) currently treats that as a valid empty payload. That makes an unset or unsupported oneof look successful, so create/update/dispatch can silently append or sign the wrong params instead of failing fast.

Possible fix

func serializeProtoParams(msg proto.Message) (map[string]any, error) {
	if msg == nil {
-		return map[string]any{}, nil
+		return nil, fmt.Errorf("missing action params")
	}

func extractCreateActionParamsMsg(req *pm.CreateActionRequest) (proto.Message, error) {
	switch p := req.Params.(type) {
	// ...
	default:
		return nil, fmt.Errorf("unsupported create-action params type %T", req.Params)
	}
}

Apply the same fail-closed pattern to the update/action extractors and let callers map it to an invalid-argument/precondition error.

Also applies to: 76-77, 120-121, 164-165

🤖 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/action_params.go` around lines 22 - 25, serializeProtoParams
currently treats a nil proto.Message as a valid empty payload which hides
unset/unsupported oneof params; change serializeProtoParams to return an error
when msg is nil instead of returning {} and update the extractor helpers (e.g.,
extractCreateActionParamsMsg, extractUpdateActionParamsMsg,
extractDispatchActionParamsMsg or similarly named functions referenced around
lines 22/76/120/164) to return (proto.Message, error) and return a descriptive
error for the default/unsupported case (e.g., "unsupported ... params type %T");
propagate that error to callers and map it to an appropriate
invalid-argument/precondition gRPC error so create/update/dispatch fail fast on
missing or unhandled params.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard nil schedules before dereferencing s.

scheduleToMap now panics on a nil *pm.ActionSchedule. For an emit-side helper, nil should map cleanly to “no schedule” instead of taking down the request path.

Suggested fix

 func scheduleToMap(s *pm.ActionSchedule) map[string]any {
+	if s == nil {
+		return map[string]any{}
+	}
 	m := map[string]any{}
 	if s.Cron != "" {
 		m["cron"] = s.Cron
 	}

📝 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 scheduleToMap(s *pm.ActionSchedule) map[string]any {
	m := map[string]any{}
	if s.Cron != "" {
	m["cron"] = s.Cron
	}
	if s.IntervalHours > 0 {
	m["interval_hours"] = s.IntervalHours
	}
	if s.RunOnAssign {
	m["run_on_assign"] = true
	}
	if s.SkipIfUnchanged {
	m["skip_if_unchanged"] = true
	}
	return m
	func scheduleToMap(s *pm.ActionSchedule) map[string]any {
	if s == nil {
	return map[string]any{}
	}
	m := map[string]any{}
	if s.Cron != "" {
	m["cron"] = s.Cron
	}
	if s.IntervalHours > 0 {
	m["interval_hours"] = s.IntervalHours
	}
	if s.RunOnAssign {
	m["run_on_assign"] = true
	}
	if s.SkipIfUnchanged {
	m["skip_if_unchanged"] = true
	}
	return m
	}

🤖 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/action_schedule.go` around lines 18 - 32, scheduleToMap
currently dereferences s and panics if called with a nil *pm.ActionSchedule;
update the function (scheduleToMap) to guard for s == nil at the top and return
an empty map (representing "no schedule") when nil. Preserve existing behavior
for non-nil schedules (populate "cron", "interval_hours", "run_on_assign",
"skip_if_unchanged") and ensure no further dereferences occur when s is nil so
callers can safely pass nil schedules.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if LpsPasswordRotated and LuksKeyRotated constants exist in eventtypes
rg -n "LpsPasswordRotated|LuksKeyRotated" --type go

Repository: manchtools/power-manage-server

Length of output: 13513

---

🏁 Script executed:

sed -n '95,120p' internal/api/audit_handler.go

Repository: manchtools/power-manage-server

Length of output: 1139

---

Use typed constants for event type names to match the codebase pattern.

Lines 112 and 117 use raw strings "LpsPasswordRotated" and "LuksKeyRotated" for event type keys, but the comment on lines 95-96 states that "EventTypes use the typed-constant strings from internal/eventtypes for compile-time-checked spelling." Lines 102-107 correctly follow this pattern with string(eventtypes.IdentityProviderCreated), string(eventtypes.UserPasswordChanged), etc.

The constants exist in internal/eventtypes/types.go (lines 128, 131) and should be used consistently:

Suggested fix

 	"lps_password": {
 		// LPS rotations carry an array of {username, password}
 		// records under "rotations[].password".
-		"LpsPasswordRotated": {paths: []string{"rotations[].password"}},
+		string(eventtypes.LpsPasswordRotated): {paths: []string{"rotations[].password"}},
 	},
 	"luks_key": {
 		// LUKS rotations are emitted via the internal handler with
 		// `passphrase` at the top level.
-		"LuksKeyRotated": {paths: []string{"passphrase"}},
+		string(eventtypes.LuksKeyRotated): {paths: []string{"passphrase"}},
 	},

📝 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

	"lps_password": {
	// LPS rotations carry an array of {username, password}
	// records under "rotations[].password".
	"LpsPasswordRotated": {paths: []string{"rotations[].password"}},
	},
	"luks_key": {
	// LUKS rotations are emitted via the internal handler with
	// `passphrase` at the top level.
	"LuksKeyRotated": {paths: []string{"passphrase"}},
	},
	"lps_password": {
	// LPS rotations carry an array of {username, password}
	// records under "rotations[].password".
	string(eventtypes.LpsPasswordRotated): {paths: []string{"rotations[].password"}},
	},
	"luks_key": {
	// LUKS rotations are emitted via the internal handler with
	// `passphrase` at the top level.
	string(eventtypes.LuksKeyRotated): {paths: []string{"passphrase"}},
	},

🤖 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/audit_handler.go` around lines 109 - 118, Replace the raw string
event type keys "LpsPasswordRotated" and "LuksKeyRotated" with the
typed-constant event types from internal/eventtypes to match the codebase
pattern; locate the map entries for "lps_password" and "luks_key" in
audit_handler.go and change the keys to use
string(eventtypes.LpsPasswordRotated) and string(eventtypes.LuksKeyRotated)
respectively so the compiler-checked constants are used consistently.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if ErrLuksKeyNotFound exists in errors.go
rg -n 'ErrLuksKeyNotFound' --type=go

Repository: manchtools/power-manage-server

Length of output: 291

---

🏁 Script executed:

# Read the test file around lines 63-75 to understand the test structure
cat -n internal/api/errors_test.go | sed -n '50,90p'

Repository: manchtools/power-manage-server

Length of output: 1410

---

🏁 Script executed:

# Find all Err* constants in errors.go
rg -n '^[[:space:]]*Err[A-Za-z]+ = ' internal/api/errors.go

Repository: manchtools/power-manage-server

Length of output: 683

---

🏁 Script executed:

# Get all Err* constants in errors.go with their line numbers
rg -n '^[[:space:]]*Err[A-Za-z]+ = ' internal/api/errors.go | wc -l

Repository: manchtools/power-manage-server

Length of output: 77

---

🏁 Script executed:

# Get all Err* constants in errors.go with complete list
rg -n '^[[:space:]]*Err[A-Za-z]+ = ' internal/api/errors.go -A 0

Repository: manchtools/power-manage-server

Length of output: 683

---

🏁 Script executed:

# Search more broadly for all Err* constants (may have different formatting)
rg -n 'Err[A-Za-z]+ =' internal/api/errors.go | head -30

Repository: manchtools/power-manage-server

Length of output: 683

---

🏁 Script executed:

# Read the entire errors.go file to see all constants defined
cat -n internal/api/errors.go | head -150

Repository: manchtools/power-manage-server

Length of output: 6211

---

The test checks only a subset of error constants, but should verify all Err* constants follow snake_case.

The test comment claims to "lock the convention that every Err* constant is snake_case," yet the cases slice includes only 11 of 65+ defined error constants. ErrLuksKeyNotFound (defined as "luks_key_not_found" and used in internal_handler.go) is one of many missing from the test. Add it along with other omitted constants, or refactor the test to dynamically validate all Err* constants in the package.

🤖 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/errors_test.go` around lines 63 - 75, The test currently
validates only 11 Err* constants; update the test to cover every Err* constant
(including ErrLuksKeyNotFound and all other Err... symbols defined in the
package) so the snake_case convention is enforced across the board—either by
expanding the cases slice to include all Err* constants (list every Err...
constant symbol) or refactor the test to programmatically collect all Err*
constants from the package (e.g., build a slice of all package-level Err...
variables used by the API and iterate over them) and assert each matches the
snake_case regexp.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Handle group_mapping marshal failures instead of dropping the error

GroupMapping at Line 88 depends on JSON produced with a discarded error (Line 65). If serialization ever fails, this mutation can append malformed/incomplete event data without surfacing an API error.

Suggested fix

-	groupMappingJSON, _ := json.Marshal(req.Msg.GroupMapping)
+	groupMappingJSON, err := json.Marshal(req.Msg.GroupMapping)
+	if err != nil {
+		return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, "failed to encode group mapping")
+	}

As per coding guidelines, internal/api/*_handler.go requires: "Never silently ignore errors."

🤖 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/idp_handler.go` around lines 72 - 89, The code currently
discards the error from json.Marshal of the group mapping (groupMappingJSON) and
blindly sets GroupMapping: json.RawMessage(groupMappingJSON) in the
payloads.IdentityProviderCreated, which can create malformed events; modify the
flow around the json.Marshal(...) that produces groupMappingJSON to capture and
check its error and, if marshal fails, return/propagate an API error (or
validation error) instead of proceeding, and only set GroupMapping on the
payload when the marshal succeeded; update any surrounding handler function (the
IDP creation handler that constructs payloads.IdentityProviderCreated) to return
that error to the caller rather than swallowing it.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

ClampDurationFloor can bypass minDur when def is smaller than the minimum.

At Line 43, a zero/negative input copies def directly, so the function may return a value < minDur despite its contract. Clamp def before assigning it.

Proposed fix

 func ClampDurationFloor(target *time.Duration, def, minDur time.Duration) {
+	if def < minDur {
+		def = minDur
+	}
 	if *target <= 0 {
 		*target = def
 		return
 	}
 	if *target < minDur {
 		*target = minDur
 	}
 }

📝 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 ClampDurationFloor(target *time.Duration, def, minDur time.Duration) {
	if *target <= 0 {
	*target = def
	return
	}
	if *target < minDur {
	*target = minDur
	}
	}
	func ClampDurationFloor(target *time.Duration, def, minDur time.Duration) {
	if def < minDur {
	def = minDur
	}
	if *target <= 0 {
	*target = def
	return
	}
	if *target < minDur {
	*target = minDur
	}
	}

🤖 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/durations.go` around lines 41 - 49, ClampDurationFloor
currently assigns def directly when *target <= 0 which can produce a value below
minDur; change the function to clamp def to minDur before assigning (e.g.,
compute a local val := def; if val < minDur { val = minDur } and then set
*target = val) so both the zero/negative branch and the existing < minDur branch
respect the minimum; update references to target, def, and minDur accordingly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify action type names against the SDK proto file
# Expected: All listed action types should be defined in the proto

# First, find the SDK version pinned in go.mod
SDK_VERSION=$(rg -oP 'github.com/manchtools/power-manage-sdk\s+\K\S+' go.mod | head -1)
echo "Pinned SDK version: $SDK_VERSION"

# Check if actions.proto exists in the SDK dependency
fd -t f 'actions.proto' --exec cat {} \;

# Search for action type enum definitions
rg -A 100 'enum\s+ActionType' --type proto 2>/dev/null || \
  fd -t f 'actions.proto' --exec rg -A 100 'enum\s+ActionType' {} \;

Repository: manchtools/power-manage-server

Length of output: 131

---

🏁 Script executed:

#!/bin/bash
# List of action types from README to verify
ACTION_TYPES=(
  "PACKAGE" "UPDATE" "REPOSITORY" "APP_IMAGE" "DEB" "RPM" "FLATPAK"
  "SHELL" "SYSTEMD" "SERVICE" "FILE" "DIRECTORY"
  "USER" "GROUP" "SSH" "SSHD" "ADMIN_POLICY" "LPS"
  "ENCRYPTION"
  "REBOOT" "SYNC"
)

# Search for each action type in proto or generated code
for type in "${ACTION_TYPES[@]}"; do
  echo "Checking: $type"
  rg -w "ACTION_TYPE_$type|ActionType$type" --type go --type proto -C 1 || echo "  ⚠ Not found"
done

Repository: manchtools/power-manage-server

Length of output: 1375

---

🏁 Script executed:

#!/bin/bash
# Find references to action types in the codebase
echo "=== Searching for action type references in code ==="
rg -i "PACKAGE|UPDATE|REPOSITORY|DEB|RPM|FLATPAK|SHELL|SYSTEMD" --type go -C 2 | head -100

echo ""
echo "=== Checking vendor or generated proto files ==="
fd -t f 'actions.pb.go' --search-path vendor --search-path go

echo ""
echo "=== Looking for SDK proto references in imports ==="
rg 'pm/v1|actions' --type go | grep -i import | head -20

echo ""
echo "=== Checking go.sum for SDK version ==="
rg 'manchtools/power-manage-sdk' go.sum

Repository: manchtools/power-manage-server

Length of output: 5501

---

🏁 Script executed:

#!/bin/bash
# Search for action type enum usage patterns
echo "=== Looking for ActionType enum or constants ==="
rg 'ActionType' --type go -B 2 -A 2 | head -150

echo ""
echo "=== Searching for action case statements or type switches ==="
rg -i 'case.*PACKAGE|case.*SHELL|case.*USER|case.*ENCRYPTION' --type go -B 2 -A 1

Repository: manchtools/power-manage-server

Length of output: 36487

---

Update action type documentation to match actual implementation and include missing types.

The listed action types are incomplete. The code actively uses ACTION_TYPE_SCRIPT_RUN (referenced in internal/api/action_validators.go, internal/api/action_params.go, and internal/actionparams/actionparams.go), which is not documented in the README. Additionally, many types listed in the README (UPDATE, REPOSITORY, DEB, RPM, SYSTEMD, FILE, GROUP, SSH, SSHD, ADMIN_POLICY, REBOOT, SYNC) have no references in the codebase. Either add SCRIPT_RUN to the documentation and clarify which types are actually implemented, or note that the README reflects planned/future action types from the canonical proto definitions.

🤖 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 `@README.md` around lines 133 - 141, The README lists action types that don't
match the codebase; add ACTION_TYPE_SCRIPT_RUN to the documented action types
and clarify which types are actually implemented vs. planned: scan references to
ACTION_TYPE_SCRIPT_RUN in internal/api/action_validators.go,
internal/api/action_params.go, and internal/actionparams/actionparams.go to
confirm its usage and then update the README paragraph to include SCRIPT_RUN and
a short note that the README reflects canonical proto definitions while
indicating which entries are implemented in code (or mark the others as
planned/future), so documentation matches actual implementation.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify indexer env var defaults against cmd/indexer/main.go
# Expected: Default values in README should match code

# Check if indexer main.go exists and display env var handling
fd -t f 'main.go' cmd/indexer --exec cat {} \;

# Search for env var default definitions
echo "=== Searching for INDEXER_* environment variable defaults ==="
rg -n 'INDEXER_' cmd/indexer/ -A 2 -B 2

# Check internal/config for shared env helpers
echo "=== Checking internal/config/env.go for env helpers ==="
rg -n 'GetEnv|Default' internal/config/ -C 3 || echo "Config package not found or no env helpers"

Repository: manchtools/power-manage-server

Length of output: 12957

---

Fix three incorrect environment variable defaults in the Indexer documentation.

The README contains outdated defaults that do not match the actual implementation in cmd/indexer/main.go:

• INDEXER_LOG_FORMAT default should be text, not json
• INDEXER_CONCURRENCY default should be 5, not 4
• INDEXER_HEALTH_ADDR default should be :8082, not :8090

🤖 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 `@README.md` around lines 491 - 504, The README's Indexer Environment Variables
list has three incorrect defaults; update the default values to match the
implementation in cmd/indexer/main.go by changing INDEXER_LOG_FORMAT default
from `json` to `text`, INDEXER_CONCURRENCY default from `4` to `5`, and
INDEXER_HEALTH_ADDR default from `:8090` to `:8082` so the documented defaults
for INDEXER_LOG_FORMAT, INDEXER_CONCURRENCY, and INDEXER_HEALTH_ADDR align with
the actual behavior in main.go.