You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Refactor directory recreation into modular components
Pin GitHub Actions and tidy Codacy docs
Refactor PBS mount guards and network UI flows
Fix nil candidate panic in raw artifact staging
Remove unused internal helpers
Fix ineffectual assignments
Fix Proxmox Backup zombie process filtering
fix: resolve golangci-lint staticcheck findings
Update go.mod
Summary by CodeRabbit
New Features
UI-driven selective restore with smart fstab merge, safer cluster restore, network-safe apply with rollback and mount-guard, PBS datastore & PVE storage directory recreation, and embedded installable documentation.
Bug Fixes & Improvements
Improved error reporting, resource cleanup, and robustness across backup, restore, notification, storage and mount workflows; better handling of edge cases and cancellations.
Chores
Pinned CI actions, updated Go toolchain, and added a Race Detector workflow.
Split the monolithic directory recreation logic into focused, testable components. Parsing of PVE/PBS config files was moved to directory_recreation_config.go (load/parse helpers and normalization for datastore.cfg). PBS-specific preflight/initialization logic, subdir creation and lock handling were extracted into directory_recreation_pbs*.go files, and PVE-specific scaffolding moved to a dedicated pve file. Ownership and permission handling was isolated in directory_recreation_ownership.go, while path/device utilities and ZFS/root-fs safety checks were factored into directory_recreation_paths.go. The main RecreateStorageDirectories/RecreateDatastoreDirectories functions were simplified to use the new loaders/creators; tests updated accordingly.
Pin CI actions to specific commit SHAs for reproducible workflows and security: codecov action pinned to a v6 commit, anchore/sbom-action (syft) and goreleaser action pinned to specific commits. Also normalize list formatting/whitespace in Codacy MCP instructions for clarity; no functional changes to behavior.
Move and refactor PBS mount guard logic into a dedicated apply implementation, splitting responsibilities and adding helper functions for validation, directory setup and bind-readonly guard mounting. Simplify and harden proc path unescaping with small utility helpers. Extract and reorganize network apply UI flow into request/flow types and separate prompt/rollback files, update tests to use test tool helpers and a fake command runner, and add multiple restore/workflow UI stubs. Also add package comments and small housekeeping changes (embed.go, imports). This improves separation of concerns, testability and readability of mount guard and network apply codepaths.
Validate nil backup candidates before starting raw artifact staging trace logging.
This prevents copyRawArtifactsToWorkdirWithLogger from dereferencing a nil candidate before returning its intended error, and adds a regression test covering the nil candidate case.
Remove stale internal helper functions left behind by earlier refactors.
This cleans up unused environment detection, container detail formatting, symlink overlay, and restore abort helper code. Also removes the now-unused input import and verifies the cleanup with unused lint and the full Go test suite.
Remove ineffectual assignments reported by golangci-lint.
This simplifies config upgrade insertion logic, cleans up mount guard request normalization, and keeps executable permission checks while discarding unused refreshed file info. Verified with ineffassign lint and the full Go test suite.
Apply the Proxmox Backup zombie-process filter before suspicious process matching.
Previously the filter ran at the end of the process scan loop, so its continue had no practical effect. This moves the check ahead of suspicious signature detection and adds regression coverage for skipped zombie processes and warned non-zombie matches.
Clean up staticcheck findings across runtime and tests, including deprecated tar usage, error string formatting, fmt.Fprintf conversions, De Morgan simplifications, and embedded selector cleanup.
Strengthen fragile tests around nil contexts, NIC repair results, identity detection, and security checks. Make PVE cluster detection tests deterministic by avoiding dependence on the host pvecm command.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Centralizes CloseIntoErr and propagates close errors; adds extensive orchestrator restore UI and PBS/PVE directory-recreation/mount-guard code; tightens collectors, notify/HTTP, TUI wrappers, many tests, and pins CI actions.
Changes
Main Cohort
Layer / File(s)
Summary
Core close helper & wide usage internal/closeerr/*, internal/orchestrator/close_error.go, internal/metrics/prometheus.go, internal/backup/*
Adds CloseIntoErr and migrates many defer f.Close() sites to closeIntoErr(&err, ...) with named returns so Close() failures can be surfaced.
New config parsing, path heuristics, preflight safety checks, ownership/chmod and lock-file management, and PBS/PVE init logic; safety gates prevent unsafe filesystem mutations.
Collectors / system probes internal/backup/collector_*.go, internal/backup/collector_system.go
Introduce best-effort probe helpers and availability gating; change many collection steps from fire-and-forget to error-propagating safeCmdOutput; add tests for pvesh/lsmod/smartctl behaviors.
Archiver streaming goroutines and add-to-tar paths now surface write/close results; checksum/optimizations use named returns with closeIntoErr; storage copy cleanup tightened.
Notify / templates / HTTP internal/notify/*
Refactor template builders to use fmt.Fprintf into builders; ensure HTTP response bodies are closed and errors handled in relay/gotify/webhook; tests updated to ignore w.Write returns.
TUI wrapper & components internal/tui/...
Switch many usages to components.Form wrapper and promoted methods; update ListFormItem/panel helpers; adjust input-capture and focus handling; update tests to use wrapper API.
Tests / cleanup / minor rewrites many *_test.go
Harden tests to fail fast on setup I/O, replace raw defers with explicit ignore-close wrappers, add new tests (collector_system, security, restore_workflow_ui_extract, decrypt), and replace many fmt.Sprintf→fmt.Fprintf uses.
CI workflows & tooling .github/workflows/*, go.mod
Add a Race Detector workflow; pin many GitHub Actions steps to specific commit SHAs; bump Go toolchain in go.mod to go1.25.10.
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
tis24dev/proxsave#204: Overlapping orchestration/collector/notify and close-error handling edits.
tis24dev/proxsave#185: Related edits touching internal/notify/email_relay.go and HTTP response handling.
"A rabbit's note on cleanup day:"
"I hopped through code with gentle paws 🐇,"
"Closed stray files and fixed small flaws."
"I nudged each defer to mind the err,"
"Sorted tests and workflows with a cheerful burr."
"Now logs are tidy — time for carrots and applause!"
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR performs an extensive refactor of core ProxSave workflows and modularizes directory recreation logic; however, it is currently not up to standards. Critical security issues were identified regarding overly permissive file permissions in /tmp and potential path traversal in PBS datastore lock file creation. Additionally, the removal of tar.TypeRegA checks in archive parsers may break compatibility with legacy backups.
Architecturally, the PR introduces extreme complexity in several files (e.g., network_apply_workflow_ui_rollback.go and decrypt_test.go), and the Export function in Prometheus metrics has become a maintenance risk. Due to the broad scope of this PR and the lack of a coverage report, there is a high risk of regression in the refactored TUI and mount guard flows.
About this PR
The absence of a coverage report for this PR prevents verification of whether new logic paths, particularly in the refactored TUI and mount guard flows, are adequately tested.
The PR scope is exceptionally broad. Combining major architectural refactoring with bug fixes and linting increases the risk of regression. Future PRs should be decomposed into smaller, atomic units.
Test suggestions
Verify directory recreation logic for PVE storage structures
Verify directory recreation logic for PBS datastores with ZFS safety checks
Verify protection against nil candidate panic in raw artifact staging
Verify that Proxmox Backup zombie processes are correctly skipped in security scans
Ensure closeIntoErr correctly captures and wraps errors from io.Closer implementations
Missing automated XSS validation for dynamic variables in notification templates
Missing unit tests for tar.TypeRegA compatibility in archive parsers
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing automated XSS validation for dynamic variables in notification templates
2. Missing unit tests for tar.TypeRegA compatibility in archive parsers
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
The lock file is created with 0o640 permissions; however, 0o600 is safer for files containing internal system metadata like PIDs. Additionally, the construction of lockPath from datastorePath (sourced from configuration) should be validated to ensure it remains within the intended datastore root, preventing potential path traversal attacks.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Setting /tmp/proxsave permissions to 0o755 is overly permissive. This allows any local user to list the directory contents, which might include sensitive state or diagnostic data. Using 0o700 is recommended to restrict access only to the owner.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The removal of the tar.TypeRegA check reduces robustness. Many older or non-standard tar implementations use TypeRegA for regular files. Consider restoring the check for both types.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The removal of the tar.TypeRegA check reduces robustness. Many older or non-standard tar implementations use TypeRegA for regular files. Consider restoring the check for both types.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Dynamic variables are written directly to the HTML body without escaping, which poses a formatting and security risk. All variable injections in HTML should be wrapped in escapeHTML().
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The Export function is currently too complex (CCN 33) and long (163 lines). Refactoring this into smaller helper methods for each logical metric group (e.g., writeCoreMetrics, writeStorageMetrics, writeStaticInfo) would improve readability and testability.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The naming of 'ensureGuardTargetUnmounted' is misleading as it returns nil when the target is mounted. It should either return an error if mounted or be renamed to reflect its actual behavior as a mount status check.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Suggestion: The closeIntoErr helper is duplicated across multiple packages. Consider centralizing this logic in a shared internal package (e.g., internal/utils) to ensure consistent error handling behavior and reduce maintenance overhead.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Nitpick: Redundant mount checks: ensureGuardTargetUnmounted and isAlreadyMounted both invoke isMounted. This logic should be consolidated to avoid multiple calls to the same underlying status check.
Root directory is skipped during base-dir discovery.
At Line 87, the loop exits before evaluating dir == "/" (or platform separator root), so "/env" and "/script" are never checked. That’s a behavior change from common break-at-root traversal and can miss valid root-based installs.
Suggested fix
- for dir != "" && dir != "." && dir != string(filepath.Separator) {+ for dir != "" && dir != "." {
if info, err := os.Stat(filepath.Join(dir, "env")); err == nil && info.IsDir() {
baseDir = dir
break
}
if info, err := os.Stat(filepath.Join(dir, "script")); err == nil && info.IsDir() {
baseDir = dir
break
}
parent := filepath.Dir(dir)
if parent == dir {
break
}
dir = parent
}
🤖 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/proxsave/runtime_helpers.go` around lines 87 - 101, The loop that
searches for a base directory (using variable dir, baseDir and checks via
os.Stat(filepath.Join(dir, "env")) and os.Stat(filepath.Join(dir, "script")))
stops before checking the filesystem root; change the traversal to perform the
env/script checks for the current dir first and only break after computing
parent := filepath.Dir(dir) and comparing parent == dir, so the root directory
is evaluated too (i.e., use an unconditional loop over dir with the stat checks
before the parent == dir break). Ensure you still set baseDir = dir and break
when a match is found.
Complete the receiver method refactor for consistency.
InfoPanel (line 44) was updated to use panel.SetBackgroundColor(...) instead of panel.Box.SetBackgroundColor(...), aligning with the PR's goal of using receiver methods. However, SuccessPanel, ErrorPanel, and WarningPanel still directly access the embedded Box field via panel.Box.SetBorderColor(...) and panel.Box.SetTitleColor(...).
Since Panel embeds *tview.Box, these methods are available directly on the Panel receiver. Update these functions for consistency.
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/tui/components/panel.go` around lines 48 - 70, SuccessPanel,
ErrorPanel, and WarningPanel still call methods on the embedded Box via
panel.Box.SetBorderColor/... and panel.Box.SetTitleColor/...; update them to
call the receiver methods on Panel directly (e.g., panel.SetBorderColor(...),
panel.SetTitleColor(...)) to match the refactor done in InfoPanel and avoid
directly referencing panel.Box in SuccessPanel, ErrorPanel, and WarningPanel.
Accept both tar.TypeReg and tar.TypeRegA for regular file validation.
The current validation rejects tar.TypeRegA, which is a valid (though deprecated) alternate representation of regular files defined in Go's archive/tar package. Go's tar implementation explicitly supports this for backwards compatibility with legacy tar archives. Rejecting TypeRegA entries will prevent restoration from older tar archives that use this deprecated format.
Update the validation to accept both:
Suggested fix
ifheader.Typeflag!=tar.TypeReg&&header.Typeflag!=tar.TypeRegA {
returnnil, fmt.Errorf("archive entry %s is not a regular file", header.Name)
}
🤖 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/orchestrator/restore_decision.go` around lines 161 - 167, The file's
readRestoreDecisionMetadata function currently rejects tar entries with
header.Typeflag == tar.TypeRegA even though TypeRegA is a valid (deprecated)
alternate regular-file flag; update the validation in
readRestoreDecisionMetadata to accept both tar.TypeReg and tar.TypeRegA by
changing the conditional that checks header.Typeflag so it only errors when
header.Typeflag is neither tar.TypeReg nor tar.TypeRegA, keeping the same error
message and behavior otherwise.
Empty trailing chunk file when source size is an exact multiple of chunkSize.
In splitFile, after writeChunk returns done=false, the loop unconditionally creates the next chunk file in writeChunk (via os.OpenFile(chunkPath, O_CREATE|...)) before checking for EOF. If the source has exactly N * chunkSize bytes, the previous call returned false, nil once written >= limit, and the following call creates …(N+1).chunk, reads 0/io.EOF, then returns true, nil, leaving a zero-byte chunk on disk.
A simple fix is to peek/avoid creating the next file when no bytes were read:
♻️ Proposed fix
func writeChunk(src *os.File, chunkPath string, buf []byte, limit int64) (done bool, err error) {
- out, err := os.OpenFile(chunkPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, defaultChunkFilePerm)- if err != nil {- return false, err- }- defer closeIntoErr(&err, out, "close chunk file")-- var written int64+ var (+ out *os.File+ written int64+ )+ openOut := func() error {+ if out != nil {+ return nil+ }+ f, oerr := os.OpenFile(chunkPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, defaultChunkFilePerm)+ if oerr != nil {+ return oerr+ }+ out = f+ return nil+ }+ defer func() {+ if out != nil {+ closeIntoErr(&err, out, "close chunk file")+ }+ }()
for written < limit {
remaining := limit - written
if remaining < int64(len(buf)) {
buf = buf[:remaining]
}
n, err := src.Read(buf)
if n > 0 {
+ if oerr := openOut(); oerr != nil {+ return false, oerr+ }
if _, wErr := out.Write(buf[:n]); wErr != nil {
return false, wErr
}
written += int64(n)
}
if err != nil {
if err == io.EOF {
- return true, nil+ return true, nil
}
return false, err
}
- if written >= limit {- return false, nil- }
}
return false, nil
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backup/optimizations.go` around lines 246 - 304,
splitFile/writeChunk create an empty trailing chunk when source size is an exact
multiple of chunkSize; modify writeChunk (or its call pattern) so it does not
create the output file before confirming there are bytes to write: perform an
initial non-destructive check/peek from src (e.g., Read into the provided buf or
use src.ReadAt/Seek to test for EOF) and only open/create chunkPath
(os.OpenFile) once at least one byte has been read; update writeChunk
signature/behavior accordingly so splitFile's loop stops without producing a
zero-length chunk when done==true after an initial EOF check.
🧹 Nitpick comments (13)
embed.go (1)
37-43: ⚡ Quick win
Document that returned Data slices should not be modified.
The defensive copy creates a new slice of DocAsset structs, but the underlying byte arrays in the Data fields are shared with the cached installableDocs. While the embedded data is likely stored in read-only memory (making modifications panic), explicitly documenting this prevents future confusion.
📝 Suggested documentation enhancement
// InstallableDocs returns the list of documentation files embedded in the
-// binary that should be written to the installation root.+// binary that should be written to the installation root. The returned+// DocAsset structs are copies, but their Data fields share the underlying+// byte arrays with the cached assets and must not be modified.
func InstallableDocs() []DocAsset {
🤖 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 `@embed.go` around lines 37 - 43, The InstallableDocs function returns a
shallow copy of the installableDocs slice but the DocAsset.Data byte slices are
shared; update the function comment for InstallableDocs to explicitly state that
callers must treat the returned DocAsset.Data as read-only and must not modify
the byte slices (or allocate copies themselves if mutation is required),
referencing DocAsset and the shared installableDocs backing data so future
maintainers understand the shared underlying memory semantics.
internal/storage/secondary.go (1)
295-298: ⚡ Quick win
Potential double-close on error path.
If tempFile.Close() fails at line 296, the function returns immediately at line 297, but the deferred cleanup at lines 245-249 will attempt to close tempFile again (since it's still non-nil). While this may only result in a harmless "already closed" error, it's cleaner to set tempFile = nil before checking the close error to prevent the retry.
♻️ Suggested fix to prevent double-close
- if err := tempFile.Close(); err != nil {- return fmt.Errorf("failed to close temporary file %s: %w", tempName, err)- }- tempFile = nil+ closeErr := tempFile.Close()+ tempFile = nil+ if closeErr != nil {+ return fmt.Errorf("failed to close temporary file %s: %w", tempName, closeErr)+ }
🤖 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/storage/secondary.go` around lines 295 - 298, The tempFile close
error path can cause a double-close because the deferred cleanup still sees
tempFile non-nil; in the block that calls tempFile.Close() (the local tempFile
variable in internal/storage/secondary.go), set tempFile = nil immediately after
calling Close() (i.e., before checking/returning on the error) so the deferred
cleanup won't try to close it again; modify the Close/error handling around
tempFile.Close() accordingly (reference tempFile and the deferred cleanup block
that closes it).
internal/tui/components/form.go (1)
56-59: ⚡ Quick win
Inconsistent wrapper usage in AddPasswordField.
Line 58 still calls f.Form.AddPasswordField(...) directly, while the surrounding methods (lines 51, 82, 109, 121-122) were updated to use the wrapper receiver (f.AddInputField(...), f.AddButton(...), etc.). For consistency with the broader refactor pattern, consider using f.AddPasswordField(...) instead.
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/tui/components/form.go` around lines 56 - 59, The AddPasswordField
wrapper currently calls the embedded form method directly
(f.Form.AddPasswordField) which is inconsistent with other wrappers; change the
call to use the wrapper receiver (call f.AddPasswordField(...) instead of
f.Form.AddPasswordField(...)) so it follows the same refactor pattern as
AddInputField/AddButton; ensure you pass the same arguments (label, "",
fieldWidth, '*', nil) when updating the call and verify there is no accidental
recursion by ensuring the wrapper delegates to the underlying form
implementation if a different internal helper exists.
Don't report changed=true when the directory removal failed.
When os.Remove(lockPath) returns an error, this returns (true, err). Callers currently discard changed on error, so this isn't observable today, but it's a leaky contract: a future caller using changed to roll back / log progress will be misled. Same is true for the rename branch — only return true after the operation actually succeeded.
♻️ Proposed cleanup
if len(entries) == 0 {
logger.Warning("PBS datastore lock: %s is a directory (invalid); removing and recreating as file", lockPath)
- return true, os.Remove(lockPath)+ if err := os.Remove(lockPath); err != nil {+ return false, fmt.Errorf("remove invalid empty lock dir %s: %w", lockPath, err)+ }+ return true, nil
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/directory_recreation_pbs_lock.go` around lines 60 - 76,
The function removeOrRenamePBSDatastoreLockDirectory currently returns (true,
err) when os.Remove or os.Rename fails; change it so the function only returns
true when the filesystem operation actually succeeded: call os.Remove(lockPath),
check its error and if non-nil return (false, err) instead of (true, err);
similarly, after attempting os.Rename(lockPath, backupPath) only return (true,
nil) when err == nil and return (false, err) on failure; keep the existing log
messages and preserve the backupPath behavior in the rename branch.
internal/metrics/prometheus.go (1)
81-242: ⚡ Quick win
Reduce the repetitive write/wrap boilerplate.
The 11 writeMetric + 8 fmt.Fprintf blocks all share the same error-wrapping pattern (write metrics file %s: %w, tmpPath). A small closure capturing f and tmpPath would eliminate ~70 lines of near-duplicate code and make it harder to introduce inconsistency in future metrics.
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/metrics/prometheus.go` around lines 81 - 242, Replace the repetitive
metric-writing error-wrapping with a small helper closure that captures f and
tmpPath and maintains a sticky error: add a wrap(err error) error that sets and
returns a single writeErr, refactor the existing
writeMetric(name,mtype,help,value) to check writeErr and call wrap on any
fmt.Fprintf/Fprintln failures, and add a writef(format string, a ...any) helper
that does the same for the ad-hoc fmt.Fprintf calls used for per-location and
info metrics; then replace all direct fmt.Fprintf/Fprintln blocks and the
current writeMetric usages to return writeErr directly instead of rewrapping
with fmt.Errorf("write metrics file %s: %w", tmpPath, err). Ensure symbols
modified are writeMetric, wrap (or writeErr), writef and use tmpPath and f
captured by the closures.
internal/backup/archiver.go (1)
25-32: ⚖️ Poor tradeoff
Consider centralizing closeIntoErr in a shared internal package.
This helper is defined here and (per the PR-wide refactor) also in internal/orchestrator/close_error.go, with identical semantics. Two copies will drift. Moving it into a small shared package (e.g. internal/ioutil or internal/closeerr) would eliminate the duplication and keep the contract uniform across backup, orchestrator, and any future callers.
Not blocking — defer if it requires touching too many import sites for this PR.
🤖 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/backup/archiver.go` around lines 25 - 32, The helper closeIntoErr is
duplicated; extract it into a small shared internal package (e.g.
internal/closeerr or internal/ioutil) and replace the copies in both backup and
orchestrator with calls to the shared implementation; create the new package
with the same semantics (either keep the function unexported and call via
package-scoped name if colocated, or export as CloseIntoErr and update callers),
update imports in files that used the old duplicate (e.g. functions currently
calling closeIntoErr in backup and the file in orchestrator), remove the
redundant definitions, and run tests to ensure behavior is unchanged.
Predictable tmpPath next to the target — consider os.CreateTemp in the same dir.
fmt.Sprintf("%s.proxsave.tmp", path) is a fixed name. Concurrent normalizations or a stale leftover from a crashed run will clash, and on os.Rename failure (line 39) the retained _ = os.Remove(tmpPath) only handles the new-temp case. Using os.CreateTemp(filepath.Dir(path), "datastore.cfg.proxsave-*") to mint a unique file in the same directory keeps os.Rename atomic and avoids the clash.
🤖 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/orchestrator/directory_recreation_pbs_config.go` around lines 35 -
42, Replace the predictable tmpPath logic with a unique temp file created in the
same directory using os.CreateTemp: call os.CreateTemp(filepath.Dir(path),
"datastore.cfg.proxsave-*") to obtain a unique file handle/name, write the
normalized contents to that temp file (ensure file mode is set via os.Chmod if
needed), close the temp file, then os.Rename(tempName, path) to atomically
replace the target; on any error remove the temp file (os.Remove) and return the
wrapped error. Locate and update the block around tmpPath, os.WriteFile and
os.Rename in directory_recreation_pbs_config.go to implement this change.
internal/orchestrator/decrypt.go (1)
411-425: 💤 Low value
Minor: temp file is leaked if tmpFile.Close() fails.
os.CreateTemp has already created the file on disk before Close() is called. If Close() returns an error you bail out without removing tmpPath, and no cleanup is returned to the caller, so the temp file lingers. Close on a fresh empty file is unlikely to fail in practice, but a small belt-and-suspenders cleanup keeps /tmp/proxsave tidy.
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/orchestrator/decrypt.go` around lines 411 - 425, The tmp file
created by os.CreateTemp (tmpFile/tmpPath) can leak if tmpFile.Close() fails
because you return before installing or running cleanup; ensure the temp is
removed on any error after creation: define cleanup (closure referencing
tmpPath) immediately after tmpPath is set, or on Close() error explicitly call
os.Remove(tmpPath) (and log any remove error) before returning, and still return
the cleanup function to the caller; update the code around tmpFile, tmpPath,
tmpFile.Close(), and cleanup so that any early return after creation always
removes the temp or provides a cleanup callback.
Minor: redundant chown of basePath when it's already in dirsToFix.
If computeMissingDirs(basePath) returns the freshly-missing base path, basePath appears in dirsToFix and is chowned in the loop, then chowned again unconditionally on line 214. It's idempotent so this is not a defect, just a small redundancy you can de-duplicate if you want — skip-worthy otherwise.
🤖 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/orchestrator/directory_recreation_pbs.go` around lines 168 - 217,
The applyPBSDatastoreOwnership function currently always calls
setDatastoreOwnership(basePath) after iterating dirsToFix, which can duplicate
ownership work when basePath is already present in dirsToFix; update
applyPBSDatastoreOwnership to check whether basePath is already in dirsToFix (or
deduplicate dirsToFix) and only call setDatastoreOwnership(basePath) when it was
not already processed, keeping the existing logger.Warning behavior on errors
and the Debug log for count/context.
LGTM — desired-state refactor is a real clarity win.
Splitting the apply path into loadPBSNotificationDesiredState → strict cleanup → syncPBSNotificationEndpoints / syncPBSNotificationMatchers makes the strict-mode contract obvious, and centralizing the listing in listPBSNotificationIDs removes duplicated --output-format=json plumbing. The redaction wiring (redactFlags + redactIndex) is preserved end-to-end into runPBSManagerRedacted.
Two small notes (skip-or-take):
upsertPBSNotificationEndpoint and upsertPBSNotificationMatcher use %v when combining the create + update errors. That's a deliberate trade-off (no errors.Is on either) but readable; errors.Join(err, upErr) would also work if you ever need programmatic inspection.
pbsEndpointRedactIndexes hard-codes []int{6} for gotify. A named constant explaining "token positional index in notification endpoint gotify create <name> <server> <token> ..." would help future readers if positional args ever shift.
🤖 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/orchestrator/pbs_notifications_api_apply.go` around lines 28 - 330,
Replace the opaque combined-error fmt.Errorf in upsertPBSNotificationEndpoint
(the error returned in the create/update failure path) and
upsertPBSNotificationMatcher with an error that wraps the joined errors using
errors.Join and %w so callers can inspect them (e.g. return fmt.Errorf("endpoint
%s:%s: %w", typ, name, errors.Join(err, upErr)) and similarly for matcher), and
replace the magic number in pbsEndpointRedactIndexes by introducing a named
constant (e.g. gotifyTokenRedactIndex) and return that constant instead of
[]int{6} so the meaning ("token positional index in `notification endpoint
gotify create <name> <server> <token> ...`") is documented next to the constant;
update references in pbsEndpointRedactIndexes accordingly.
Similar to the pipe cleanup, the anonymous closure pattern for file cleanup seems unnecessarily verbose for test code unless addressing a specific linter requirement.
Also applies to: 276-276
🤖 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/orchestrator/encryption_exported_test.go` at line 248, The anonymous
closure used to close the test file (defer func() { _ = f.Close() }()) is
unnecessarily verbose; replace it with a simple defer f.Close() in the test(s)
in encryption_exported_test.go (references: variable f and its close calls
around the current occurrences) and make the same replacement at the other
occurrence noted (around line 276) to simplify the cleanup pattern.
180-187: 💤 Low value
Consider simpler error discard pattern.
The deferred anonymous closures (defer func() { _ = ...Close() }()) explicitly discard errors, which is appropriate for test cleanup. However, unless required by a linter, the simpler pattern defer func() { _ = f.Close() }() or even direct defer f.Close() should suffice in test code.
If this change addresses a specific linter requirement (e.g., ineffectual assignment), consider documenting that or using a nolint directive for clarity.
🤖 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/orchestrator/encryption_exported_test.go` around lines 180 - 187,
Replace the verbose deferred anonymous closures that discard Close() errors with
the simpler pattern; locate the variables inR, inW, outR, outW in
encryption_exported_test.go (the test cleanup block) and change the four lines
like `defer func() { _ = inR.Close() }()` to simple `defer inR.Close()` (and
similarly for inW, outR, outW); if a linter requires explicit error discard,
instead use the shorter `defer func() { _ = inR.Close() }()` form or add a
nolint with a brief comment.
The 90-second timeout for fstab merge confirmation is hardcoded. If this timeout needs to vary based on context or configuration, consider extracting it as a parameter or constant.
🤖 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/orchestrator/restore_workflow_ui_fstab.go` at line 36, The hardcoded
90-second timeout passed to ui.ConfirmFstabMerge should be made configurable:
replace the literal 90*time.Second with a named constant or a configurable
parameter (e.g., fstabMergeTimeout or cfg.FstabMergeTimeout) and update the call
site in restore workflow where ConfirmFstabMerge is invoked; ensure the new
constant/field has a sensible default and is documented so callers can override
it when creating the orchestrator or UI.
🤖 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 @.github/instructions/codacy.instructions.md:
- Around line 10-12: The markdown bullets for `rootPath`, `file`, and `tool`
have one extra leading space causing MD005 failures; normalize indentation by
removing the extra leading space so these list items align with their sibling
bullets (apply the same fix for the other affected groups listed: lines
referencing the other bullet ranges), ensuring all top-level bullets use the
same indentation level throughout the document.
In `@internal/backup/collector_system.go`:
- Around line 529-535: These inventory probes (calls to c.safeCmdOutput with
commandSpec and filepath.Join(commandsDir, "...")) must be best-effort: do not
let their errors abort the whole collection. For each nonessential probe (e.g.,
hostname -f, lsusb, systemctl, dmidecode, sensors) wrap the execution in a
lightweight capability check (use exec.LookPath("cmd") for presence, uid check
or /sys/device checks for dmidecode, and container/service checks for systemctl)
and if the capability is missing log a warning/debug and skip running it; if you
still call c.safeCmdOutput, swallow its error (log it) instead of returning it
so collection continues. Update the sites where these probes occur (the blocks
invoking c.safeCmdOutput with commandSpec and commandsDir for those utilities)
to follow this pattern.
In `@internal/checks/checks.go`:
- Around line 378-382: The defer that logs f.Close() errors should instead treat
Close() failure as a real acquisition failure: modify the function that opens
the lock file (the scope with the deferred func referencing f and lockPath) to
capture and propagate Close() errors (e.g., convert the function to use a named
error return or check Close() immediately before returning success) so that if
f.Close() returns an error you return a non-nil error and do not report the lock
as successfully acquired; keep the log but ensure the Close error prevents
success.
In `@internal/cli/args.go`:
- Around line 212-223: The help writer prints most text to the provided writer w
but calls flag.PrintDefaults(), which writes to the global flag output instead;
update printHelp to temporarily set the flag package output to w before calling
flag.PrintDefaults() and restore the original output afterward so all help text
goes to w; locate the printHelp function and use
flag.CommandLine.Output()/SetOutput or flag.CommandLine.SetOutput(w) with
backing save/restore of the previous io.Writer around the flag.PrintDefaults()
call to ensure consistent, testable output.
In `@internal/orchestrator/directory_recreation_ownership.go`:
- Around line 72-76: The function ensureDatastoreDirectoryMode currently
swallows all os.Stat errors; change the control flow so real Stat failures are
returned immediately and only the non-directory case is ignored. Specifically,
in ensureDatastoreDirectoryMode replace the existing "if err != nil ||
!info.IsDir() { return nil }" logic with: call os.Stat(path), if err != nil
return err, then check if !info.IsDir() { return nil } so only the non-directory
case skips chmod while ENOENT/EACCES/EIO (Stat errors) propagate.
In `@internal/orchestrator/directory_recreation_pbs_config.go`:
- Around line 29-32: The backup filename generation using backupPath with
nowRestore().Format("20060102-150405") can collide when runs occur within the
same second; update the code that builds backupPath (the filepath.Join call) and
the write logic (os.WriteFile) to produce a unique filename per run—for example,
use os.CreateTemp with a timestamped prefix derived from nowRestore() (or append
nowRestore().UnixNano()/a short random suffix) to avoid collisions, then write
the raw bytes to that temp file (ensuring 0600 permissions) and return errors as
before; adjust references to backupPath, nowRestore, filepath.Join, and
os.WriteFile accordingly.
- Around line 25-32: Change the creation of the proxsave directory and backup
file to avoid world-enumerable directory and predictable shared-tmp races:
replace the os.MkdirAll("/tmp/proxsave", 0o755) call with creation of a private
per-run directory (either use os.MkdirAll with 0o700 on an already-trusted
proxsave runtime path or use os.MkdirTemp("/tmp", "proxsave-") to create a
unique temp dir), then write the backup file to that directory via the
backupPath variable before calling os.WriteFile; also ensure directory
permissions are 0o700 and the backup file remains 0o600 so only the owner can
list or read the backup (adjust logic around nowRestore(), backupPath,
os.MkdirAll/os.MkdirTemp, and os.WriteFile accordingly).
In `@internal/orchestrator/mount_guard_apply.go`:
- Around line 180-215: The mount-state probe errors are currently treated as
"not mounted" (safe to guard) where mounted, err := isMounted(guardTarget)
returns an error; change that so probe failures are considered inconclusive and
do not allow protectOfflineTarget() to proceed: in the block that currently logs
a.warning("PBS mount guard: unable to check mount status...") and returns true,
return false instead and update the log message to indicate an inconclusive
probe. Keep the existing strict checks in targetIsMounted and
targetMovedOffRootFS (they already require err == nil), and ensure
mountAttemptSucceeded still only signals success when those helper methods
return true.
In `@internal/orchestrator/restore_archive.go`:
- Line 274: The defer call defer closeIntoErr(&err, logFile, "close detailed
restore log") can escalate a non-fatal log-close failure into a restore error;
change it so closing the best-effort log does not mutate the outer err—e.g.,
replace the defer with a closure that calls logFile.Close() and, if it returns a
non-nil error, logs or warns about the failure (but does not assign into err) so
the restore result is not overwritten; keep references to closeIntoErr, logFile
and the "close detailed restore log" context to locate the change.
In `@internal/orchestrator/restore_workflow_ui_extract.go`:
- Around line 255-269: The staging directory is created with overly permissive
0o755; change the permissions to 0o700 when creating the stage root in
extractStagedCategories (use restoreFS.MkdirAll(w.stageRoot, 0o700)) and
likewise tighten the parent /tmp/proxsave creation in the code that implements
stageDestRoot / restore_archive.go (replace its MkdirAll mode 0o755 with 0o700);
also audit the exportRoot creation (the export root creation site around line
177) and set its MkdirAll to 0o700 so staged/exported sensitive data is not
world-traversable while extracting via extractSelectiveArchive/atomicFileChmod.
In `@internal/orchestrator/restore_workflow_ui_full.go`:
- Around line 58-63: The validate method in fullRestoreUIFlow currently checks
f.prepared.Manifest.ArchivePath but ArchivePath is a direct field on the
preparedBundle; update the validation in fullRestoreUIFlow.validate to check
f.prepared.ArchivePath == "" (instead of f.prepared.Manifest.ArchivePath) so the
function uses the correct field on the preparedBundle and mirrors other usages
of f.prepared.ArchivePath in the codebase.
In `@internal/orchestrator/restore_workflow_ui_run.go`:
- Around line 55-65: The prepared bundle can be created by prepareBundle(), so
change run() to call prepareBundle() (the function used inside
prepareBundleAndPlan()) directly and immediately defer w.prepared.Cleanup() as
soon as prepareBundle() returns a non-nil w.prepared; then perform the remaining
planning steps (analyzeArchive, confirmCompatibility, selectRestorePlan,
configurePlanForRuntime) and return errors as before. In short: obtain the
prepared bundle first, register defer w.prepared.Cleanup() right after
successful prepareBundle() return, then continue with the rest of
prepareBundleAndPlan's logic; keep runFullRestoreWithUI(w.ctx, w.ui,
w.candidate, w.prepared, ...) and runSelectiveRestore() behavior unchanged so
cleanup always runs.
---
Outside diff comments:
In `@cmd/proxsave/runtime_helpers.go`:
- Around line 87-101: The loop that searches for a base directory (using
variable dir, baseDir and checks via os.Stat(filepath.Join(dir, "env")) and
os.Stat(filepath.Join(dir, "script"))) stops before checking the filesystem
root; change the traversal to perform the env/script checks for the current dir
first and only break after computing parent := filepath.Dir(dir) and comparing
parent == dir, so the root directory is evaluated too (i.e., use an
unconditional loop over dir with the stat checks before the parent == dir
break). Ensure you still set baseDir = dir and break when a match is found.
In `@internal/backup/optimizations.go`:
- Around line 246-304: splitFile/writeChunk create an empty trailing chunk when
source size is an exact multiple of chunkSize; modify writeChunk (or its call
pattern) so it does not create the output file before confirming there are bytes
to write: perform an initial non-destructive check/peek from src (e.g., Read
into the provided buf or use src.ReadAt/Seek to test for EOF) and only
open/create chunkPath (os.OpenFile) once at least one byte has been read; update
writeChunk signature/behavior accordingly so splitFile's loop stops without
producing a zero-length chunk when done==true after an initial EOF check.
In `@internal/orchestrator/restore_decision.go`:
- Around line 161-167: The file's readRestoreDecisionMetadata function currently
rejects tar entries with header.Typeflag == tar.TypeRegA even though TypeRegA is
a valid (deprecated) alternate regular-file flag; update the validation in
readRestoreDecisionMetadata to accept both tar.TypeReg and tar.TypeRegA by
changing the conditional that checks header.Typeflag so it only errors when
header.Typeflag is neither tar.TypeReg nor tar.TypeRegA, keeping the same error
message and behavior otherwise.
In `@internal/tui/components/panel.go`:
- Around line 48-70: SuccessPanel, ErrorPanel, and WarningPanel still call
methods on the embedded Box via panel.Box.SetBorderColor/... and
panel.Box.SetTitleColor/...; update them to call the receiver methods on Panel
directly (e.g., panel.SetBorderColor(...), panel.SetTitleColor(...)) to match
the refactor done in InfoPanel and avoid directly referencing panel.Box in
SuccessPanel, ErrorPanel, and WarningPanel.
---
Nitpick comments:
In `@embed.go`:
- Around line 37-43: The InstallableDocs function returns a shallow copy of the
installableDocs slice but the DocAsset.Data byte slices are shared; update the
function comment for InstallableDocs to explicitly state that callers must treat
the returned DocAsset.Data as read-only and must not modify the byte slices (or
allocate copies themselves if mutation is required), referencing DocAsset and
the shared installableDocs backing data so future maintainers understand the
shared underlying memory semantics.
In `@internal/backup/archiver.go`:
- Around line 25-32: The helper closeIntoErr is duplicated; extract it into a
small shared internal package (e.g. internal/closeerr or internal/ioutil) and
replace the copies in both backup and orchestrator with calls to the shared
implementation; create the new package with the same semantics (either keep the
function unexported and call via package-scoped name if colocated, or export as
CloseIntoErr and update callers), update imports in files that used the old
duplicate (e.g. functions currently calling closeIntoErr in backup and the file
in orchestrator), remove the redundant definitions, and run tests to ensure
behavior is unchanged.
In `@internal/metrics/prometheus.go`:
- Around line 81-242: Replace the repetitive metric-writing error-wrapping with
a small helper closure that captures f and tmpPath and maintains a sticky error:
add a wrap(err error) error that sets and returns a single writeErr, refactor
the existing writeMetric(name,mtype,help,value) to check writeErr and call wrap
on any fmt.Fprintf/Fprintln failures, and add a writef(format string, a ...any)
helper that does the same for the ad-hoc fmt.Fprintf calls used for per-location
and info metrics; then replace all direct fmt.Fprintf/Fprintln blocks and the
current writeMetric usages to return writeErr directly instead of rewrapping
with fmt.Errorf("write metrics file %s: %w", tmpPath, err). Ensure symbols
modified are writeMetric, wrap (or writeErr), writef and use tmpPath and f
captured by the closures.
In `@internal/orchestrator/decrypt.go`:
- Around line 411-425: The tmp file created by os.CreateTemp (tmpFile/tmpPath)
can leak if tmpFile.Close() fails because you return before installing or
running cleanup; ensure the temp is removed on any error after creation: define
cleanup (closure referencing tmpPath) immediately after tmpPath is set, or on
Close() error explicitly call os.Remove(tmpPath) (and log any remove error)
before returning, and still return the cleanup function to the caller; update
the code around tmpFile, tmpPath, tmpFile.Close(), and cleanup so that any early
return after creation always removes the temp or provides a cleanup callback.
In `@internal/orchestrator/directory_recreation_pbs_config.go`:
- Around line 35-42: Replace the predictable tmpPath logic with a unique temp
file created in the same directory using os.CreateTemp: call
os.CreateTemp(filepath.Dir(path), "datastore.cfg.proxsave-*") to obtain a unique
file handle/name, write the normalized contents to that temp file (ensure file
mode is set via os.Chmod if needed), close the temp file, then
os.Rename(tempName, path) to atomically replace the target; on any error remove
the temp file (os.Remove) and return the wrapped error. Locate and update the
block around tmpPath, os.WriteFile and os.Rename in
directory_recreation_pbs_config.go to implement this change.
In `@internal/orchestrator/directory_recreation_pbs_lock.go`:
- Around line 60-76: The function removeOrRenamePBSDatastoreLockDirectory
currently returns (true, err) when os.Remove or os.Rename fails; change it so
the function only returns true when the filesystem operation actually succeeded:
call os.Remove(lockPath), check its error and if non-nil return (false, err)
instead of (true, err); similarly, after attempting os.Rename(lockPath,
backupPath) only return (true, nil) when err == nil and return (false, err) on
failure; keep the existing log messages and preserve the backupPath behavior in
the rename branch.
In `@internal/orchestrator/directory_recreation_pbs.go`:
- Around line 168-217: The applyPBSDatastoreOwnership function currently always
calls setDatastoreOwnership(basePath) after iterating dirsToFix, which can
duplicate ownership work when basePath is already present in dirsToFix; update
applyPBSDatastoreOwnership to check whether basePath is already in dirsToFix (or
deduplicate dirsToFix) and only call setDatastoreOwnership(basePath) when it was
not already processed, keeping the existing logger.Warning behavior on errors
and the Debug log for count/context.
In `@internal/orchestrator/encryption_exported_test.go`:
- Line 248: The anonymous closure used to close the test file (defer func() { _
= f.Close() }()) is unnecessarily verbose; replace it with a simple defer
f.Close() in the test(s) in encryption_exported_test.go (references: variable f
and its close calls around the current occurrences) and make the same
replacement at the other occurrence noted (around line 276) to simplify the
cleanup pattern.
- Around line 180-187: Replace the verbose deferred anonymous closures that
discard Close() errors with the simpler pattern; locate the variables inR, inW,
outR, outW in encryption_exported_test.go (the test cleanup block) and change
the four lines like `defer func() { _ = inR.Close() }()` to simple `defer
inR.Close()` (and similarly for inW, outR, outW); if a linter requires explicit
error discard, instead use the shorter `defer func() { _ = inR.Close() }()` form
or add a nolint with a brief comment.
In `@internal/orchestrator/pbs_notifications_api_apply.go`:
- Around line 28-330: Replace the opaque combined-error fmt.Errorf in
upsertPBSNotificationEndpoint (the error returned in the create/update failure
path) and upsertPBSNotificationMatcher with an error that wraps the joined
errors using errors.Join and %w so callers can inspect them (e.g. return
fmt.Errorf("endpoint %s:%s: %w", typ, name, errors.Join(err, upErr)) and
similarly for matcher), and replace the magic number in pbsEndpointRedactIndexes
by introducing a named constant (e.g. gotifyTokenRedactIndex) and return that
constant instead of []int{6} so the meaning ("token positional index in
`notification endpoint gotify create <name> <server> <token> ...`") is
documented next to the constant; update references in pbsEndpointRedactIndexes
accordingly.
In `@internal/orchestrator/restore_workflow_ui_fstab.go`:
- Line 36: The hardcoded 90-second timeout passed to ui.ConfirmFstabMerge should
be made configurable: replace the literal 90*time.Second with a named constant
or a configurable parameter (e.g., fstabMergeTimeout or cfg.FstabMergeTimeout)
and update the call site in restore workflow where ConfirmFstabMerge is invoked;
ensure the new constant/field has a sensible default and is documented so
callers can override it when creating the orchestrator or UI.
In `@internal/storage/secondary.go`:
- Around line 295-298: The tempFile close error path can cause a double-close
because the deferred cleanup still sees tempFile non-nil; in the block that
calls tempFile.Close() (the local tempFile variable in
internal/storage/secondary.go), set tempFile = nil immediately after calling
Close() (i.e., before checking/returning on the error) so the deferred cleanup
won't try to close it again; modify the Close/error handling around
tempFile.Close() accordingly (reference tempFile and the deferred cleanup block
that closes it).
In `@internal/tui/components/form.go`:
- Around line 56-59: The AddPasswordField wrapper currently calls the embedded
form method directly (f.Form.AddPasswordField) which is inconsistent with other
wrappers; change the call to use the wrapper receiver (call
f.AddPasswordField(...) instead of f.Form.AddPasswordField(...)) so it follows
the same refactor pattern as AddInputField/AddButton; ensure you pass the same
arguments (label, "", fieldWidth, '*', nil) when updating the call and verify
there is no accidental recursion by ensuring the wrapper delegates to the
underlying form implementation if a different internal helper exists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2ad5d35-6095-414d-a6f6-5866f4683645
📥 Commits
Reviewing files that changed from the base of the PR and between c7f28e4 and 15c533c.
Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.
Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
CodeRabbit commands
These commands are invoked using PR/Issue comments.
@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai rate limit to show your current review rate limit status.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai evaluate custom pre-merge check --instructions <custom-checks-instructions> --name <custom-checks-title> [--mode <error|warning>] to validate the custom pre-merge checks instructions. Defaults to error when --mode is omitted.
@coderabbitai ignore pre-merge checks to override pre-merge checks and get an approval on PR.
@coderabbitai run pre-merge checks to run pre-merge checks on the pull request.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Status, support, documentation and community
Visit our status page to check the current availability of CodeRabbit.
Create a ticket on our support page for assistance with any issues or questions.
Visit our documentation site for detailed information on how to use CodeRabbit.
Join our Discord community to connect with other users and get help from the community.
Follow us on X/Twitter for updates and announcements.
- Make nonessential system inventory probes best-effort
- Harden restore staging/export permissions and PBS datastore cfg temp handling
- Fix lock close propagation, help output routing, mount guard probe behavior, and chunk splitting
- Share close-error handling and clean up several small review findings
- Add coverage for tar TypeRegA restore metadata entries
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
♻️ Duplicate comments (1)
internal/backup/collector_system.go (1)
60-78: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Propagate cancellation out of collectBestEffortProbe.
Line 75 downgrades every safeCmdOutput failure to a warning. That also swallows context.Canceled / context.DeadlineExceeded, so a canceled collection can keep running until some later explicit ctx.Err() check. Please return context errors here and only suppress real probe failures.
Then let the call sites return the error when this helper reports 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/backup/collector_system.go` around lines 60 - 78,
collectBestEffortProbe currently swallows all errors from safeCmdOutput and only
logs a warning, which hides context cancellations; change collectBestEffortProbe
(now a void) to return error, call safeCmdOutput and if it returns
context.Canceled or context.DeadlineExceeded (or ctx.Err() != nil) immediately
return that error instead of logging, otherwise log/suppress non-context probe
failures and return nil; update all call sites of collectBestEffortProbe to
propagate/return the error so collection stops on cancellation while still
ignoring ordinary probe failures.
Prefer deferring cleanup immediately after successful resource acquisition.
The explicit close calls on lines 180-181 followed by t.Fatalf will cause the deferred closes on lines 184-185 to execute on already-closed pipes. While harmless in test code (the error is ignored), the idiomatic Go pattern is to defer cleanup immediately after successful resource creation:
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/orchestrator/encryption_exported_test.go` around lines 180 - 181,
Change the test to defer closing the pipes immediately after each successful
os.Pipe() call instead of doing explicit closes before t.Fatalf; after creating
inR,inW and checking err, call defer inR.Close() and defer inW.Close(), and
likewise after creating outR,outW call defer outR.Close() and defer
outW.Close(), so you remove the explicit _ = inR.Close()/inW.Close() before
t.Fatalf and avoid double-closing while keeping t.Fatalf behavior intact.
os.MkdirTemp creates the directory with 0o700 permissions by default, so the explicit os.Chmod(backupDir, 0o700) is redundant and triggers a false-positive static analysis warning.
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/orchestrator/directory_recreation_pbs_config.go` around lines 50 -
52, The explicit os.Chmod(backupDir, 0o700) is redundant because os.MkdirTemp
already creates the temp directory with 0o700; remove the os.Chmod call that
references backupDir to avoid the static-analysis false positive and keep the
original os.MkdirTemp usage intact (locate the os.Chmod(backupDir, 0o700) call
in the function that creates backupDir and delete that single statement).
The inDatastoreBlock flag is set when encountering any datastore: line and never reset, meaning all subsequent non-indented, non-ignored lines will be indented. This is correct if datastore.cfg exclusively contains datastore: blocks, but would incorrectly indent other top-level sections if present.
Given the function name normalizePBSDatastoreCfg and the specific PBS config context, this assumption appears intentional. Consider adding a comment clarifying that the normalizer expects datastore-block-only content.
📝 Suggested clarifying comment
func normalizePBSDatastoreCfgContent(content string) (string, int) {
+ // Assumes content contains only datastore: blocks and their properties.+ // Once a datastore block is encountered, all non-ignored, non-indented lines+ // are treated as properties requiring indentation.
lines := strings.Split(content, "\n")
inDatastoreBlock := 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/orchestrator/directory_recreation_pbs_config.go` around lines 115 -
147, The normalizer (normalizePBSDatastoreCfgContent) sets inDatastoreBlock on
any "datastore:" and never resets, which will wrongly indent subsequent
top-level sections unless the file contains only datastore blocks; update the
code by adding a clear explanatory comment above normalizePBSDatastoreCfgContent
(and/or classifyPBSDatastoreCfgLine) stating the function assumes datastore-only
content and that inDatastoreBlock is intentionally never cleared, or if you want
to support mixed config files, change the logic in classifyPBSDatastoreCfgLine
to detect other top-level section headers and reset inDatastoreBlock
accordingly—refer to the functions normalizePBSDatastoreCfgContent and
classifyPBSDatastoreCfgLine to locate where to add the comment or adjust the
reset logic.
internal/backup/optimizations.go (1)
293-293: 💤 Low value
Consider tightening chunk file permissions.
The chunk file is created with defaultChunkFilePerm (0o640 = rw-r-----), which grants group-read access. Unless the deployment requires group access to backup chunks, 0o600 (rw-------) better follows least-privilege principles.
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/backup/optimizations.go` at line 293, The chunk file is opened with
defaultChunkFilePerm (used in os.OpenFile in optimizations.go) which is
currently 0o640 and grants group read; change the constant defaultChunkFilePerm
to 0o600 so files are created with rw------- (least-privilege) and ensure any
tests or code paths referencing defaultChunkFilePerm continue to compile with
the new value.
Initialization flow is correct; consider deferring dirsToFix append until MkdirAll succeeds.
The current ensurePBSDatastoreSubdirs adds a path to dirsToFix (line 196) regardless of whether the subsequent os.MkdirAll (line 198) succeeds. On MkdirAll failure, applyPBSDatastoreOwnership will then call setDatastoreOwnership against a path that doesn't exist, producing an extra "could not set ownership" warning that just echoes the prior failure. Functionally harmless, but slightly noisier on partial-failure runs.
♻️ Optional reorder so the path is only tracked when actually created
Note: 0o750 on lines 174 and 198 is flagged by the linter, but it appears intentional given the backup:backup ownership and group-traversal needs documented for setDatastoreOwnership — no change suggested there.
🤖 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/orchestrator/directory_recreation_pbs.go` around lines 168 - 203,
The dirsToFix slice is being appended before os.MkdirAll succeeds in
ensurePBSDatastoreSubdirs which causes applyPBSDatastoreOwnership (and
ultimately setDatastoreOwnership) to try to change ownership of non-existent
paths; fix this by only appending path to dirsToFix after os.MkdirAll returns
nil, i.e. call os.MkdirAll(path, 0750) first, handle/log any error
(logger.Warning) and only when it succeeds set changed = true and append the
path to dirsToFix; keep the rest of the flow (initializePBSDatastore calling
applyPBSDatastoreOwnership and ensurePBSDatastoreLockFile) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/backup/collector_system.go`:
- Around line 971-975: The lsmod and SMART discovery calls should be best-effort
like lsusb/dmidecode: modify the safeCmdOutput invocation for the lsmod block
(the commandSpec("lsmod") call and its filepath.Join(commandsDir, "lsmod.txt")
output) and the smartctl --scan invocation (the smartctl command path/args and
its output file) so that failures do not cause the overall collection to abort;
pass the same permissive flag/option used by lsusb/dmidecode (or change the
boolean error-fail parameter to false) and ensure errors are logged at
debug/trace level while continuing the collection.
In `@internal/checks/checks.go`:
- Around line 381-397: When creating the lock file in the block that calls
f.WriteString, syncFile and f.Close (the code that sets
result.Error/result.Message and logs via c.logger), ensure any failure during
WriteString or the final Close removes the partially-created lock file so it
doesn't leave a stale .backup.lock; update the error paths after WriteString and
after the failing Close to attempt to close the file if still open and then call
os.Remove(lockPath) (logging any remove/close errors via c.logger.Warning)
before setting result.Error/result.Message and returning; reference the
variables and functions in the diff (f, WriteString, syncFile, f.Close,
lockPath, c.logger, result) when making the change.
In `@internal/orchestrator/restore_decision_test.go`:
- Around line 65-72: The test TestReadRestoreDecisionMetadataAcceptsTypeRegA
uses the deprecated tar.TypeRegA; replace that usage by defining a local
constant (e.g., const nulTypeFlag byte = 0) and use nulTypeFlag in the
tar.Header Typeflag field instead of tar.TypeRegA so the test preserves the NUL
type behavior without referencing the deprecated API; update any occurrences in
this test function to reference the new local constant.
In `@internal/orchestrator/restore_workflow_ui_extract.go`:
- Around line 181-209: The SAFE cluster apply currently only checks w.exportRoot
so it may run against a partially failed extraction; change runClusterSafeApply
to require a successful export extraction (e.g. ensure w.exportLogPath is set or
add a dedicated flag like w.exportExtracted set when extractSelectiveArchive
returns successfully) before calling runSafeClusterApplyWithUI; update
runClusterSafeApply to skip and log a warning if the export extraction did not
complete (use symbols runClusterSafeApply, extractSelectiveArchive,
w.exportLogPath or a new w.exportExtracted, and runSafeClusterApplyWithUI).
- Around line 245-277: stageAndApplySensitiveCategories currently proceeds to
applyStagedCategories even when extractStagedCategories swallowed non-abort
errors; change extractStagedCategories to return (bool, error) where the bool
indicates successful, complete staging (true) vs staging-with-warnings/errors
(false). Update stageAndApplySensitiveCategories to call success,err :=
extractStagedCategories(); if err != nil return err; if !success {
w.logger.Warning("Skipping apply due to staged extraction errors"); return nil }
so applyStagedCategories is only invoked when staging succeeded. Keep
handleStageExtractError semantics (use it to set w.restoreHadWarnings) but
ensure extractStagedCategories returns false when handleStageExtractError
returns nil for non-abort errors and only sets w.stageLogPath and returns true
on full success.
In `@internal/orchestrator/restore_workflow_ui_plan.go`:
- Around line 13-18: The helper method prepareBundleAndPlan on
restoreUIWorkflowRun is currently unused and flagged by linters; either call it
from the main restore path (replace the current separate prepareBundle() then
planPreparedBundle() sequence with a single call to prepareBundleAndPlan inside
the restore orchestration function that handles UI workflows) or remove the
helper entirely. Locate the restore orchestration entry (the function that
currently invokes prepareBundle() and planPreparedBundle() for
restoreUIWorkflowRun) and swap those calls for a single prepareBundleAndPlan()
invocation, or delete prepareBundleAndPlan() if no single-call refactor is
appropriate; ensure any error handling and return values remain consistent with
the existing callers.
---
Duplicate comments:
In `@internal/backup/collector_system.go`:
- Around line 60-78: collectBestEffortProbe currently swallows all errors from
safeCmdOutput and only logs a warning, which hides context cancellations; change
collectBestEffortProbe (now a void) to return error, call safeCmdOutput and if
it returns context.Canceled or context.DeadlineExceeded (or ctx.Err() != nil)
immediately return that error instead of logging, otherwise log/suppress
non-context probe failures and return nil; update all call sites of
collectBestEffortProbe to propagate/return the error so collection stops on
cancellation while still ignoring ordinary probe failures.
---
Nitpick comments:
In `@internal/backup/optimizations.go`:
- Line 293: The chunk file is opened with defaultChunkFilePerm (used in
os.OpenFile in optimizations.go) which is currently 0o640 and grants group read;
change the constant defaultChunkFilePerm to 0o600 so files are created with
rw------- (least-privilege) and ensure any tests or code paths referencing
defaultChunkFilePerm continue to compile with the new value.
In `@internal/orchestrator/directory_recreation_pbs_config.go`:
- Around line 50-52: The explicit os.Chmod(backupDir, 0o700) is redundant
because os.MkdirTemp already creates the temp directory with 0o700; remove the
os.Chmod call that references backupDir to avoid the static-analysis false
positive and keep the original os.MkdirTemp usage intact (locate the
os.Chmod(backupDir, 0o700) call in the function that creates backupDir and
delete that single statement).
- Around line 115-147: The normalizer (normalizePBSDatastoreCfgContent) sets
inDatastoreBlock on any "datastore:" and never resets, which will wrongly indent
subsequent top-level sections unless the file contains only datastore blocks;
update the code by adding a clear explanatory comment above
normalizePBSDatastoreCfgContent (and/or classifyPBSDatastoreCfgLine) stating the
function assumes datastore-only content and that inDatastoreBlock is
intentionally never cleared, or if you want to support mixed config files,
change the logic in classifyPBSDatastoreCfgLine to detect other top-level
section headers and reset inDatastoreBlock accordingly—refer to the functions
normalizePBSDatastoreCfgContent and classifyPBSDatastoreCfgLine to locate where
to add the comment or adjust the reset logic.
In `@internal/orchestrator/directory_recreation_pbs.go`:
- Around line 168-203: The dirsToFix slice is being appended before os.MkdirAll
succeeds in ensurePBSDatastoreSubdirs which causes applyPBSDatastoreOwnership
(and ultimately setDatastoreOwnership) to try to change ownership of
non-existent paths; fix this by only appending path to dirsToFix after
os.MkdirAll returns nil, i.e. call os.MkdirAll(path, 0750) first, handle/log any
error (logger.Warning) and only when it succeeds set changed = true and append
the path to dirsToFix; keep the rest of the flow (initializePBSDatastore calling
applyPBSDatastoreOwnership and ensurePBSDatastoreLockFile) unchanged.
In `@internal/orchestrator/encryption_exported_test.go`:
- Around line 180-181: Change the test to defer closing the pipes immediately
after each successful os.Pipe() call instead of doing explicit closes before
t.Fatalf; after creating inR,inW and checking err, call defer inR.Close() and
defer inW.Close(), and likewise after creating outR,outW call defer outR.Close()
and defer outW.Close(), so you remove the explicit _ = inR.Close()/inW.Close()
before t.Fatalf and avoid double-closing while keeping t.Fatalf behavior intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c467bec-78ba-4cab-bc91-129b60ef99d3
📥 Commits
Reviewing files that changed from the base of the PR and between 15c533c and e62a959.
Route the UI restore workflow through prepareBundleAndPlan so the helper is no longer dead code.
Preserve prepared bundle cleanup semantics when planning fails, and add a regression test for that failure path. Add the missing go.sum checksums for the tcell version required by go.mod.
Make staged extraction report whether staging completed successfully, not just whether it returned a fatal error.
Skip staged apply when extraction errors were downgraded to warnings, preserving restoreHadWarnings behavior and only recording stageLogPath after full staging success. Add regression coverage for incomplete staging.
Require a completed export extraction before running SAFE cluster apply.
Use exportLogPath as the success marker from extractSelectiveArchive, skip SAFE apply with a warning when export extraction did not complete, and add regression coverage for the partial-export path.
Replace deprecated tar.TypeRegA usage with explicit NUL typeflag constants.
Keep restore metadata parsing behavior for tar entries with typeflag 0, update the regression test to exercise that behavior directly, and remove the deprecated symbol from both test and production code.
Remove partially-created backup lock files when lock creation fails during WriteString or the final Close.
Log cleanup failures as warnings, preserve the original lock acquisition error, and add regression coverage for write and close failure paths.
Route lsmod and smartctl --scan through the best-effort probe path so failures no longer abort system collection.
Add debug-level handling for best-effort command failures and regression coverage to ensure lsmod and SMART scan errors continue without warnings or output files.
Make collectBestEffortProbe return cancellation and deadline errors instead of swallowing them as best-effort probe failures.
Propagate those errors from all probe call sites and preserve ordinary probe failures as debug-only skips. Extend command classification so canceled command executions are not downgraded to non-critical failures, and add tests for canceled/deadline probe paths.
Register pipe close defers immediately after each successful os.Pipe call in the non-interactive encryption recipient test.
Remove manual pipe closes from the second pipe failure path, preserving t.Fatalf behavior while avoiding duplicated cleanup handling.
This currently passes even if safe apply is attempted but happens to return nil. Add at least one assertion on a skip signal (warning flag and/or no apply artifact/log) so the regression guard is explicit.
🤖 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/orchestrator/restore_workflow_ui_extract_test.go` around lines 20 -
22, The test currently only asserts no error from w.runClusterSafeApply(); add
an explicit assertion that a skip occurred by checking a skip indicator or
absence of an apply artifact/log: after calling w.runClusterSafeApply(), assert
either a boolean field (e.g. w.safeApplySkipped), a returned skip flag (extend
runClusterSafeApply to return (error, skipped) if needed), or that apply
artifacts/log entries are absent (e.g. len(w.applyArtifacts)==0 or search the
test logger for the "skipped" message); if the skip condition is not met call
t.Fatalf to fail the test.
.github/workflows/race.yml (1)
8-34: ⚡ Quick win
Consider adding timeout and Go module caching.
The race detector can be slow on large codebases. Consider adding a job timeout and Go module caching to improve reliability and performance.
⚡ Optional improvements for performance and reliability
The cache: true option in setup-go automatically caches Go modules based on go.sum, and timeout-minutes prevents runaway tests from consuming excessive CI resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/race.yml around lines 8 - 34, Add a job timeout and enable
Go module caching for the race job: set timeout-minutes on the "race" job (e.g.,
30) to avoid runaway runs, and add cache: true under the "Setup Go" step's with
block (actions/setup-go@v6) so modules are cached based on go.sum; keep the
existing "Resolve race toolchain" and "Run race detector" steps (env GOTOOLCHAIN
and go test -race ./...) unchanged.
internal/backup/collector_system_test.go (1)
164-169: 💤 Low value
Optional: avoid (nil, nil) from the Stat mock.
Stat returning a nil os.FileInfo on success is unusual and only works here because the single caller (collectHardwareInfo's if _, err := c.depStat(...); err == nil) discards the info value. If a future change starts using the FileInfo (e.g., info.IsDir(), info.Mode()), this mock will silently panic the test. Returning a small fake os.FileInfo (or reusing an existing helper, e.g., os.Stat(tempDir) for a real FileInfo) would future-proof the test.
♻️ Suggested tweak
- Stat: func(path string) (os.FileInfo, error) {- if strings.HasSuffix(path, "/usr/sbin/smartctl") {- return nil, nil- }- return nil, os.ErrNotExist- },+ Stat: func(path string) (os.FileInfo, error) {+ if strings.HasSuffix(path, "/usr/sbin/smartctl") {+ // Reuse a real FileInfo so future callers that inspect mode/IsDir don't NPE.+ return os.Stat(tempDir)+ }+ return nil, os.ErrNotExist+ },
🤖 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/backup/collector_system_test.go` around lines 164 - 169, The Stat
mock in the test should not return (nil, nil); update the mocked Stat
implementation used by collectHardwareInfo (the c.depStat callback) so it
returns a valid os.FileInfo when simulating success instead of nil — e.g.,
obtain a real FileInfo via os.Stat on a temp file/dir or construct a minimal
os.FileInfo stub; ensure the mock still returns os.ErrNotExist for other paths
to preserve current behavior so future uses of info.IsDir()/info.Mode() won't
panic.
🤖 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 @.github/workflows/race.yml:
- Around line 12-19: The workflow uses mutable action tags actions/checkout@v6
and actions/setup-go@v6; replace those version tags with their corresponding
commit SHAs (pin to immutable refs) by looking up the latest recommended commit
SHAs for actions/checkout and actions/setup-go and updating the uses lines to
use the full SHA refs (e.g., actions/checkout@<commit-sha> and
actions/setup-go@<commit-sha>) so the Checkout and Setup Go steps are pinned to
specific commits.
In `@internal/backup/collector.go`:
- Around line 1048-1053: The early return on isContextCancellationError in the
depRunCommand call causes pvesh timeout cancellations to abort callers even when
those calls are marked non‑critical; change the logic so that if the
cancellation is from the pvesh-specific deadline and the call is non‑critical
(use the call spec/flag that indicates critical=false, e.g., the spec or the way
safeCmdOutput/safeCmdOutput callers mark critical), you do not return the error
but treat it as a skipped/non‑fatal result (clear err or set a non‑fatal status
on result) and continue; only return early when the cancellation reflects the
parent context (or when spec.Critical==true). Use symbols depRunCommand,
isContextCancellationError, safeCmdOutput and the spec/critical flag (or
propagate a sentinel pvesh timeout error) to implement this conditional
behavior.
In `@internal/orchestrator/encryption_exported_test.go`:
- Around line 178-179: The deferred inR.Close() and inW.Close() calls ignore
returned errors; replace those plain defers with testing cleanup functions that
check and handle errors (e.g., use t.Cleanup or a similar checked cleanup
callback) to call inR.Close() and inW.Close() and fail or log the test if
Close() returns an error; locate the two calls to inR.Close() and inW.Close() in
encryption_exported_test.go and wrap each in a checked cleanup closure that
captures and handles the Close() error instead of deferring it unhandled.
In `@internal/orchestrator/restore_workflow_ui_extract_test.go`:
- Line 17: Replace the hardcoded absolute path "/missing.tar" used in the test
fixture with a deterministic missing path built from the test's temporary
directory; specifically, set preparedBundle.ArchivePath to a path created via
t.TempDir() (e.g., filepath.Join(t.TempDir(), "missing.tar")) so the three
occurrences that reference preparedBundle and its ArchivePath (lines around the
three fixtures) use a guaranteed-missing, test-local file instead of a fixed
root path.
---
Nitpick comments:
In @.github/workflows/race.yml:
- Around line 8-34: Add a job timeout and enable Go module caching for the race
job: set timeout-minutes on the "race" job (e.g., 30) to avoid runaway runs, and
add cache: true under the "Setup Go" step's with block (actions/setup-go@v6) so
modules are cached based on go.sum; keep the existing "Resolve race toolchain"
and "Run race detector" steps (env GOTOOLCHAIN and go test -race ./...)
unchanged.
In `@internal/backup/collector_system_test.go`:
- Around line 164-169: The Stat mock in the test should not return (nil, nil);
update the mocked Stat implementation used by collectHardwareInfo (the c.depStat
callback) so it returns a valid os.FileInfo when simulating success instead of
nil — e.g., obtain a real FileInfo via os.Stat on a temp file/dir or construct a
minimal os.FileInfo stub; ensure the mock still returns os.ErrNotExist for other
paths to preserve current behavior so future uses of info.IsDir()/info.Mode()
won't panic.
In `@internal/orchestrator/restore_workflow_ui_extract_test.go`:
- Around line 20-22: The test currently only asserts no error from
w.runClusterSafeApply(); add an explicit assertion that a skip occurred by
checking a skip indicator or absence of an apply artifact/log: after calling
w.runClusterSafeApply(), assert either a boolean field (e.g.
w.safeApplySkipped), a returned skip flag (extend runClusterSafeApply to return
(error, skipped) if needed), or that apply artifacts/log entries are absent
(e.g. len(w.applyArtifacts)==0 or search the test logger for the "skipped"
message); if the skip condition is not met call t.Fatalf to fail the test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ac40cd9-a17f-41e3-ac0f-55580c50dcc7
📥 Commits
Reviewing files that changed from the base of the PR and between e62a959 and 08c5969.
Limit RunGoBackup integration tests to a minimal synthetic backup config so they do not collect and compress runner home directories under GitHub Actions.
Also make the TUI simulation harness create a fresh SimulationScreen per app instance, avoiding hangs when multi-step prompts reopen a TUI after the previous screen was finalized.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/orchestrator/tui_simulation_test.go (1)
48-52: ⚡ Quick win
Add screen.Fini() cleanup to properly finalize the SimulationScreen.
The screen is initialized via screen.Init() at line 49, but never finalized. tcell's standard pattern requires calling screen.Fini() after screen.Init() to properly clean up screen state and close internal resources (buffers and channels). While SimulationScreen doesn't hold OS terminal resources, following tcell's cleanup convention is the correct approach.
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/orchestrator/tui_simulation_test.go` around lines 48 - 52, The test
initializes a SimulationScreen with screen.Init() but never finalizes it; add a
call to screen.Fini() to properly clean up the SimulationScreen (best practice
is to defer screen.Fini() immediately after a successful call to screen.Init()).
Locate the tcell.NewSimulationScreen(...) / screen.Init() block and insert defer
screen.Fini() right after Init succeeds so that resources are always released
even if the test fails; ensure this uses the same screen variable referenced by
screen.SetSize(...).
🤖 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/orchestrator/tui_simulation_test.go`:
- Around line 48-52: The test creates a fresh SimulationScreen per app
(SimulationScreen and screen variable) but the key-injection goroutine is
guarded by injectOnce.Do(), so the closure captures the first screen and
subsequent apps (created by promptExistingPathDecisionTUI ->
promptNewPathInputTUI) never receive injected keys; move injectOnce (and its Do
call) into the replacement newTUIApp function so each new SimulationScreen
instance starts its own injection goroutine tied to its local screen, or
alternatively remove injectOnce and start a per-app injector in newTUIApp to
ensure the injection closure references the correct screen for each app.
---
Nitpick comments:
In `@internal/orchestrator/tui_simulation_test.go`:
- Around line 48-52: The test initializes a SimulationScreen with screen.Init()
but never finalizes it; add a call to screen.Fini() to properly clean up the
SimulationScreen (best practice is to defer screen.Fini() immediately after a
successful call to screen.Init()). Locate the tcell.NewSimulationScreen(...) /
screen.Init() block and insert defer screen.Fini() right after Init succeeds so
that resources are always released even if the test fails; ensure this uses the
same screen variable referenced by screen.SetSize(...).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1ad8d73-a676-42bf-8882-b7aa42fa251b
📥 Commits
Reviewing files that changed from the base of the PR and between 08c5969 and 5036b16.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Description: Search for patterns that might create multiple TUI apps in tests# Look for test functions that might create multiple app instances
rg -n 'newTUIApp.*newTUIApp|Run.*Run|Stop.*Start' --type go internal/orchestrator/
# Check if any prompt functions internally create new apps
ast-grep --pattern 'func prompt$_($$$) $_ { $$$ app := $$$ $$$}'# Look for comments mentioning multiple apps or screens
rg -n -i 'multiple.*app|reopen.*tui|second.*app' --type go internal/orchestrator/
Repository: tis24dev/proxsave
Length of output: 7704
🏁 Script executed:
# First, locate and examine the test file
find . -name "tui_simulation_test.go" -type f
Repository: tis24dev/proxsave
Length of output: 108
🏁 Script executed:
# Once found, read the entire test file to understand the test structure
cat -n internal/orchestrator/tui_simulation_test.go
Repository: tis24dev/proxsave
Length of output: 13761
🏁 Script executed:
# Check if there are multiple newTUIApp calls within a single test function
rg -n 'newTUIApp|injectOnce|func Test' --type go internal/orchestrator/tui_simulation_test.go
Repository: tis24dev/proxsave
Length of output: 1038
🏁 Script executed:
# Look for the main tui_simulation.go implementation to understand injectOnce setup
cat -n internal/orchestrator/tui_simulation.go | head -150
Repository: tis24dev/proxsave
Length of output: 133
🏁 Script executed:
# Find the prompt function implementations
rg -n 'func promptExistingPathDecisionTUI|func promptNewPathInputTUI|func promptDecryptSecretTUI' --type go internal/orchestrator/
Repository: tis24dev/proxsave
Length of output: 622
🏁 Script executed:
# Check what these prompt functions do - look for tui.NewApp or app creation
rg -n 'tui\.NewApp\|NewApp\(\)' --type go internal/orchestrator/ | head -20
Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Search for how the app is created in the test helper
rg -n 'screenEnv\|newTUIApp' --type go internal/orchestrator/ | grep -v test| head -30
Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Read the prompt function implementations
cat -n internal/orchestrator/workflow_ui_tui_decrypt_prompts.go
Repository: tis24dev/proxsave
Length of output: 7050
🏁 Script executed:
# Look for tui.NewApp() calls in the codebase
rg -n 'tui\.NewApp\|NewApp\(\)' --type go internal/orchestrator/
Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Check the test helper functions to understand how apps are created
rg -n 'newTUIApp|newTUIWorkflowUI|newTUIRestoreWorkflowUI' --type go internal/orchestrator/ -A 5 | head -60
Repository: tis24dev/proxsave
Length of output: 5145
🏁 Script executed:
# Let me trace through what happens in TestPromptExistingPathDecisionTUI_NewPathContextCanceledWhileRunning# It calls promptExistingPathDecisionTUI with keys that trigger the "Use different path" option# Let's verify this flow
cat -n internal/orchestrator/tui_simulation_test.go | sed -n '237,257p'
Repository: tis24dev/proxsave
Length of output: 1084
🏁 Script executed:
# Check if there are any tests that might try to type keys into a second app# by checking test patterns that inject multiple keys and call the same prompt
rg -B5 -A10 'withSimAppSequence.*tcell.KeyRight' --type go internal/orchestrator/tui_simulation_test.go
Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Let me look at what happens in the test that has multiple keys and nested prompts
cat -n internal/orchestrator/tui_simulation_test.go | sed -n '275,325p'
Repository: tis24dev/proxsave
Length of output: 2277
Multiple TUI apps in sequence will only receive key injection for the first instance.
The refactoring creates a fresh SimulationScreen for each app (lines 48-52), but the key injection goroutine runs only once per test due to injectOnce.Do() (line 68). The screen variable captured in the injection closure (line 103) references the first app's screen. When a prompt function internally creates multiple app instances—as promptExistingPathDecisionTUI() does at line 67 of workflow_ui_tui_decrypt_prompts.go by calling promptNewPathInputTUI()—only the first app receives injected keys. The second app would block waiting for input, causing test failures or hangs.
Consider moving injectOnce inside the replacement newTUIApp function so each app gets its own injection routine, or document that only one app should be created per test.
🤖 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/orchestrator/tui_simulation_test.go` around lines 48 - 52, The test
creates a fresh SimulationScreen per app (SimulationScreen and screen variable)
but the key-injection goroutine is guarded by injectOnce.Do(), so the closure
captures the first screen and subsequent apps (created by
promptExistingPathDecisionTUI -> promptNewPathInputTUI) never receive injected
keys; move injectOnce (and its Do call) into the replacement newTUIApp function
so each new SimulationScreen instance starts its own injection goroutine tied to
its local screen, or alternatively remove injectOnce and start a per-app
injector in newTUIApp to ensure the injection closure references the correct
screen for each app.
copyRegularFile can report failure while counting success stats.
Because closeIntoErr runs at defer time (Line 868), a source close failure can make the function return an error afterFilesProcessed/BytesCollected are incremented (Line 878-879), and without incrementing FilesFailed. This leaves inconsistent collection stats.
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/backup/collector.go` around lines 857 - 883, copyRegularFile
currently defers closeIntoErr so a close error can be set after you already call
c.incFilesProcessed and c.addBytesCollected, producing inconsistent stats; fix
it by ensuring the source file is closed (and any close error applied to the
function error) before incrementing success counters: after
copyRegularFileContents returns, call closeIntoErr (or explicitly close srcFile
and propagate its error into err) and handle that error before calling
c.applyMetadata, c.incFilesProcessed, c.addBytesCollected and the logger; adjust
use of closeIntoErr, srcFile, c.applyMetadata, c.incFilesProcessed and
c.addBytesCollected so success stats are only updated if no close error
occurred.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
73-77: ⚡ Quick win
Pin GoReleaser binary version instead of latest.
The action itself is pinned, but with.version: latest still introduces release-time drift. Prefer a fixed GoReleaser version for reproducible releases. Current stable version is v2.15.4, or use pessimistic semver like ~> v2 to get the latest v2.x patch.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 73 - 77, The workflow currently
sets the GoReleaser action input "version" to "latest"; change the
"with.version" value to a pinned semantic version (for reproducible builds) such
as "v2.15.4" or a pessimistic semver like "~> v2" instead of "latest", updating
the block that uses goreleaser/goreleaser-action so the release step uses a
fixed GoReleaser binary version.
🤖 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.
Outside diff comments:
In `@internal/backup/collector.go`:
- Around line 857-883: copyRegularFile currently defers closeIntoErr so a close
error can be set after you already call c.incFilesProcessed and
c.addBytesCollected, producing inconsistent stats; fix it by ensuring the source
file is closed (and any close error applied to the function error) before
incrementing success counters: after copyRegularFileContents returns, call
closeIntoErr (or explicitly close srcFile and propagate its error into err) and
handle that error before calling c.applyMetadata, c.incFilesProcessed,
c.addBytesCollected and the logger; adjust use of closeIntoErr, srcFile,
c.applyMetadata, c.incFilesProcessed and c.addBytesCollected so success stats
are only updated if no close error occurred.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 73-77: The workflow currently sets the GoReleaser action input
"version" to "latest"; change the "with.version" value to a pinned semantic
version (for reproducible builds) such as "v2.15.4" or a pessimistic semver like
"~> v2" instead of "latest", updating the block that uses
goreleaser/goreleaser-action so the release step uses a fixed GoReleaser binary
version.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 872eb089-6724-44fa-9341-55da4749dd11
📥 Commits
Reviewing files that changed from the base of the PR and between 5036b16 and fff25e8.
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
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores