feat: 2026.06 closing tech-debt audit cleanup (PR G)#83
Merged
Conversation
Wave-2 of the 2026.06 closing audit picks up the SDK helpers landed on the SDK side (sys/fs.SafeReplaceFile, sys/fs.SafeBackupAndReplace, sys/exec.RunWithCLocale) and adapts the agent to the post-bundle sys/user API where group helpers (GroupCreate, GroupEnsureExists, GroupDelete, GroupAddUser, GroupRemoveUser) now return (*exec.Result, error) instead of just error so callers can inspect stderr/exit code. Includes: - go.mod replace pinned to ../sdk for the duration of the wave-2 chain; orchestrator will swap back to a remote pin once the SDK PRs land - golang.org/x/net auto-bumped to v0.53.0 by `go mod tidy`, closing audit F049 (GO-2026-4918 reachable through executor.downloadFile) - discard the unused *exec.Result at every group-helper call site (action_ssh.go, action_user.go, group.go, helpers.go, sudo.go) Note: the SDK pin in go.mod points at ../sdk during the wave-2 PR chain — the comment block above the replace directive documents the swap-back lifecycle. Note (audit F052): the agent module is still outside the parent go.work workspace (which lists only ./sdk and ./server); workspace-root go vet/test runs continue to silently skip the agent. Adding ./agent to go.work changes the meaning of every contributor's local "run from workspace root" command and is left for an explicit follow-up. Note (audit F063): no `toolchain` directive added to go.mod yet; a contributor with go > 1.25 installed gets the system toolchain by default. Owed.
Closing-audit follow-ups carried over from Bundle A. None of these have race-detector coverage yet (the loops touched here are not exercised by go test -race), but the patterns are visible in the diffs. - F002 runtime.go: periodicSync no longer shares an address-of syncInterval with runAgent. The child owns its own copy; the parent receives interval updates over a buffered channel that drains during waitForStreamEnd so the latest value carries into the next reconnect. - F003 luks.go + executor.go: every read of luksKeyStore / store / actionStore goes through the accessors (getLuksKeyStore / getStore / getActionStore). Snapshotted once per call so the rest of the function operates on a consistent view instead of racing SetLuksKeyStore in the reconnect loop. New getActionStore accessor added to match the existing getStore / getLuksKeyStore pattern. - F004 terminal.go + handler.go + main.go: terminalSweepLoop now selects on a stop channel populated by SetTerminalSender and closed by StopTerminalSweeper at shutdown. main.go calls StopTerminalSweeper after runAgent returns. Idempotent: pre-Start and double-call both no-op. - F019 scheduler.go: corrected the misleading comment that said mu protects resultsCh — it does not, and never did. resultsCh is set once in New() and never reassigned, safe to read/write without the lock. - F020 scheduler.go: Stop() is now safe on a never-Start()'d scheduler. close(s.stopCh) on a nil channel previously panicked. - F036 scheduler.go: documented the resultsCh buffer-size choice (100 ≈ tick volume × 2; persisted to SQLite on full so syncPendingResults picks them up on reconnect). - F042 + F048 executor.go + scheduler.go + scheduler_test.go: the AGENT_UPDATE per-cycle dedup flag moves from package-level globals to per-Executor fields. Scheduler reaches the new ResetUpdateCycle() through the ActionExecutor interface; the mock in scheduler_test.go now records the resets. Closes the silent state-sharing between any future second Executor and production. - F043 luks.go: resolveLuksConflict's argmax is now slices.MaxFunc with an explicit comparator instead of the chained-if pyramid. Adding a fourth tie-breaker is a one-line change. - F045 terminal.go: closeTerminal logs which path triggered the close (sweeper / stop / pump-failure) so post-cleanup logs can distinguish them. The reason parameter was previously `_ = reason` at the bottom. - F053 terminal.go: the deferred SendTerminalStateChange(EXITED) at the bottom of pumpTerminalOutput is now timeout-bounded (5s) so a hung gateway peer cannot wedge the goroutine, blocking new sessions on the terminal-limit slot. Note (audit F005, F032, F033): per-package coverage on luks.go, runtime.go, agent_update.go is still a deferred test-debt item; covered through integration tests but not unit tests. Owed for a follow-up PR; the structural changes here make the fakes possible.
…mp under data dir
The "always log errors and never ignore them" sweep that Bundle A
landed for luks.go / lps.go / agent_update.go finishes here on the
files that were deferred. Same fix shape (`if _, err := runSudoCmd(...
); err != nil { e.logger.Warn(...) }` for cleanup paths, returned
error for state-changing paths) applied uniformly.
- F050 cmd_luks.go: SetLuksDeviceKeyType + AddLuksPassphraseHash
errors are no longer discarded after a successful slot-7
enrollment. Operator now sees a non-zero exit + stderr if the
local state row would otherwise diverge from the actual LUKS
slot. Without this, the next reconcileDeviceKey tick sees
device_key_type=tpm/none and re-enrolls, wiping the user's
passphrase silently.
- F012 + F054 action_repository.go: ~28 sudo error-discard sites
swept. State-changing operations (chmod 644 on the apt keyring,
zypper modifyrepo --name / --enable / --disable / --refresh)
return errors so a divergence from operator intent is visible.
Cleanup operations (rm -f on legacy files / GPG keys / repo files
in the cleanupConflictingAptRepos sweep, the ABSENT-path triple
rm, the dnf rpm --import + makecache idempotent calls, the
pacman -Sy sync, the zypper pre-add removerepo) log at Warn so a
partial cleanup is surfaced even when the surrounding action
reports SUCCESS.
- F056 action_flatpak.go: nil-check the SDK output before the
pinOutput.Stdout += line (SDK can legitimately return nil on
success); log at Warn on flatpak mask --remove failure.
- F061 action_package.go: ensurePackageUnpinned's error is now
logged inside ensurePackageAbsent. The previous silent ignore
surfaced the package manager's "held" message instead of the
underlying unpin failure when removal of a pinned package failed.
- F024 action_repository.go: the dearmor temp file is now created
under <DataDir>/gpg-tmp (0700 root-owned) instead of $TMPDIR.
install.sh sets PrivateTmp=false on the systemd unit so sudo
keeps working, which means a co-resident process in the shared
tmp namespace could otherwise race the dearmor write. Falls back
to $TMPDIR if updateCfg or its DataDir is unset (test paths,
ad-hoc invocations).
Audit F051. The previous package-name extraction split the URL filename on `_` and took the first field, which is correct for Debian's `name_version_arch.deb` filename convention but wrong for mirror layouts and download proxies that publish under custom filenames (myapp_utils.deb with no version/arch). The mismatch silently reinstalled the package on every run and caused incorrect ABSENT skip detection. Mirrors the rpm path's contemporaneous `rpm -qp NAME` query: download the file first, ask `dpkg-deb -f Package` for the canonical NAME from the control file, then check installation status. ABSENT now also downloads (wasteful when the package is already absent, but operator-correct across naming conventions). Adds a defence-in-depth Debian package-name regex (`^[a-z0-9][a-z0-9+.-]*$`) to validate the dpkg-deb output before it reaches downstream tooling. argument-mode exec.Command isn't shell-injectable, but a misnamed Package field in an attacker-published .deb could still confuse the surrounding code.
…WithCLocale; drop dead writeUpdateState
Consumes the wave-2 SDK helpers (sys/fs.SafeReplaceFile,
sys/fs.SafeBackupAndReplace, sys/exec.RunWithCLocale) and closes the
remaining symlink-race + locale-parser audit findings.
- F022 action_user.go: authorized_keys is now written via
sysfs.SafeReplaceFile (O_NOFOLLOW + renameat2 RENAME_NOREPLACE).
Replaces the prior `mkdir -p` / chown / sudo `tee` / chown chain,
which followed symlinks at the tee step — a local user able to
plant a symlink at ~/.ssh/authorized_keys between the chown and
the tee could redirect the sudo'd write to e.g. /etc/cron.d/root.
The agent runs as root, so direct file IO works without sudo'd
tee. Ownership is set after the safe write.
- F023 agent_update.go: the cp / chmod / mv self-update dance is
replaced with sysfs.SafeBackupAndReplace, which uses renameat2 +
O_NOFOLLOW for both the .bak save and the live binary write.
Defeats symlink-replacement on group-writable install dirs that
the prior `cp --` / `mv --` defenses (option-injection only)
could not catch.
- F025 lps.go: getLastAuthTime now invokes `last` via
sysexec.RunWithCLocale, forcing LC_ALL=C and LANG=C so the
weekday parser ("Mon", "Tue", ...) is deterministic. Under a
non-English LANG, last(1) translates the weekdays and the parser
silently returned "could not parse date", causing the rotation
logic to fall back to the caller's default and rotate every tick.
- F018 agent_update.go: writeUpdateState was production-dead — only
the tests called it. Deleted in production, replaced inside the
test file with a `writeStateForTest` fixture helper that fabricates
the state.json directly. The reader / clearer / CheckStartupUpdateState
cleanup path is preserved because an older agent version may have
written state.json on this host before the self-test approach made
rollback unnecessary.
- F028 agent_update.go: getBinaryVersion now matches the documented
two-field "<binary> <version>" output explicitly with parts[1]
instead of "last whitespace token". A future format like
"power-manage-agent 2026.05.07 (commit abc123)" would have
returned "abc123)" under the previous parser.
Note: agent_update.go also carries the F042/F048 per-instance dedup
field; that change ships in the previous concurrency commit but
touches this file too — the additions here are co-located with the
F023 SafeBackupAndReplace swap to keep the file's history readable.
Note (audit F033): no unit test for executeAgentUpdate yet — the
self-update flow's actual swap remains 0% covered. The
SafeBackupAndReplace adoption makes the swap itself easier to fake
in a future test by pointing at a tempdir BinaryPath.
…ity quality Rolls up the remaining medium/low audit findings that did not need a dedicated commit. All small, all self-contained. - F038 credentials.go + F060 credentials_test.go: Load() rejects an unknown future "pmcred:vN:" prefix with a re-enroll hint instead of falling through to AES-GCM and surfacing as an opaque auth-tag error. New credentials_test.go covers the Save/Load round-trip, Save idempotency, v1 magic prefix on fresh writes, future-magic rejection, corrupt-ciphertext failure, missing-salt failure, and Exists/Delete. Closes the 0% coverage gap on credentials. - F009 store.go: documented the "sqlite" driver / "sqlite3" goose dialect mismatch so future readers don't try to "fix" it. - F021 store.go: documented why the WAL-mode comment over-promises given the Go-level RWMutex serialises reads against writes. - F035 store.go: result IDs append a 4-byte crypto/rand suffix to avoid collisions when the same action fires twice in the same nanosecond. Falls back to time-only with a Warn log if crypto/rand ever fails. - F044 store.go: the cron parser is constructed once at package init instead of per-call inside the scheduler hot loop. - F055 wifi.go: ABSENT no longer always reports `changed=true`. Calls ConnectionExists first; reports the actual prior-state and short-circuits the operator-visible event when the connection was already absent. Pattern matches the DNF/Zypper repository short-circuits. - F057 cmd_query.go: replaced the bubble sort with sort.Strings. - F058 README.md: documented the cert-rotation system-roots vs internal-CA trust asymmetry under Security → Network. An operator fronting the control server with a corporate-CA proxy now has a pointer to "your CA bundle must include that proxy's root". - F059 tty.go: IsTTYEnabled accepts the common truthy spellings (1/true/enabled/yes/on, case-insensitive, whitespace-trimmed) so a manual sqlite edit to "true" doesn't silently disable. - F062 README.md: added the missing WIFI row to the Desired State Summary. - F039 README.md: documented the LUKS executor's exception to the "executor delegates to sys/" rule (the LuksKeyStore stream-bound path) so operators understand LUKS rotations require gateway connectivity.
… StreamType callback Pulls in the closing SDK audit cleanup (manchtools/power-manage-sdk#57): the new sys/* helpers (SafeReplaceFile, SafeBackupAndReplace, RunWithCLocale already in 4a777d2), locale-stable pkg/* read paths, the systemd tri-state runSystemctlQuery whitelist, the loginctl Key=Value parser, and the CVE-fix go-directive bump (1.25.10). Replaces the temporary 'replace ... => ../sdk' from 0fb9477 (which was a development convenience while SDK PR #57 was open) with the merged-main pseudo-version. go mod tidy auto-bumped the go directive to 1.25.10 to match the SDK's transitive requirement. Adopts the typed StreamType callback signature: SDK PR #57 changed exec.OutputCallback from 'func(streamType int, ...)' to 'func(streamType StreamType, ...)' so callers can use the typed constant directly without an int cast. Updates handler.go's outputCallback to match.
📝 WalkthroughWalkthroughThis PR refactors the agent's executor state management from package-level globals to per-instance semantics, fixes a concurrent-access race in the runtime sync interval, hardens action execution with explicit error handling and accessor methods, improves terminal lifecycle management with explicit shutdown, and adds comprehensive credential storage validation and tests. ChangesCore Executor and Runtime Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Comment |
The SDK pin bump (c330255) brought go directive 1.25.10 (transitive from SDK), which the prior golang:1.25-bookworm base image cannot satisfy — go mod download fails. Same fix as the SDK side (manchtools/power-manage-sdk#57): bump base to golang:1.26-bookworm, which satisfies any 1.25.x or 1.26.x requirement. Applied to all four integration Dockerfiles (debian, archlinux, fedora, opensuse).
…5.10 Same root cause as the prior Dockerfile FROM bump (ebb2cc4): the generated go.work in the test container claimed 'go 1.25' which is < 'go 1.25.10' that agent/go.mod requires after the SDK pin bump. Bump the printf'd go.work directive to 'go 1.25.10' so go mod download is allowed to proceed inside the container build.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/executor/action_deb.go (1)
47-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the writable-FS check until after the installed-state short-circuit.
With the new canonical-name flow, both branches now hit
requireWritableFSbefore they know whetherdpkg -i/dpkg -ris needed. On a read-only host,PRESENTwill fail even when the package is already installed, andABSENTwill fail even when it is already absent.Proposed fix
case pb.DesiredState_DESIRED_STATE_PRESENT: - // Repair filesystem if mounted read-only - if out, err := e.requireWritableFS(ctx); err != nil { - return out, false, err - } - tmpFile, err := os.CreateTemp("", "*.deb") if err != nil { return nil, false, fmt.Errorf("create temp file: %w", err) @@ if e.isDebInstalled(pkgName) { return &pb.CommandOutput{ ExitCode: 0, Stdout: fmt.Sprintf("deb package %s is already installed", pkgName), }, false, nil } + if out, err := e.requireWritableFS(ctx); err != nil { + return out, false, err + } output, err := runSudoCmd(ctx, "dpkg", "-i", tmpFile.Name()) @@ case pb.DesiredState_DESIRED_STATE_ABSENT: - if out, err := e.requireWritableFS(ctx); err != nil { - return out, false, err - } tmpFile, err := os.CreateTemp("", "*.deb") if err != nil { return nil, false, fmt.Errorf("create temp file: %w", err) @@ if !e.isDebInstalled(pkgName) { return &pb.CommandOutput{ ExitCode: 0, Stdout: fmt.Sprintf("deb package %s is already not installed", pkgName), }, false, nil } + if out, err := e.requireWritableFS(ctx); err != nil { + return out, false, err + } output, err := runSudoCmd(ctx, "dpkg", "-r", pkgName)Also applies to: 106-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/executor/action_deb.go` around lines 47 - 77, Move the call to requireWritableFS so it runs only after we short-circuit on installed state: first download and resolve the canonical package name via debPackageName (using e.downloadFile and tmpFile), call e.isDebInstalled(pkgName) and return early for PRESENT/ABSENT cases, and only if we need to install or remove the package call requireWritableFS before invoking dpkg operations; update the same pattern in the later install/remove branch (the code around debPackageName/isDebInstalled and requireWritableFS at the other block) so checks for writable FS happen after checking installed state.internal/executor/action_repository.go (1)
161-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t report APT cleanup as successful when the
rmfailed.These branches now log deletion failures, but they still record cleanup/output as if the stale artifact was removed. That means the action can return
changed=trueor evensuccesswhile the conflicting.sources/.listfile is still on disk and APT will keep parsing it.A safer fix is to only flip
cleanedUp/append the “removed” message after thermsucceeds, and to propagate a real error from theABSENT/legacy-cleanup paths when an active repo file could not be deleted.Suggested direction
-func (e *Executor) cleanupConflictingAptRepos(ctx context.Context, url, skipRepoFile, skipKeyFile string, output *strings.Builder) bool { +func (e *Executor) cleanupConflictingAptRepos(ctx context.Context, url, skipRepoFile, skipKeyFile string, output *strings.Builder) (bool, error) { ... - output.WriteString(fmt.Sprintf("removing conflicting repository config: %s\n", filePath)) - cleanedUp = true - if _, err := runSudoCmd(ctx, "rm", "-f", filePath); err != nil { - e.logger.Warn(...) - } + if _, err := runSudoCmd(ctx, "rm", "-f", filePath); err != nil { + return cleanedUp, fmt.Errorf("failed to remove conflicting repo file %s: %w", filePath, err) + } + output.WriteString(fmt.Sprintf("removed conflicting repository config: %s\n", filePath)) + cleanedUp = true- if _, err := runSudoCmd(ctx, "rm", "-f", legacyFile); err != nil { - e.logger.Warn(...) - } + if _, err := runSudoCmd(ctx, "rm", "-f", legacyFile); err != nil { + return nil, false, fmt.Errorf("failed to remove legacy repo file %s: %w", legacyFile, err) + }Also applies to: 206-213, 232-246, 269-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/executor/action_repository.go` around lines 161 - 163, The cleanup code in action_repository.go sets cleanedUp and writes "removing conflicting repository config" before actually removing files, so update each removal branch (the blocks that reference variables/filePath/output/cleanedUp and perform os.Remove or similar in the ABSENT/legacy-cleanup/other repo-cleanup paths) to defer setting cleanedUp = true and appending the "removed" message until after the remove call returns nil; if os.Remove (or the platform-specific removal call used) returns an error, log that error to output/process logger and return a real error from the function instead of marking the action as changed/successful. Ensure this change is applied consistently to the blocks currently around the ranges that touch filePath (the branches you noted: the ones now logging deletion failures at ~161, 206-213, 232-246, 269-283) so a failed rm prevents cleanedUp from being set and the caller observes the failing error.internal/handler/terminal.go (1)
122-140:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReconnecting swaps the handler sender, but live sessions keep the dead client.
runAgentcallsSetTerminalSender(client)on every reconnect, but eachterminalSessionkeeps the sender captured at creation time. That means a session opened before disconnect continues sending output/state to the stale client after reconnect, so output is black-holed and the session can keep consuming a slot until idle timeout or manual stop. Either tear down live sessions when the sender changes, or make session sends rebind to the current handler sender.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/handler/terminal.go` around lines 122 - 140, SetTerminalSender currently swaps h.terminalSender but existing terminalSession instances keep the old sender; update SetTerminalSender so that when terminalSender changes you either rebind each live terminalSession to the new sender or tear them down. Specifically, inside Handler.SetTerminalSender (and while holding h.mu) detect if the new sender differs from h.terminalSender, then iterate h.terminals and for each *terminalSession either call a provided method to update its sender (e.g., session.UpdateSender(newSender)) or call session.Stop/Close to terminate the session so it won’t send to the stale client; ensure the update/stop is safe under the same lock or by using per-session locks so you don’t race with terminalSweepLoop or session goroutines.
🧹 Nitpick comments (1)
internal/executor/lps.go (1)
301-303: ⚡ Quick winThread the caller context into
getLastAuthTime.The context is available at the
setupLpsPasswordslevel but is not propagated throughshouldRotateLpstogetLastAuthTime. This causes cancelled/deadline'd operations to ignore the parent context and block until the local 30-second timeout fires instead of respecting the caller's cancellation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/executor/lps.go` around lines 301 - 303, The getLastAuthTime call creates its own background context with a 30s timeout instead of using the caller's context, so cancellations/deadlines aren’t respected; modify getLastAuthTime to accept a context parameter (ctx context.Context), propagate the context from setupLpsPasswords through shouldRotateLps into getLastAuthTime, and inside getLastAuthTime derive a child timeout with context.WithTimeout(ctx, 30*time.Second) before calling sysexec.RunWithCLocale so sysexec.RunWithCLocale(ctx, ...) uses the caller's cancellation while still bounding the local operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/power-manage-agent/runtime.go`:
- Around line 203-227: The non-blocking send to intervalUpdatesOut can leave an
older duration in the size-1 buffer and drop the newer interval; change the
parameter to a bidirectional chan time.Duration (instead of chan<-), and before
sending in doSync drain any existing queued value with a single non-blocking
receive (e.g. select { case <-intervalUpdatesOut: default: }) so the slot is
freed, then send the new syncInterval (blocking or non-blocking as you prefer);
update callers (runAgent) to accept the bidirectional channel signature and
ensure the channel semantics match this drain-then-send behavior so the latest
server interval replaces stale values.
In `@internal/executor/action_flatpak.go`:
- Around line 70-79: The pin step currently treats a failure as a warning and
still returns success; when params.Pin is true, check the result of
runSudoCmd("flatpak", "mask", ...) and if pinErr != nil (or pinOutput indicates
failure) return that error instead of only appending a warning: ensure the
function that calls runSudoCmd (the block handling params.Pin)
constructs/returns a proper error (and preserves output.Stdout via
output/pb.CommandOutput) so the overall action fails when pinErr is non-nil;
update references to params.Pin, runSudoCmd, pinOutput, pinErr, and output to
implement this control flow.
In `@internal/executor/action_repository.go`:
- Around line 754-758: The code only handles repo.Autorefresh==true; add an
explicit else branch to mirror the Enabled handling so the false case calls
runSudoCmd(ctx, "zypper", "--non-interactive", "modifyrepo", "--no-refresh",
name) and returns a wrapped error on failure; locate the Autorefresh block
around repo.Autorefresh and update it to call --refresh when true and
--no-refresh when false, using the same error formatting as the existing enable
path.
In `@internal/executor/agent_update.go`:
- Around line 222-243: The inline comment claiming "The agent runs as root" is
incorrect and SafeBackupAndReplace is invoked without any context or privilege
escalation; remove or correct that comment and ensure the operation can write to
root-owned targets (cfg.BinaryPath / bakPath) when running as the power-manage
user by either (a) confirming SafeBackupAndReplace internally performs privilege
escalation or (b) switching to the existing executor privilege pattern: call the
privileged helper with a context/sudo path similar to writeFileWithSudo /
atomicWriteFile (i.e., pass a context/escrow that triggers sudo/doas or invoke a
sudo-enabled wrapper) so replacing the binary at cfg.BinaryPath succeeds in
production.
In `@internal/executor/lps.go`:
- Around line 141-143: The code currently logs a warning when
st.SetLpsUserState(actionID, username, now, hashStr) fails but continues,
causing a rotated password to be lost if state can't be persisted; change the
behavior so the caller fails the action: replace the warn-only path in the block
where SetLpsUserState is called (and currently uses e.logger.Warn) with code
that returns or propagates the error (wrap with context like "failed to save LPS
user state" including actionID and username) so the reconcile stops instead of
proceeding and causing stale local state (ensuring callers of the function will
see the error and not return the new password on disk).
In `@internal/executor/luks.go`:
- Around line 93-101: The code currently ignores the error returned by
st.GetLuksState(actionID) causing store failures to be treated as missing state;
update the call in the LUKS handling path to capture the error (e.g.,
localState, err := st.GetLuksState(actionID)) and if err != nil return nil,
false, nil, fmt.Errorf("failed to read local LUKS state: %w", err); keep using
getStore() and actionID as the locating symbols so the function returns the real
read error instead of proceeding with nil localState.
In `@internal/executor/wifi.go`:
- Around line 47-63: The current logic only uses the result of
network.ConnectionExists (existed) to set the returned changed flag, but
network.Delete may also remove certDir files; update the logic so changed is
true if either the connection existed OR certDir contained files before deletion
(or if network.Delete reports it removed certs). Probe certDir (e.g., os.Stat /
directory listing) before calling network.Delete, or have network.Delete return
an explicit bool indicating cert cleanup, then combine that with existed to
compute changed; update the return to use this combined flag and adjust the
stdout message to reflect removal of connection and/or certs accordingly
(referencing network.ConnectionExists, network.Delete, conName, certDir and the
existed variable).
In `@internal/scheduler/scheduler.go`:
- Around line 217-227: Stop currently closes s.stopCh then sets it to nil while
runDueActions() (started by Start()) may still be running and reading s.stopCh
without holding s.mu; remove the assignment that nulled the channel in
Scheduler.Stop (do not set s.stopCh = nil after close) so the closed channel
remains selectable until the goroutine exits, or alternatively arrange for the
run loop (runDueActions) to clear s.stopCh under s.mu when it returns; adjust
Scheduler.Stop, Start, and/or runDueActions accordingly to avoid clearing
s.stopCh while the run loop can still access it.
In `@internal/store/tty.go`:
- Around line 6-7: The comment describing which values enable TTY is out of sync
with the parser: update the comment that begins with 'Value is
"1"/"true"/"enabled"...' to also list "yes" and "on" (and state that matching is
case-insensitive and whitespace-trimmed) so it reflects the actual parser
behavior; apply the same comment change to the other identical block further
down (the second comment that documents the TTY truthy values).
In `@test/Dockerfile.integration`:
- Line 1: The Dockerfile currently leaves the container running as root (only
has the FROM golang:1.26-bookworm line); add steps to create a non-root user and
switch to it by creating a uid/gid (e.g., groupadd/useradd or adduser), change
ownership of any work directories/build artifacts to that user, and add a USER
<user> instruction so the integration container runs unprivileged; target the
Dockerfile lines around the base image (FROM golang:1.26-bookworm) to insert
these commands and the final USER directive.
---
Outside diff comments:
In `@internal/executor/action_deb.go`:
- Around line 47-77: Move the call to requireWritableFS so it runs only after we
short-circuit on installed state: first download and resolve the canonical
package name via debPackageName (using e.downloadFile and tmpFile), call
e.isDebInstalled(pkgName) and return early for PRESENT/ABSENT cases, and only if
we need to install or remove the package call requireWritableFS before invoking
dpkg operations; update the same pattern in the later install/remove branch (the
code around debPackageName/isDebInstalled and requireWritableFS at the other
block) so checks for writable FS happen after checking installed state.
In `@internal/executor/action_repository.go`:
- Around line 161-163: The cleanup code in action_repository.go sets cleanedUp
and writes "removing conflicting repository config" before actually removing
files, so update each removal branch (the blocks that reference
variables/filePath/output/cleanedUp and perform os.Remove or similar in the
ABSENT/legacy-cleanup/other repo-cleanup paths) to defer setting cleanedUp =
true and appending the "removed" message until after the remove call returns
nil; if os.Remove (or the platform-specific removal call used) returns an error,
log that error to output/process logger and return a real error from the
function instead of marking the action as changed/successful. Ensure this change
is applied consistently to the blocks currently around the ranges that touch
filePath (the branches you noted: the ones now logging deletion failures at
~161, 206-213, 232-246, 269-283) so a failed rm prevents cleanedUp from being
set and the caller observes the failing error.
In `@internal/handler/terminal.go`:
- Around line 122-140: SetTerminalSender currently swaps h.terminalSender but
existing terminalSession instances keep the old sender; update SetTerminalSender
so that when terminalSender changes you either rebind each live terminalSession
to the new sender or tear them down. Specifically, inside
Handler.SetTerminalSender (and while holding h.mu) detect if the new sender
differs from h.terminalSender, then iterate h.terminals and for each
*terminalSession either call a provided method to update its sender (e.g.,
session.UpdateSender(newSender)) or call session.Stop/Close to terminate the
session so it won’t send to the stale client; ensure the update/stop is safe
under the same lock or by using per-session locks so you don’t race with
terminalSweepLoop or session goroutines.
---
Nitpick comments:
In `@internal/executor/lps.go`:
- Around line 301-303: The getLastAuthTime call creates its own background
context with a 30s timeout instead of using the caller's context, so
cancellations/deadlines aren’t respected; modify getLastAuthTime to accept a
context parameter (ctx context.Context), propagate the context from
setupLpsPasswords through shouldRotateLps into getLastAuthTime, and inside
getLastAuthTime derive a child timeout with context.WithTimeout(ctx,
30*time.Second) before calling sysexec.RunWithCLocale so
sysexec.RunWithCLocale(ctx, ...) uses the caller's cancellation while still
bounding the local operation.
🪄 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: 2426143a-745e-4935-b754-86013ff3f8c4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
README.mdcmd/power-manage-agent/cmd_luks.gocmd/power-manage-agent/cmd_query.gocmd/power-manage-agent/main.gocmd/power-manage-agent/runtime.gogo.modinternal/credentials/credentials.gointernal/credentials/credentials_test.gointernal/executor/action_deb.gointernal/executor/action_flatpak.gointernal/executor/action_package.gointernal/executor/action_repository.gointernal/executor/action_ssh.gointernal/executor/action_user.gointernal/executor/agent_update.gointernal/executor/agent_update_test.gointernal/executor/executor.gointernal/executor/group.gointernal/executor/helpers.gointernal/executor/lps.gointernal/executor/luks.gointernal/executor/sudo.gointernal/executor/wifi.gointernal/handler/handler.gointernal/handler/terminal.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.gointernal/store/store.gointernal/store/tty.gotest/Dockerfile.integrationtest/Dockerfile.integration.archlinuxtest/Dockerfile.integration.fedoratest/Dockerfile.integration.opensuse
Comment on lines
+203
to
+227
| intervalUpdatesOut chan<- time.Duration, | ||
| syncTrigger <-chan struct{}, | ||
| logger *slog.Logger, | ||
| ) { | ||
| syncInterval := initialInterval | ||
| ticker := time.NewTicker(syncInterval) | ||
| defer ticker.Stop() | ||
|
|
||
| logger.Info("periodic sync started", "interval", syncInterval.String()) | ||
|
|
||
| doSync := func(reason string) { | ||
| logger.Info("syncing actions", "reason", reason) | ||
| newInterval := syncActionsFromServer(ctx, client, sched, false, logger) | ||
| if newInterval > 0 && newInterval != *syncInterval { | ||
| *syncInterval = newInterval | ||
| ticker.Reset(*syncInterval) | ||
| if newInterval > 0 && newInterval != syncInterval { | ||
| syncInterval = newInterval | ||
| ticker.Reset(syncInterval) | ||
| logger.Info("sync interval updated", "new_interval", syncInterval.String()) | ||
| // Best-effort report back to runAgent — drop on full | ||
| // because the channel is buffered with 1 slot and a | ||
| // stale pending update would just be overwritten by | ||
| // the next one anyway. | ||
| select { | ||
| case intervalUpdatesOut <- syncInterval: | ||
| default: | ||
| } |
There was a problem hiding this comment.
intervalUpdatesOut can retain a stale interval across reconnects.
With a size-1 buffer, this non-blocking send drops the new interval whenever an older one is still queued. That means runAgent can carry forward an outdated value even though the comments describe this path as preserving the latest server interval. Make intervalUpdatesOut bidirectional here and drain-before-send, or otherwise replace the queued value instead of dropping the new one.
Suggested direction
- intervalUpdatesOut chan<- time.Duration,
+ intervalUpdatesOut chan time.Duration,
...
- select {
- case intervalUpdatesOut <- syncInterval:
- default:
- }
+ select {
+ case <-intervalUpdatesOut:
+ default:
+ }
+ select {
+ case intervalUpdatesOut <- syncInterval:
+ default:
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/power-manage-agent/runtime.go` around lines 203 - 227, The non-blocking
send to intervalUpdatesOut can leave an older duration in the size-1 buffer and
drop the newer interval; change the parameter to a bidirectional chan
time.Duration (instead of chan<-), and before sending in doSync drain any
existing queued value with a single non-blocking receive (e.g. select { case
<-intervalUpdatesOut: default: }) so the slot is freed, then send the new
syncInterval (blocking or non-blocking as you prefer); update callers (runAgent)
to accept the bidirectional channel signature and ensure the channel semantics
match this drain-then-send behavior so the latest server interval replaces stale
values.
Comment on lines
70
to
79
| if params.Pin { | ||
| pinOutput, pinErr := runSudoCmd(ctx, "flatpak", "mask", systemFlag, params.AppId) | ||
| if output == nil { | ||
| output = &pb.CommandOutput{} | ||
| } | ||
| if pinErr != nil { | ||
| if output != nil { | ||
| output.Stdout += "\nWarning: failed to pin application: " + pinErr.Error() | ||
| } | ||
| output.Stdout += "\nWarning: failed to pin application: " + pinErr.Error() | ||
| } else if pinOutput != nil { | ||
| output.Stdout += "\n" + pinOutput.Stdout | ||
| } |
There was a problem hiding this comment.
Return an error when the requested pin step fails.
Line 70 makes Pin part of the requested state, but Lines 75-79 only append a warning and still return success. That leaves the app installed-but-unpinned while the action reports success.
Proposed fix
if params.Pin {
pinOutput, pinErr := runSudoCmd(ctx, "flatpak", "mask", systemFlag, params.AppId)
if output == nil {
output = &pb.CommandOutput{}
}
if pinErr != nil {
- output.Stdout += "\nWarning: failed to pin application: " + pinErr.Error()
+ output.Stderr += "\nfailed to pin application: " + pinErr.Error()
+ return output, false, fmt.Errorf("flatpak install succeeded but pin failed: %w", pinErr)
} else if pinOutput != nil {
output.Stdout += "\n" + pinOutput.Stdout
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/action_flatpak.go` around lines 70 - 79, The pin step
currently treats a failure as a warning and still returns success; when
params.Pin is true, check the result of runSudoCmd("flatpak", "mask", ...) and
if pinErr != nil (or pinOutput indicates failure) return that error instead of
only appending a warning: ensure the function that calls runSudoCmd (the block
handling params.Pin) constructs/returns a proper error (and preserves
output.Stdout via output/pb.CommandOutput) so the overall action fails when
pinErr is non-nil; update references to params.Pin, runSudoCmd, pinOutput,
pinErr, and output to implement this control flow.
Comment on lines
754
to
+758
| // Set autorefresh | ||
| if repo.Autorefresh { | ||
| runSudoCmd(ctx, "zypper", "--non-interactive", "modifyrepo", "--refresh", name) | ||
| if _, err := runSudoCmd(ctx, "zypper", "--non-interactive", "modifyrepo", "--refresh", name); err != nil { | ||
| return nil, false, fmt.Errorf("failed to enable autorefresh on zypper repo %s: %w", name, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does zypper addrepo --refreshenable autorefresh by default, and what is the documentedzypper modifyrepo option to disable autorefresh for an existing repository?
💡 Result:
Yes—zypper addrepo --refresh enables autorefresh for the repository being added, i.e., it adds the repository with auto-refresh turned on, rather than leaving it disabled by default. This is because newly added repositories have auto-refresh disabled by default, and the man page states that to enable auto-refresh you must use --refresh of the modifyrepo command (i.e., the per-repo switch). [1][2] To disable autorefresh for an already-existing repository via zypper modifyrepo, the documented option is --no-refresh (short form -R). [2][3] Example: - Disable autorefresh for repo alias myrepo: sudo zypper modifyrepo --no-refresh myrepo (or sudo zypper modifyrepo -R myrepo). [3][2]
Citations:
- 1: https://www.unix.com/man_page/suse/8/zypper/
- 2: https://manpages.ubuntu.com/manpages/xenial/man8/zypper.8.html
- 3: http://jniq.blogspot.com/2008_05_04_archive.html
🏁 Script executed:
cat -n internal/executor/action_repository.go | sed -n '730,780p'Repository: manchtools/power-manage-agent
Length of output: 2441
Add explicit disable path for repo.Autorefresh=false using --no-refresh flag.
The Autorefresh handling (lines 754-759) only enforces the true case. When repo.Autorefresh=false, the code takes no action, leaving the repository with autorefresh enabled by default. Add an else clause mirroring the pattern used for the Enabled field to explicitly disable autorefresh using zypper modifyrepo --no-refresh:
Expected fix pattern
// Set autorefresh
if repo.Autorefresh {
if _, err := runSudoCmd(ctx, "zypper", "--non-interactive", "modifyrepo", "--refresh", name); err != nil {
return nil, false, fmt.Errorf("failed to enable autorefresh on zypper repo %s: %w", name, err)
}
} else {
if _, err := runSudoCmd(ctx, "zypper", "--non-interactive", "modifyrepo", "--no-refresh", name); err != nil {
return nil, false, fmt.Errorf("failed to disable autorefresh on zypper repo %s: %w", name, err)
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/action_repository.go` around lines 754 - 758, The code only
handles repo.Autorefresh==true; add an explicit else branch to mirror the
Enabled handling so the false case calls runSudoCmd(ctx, "zypper",
"--non-interactive", "modifyrepo", "--no-refresh", name) and returns a wrapped
error on failure; locate the Autorefresh block around repo.Autorefresh and
update it to call --refresh when true and --no-refresh when false, using the
same error formatting as the existing enable path.
Comment on lines
+141
to
143
| if err := st.SetLpsUserState(actionID, username, now, hashStr); err != nil { | ||
| e.logger.Warn("failed to save LPS user state", "action_id", actionID, "username", username, "error", err) | ||
| } |
There was a problem hiding this comment.
Fail the action when the rotated-password state cannot be persisted.
If SetLpsUserState fails, the password has already changed but local state stays stale. shouldRotateLps treats missing state as "initial", so the next reconcile can rotate the same user again and invalidate the password you just returned.
💡 Suggested fix
- if err := st.SetLpsUserState(actionID, username, now, hashStr); err != nil {
- e.logger.Warn("failed to save LPS user state", "action_id", actionID, "username", username, "error", err)
- }
+ if err := st.SetLpsUserState(actionID, username, now, hashStr); err != nil {
+ e.logger.Error("failed to save LPS user state", "action_id", actionID, "username", username, "error", err)
+ output.WriteString(fmt.Sprintf("LPS: %s — rotated password but failed to persist local state\n", username))
+ anyError = fmt.Errorf("persist LPS state for %s: %w", username, err)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/lps.go` around lines 141 - 143, The code currently logs a
warning when st.SetLpsUserState(actionID, username, now, hashStr) fails but
continues, causing a rotated password to be lost if state can't be persisted;
change the behavior so the caller fails the action: replace the warn-only path
in the block where SetLpsUserState is called (and currently uses e.logger.Warn)
with code that returns or propagates the error (wrap with context like "failed
to save LPS user state" including actionID and username) so the reconcile stops
instead of proceeding and causing stale local state (ensuring callers of the
function will see the error and not return the new password on disk).
Comment on lines
+93
to
+101
| st := e.getStore() | ||
| if st == nil { | ||
| return nil, false, nil, fmt.Errorf("agent store not configured") | ||
| } | ||
|
|
||
| var output strings.Builder | ||
|
|
||
| // Load local state | ||
| localState, _ := e.store.GetLuksState(actionID) | ||
| localState, _ := st.GetLuksState(actionID) |
There was a problem hiding this comment.
Fail closed if local LUKS state can't be read.
Ignoring GetLuksState errors here turns a store failure into localState == nil, which sends the PRESENT path down the first-run flow: rediscover volume, retry ownership, and potentially rotate against the wrong device. Please return the read error instead of treating it as "no state".
Suggested fix
st := e.getStore()
if st == nil {
return nil, false, nil, fmt.Errorf("agent store not configured")
}
var output strings.Builder
// Load local state
- localState, _ := st.GetLuksState(actionID)
+ localState, err := st.GetLuksState(actionID)
+ if err != nil {
+ return nil, false, nil, fmt.Errorf("get luks state: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| st := e.getStore() | |
| if st == nil { | |
| return nil, false, nil, fmt.Errorf("agent store not configured") | |
| } | |
| var output strings.Builder | |
| // Load local state | |
| localState, _ := e.store.GetLuksState(actionID) | |
| localState, _ := st.GetLuksState(actionID) | |
| st := e.getStore() | |
| if st == nil { | |
| return nil, false, nil, fmt.Errorf("agent store not configured") | |
| } | |
| var output strings.Builder | |
| // Load local state | |
| localState, err := st.GetLuksState(actionID) | |
| if err != nil { | |
| return nil, false, nil, fmt.Errorf("get luks state: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/luks.go` around lines 93 - 101, The code currently ignores
the error returned by st.GetLuksState(actionID) causing store failures to be
treated as missing state; update the call in the LUKS handling path to capture
the error (e.g., localState, err := st.GetLuksState(actionID)) and if err != nil
return nil, false, nil, fmt.Errorf("failed to read local LUKS state: %w", err);
keep using getStore() and actionID as the locating symbols so the function
returns the real read error instead of proceeding with nil localState.
Comment on lines
+47
to
+63
| existed, existsErr := network.ConnectionExists(ctx, conName) | ||
| if existsErr != nil { | ||
| e.logger.Warn("wifi ABSENT: ConnectionExists failed; conservatively reporting changed=true", | ||
| "connection", conName, "error", existsErr) | ||
| existed = true | ||
| } | ||
| if err := network.Delete(ctx, conName, certDir); err != nil { | ||
| return nil, false, fmt.Errorf("delete connection: %w", err) | ||
| } | ||
| stdout := fmt.Sprintf("connection %s already absent\n", conName) | ||
| if existed { | ||
| stdout = fmt.Sprintf("removed connection %s\n", conName) | ||
| } | ||
| return &pb.CommandOutput{ | ||
| ExitCode: 0, | ||
| Stdout: fmt.Sprintf("removed connection %s\n", conName), | ||
| }, true, nil | ||
| Stdout: stdout, | ||
| }, existed, nil |
There was a problem hiding this comment.
changed should also account for EAP-TLS cert cleanup.
This now keys changed off ConnectionExists only, but ABSENT also deletes certDir. If the profile is already gone and stale certs are still present, network.Delete(...) can remove files while this returns changed=false and "already absent". Please base the result on both resources, or probe certDir before deleting it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/wifi.go` around lines 47 - 63, The current logic only uses
the result of network.ConnectionExists (existed) to set the returned changed
flag, but network.Delete may also remove certDir files; update the logic so
changed is true if either the connection existed OR certDir contained files
before deletion (or if network.Delete reports it removed certs). Probe certDir
(e.g., os.Stat / directory listing) before calling network.Delete, or have
network.Delete return an explicit bool indicating cert cleanup, then combine
that with existed to compute changed; update the return to use this combined
flag and adjust the stdout message to reflect removal of connection and/or certs
accordingly (referencing network.ConnectionExists, network.Delete, conName,
certDir and the existed variable).
Comment on lines
217
to
+227
| func (s *Scheduler) Stop() { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| if !s.running { | ||
| return | ||
| } | ||
|
|
||
| close(s.stopCh) | ||
| if s.stopCh != nil { | ||
| close(s.stopCh) | ||
| s.stopCh = nil | ||
| } |
There was a problem hiding this comment.
Don't nil out stopCh before the run loop exits.
Start() reads s.stopCh without mu, so if Stop() closes the channel and then sets it to nil while runDueActions() is still running, the next loop iteration can select on a nil channel and miss the stop signal entirely. Keeping the closed channel in place until the goroutine returns avoids that race.
Suggested fix
func (s *Scheduler) Stop() {
s.mu.Lock()
defer s.mu.Unlock()
if !s.running {
return
}
if s.stopCh != nil {
close(s.stopCh)
- s.stopCh = nil
}
s.running = false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scheduler/scheduler.go` around lines 217 - 227, Stop currently
closes s.stopCh then sets it to nil while runDueActions() (started by Start())
may still be running and reading s.stopCh without holding s.mu; remove the
assignment that nulled the channel in Scheduler.Stop (do not set s.stopCh = nil
after close) so the closed channel remains selectable until the goroutine exits,
or alternatively arrange for the run loop (runDueActions) to clear s.stopCh
under s.mu when it returns; adjust Scheduler.Stop, Start, and/or runDueActions
accordingly to avoid clearing s.stopCh while the run loop can still access it.
Comment on lines
+6
to
+7
| // Value is "1"/"true"/"enabled" (case-insensitive, whitespace-trimmed) | ||
| // when enabled; any other value (including absent) means disabled. |
There was a problem hiding this comment.
Keep truthy-value comments aligned with actual parser behavior.
The comments describe only 1/true/enabled, but the implementation also treats yes/on as enabled.
Suggested comment-only fix
-// Value is "1"/"true"/"enabled" (case-insensitive, whitespace-trimmed)
+// Value is "1"/"true"/"enabled"/"yes"/"on" (case-insensitive, whitespace-trimmed)
// when enabled; any other value (including absent) means disabled.
@@
-// truthy spellings (1/true/enabled, case-insensitive, whitespace-trimmed)
+// truthy spellings (1/true/enabled/yes/on, case-insensitive, whitespace-trimmed)Also applies to: 19-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/tty.go` around lines 6 - 7, The comment describing which
values enable TTY is out of sync with the parser: update the comment that begins
with 'Value is "1"/"true"/"enabled"...' to also list "yes" and "on" (and state
that matching is case-insensitive and whitespace-trimmed) so it reflects the
actual parser behavior; apply the same comment change to the other identical
block further down (the second comment that documents the TTY truthy values).
| @@ -1,4 +1,4 @@ | |||
| FROM golang:1.25-bookworm | |||
| FROM golang:1.26-bookworm | |||
There was a problem hiding this comment.
Container still runs as root by default.
No USER is set, so integration runs default to root, which can mask permission regressions and violates DS-0002 hardening guidance.
Suggested fix
RUN chown -R power-manage:power-manage /workspace /home/power-manage
+USER power-manage🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Dockerfile.integration` at line 1, The Dockerfile currently leaves the
container running as root (only has the FROM golang:1.26-bookworm line); add
steps to create a non-root user and switch to it by creating a uid/gid (e.g.,
groupadd/useradd or adduser), change ownership of any work directories/build
artifacts to that user, and add a USER <user> instruction so the integration
container runs unprivileged; target the Dockerfile lines around the base image
(FROM golang:1.26-bookworm) to insert these commands and the final USER
directive.
… direct SafeReplaceFile The wave-2 SDK helper adoption (4a777d2) replaced the prior sudo-tee SSH-key-write with sysfs.SafeReplaceFile to close the symlink race in /home/<user>/.ssh/authorized_keys. SafeReplaceFile uses os.CreateTemp + renameat2 directly from the agent process, which only works when the agent runs as root — power-manage (sudoers mode, the chosen 2026.06 architecture per design decision #1) can't write into a 700 user-owned .ssh dir. CI exposed this as a 'permission denied' on TestIntegration_EdgeCase_SSHDirWrongPermissions. Replaced SafeReplaceFile with: write content to /tmp as the agent process, then sudo install -T -m 600 -o <user> -g <group> tmp dest. install replaces the destination atomically and applies mode + owner + group up-front in one syscall, working both as root (production) and via sudo escalation (sudoers mode + integration tests). The -T flag forces dest-as-file so a directory at the destination can't reroute the copy. Symlink-race protection is weaker than SafeReplaceFile but the threat model collapses to 'the user redirecting their own keys' — the user already owns the dir at 700 and could just edit the file directly. Also added /usr/bin/install and /bin/install to the sudoers template so the new escalation works end-to-end. Dropped the unused sysfs import.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/executor/action_user.go`:
- Around line 567-600: The current use of install -T (invoked via runSudoCmd
with tmpPath -> authKeysFile) is vulnerable to symlink races; replace it with an
atomic, privileged rename into the user's .ssh directory. Concretely: instead of
creating tmpPath in /tmp and calling install, create the temp file inside the
same directory as authKeysFile (or create a temp in a safe location and then
perform the rename using renameat/rename under elevated privileges), set
owner/mode via fchown/fchmod while privileged, then perform an atomic rename to
authKeysFile via a privileged helper or by having runSudoCmd execute a small
helper that does renameat(2) and the chown/chmod operations; update code around
tmpFile/tmpPath, runSudoCmd invocation and any calls that trust install -T (and
ensure homeGroupFor(params) and params.Username are applied inside the
privileged rename helper).
🪄 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: c5e7d145-925e-435b-bd48-db9c10015a44
📒 Files selected for processing (2)
internal/executor/action_user.gointernal/setup/sudoers.tmpl
Comment on lines
+567
to
+600
| // Write authorized_keys atomically via a sudo-escalated install(1). | ||
| // install replaces the destination in one syscall after copying the | ||
| // source content, applying mode + owner + group up-front; -T forces | ||
| // the destination to be treated as a file path (so a directory at | ||
| // the destination doesn't get the file copied INTO it). The temp | ||
| // source lives under /tmp where the agent process can always | ||
| // write, regardless of whether the agent runs as root or via the | ||
| // sudoers escalation model — sysoers mode is the chosen 2026.06 | ||
| // architecture (see project_2026_06_design_decisions.md #1) so | ||
| // directly writing under the user's 700 .ssh dir from the agent | ||
| // process is not viable. | ||
| // | ||
| // The previous shape (`mkdir -p` → `chown` → sudo `tee` → `chown`) | ||
| // followed symlinks at the tee step: a local user able to plant a | ||
| // symlink at ~/.ssh/authorized_keys between the chown and the tee | ||
| // could redirect the write to e.g. /etc/cron.d/root. install -T | ||
| // closes that vector by refusing to write through a symlink-loop | ||
| // directory mismatch and by setting the destination's metadata in | ||
| // the same call instead of a separate chown round-trip. | ||
| tmpFile, err := os.CreateTemp("", "agent-authorized-keys-") | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to create temp authorized_keys: %w", err) | ||
| } | ||
|
|
||
| // Set ownership and permissions on authorized_keys | ||
| if _, err := runSudoCmd(ctx, "chown", ownership, authKeysFile); err != nil { | ||
| return false, fmt.Errorf("failed to set authorized_keys ownership: %w", err) | ||
| tmpPath := tmpFile.Name() | ||
| defer os.Remove(tmpPath) | ||
| if _, err := tmpFile.WriteString(desiredContent); err != nil { | ||
| tmpFile.Close() | ||
| return false, fmt.Errorf("failed to write temp authorized_keys: %w", err) | ||
| } | ||
| if err := tmpFile.Close(); err != nil { | ||
| return false, fmt.Errorf("failed to close temp authorized_keys: %w", err) | ||
| } | ||
| if _, err := runSudoCmd(ctx, "chmod", "600", authKeysFile); err != nil { | ||
| return false, fmt.Errorf("failed to set authorized_keys permissions: %w", err) | ||
| if _, err := runSudoCmd(ctx, "install", "-m", "600", "-o", params.Username, "-g", homeGroupFor(params), "-T", tmpPath, authKeysFile); err != nil { | ||
| return false, fmt.Errorf("failed to install authorized_keys: %w", err) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GNU coreutils install -T guarantee atomic replacement or protect against symlink races when the destination path is attacker-controlled?
💡 Result:
No, GNU coreutils install -T does not guarantee atomic replacement or protect against symlink races when the destination path is attacker-controlled [1][2][3]. The official documentation explains that the -T option (--no-target-directory) prevents treating the destination as a directory if it is one or a symlink to a directory, ensuring install source dest treats dest as a file path rather than installing into dest/source. This avoids certain races where dest might be created as a directory by another process between stat and operation, as illustrated in the mv example: without -T, mv /tmp/source /tmp/dest might mv to /tmp/dest/source if dest becomes a directory; with -T, it guarantees mv to /tmp/dest [2]. However, for symlink races in file replacement, install (like cp) typically works by: checking if dest exists (lstat), possibly unlinking it if --force or similar, then creating/opening dest for writing (possibly after truncating), and copying data [3]. This creates a TOCTOU window where an attacker controlling the directory can replace dest with a symlink after unlink/check but before open/create, redirecting the write to an arbitrary location if running privileged. No sources indicate install uses atomic techniques like writing to a temp file and renameat (which is atomic within the same filesystem [4]) or O_EXCL|O_CREAT without prior unlink. No CVEs or bugs were found for GNU coreutils install regarding symlink races (all recent CVEs like CVE-2026-35355/56 are for uutils Rust rewrite [5][6]), and coreutils src/install.c shows standard cp-like logic without special atomic protections for -T [3]. The -T option protects specifically against directory/symlink-to-directory misinterpretation races but not general symlink following or replacement races on attacker-controlled paths. For full protection, use filesystem mounts with ms_bind,noexec,nodev,nosuid or privileged dirfd-based ops (openat etc.), but install does not employ these [7].
Citations:
- 1: https://www.gnu.org/software/coreutils/install
- 2: https://www.gnu.org/software/coreutils/manual/coreutils
- 3: https://github.com/coreutils/coreutils/blob/master/src/install.c
- 4: https://man7.org/linux/man-pages/man2/renameat.2.html
- 5: https://nvd.nist.gov/vuln/detail/CVE-2026-35355
- 6: https://nvd.nist.gov/vuln/detail/CVE-2026-35356
- 7: https://instatunnel.my/blog/symlink-attacks-when-file-operations-betray-your-trust
Use an atomic, symlink-safe approach for the privileged authorized_keys write instead of install -T.
The -T flag in install(1) prevents treating the destination as a directory, but does not protect against symlink races. Since the destination ~/.ssh/authorized_keys is under a user-owned directory, the user can race the write by replacing authorized_keys or parent path components with a symlink between the prepare steps and the install execution. When run with sudo, this redirects the write to an arbitrary file with root privileges, enabling arbitrary-file clobber.
To close this race, use an atomic replacement pattern such as tempfile + renameat(2) within a privileged context (e.g., via dirfd-based operations or a dedicated privileged helper), not install(1) on an attacker-controlled destination path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/action_user.go` around lines 567 - 600, The current use of
install -T (invoked via runSudoCmd with tmpPath -> authKeysFile) is vulnerable
to symlink races; replace it with an atomic, privileged rename into the user's
.ssh directory. Concretely: instead of creating tmpPath in /tmp and calling
install, create the temp file inside the same directory as authKeysFile (or
create a temp in a safe location and then perform the rename using
renameat/rename under elevated privileges), set owner/mode via fchown/fchmod
while privileged, then perform an atomic rename to authKeysFile via a privileged
helper or by having runSudoCmd execute a small helper that does renameat(2) and
the chown/chmod operations; update code around tmpFile/tmpPath, runSudoCmd
invocation and any calls that trust install -T (and ensure homeGroupFor(params)
and params.Username are applied inside the privileged rename helper).
… fix Pulls in manchtools/power-manage-sdk#58 which restores the pre-PR-#57 contract that pkg/* commands return a non-nil error when the underlying process exits non-zero. Without this the agent reported EXECUTION_STATUS_SUCCESS for failed installs (e.g. apt install of a non-existent package), which TestIntegration_Package_*/InstallNonExistent caught across all 4 distros on PR G CI.
action_deb.go: the wave-2 commit (a1eedc8) made the ABSENT path download the .deb to read its canonical Package name from dpkg-deb, mirroring the PRESENT path. That breaks the common operator intent — for ABSENT we want 'make sure this isn't installed', and a 404 on the URL means the artifact is gone (the package physically cannot still be installed via that URL anymore). Wave-2's change made TestIntegration_Deb/RemoveAbsent fail with 'download: 404 Not Found' instead of the desired SUCCESS. Restored the URL-filename heuristic for the ABSENT path only — the Debian mirror filename grammar is name_version_arch.deb, take the first underscore-separated field. Validate against the Debian package-name regex so a malformed URL surfaces a clear validation error rather than silently downloading. PRESENT keeps the download + dpkg-deb authoritative read because installing requires the .deb file anyway. New helper debPackageNameFromURL. luks.go: local CR pass on the agent branch flagged that setupLuks inlined e.getActionStore() at the conflict-resolution check while F003's explicit rule is to snapshot accessors at function start (otherwise concurrent SetActionStore can change the value mid-execution). Snapshot to local 'as'. Same pass also flagged RevokeLuksDeviceKey's combined nil-check — split into two distinct checks with specific error messages (was: 'LUKS key store not configured' even when it was actually the agent store that was nil).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/executor/action_deb.go`:
- Around line 108-123: The ABSENT branch currently reconstructs the canonical
package name with debPackageNameFromURL(params.Url), which breaks uninstall for
packages installed via non-canonical URLs; change the logic to read the
canonical package name from persisted install metadata or from an explicit
action param (e.g. params.PackageName or params.CanonicalName) instead of
calling debPackageNameFromURL, and surface a validation error only if that
metadata/param is missing; update the ABSENT handling (and the similar code path
around the block that currently calls debPackageNameFromURL in the 169-191
region) to use the stored/explicit name and fall back to an explicit validation
message rather than attempting to parse the URL.
- Line 27: The package name regex validDebPkgName currently allows
single-character names because it uses `*` for the tail segment; update the
pattern used by validDebPkgName from `^[a-z0-9][a-z0-9+.-]*$` to require at
least one additional character (change `*` to `+`) so it becomes
`^[a-z0-9][a-z0-9+.-]+$`, ensuring Debian's minimum two-character package-name
rule is enforced wherever validDebPkgName is used.
🪄 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: 36e914d3-14a0-477c-8a66-cc9cd50f22b8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/executor/action_deb.gointernal/executor/luks.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/executor/luks.go
| // shell injection, but a misnamed package field could still confuse | ||
| // downstream tooling, and the grammar is narrow enough that a strict | ||
| // regex costs nothing. | ||
| var validDebPkgName = regexp.MustCompile(`^[a-z0-9][a-z0-9+.-]*$`) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the current Debian Policy manual, what are the exact syntax and minimum-length requirements for binary package names?
💡 Result:
Binary package names: 1) Exact syntax (character set): must consist only of lower case letters (a-z), digits (0-9), plus (+) and minus (-) signs, and periods (.). [1] 2) Minimum length requirement: must be at least two characters long. [1] 3) Additional rule (start character): must start with an alphanumeric character. [1] 4) Relationship to binary packages: Debian Policy states that binary package names must follow the same syntax and restrictions as source package names. [1] Therefore, per the current Debian Policy manual, the complete requirements are: lowercase a-z, digits 0-9, +, -, and. only; length >= 2; first character must be alphanumeric. [1]
Citations:
🏁 Script executed:
cat -n internal/executor/action_deb.go | sed -n '25,30p;153,158p;186,192p'Repository: manchtools/power-manage-agent
Length of output: 993
Enforce Debian Policy's minimum 2-character package-name requirement.
The regex currently allows single-character names (e.g., "a") due to the * quantifier permitting zero additional characters. Debian Policy requires package names to be at least two characters long. Change * to + to enforce this constraint.
Suggested fix
-var validDebPkgName = regexp.MustCompile(`^[a-z0-9][a-z0-9+.-]*$`)
+var validDebPkgName = regexp.MustCompile(`^[a-z0-9][a-z0-9+.-]+$`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var validDebPkgName = regexp.MustCompile(`^[a-z0-9][a-z0-9+.-]*$`) | |
| var validDebPkgName = regexp.MustCompile(`^[a-z0-9][a-z0-9+.-]+$`) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/action_deb.go` at line 27, The package name regex
validDebPkgName currently allows single-character names because it uses `*` for
the tail segment; update the pattern used by validDebPkgName from
`^[a-z0-9][a-z0-9+.-]*$` to require at least one additional character (change
`*` to `+`) so it becomes `^[a-z0-9][a-z0-9+.-]+$`, ensuring Debian's minimum
two-character package-name rule is enforced wherever validDebPkgName is used.
Comment on lines
+108
to
+123
| // For ABSENT we want to remove a package the operator | ||
| // previously installed by URL. Downloading the .deb just to | ||
| // read its Package field is wasteful when it's already gone, | ||
| // AND it actively breaks the common case where the URL is | ||
| // dead because the artifact was deleted upstream after the | ||
| // install (ABSENT then degrades to "download failed" instead | ||
| // of the desired "already absent"). Use the URL filename's | ||
| // `name_version_arch.deb` segment as the canonical-name | ||
| // heuristic — same shape every Debian mirror uses, and the | ||
| // only field we need is `name`. If the filename doesn't | ||
| // match, the action is misconfigured and we surface that as | ||
| // a validation error rather than a silent download attempt. | ||
| pkgName, err := debPackageNameFromURL(params.Url) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } |
There was a problem hiding this comment.
ABSENT now breaks installs that came from non-canonical artifact URLs.
Line 120 re-derives the package name from the URL, but the PRESENT path intentionally stopped trusting URL filenames. Any package installed from a proxy/redirect/download endpoint that does not expose name_version_arch.deb in the path will install successfully and later fail to uninstall with a validation error. Please carry the canonical package name as metadata, or make it explicit in the action params, instead of reconstructing it from the URL here.
Also applies to: 169-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/executor/action_deb.go` around lines 108 - 123, The ABSENT branch
currently reconstructs the canonical package name with
debPackageNameFromURL(params.Url), which breaks uninstall for packages installed
via non-canonical URLs; change the logic to read the canonical package name from
persisted install metadata or from an explicit action param (e.g.
params.PackageName or params.CanonicalName) instead of calling
debPackageNameFromURL, and surface a validation error only if that
metadata/param is missing; update the ABSENT handling (and the similar code path
around the block that currently calls debPackageNameFromURL in the 169-191
region) to use the stored/explicit name and fall back to an explicit validation
message rather than attempting to parse the URL.
PaulDotterer
added a commit
that referenced
this pull request
Jun 9, 2026
…alDir, UserPath, crash-safe SafeBackupAndReplace Picks up the SDK helpers this branch consumes now that sdk#83 is merged to main. Resolves the standalone (GOWORK=off) build's undefined AssertRealDir / OpenRealDir / FchownNoFollow / UserPath / RunStreamingChildPath that the lint CI job hit against the old pin.
PaulDotterer
added a commit
that referenced
this pull request
Jun 9, 2026
* sec(sudo): fix !! deny inversion + scope TerminalAdmin Defaults to group The agent-protection rules used a double-bang (`!!/usr/bin/visudo`, `!!systemctl * power-manage-agent*`). Per sudoers(5) an even number of '!' cancels out to an ALLOW, so these GRANTED visudo (sudoers edit -> root) and GRANTED stopping/disabling the managed agent — the opposite of the comment's stated intent. Collapse to a single '!' deny. The TERMINAL_ADMIN Defaults block was emitted as bare `Defaults requiretty`/`timestamp_timeout=0`, which in an /etc/sudoers.d drop-in applies host-globally — breaking root's non-TTY sudo (cron, systemd, ansible) and stripping credential caching for every other admin. Scope to `Defaults:%<group>` per the ADR's group-only threat model. Tests pin: no double-bang ever appears, agent/visudo denies are real, and Defaults are group-scoped not host-global. visudo -c still accepts. * fix(user): never unlock a passwordless account on reconcile updateUser computed desiredLocked from params.Disabled alone. A no_password (#94) or system_user account gets NO password at create and sits at the shadow-locked '!' default; on the next reconcile Disabled is false but currentInfo.Locked is true, so updateUser ran usermod -U, stripped the '!', and produced a PASSWORDLESS login path — defeating the no_password contract for pm-tty-* accounts. Introduce desiredAccountLocked() as the shared source of truth mirroring createUser's password-skip condition (Disabled || NoPassword || SystemUser); unlock only fires for an account that actually has a password hash to restore. Property test pins the cross-function invariant for all flag combinations. * sec(user): close ~/.ssh chown/chmod symlink TOCTOU (F022) setupSSHKeys ran path-based chown/chmod on ~/.ssh, which the target user owns — they can plant ~/.ssh (or authorized_keys) as a symlink to /etc/shadow between operations, and a root-run chown without -h transfers ownership of the symlink target. SafeReplaceFile hardened the write but the surrounding ownership fixups re-opened the vector. Guard the .ssh path with sysfs.AssertRealDir (rejects symlinked/non-dir paths) before the privileged ops, and pass chown -h/--no-dereference on both the dir and the file so a racing symlink swap can't be dereferenced. The reusable guard is extracted into the SDK (sdk sys/fs.AssertRealDir); agent repin lands at finalization. * sec(handler): verify CA signature on SYNC instant-action fast-path OnActionWithStreaming short-circuits ACTION_TYPE_SYNC and returns success before ExecuteWithStreaming runs — so the executor's F-31 signature check never fired on this path, leaving SYNC forgeable by a compromised gateway/Valkey despite #90's 'no more REBOOT/SYNC bypass' claim. A forged unsigned SYNC could force resyncs at will. Extract the verification into Executor.VerifyAction and call it from both the executor and the SYNC fast-path; an unsigned/tampered SYNC is now refused (FAILED, no trigger), a validly signed SYNC still succeeds, and a nil verifier (signing disabled) remains a no-op. * fix(scheduler): remove stopCh data race from the F020 Stop() fix The F020 fix added `s.stopCh = nil` under the lock, but Start's select read s.stopCh WITHOUT the lock on every iteration — an unsynchronized read/write the race detector flags, plus a window where the select could re-read nil and miss the close (Stop failing to halt the loop unless paired with ctx cancellation). Capture stopCh into a local under the lock in Start and select on the local; Stop closes the still-shared channel once (guarded by the running flag) and no longer nils it. New -race test pins that Stop halts the loop without ctx cancel, and that Stop stays pre-Start/double-call safe. * fix(store): stop double-executing dispatched actions handler.OnAction stores an action via AddAction -> SaveAction and then executes it immediately, but SaveAction set next_execute_at to 'now' (calculateNextExecute with lastExecuted=nil), so the scheduler's ticker re-ran the action a second time — a real double-execution for non-idempotent SHELL actions. The SyncActions standalone path already guards this by advancing the cursor to the future. Compute SaveAction's cursor as calculateNextExecute(action, &now, false) so the stored copy lands in the future for every schedule shape (nil, run_on_assign, interval, cron). run_on_assign's immediate-run intent is satisfied by the handler's inline execution, so the now-unused runOnAssign parameter is dropped from SaveAction/AddAction. Test pins that a freshly dispatched action is never immediately due. * fix(store): normalize protojson whitespace in change detection SyncActions / SyncStandaloneAndGrouped detected changes by byte-comparing stored vs freshly-marshaled protojson. protojson injects insignificant whitespace seeded per-binary, so after an agent self-update the new binary's marshal can differ byte-wise from the stored blob — flagging EVERY action as changed (immediate re-execution of the whole set) and resetting every group's cadence. Add canonicalProtoJSON (protojson + json.Compact) for all stored blobs and compact the stored side at each comparison, so whitespace-only drift compares equal (including pre-upgrade rows). Tests inject a space after a colon — a position protojson never emits — to pin the contract deterministically regardless of the per-process whitespace seed. * fix(deb,rpm): make ABSENT use the authoritative package name deb ABSENT resolved the package name from the URL filename while PRESENT installed under dpkg-deb's canonical Package field. When the two differ (non-standard filename, custom artifact store), flipping an action to ABSENT checked the wrong name and silently reported 'already not installed' while the package stayed. ABSENT now prefers the canonical name (download + dpkg-deb), matching PRESENT, and falls back to the URL-filename heuristic only when the download fails — preserving the graceful dead-URL behavior the path was designed for. rpm ABSENT has no reliable URL name heuristic, so its dead-URL case now returns an actionable error instead of a bare 'download: 404'. Tests pin the deb URL fallback parser, including the no-underscore case it must refuse rather than mis-target. * fix(repo,update): stop dropping apt index-refresh and dist-upgrade errors action_repository.go discarded apt.Update()'s error after writing a repo file — the only sibling the F012 sweep missed (dnf/pacman/zypper all Warn on refresh failure). A typo'd URL or unreadable key made the index refresh fail while the action still reported ExitCode 0 / SUCCESS. action_update.go discarded apt.DistUpgrade()'s error entirely, so a half-completed held-back/kernel transition reported a clean SUCCESS. Warn + surface the apt update failure (keeping the config that landed), and errors.Join the dist-upgrade error into the returned error so the action reports FAILED. Sibling sweep confirms no other dropped package-manager errors remain in these files. * fix(flatpak): converge pin on already-installed; fail on pin failure Both flatpak PRESENT paths (system + per-user) early-returned/skipped on 'already installed' without ever applying the mask, so flipping Pin=true on an existing app — or a pin lost out-of-band — never converged. And a pin failure was only a Stdout warning / Warn log on a SUCCESS result, so fleets reported compliant while apps stayed unpinned and free to update. Add ensureFlatpakMasked (idempotent, injectable runner): it masks only when not already masked, reports whether it changed state, and returns a mask failure as a real error — mirroring action_package.go's pin contract. Wire it into all four PRESENT sites (system already-installed, system fresh install, per-user already-installed, per-user fresh install). Unit tests cover converge / idempotent / fail-as-error. * fix(executor): run per-user shell with the user's PATH, not root's runAsUserStreaming passed a non-empty env to the SDK's RunStreaming, which injected the agent's (root's) PATH for the child — so user-scoped scripts ran with root's PATH and the user's ~/.local/bin was ignored, against desktop.EnvFor's documented contract. Use the new sysexec.RunStreamingChildPath with desktop.UserPath(s) so the child gets the target user's curated PATH (PATH is blocklisted from envVars, so it must be passed as the trusted child PATH). * sec(enroll): serialize enrollment + cache status to stop Argon2id DoS Two issues on the 0666 enrollment socket: - Enroll had no mutual exclusion. The rate limiter allows up to 5 concurrent requests, which could each pass the Exists() check, register a duplicate device, race Save (last write wins, key/cert may mismatch the saved creds), and fire onEnrolled more than the single-buffer enrollCh absorbs (wedging the handler). Serialize the whole Enroll body behind enrollMu. - GetEnrollmentStatus ran credStore.Load() — a 64 MiB Argon2id derivation — on every unauthenticated call: a trivial local CPU/memory DoS. Cache the device id after the first load and serialize that one load behind statusMu so a concurrent flood collapses to a single derivation. Un-enrolled status stays a cheap stat. Tests (incl. a concurrent flood under -race) pin at-most-one Load. * fix(store): set SQLite pragmas on the DSN, not one connection foreign_keys is per-connection in SQLite and OFF by default. Setting it with a single db.Exec only affected whichever pooled connection served that call, so concurrent readers opened connections with FK enforcement OFF — ON DELETE CASCADE (results -> actions, group_members) fired or not depending on which connection a statement landed on, risking orphaned rows. Move foreign_keys, journal_mode(WAL) and a 5s busy_timeout onto the DSN so every pooled connection gets them (busy_timeout also lets the tty/luks CLI wait for the daemon's writer instead of failing with SQLITE_BUSY). Test forces multiple live connections and asserts each enforces foreign keys. * fix(luks): persist user_passphrase mode so reconcile converges reconcileDeviceKey's USER_PASSPHRASE branch revoked the current key and then no-op'd (the user enrolls the passphrase via the CLI flow) but never persisted device_key_type. localState stayed "none", so currentType != desiredType held on every tick and the action reported changed=true forever, emitting spurious 'device-bound key updated' metadata until the user happened to run the CLI. Persist device_key_type="user_passphrase" (mirroring enrollTpm's persist of "tpm") so the mode converges. Test pins persistence + idempotent second reconcile. * fix(agent): reload rotated certificate before each reconnect startCertRotation renews the mTLS cert at 80% lifetime and persists it to disk, but runAgent built every reconnect's mTLS client from the in-memory creds loaded once at startup. A long-lived agent whose connection dropped after the old cert's NotAfter would then present the stale (expired) cert on every reconnect and fail the handshake forever — offline until a manual restart — while a valid renewed cert sat unused on disk. Thread credStore into runAgent and reload credentials from disk before each reconnect (first connection still uses the freshly-loaded creds; a transient reload error falls back to the in-memory copy). Tests cover picking up a rotated cert and the fallback-on-error path. * chore: remove dead internal/validate package internal/validate.Struct had zero callers (dead since at least March). Nothing in the agent validated server-sent action structs through it before fields were used as file paths / exec args, so it was a false reassurance that 'the agent validates input' — actual validation is ad-hoc per-executor (validRepoName, validDebPkgName, the SSH-key newline guards, etc.). Delete it rather than leave a no-op safety net; if a central validation boundary is wanted it should be wired into the dispatch path deliberately. * docs(executor): correct self-update comment to match copy-then-swap SafeBackupAndReplace now backs up by copy (SDK fix), so the live binary is never renamed away before the new write. Update the agent_update comment that claimed the old rename-first behavior left the binary intact 'because the new write happens last' — it now genuinely does. * fix(luks-cli): apply root privilege backend in set-passphrase The 'luks set-passphrase' CLI runs the same privileged cryptsetup helpers (WipeTPM/KillSlot/AddKeyToSlot) as the daemon, but never called applyBackendOverrides — so the SDK stayed on its default sudo (-n) backend even when running as root, and every cryptsetup call failed on hosts where root can't sudo -n (openSUSE Defaults targetpw), the exact quirk the root backend was added to avoid. Extract setPrivilegeBackend / setEncryptionBackend from applyBackendOverrides and add applyCLIBackends (privilege + encryption, no service backend) for the standalone subcommands; call it at the top of runLuksSetPassphrase. Test pins root/doas/uid-0 resolution; existing applyBackendOverrides tests unchanged. * fix: atomic downloads, UTC sync timestamps, checked row iteration - downloadFile now streams into a temp file in the destination dir and atomically renames over dest only on full success. os.Create truncated dest in place first, so a 404 / partial / checksum-mismatch destroyed an existing file (a working AppImage being upgraded was left as nothing). Also compare the checksum with EqualFold+TrimSpace so an uppercase/padded-but-correct hash no longer fails the install, matching the idempotency skip-check. - store sync-path timestamps use time.Now().UTC() (RecordExecution and the upsert cursors) so they compare correctly against the UTC next_execute_at values rather than skewing by the host offset. - check rows.Err()/groupRows.Err() after the three SyncActions / SyncStandaloneAndGrouped transaction loops; a truncated cursor would otherwise mis-classify still-present actions/groups as new and re-run them. Tests cover the download cases. * fix(agent): stop double-sending offline results on reconnect An action executed while disconnected is both persisted (unsynced) and buffered in the scheduler results channel. On reconnect syncPendingResults runs first and sends + marks the stored copy synced; then sendScheduledResults drains the same execution still sitting in the channel and sends it again. The wire ActionResult carries no result id, so the server sees two distinct result events per offline execution on every reconnect. Add Store.IsResultSynced and skip an already-synced result in sendScheduledResults (a missing row counts as handled). Test pins the unsynced -> synced -> missing transitions. * fix(install): caps for in-process root, download-before-stop, drop removed deploy setup - CapabilityBoundingSet adds CAP_KILL, CAP_SETFCAP, CAP_NET_RAW. After the root-mode refactor the agent's OWN syscalls are bound by this set (not just its children), so without CAP_KILL terminal-session teardown of pm-tty-* process groups returns EPERM (procs leak), without CAP_SETFCAP dpkg/rpm can't set file capabilities during installs, and without CAP_NET_RAW file-cap binaries like ping exec-fail. - main() downloads/stages the new binary BEFORE stopping the running agent. download_binary stages atomically, so a failed download under set -e now aborts with the existing agent still running instead of leaving the device offline with no binary. - Makefile deploy dropped 'sudo $(REMOTE_BIN) setup' — the setup subcommand was removed in #93, so deploy errored. It is now a binary swap + restart (use 'make install' for a full install). * fix(deb): hard-error on parse failure of a successfully-downloaded .deb CodeRabbit: in the ABSENT name resolution, a dpkg-deb parse failure AFTER a successful (checksum-validated) download is a real corruption/format error, not a stale URL — falling back to the URL-filename heuristic there could target and remove the wrong package. Only fall back to the URL heuristic when the DOWNLOAD itself fails. * harden: fd-based ~/.ssh ownership, oversize download test, SYNC verify note - setupSSHKeys now obtains an O_NOFOLLOW directory handle via sysfs.OpenRealDir and applies ownership/mode through the fd (fchown/fchmod), and writes authorized_keys ownership via sysfs.FchownNoFollow — a TOCTOU-proof close of F022 rather than the prior 'AssertRealDir + chown -h' (chmod has no -h, so that left the dir-mode path open). - downloadFile: add a test that an oversize streamed body (chunked, no Content-Length) is rejected by the size guard and leaves an existing dest file intact. - handler SYNC path: document why we verify the signature (which binds the action type) but deliberately do NOT inspect params_canonical here — the cross-repo signed-vs-executed gap is tracked in sdk#82. * chore(sdk): repin to merged SDK 214e5965 (#83) — AssertRealDir/OpenRealDir, UserPath, crash-safe SafeBackupAndReplace Picks up the SDK helpers this branch consumes now that sdk#83 is merged to main. Resolves the standalone (GOWORK=off) build's undefined AssertRealDir / OpenRealDir / FchownNoFollow / UserPath / RunStreamingChildPath that the lint CI job hit against the old pin.
6 tasks
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
Closing tech-debt audit (PR G) for the 2026.06 cleanup release. Addresses ~36 of 63 findings from
agent/TECH_DEBT_AUDIT.mdplus the SDK pin bump that picks up manchtools/power-manage-sdk#57.Seven thematic commits:
0fb9477chore(sdk): repin to local SDK checkout, align with new sys/user APIa68b38efix(concurrency): close shared-pointer race, lock bypass, sweeper leak (F002, F003, F004, F019, F020, F036, F042, F043, F045, F048, F053)efb6cd5fix(errors): always log errors across executor + cmd_luks; pin GPG temp under data dir (F012, F024, F050, F054, F056, F061)a1eedc8refactor(executor): query dpkg-deb for the canonical .deb package name (F051)4a777d2fix(security): adopt SDK SafeReplaceFile / SafeBackupAndReplace / RunWithCLocale; drop dead writeUpdateState (F018, F022, F023, F025, F028)ad79281fix: closing cleanup pass — credentials, store, wifi, docs, low-severity quality (F009, F021, F035, F038, F039, F044, F055, F057-F060, F062)c330255chore(sdk): repin SDK to merged PR #57 commit (3bfd7be) + adopt typed StreamType callbackCross-repo dependency
Pairs with manchtools/power-manage-sdk#57 (already merged). The final
chore(sdk)commit swaps the temporaryreplace ... => ../sdk(used during SDK PR #57 development) for the merged-main pseudo-version, and updatesinternal/handler/handler.goto match the SDK's newfunc(streamType StreamType, ...)callback signature (wasintfor source-compat — now typed).go mod tidyauto-bumped the go directive to 1.25.10 for the SDK's CVE-fix toolchain requirement.The matching server PR (manchtools/power-manage-server#193) is in flight in parallel.
Notes for review
git grepconfirmed../agentnot in root go.work) and F063 (toolchain directive in go.mod) were surfaced as commit-message Notes rather than implemented inline — they change every contributor's "run from workspace root" command and need an explicit decision rather than a "best-guess" patch.luks.go/runtime.go/executeAgentUpdate/ integration suite docs are L-effort. Perfeedback_no_deferrals_in_release_window.md, no deferral — but they're large enough to warrant their own dedicated PR with the testcontainer harness for full coverage. Flagged here so the reviewer can decide whether to fold them into this PR or split.Test plan
go build ./...— verified clean locallygo test -short ./...— verified locallySummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores