feat: rc11 consolidation — derived system actions, terminal-error split, install.sh, guided setup#83
Merged
Merged
Conversation
…gateway_not_registered code, install.sh + guided setup.sh Bundles four rc11 issues into one PR per the project's CodeRabbit efficiency preference. Each section below maps to a tracking issue. #77 — system actions as a derived projection System actions (pm-tty-*, USER provision, SSH access) become a pure projection of user / role / group / SCIM source state. Handlers no longer mutate the manager directly; instead, a post-commit event listener fires SyncUserSystemActions for the affected users automatically. * internal/store/store.go — generalises OnEventAppended into a listener slice via RegisterEventListener; existing search-index hook keeps working unchanged. * internal/api/system_actions_listener.go (new) — AffectedFromEvent classifier mapping every permission-shaping event type to its SyncOp (per-user, fan-out → all, deliberate no-op for UserDeleted). 15-case unit test locks in the contract. * internal/api/system_actions.go — adds StartReconciliation, the periodic safety net (1m default, 5m sweep timeout, atomic overlap guard, per-sweep context). Demotes the per-sweep success log from Info to Debug so the tight cadence doesn't spam. * cmd/control/main.go — registers the listener post-commit and starts the reconciler. Also rewires scim.NewHandler to receive the SystemActionManager so SCIM's user-delete path can finally call CleanupDeletedUserActions. * internal/scim/handler.go + users.go — defines a narrow SystemActionsCleaner interface (no api → scim cycle), threads it through, calls cleanup on the SCIM delete path. SCIM previously bypassed cleanup entirely, leaving orphan pm-tty-* and USER actions on every device the deleted user touched. * internal/api/user_handler.go — removes all 8 scattered SyncUserSystemActions calls (the listener covers them). #78 — partial terminal config warn When an operator sets one or two of the three terminal env vars (GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE, GATEWAY_WEB_LISTEN_ADDR, GATEWAY_TTY_DOMAIN) but not all, the gateway used to silently skip terminal registration. Operators only discovered it when StartTerminal failed hours later with a generic toast. rc11 surfaces the partial state at startup: * internal/config/config.go — Validate() now returns (warnings []string, err error). Adds TTYDomainExplicitlySet field tracking whether GATEWAY_TTY_DOMAIN was set in the env (separate from the always-falls-back-to-GATEWAY_DOMAIN behavior of TraefikTTYHost). New partial-config detection emits a warning naming the missing var(s); all-set and all-unset stay silent. * cmd/gateway/main.go — logs each warning at Warn before the fatal-check. * internal/config/config_test.go — TestValidate_PartialTerminalConfig covers all 7 partial-config combinations. #79 — gateway_not_registered error code StartTerminal's "gateway has no terminal URL registered" path used to return the generic ErrInternal code. Web mapped that to "An unexpected error occurred" — useless. Real diagnosis: GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE wasn't set on the gateway. * internal/api/errors.go — new ErrGatewayNotRegistered. * internal/api/terminal_handler.go — terminal_handler.go:274 now returns the new code with a server-side message that spells out the operator action. The other ErrInternal site at line 279 (registry unreachable) stays — that one IS a generic infra failure. * Matching const lands in sdk/ts/errors.ts (sdk PR) and paraglide / map entries land in web/src/lib/errors.ts (web PR). The errors-parity test exercises both sides. #80 — bootstrap install.sh + guided setup.sh Fresh installs used to require ~10 manual env edits across multiple unrelated sections, each with its own footgun (URL-safe indexer password, distinct TTY domain, hex-only encryption key). rc11 ships a one-line installer: curl -fsSL .../install.sh | sudo bash * deploy/install.sh (new) — preflight (docker + compose plugin present, daemon reachable; does NOT install docker), download deploy/ tree from the chosen RELEASE_TAG (default latest-rc), init .env, run setup.sh, pull images, docker compose up -d, print summary with URLs. * deploy/setup.sh — adds guided mode that prompts for missing values, auto-generates strong defaults for every secret (openssl rand for postgres/valkey/JWT/encryption-key/admin password; -hex 32 for the URL-safe-required indexer password and the encryption key), validates URL-safety / hex / hostname constraints inline, and auto-composes the GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE from the chosen TTY domain so the operator never types {id} by hand. --no-prompt and non-TTY stdin preserve the existing CI-friendly path. * deploy/QUICKSTART.md (new) — operator-facing one-page guide. * deploy/.env.example — documents the two new CONTROL_SYSTEM_ACTION_RECONCILE_* env vars. Tests: full server build clean. Targeted suite (config validate matrix, system-action listener classifier, error-code parity, existing terminal/login/registration regressions) all green. Closes #77, #78, #79, #80.
…p.sh disable-clears
Pre-push review round 1 surfaced three issues on the rc11
consolidation commit. All three legit; fixed before push.
1. Synchronous listener turns admin RPCs into O(all-users) work
(internal/store/store.go fan-out path, internal/api/system_actions_listener.go).
The store fires listeners synchronously inside AppendEvent. The
system-action listener's SyncOpSyncAll branch calls
SyncAllUsersSystemActions on a fan-out event (RoleUpdated,
RoleDeleted, UserGroupRoleAssigned/Revoked, UserGroupDeleted,
ServerSettingUpdated). On a 10k-user fleet that's a multi-second
blocking call on the request path — the operator's "save role"
click waits for the full fleet sweep.
Detach the entire dispatch into a goroutine. context.Background()
rather than the AppendEvent ctx because the RPC may have already
returned (cancelling its ctx) by the time the sync runs; we want
the work to complete regardless. The periodic reconciler is the
safety net if the goroutine itself dies before logging.
Per-user paths (SyncOpSyncUser) are also async for consistency —
the work is small per-user but inconsistent semantics across
listener branches would have been a footgun.
2. ErrGatewayNotRegistered conflates two distinct cases
(internal/api/terminal_handler.go).
The previous cut returned ErrGatewayNotRegistered for both:
a. Persistent — gateway alive, terminal URL never published
(operator forgot GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE).
b. Transient — gateway died between the device→gateway lookup
and the gateway→URL lookup.
Same code → same UI message → operator gets a config-error toast
for what was actually a retry-and-recover situation.
Probe the internal-URL key for the same gatewayID to distinguish:
if internal is present, the gateway is alive and the missing
terminal URL is persistent (case a → ErrGatewayNotRegistered with
the "set the env" message). If the internal probe also returns
ErrNoGateway, the gateway is gone (case b → ErrDeviceNotConnected
with a retry-shortly message). Other probe failures fall through
to the generic registry-unavailable path.
3. Guided setup can't disable terminals on a rerun
(deploy/setup.sh).
If .env already had GATEWAY_TTY_DOMAIN /
GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE / GATEWAY_WEB_LISTEN_ADDR
set from a prior run, answering No to "Enable remote terminal
sessions?" only skipped writing — it didn't clear the existing
values. Operator thought terminals were off; gateway still
published its terminal URL on next boot.
Add clear_env_var helper that removes a key=value line entirely.
The No path now explicitly clears all three terminal env vars
when present, with a log line confirming the change so the
operator can see the disable actually took effect.
Targeted tests still green: config Validate matrix, system-action
listener classifier (15 cases), error-code parity, terminal token
single-use, login bypass regression, gateway URL validator.
The async system-action listener spawned goroutines with context.Background() and no timeout. A wedged DB or signer would have leaked one goroutine per event indefinitely, and a burst of role/group changes could pile up. Bound each dispatch to a configurable timeout (default 5m, matching the periodic reconciler) via CONTROL_SYSTEM_ACTION_RECONCILE_TIMEOUT. Goroutine still uses a fresh Background-rooted context — the originating RPC may have returned before the sync runs, so the timeout is the only stop signal. Tests: - terminal_handler_test.go: split persistent-misconfig vs transient-loss paths (the rc11 #79 review fix). Asserts ErrGatewayNotRegistered with the GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE hint when the gateway is alive but has not published its terminal URL, and a transient retry message when the gateway has disappeared. - deploy/setup_test.sh: bash-level regression suite for the rc11 #80 review fix (clear_env_var was added so the disable-terminals branch actually clears all three vars on rerun, not silently no-op). Covers write_env_var add/update/comment-preserve, clear_env_var existing/missing, and the disable-terminals-clears-three-vars scenario specifically.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces direct handler-triggered system-action syncs with a post-commit store event listener and an optional periodic reconciler. Adds store listener registration, event→sync classification, listener dispatch with timeouts/semaphores/coalescing, reconciliation scheduling/guards, SCIM delete cleanup wiring, config warnings, and deploy/installer additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Mutating<br/>RPC Handler
participant Store as Store
participant Listener as SystemAction<br/>Listener
participant Manager as SystemAction<br/>Manager
Handler->>Store: AppendEvent(permission-shaping)
activate Store
Store->>Store: commit transaction
Store->>Store: fireListeners(ctx, event)
deactivate Store
activate Listener
Note over Listener: AffectedFromEvent → (op, userIDs)\nspawn non-blocking goroutine (timeout, semaphore)
alt SyncOpSyncUser
Listener->>Manager: SyncUserSystemActions(ctx, userID)
else SyncOpSyncAll
Listener->>Manager: SyncAllUsersSystemActions(ctx)
else SyncOpCleanupUser
Listener->>Manager: CleanupDeletedUserActions(ctx, userID)
end
Manager-->>Listener: result (errors logged)
deactivate Listener
Handler-->>Handler: RPC returns (post-commit)
sequenceDiagram
participant Boot as Control Server<br/>Startup
participant Store as Store
participant Manager as SystemAction<br/>Manager
Boot->>Manager: SyncAllUsersSystemActions(ctx) -- one-shot
Manager-->>Boot: initial sync complete (Info)
Boot->>Store: RegisterEventListener(SystemActionListener)
Boot->>Manager: StartReconciliation(ctx, interval, timeout)
activate Manager
loop every interval (if interval>0)
Manager->>Manager: if sweep in-flight -> log & skip
Manager->>Manager: ctx_sweep := context.WithTimeout(ctx, timeout)
Manager->>Manager: SyncAllUsersSystemActions(ctx_sweep)
Manager->>Manager: clear in-flight guard
end
deactivate Manager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
internal/store/store.go (1)
35-39: Document the ctx lifetime contract for listeners.The
ctxpassed to listeners is the caller's request context, which gets canceled as soon asAppendEventreturns. Listeners that do anything beyond a quick in-memory dispatch (likeSystemActionListener, which spawns a goroutine withcontext.Background()) need to be aware of this. Worth a one-liner on theEventListenerdoc to spell it out so future listener authors don't accidentally use the passedctxfor downstream DB calls and get surprising cancellations.// EventListener is a post-commit hook fired after a successful // AppendEvent. Listeners are invoked synchronously in registration // order; a slow listener will stall subsequent listeners on the same -// event. None are allowed to mutate the event row. +// event. None are allowed to mutate the event row. +// +// The ctx passed in is the caller's request context and will be +// cancelled when AppendEvent returns; listeners that perform +// asynchronous work must derive their own context (typically +// context.Background with an explicit timeout). type EventListener func(ctx context.Context, ev PersistedEvent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/store.go` around lines 35 - 39, Update the EventListener doc comment to state the ctx lifetime contract: note that the ctx passed to EventListener is the caller's request context and will be canceled as soon as AppendEvent returns, so listeners must not rely on it for long-running work or downstream DB calls; for background work (e.g. in SystemActionListener which spawns goroutines) create a new context (context.Background() or a derived context with its own cancellation) and avoid mutating the PersistedEvent.deploy/install.sh (2)
103-103: Use single quotes for the trap body (Shellcheck SC2064).
$tmpdirhappens to be set on the previous line and never reassigned, so the current double-quoted form works, but defensively single-quoting expands the variable at trap-fire time, which is the intent for a cleanup hook and what Shellcheck flags here.- trap "rm -rf '$tmpdir'" EXIT + trap 'rm -rf "$tmpdir"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/install.sh` at line 103, The trap currently uses double quotes so $tmpdir is expanded immediately; change the trap invocation to use single quotes for the body (trap 'rm -rf "$tmpdir"' EXIT) so the tmpdir variable is expanded at trap execution time; update the trap call where tmpdir is set and referenced (the trap line referencing tmpdir) to use single-quoted body to satisfy Shellcheck SC2064 and ensure correct cleanup behavior.
228-228: Simplify the logs command to not require path qualification.The
docker composecommand automatically detectscompose.ymlor other standard compose filenames in the current directory. Since the script already usescd "$INSTALL_DIR"elsewhere, this command can be simplified:- echo " Logs: docker compose -f $INSTALL_DIR/compose.yml logs -f" + echo " Logs: (cd $INSTALL_DIR && docker compose logs -f)"This makes the command clearer and more robust to future changes in the compose filename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/install.sh` at line 228, Update the echo that prints the logs command to remove the path-qualified compose file; replace the current message that references "docker compose -f $INSTALL_DIR/compose.yml logs -f" with a simplified "docker compose logs -f" since the script already cds into INSTALL_DIR and docker compose will auto-detect compose.yml (reference the echo line that prints the Logs command and the INSTALL_DIR/compose.yml filename).cmd/control/main.go (1)
780-784: Guard againstSystemActionReconcileTimeout=0to avoid an already-cancelled per-sweep context.The default is
5*time.Minute, but operators can setCONTROL_SYSTEM_ACTION_RECONCILE_TIMEOUT=0(or a negative duration) via env. UnlikeSystemActionListener, which falls back todefaultListenerSyncTimeoutwhensyncTimeout <= 0,SystemActionManager.StartReconciliationpasses the value straight tocontext.WithTimeout(ctx, sweepTimeout)— a0timeout produces an immediately-expired context, so every sweep would fail withcontext deadline exceededand only the listener path would actually do work. Same concern forSystemActionReconcileInterval: a negative value wouldpanicintime.NewTicker(only<= 0is checked as "disabled" inStartReconciliation, treating negative as disabled — verify this matches the doc string).Consider clamping these to sane minimums (or rejecting them) in
parseFlags, the same wayDynamicGroupEvalIntervalis bounded a few lines below.♻️ Proposed clamp
+ // rc11 `#77`: a 0 sweep timeout would make context.WithTimeout return an + // already-cancelled context on every tick. Clamp to the default rather + // than silently breaking the reconciler. + if cfg.SystemActionReconcileInterval < 0 { + cfg.SystemActionReconcileInterval = 0 // disabled + } + if cfg.SystemActionReconcileTimeout <= 0 { + cfg.SystemActionReconcileTimeout = 5 * time.Minute + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/control/main.go` around lines 780 - 784, Clamp user-provided system-action reconcile flags in parseFlags: if cfg.SystemActionReconcileTimeout <= 0 set it to a sane minimum (e.g. the same fallback used by the listener or a small positive duration) and if cfg.SystemActionReconcileInterval <= 0 treat it as disabled consistently (or clamp to a minimum >0 if you prefer enabling); this prevents passing a zero/negative sweepTimeout into SystemActionManager.StartReconciliation (which calls context.WithTimeout and produces an immediately-canceled context) and matches the bounding behavior used for DynamicGroupEvalInterval.deploy/setup.sh (1)
314-330: Use a same-directory temp file for atomic.envrewrite.The comment promises an atomic update, but
mktempdefaults to$TMPDIR(typically/tmp), which is often a separate mount from$SCRIPT_DIR. When that's the case,mv "$tmp" "$envfile"falls back to a non-atomic copy-then-unlink and also overwrites the file with the temp file's permissions/ownership rather than preserving.env's.Creating the temp inside
SCRIPT_DIRkeepsmvas a same-filesystemrename(2)(atomic POSIX-wise) and lets you copy mode/owner from the original first. Same fix applies toclear_env_var.♻️ Proposed fix
write_env_var() { local key="$1" value="$2" envfile="$SCRIPT_DIR/.env" if grep -qE "^${key}=" "$envfile"; then - # Use a non-/ delimiter for sed because values frequently contain / local tmp - tmp="$(mktemp)" + tmp="$(mktemp "${envfile}.XXXXXX")" awk -v k="$key" -v v="$value" ' BEGIN { found = 0 } $0 ~ "^"k"=" { print k"="v; found = 1; next } { print } END { if (!found) print k"="v } ' "$envfile" > "$tmp" + # Preserve original mode so .env stays 0600 across rewrites. + chmod --reference="$envfile" "$tmp" 2>/dev/null || chmod 600 "$tmp" mv "$tmp" "$envfile" else printf '%s=%s\n' "$key" "$value" >> "$envfile" fi }internal/api/system_actions_listener.go (3)
56-73: Minor: fall back toStreamIDparsing whenevent.data["user_id"]is missing.For
UserRoleAssigned/UserRoleRevokedandUserGroupMemberAdded/UserGroupMemberRemoved, the classifier returnsSyncOpNonewhen the data payload is missing or malformed. The comment notes this is intentional ("read the data payload to avoid parsing the colon-joined StreamID"), and the periodic reconciler is a safety net — so this is correct.Worth considering, though: if a future emitter regresses on the data-payload contract, the listener silently no-ops for up to one reconcile interval (default 1m). For the role case,
StreamIDisuser_id:role_idand astrings.Cut(e.StreamID, ":")fallback would give the same answer at no extra cost and tighten the failure mode to be self-correcting. Same idea isn't possible for group membership since the stream is the group, not the user.Not blocking — flagging in case it's worth the defensive fallback for the role case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener.go` around lines 56 - 73, The handler currently returns SyncOpNone when userIDFromEventData(e) is empty for "UserRoleAssigned"/"UserRoleRevoked"; add a defensive fallback that parses the event StreamID (which is "user_id:role_id") to extract the user id using strings.Cut(e.StreamID, ":") and, if that yields a non-empty uid, return SyncOpSyncUser, []string{uid} instead of SyncOpNone; keep the group-member cases unchanged (no fallback) and reference the case labels, userIDFromEventData, e.StreamID, and SyncOpSyncUser when making the change.
150-193: Unbounded goroutine spawn + no overlap guard for fan-out events.The listener intentionally drops AppendEvent's context and spawns a fresh goroutine per event (good), but there's no upper bound on concurrency. Two real-world scenarios this matters for:
- Burst of per-user events. A bulk role assignment / SCIM sync that emits, say, 5k
UserRoleAssignedevents spawns 5k concurrentSyncUserSystemActionscalls — each of which holds a DB connection and exercises the action signer. Easy to exhaust the pgx pool or saturate the signer well below DB limits.- Repeated fan-out events. Multiple
RoleUpdated/ServerSettingUpdatedevents in quick succession each kick off a fullSyncAllUsersSystemActions, all running in parallel. The periodic reconciler uses anatomic.Boolto prevent overlapping sweeps (persystem_actions.go:83-115); the listener's fan-out path has no such guard, so N admin clicks → N concurrent O(all-users) sweeps stepping on each other.Recommend (a) a buffered worker pool (or
errgroupwithSetLimit) so the listener back-pressures rather than fans out unbounded, and (b) reusing the reconciler's "running" flag for theSyncOpSyncAllpath so consecutive fan-out events coalesce instead of stack.♻️ Sketch (worker pool + coalesce SyncAll)
// At construction time: sem := make(chan struct{}, 16) // bounded concurrency var syncAllRunning atomic.Bool // coalesce fan-outs return func(_ context.Context, e store.PersistedEvent) { op, userIDs := AffectedFromEvent(e) if op == SyncOpNone { return } if op == SyncOpSyncAll && !syncAllRunning.CompareAndSwap(false, true) { // a sweep is already in flight; this event will be picked up by it // (or by the next reconciler tick). return } select { case sem <- struct{}{}: default: logger.Warn("system-action listener: backpressure, dropping event (reconciler will catch up)", "event_type", e.EventType, "event_id", e.ID) if op == SyncOpSyncAll { syncAllRunning.Store(false) } return } go func() { defer func() { <-sem }() if op == SyncOpSyncAll { defer syncAllRunning.Store(false) } // ... existing dispatch ... }() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener.go` around lines 150 - 193, SystemActionListener currently spawns an unbounded goroutine per event (via the anon goroutine in SystemActionListener after AffectedFromEvent), which can exhaust DB/signing resources for many SyncUserSystemActions or run multiple SyncAllUsersSystemActions concurrently; change it to use a bounded concurrency semaphore/worker pool (e.g., a buffered channel or errgroup.SetLimit) to limit concurrent goroutines and apply backpressure (log+drop when pool is full), and reuse the reconciler's running flag (atomic.Bool) for the SyncOpSyncAll path so SyncAllUsersSystemActions coalesces (CompareAndSwap to avoid starting another sweep and ensure the flag is cleared after the sweep completes); ensure the semaphore slot is always released and the syncAll flag is cleared in all return paths so no leaks occur.
154-162: Usecontext.WithoutCancelto preserve request ID correlation in async operations.The Background-rooted goroutine context correctly drops cancellation after the RPC returns, but it also discards the request ID stored in the AppendEvent context via the RequestID middleware. This breaks observability—the "sync user failed" / "sync all users failed" error logs cannot be correlated to the RPC that triggered the event.
Since the project targets Go 1.25 (well above the 1.21 requirement),
context.WithoutCancel(parent)is available and stable. It preserves context values while detaching from cancellation, which matches the intent here.♻️ Suggested change
- return func(_ context.Context, e store.PersistedEvent) { + return func(parent context.Context, e store.PersistedEvent) { op, userIDs := AffectedFromEvent(e) if op == SyncOpNone { return } go func() { - ctx, cancel := context.WithTimeout(context.Background(), syncTimeout) + // Preserve request-id and other context values from the AppendEvent ctx but + // detach from its cancellation — RPC has already returned. + ctx, cancel := context.WithTimeout(context.WithoutCancel(parent), syncTimeout) defer cancel()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener.go` around lines 154 - 162, The goroutine currently uses context.Background() which drops request-scoped values (like RequestID); rename the ignored parameter "_" to "ctx" in the outer handler func signature and replace context.Background() with context.WithoutCancel(ctx) when creating the goroutine's ctx (still use context.WithTimeout and defer cancel as before) so the async work is detached from cancellation but preserves request values for logging in functions like AffectedFromEvent and the subsequent sync error logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/install.sh`:
- Around line 139-148: After extracting the tarball with tar -xzf "$tarball" ...
'*/deploy/*', add a verification step that at least one known-required file from
the deploy payload (for example the expected setup script like setup.sh or a
marker file) exists in "$INSTALL_DIR" (or the extracted deploy subdir); if the
file is missing, call the existing logging function (e.g., log_error) with a
clear message that the tarball did not contain the deploy/ payload and exit
nonzero (exit 1) to prevent continuing into restore/copy and running setup.sh
against an empty directory.
In `@deploy/QUICKSTART.md`:
- Around line 22-55: The docs show env var prefixes that only apply to the local
curl process and won't be seen by sudo; update examples to pass the vars to the
command run under sudo (or export + sudo -E). Replace the lines using
"RELEASE_TAG=... curl ... | sudo bash" and "RELEASE_TAG=... sudo bash
/opt/power-manage/install.sh" with working forms such as "curl -fsSL ... | sudo
RELEASE_TAG=v2026.05 bash" and "sudo RELEASE_TAG=v2026.06 bash
/opt/power-manage/install.sh", and mention the alternative "export NO_PROMPT=1
&& sudo -E bash ..." if you prefer preserving the environment; reference the
RELEASE_TAG, NO_PROMPT, INSTALL_DIR and install.sh usages so the installer
receives the variables.
In `@deploy/setup_test.sh`:
- Around line 71-83: The current pattern runs the test in a subshell (...) and
then inspects $? which dead-ends under set -e; change the control flow to
evaluate the subshell directly in the if condition so the command's exit is
handled by the if instead of triggering set -e; specifically replace the "( ...
)" + subsequent "if [[ $? -eq 0 ]]" logic with "if ( <run test command> ); then"
and move the PASS_COUNT/FAIL_COUNT increments into the corresponding then/else
branches (referencing the existing subshell invocation and the PASS_COUNT and
FAIL_COUNT variables).
- Line 17: The variable SCRIPT_DIR_REAL is defined but never used; either remove
the assignment to SCRIPT_DIR_REAL or use it where appropriate (for example
reference SCRIPT_DIR_REAL in a debug/log echo to show the script invocation
directory or replace other dirname "${BASH_SOURCE[0]}" uses with
SCRIPT_DIR_REAL). Locate the assignment to SCRIPT_DIR_REAL and either delete
that line or wire the variable into an existing logging/debug statement so the
value is actually read (ensuring no other logic relies on a different variable
like SCRIPT_DIR).
In `@deploy/setup.sh`:
- Line 428: The current computation of default_tty using
default_tty="tty.${CONTROL_DOMAIN#*.}" (and the similar admin default
admin@${CONTROL_DOMAIN#*.}) misbehaves when CONTROL_DOMAIN has no dot because
${CONTROL_DOMAIN#*.} yields the whole string; update the logic to detect whether
CONTROL_DOMAIN contains a dot (e.g., test with a shell pattern) and only strip
the left-most label when a dot exists, otherwise fall back to a safer value
(empty suffix or a sensible default) before constructing default_tty and the
admin email; adjust the assignments where default_tty and the admin email
default are set (referencing default_tty, CONTROL_DOMAIN, and the admin email
default occurrence) so single-label CONTROL_DOMAIN produces a graceful fallback
instead of embedding the whole hostname.
In `@internal/api/system_actions.go`:
- Around line 83-112: The StartReconciliation loop currently uses
context.WithTimeout(ctx, sweepTimeout) without checking sweepTimeout, so a
non‑positive sweepTimeout creates an already-canceled context and prevents
SyncAllUsersSystemActions from running; update StartReconciliation to validate
and clamp sweepTimeout (e.g. if sweepTimeout <= 0 set it to a sensible default
or minimum like time.Second or a configured fallback) before starting the
goroutine so that context.WithTimeout always receives a positive duration, and
keep all existing uses of running, ticker and SyncAllUsersSystemActions
unchanged.
In `@internal/store/store.go`:
- Around line 75-82: fireListeners currently calls OnEventAppended and each
entry in listeners synchronously so any panic in a listener bubbles up and can
fail AppendEvent/AppendEventWithVersion; change fireListeners to invoke the
legacy OnEventAppended and each listener in separate goroutines
(fire-and-forget) and wrap each goroutine's call in a defer/recover to catch and
log panics without propagating them, passing the same ctx and row values (avoid
mutating row) so post-commit notifications are non-blocking and isolated per the
EventListener design.
---
Nitpick comments:
In `@cmd/control/main.go`:
- Around line 780-784: Clamp user-provided system-action reconcile flags in
parseFlags: if cfg.SystemActionReconcileTimeout <= 0 set it to a sane minimum
(e.g. the same fallback used by the listener or a small positive duration) and
if cfg.SystemActionReconcileInterval <= 0 treat it as disabled consistently (or
clamp to a minimum >0 if you prefer enabling); this prevents passing a
zero/negative sweepTimeout into SystemActionManager.StartReconciliation (which
calls context.WithTimeout and produces an immediately-canceled context) and
matches the bounding behavior used for DynamicGroupEvalInterval.
In `@deploy/install.sh`:
- Line 103: The trap currently uses double quotes so $tmpdir is expanded
immediately; change the trap invocation to use single quotes for the body (trap
'rm -rf "$tmpdir"' EXIT) so the tmpdir variable is expanded at trap execution
time; update the trap call where tmpdir is set and referenced (the trap line
referencing tmpdir) to use single-quoted body to satisfy Shellcheck SC2064 and
ensure correct cleanup behavior.
- Line 228: Update the echo that prints the logs command to remove the
path-qualified compose file; replace the current message that references "docker
compose -f $INSTALL_DIR/compose.yml logs -f" with a simplified "docker compose
logs -f" since the script already cds into INSTALL_DIR and docker compose will
auto-detect compose.yml (reference the echo line that prints the Logs command
and the INSTALL_DIR/compose.yml filename).
In `@internal/api/system_actions_listener.go`:
- Around line 56-73: The handler currently returns SyncOpNone when
userIDFromEventData(e) is empty for "UserRoleAssigned"/"UserRoleRevoked"; add a
defensive fallback that parses the event StreamID (which is "user_id:role_id")
to extract the user id using strings.Cut(e.StreamID, ":") and, if that yields a
non-empty uid, return SyncOpSyncUser, []string{uid} instead of SyncOpNone; keep
the group-member cases unchanged (no fallback) and reference the case labels,
userIDFromEventData, e.StreamID, and SyncOpSyncUser when making the change.
- Around line 150-193: SystemActionListener currently spawns an unbounded
goroutine per event (via the anon goroutine in SystemActionListener after
AffectedFromEvent), which can exhaust DB/signing resources for many
SyncUserSystemActions or run multiple SyncAllUsersSystemActions concurrently;
change it to use a bounded concurrency semaphore/worker pool (e.g., a buffered
channel or errgroup.SetLimit) to limit concurrent goroutines and apply
backpressure (log+drop when pool is full), and reuse the reconciler's running
flag (atomic.Bool) for the SyncOpSyncAll path so SyncAllUsersSystemActions
coalesces (CompareAndSwap to avoid starting another sweep and ensure the flag is
cleared after the sweep completes); ensure the semaphore slot is always released
and the syncAll flag is cleared in all return paths so no leaks occur.
- Around line 154-162: The goroutine currently uses context.Background() which
drops request-scoped values (like RequestID); rename the ignored parameter "_"
to "ctx" in the outer handler func signature and replace context.Background()
with context.WithoutCancel(ctx) when creating the goroutine's ctx (still use
context.WithTimeout and defer cancel as before) so the async work is detached
from cancellation but preserves request values for logging in functions like
AffectedFromEvent and the subsequent sync error logs.
In `@internal/store/store.go`:
- Around line 35-39: Update the EventListener doc comment to state the ctx
lifetime contract: note that the ctx passed to EventListener is the caller's
request context and will be canceled as soon as AppendEvent returns, so
listeners must not rely on it for long-running work or downstream DB calls; for
background work (e.g. in SystemActionListener which spawns goroutines) create a
new context (context.Background() or a derived context with its own
cancellation) and avoid mutating the PersistedEvent.
🪄 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: e87c706c-fd81-4c46-89d9-8cc5042acb4f
📒 Files selected for processing (20)
cmd/control/main.gocmd/gateway/main.godeploy/.env.exampledeploy/QUICKSTART.mddeploy/install.shdeploy/setup.shdeploy/setup_test.shinternal/api/errors.gointernal/api/system_actions.gointernal/api/system_actions_listener.gointernal/api/system_actions_listener_test.gointernal/api/terminal_handler.gointernal/api/terminal_handler_test.gointernal/api/user_handler.gointernal/config/config.gointernal/config/config_test.gointernal/scim/handler.gointernal/scim/handler_test.gointernal/scim/users.gointernal/store/store.go
CodeRabbit's full review caught a critical test-harness bug, two major
findings, and several smaller ones. Bundling all fixes here so the next
review round only has to cover the remaining concurrency design.
Critical
--------
- deploy/setup_test.sh: the run_case wrapper used `( ... ); if [[ $? -eq 0 ]]`
under `set -euo pipefail`. Bash's set-e kills the parent on the
subshell's non-zero exit *before* $? is read, so the suite would have
bailed mid-run on the first real failure and FAIL_COUNT / the summary
line were unreachable. Switched to `if (...); then` (the idiomatic
shell pattern that suspends set-e for the conditional). Added a
synthetic always-failing case + tally adjustment so the FAIL path is
exercised on every run — proves the harness behaves as documented.
Major
-----
- internal/store/store.go: fireListeners invoked listeners synchronously
with no recover(). A panic in any listener (or the legacy
OnEventAppended hook) propagated up through AppendEvent and failed
the RPC even though the event was already committed — broke the
"event committed → RPC succeeds" contract the CQRS layer relies on.
Wrapped each invocation in a per-listener defer/recover that logs to
stderr; documented the ctx-lifetime + panic-isolation contract on
EventListener.
- deploy/QUICKSTART.md: the documented form
`RELEASE_TAG=v2026.05 curl … | sudo bash` does NOT pass the variable
through. sudo's env_reset strips the var from the environment of the
process it spawns, so the prefix only scoped to `curl` (which doesn't
read RELEASE_TAG). Operators copy-pasting silently got `latest-rc`
with no error. Rewrote all four examples to the working form
`curl … | sudo VAR=… bash` and added a one-line note explaining
why the placement matters.
Minor
-----
- deploy/install.sh: tar `--wildcards '*/deploy/*'` exits 0 on empty
match (e.g. a future tag that renames deploy/). Added post-extract
sanity check on setup.sh + .env.example — fails loudly instead of
letting the operator chase a confusing "setup.sh: command not found"
later. Also fixed trap quoting (SC2064) so $tmpdir expands at
trap-fire time.
- deploy/setup.sh: write_env_var/clear_env_var used `mktemp` (defaults
to $TMPDIR), so `mv` fell back to non-atomic copy-then-unlink when
/tmp is a separate mount, *and* overwrote .env's mode/owner with the
temp file's. Switched to `mktemp "${envfile}.XXXXXX"` for a same-fs
rename(2), and copy mode via `chmod --reference="$envfile"` so .env
stays 0600 across re-runs.
- internal/api/system_actions.go: StartReconciliation passed
sweepTimeout straight to context.WithTimeout. A 0/negative timeout
produced an already-cancelled context every tick — the durability
safety net silently failed every interval. Added an in-function
clamp that falls back to interval-as-ceiling, defending in depth on
top of the parseFlags clamp.
- cmd/control/main.go: clamp SystemActionReconcileTimeout / Interval
in parseFlags (mirroring DynamicGroupEvalInterval). Negative interval
→ disabled; non-positive timeout → 5m default.
Listener concurrency (rc11 #77 round 3)
---------------------------------------
- internal/api/system_actions_listener.go:
- Switched goroutine ctx from context.Background() to
context.WithoutCancel(parent) so request-id and other request-
scoped values flow into "sync user/all failed" error logs and
correlate back to the triggering RPC.
- Bounded the in-flight goroutine count via a 16-slot semaphore.
Without this, a SCIM bulk role assignment emitting thousands of
UserRoleAssigned events would have spawned thousands of concurrent
SyncUserSystemActions calls, saturating the pgx pool / signer.
Over-cap events are dropped + logged; the periodic reconciler
catches up within one interval, which is the right back-pressure
trade-off.
- Added an atomic.Bool to coalesce SyncOpSyncAll dispatches. Multiple
consecutive RoleUpdated / ServerSettingUpdated events used to stack
N concurrent O(all-users) sweeps stepping on each other; now
subsequent fan-outs return early while one is in flight, matching
the reconciler's tick-coalesce pattern.
- Added a defensive StreamID-prefix fallback for UserRoleAssigned/
UserRoleRevoked when event.data["user_id"] is missing (the stream
id is `user_id:role_id`, so strings.Cut gives the same answer at
no cost). Group-membership cases keep no fallback because the
stream id is the group, not the user.
Tests
-----
- deploy/setup_test.sh: 7 cases now (6 real + 1 synthetic FAIL).
- internal/api/system_actions_listener_test.go: replaced the
"missing user_id → SyncOpNone" assertion (no longer holds) with
three cases covering the new fallback semantics (data-missing →
StreamID prefix; no colon in StreamID → SyncOpNone; group-member
with missing data still → SyncOpNone).
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
internal/api/terminal_handler_test.go (1)
388-390: Optional: assert on the structured error code rather than message substring.Both tests assert the operator-facing message text via
assert.Contains(connectErr.Error(), ...), which couples them to the exact wording. The handler attaches a structuredpm.ErrorDetail{Code: ...}viaapiErrorCtx, so the persistent case can assertCode == ErrGatewayNotRegisteredand the transient caseCode == ErrDeviceNotConnecteddirectly — that's the actual contract the SDK/web client switches on, and it survives copy edits to the human-readable message. The substring check is still useful as a secondary assertion (the env-var name is the actionable hint), but the code assertion should be primary.♻️ Sketch of structured-detail assertion
// Pull the first ErrorDetail and unmarshal into pm.ErrorDetail. require.NotEmpty(t, connectErr.Details()) var detail pm.ErrorDetail _, err = connectErr.Details()[0].Value(&detail) require.NoError(t, err) assert.Equal(t, api.ErrGatewayNotRegistered, detail.Code)Also applies to: 432-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/terminal_handler_test.go` around lines 388 - 390, The test should assert the structured error code from the gRPC Connect error instead of only checking the human message: pull connectErr.Details(), require it's non-empty, unmarshal the first detail into a pm.ErrorDetail and assert detail.Code equals the expected constant (use api.ErrGatewayNotRegistered for the persistent-misconfig case and api.ErrDeviceNotConnected for the transient case), and optionally keep the existing assert.Contains(connectErr.Error(), "GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE") as a secondary, actionable hint check; locate these changes around the existing connectErr assertions in the test (symbols: connectErr, pm.ErrorDetail, api.ErrGatewayNotRegistered, api.ErrDeviceNotConnected).internal/scim/handler_test.go (1)
36-36: No test exercises the SCIM-delete cleanup path.Every test passes
nilhere, so the rc11#77fix — "SCIM was previously bypassing the system-actions cleanup entirely on user deletion" — has no regression test. A trivial fake implementingSystemActionsCleanerwould letTestDeleteUser_Success(line 302) assert thatCleanupDeletedUserActionsis actually invoked with the loaded user projection when the last identity link is removed, and conversely that it is not invoked when other links remain. That's the exact regression the PR is meant to prevent.🔧 Sketch
type fakeCleaner struct { calls []db.UsersProjection } func (f *fakeCleaner) CleanupDeletedUserActions(_ context.Context, u db.UsersProjection) error { f.calls = append(f.calls, u) return nil } // In a new test: setupSCIM wires &fakeCleaner{}, exercise DELETE on a single-link user, // assert len(fake.calls) == 1 and fake.calls[0].ID == userID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scim/handler_test.go` at line 36, Tests never exercise the SCIM delete cleanup path because NewHandler is constructed with nil systemActions; add a fake SystemActionsCleaner to the SCIM handler tests and assert its CleanupDeletedUserActions is called only when the last identity link is removed. Implement a minimal fake type that records calls and inject it into scim.NewHandler in TestDeleteUser_Success (and add a complementary case where multiple links remain), then after performing the DELETE request assert the fake's calls length and that the recorded db.UsersProjection.ID equals the deleted user ID; ensure the fake implements CleanupDeletedUserActions(ctx context.Context, u db.UsersProjection) error and returns nil so it can be used in setupSCIM wiring.deploy/setup_test.sh (1)
47-75: Inlined helpers diverge fromsetup.sh— chmod preservation is silently untested.The comment at lines 44–46 claims these tests "would catch a divergence as a FAIL anyway," but the inlined
write_env_var/clear_env_varhere drop thechmod --reference="$envfile" "$tmp" 2>/dev/null || chmod 600 "$tmp"lines that the canonicaldeploy/setup.shversions (lines 322–338, 345–360) use to preserve the.envfile's permissions across the atomic-replace. That permission preservation is exactly the kind of subtle regression a smoke test should pin: a future edit tosetup.shthat breaks the chmod path would still pass this harness.Either copy the chmod lines into the inlined helpers and add an assertion that the resulting
.envmode survives the round-trip, orsourcesetup.shwith a guard to skipmainso the test exercises the real implementation. The inline-and-hope-they-stay-in-sync approach is a maintenance footgun worth flagging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/setup_test.sh` around lines 47 - 75, The inlined helpers write_env_var and clear_env_var drop the chmod preservation used in deploy/setup.sh so add the missing atomic-replace permission-preservation logic (i.e., after writing to the temp file run the same chmod --reference="$envfile" "$tmp" 2>/dev/null || chmod 600 "$tmp" before mv "$tmp" "$envfile") and add an assertion in the test that the .env file mode is preserved across the round-trip; alternatively replace the inline helpers by sourcing deploy/setup.sh with a guard to avoid running main so the test exercises the real write_env_var and clear_env_var implementations instead of duplicating them.deploy/setup.sh (2)
362-378: Secret prompt echoes typed input — considerread -sfor the No-path.When the operator answers "n" to the auto-generate prompt, the manually typed secret (Postgres password, JWT secret, encryption key, admin password) is echoed to the terminal. On a shared/recorded session this leaks the freshly-typed secret. The auto-generate path doesn't have this issue because the value never traverses stdin.
🔒 Suggested change
else - read -r -p " Enter value: " REPLY_VALUE + read -r -s -p " Enter value: " REPLY_VALUE + echo fi(
echoafterread -sadds the newline that-ssuppresses.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/setup.sh` around lines 362 - 378, The prompt_secret function currently uses read -r to accept manual secret input which echoes the secret; change the "no" branch so it uses a silent read (read -s) to set REPLY_VALUE when the user opts to enter a value, then echo a newline after the silent read to restore prompt formatting; keep the existing placeholder check (is_placeholder), generation path (eval "$gen_cmd") and assignments to REPLY_VALUE unchanged, and ensure the prompt text remains the same so behavior for generation vs manual entry is preserved.
540-556: Unknown flags are silently ignored.The
casehas no fall-through to error on unrecognized arguments, so./setup.sh --noprompt(typo) silently runs guided mode. Minor robustness gap.📝 Suggested fix
case "$arg" in --no-prompt) NO_PROMPT=1 ;; -h|--help) ... ;; + *) + log_error "Unknown argument: $arg" + log_error " See: $0 --help" + exit 2 + ;; esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/setup.sh` around lines 540 - 556, The script currently iterates over arguments (for arg in "$@") and branches via the case statement but ignores unknown flags; update the case in setup.sh to add a default branch (e.g., *) that prints a clear error mentioning the unrecognized argument ($arg), shows usage or hint, and exits non-zero; reference the existing NO_PROMPT variable and the --no-prompt / -h|--help patterns so the new default handler rejects typos like --noprompt and prevents silent acceptance.internal/api/system_actions_listener_test.go (2)
236-246: Considerslices.Equalfrom the standard library.Go 1.21+ ships
slices.Equalwhich is functionally identical toequalStringSlices. Replacing the helper trims the test file slightly. Purely cosmetic; ignore if you prefer the explicit helper for readability.-import ( - "encoding/json" - "testing" +import ( + "encoding/json" + "slices" + "testing" - if !equalStringSlices(gotUsers, tc.wantUsers) { + if !slices.Equal(gotUsers, tc.wantUsers) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener_test.go` around lines 236 - 246, Replace the custom equalStringSlices helper with the standard library helper slices.Equal: remove the equalStringSlices function and call slices.Equal(a, b) where it was used; add the "slices" import to the test file (Go 1.21+) so the code compiles, and ensure any references to equalStringSlices are updated to slices.Equal.
24-225: Solid coverage; consider adding the remaining per-user/fan-out event types as a regression net.The 17 cases cover all four
SyncOpoutcomes plus the StreamID-fallback edge cases foruser_roleand the no-fallback case foruser_group. Several event types inAffectedFromEvent's switch (e.g.UserEnabled,UserProvisioningSettingsUpdated,UserSshSettingsUpdated,UserProfileUpdated,UserSshKeyRemoved,UserEmailChanged,UserGroupMemberRemoved,UserGroupRoleRevoked,UserGroupQueryUpdated) share a switch case with one already tested, so they're covered transitively today — but a future refactor that promotes one of them to a separate case (or accidentally drops one from the case list) wouldn't trip a test.Optional: a tiny table-driven loop that asserts each event-type literal individually would lock that contract down without much extra code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener_test.go` around lines 24 - 225, TestAffectedFromEvent lacks explicit single-event tests for many event-type literals handled by AffectedFromEvent, risking regressions if cases are refactored; add a small table-driven subtest inside TestAffectedFromEvent that iterates over the remaining event-type strings (e.g. "UserEnabled","UserProvisioningSettingsUpdated","UserSshSettingsUpdated","UserProfileUpdated","UserSshKeyRemoved","UserEmailChanged","UserGroupMemberRemoved","UserGroupRoleRevoked","UserGroupQueryUpdated", etc.) and for each constructs a minimal store.PersistedEvent and asserts AffectedFromEvent returns the expected SyncOp (SyncOpSyncUser, SyncOpSyncAll or SyncOpNone) and expected users where applicable; reference the existing AffectedFromEvent call and SyncOp constants in the new loop so the test locks the mapping of event-type literals to SyncOp outcomes.internal/store/store.go (1)
36-83: Listener API is well-documented; the "not safe for concurrent registration" contract is fragile though.The documented lifecycle (register at boot, then start serving) works for the current single-call-site wiring in
cmd/control/main.go, but it's an implicit invariant the Go race detector won't catch if a future caller registers afterAppendEventis in flight (concurrent slice append + range read is a data race). Two practical options if you want to harden this without changing the boot path:
- Lock
listenerswith async.RWMutex(RLock aroundrangeinfireListeners, Lock around append inRegisterEventListener) — cheap, well-trodden.- Or rename to
RegisterEventListenerAtBoot/ panic if called after a sentinel "started" flag flips, to make misuse loud.Not blocking — current call sites obey the contract. Flagging because the field is exported-by-implication via a public method, so a future package adopter could trip it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/store.go` around lines 36 - 83, The RegisterEventListener/listeners slice is racy if callers append while fireListeners ranges the slice; add a sync.RWMutex to Store (e.g., listenersMu) and use listenersMu.Lock() when appending in RegisterEventListener and listenersMu.RLock() around the iteration in fireListeners (and any other readers) to prevent concurrent append/range races; alternatively, if you prefer a fail-fast approach, add a started bool on Store and have RegisterEventListener panic if started is true (flip started when AppendEvent begins) — implement the locking approach unless you intentionally want the stricter panic-on-late-register behavior.deploy/QUICKSTART.md (1)
6-6: Casing inconsistency for the GitHub org slug.This file uses
MANCHTOOLS(uppercase) on lines 6 and 62, whiledeploy/install.shline 28 usesmanchtools(lowercase). GitHub treats org slugs case-insensitively for HTTP fetches, so both work, but pinning canonical case avoids subtle issues with mirrors/proxies, redirect caches, and anchor-checking link checkers. Pick one (the lowercase form matches the slug stored inGITHUB_REPOand PR objectives) and use it everywhere.📝 Suggested doc fix
-curl -fsSL https://raw.githubusercontent.com/MANCHTOOLS/power-manage-server/main/deploy/install.sh | sudo bash +curl -fsSL https://raw.githubusercontent.com/manchtools/power-manage-server/main/deploy/install.sh | sudo bash-git clone https://github.com/MANCHTOOLS/power-manage-server.git +git clone https://github.com/manchtools/power-manage-server.gitAlso applies to: 62-62, 239-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/QUICKSTART.md` at line 6, Replace the uppercase GitHub org slug "MANCHTOOLS" with the lowercase "manchtools" in deploy/QUICKSTART.md where the curl/install command and other references use the org slug (e.g., the curl command string and the other occurrences called out in the comment); ensure consistency with deploy/install.sh which uses "manchtools" and with the GITHUB_REPO canonical slug so all instances in QUICKSTART.md exactly match the lowercase form.deploy/install.sh (1)
215-221: Sourcing.envunderset -euo pipefailis mildly fragile.
set -uis enabled byset -euo pipefailat line 23, so any line in.envthat references an unset variable (e.g.,KEY=${OTHER:?}orKEY=$LITERAL_DOLLAR_SIGNtypos) will abort the script. This is unlikely with the values setup.sh writes, but if an operator hand-edits.envand introduces a stray$it becomes hard to diagnose. Optional: temporarily relaxset +uaround the source, thenset -uagain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/install.sh` around lines 215 - 221, In print_summary(), avoid aborts from undefined vars when sourcing ./.env under set -euo pipefail by temporarily disabling "nounset": run set +u before you set -a and source ./.env, then restore strict nounset with set -u after you unset -a; i.e., wrap the source ./.env between set +u / set -u while keeping the export behavior via set -a so sourcing won't fail on accidental $VAR references.cmd/control/main.go (1)
514-528:OnEventAppendedis reassigned afterRegisterEventListener— be aware of the ordering implication.
fireListenersinvokesOnEventAppendedfirst and then the registered listeners. Between line 381 (RegisterEventListenerfor system actions) and line 516 (assignment ofOnEventAppendedfor search indexing), any committed events fire only the system-action listener; events emitted before line 381 fire neither (caught by the startup sweep). This is the current intended boot behavior, but if future code starts emitting events between lines 381–516 that need search-indexing on first occurrence, it'll silently miss them.Optional refactor: move the search-indexer wiring into a
RegisterEventListenercall too (instead of writing to the publicOnEventAppendedfield) and place it next to the system-action registration so the order is explicit and concurrent-safe by construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/control/main.go` around lines 514 - 528, OnEventAppended is being reassigned after RegisterEventListener which creates an ordering gap where events emitted between the system-action listener registration and the reassignment won't be indexed; change the search-index wiring to register as a listener via st.RegisterEventListener instead of assigning st.OnEventAppended, move that RegisterEventListener call next to the system-action registration so ordering is explicit, and inside the listener call searchIdx.EnqueueReindex with the same payload (search.ScopeAuditEvent, id, taskqueue.SearchEntityData with EventType/StreamType/ActorType/ActorID/StreamID/OccurredAt) and remove the direct assignment to OnEventAppended to avoid the race/ordering issue.internal/api/system_actions_listener.go (1)
178-184: Remove unusedst *store.Storeparameter fromSystemActionListener.The
stargument is never referenced in the function body—onlymgr,logger, andsyncTimeoutare used. The caller passesstpurely for syntactic convenience when registering the listener, which is awkward and risks confusion.Proposed change
Update the function signature in
internal/api/system_actions_listener.go:-func SystemActionListener(mgr *SystemActionManager, st *store.Store, logger *slog.Logger, syncTimeout time.Duration) store.EventListener { +func SystemActionListener(mgr *SystemActionManager, logger *slog.Logger, syncTimeout time.Duration) store.EventListener {Update the doc comment example:
-// st.RegisterEventListener(api.SystemActionListener(svc.SystemActions(), st, logger, cfg.SystemActionReconcileTimeout)) +// st.RegisterEventListener(api.SystemActionListener(svc.SystemActions(), logger, cfg.SystemActionReconcileTimeout))Update the call site in
cmd/control/main.go:381:st.RegisterEventListener(api.SystemActionListener( svc.SystemActions(), - st, logger.With("component", "system_action_listener"), cfg.SystemActionReconcileTimeout, ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener.go` around lines 178 - 184, Remove the unused parameter st *store.Store from the SystemActionListener function signature and all references to it: update the function declaration for SystemActionListener(mgr *SystemActionManager, logger *slog.Logger, syncTimeout time.Duration) (removing st), adjust the doc comment/example that shows calling SystemActionListener to match the new signature, and update the call site (e.g., where SystemActionListener is invoked in cmd/control/main.go) to stop passing the store argument so the call matches the new parameter list; ensure compile by rebuilding and fixing any related import or variable name discrepancies caused by the signature change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/install.sh`:
- Around line 5-6: The install flow pipes install.sh into a non-TTY bash so the
child script ./setup.sh inherits the pipe as stdin and cannot present
interactive prompts (setup.sh uses NO_PROMPT and check_env), causing premature
failure; modify the install.sh invocation of ./setup.sh (the call around the
current ./setup.sh launch) to redirect stdin from /dev/tty when available (e.g.
test for /dev/tty or that a tty exists and run "./setup.sh </dev/tty") so
setup.sh’s prompt loop reads from the controlling terminal while preserving
non-interactive behavior when no tty is present.
---
Nitpick comments:
In `@cmd/control/main.go`:
- Around line 514-528: OnEventAppended is being reassigned after
RegisterEventListener which creates an ordering gap where events emitted between
the system-action listener registration and the reassignment won't be indexed;
change the search-index wiring to register as a listener via
st.RegisterEventListener instead of assigning st.OnEventAppended, move that
RegisterEventListener call next to the system-action registration so ordering is
explicit, and inside the listener call searchIdx.EnqueueReindex with the same
payload (search.ScopeAuditEvent, id, taskqueue.SearchEntityData with
EventType/StreamType/ActorType/ActorID/StreamID/OccurredAt) and remove the
direct assignment to OnEventAppended to avoid the race/ordering issue.
In `@deploy/install.sh`:
- Around line 215-221: In print_summary(), avoid aborts from undefined vars when
sourcing ./.env under set -euo pipefail by temporarily disabling "nounset": run
set +u before you set -a and source ./.env, then restore strict nounset with set
-u after you unset -a; i.e., wrap the source ./.env between set +u / set -u
while keeping the export behavior via set -a so sourcing won't fail on
accidental $VAR references.
In `@deploy/QUICKSTART.md`:
- Line 6: Replace the uppercase GitHub org slug "MANCHTOOLS" with the lowercase
"manchtools" in deploy/QUICKSTART.md where the curl/install command and other
references use the org slug (e.g., the curl command string and the other
occurrences called out in the comment); ensure consistency with
deploy/install.sh which uses "manchtools" and with the GITHUB_REPO canonical
slug so all instances in QUICKSTART.md exactly match the lowercase form.
In `@deploy/setup_test.sh`:
- Around line 47-75: The inlined helpers write_env_var and clear_env_var drop
the chmod preservation used in deploy/setup.sh so add the missing atomic-replace
permission-preservation logic (i.e., after writing to the temp file run the same
chmod --reference="$envfile" "$tmp" 2>/dev/null || chmod 600 "$tmp" before mv
"$tmp" "$envfile") and add an assertion in the test that the .env file mode is
preserved across the round-trip; alternatively replace the inline helpers by
sourcing deploy/setup.sh with a guard to avoid running main so the test
exercises the real write_env_var and clear_env_var implementations instead of
duplicating them.
In `@deploy/setup.sh`:
- Around line 362-378: The prompt_secret function currently uses read -r to
accept manual secret input which echoes the secret; change the "no" branch so it
uses a silent read (read -s) to set REPLY_VALUE when the user opts to enter a
value, then echo a newline after the silent read to restore prompt formatting;
keep the existing placeholder check (is_placeholder), generation path (eval
"$gen_cmd") and assignments to REPLY_VALUE unchanged, and ensure the prompt text
remains the same so behavior for generation vs manual entry is preserved.
- Around line 540-556: The script currently iterates over arguments (for arg in
"$@") and branches via the case statement but ignores unknown flags; update the
case in setup.sh to add a default branch (e.g., *) that prints a clear error
mentioning the unrecognized argument ($arg), shows usage or hint, and exits
non-zero; reference the existing NO_PROMPT variable and the --no-prompt /
-h|--help patterns so the new default handler rejects typos like --noprompt and
prevents silent acceptance.
In `@internal/api/system_actions_listener_test.go`:
- Around line 236-246: Replace the custom equalStringSlices helper with the
standard library helper slices.Equal: remove the equalStringSlices function and
call slices.Equal(a, b) where it was used; add the "slices" import to the test
file (Go 1.21+) so the code compiles, and ensure any references to
equalStringSlices are updated to slices.Equal.
- Around line 24-225: TestAffectedFromEvent lacks explicit single-event tests
for many event-type literals handled by AffectedFromEvent, risking regressions
if cases are refactored; add a small table-driven subtest inside
TestAffectedFromEvent that iterates over the remaining event-type strings (e.g.
"UserEnabled","UserProvisioningSettingsUpdated","UserSshSettingsUpdated","UserProfileUpdated","UserSshKeyRemoved","UserEmailChanged","UserGroupMemberRemoved","UserGroupRoleRevoked","UserGroupQueryUpdated",
etc.) and for each constructs a minimal store.PersistedEvent and asserts
AffectedFromEvent returns the expected SyncOp (SyncOpSyncUser, SyncOpSyncAll or
SyncOpNone) and expected users where applicable; reference the existing
AffectedFromEvent call and SyncOp constants in the new loop so the test locks
the mapping of event-type literals to SyncOp outcomes.
In `@internal/api/system_actions_listener.go`:
- Around line 178-184: Remove the unused parameter st *store.Store from the
SystemActionListener function signature and all references to it: update the
function declaration for SystemActionListener(mgr *SystemActionManager, logger
*slog.Logger, syncTimeout time.Duration) (removing st), adjust the doc
comment/example that shows calling SystemActionListener to match the new
signature, and update the call site (e.g., where SystemActionListener is invoked
in cmd/control/main.go) to stop passing the store argument so the call matches
the new parameter list; ensure compile by rebuilding and fixing any related
import or variable name discrepancies caused by the signature change.
In `@internal/api/terminal_handler_test.go`:
- Around line 388-390: The test should assert the structured error code from the
gRPC Connect error instead of only checking the human message: pull
connectErr.Details(), require it's non-empty, unmarshal the first detail into a
pm.ErrorDetail and assert detail.Code equals the expected constant (use
api.ErrGatewayNotRegistered for the persistent-misconfig case and
api.ErrDeviceNotConnected for the transient case), and optionally keep the
existing assert.Contains(connectErr.Error(),
"GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE") as a secondary, actionable hint check;
locate these changes around the existing connectErr assertions in the test
(symbols: connectErr, pm.ErrorDetail, api.ErrGatewayNotRegistered,
api.ErrDeviceNotConnected).
In `@internal/scim/handler_test.go`:
- Line 36: Tests never exercise the SCIM delete cleanup path because NewHandler
is constructed with nil systemActions; add a fake SystemActionsCleaner to the
SCIM handler tests and assert its CleanupDeletedUserActions is called only when
the last identity link is removed. Implement a minimal fake type that records
calls and inject it into scim.NewHandler in TestDeleteUser_Success (and add a
complementary case where multiple links remain), then after performing the
DELETE request assert the fake's calls length and that the recorded
db.UsersProjection.ID equals the deleted user ID; ensure the fake implements
CleanupDeletedUserActions(ctx context.Context, u db.UsersProjection) error and
returns nil so it can be used in setupSCIM wiring.
In `@internal/store/store.go`:
- Around line 36-83: The RegisterEventListener/listeners slice is racy if
callers append while fireListeners ranges the slice; add a sync.RWMutex to Store
(e.g., listenersMu) and use listenersMu.Lock() when appending in
RegisterEventListener and listenersMu.RLock() around the iteration in
fireListeners (and any other readers) to prevent concurrent append/range races;
alternatively, if you prefer a fail-fast approach, add a started bool on Store
and have RegisterEventListener panic if started is true (flip started when
AppendEvent begins) — implement the locking approach unless you intentionally
want the stricter panic-on-late-register behavior.
🪄 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: aad4f995-3118-494e-bd7c-9e84102eff1f
📒 Files selected for processing (20)
cmd/control/main.gocmd/gateway/main.godeploy/.env.exampledeploy/QUICKSTART.mddeploy/install.shdeploy/setup.shdeploy/setup_test.shinternal/api/errors.gointernal/api/system_actions.gointernal/api/system_actions_listener.gointernal/api/system_actions_listener_test.gointernal/api/terminal_handler.gointernal/api/terminal_handler_test.gointernal/api/user_handler.gointernal/config/config.gointernal/config/config_test.gointernal/scim/handler.gointernal/scim/handler_test.gointernal/scim/users.gointernal/store/store.go
CodeRabbit's second pass after round-3 caught one real bug + a dozen nitpicks worth taking. Applied all of them here. Major ----- - deploy/install.sh: when the installer is invoked via the documented `curl -fsSL .../install.sh | sudo bash` flow, install.sh's stdin is the pipe — and so is setup.sh's inherited stdin. setup.sh's `[[ "$NO_PROMPT" -eq 0 && -t 0 ]]` correctly auto-falls-back to non-interactive mode, but with a fresh .env (just copied from .env.example), check_env then aborts on the CHANGE_ME placeholders with a confusing "POSTGRES_PASSWORD must be set" message. Net effect: the QUICKSTART one-liner could not actually reach guided setup. Added a `</dev/tty` redirect when /dev/tty is readable so the prompt loop reads from the controlling terminal even when install.sh was piped. CI / true non-interactive contexts (no /dev/tty) still go through the existing fallback. Minor ----- - deploy/install.sh: print_summary's `source ./.env` ran under `set -u`, so a typo'd `$VAR` reference in .env would abort the installer's success path. Wrapped the source with `set +u` / `set -u` while keeping `set -a` for export. - deploy/setup.sh prompt_secret: manual-entry No-branch echoed the typed secret to the terminal — minor leak risk on shared/recorded sessions. Switched to `read -r -s` + explicit newline. - deploy/setup.sh arg parsing: `--noprompt` (typo) silently ran guided mode. Added a `*)` default branch that prints the unknown flag and exits 2. - internal/store/store.go: documented "register at boot, then serve" contract relied on call-site discipline; Go's race detector cannot catch a future concurrent registration. Added a sync.RWMutex around listeners + OnEventAppended. fireListeners snapshots under RLock and dispatches outside the mutex so slow listeners don't block readers. - cmd/control/main.go: the search-indexer wired into st.OnEventAppended via direct field assignment AFTER the system-action RegisterEventListener call. Migrated to a second RegisterEventListener call so both consumers share the same wiring, the same mutex, and the same panic-recovery wrapper. Eliminates the ordering-gap concern between the two registrations. - internal/api/system_actions_listener.go: removed unused `st *store.Store` parameter from SystemActionListener (only mgr, logger, syncTimeout are used). Updated the doc-comment example and the cmd/control/main.go call site. - deploy/setup_test.sh: the inlined helpers had drifted from setup.sh's canonical implementation — they dropped the `chmod --reference="$envfile"` line that preserves .env's 0600 mode across rewrites. The "tests would FAIL on divergence" claim was therefore not true for this specific regression class. Synced the inlined helpers back to setup.sh and added two new cases (case_write_env_var_preserves_mode_0600, case_clear_env_var_preserves_mode_0600) that fail if a future edit drops the chmod call. - deploy/QUICKSTART.md: lowercased the GitHub org slug to match install.sh's canonical `manchtools` (was inconsistent uppercase MANCHTOOLS in two places). Tests ----- - internal/api/terminal_handler_test.go: replaced message-substring assertions with structured Code assertions via connectErr.Details()[0].Value().(*pm.ErrorDetail). The Code is the actual contract the SDK + web client switch on — substring assertions stay as a secondary check that the operator hint is present. Added the errorCode helper. - internal/api/system_actions_listener_test.go: replaced equalStringSlices with stdlib slices.Equal. Added TestAffectedFromEvent_PerLiteral that asserts every event-type literal handled by AffectedFromEvent's switch maps to the expected SyncOp. Locks the per-literal contract so a future refactor that drops one from a shared case is observable. - internal/scim/handler_test.go: rc11 #77's SCIM-side cleanup wiring had no regression test (every existing test passed nil for systemActions). Added fakeSystemActionsCleaner + setupSCIMWithCleaner variant + TestDeleteUser_LastLink_TriggersSystemActionsCleanup that exercises the DELETE → CleanupDeletedUserActions(loaded projection) path end-to-end. Asserts call count == 1 and the projection ID matches the deleted user — the exact regression the PR is meant to prevent. Verification ------------ - go build ./... clean across server / sdk / agent - go vet ./... clean - go test -race ./internal/api/ -run TestAffectedFromEvent : pass - go test ./internal/api/ -run 'TestAffectedFromEvent|TestStartTerminal_GatewayNotRegistered' : 30+ subcases pass - go test ./internal/scim/ -run TestDeleteUser : both cases pass - bash deploy/setup_test.sh : 8 real + 1 synthetic; all passing
Closes the last CodeRabbit nit from PR #83 that I'd previously deferred as non-blocking. Adds a parent_domain helper and uses it for the TTY domain default and the admin email default so single-label hostnames (`localhost`, `controlplane`) don't yield self-referential or odd defaults. - parent_domain "control.example.com" → "example.com" - parent_domain "localhost" → "" (single label, no parent) - parent_domain "a.b.c.example.com" → "b.c.example.com" For default_tty: empty when CONTROL_DOMAIN is single-label, forcing the operator to type something meaningful instead of accepting `tty.localhost` (which would never resolve and would just shadow the control host on a single-label local-test setup). For admin email: `admin@<parent>` for dotted domains, `admin@<full>` for single-label cases — `admin@localhost` is a valid local-delivery address so we keep something usable rather than blanking the field. Bash test coverage: three new cases pin the helper's three branches (dotted / single-label / deep subdomain). Setup test suite is now 11 real cases + 1 synthetic.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
internal/api/terminal_handler_test.go (1)
456-457: Optional: the"retry"positive-substring assertion is brittle.The negative assertion on
GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE(lines 454–455) is high-signal — it directly guards the regression that motivated the split. The positive"retry"check, however, couples the test to operator-facing copy that the helper comment at lines 26–27 explicitly says is "free to evolve with copy edits". A reasonable rewording like "please try again later" or "temporarily unavailable" would fail this assertion without any behavior change.Since
connect.CodeUnavailablealready encodes the retry semantics for clients, andErrDeviceNotConnectedcarries the differentiation, consider dropping the positive substring or loosening it to a small alternation.♻️ Suggested loosening
- assert.Contains(t, connectErr.Error(), "retry", - "transient path message should hint at retry") + msg := strings.ToLower(connectErr.Error()) + assert.True(t, + strings.Contains(msg, "retry") || strings.Contains(msg, "try again") || strings.Contains(msg, "temporarily"), + "transient path message should hint at retry semantics, got: %q", connectErr.Error())Or simply drop it —
CodeUnavailable+ErrDeviceNotConnected+ the negative assertion already lock the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/terminal_handler_test.go` around lines 456 - 457, The test's positive substring assertion assert.Contains(t, connectErr.Error(), "retry") is brittle; remove that assertion (or replace it with a small alternation if you prefer) and rely on the existing negative check against GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE together with connect.CodeUnavailable and ErrDeviceNotConnected to enforce the contract — locate the check in terminal_handler_test.go that asserts connectErr.Error() contains "retry" and delete or relax it so the test no longer depends on operator-facing copy.cmd/control/main.go (1)
849-860: Consider mirroring theDynamicGroupEvalIntervalmin/max clamp pattern.
SystemActionReconcileIntervalis only clamped against negative values. A misconfiguredCONTROL_SYSTEM_ACTION_RECONCILE_INTERVAL=1mswould create a ticker firing every millisecond, repeatedly invokingSyncAllUsersSystemActionsand likely overwhelming the DB (the atomic-bool guard inStartReconciliationwould skip overlapping sweeps but still produce a flood of warning logs every tick). The neighbouringDynamicGroupEvalIntervalblock at lines 841-847 enforces a sensible floor (30m) and ceiling (8h). A floor (e.g., 10s) on the reconcile interval would similarly make the operator-facing knob safe-by-default.Not blocking — the default of 1 minute is reasonable and operators are unlikely to set this to a pathological value, but worth considering for parity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/control/main.go` around lines 849 - 860, Mirror the DynamicGroupEvalInterval clamp for system actions: in the same init/config section where cfg.SystemActionReconcileInterval and cfg.SystemActionReconcileTimeout are set, enforce a sensible minimum and maximum for cfg.SystemActionReconcileInterval (e.g., clamp to a floor of 10s and a ceiling of 8h similar to DynamicGroupEvalInterval) instead of only disallowing negatives; keep the current fallback for SystemActionReconcileTimeout but ensure StartReconciliation/SyncAllUsersSystemActions behavior remains unchanged (the atomic-bool guard still handles overlaps).internal/scim/handler_test.go (1)
48-52:lastCall()will panic on empty calls slice.
f.calls[len(f.calls)-1]indexes[-1]if no call has been recorded, producing a panic instead of a clean test failure. The current test assertscallCount == 1viarequire.Equalbefore invokinglastCall(), so it's safe today, but a future test that forgets the guard will fail with a panic stack trace rather than a readable assertion error.♻️ Optional hardening
func (f *fakeSystemActionsCleaner) lastCall() db.UsersProjection { f.mu.Lock() defer f.mu.Unlock() + if len(f.calls) == 0 { + return db.UsersProjection{} + } return f.calls[len(f.calls)-1] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scim/handler_test.go` around lines 48 - 52, The helper fakeSystemActionsCleaner.lastCall currently panics when f.calls is empty by indexing f.calls[len(f.calls)-1]; change lastCall to guard against empty slice (e.g., if len(f.calls) == 0 { return db.UsersProjection{} }) so it returns a safe zero-value instead of panicking, or alternatively return an (value, bool) or error; update any callers/tests accordingly (references: fakeSystemActionsCleaner.lastCall and f.calls).internal/api/system_actions_listener_test.go (1)
234-321: Per-literal test is a good guard against accidental case-list regressions.The literal-by-literal sweep catches the failure mode where a future refactor consolidates the switch and silently drops one literal from a shared case (which the structured
TestAffectedFromEventwould miss because it only samples one literal per branch). One minor observation: there's no case here for malformed JSON inevent.Data(e.g.,[]byte("{invalid")), which would exercise thejson.Unmarshalerror path inuserIDFromEventDataatsystem_actions_listener.golines 117-120 — currently untested. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener_test.go` around lines 234 - 321, Add a unit test that sends a malformed JSON payload to AffectedFromEvent to exercise the json.Unmarshal error path in userIDFromEventData: create a PersistedEvent for a data-driven user event (e.g., EventType "UserRoleAssigned", StreamType "user_role", StreamID "user-y:role-z") with Data set to invalid bytes like []byte("{invalid") and assert the returned op is SyncOpSyncUser and the users fallback to ["user-y"]; place this inside TestAffectedFromEvent_PerLiteral near the other per-data cases so it explicitly covers the error branch in userIDFromEventData and prevents regressions.deploy/setup_test.sh (1)
44-88: Consider source-guardingsetup.shso the harness can test the real helpers.The inlined copies of
write_env_var,clear_env_var, andparent_domain(lines 50-88) duplicate the production implementations atdeploy/setup.sh:319-326,338-354,361-374. The comment above acknowledges the drift risk, and round-3 review already caught one such drift on thechmod --referencecall. As long as the test inlines, a regression insetup.shthat's not mirrored into the harness will pass green.Since we already gate execution with
if [[ "$NO_PROMPT" -eq 0 && -t 0 ]]etc., wrapping the bottom ofsetup.shin a "main only when executed, not when sourced" sentinel would letsetup_test.shsource the real functions and delete the inline copies.♻️ Sketch
In
deploy/setup.sh(bottom of file):-main "$@" +# Only run main when executed directly; allow sourcing for tests. +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + main "$@" +fiIn
deploy/setup_test.shrun_casesubshell:- # Inline the helper definitions we need to test. Must stay in - # sync with setup.sh — including the same-fs mktemp + - # chmod-reference dance that preserves .env's mode across - # rewrites. ... - write_env_var() { ... } - clear_env_var() { ... } - parent_domain() { ... } + # Source the real helpers so a regression in setup.sh shows + # up here instead of being masked by an out-of-date inline copy. + # shellcheck disable=SC1091 + source "$(dirname "${BASH_SOURCE[0]}")/setup.sh"Note: the top-level
for arg in "$@"parser atsetup.sh:583-606would also need to be moved into a function or guarded the same way so sourcing doesn't reject the test harness's own argv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/setup_test.sh` around lines 44 - 88, The test harness currently inlines write_env_var, clear_env_var, and parent_domain, risking silent drift; instead add a "main-only when executed" sentinel to deploy/setup.sh (so its helpers are defined on source) and move or guard the top-level argv parsing (the for arg in "$@" parser) behind the same sentinel or into a function, then remove the duplicated helper definitions from deploy/setup_test.sh and have it source deploy/setup.sh (keeping the existing NO_PROMPT/tTY checks intact) so the tests exercise the real implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/setup.sh`:
- Around line 560-576: The final summary block prints a stale ADMIN_EMAIL and
reprints stored admin passwords on reruns; fix by (1) after calling
write_env_var for ADMIN_EMAIL (in the prompt_string/ADMIN_EMAIL sequence) update
the shell variable from the prompt result (e.g., set ADMIN_EMAIL="$REPLY_VALUE"
or otherwise mirror what you did for CONTROL_DOMAIN/GATEWAY_DOMAIN) so the
displayed "Email:" shows the value just written, and (2) change how
prompt_secret is used so the caller knows whether the password was newly
generated/entered this run (add/return a flag like REPLY_GENERATED or have
prompt_secret set a sentinel variable) and only show the "write this down NOW"
banner and echo the password when that flag is true; leave admin_pass populated
for other logic but suppress printing when the password was from the existing
.env (use is_placeholder only to detect placeholder, not to infer "just
created").
In `@internal/api/system_actions_listener.go`:
- Around line 212-244: The spawned goroutine that calls
mgr.SyncUserSystemActions and mgr.SyncAllUsersSystemActions lacks panic
recovery, so wrap the goroutine body with a top-level defer that recovers
panics, logs the panic and stack trace via the same logger used in this file,
and prevents the panic from propagating; ensure the existing deferred cleanup
(reading from sem and, when op == SyncOpSyncAll, syncAllInFlight.Store(false))
still runs even if a panic occurs by placing the recover defer before or
combined with those defers so both the recovery log and resource-release actions
execute; update the anonymous goroutine that handles cases SyncOpSyncUser,
SyncOpCleanupUser, and SyncOpSyncAll to include this defensive recover logic
around calls to mgr.SyncUserSystemActions and mgr.SyncAllUsersSystemActions.
- Around line 58-82: The comment is out-of-date and group-member handlers can
see composite StreamID values, so add the same defensive fallback used in the
role handlers: inside the "UserGroupMemberAdded"/"UserGroupMemberRemoved"
branch, after calling userIDFromEventData(e) and seeing an empty uid, attempt to
split e.StreamID with strings.Cut(e.StreamID, ":") and use the left part as the
user id if present; then return SyncOpSyncUser with that uid (otherwise keep
returning SyncOpNone). Update the branch that handles these events and reference
userIDFromEventData, e.StreamID, and the SyncOpSyncUser/SyncOpNone returns.
In `@internal/store/store.go`:
- Around line 112-138: fireListeners dispatches listeners synchronously which
lets a slow listener (the audit indexing listener that calls
searchIdx.EnqueueReindex in cmd/control/main.go) block AppendEvent and RPCs; fix
by making the audit/indexing listener non-blocking at its registration site:
wrap the call to searchIdx.EnqueueReindex inside a goroutine (spawned from the
listener registered with Store.RegisterEventListener / OnEventAppended) and make
it best-effort (catch/recover panics and log failures) so fireListeners and
functions like OnEventAppended remain synchronous but cheap; reference
searchIdx.EnqueueReindex, the listener registration in cmd/control/main.go, and
fireListeners in internal/store/store.go when making this change.
---
Nitpick comments:
In `@cmd/control/main.go`:
- Around line 849-860: Mirror the DynamicGroupEvalInterval clamp for system
actions: in the same init/config section where cfg.SystemActionReconcileInterval
and cfg.SystemActionReconcileTimeout are set, enforce a sensible minimum and
maximum for cfg.SystemActionReconcileInterval (e.g., clamp to a floor of 10s and
a ceiling of 8h similar to DynamicGroupEvalInterval) instead of only disallowing
negatives; keep the current fallback for SystemActionReconcileTimeout but ensure
StartReconciliation/SyncAllUsersSystemActions behavior remains unchanged (the
atomic-bool guard still handles overlaps).
In `@deploy/setup_test.sh`:
- Around line 44-88: The test harness currently inlines write_env_var,
clear_env_var, and parent_domain, risking silent drift; instead add a "main-only
when executed" sentinel to deploy/setup.sh (so its helpers are defined on
source) and move or guard the top-level argv parsing (the for arg in "$@"
parser) behind the same sentinel or into a function, then remove the duplicated
helper definitions from deploy/setup_test.sh and have it source deploy/setup.sh
(keeping the existing NO_PROMPT/tTY checks intact) so the tests exercise the
real implementations.
In `@internal/api/system_actions_listener_test.go`:
- Around line 234-321: Add a unit test that sends a malformed JSON payload to
AffectedFromEvent to exercise the json.Unmarshal error path in
userIDFromEventData: create a PersistedEvent for a data-driven user event (e.g.,
EventType "UserRoleAssigned", StreamType "user_role", StreamID "user-y:role-z")
with Data set to invalid bytes like []byte("{invalid") and assert the returned
op is SyncOpSyncUser and the users fallback to ["user-y"]; place this inside
TestAffectedFromEvent_PerLiteral near the other per-data cases so it explicitly
covers the error branch in userIDFromEventData and prevents regressions.
In `@internal/api/terminal_handler_test.go`:
- Around line 456-457: The test's positive substring assertion
assert.Contains(t, connectErr.Error(), "retry") is brittle; remove that
assertion (or replace it with a small alternation if you prefer) and rely on the
existing negative check against GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE together
with connect.CodeUnavailable and ErrDeviceNotConnected to enforce the contract —
locate the check in terminal_handler_test.go that asserts connectErr.Error()
contains "retry" and delete or relax it so the test no longer depends on
operator-facing copy.
In `@internal/scim/handler_test.go`:
- Around line 48-52: The helper fakeSystemActionsCleaner.lastCall currently
panics when f.calls is empty by indexing f.calls[len(f.calls)-1]; change
lastCall to guard against empty slice (e.g., if len(f.calls) == 0 { return
db.UsersProjection{} }) so it returns a safe zero-value instead of panicking, or
alternatively return an (value, bool) or error; update any callers/tests
accordingly (references: fakeSystemActionsCleaner.lastCall and f.calls).
🪄 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: 2132a632-675d-496e-b407-9e1451c4162d
📒 Files selected for processing (10)
cmd/control/main.godeploy/QUICKSTART.mddeploy/install.shdeploy/setup.shdeploy/setup_test.shinternal/api/system_actions_listener.gointernal/api/system_actions_listener_test.gointernal/api/terminal_handler_test.gointernal/scim/handler_test.gointernal/store/store.go
✅ Files skipped from review due to trivial changes (2)
- deploy/QUICKSTART.md
- deploy/install.sh
CodeRabbit's incremental review on the round-4 commits caught a real
admin-credential leak, two listener correctness gaps, and a handful of
worthwhile robustness improvements. Implemented all 4 actionable + 5
nitpick findings.
Major
-----
- deploy/setup.sh: the final summary block had two display bugs.
1. ADMIN_EMAIL was sourced from .env BEFORE the prompt ran and
never re-read, so on a fresh install the "Email:" line printed
empty next to the freshly chosen password. Fix: mirror
ADMIN_EMAIL="$REPLY_VALUE" after write_env_var, matching what
CONTROL_DOMAIN/GATEWAY_DOMAIN already do.
2. The "write this down NOW; not shown again" banner gated on
is_placeholder($admin_pass), which is false when the existing
password was simply kept verbatim — so EVERY rerun re-printed
the stored bootstrap password to stdout / install logs / scroll
buffer. Fix: prompt_secret now sets REPLY_GENERATED=1 only when
the operator generates or types a value this run; the banner
gates on that flag instead of inferring intent from the value.
- internal/api/system_actions_listener.go: comment claimed "group
membership uses the group ID as StreamID, so user_id is data-only,"
but SCIM, the API user-group handler, and testutil all emit
composite "groupID:userID" — only IDP uses bare groupID. A future
emitter that drops data.user_id while keeping the composite form
would silently SyncOpNone for up to one reconcile interval. Added
the symmetric StreamID-suffix fallback (taking the part AFTER the
colon, not before like the role case takes the part BEFORE).
- internal/api/system_actions_listener.go: dispatch goroutine had no
panic recovery. fireListeners' defer/recover only protects the
listener function, which returned the moment we spawned the
goroutine — a panic in mgr.SyncUserSystemActions or
SyncAllUsersSystemActions (nil deref, library bug, etc.) would
have crashed the entire control server. Added a top-level recover
that logs the panic with event metadata and lets the sem release +
syncAllInFlight reset still run.
Minor
-----
- internal/store/store.go is dispatched synchronously, so a slow
listener body extends every state-changing RPC's tail latency.
cmd/control/main.go's audit-index listener was the worst case
because EnqueueReindex hits Valkey on every AppendEvent. Wrapped
the EnqueueReindex call in a goroutine at the registration site
(with its own recover) so a Valkey RTT spike can't stall RPCs. The
taskqueue client has its own per-call timeouts, and the work is
already best-effort, so detaching is safe.
- cmd/control/main.go: clamped SystemActionReconcileInterval to
[10s, 8h] when non-zero (mirroring the DynamicGroupEvalInterval
pattern). Without the floor, a misconfigured 1ms tick would have
produced a flood of ticker events; the StartReconciliation atomic
guard would skip overlapping sweeps but still log a warning every
millisecond.
Tests
-----
- internal/api/terminal_handler_test.go: dropped the brittle
`assert.Contains(connectErr.Error(), "retry")` substring assertion
on the transient-loss case. CodeUnavailable + ErrDeviceNotConnected
+ the negative substring against GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE
already lock the contract — the positive copy match was just a
hostage to operator-message edits.
- internal/scim/handler_test.go: lastCall() guards against an empty
slice, returning the zero value so a future test that forgets to
assert callCount first fails with a readable diff instead of a
panic stack trace.
- internal/api/system_actions_listener_test.go: added three new cases:
* group-member event with composite StreamID + missing data.user_id
→ SyncOpSyncUser (covers the new fallback)
* UserGroupMemberRemoved twin
* UserRoleAssigned with malformed JSON in Data → falls back to
StreamID prefix (exercises the json.Unmarshal error path in
userIDFromEventData that was previously untested)
Architecture
------------
- deploy/setup.sh: wrapped argv parsing in a parse_flags function and
the bottom-of-file invocation in `if [[ "${BASH_SOURCE[0]}" ==
"${0}" ]]; then ... fi`. setup.sh is now safely sourceable — its
helpers load without running main or rejecting the harness's argv.
- deploy/setup_test.sh: deleted ALL inlined helper copies. The
harness now sources setup.sh and exercises the real bodies, so a
future regression in setup.sh (the chmod-preservation drift caught
in round 3, e.g.) shows up as a test failure here instead of being
masked by an out-of-date inline copy.
Order of source vs. SCRIPT_DIR override matters: the override has
to come AFTER the source, because setup.sh sets SCRIPT_DIR at the
top to its own install dir. The pre-source override I'd written
first was silently overwritten and the helpers wrote into the real
deploy/.env — tests still went green because the helpers worked
against the wrong file. Added case_isolation_writes_in_tmpdir as a
guard so this can't regress silently again.
Verification
------------
- go build ./... clean
- go vet ./... clean
- go test -race ./internal/api/ -run AffectedFromEvent : pass (no races)
- go test ./internal/api/ -run 'TestAffectedFromEvent|TestStartTerminal_GatewayNotRegistered' : 30+ subcases pass
- go test ./internal/scim/ -run TestDeleteUser : both pass
- bash deploy/setup_test.sh : 12 real + 1 synthetic, all passing
- ./deploy/setup.sh --help still works (source-guard regression check)
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/api/system_actions_listener.go (1)
64-66:⚠️ Potential issue | 🟡 MinorStale comment: role case is no longer the only place the fallback is meaningful.
The comment claims "The role case is the only place this fallback is meaningful — group membership uses the group ID as StreamID, so user_id is data-only," but the very next case (lines 75-93) now has a symmetric
StreamIDfallback for composite"groupID:userID"emitters (added per a previous review round). Update or drop the parenthetical so the rationale matches what the group branch actually does.📝 Suggested wording
- // up to one reconcile interval." The role case is the only - // place this fallback is meaningful — group membership uses - // the group ID as StreamID, so user_id is data-only. + // up to one reconcile interval." The group-member case below + // applies the same defensive pattern for its + // "groupID:userID" composite emitters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener.go` around lines 64 - 66, The comment referencing "The role case is the only place this fallback is meaningful" is stale because the group branch now also falls back to StreamID for composite "groupID:userID" emitters; update the parenthetical in the comment (near the StreamID fallback logic and the group membership branch handling composite emitters) to reflect that the fallback is meaningful for both role and composite group:user emitters, or remove the parenthetical entirely so the rationale matches the actual StreamID fallback behavior.
🧹 Nitpick comments (6)
cmd/control/main.go (1)
866-885: Considera advertir cuandointerval<sweepTimeoutya están sobreescritos por el clamp.El clamp es defensivo y correcto, pero silencia errores de configuración. Si un operador escribe
CONTROL_SYSTEM_ACTION_RECONCILE_INTERVAL=1msesperando un comportamiento agresivo, ahora recibirá 10s sin saberlo. Unslog.Warnaquí (similar al patrón deenvBool/envDurationarriba) ayudaría a depurar configuraciones mal interpretadas:♻️ Refactor sugerido
if cfg.SystemActionReconcileInterval < 0 { + slog.Warn("CONTROL_SYSTEM_ACTION_RECONCILE_INTERVAL was negative, treating as disabled (0)", + "value", cfg.SystemActionReconcileInterval) cfg.SystemActionReconcileInterval = 0 // treat as disabled, matching StartReconciliation } else if cfg.SystemActionReconcileInterval > 0 { if cfg.SystemActionReconcileInterval < 10*time.Second { + slog.Warn("CONTROL_SYSTEM_ACTION_RECONCILE_INTERVAL clamped up to 10s minimum", + "value", cfg.SystemActionReconcileInterval) cfg.SystemActionReconcileInterval = 10 * time.Second } else if cfg.SystemActionReconcileInterval > 8*time.Hour { + slog.Warn("CONTROL_SYSTEM_ACTION_RECONCILE_INTERVAL clamped down to 8h maximum", + "value", cfg.SystemActionReconcileInterval) cfg.SystemActionReconcileInterval = 8 * time.Hour } } if cfg.SystemActionReconcileTimeout <= 0 { + slog.Warn("CONTROL_SYSTEM_ACTION_RECONCILE_TIMEOUT was non-positive, defaulting to 5m", + "value", cfg.SystemActionReconcileTimeout) cfg.SystemActionReconcileTimeout = 5 * time.Minute }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/control/main.go` around lines 866 - 885, Detect when the configured SystemActionReconcileInterval was clamped or when the (possibly-clamped) SystemActionReconcileInterval is less than SystemActionReconcileTimeout and emit a warning via slog.Warn so operators know their values were adjusted; specifically, capture the original interval parsed (before modifying cfg.SystemActionReconcileInterval), perform the existing clamp logic, then if original != cfg.SystemActionReconcileInterval or cfg.SystemActionReconcileInterval < cfg.SystemActionReconcileTimeout call slog.Warn with a clear message referencing SystemActionReconcileInterval and SystemActionReconcileTimeout (same file’s cfg variables and StartReconciliation semantics) so the log explains the original value and the applied clamped value.deploy/setup.sh (2)
484-494: Reescritura silenciosa deGATEWAY_PUBLIC_TERMINAL_URL_TEMPLATEyGATEWAY_WEB_LISTEN_ADDRen re-ejecuciones.Cuando el operador re-ejecuta
setup.sh, responde Sí a "Enable remote terminal sessions?" y mantiene unGATEWAY_TTY_DOMAINya configurado, las líneas 488 y 493 sobrescriben incondicionalmente la plantilla de URL y la dirección del listener — incluso si el operador había personalizado esos valores manualmente en.env(puerto distinto, esquema, prefijo de ruta). El comportamiento deprompt_stringal "ya configurado, manteniendo" no aplica aquí porque estos dos valores nunca se preguntan.No es bloqueante (el bloque de comentarios señala que la composición es intencional), pero sería más amigable comprobar si ya están establecidos con un valor no-placeholder antes de reescribirlos:
♻️ Refactor sugerido
- write_env_var GATEWAY_TTY_DOMAIN "$REPLY_VALUE" - local tty_dom="$REPLY_VALUE" - - # Auto-compose the URL template. Operator never types {id}. - write_env_var GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE "wss://${tty_dom}/gw/{id}/terminal" - echo " ✓ GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE composed automatically." - - # The TTY HTTP listener inside the container — Traefik - # terminates public TLS and forwards cleartext. - write_env_var GATEWAY_WEB_LISTEN_ADDR ":8443" - echo " ✓ GATEWAY_WEB_LISTEN_ADDR set to :8443." + write_env_var GATEWAY_TTY_DOMAIN "$REPLY_VALUE" + local tty_dom="$REPLY_VALUE" + + # Auto-compose the URL template only if the operator hasn't + # already customized it. Operator never types {id} from scratch. + if is_placeholder "${GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE:-}"; then + write_env_var GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE "wss://${tty_dom}/gw/{id}/terminal" + echo " ✓ GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE composed automatically." + else + echo " ✓ GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE already set, keeping." + fi + + # The TTY HTTP listener inside the container — Traefik + # terminates public TLS and forwards cleartext. + if [[ -z "${GATEWAY_WEB_LISTEN_ADDR:-}" ]]; then + write_env_var GATEWAY_WEB_LISTEN_ADDR ":8443" + echo " ✓ GATEWAY_WEB_LISTEN_ADDR set to :8443." + else + echo " ✓ GATEWAY_WEB_LISTEN_ADDR already set, keeping." + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/setup.sh` around lines 484 - 494, Actualmente el script siempre reescribe GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE y GATEWAY_WEB_LISTEN_ADDR usando write_env_var tras leer GATEWAY_TTY_DOMAIN; cambia esto para que antes de llamar a write_env_var compruebes el valor existente de esas dos variables (leer desde el .env o la función/variable que almacene valores actuales) y sólo compongas/reescribas si están vacías o coinciden con un patrón/placeholder auto-generado; en caso contrario, preserva el valor personalizado del operador. Refiérete a GATEWAY_TTY_DOMAIN, GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE, GATEWAY_WEB_LISTEN_ADDR y la invocación write_env_var en el bloque donde actualmente se hace la composición para implementar la comprobación y la rama de no-sobrescritura.
543-548: Validación deJWT_SECRETabortasetup.shpara valores existentes legítimos pero cortos.
prompt_secretretorna el valor actual sin cambios cuando ya está establecido y no es un placeholder, y luego la línea 544 valida la longitud. Si un operador heredó unJWT_SECRETde una versión anterior que no imponía 32 caracteres mínimos, la re-ejecución desetup.shaborta con "must be at least 32 characters" sin opción de regenerar — esto es la primera vez que el operador ve la nueva regla.Considera ofrecer regeneración interactiva en lugar de abortar (similar al patrón de
prompt_secret):♻️ Refactor sugerido
- prompt_secret "JWT secret (JWT_SECRET, min 32 chars)" "openssl rand -base64 48" "${JWT_SECRET:-}" - if [[ ${`#REPLY_VALUE`} -lt 32 ]]; then - log_error "JWT_SECRET must be at least 32 characters; got ${`#REPLY_VALUE`}. Aborting." - exit 1 - fi - write_env_var JWT_SECRET "$REPLY_VALUE" + prompt_secret "JWT secret (JWT_SECRET, min 32 chars)" "openssl rand -base64 48" "${JWT_SECRET:-}" + if [[ ${`#REPLY_VALUE`} -lt 32 ]]; then + log_warn "Existing JWT_SECRET is ${`#REPLY_VALUE`} chars (< 32). The control server will refuse to start." + if prompt_yes_no "Regenerate JWT_SECRET now?"; then + REPLY_VALUE="$(openssl rand -base64 48)" + REPLY_GENERATED=1 + else + log_error "JWT_SECRET must be at least 32 characters; aborting." + exit 1 + fi + fi + write_env_var JWT_SECRET "$REPLY_VALUE"El mismo patrón aplica a
INDEXER_POSTGRES_PASSWORD(líneas 533-537) yCONTROL_ENCRYPTION_KEY(líneas 552-555), aunque la probabilidad de heredar valores inválidos en esos dos es menor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/setup.sh` around lines 543 - 548, The current length check for JWT_SECRET aborts setup when an existing (short) secret is present; change the flow to offer interactive regeneration instead: after calling prompt_secret and getting REPLY_VALUE for JWT_SECRET, if ${`#REPLY_VALUE`} < 32 then prompt the user with a clear choice to regenerate a new secret (suggest using "openssl rand -base64 48") or keep the existing value; if they choose regenerate, set REPLY_VALUE to the newly generated value and re-validate (loop until length >= 32 or the user explicitly accepts a short secret), and only abort if the user cancels; apply the same interactive regeneration pattern to INDEXER_POSTGRES_PASSWORD and CONTROL_ENCRYPTION_KEY, using their respective default generators and write_env_var to persist the final chosen value.deploy/QUICKSTART.md (1)
6-6: Inconsistencia de mayúsculas/minúsculas en la URL de GitHub respecto adeploy/install.sh.
deploy/install.shlínea 5 usaMANCHTOOLS/power-manage-server(mayúsculas) mientras que aquí (línea 6) y enGITHUB_REPOdentro deinstall.sh(línea 28) se usamanchtools/power-manage-server(minúsculas). Aunque GitHub normaliza el caso del propietario en la mayoría de los redireccionamientos HTTP, tener tres formas distintas en la documentación de instalación añade fricción al diagnóstico cuando un proxy corporativo o un mirror agresivo respeta el caso. Sugiero unificar a la forma canónica del repositorio (la que usa la PR:manchtools).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/QUICKSTART.md` at line 6, Unifica la capitalización del nombre del repositorio a la forma canónica "manchtools/power-manage-server": replace the URL string in QUICKSTART.md ("https://raw.githubusercontent.com/manchtools/power-manage-server/main/deploy/install.sh") so it uses all-lowercase "manchtools", and also ensure the GITHUB_REPO variable inside deploy/install.sh and any other occurrences (e.g., the `deploy/install.sh` reference and the curl command in QUICKSTART.md) use the same lowercase form to avoid inconsistent casing across docs and scripts.deploy/install.sh (1)
169-177: Mensaje engañoso cuando se ejecuta conNO_PROMPT=1y.envya existe.
init_envimprime "guided setup will only prompt for missing values" incluso cuando luegorun_setupse invoca con--no-prompt, en cuyo caso no hay prompts. No es funcionalmente incorrecto, pero confunde en el flujo CI / re-runs no interactivos. Podrías condicionar el mensaje aNO_PROMPT.♻️ Refactor sugerido
init_env() { cd "$INSTALL_DIR" if [[ ! -f .env ]]; then cp .env.example .env - log_info "Created .env from .env.example — guided setup will fill it in." + if [[ "$NO_PROMPT" == "1" ]]; then + log_info "Created .env from .env.example — fill in real values before re-running with NO_PROMPT=1." + else + log_info "Created .env from .env.example — guided setup will fill it in." + fi else - log_info ".env already exists — guided setup will only prompt for missing values." + if [[ "$NO_PROMPT" == "1" ]]; then + log_info ".env already exists — validating directly (NO_PROMPT=1)." + else + log_info ".env already exists — guided setup will only prompt for missing values." + fi fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/install.sh` around lines 169 - 177, The log message in init_env currently always says "guided setup will only prompt for missing values" even when NO_PROMPT=1 is used (which causes run_setup to run with --no-prompt); update init_env to detect the NO_PROMPT environment variable (or equivalent flag) and emit a different message when non-interactive: if NO_PROMPT is set/true, log something like ".env already exists — guided setup will run non-interactively and not prompt" otherwise keep the existing message about prompting for missing values; reference the init_env function and the NO_PROMPT flag so the message aligns with how run_setup is invoked.internal/api/system_actions_listener.go (1)
246-253:SyncOpSyncUserloop shares one timeout across alluserIDs.
AffectedFromEventcurrently returns at most one user ID forSyncOpSyncUser, so this is not a live bug. But the API permits a slice, and if a future classifier case yields multiple IDs, a singlesyncTimeoutcovering the entire loop means later users can be silently skipped (ctx.Err() == context.DeadlineExceeded) once the budget is consumed by earlier ones — exactly the kind of bounded-drift case the listener API is supposed to surface (or hand to the reconciler). Consider either documenting the "one ID per event" contract on the return type, or scoping the deadline per-user.♻️ Optional per-user scoping
case SyncOpSyncUser: for _, uid := range userIDs { - if err := mgr.SyncUserSystemActions(ctx, uid); err != nil { + uctx, ucancel := context.WithTimeout(context.WithoutCancel(parent), syncTimeout) + if err := mgr.SyncUserSystemActions(uctx, uid); err != nil { logger.Error("system-action listener: sync user failed", "user_id", uid, "event_type", e.EventType, "event_id", e.ID, "error", err) } + ucancel() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/system_actions_listener.go` around lines 246 - 253, The SyncOpSyncUser loop uses a shared timeout for all userIDs which can let one slow user exhaust the deadline and silently skip others; inside the case for SyncOpSyncUser, scope the deadline per-user by creating a new context with context.WithTimeout(ctx, syncTimeout) for each uid, call mgr.SyncUserSystemActions with that per-user ctx, and ensure you call cancel() (or defer cancel in a short-lived closure) each iteration; alternatively, if you intend to keep a single-ID invariant, add a clear comment on the contract for userIDs returned by AffectedFromEvent and validate/assert len(userIDs) <= 1 in the SyncOpSyncUser branch (referencing SyncOpSyncUser, userIDs, syncTimeout, and mgr.SyncUserSystemActions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/setup_test.sh`:
- Around line 226-234: Replace the heuristic that assumes a single failure is
the synthetic meta-test by explicitly tracking the meta-test outcome (e.g., set
META_FAIL=1 or 0 based on the result of case_meta_failure) and compute
REAL_FAILS=$((FAIL_COUNT - META_FAIL)); ensure REAL_FAILS is not negative (clamp
to 0), update the summary echo to show the meta-test status and real fail count,
and keep the final exit condition as [[ $REAL_FAILS -eq 0 ]] so the script only
reports success when no real tests failed.
In `@internal/api/terminal_handler.go`:
- Around line 287-294: The branch that checks
h.registry.LookupGatewayInternalURL and immediately returns
ErrGatewayNotRegistered when the internal URL exists but the terminal URL is
missing can misclassify transient key-skew; update the logic inside that branch
to perform a short dampened retry before returning the
persistent-misconfiguration error: when internalErr == nil and terminal URL
missing, retry a few times (e.g., 2–3 attempts) with small sleeps/backoff and
re-check the terminal key via the same registry lookup path (the code around
h.registry.LookupGatewayInternalURL in terminal_handler.go), and only after
retries still fail return the apiErrorCtx(ErrGatewayNotRegistered, ...); this
keeps the rest of the handler unchanged but avoids false positives from
independent goroutine ticker drift.
---
Duplicate comments:
In `@internal/api/system_actions_listener.go`:
- Around line 64-66: The comment referencing "The role case is the only place
this fallback is meaningful" is stale because the group branch now also falls
back to StreamID for composite "groupID:userID" emitters; update the
parenthetical in the comment (near the StreamID fallback logic and the group
membership branch handling composite emitters) to reflect that the fallback is
meaningful for both role and composite group:user emitters, or remove the
parenthetical entirely so the rationale matches the actual StreamID fallback
behavior.
---
Nitpick comments:
In `@cmd/control/main.go`:
- Around line 866-885: Detect when the configured SystemActionReconcileInterval
was clamped or when the (possibly-clamped) SystemActionReconcileInterval is less
than SystemActionReconcileTimeout and emit a warning via slog.Warn so operators
know their values were adjusted; specifically, capture the original interval
parsed (before modifying cfg.SystemActionReconcileInterval), perform the
existing clamp logic, then if original != cfg.SystemActionReconcileInterval or
cfg.SystemActionReconcileInterval < cfg.SystemActionReconcileTimeout call
slog.Warn with a clear message referencing SystemActionReconcileInterval and
SystemActionReconcileTimeout (same file’s cfg variables and StartReconciliation
semantics) so the log explains the original value and the applied clamped value.
In `@deploy/install.sh`:
- Around line 169-177: The log message in init_env currently always says "guided
setup will only prompt for missing values" even when NO_PROMPT=1 is used (which
causes run_setup to run with --no-prompt); update init_env to detect the
NO_PROMPT environment variable (or equivalent flag) and emit a different message
when non-interactive: if NO_PROMPT is set/true, log something like ".env already
exists — guided setup will run non-interactively and not prompt" otherwise keep
the existing message about prompting for missing values; reference the init_env
function and the NO_PROMPT flag so the message aligns with how run_setup is
invoked.
In `@deploy/QUICKSTART.md`:
- Line 6: Unifica la capitalización del nombre del repositorio a la forma
canónica "manchtools/power-manage-server": replace the URL string in
QUICKSTART.md
("https://raw.githubusercontent.com/manchtools/power-manage-server/main/deploy/install.sh")
so it uses all-lowercase "manchtools", and also ensure the GITHUB_REPO variable
inside deploy/install.sh and any other occurrences (e.g., the
`deploy/install.sh` reference and the curl command in QUICKSTART.md) use the
same lowercase form to avoid inconsistent casing across docs and scripts.
In `@deploy/setup.sh`:
- Around line 484-494: Actualmente el script siempre reescribe
GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE y GATEWAY_WEB_LISTEN_ADDR usando
write_env_var tras leer GATEWAY_TTY_DOMAIN; cambia esto para que antes de llamar
a write_env_var compruebes el valor existente de esas dos variables (leer desde
el .env o la función/variable que almacene valores actuales) y sólo
compongas/reescribas si están vacías o coinciden con un patrón/placeholder
auto-generado; en caso contrario, preserva el valor personalizado del operador.
Refiérete a GATEWAY_TTY_DOMAIN, GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE,
GATEWAY_WEB_LISTEN_ADDR y la invocación write_env_var en el bloque donde
actualmente se hace la composición para implementar la comprobación y la rama de
no-sobrescritura.
- Around line 543-548: The current length check for JWT_SECRET aborts setup when
an existing (short) secret is present; change the flow to offer interactive
regeneration instead: after calling prompt_secret and getting REPLY_VALUE for
JWT_SECRET, if ${`#REPLY_VALUE`} < 32 then prompt the user with a clear choice to
regenerate a new secret (suggest using "openssl rand -base64 48") or keep the
existing value; if they choose regenerate, set REPLY_VALUE to the newly
generated value and re-validate (loop until length >= 32 or the user explicitly
accepts a short secret), and only abort if the user cancels; apply the same
interactive regeneration pattern to INDEXER_POSTGRES_PASSWORD and
CONTROL_ENCRYPTION_KEY, using their respective default generators and
write_env_var to persist the final chosen value.
In `@internal/api/system_actions_listener.go`:
- Around line 246-253: The SyncOpSyncUser loop uses a shared timeout for all
userIDs which can let one slow user exhaust the deadline and silently skip
others; inside the case for SyncOpSyncUser, scope the deadline per-user by
creating a new context with context.WithTimeout(ctx, syncTimeout) for each uid,
call mgr.SyncUserSystemActions with that per-user ctx, and ensure you call
cancel() (or defer cancel in a short-lived closure) each iteration;
alternatively, if you intend to keep a single-ID invariant, add a clear comment
on the contract for userIDs returned by AffectedFromEvent and validate/assert
len(userIDs) <= 1 in the SyncOpSyncUser branch (referencing SyncOpSyncUser,
userIDs, syncTimeout, and mgr.SyncUserSystemActions).
🪄 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: ecf634a0-774d-403c-9713-fd893e82d6c3
📒 Files selected for processing (20)
cmd/control/main.gocmd/gateway/main.godeploy/.env.exampledeploy/QUICKSTART.mddeploy/install.shdeploy/setup.shdeploy/setup_test.shinternal/api/errors.gointernal/api/system_actions.gointernal/api/system_actions_listener.gointernal/api/system_actions_listener_test.gointernal/api/terminal_handler.gointernal/api/terminal_handler_test.gointernal/api/user_handler.gointernal/config/config.gointernal/config/config_test.gointernal/scim/handler.gointernal/scim/handler_test.gointernal/scim/users.gointernal/store/store.go
Two minor CodeRabbit findings on round 5. Untracked server/scripts/
seed_action_library.sh stays out of this PR — separate test-infra
dev-tool that doesn't belong in rc11's runtime scope.
Round 6 finding 1 — terminal_handler.go:294
-------------------------------------------
The persistent-vs-transient terminal-error split classified a
"terminal URL missing but internal URL present" state as persistent
operator misconfig immediately. But the two registry keys
(pm:gateway:terminal:<id> and pm:gateway:internal:<id>) are
refreshed by independent goroutines with independent tickers — same
TTL and same refresh interval, but ticker drift can let the
terminal-URL key expire ~tens of ms before the internal-URL key
gets re-extended. That race surfaced as a false-positive operator
alarm ("set GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE") for what was
actually transient skew.
Added a drift-tolerant retry: when the internal-URL probe says the
gateway is alive, re-look up the terminal URL up to 3 times with
30ms backoff before classifying as persistent. Total worst-case
latency added to the legitimate persistent-misconfig path: 90ms.
Retries respect ctx cancellation and bail on non-ErrNoGateway
errors so a wedged registry doesn't extend the wait. If the
terminal URL recovers during the retry window we return it
normally — operator never sees a misclassified error.
A structural fix (couple both registrations into one shared refresh
goroutine) would eliminate the drift class entirely; deferred to
2026.06 — touches gateway boot wiring that's out of rc11 scope.
Round 6 finding 2 — deploy/setup_test.sh:234
--------------------------------------------
The "synthetic only" detection used FAIL_COUNT == 1 → REAL_FAILS = 0,
which can mask a regression: if a real test fails AND a future
refactor accidentally makes the synthetic case pass (return 1 →
return 0), both conditions yield FAIL_COUNT == 1 but real_fails != 0
and the suite reports green.
Replaced the heuristic with explicit before/after tracking around
the meta-test invocation. If META_FAILED != 1 the harness exits 2
with an "expected synthetic failure to fail" diagnostic — the only
way the meta-test can pass is if the test framework itself is
broken, and that should fail loudly rather than silently invert the
real-fails count.
Verified
--------
- bash deploy/setup_test.sh: 12 real + 1 synthetic, all expected
- go test ./internal/api/ -run TestStartTerminal_GatewayNotRegistered:
both persistent + transient cases pass; persistent path takes
~90ms longer due to retry loop (within timeout budget)
- go vet ./... clean
- go build ./... clean
This was referenced May 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles the four rc11 follow-ups so CodeRabbit gets one review per repo instead of four small ones.
#77 — Derived-projection system actions
Replaces the 8 scattered
SyncUserSystemActionscalls in user/SCIM handlers with a single store-side event listener. The listener classifies each persisted event into aSyncOp(per-user / per-cleanup / fan-out-all) via the newAffectedFromEventfunction and dispatches asynchronously. A periodic reconciler (StartReconciliation, default 1m) is the safety net for events the classifier has not yet learned about, matching the existing search-indexer derived-projection pattern.The listener spawns a goroutine per dispatch (so AppendEvent's post-commit path is never blocked) and bounds each goroutine with
CONTROL_SYSTEM_ACTION_RECONCILE_TIMEOUT(default 5m) so a wedged DB or signer cannot leak goroutines indefinitely.UserDeletedis deliberately NOT classified —CleanupDeletedUserActionsneeds the user projection loaded BEFORE the event is applied, so it stays in the handler that emits the event (with SCIM now using the same pattern via the newSystemActionsCleanerinterface).#78 — Partial-terminal-config warn
config.Validate()now returns(warnings []string, err error)and warns when only a subset of the three terminal vars is set, since silently treating that as "terminals disabled" caused operator confusion. AddedTTYDomainExplicitlySetso the auto-derived URL template can be distinguished from an operator-set one.#79 —
gateway_not_registerederror codeSplits the previous "device not connected" overloading. When
LookupGatewayDialreturnsErrNoGateway, the handler now probesLookupGatewayInternalURL:device_not_connected, transient)gateway_not_registered, persistent misconfig — surfaces a hint to setGATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE)Paired with
manchtools/power-manage-sdkandmanchtools/power-manage-webPRs for the const + i18n key.#80 — install.sh + guided setup.sh
New
deploy/install.shbootstrap installer. Refactoreddeploy/setup.shwith an interactive guided env loop (prompt_secret,prompt_string,prompt_yes_no), aclear_env_varhelper, and a No-branch on the terminal prompt that explicitly clearsGATEWAY_TTY_DOMAIN,GATEWAY_PUBLIC_TERMINAL_URL_TEMPLATE, andGATEWAY_WEB_LISTEN_ADDRon rerun (the previous cut silently no-op'd, leaving stale values from the first run).--no-promptand non-TTY stdin preserve the CI path.Review-round fixes already in this PR
SyncAllUsersSystemActionssynchronously inside the AppendEvent post-commit hook, which would have made small admin RPCs O(all-users). Now goroutine-dispatched.context.Background()+ no timeout would have leaked one goroutine per event under DB/signer wedge. Now bounded byCONTROL_SYSTEM_ACTION_RECONCILE_TIMEOUT.device_not_connectedcode for both gateway-gone and gateway-alive-no-terminal-URL, hiding a config bug behind a transient-retry message.Test plan
go build ./...clean across server / agent / sdkgo test ./internal/config/— okgo test -run AffectedFromEvent ./internal/api/— 15 cases pass (every classifier branch + defensive missing-user_id)go test -run TestStartTerminal_GatewayNotRegistered ./internal/api/— 2 cases pass (persistent misconfig vs transient loss)bash deploy/setup_test.sh— 6 cases pass (write_env_var add/update/preserve, clear_env_var existing/missing, disable-terminals-clears-three-vars)Closes #77, #78, #79, #80.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests