refactor: address audit Bundle D — code-org quick wins (F016, F028, F049)

[PaulDotterer]

Summary

Fourth bundle from the 2026-05-07 server tech-debt audit. Builds on the
3 already-merged audit PRs (#141, #144, #147) + the #159 Phase 2 port.
Closes F016, F028, F049. Filed F024 (#164) + F032/F048 (#165) as
deferred — they deserve focused PRs with their own CR review surface
rather than riding inside this small bundle.

• F016 — Dedupe ActionSigner interface across api + control. The
  canonical declaration now lives in `internal/ca/signer.go` (next to
  `NewActionSigner`); both consumers import `ca.ActionSigner`. Tests
  keep working because the interface still allows a `NoOpSigner`
  double; production keeps using `*verify.ActionSigner` from the SDK.
• F028 — Drop the reimplemented `indexByte` in
  `internal/handler/agent.go`. Use `strings.IndexByte` directly.
• F049 — Move `scheduleToMap` / `scheduleFromJSON` to a new
  `internal/actionparams/schedule.go`. Three sibling handlers
  (action_set, definition, internal_handler) no longer have to import
  action_handler-internal helpers. CR-CLI also caught a latent silent
  `json.Unmarshal` failure path during the move — corrupted JSONB now
  emits an `actionparams: schedule JSON malformed` slog.Warn instead
  of silently returning nil.

CR-CLI returned 0 findings on the final commit before push.

Filed as deferred issues

• audit F024: extract dispatchEach helper for 5 fan-out RPCs #164 — F024 `dispatchEach` helper for the 5 fan-out RPCs
• audit F032+F048: dispatchInputs struct in DispatchAction (and var params any clarification) #165 — F032+F048 `dispatchInputs` struct (touches ~30 references)

Test plan

• Lint workflow green
• Tests workflow green

Summary by CodeRabbit

• Refactor
  • Centralized schedule serialization/deserialization into a single helper to reduce duplication and improve robustness when schedules are empty or malformed.
  • Standardized action signing across services and workers to unify signing behavior.
  • Improved hostname parsing in agent bootstrap redirect handling for better IPv6 and port handling.

[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: e22a60e9-262e-4fe0-a096-840ac674b9dc

📥 Commits

Reviewing files that changed from the base of the PR and between bf846c0 and efb53ed.

📒 Files selected for processing (1)

• internal/handler/agent.go

---

📝 Walkthrough

Walkthrough

Centralizes schedule (de)serialization into internal/actionparams, adds an exported ActionSigner interface in internal/ca, updates inbox worker, service/system managers, and API handlers to use the new contracts, and replaces custom host:port parsing with net.SplitHostPort.

Changes

Schedule Helper and Signer Interface Centralization

Layer / File(s)	Summary
Interface and Contract Definitions internal/ca/signer.go, internal/actionparams/schedule.go	Adds exported ActionSigner interface in internal/ca and ScheduleToMap/ScheduleFromJSON helpers in internal/actionparams.
Inbox Worker Signer Migration internal/control/inbox_worker.go	Removes local ActionSigner interface; changes InboxWorker and NewInboxWorker to accept ca.ActionSigner.
Service & System Manager Signer Wiring internal/api/service.go, internal/api/system_actions.go	Updates NewControlService and NewSystemActionManager / SystemActionManager.signer to use ca.ActionSigner.
Action Set Handler Schedule Wiring internal/api/action_set_handler.go	Imports internal/actionparams; switches CreateActionSet and actionSetToProto to actionparams.ScheduleToMap / ScheduleFromJSON.
Definition Handler Schedule Wiring internal/api/definition_handler.go	Imports internal/actionparams; switches CreateDefinition, UpdateDefinitionSchedule, and definitionToProto to use the centralized schedule helpers.
Internal Handler Schedule Wiring internal/api/internal_handler.go	Switches ProxySyncActions, dbActionToWireAction, and dbResolvedActionToWireAction to actionparams.ScheduleFromJSON.
Bootstrap Redirect Middleware internal/handler/agent.go	Uses net.SplitHostPort to strip ports from r.Host and removes the local indexByte helper.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• manchtools/power-manage-server#91: Modifies schedule (de)serialization sites and is directly related to centralizing schedule helpers.
• manchtools/power-manage-server#25: Original PR that introduced local schedule persistence helpers which are now centralized.

Poem

🐰 I hop through maps and JSON well,

Signers now ring the single bell,
Helpers gather, tidy, bright,
Handlers sip from one true light,
Code hops home at dusk—what a sight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main refactoring effort addressing three audit findings (F016, F028, F049) related to code organization quick wins.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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/code-org-quick-wins

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/actionparams/schedule.go (1)

44-72: ⚡ Quick win

ScheduleFromJSON doesn't validate IntervalHours ≥ 0, creating a silent gap opposite to ScheduleToMap's write guard.

ScheduleToMap only stores interval_hours when > 0, but ScheduleFromJSON doesn't apply a reciprocal check on read. A corrupted JSONB row with "interval_hours": -5 passes the empty-object guard at line 63 (because -5 != 0) and propagates IntervalHours: -5 to the agent scheduler without logging anything — inconsistent with the warn-on-corrupt-data intent already present on line 58.

🛡️ Proposed defensive check

 	if raw.Cron == "" && raw.IntervalHours == 0 && !raw.RunOnAssign && !raw.SkipIfUnchanged {
 		return nil
 	}
+	if raw.IntervalHours < 0 {
+		slog.Warn("actionparams: schedule JSON has negative interval_hours; clamping to zero",
+			"interval_hours", raw.IntervalHours)
+		raw.IntervalHours = 0
+	}
 	return &pm.ActionSchedule{

🤖 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/actionparams/schedule.go` around lines 44 - 72, ScheduleFromJSON
must reject negative interval_hours to match ScheduleToMap's write guard: after
unmarshalling, check if raw.IntervalHours is negative; if so, emit a structured
slog.Warn similar to the existing malformed JSON log (include "bytes",
len(data), "error" or a clear message and the invalid value) and treat the row
as "no schedule" by returning nil instead of creating a pm.ActionSchedule; keep
all other behavior unchanged.

🤖 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/handler/agent.go`:
- Around line 230-232: The current reqHost trimming uses
strings.IndexByte(reqHost, ':') which breaks bracketed IPv6 like
"[2001:db8::1]:443"; replace that logic in internal/handler/agent.go so you
parse host:port correctly: use net.SplitHostPort(reqHost) to get the host when a
port is present, and on error fall back to leaving reqHost as-is (or, if you
must strip a port, only strip using the last ':' when the host is not bracketed)
so bracketed IPv6 addresses remain intact; update the code around the reqHost
variable (where strings.IndexByte is used) to use this safer parsing approach.

---

Nitpick comments:
In `@internal/actionparams/schedule.go`:
- Around line 44-72: ScheduleFromJSON must reject negative interval_hours to
match ScheduleToMap's write guard: after unmarshalling, check if
raw.IntervalHours is negative; if so, emit a structured slog.Warn similar to the
existing malformed JSON log (include "bytes", len(data), "error" or a clear
message and the invalid value) and treat the row as "no schedule" by returning
nil instead of creating a pm.ActionSchedule; keep all other behavior unchanged.

🪄 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: c1b9bfd4-6852-4703-81b0-7a4dfa0ddd36

📥 Commits

Reviewing files that changed from the base of the PR and between 6170cbf and bf846c0.

📒 Files selected for processing (9)

• internal/actionparams/schedule.go
• internal/api/action_set_handler.go
• internal/api/definition_handler.go
• internal/api/internal_handler.go
• internal/api/service.go
• internal/api/system_actions.go
• internal/ca/signer.go
• internal/control/inbox_worker.go
• internal/handler/agent.go