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
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.
- 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
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.
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.
@tis24dev has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 5 seconds before requesting another review.
You’ve run out of usage credits. Purchase more in the billing tab.
⌛ How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Reviewing files that changed from the base of the PR and between 5e2d95e and d4a97f7.
📒 Files selected for processing (5)
cmd/proxsave/main.go
cmd/proxsave/main_runtime.go
docs/INSTALL.md
docs/RESTORE_GUIDE.md
go.mod
📝 Walkthrough
Walkthrough
Pins CI actions and adds a race workflow. Introduces closeerr and propagates robust close/error handling. Major orchestrator refactor: new UI-driven restore planning/apply, network apply with rollback/diagnostics, PBS/PVE directory recreation, mount guards. Updates collectors, notifications, metrics, security, storage, and extensive tests. Toolchain bumped.
Changes
Restore and Apply Orchestration
Layer / File(s)
Summary
UI-driven planning and run internal/orchestrator/restore_workflow_ui_*.go, ..._run.go, ..._full.go
Form/List/Panel wrappers use promoted methods; simulation harness improvements.
cmd/runtime cmd/proxsave/*
Safer defers and loop simplifications.
Sequence Diagram(s)
sequenceDiagram
participant User
participant RestoreWorkflow
participant Filesystem
participant NetApply
participant PBSGuards
User->>RestoreWorkflow: Select mode and categories
RestoreWorkflow->>Filesystem: Extract normal/staged payloads
RestoreWorkflow->>PBSGuards: Maybe apply datastore mount guards
RestoreWorkflow->>NetApply: Preflight validate
NetApply-->>RestoreWorkflow: Result (ok/fail)
alt ok
RestoreWorkflow->>NetApply: Apply with rollback timer
User-->>RestoreWorkflow: Commit decision
RestoreWorkflow->>NetApply: Disarm or allow rollback
else fail
RestoreWorkflow->>Filesystem: Auto-rollback staged network or prompt
end
Loading
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~150 minutes
Possibly related PRs
tis24dev/proxsave#189 — Earlier restore UI flow changes overlap with new workflow delegation and planning.
tis24dev/proxsave#198 — Previous edits to directory recreation intersect with new PBS/PVE recreation modules here.
tis24dev/proxsave#185 — Touches email relay code path also updated for response-body close handling.
Poem
I hop through logs and tidy ends,
Close the files, ensure they mend.
Restore’s a waltz with cautious tread,
Rollback ready, paths well-led.
Tags are pinned, the tests all race—
Ship it swift, with bunny grace. 🐇✨
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR includes a significant refactoring of the orchestrator and a version upgrade to Go 1.25. While it successfully addresses several critical bugs—including nil pointer dereferences and zombie process filtering—the analysis indicates that the code is currently not up to standards due to high-severity security and quality issues.
Key concerns that should be addressed before merging include insecure file permissions (0600/0700) for backup artifacts and storage directories, and a potential path traversal vulnerability in the PBS inspection logic. Furthermore, the modern Go 1.25 feature errors.Join should be utilized in error-handling helpers to prevent masking critical I/O errors during file closure.
About this PR
Across the codebase, particularly in 'internal/closeerr/closeerr.go' and 'internal/orchestrator/backup_safety.go', the pattern of handling errors during 'Close()' should be modernized to use 'errors.Join'. In the context of a backup tool, discarding errors from 'Close' when another error is present can mask critical failure signals such as 'Disk Full' or data-flushing issues.
Test suggestions
Zombie Proxmox Backup process filtering in security check
Nil candidate check in raw artifact staging
Skipping staged apply when extraction fails
Best-effort hardware probes handle command failures without warning
Propagate context cancellation during best-effort probes
Partial lock file cleanup after write/close failure
Restore metadata entry accepts NUL (TypeRegA) type flag
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Reduce lock file permissions to 0600 to follow the principle of least privilege. Additionally, consider implementing a mechanism to ensure the '.lock' file is cleaned up if the process crashes, and add the PID to the file to allow for stale lock detection, similar to existing checks in 'internal/checks/checks.go'.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The 'Export' function is highly complex (cyclomatic complexity 33) and repetitive. Refactor it to use a data-driven approach for writing metrics to improve maintainability.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Since the project is using Go 1.25, use 'errors.Join' to accumulate the close error instead of discarding it when an earlier error is present. This ensures critical signals like flush failures are preserved.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Suggestion: Consider using 'errors.Join' within the 'closeIntoErr' helper to ensure that if a file write fails AND the subsequent close fails, both errors are reported to the user.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Suggestion: Verify that this 'return nil' correctly implements the 'Skip staged apply after staging errors' requirement by ensuring no further apply logic runs for this candidate.
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.
Fix the TUI simulation helper so each new SimulationScreen gets its own key injector instead of reusing a closure tied to the first screen.
Add a regression test covering the nested existing-path -> new-path prompt flow.
Verified with:
- GOTOOLCHAIN=go1.25.10+auto go test ./internal/orchestrator -run 'TestPrompt.*TUI|TestTUIWorkflowUIPromptDecryptSecret|TestPromptYesNoTUI|TestShowRestorePlanTUI|TestSelectCategoriesTUI' -count=1
- GOTOOLCHAIN=go1.25.10+auto go test ./internal/orchestrator -count=1
These writes still inject raw values like Hostname, storage summaries, usage strings, and ScriptVersion straight into HTML, while later rows correctly use escapeHTML. Any &, <, or " in those fields will break the markup, and config-derived values become an HTML-injection surface.
Apply the same treatment to the other raw string interpolations in this block.
Also applies to: 262-262
🤖 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/notify/templates.go` around lines 114 - 177, Several HTML writes in
the template (the fmt.Fprintf and html.WriteString calls that interpolate
data.Hostname, data.LocalStatusSummary, data.LocalUsed/Free/Percent,
data.SecondaryStatusSummary, data.SecondaryUsed/Free/Percent,
data.CloudStatusSummary, ScriptVersion, etc.) inject raw strings; update each
interpolated value to be passed through the existing escapeHTML helper before
insertion (e.g., replace direct uses of data.Hostname, data.LocalStatusSummary,
GetStorageEmoji(...) outputs and all data.* used in fmt.Fprintf/html.WriteString
here and the similar occurrence around the ScriptVersion usage) so every user-
or config-derived string is escaped prior to writing into the HTML buffer.
internal/tui/components/form_test.go (1)
74-79: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Inconsistent wrapper usage.
Line 74-75 correctly use form.GetFormItemCount(), but line 77 falls back to form.Form.GetFormItem(0) instead of using the wrapper method form.GetFormItem(0).
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_test.go` around lines 74 - 79, Test mixes direct
access to the embedded Form with the wrapper: replace the direct call
form.Form.GetFormItem(0) with the wrapper method form.GetFormItem(0) so the test
consistently uses form.GetFormItemCount() and form.GetFormItem(0), then assert
the returned item is a *tview.InputField and call GetLabel() on it to check it
equals "Password".
Cancel sibling PXAR workers when the first datastore scan fails.
firstErr is captured here, but nothing tells the other goroutines to stop. A single failing datastore can now leave this step waiting for every other PXAR scan to finish, which is especially painful on large datastores. Reintroducing a child context and canceling it when firstErr is set preserves fast-fail behavior.
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_pbs_datastore.go` around lines 540 - 593, Wrap the
worker goroutines in a child cancellable context created with
context.WithCancel(ctx) inside runPBSPXARStep, pass that child context into fn
(replace uses of ctx in the goroutine with childCtx), and call cancel() as soon
as you record firstErr so sibling PXAR workers stop; also listen on
childCtx.Done() in the semaphore-acquire select to abort acquisition when
cancellation happens. Ensure you still return the recorded firstErr after
wg.Wait and respect the original ctx.Err() if set.
Preserve the compressor start error instead of returning a pipe error.
When cmd.Start() fails, closing pw causes the tar goroutine to report ErrClosedPipe, and these branches can return that secondary error instead of the real failed to start xz/zstd/... cause. Close the pipe with the start error, drain errChan, then return the start error.
Apply the same pattern to the zstd and generic pipeTarThroughCommand start-failure branches.
Also applies to: 686-691, 769-774
🤖 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 613 - 618, When cmd.Start() fails
in the compressor start-failure branches (the xz branch around cmd.Start(), the
zstd branch, and the generic pipeTarThroughCommand start-failure code path),
preserve and return that start error instead of the subsequent ErrClosedPipe:
call pw.CloseWithError(startErr) (using the actual start error), then drain
errChan (receive the tar goroutine error but ignore a closed-pipe result), and
finally return the original startErr (not the pipe error); update all three
branches to follow this pattern so the real compressor start failure is
returned.
🧹 Nitpick comments (11)
.github/instructions/codacy.instructions.md (1)
27-28: ⚡ Quick win
Consider breaking line 28 into sub-bullets for readability.
Line 28 contains multiple URLs with inconsistent formatting styles (one with full description in parentheses, one with a template placeholder). Breaking this into sub-bullets would improve scannability:
♻️ Suggested formatting improvement
-- Try to reset the MCP on the extension-- If the user is using VSCode, suggest them to review their Copilot > MCP settings in Github, under their organization or personal account. Refer them to Settings > Copilot > Enable MCP servers in Copilot. Suggested URL (https://github.com/settings/copilot/features) or https://github.com/organizations/{organization-name}/settings/copilot/features (This can only be done by their organization admins / owners)+- Try to reset the MCP on the extension+- If the user is using VSCode, suggest them to review their Copilot > MCP settings in Github:+ - Navigate to Settings > Copilot > Enable MCP servers in Copilot+ - For personal accounts: https://github.com/settings/copilot/features+ - For organizations: https://github.com/organizations/{organization-name}/settings/copilot/features+ - Note: Organization settings can only be modified by admins/owners
🤖 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/instructions/codacy.instructions.md around lines 27 - 28, The
paragraph starting with "Try to reset the MCP on the extension" bundles multiple
actions and URLs; split that long line into sub-bullets for clarity: one bullet
for resetting the MCP, one for VSCode-specific instructions referencing "Copilot
> MCP settings", and separate bullets for each URL
(https://github.com/settings/copilot/features and the template
https://github.com/organizations/{organization-name}/settings/copilot/features)
with consistent formatting and a short note that the org URL requires admin
access; keep the original phrasing ("Try to reset the MCP on the extension",
"Copilot > MCP settings") to make the change easy to locate.
internal/checks/checks_test.go (1)
1180-1208: 💤 Low value
Nice coverage for the close-failure → partial-lock-removal path.
Hijacking syncFile to close the underlying *os.File is a neat way to force the production f.Close() on line 393 of checks.go to return os.ErrClosed, deterministically exercising the new close-failure branch. Assertions on both the wrapped error text ("failed to close lock file") and the absence of the lock file pin down the contract well.
One small consideration: this test depends on syncFile being called before the production close. If the future implementation ever reorders or removes the explicit syncFile call, the close-failure path would silently stop being covered. Adding a brief inline comment to syncFile = func(...) { return f.Close() } explaining the intent (closing here causes the subsequent production Close() to fail) would help future maintainers. Optional.
🤖 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/checks/checks_test.go` around lines 1180 - 1208,
TestCheckLockFile_CloseFailsRemovesPartialLock is relying on overriding the
package-level syncFile to force an early f.Close() so the subsequent production
Close() in checks.go fails; add a brief inline comment next to the assignment
syncFile = func(f *os.File) error { return f.Close() } that states "close here
causes the subsequent production Close() to return os.ErrClosed and exercise the
close-failure branch" so future maintainers understand the intent and the
dependency on invocation order.
internal/checks/checks.go (3)
441-464: 💤 Low value
Close error is now surfaced, but the EIO retry loop won't retry on Close failure.
Capturing closeErr into lastErr so it flows into the existing error-classification switch is the right call. Just flagging that the retry-on-EIO logic (lines 455–460) only applies when createTestFile itself returns the error — a Close-time EIO will not be retried because the break on line 449 exits the attempt loop unconditionally. For a permission probe this is almost certainly fine (Close errors here are rare and usually non-transient), so noting it for awareness rather than asking for a change.
🤖 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/checks/checks.go` around lines 441 - 464, The loop captures closeErr
into lastErr but always breaks immediately after a Close failure, so Close-time
EIOs aren't retried; update the attempt loop around createTestFile/Close (the
code using createTestFile, closeErr, lastErr, syscall.EIO, retryDelay,
maxAttempts and c.logger.Warning) to treat a Close error the same as a
createTestFile EIO: after setting lastErr = closeErr, check if
errors.Is(closeErr, syscall.EIO) && attempt < maxAttempts, log a warning via
c.logger.Warning, sleep retryDelay and continue to retry; otherwise proceed to
break so non-transient close errors still stop retrying.
583-647: 💤 Low value
Error message normalization and cleanup hardening — LGTM.
A few nice properties of this block:
All wrapped errors now consistently use lowercased temp … prefixes and include the offending path, matching the casing change in backup_run_phases.go.
The defer func() { _ = osRemove(testFile) }() on line 629 guarantees the permission-test file is cleaned up regardless of whether the subsequent symlink test fails or succeeds — addresses the prior leak window.
Symlink-test cleanup (640–642) is now warn-only and ignores ENOENT, so a flaky removal won't mask an otherwise successful check.
One minor asymmetry: testFile is removed via defer while testSymlink is removed inline. Not a correctness issue (the symlink path only runs after the writability check passes), but moving symlink cleanup to a defer as well would make the cleanup pattern uniform. Optional.
🤖 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/checks/checks.go` around lines 583 - 647, Move the symlink cleanup
to a deferred removal to match the permission-test pattern: after creating
testSymlink with osSymlink, add a defer that calls osRemove(testSymlink)
(ignoring errors similarly to the existing defer for testFile) so both testFile
and testSymlink are guaranteed to be cleaned up; keep the existing inline
warning-only removal logic if you want to log failures but ensure the defer runs
regardless of subsequent errors.
381-399: 💤 Low value
Robust partial-lock cleanup on write/close failure — looks good.
The flow correctly handles all three failure modes:
WriteString fails → close (warn on close error), remove partial lock, return wrapped write error.
syncFile fails → warn only (best-effort durability), continue.
Close fails → warn, remove partial lock, return wrapped close error.
One small redundancy worth considering: on the close-failure branch (lines 393–399) the close error is both Warning-logged and returned as result.Error. Since callers will log/handle the returned error, the inline Warning is mostly duplicate noise. Dropping it (or demoting to Debug) would keep the log cleaner without losing information — but this is purely cosmetic.
🤖 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/checks/checks.go` around lines 381 - 399, The close-failure branch
currently both calls c.logger.Warning(...) and returns the wrapped error via
result.Error, creating duplicate logging; update the branch that handles the
f.Close() error (the block that calls f.Close(),
c.removePartialLockFile(lockPath), and sets result.Error/result.Message) to stop
emitting the Warning log — either remove the c.logger.Warning(...) call or
change it to c.logger.Debug(...) so only the returned result.Error is used for
higher-level logging; keep the call to c.removePartialLockFile(lockPath),
wrapping the close error into result.Error, and leave syncFile(...) and
WriteString handling unchanged.
.github/workflows/release.yml (1)
73-77: ⚡ Quick win
Pin an explicit GoReleaser version instead of version: latest.
Line 75 uses version: latest, which keeps the binary mutable and prevents reproducible releases. Use a semver range like version: "~> v2" (the recommended approach per the goreleaser-action documentation) to lock the GoReleaser version while allowing patch updates.
🤖 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/release.yml around lines 73 - 77, The workflow pins the
goreleaser action with "version: latest" making releases non-reproducible;
update the action input in the release job (the goreleaser/goreleaser-action
usage block and its "version" key) to a semver range such as "~> v2" (or another
specific supported semver range per goreleaser-action docs) so the action binary
is fixed while still allowing patch updates.
internal/safeexec/safeexec.go (1)
21-163: 💤 Low value
Optional: Standardize factory formatting for consistency.
The factory functions mix single-line and multi-line formatting. While purely cosmetic, standardizing to one style would improve visual consistency.
This is entirely optional and does not affect functionality.
🤖 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/safeexec/safeexec.go` around lines 21 - 163, The map
allowedCommandFactories contains a mix of single-line and multi-line anonymous
factory functions (e.g., the "blkid" entry and many others); choose a consistent
formatting style (either single-line one-liner or multi-line with explicit
return) and apply it across all entries in allowedCommandFactories, updating
each anonymous func(ctx context.Context, args ...string) *exec.Cmd (for example
"blkid", "chattr", "mountpoint", etc.) so the entire map uses the same style for
visual consistency.
internal/identity/identity_test.go (1)
1140-1197: 🏗️ Heavy lift
These subtests stop asserting anything on Linux.
On the platform that uses the sysfs path, both tests now become log-only and can no longer catch regressions in bridge/wireless detection. Please keep a deterministic Linux assertion path here—e.g. by injecting the sysfs readers or by splitting the name-fallback logic into a pure helper and testing that directly.
🤖 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/identity/identity_test.go` around lines 1140 - 1197, The tests
currently bail out on linux because sysfs-based detection in isBridgeInterface
and isWirelessInterface makes the results non-deterministic; refactor by
extracting the name-only logic into pure helpers (e.g., isBridgeInterfaceByName
and isWirelessInterfaceByName) or add an injectable sysfs reader used by
isBridgeInterface/isWirelessInterface, then update the tests to call these pure
name-based helpers (or pass a mocked sysfs reader) so assertions run
deterministically on Linux; locate and change the logic inside isBridgeInterface
and isWirelessInterface to delegate to the new helpers or injectable reader and
update TestIsBridgeInterfaceByName/TestIsWirelessInterfaceByName to assert
against the pure helpers instead of skipping on linux.
The 90-second timeout for fstab merge confirmation is hardcoded. Consider documenting why this specific duration was chosen (e.g., sufficient for operator review of mount points and remapping).
🤖 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` around lines 19 - 20, The
constant fstabMergeTimeout (currently set to 90 * time.Second) lacks rationale;
update the code by adding a brief comment/docstring above fstabMergeTimeout that
explains why 90s was chosen (e.g., allows operator to review mount points and
remapping in the UI, expected human confirmation time, and safety margin) and
note whether this value is adjustable (e.g., via config/flag) or why it should
remain fixed; reference the symbol fstabMergeTimeout and the surrounding restore
workflow UI merge logic so reviewers can see the explanation next to the
constant.
Consider extracting the prompt timeout to a named constant.
The 90*time.Second confirmation timeout at line 162 is a magic number that sits next to defaultNetworkRollbackTimeout in the same message. A named constant (e.g., defaultNetworkApplyConfirmTimeout) would make it easier to keep these two timeouts coherent and to tune the prompt wait independently of the rollback window.
🤖 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/network_apply_workflow_ui_prompt.go` around lines 151 -
165, Extract the magic timeout 90*time.Second used in confirmApplyNow when
calling f.ui.ConfirmAction into a named package-level constant (e.g.,
defaultNetworkApplyConfirmTimeout) and replace the literal with that constant;
update the declaration near defaultNetworkRollbackTimeout so both timeouts are
easy to see and tune together, and keep the change localized to the
confirmApplyNow function and the new constant to avoid altering ConfirmAction's
signature.
Consider renaming the error method to avoid shadowing the built-in type.
error is the name of Go's built-in interface, and a method named error on this flow type is also used just once (line 275). Either inlining f.logger.Error(...) at the single call site or renaming to errorf/logError would avoid the visual collision and keep symmetry with debug/info/warning clearer. Note: this is only a readability nit since methods can share names with built-in types without issue.
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/network_apply_workflow_ui_rollback.go` around lines 505
- 509, The method named error on the networkRollbackUIApplyFlow conflicts
visually with Go's built-in error type; rename the method (e.g., to errorf or
logError) and update its single call site (the usage of f.error) to the new
name, or alternatively inline the f.logger.Error(...) at that call site; ensure
the renamed method still checks f.logger != nil and forwards to
f.logger.Error(format, args...).
🤖 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/logging/logger.go`:
- Around line 75-79: The close path currently returns on Close error before
clearing the logger state, leaving l.logFile as a stale handle; change the logic
in the close method that calls l.logFile.Close() so you call Close, capture its
error, then set l.logFile = nil (and any related state) before returning the
error (e.g., call l.logFile.Close(), err := ..., then l.logFile = nil, then if
err != nil return fmt.Errorf("failed to close existing log file: %w", err)).
Ensure you reference and clear l.logFile to avoid silent drop of future file
writes.
In `@internal/notify/email.go`:
- Around line 1154-1156: Headers are written verbatim and allow CR/LF injection
via toHeader and e.config.From; before writing in the email builder (the block
that calls fmt.Fprintf(&email, "To: %s\n", toHeader), fmt.Fprintf(&email, "From:
%s\n", e.config.From)), validate and sanitize those values by rejecting or
removing any '\r' or '\n' (or using strict parsing like net/mail.Address
parsing) and normalize/escape or return an error for invalid input; implement a
small helper (e.g., sanitizeHeaderValue or parseAndFormatAddress) and use it for
toHeader and e.config.From so no raw CR/LF can be emitted into the MIME header.
In `@internal/orchestrator/backup_safety.go`:
- Around line 447-457: When a write (io.Copy from tarReader into outFile) or the
subsequent outFile.Close() fails in the restore loop, remove the partially
written target file before continuing; update the failure branches inside the
block that calls io.Copy(...) and the block that calls outFile.Close() to call
os.Remove(target) (handling/remove and logging any Remove error) after
attempting to close the file and before continuing, referencing the existing
variables outFile, target, tarReader and logger.Warning for locating and logging
the removal step.
In `@internal/orchestrator/directory_recreation_config.go`:
- Around line 155-163: In parseConfigPath, stop using strings.Fields which
splits paths with spaces; instead detect the "path " prefix, slice off that
prefix from the original line and return strings.TrimSpace of the remainder so
values like "path /mnt/my store" return the full "/mnt/my store"; update
parseConfigPath to use that slicing and trimming logic while preserving the same
boolean success return.
In `@internal/orchestrator/directory_recreation_ownership.go`:
- Around line 20-27: The current flow returns early when
lookupBackupOwnership(path, logger) yields found==false, skipping
ensureDatastoreDirectoryMode; change it so only a real error from lookup causes
an immediate return (keep "if err != nil { return err }"), but if found==false
do not return—skip calling chownDatastorePath and proceed to call
ensureDatastoreDirectoryMode(path, logger). If found==true, call
chownDatastorePath(uid,gid,logger) and handle its error as before, then always
call ensureDatastoreDirectoryMode and return its result; reference
lookupBackupOwnership, chownDatastorePath, and ensureDatastoreDirectoryMode to
locate and adjust the logic.
In `@internal/orchestrator/directory_recreation_paths.go`:
- Around line 150-154: The current isCommonZFSMountPath function is too
permissive because strings.Contains("backup") and "datastore" match unrelated
paths; update isCommonZFSMountPath to only treat strong, mount-like path
prefixes as ZFS (e.g., keep strings.HasPrefix(pathLower, "/mnt/") and replace
the broad Contains checks with prefix/segment-aware checks such as
strings.HasPrefix(pathLower, "/backup/") or strings.HasPrefix(pathLower,
"/datastore/") or checks that look for "/backup/" and "/datastore/" with path
separators), or ideally consult mount information if available; make the change
in isCommonZFSMountPath so only true mount-style paths are classified as ZFS.
In `@internal/orchestrator/directory_recreation_pbs_config.go`:
- Around line 77-102: The function writePBSDatastoreCfgAtomically currently
writes and renames a temp file without syncing; modify it to call tmpFile.Sync()
(handle and return any error) after writing and before closing/renaming to
ensure data is flushed to stable storage, and after a successful
os.Rename(tmpPath, path) call also fsync the containing directory (open
filepath.Dir(path) and call Sync on that dir file) to ensure the directory entry
is persisted; keep the existing defer cleanup behavior and propagate any
sync/fsync errors back to the caller.
In `@internal/orchestrator/directory_recreation_pbs.go`:
- Around line 190-200: The ensurePBSDatastoreSubdirs function currently swallows
os.MkdirAll failures and only logs a warning, allowing initializePBSDatastore to
treat a failed subdir creation as success; change ensurePBSDatastoreSubdirs to
propagate the first os.MkdirAll error instead of continuing (e.g., change
signature to return (bool, []string, error) or otherwise return an error), stop
the loop on that error, and update the caller initializePBSDatastore to handle
and return that error so datastore initialization fails when creating
.chunks/.index (refer to ensurePBSDatastoreSubdirs, pbsDatastoreSubdirs and
initializePBSDatastore).
In `@internal/orchestrator/directory_recreation_pve.go`:
- Around line 30-31: The call to createStorageSubdirs currently logs failures
but always returns nil; change the code to surface errors instead: modify
createStorageSubdirs to return an error (or propagate its error) and in
directory_recreation_pve.go replace the fire-and-forget call with error handling
that returns the error upstream (e.g., if err := createStorageSubdirs(basePath,
subdirs, logger); err != nil { logger.Error(...); return err }). Ensure all
callers of createStorageSubdirs (including the block around the current lines
34–40) are updated to check and return the error so partial directory creation
doesn't go unnoticed.
In `@internal/orchestrator/mount_guard.go`:
- Around line 32-40: guardMountPoint currently probes mount state twice
(ensureGuardTargetUnmounted then isAlreadyMounted) which can race; change it to
perform a single probe and act on that result. Modify ensureGuardTargetUnmounted
(or add a new helper like probeMountState) to return both an error and a boolean
"isMounted" (or return a typed status) so guardMountPoint can call
normalizeGuardMountRequest, call the single probe
(ensureGuardTargetUnmounted/probeMountState) once, and then decide: if probe
returns an error, wrap and return it; if probe reports already mounted, return
nil; otherwise proceed to bind-mount. Update references to
ensureGuardTargetUnmounted and isAlreadyMounted accordingly in guardMountPoint
(and the similar block at lines 64-78) so no second probe is done.
In `@internal/orchestrator/pbs_notifications_api_apply.go`:
- Around line 81-104: The warning/error logging calls in
buildPBSNotificationDesiredState (switch default and other branches) and in
buildPBSNotificationEndpoint are dereferencing logger without a nil-check;
update both functions to guard logger usage by either early-setting a no-op
logger when logger == nil or wrapping each logger.Warning / logger.Error call in
an if logger != nil condition so that missing logger won't panic when
encountering unknown sections or malformed endpoints; reference the functions
buildPBSNotificationDesiredState and buildPBSNotificationEndpoint and ensure all
places that call logger.* within these functions (including the default case and
any endpoint-validation branches) are guarded.
In `@internal/orchestrator/restore_workflow_ui_backups_services.go`:
- Around line 131-147: prepareRestoreServices currently accumulates cleanup
funcs from preparePVEClusterRestore and preparePBSServices but if a later
prepare call returns an error (including ErrRestoreAborted) it returns early
without running the already-registered cleanups, leaving services stopped; fix
by ensuring any previously appended cleanup functions are executed before
returning an error from prepareRestoreServices (e.g., run cleanups in reverse
order immediately when a subsequent prepare returns non-nil error), referencing
prepareRestoreServices, preparePVEClusterRestore, preparePBSServices and
ErrRestoreAborted to locate the relevant logic.
In `@internal/orchestrator/restore_workflow_ui_run.go`:
- Around line 55-63: After calling prepareBundleAndPlan() in
restoreUIWorkflowRun.run(), register the defer to call w.prepared.Cleanup()
immediately after w.prepared may be set (i.e., right after the
prepareBundleAndPlan() call) so that cleanup runs even if prepareBundleAndPlan()
later returns an error; ensure you still check err and return it, but move the
defer placement to occur before the err check and guard it with if w.prepared !=
nil to avoid nil dereference.
In `@internal/orchestrator/restore_workflow_ui_tfa.go`:
- Around line 25-27: The prompt for TFA compatibility should not be skipped when
logger is nil; ensure the decision to ask the user depends only on addNow (and
addNames) not on logger. Modify the code around addNow/logger: move the user
prompt (the logic that mutates selected) out of any logger != nil guard so that
the prompt runs whenever addNow is false, and restrict logger usage to only
emitting the Warning about re-enrollment (use logger.Warning(...,
strings.Join(addNames, ", ")) only if logger != nil). Keep references to addNow,
addNames, logger and selected so you update the correct block in
restore_workflow_ui_tfa.go.
In `@internal/orchestrator/restore_workflow_ui.go`:
- Around line 65-77: Update normalizeRestoreWorkflowUIError to detect EOF using
errors.Is(err, io.EOF) instead of the direct equality check; specifically change
the condition that currently does "err == io.EOF" so wrapped EOFs (e.g.,
fmt.Errorf(...%w...)) trigger the logger.Warning call and return
ErrRestoreAborted. Keep the existing logger.Warning message and the subsequent
call to restoreWorkflowInputAborted(ctx, err) unchanged so behavior and other
sentinel handling remain consistent.
In `@internal/storage/filesystem.go`:
- Around line 205-209: The defer that removes the temporary test file should be
registered before the early return so it always runs; move the defer func() { _
= os.Remove(testFile) }() to immediately after the test file is created (or at
least before the f.Close() error check) so that regardless of f.Close() failing
the cleanup will execute, and preserve the existing d.logger.Debug call and
return false behavior when f.Close() returns an error.
---
Outside diff comments:
In `@internal/backup/archiver.go`:
- Around line 613-618: When cmd.Start() fails in the compressor start-failure
branches (the xz branch around cmd.Start(), the zstd branch, and the generic
pipeTarThroughCommand start-failure code path), preserve and return that start
error instead of the subsequent ErrClosedPipe: call pw.CloseWithError(startErr)
(using the actual start error), then drain errChan (receive the tar goroutine
error but ignore a closed-pipe result), and finally return the original startErr
(not the pipe error); update all three branches to follow this pattern so the
real compressor start failure is returned.
In `@internal/backup/collector_pbs_datastore.go`:
- Around line 540-593: Wrap the worker goroutines in a child cancellable context
created with context.WithCancel(ctx) inside runPBSPXARStep, pass that child
context into fn (replace uses of ctx in the goroutine with childCtx), and call
cancel() as soon as you record firstErr so sibling PXAR workers stop; also
listen on childCtx.Done() in the semaphore-acquire select to abort acquisition
when cancellation happens. Ensure you still return the recorded firstErr after
wg.Wait and respect the original ctx.Err() if set.
In `@internal/notify/templates.go`:
- Around line 114-177: Several HTML writes in the template (the fmt.Fprintf and
html.WriteString calls that interpolate data.Hostname, data.LocalStatusSummary,
data.LocalUsed/Free/Percent, data.SecondaryStatusSummary,
data.SecondaryUsed/Free/Percent, data.CloudStatusSummary, ScriptVersion, etc.)
inject raw strings; update each interpolated value to be passed through the
existing escapeHTML helper before insertion (e.g., replace direct uses of
data.Hostname, data.LocalStatusSummary, GetStorageEmoji(...) outputs and all
data.* used in fmt.Fprintf/html.WriteString here and the similar occurrence
around the ScriptVersion usage) so every user- or config-derived string is
escaped prior to writing into the HTML buffer.
In `@internal/tui/components/form_test.go`:
- Around line 74-79: Test mixes direct access to the embedded Form with the
wrapper: replace the direct call form.Form.GetFormItem(0) with the wrapper
method form.GetFormItem(0) so the test consistently uses form.GetFormItemCount()
and form.GetFormItem(0), then assert the returned item is a *tview.InputField
and call GetLabel() on it to check it equals "Password".
---
Nitpick comments:
In @.github/instructions/codacy.instructions.md:
- Around line 27-28: The paragraph starting with "Try to reset the MCP on the
extension" bundles multiple actions and URLs; split that long line into
sub-bullets for clarity: one bullet for resetting the MCP, one for
VSCode-specific instructions referencing "Copilot > MCP settings", and separate
bullets for each URL (https://github.com/settings/copilot/features and the
template
https://github.com/organizations/{organization-name}/settings/copilot/features)
with consistent formatting and a short note that the org URL requires admin
access; keep the original phrasing ("Try to reset the MCP on the extension",
"Copilot > MCP settings") to make the change easy to locate.
In @.github/workflows/release.yml:
- Around line 73-77: The workflow pins the goreleaser action with "version:
latest" making releases non-reproducible; update the action input in the release
job (the goreleaser/goreleaser-action usage block and its "version" key) to a
semver range such as "~> v2" (or another specific supported semver range per
goreleaser-action docs) so the action binary is fixed while still allowing patch
updates.
In `@internal/checks/checks_test.go`:
- Around line 1180-1208: TestCheckLockFile_CloseFailsRemovesPartialLock is
relying on overriding the package-level syncFile to force an early f.Close() so
the subsequent production Close() in checks.go fails; add a brief inline comment
next to the assignment syncFile = func(f *os.File) error { return f.Close() }
that states "close here causes the subsequent production Close() to return
os.ErrClosed and exercise the close-failure branch" so future maintainers
understand the intent and the dependency on invocation order.
In `@internal/checks/checks.go`:
- Around line 441-464: The loop captures closeErr into lastErr but always breaks
immediately after a Close failure, so Close-time EIOs aren't retried; update the
attempt loop around createTestFile/Close (the code using createTestFile,
closeErr, lastErr, syscall.EIO, retryDelay, maxAttempts and c.logger.Warning) to
treat a Close error the same as a createTestFile EIO: after setting lastErr =
closeErr, check if errors.Is(closeErr, syscall.EIO) && attempt < maxAttempts,
log a warning via c.logger.Warning, sleep retryDelay and continue to retry;
otherwise proceed to break so non-transient close errors still stop retrying.
- Around line 583-647: Move the symlink cleanup to a deferred removal to match
the permission-test pattern: after creating testSymlink with osSymlink, add a
defer that calls osRemove(testSymlink) (ignoring errors similarly to the
existing defer for testFile) so both testFile and testSymlink are guaranteed to
be cleaned up; keep the existing inline warning-only removal logic if you want
to log failures but ensure the defer runs regardless of subsequent errors.
- Around line 381-399: The close-failure branch currently both calls
c.logger.Warning(...) and returns the wrapped error via result.Error, creating
duplicate logging; update the branch that handles the f.Close() error (the block
that calls f.Close(), c.removePartialLockFile(lockPath), and sets
result.Error/result.Message) to stop emitting the Warning log — either remove
the c.logger.Warning(...) call or change it to c.logger.Debug(...) so only the
returned result.Error is used for higher-level logging; keep the call to
c.removePartialLockFile(lockPath), wrapping the close error into result.Error,
and leave syncFile(...) and WriteString handling unchanged.
In `@internal/identity/identity_test.go`:
- Around line 1140-1197: The tests currently bail out on linux because
sysfs-based detection in isBridgeInterface and isWirelessInterface makes the
results non-deterministic; refactor by extracting the name-only logic into pure
helpers (e.g., isBridgeInterfaceByName and isWirelessInterfaceByName) or add an
injectable sysfs reader used by isBridgeInterface/isWirelessInterface, then
update the tests to call these pure name-based helpers (or pass a mocked sysfs
reader) so assertions run deterministically on Linux; locate and change the
logic inside isBridgeInterface and isWirelessInterface to delegate to the new
helpers or injectable reader and update
TestIsBridgeInterfaceByName/TestIsWirelessInterfaceByName to assert against the
pure helpers instead of skipping on linux.
In `@internal/orchestrator/network_apply_workflow_ui_prompt.go`:
- Around line 151-165: Extract the magic timeout 90*time.Second used in
confirmApplyNow when calling f.ui.ConfirmAction into a named package-level
constant (e.g., defaultNetworkApplyConfirmTimeout) and replace the literal with
that constant; update the declaration near defaultNetworkRollbackTimeout so both
timeouts are easy to see and tune together, and keep the change localized to the
confirmApplyNow function and the new constant to avoid altering ConfirmAction's
signature.
In `@internal/orchestrator/network_apply_workflow_ui_rollback.go`:
- Around line 505-509: The method named error on the networkRollbackUIApplyFlow
conflicts visually with Go's built-in error type; rename the method (e.g., to
errorf or logError) and update its single call site (the usage of f.error) to
the new name, or alternatively inline the f.logger.Error(...) at that call site;
ensure the renamed method still checks f.logger != nil and forwards to
f.logger.Error(format, args...).
In `@internal/orchestrator/restore_workflow_ui_fstab.go`:
- Around line 19-20: The constant fstabMergeTimeout (currently set to 90 *
time.Second) lacks rationale; update the code by adding a brief
comment/docstring above fstabMergeTimeout that explains why 90s was chosen
(e.g., allows operator to review mount points and remapping in the UI, expected
human confirmation time, and safety margin) and note whether this value is
adjustable (e.g., via config/flag) or why it should remain fixed; reference the
symbol fstabMergeTimeout and the surrounding restore workflow UI merge logic so
reviewers can see the explanation next to the constant.
In `@internal/safeexec/safeexec.go`:
- Around line 21-163: The map allowedCommandFactories contains a mix of
single-line and multi-line anonymous factory functions (e.g., the "blkid" entry
and many others); choose a consistent formatting style (either single-line
one-liner or multi-line with explicit return) and apply it across all entries in
allowedCommandFactories, updating each anonymous func(ctx context.Context, args
...string) *exec.Cmd (for example "blkid", "chattr", "mountpoint", etc.) so the
entire map uses the same style for visual consistency.
🪄 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: 38c31e85-e07c-4500-869c-80c1c8bdac48
📥 Commits
Reviewing files that changed from the base of the PR and between c7f28e4 and 5e2d95e.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Clear l.logFile before returning on close failure.
At Line 75, an error return happens before resetting l.logFile. That can leave a stale handle in logger state, and later writes (which ignore write errors) can silently drop file logs.
💡 Proposed fix
// If a log file is already open, close it first.
if l.logFile != nil {
- if err := l.logFile.Close(); err != nil {+ prev := l.logFile+ l.logFile = nil+ if err := prev.Close(); err != nil {
return fmt.Errorf("failed to close existing log file: %w", err)
}
- l.logFile = 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/logging/logger.go` around lines 75 - 79, The close path currently
returns on Close error before clearing the logger state, leaving l.logFile as a
stale handle; change the logic in the close method that calls l.logFile.Close()
so you call Close, capture its error, then set l.logFile = nil (and any related
state) before returning the error (e.g., call l.logFile.Close(), err := ...,
then l.logFile = nil, then if err != nil return fmt.Errorf("failed to close
existing log file: %w", err)). Ensure you reference and clear l.logFile to avoid
silent drop of future file writes.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Reject CR/LF before writing mail headers.
To and From are emitted verbatim into the MIME header block. A configured or auto-detected value containing \r/\n can inject extra headers or produce a malformed message. Please sanitize or strictly parse these fields before writing them.
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/notify/email.go` around lines 1154 - 1156, Headers are written
verbatim and allow CR/LF injection via toHeader and e.config.From; before
writing in the email builder (the block that calls fmt.Fprintf(&email, "To:
%s\n", toHeader), fmt.Fprintf(&email, "From: %s\n", e.config.From)), validate
and sanitize those values by rejecting or removing any '\r' or '\n' (or using
strict parsing like net/mail.Address parsing) and normalize/escape or return an
error for invalid input; implement a small helper (e.g., sanitizeHeaderValue or
parseAndFormatAddress) and use it for toHeader and e.config.From so no raw CR/LF
can be emitted into the MIME header.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Remove the target when a restore write or close fails.
This path logs and continues, but it leaves the partially restored file in place. For rollback code that's dangerous: a truncated config is usually worse than no restore at all. Delete target on both the io.Copy and Close failure branches.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
if_, err:=io.Copy(outFile, tarReader); err!=nil {
outFile.Close()
ifcloseErr:=outFile.Close(); closeErr!=nil {
logger.Warning("Cannot close partially restored file %s: %v", target, closeErr)
logger.Warning("Cannot close restored file %s: %v", target, err)
continue
}
🤖 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/backup_safety.go` around lines 447 - 457, When a write
(io.Copy from tarReader into outFile) or the subsequent outFile.Close() fails in
the restore loop, remove the partially written target file before continuing;
update the failure branches inside the block that calls io.Copy(...) and the
block that calls outFile.Close() to call os.Remove(target) (handling/remove and
logging any Remove error) after attempting to close the file and before
continuing, referencing the existing variables outFile, target, tarReader and
logger.Warning for locating and logging the removal step.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Parse the full path value instead of the second token.
strings.Fields truncates path /mnt/my store to /mnt/my, so directory recreation will target the wrong location for paths containing spaces. Slice off the "path " prefix and trim the remainder instead.
Suggested fix
func parseConfigPath(line string) (string, bool) {
if !strings.HasPrefix(line, "path ") {
return "", false
}
- parts := strings.Fields(line)- if len(parts) < 2 {+ value := strings.TrimSpace(strings.TrimPrefix(line, "path "))+ if value == "" {
return "", false
}
- return parts[1], true+ return value, true
}
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
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_config.go` around lines 155 - 163,
In parseConfigPath, stop using strings.Fields which splits paths with spaces;
instead detect the "path " prefix, slice off that prefix from the original line
and return strings.TrimSpace of the remainder so values like "path /mnt/my
store" return the full "/mnt/my store"; update parseConfigPath to use that
slicing and trimming logic while preserving the same boolean success return.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Don’t skip the mode fix when backup lookup is unavailable.
If user.Lookup("backup") returns found == false, this returns early and never calls ensureDatastoreDirectoryMode. That leaves recreated datastore directories with potentially wrong permissions even though the chmod step is still safe and independent of chown.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
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_ownership.go` around lines 20 -
27, The current flow returns early when lookupBackupOwnership(path, logger)
yields found==false, skipping ensureDatastoreDirectoryMode; change it so only a
real error from lookup causes an immediate return (keep "if err != nil { return
err }"), but if found==false do not return—skip calling chownDatastorePath and
proceed to call ensureDatastoreDirectoryMode(path, logger). If found==true, call
chownDatastorePath(uid,gid,logger) and handle its error as before, then always
call ensureDatastoreDirectoryMode and return its result; reference
lookupBackupOwnership, chownDatastorePath, and ensureDatastoreDirectoryMode to
locate and adjust the logic.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Run already-registered cleanups when a later prepare step fails.
If preparePVEClusterRestore() succeeds and preparePBSServices() then errors or returns ErrRestoreAborted, this function drops the accumulated cleanup and returns nil, leaving the earlier service stop in effect. That can strand the host in a degraded state before restore even starts.
Suggested fix
func (w *restoreUIWorkflowRun) prepareRestoreServices() (func(), error) {
var cleanups []func()
+ runCleanups := func() {+ for i := len(cleanups) - 1; i >= 0; i-- {+ cleanups[i]()+ }+ }
if cleanup, err := w.preparePVEClusterRestore(); err != nil {
return nil, err
} else if cleanup != nil {
cleanups = append(cleanups, cleanup)
}
if cleanup, err := w.preparePBSServices(); err != nil {
+ runCleanups()
return nil, err
} else if cleanup != nil {
cleanups = append(cleanups, cleanup)
}
- return func() {- for i := len(cleanups) - 1; i >= 0; i-- {- cleanups[i]()- }- }, nil+ return runCleanups, nil
}
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
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_backups_services.go` around lines
131 - 147, prepareRestoreServices currently accumulates cleanup funcs from
preparePVEClusterRestore and preparePBSServices but if a later prepare call
returns an error (including ErrRestoreAborted) it returns early without running
the already-registered cleanups, leaving services stopped; fix by ensuring any
previously appended cleanup functions are executed before returning an error
from prepareRestoreServices (e.g., run cleanups in reverse order immediately
when a subsequent prepare returns non-nil error), referencing
prepareRestoreServices, preparePVEClusterRestore, preparePBSServices and
ErrRestoreAborted to locate the relevant logic.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Defer prepared-bundle cleanup before the error return.
If prepareBundleAndPlan() allocates w.prepared and then fails, the early return skips Cleanup() and leaks the prepared bundle/tempdir. Register the defer immediately after the call, before checking err.
Suggested fix
func (w *restoreUIWorkflowRun) run() error {
fallbackToFullRestore, err := w.prepareBundleAndPlan()
+ if w.prepared != nil {+ defer w.prepared.Cleanup()+ }
if err != nil {
return err
}
- if w.prepared != nil {- defer w.prepared.Cleanup()- }
if fallbackToFullRestore {
return runFullRestoreWithUI(w.ctx, w.ui, w.candidate, w.prepared, w.destRoot, w.logger, w.cfg.DryRun)
}
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
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_run.go` around lines 55 - 63, After
calling prepareBundleAndPlan() in restoreUIWorkflowRun.run(), register the defer
to call w.prepared.Cleanup() immediately after w.prepared may be set (i.e.,
right after the prepareBundleAndPlan() call) so that cleanup runs even if
prepareBundleAndPlan() later returns an error; ensure you still check err and
return it, but move the defer placement to occur before the err check and guard
it with if w.prepared != nil to avoid nil dereference.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Don’t gate the TFA compatibility prompt on logger.
logger is only needed for the decline warning, but Line 34 makes it a hard precondition for prompting. If a caller passes nil, this silently skips the new safeguard and returns the original category set unchanged.
Suggested fix
func maybeAddRecommendedCategoriesForTFA(ctx context.Context, ui RestoreWorkflowUI, logger *logging.Logger, selected []Category, available []Category) ([]Category, error) {
if !shouldPromptForTFARecommendations(ui, logger, selected) {
return selected, nil
}
@@
if !addNow {
- logger.Warning("Access control selected without %s; WebAuthn users may require re-enrollment if the UI origin changes", strings.Join(addNames, ", "))+ if logger != nil {+ logger.Warning("Access control selected without %s; WebAuthn users may require re-enrollment if the UI origin changes", strings.Join(addNames, ", "))+ }
return selected, nil
}
return dedupeCategoriesByID(append(selected, addCategories...)), nil
}
func shouldPromptForTFARecommendations(ui RestoreWorkflowUI, logger *logging.Logger, selected []Category) bool {
- return ui != nil &&- logger != nil &&- (hasCategoryID(selected, "pve_access_control") || hasCategoryID(selected, "pbs_access_control"))+ return ui != nil &&+ (hasCategoryID(selected, "pve_access_control") || hasCategoryID(selected, "pbs_access_control"))
}
Also applies to: 32-35
🤖 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_tfa.go` around lines 25 - 27, The
prompt for TFA compatibility should not be skipped when logger is nil; ensure
the decision to ask the user depends only on addNow (and addNames) not on
logger. Modify the code around addNow/logger: move the user prompt (the logic
that mutates selected) out of any logger != nil guard so that the prompt runs
whenever addNow is false, and restrict logger usage to only emitting the Warning
about re-enrollment (use logger.Warning(..., strings.Join(addNames, ", ")) only
if logger != nil). Keep references to addNow, addNames, logger and selected so
you update the correct block in restore_workflow_ui_tfa.go.
logger.Warning("SAFE apply: failed to parse pools from export: %v", poolsErr)
pools = nil
}
applyPools := false
allowPoolMove := false
if len(pools) > 0 {
poolNames := summarizePoolIDs(pools, 10)
message := fmt.Sprintf("Found %d pool(s) in exported user.cfg.\n\nPools: %s\n\nApply pool definitions now? (Membership will be applied later in this SAFE apply flow.)", len(pools), poolNames)
logger.Warning("SAFE cluster restore: VM/CT configs not found for current node %s in export; available nodes: %s", currentNode, strings.Join(exportNodes, ", "))
logging.DebugStep(logger, "safe cluster apply (ui)", "datacenter.cfg not found (err=%v)", dcErr)
logger.Info("No datacenter.cfg found in export")
}
if err == io.EOF {
logger.Warning("Restore input closed unexpectedly (EOF). This usually means the interactive UI lost access to stdin/TTY (e.g., SSH disconnect or non-interactive execution). Re-run with --restore --cli from an interactive shell.")
return ErrRestoreAborted
}
// Apply pool membership after VM configs and storage/datacenter apply.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Use errors.Is(err, io.EOF) to handle wrapped EOFs.
err == io.EOF only matches a bare sentinel. Any caller in the workflow chain that wraps EOF (e.g. fmt.Errorf("read prompt: %w", io.EOF)) will bypass both the operator-facing warning and the ErrRestoreAborted normalization, leaving the original wrapped error to surface to the caller. The rest of this helper already uses errors.Is for its sentinels, so this is also a consistency win.
🛡️ Proposed fix
- if err == io.EOF {+ if errors.Is(err, io.EOF) {
logger.Warning("Restore input closed unexpectedly (EOF). This usually means the interactive UI lost access to stdin/TTY (e.g., SSH disconnect or non-interactive execution). Re-run with --restore --cli from an interactive shell.")
return ErrRestoreAborted
}
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
logger.Warning("SAFE apply: failed to parse pools from export: %v", poolsErr)
pools=nil
}
applyPools:=false
allowPoolMove:=false
iflen(pools) >0 {
poolNames:=summarizePoolIDs(pools, 10)
message:=fmt.Sprintf("Found %d pool(s) in exported user.cfg.\n\nPools: %s\n\nApply pool definitions now? (Membership will be applied later in this SAFE apply flow.)", len(pools), poolNames)
logger.Warning("SAFE cluster restore: VM/CT configs not found for current node %s in export; available nodes: %s", currentNode, strings.Join(exportNodes, ", "))
logging.DebugStep(logger, "safe cluster apply (ui)", "datacenter.cfg not found (err=%v)", dcErr)
logger.Info("No datacenter.cfg found in export")
}
iferr== io.EOF {
logger.Warning("Restore input closed unexpectedly (EOF). This usually means the interactive UI lost access to stdin/TTY (e.g., SSH disconnect or non-interactive execution). Re-run with --restore --cli from an interactive shell.")
returnErrRestoreAborted
}
// Apply pool membership after VM configs and storage/datacenter apply.
logger.Warning("Restore input closed unexpectedly (EOF). This usually means the interactive UI lost access to stdin/TTY (e.g., SSH disconnect or non-interactive execution). Re-run with --restore --cli from an interactive shell.")
returnErrRestoreAborted
}
ifrestoreWorkflowInputAborted(ctx, err) {
returnErrRestoreAborted
}
returnerr
}
🤖 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.go` around lines 65 - 77, Update
normalizeRestoreWorkflowUIError to detect EOF using errors.Is(err, io.EOF)
instead of the direct equality check; specifically change the condition that
currently does "err == io.EOF" so wrapped EOFs (e.g., fmt.Errorf(...%w...))
trigger the logger.Warning call and return ErrRestoreAborted. Keep the existing
logger.Warning message and the subsequent call to
restoreWorkflowInputAborted(ctx, err) unchanged so behavior and other sentinel
handling remain consistent.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Critical: Deferred cleanup won't execute when close fails.
The defer func() { _ = os.Remove(testFile) }() on line 209 is placed after the early return on line 207, so it will never be registered (and thus never execute) if f.Close() fails. This leaves the test file un-cleaned in that error path.
Move the defer before the close check:
f, err := os.Create(testFile)
if err != nil {
d.logger.Debug("Cannot create test file for ownership check: %v", err)
return false
}
+ defer func() { _ = os.Remove(testFile) }()
if err := f.Close(); err != nil {
d.logger.Debug("Cannot close test file for ownership check: %v", err)
return false
}
- defer func() { _ = os.Remove(testFile) }()
🤖 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/filesystem.go` around lines 205 - 209, The defer that
removes the temporary test file should be registered before the early return so
it always runs; move the defer func() { _ = os.Remove(testFile) }() to
immediately after the test file is created (or at least before the f.Close()
error check) so that regardless of f.Close() failing the cleanup will execute,
and preserve the existing d.logger.Debug call and return false behavior when
f.Close() returns an error.
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
Infrastructure