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
* Add native Pushover support as a webhook format
Pushover requires the application token and user/group key in the JSON
body (not in headers), and its API rejects proxsave's existing generic
payload shape. Until now users had no way to wire Pushover up without
running an external relay.
This adds `pushover` as a first-class WEBHOOK_*_FORMAT alongside
discord/slack/teams/generic. It reuses the existing AUTH_TOKEN /
AUTH_USER fields for Pushover credentials (AUTH_TYPE stays "none"),
introduces an optional WEBHOOK_<NAME>_PRIORITY (-2..1, default 0;
emergency priority 2 deferred), and produces a Pushover-shaped JSON
body with rune-aware truncation to the API's 250-char title and
1024-char message limits.
Validation: missing token/user is reported at payload-build time;
out-of-range priority is rejected by NewWebhookNotifier so misconfig
fails fast at startup rather than per-send.
* Address CodeRabbit review on PR #199
- docs/EXAMPLES.md: replace real-looking Pushover token/user values with
placeholders so secret scanners stop flagging the example, and update
the Example 7 scenario header and expected-results bullets so the
narrative matches the Discord + Pushover configuration block.
- internal/notify/webhook.go: skip the debug payload preview/content
log when the resolved format is "pushover". The Pushover payload puts
token + user in the JSON body (not headers), so the existing preview
would write both credentials into debug logs.
* Validate Pushover method and default-format resolution
Fail fast for invalid Pushover webhook configuration.
- resolve effective webhook format and method centrally
- validate Pushover PRIORITY after default format resolution
- require POST for Pushover endpoints at init time
- add regression tests for default-format and invalid method cases
---------
Co-authored-by: Damiano <71268257+tis24dev@users.noreply.github.com>
Remove t.Parallel() from orchestrator tests to avoid unsafe concurrency around package-level test dependencies such as restoreFS, restoreCmd, and restoreTime.
Fix the decrypt TUI E2E test harness by taking synchronized snapshots of the tcell simulation screen instead of reading screen contents concurrently with rendering.
Verified with:
- go test ./internal/orchestrator -count=1
- go test -race ./internal/orchestrator -count=1
- go vet ./internal/orchestrator
Replace direct exec.CommandContext usages with a new internal/safeexec package and introduce a CommandSpec type for validated command invocation. Collector APIs now accept CommandSpec (with validation and stringification) and use safe execution helpers; many PBS-related collector calls and tests were updated accordingly. Archiver and other components were updated to propagate command creation errors, improving error handling and security when spawning external processes. Added internal/safeexec implementation and tests.
Introduce backgroundRollbackCallKey helper and update numerous rollback tests to use it; add TestRunBackgroundRollbackTimer_UsesPositionalArgsForScriptPath. Add ValidateRemoteRelativePath unit test. Extend internal/safeexec to recognize "false" and "sysctl" commands and add a nosemgrep comment for TrustedCommandContext. Remove unused os/exec import in cmd/proxsave/upgrade.go. Add .github/instructions/codacy.instructions.md with Codacy MCP Server rules and guidelines.
Split the large backup startup/orchestration code into focused cmd/proxsave modules: backup_mode, backup_execution, backup_storage and backup_notifications, and updated main.go to call runBackupMode. Centralized orchestration flow (pre-checks, storage init, notifications, RunGoBackup invocation, stats persistence and exit/status logging) and improved error handling for backup runs. Improved collector validation in internal/backup: added validateExcludePatterns, combined validation checks (ensure at least one collection option, CLI timeouts, pxar concurrency, max PVE backup size and absolute system root prefix) and normalization steps. Also added/updated orchestrator helpers and tests to support the refactor.
Introduce BrickID doc and helper constructors (brick, collectorBrick, pbsCommandBrick, systemCommandBrick, pbsInventoryBrick) to standardize collection-brick creation, and split the large collector_bricks implementation into multiple focused files under internal/backup (common, pbs, pbs_features, pbs_inventory, pbs_runtime, pve, pve_finalize, pve_jobs, pve_storage, system). Remove the in-file recipe and brick definitions from collector_bricks.go and update tests (collector_bricks_test.go) to match the new layout. This reorganizes the code for better separation of concerns and easier maintenance.
Split and reorganize the large monolithic cmd/proxsave/main.go into a modular run pipeline. Main now delegates to staged helper functions (startMainRun, setupRunContext, preparePreRuntimeArgs, prepareRuntime, runRuntime) and most runtime logic and helpers were moved into new files under cmd/proxsave/ (main_config_modes.go, main_defers.go, main_footer.go, main_identity.go, main_lifecycle.go, main_modes.go, main_modes_test.go, main_network.go, main_restore_decrypt.go, main_runtime.go, main_runtime_log.go, main_security.go, main_signals.go, main_state.go, main_support.go, main_update.go). This reduces imports and responsibilities in main.go, improves readability and testability, and adds a focused unit test (main_modes_test.go). No behavioral changes intended—code was relocated and organized for clearer control flow and easier maintenance.
Refactor the large restore.go by extracting archive, extraction, decompression, service/ZFS handling and cluster-apply logic into dedicated files (restore_archive.go, restore_archive_entries.go, restore_archive_extract.go, restore_archive_paths.go, restore_cluster_apply.go, restore_decompression.go, restore_services.go, restore_zfs.go and related additions). Clean up imports and unused globals in restore.go, add ErrRestoreAborted and a doc comment for RunRestoreWorkflow, and update tests to use the new restoreArchiveOptions API for archive extraction. This improves modularity and maintainability of the restore code.
Use quoted-printable encoding for text and HTML parts to ensure 7-bit-safe emails and avoid 8bit transfer encoding. Add encodeQuotedPrintableBody helper and required imports, update Content-Transfer-Encoding headers in buildEmailMessage for both plain/text and html parts, and add a test (TestEmailNotifierBuildEmailMessage_EncodesUTF8BodiesAsSevenBitSafe) that verifies quoted-printable is used and no raw non-ASCII bytes remain in the message.
Reject empty or absolute hardlink Linkname values and normalize them with filepath.FromSlash. Resolve the hardlink target via resolvePathWithinRootFS and use the resolved path for creation to prevent links escaping the extraction root. Add tests: recordingLinkFS to capture Link() args, TestExtractHardlink_UsesResolvedTargetPath to ensure the resolved target is used, and TestExtractHardlink_RejectsSymlinkEscapeTarget to verify symlink-escape targets are rejected.
Add new unit tests for the orchestrator package. Tests cover estimatedBackupSizeGB scaling and minimum, BackupError wrapping and default messages for disk validation, backup metrics exit code logic, filling backup timing fields, and building backup collector config merging runtime excludes and blacklist. Also add restore tests for extractPlainArchive honoring skip functions and for runSafeClusterApplyWithUI ensuring storage/datacenter apply is skipped when storage_pve is staged.
Return the first capture error from PVE collection loops (so collection continues but callers get notified) and surface errors from schedule captures. Add tests to assert capture-error behavior for backup history, replication status, and schedule collection. Introduce lookupAbsolutePath and use it to resolve systemctl/mailq paths (and use TrustedCommandContext/commandForMailTool) to make mail/service checks more robust. Clear stale restore abort info at start of runRestoreWorkflowWithUI and add a test ensuring abort info is cleared before validation.
Ignore build metadata when comparing semantic versions by trimming any '+' suffix in isNewerVersion, and add unit tests covering build-metadata cases. Also update related docs and metadata: fix Codacy instructions YAML frontmatter formatting, update the PayPal donate link in README, and rename a config key in EXAMPLES.md (BACKUP_PXAR_FILES -> PXAR_SCAN_ENABLE).
Report failed files in restore summary and refine logging/guards. The restore summary now prints the number of failed files and includes them in the total archive count; summary log messages omit the "see detailed log" hint when no log path is configured. shouldSkipRestoreEntryTarget was tightened to match the /etc/pve boundary (exact match or trailing slash) to avoid false positives. Also changed a PBS user-list parse log from Debug to Warning and adjusted a couple of tests to account for whitespace-only config path handling. Added tests covering the restore summary and /etc/pve boundary behavior.
Add Lchown and UtimesNano to the FS interface and implement them in osFS and test fakes; update restore code to use restoreFS.Lchown/UtimesNano instead of direct os/syscall calls. Simplify environment runCommand to use safeexec.TrustedCommandContext and update tests to call echo via LookPath. Refactor Proxmox notification header parsing: introduce parseSectionHeader with a regexp-based validation and keep parseProxmoxNotificationHeader as a wrapper; update callers. Move panic handling in main lifecycle into a deferred function and remove an extra success print in config apply. Also include minor docs/formatting tweaks and add a test simulating execCommand failure in namespaces tests.
Centralize and harden command execution: introduce runAndClassifyCommand with commandRunOptions/result and classification enums; refactor safeCmdOutput and captureCommandOutput to reuse this logic, improving logging, output summarization, unprivileged-container handling, systemctl special cases, and dry-run/lookpath handling.
Runtime and signal updates: detect unprivileged container earlier in bootstrap (rt.unprivilegedInfo), remove duplicate detection in logging, stop signal notification on exit, and make stdin-close handling safe via a scoped sync.Once. Also fix heap profiling to defer file close.
Other changes: make pveContext methods use pointer receivers and nil-checks for safety; enforce Pushover webhook auth (require Token and User) and add tests; expand safeexec tests to validate the list of allowed commands.
Propagate filesystem detection errors for cloud backends so callers get diagnostics (detectFilesystemInfo now returns an error for LocationCloud and backup init logs include the failure reason). Adjust backup storage init logging to include the detection reason and mark cloud disabled. Change validateModeCompatibility to accumulate and return all compatibility violations (with a new test added). Rename helper 'newer' to 'meetsMinimum' for clarity and normalize directory mode literal to 0o755. Tests updated/added to cover the new behaviors.
Pass context.Context through ZFS restore helper functions and tests so operations can be cancelled and use caller-provided contexts. checkZFSPoolsAfterRestore, detectImportableZFSPools and logNoImportableZFSPools now accept a context, check ctx.Err, and pass ctx to restoreCmd.Run. The UI caller was updated to forward the workflow context. Tests were updated to supply contexts and two new tests verify context propagation and behavior on canceled contexts. FakeCommandRunner was extended to record contexts for assertions.
Change decompression reader APIs to return io.ReadCloser and ensure readers are closed with errors propagated. Introduce closeDecompressionReader helper to defer Close() and convert close errors into the function's error return. Update callers to use the helper and simplify test cleanup; add a test (TestExtractArchiveNativeReturnsDecompressionCloseError) that verifies decompressor Close() errors surface. Also adjust imports and minor test fixes to read directly from readers.
Refactor how VM and storage configs are applied: storageBlock now stores Type and proxmoxNotificationEntry entries instead of raw data, and VM/storage config files are parsed into --key=value pvesh arguments rather than written to temp files and passed via --filename/-conf. Added parsing helpers (parseColonConfigLine, pveshArgsFromColonConfigFile/Lines, pveshArgsFromProxmoxEntries, storageBlockPveshArgs, storageEntryValue) and updated applyVMConfigs/applyStorageCfg to build proper pvesh calls (e.g. "pvesh create /storage --storage=... --type=... ..."). Tests were updated to reflect the new argument format and to assert that deprecated flags (--filename, -conf) are no longer used.
Add a new ExitEncryptionError exit code and string mapping to represent encryption setup/processing failures. Update createBackupArchive to report the "encryption" phase and use ExitEncryptionError when age recipient preparation fails. Add tests: backup_run_phases_test verifies age-recipient failures are classified as encryption, and exit_codes_test updated to include the new code. Also fix RunGoBackup to return the collected stats value when prepareBackupWorkspace fails so callers receive consistent results.
Change writeArchiveChecksum to return an error and propagate failures: writeArchiveChecksum now returns a wrapped error on write failure (uses 0o640 file mode) and verifyAndWriteBackupArtifacts returns a BackupError when checksum writing fails. Make server identity detection injectable for testing by introducing runtimeServerIdentityDetector and only run detection when ServerID is not configured; add unit tests for initializeServerIdentity. Add unit test ensuring checksum write errors are propagated. Also add Node.js 24 setup to the Dependabot automerge workflow so Dependabot metadata step runs under Node 24.
Add isContextCancellationError helper and use it across PVE collection bricks and storage/metadata steps so context cancellations are propagated (returning the ctx error) instead of being treated as regular failures. Add tests ensuring guest and storage bricks propagate cancellations. Include new standalone collection flags (BackupPBSNotificationsPriv, BackupRootHome, BackupScriptRepository, BackupUserHomes) in collectionOptionFlags and add tests validating they are accepted. Change validateCloudSettings to accept absolute cloud remote refs with a new isAbsoluteCloudRemoteRef helper and add tests for absolute refs and path validation. Remove unused setBackupResult from main_state.go and update related imports in tests.
Introduce simAppCompletionTimeout and group timing constants. Improve test timeout handling by reporting errors and stopping the app if the initial draw doesn't occur or the simulation doesn't finish within the completion timeout after injecting keys. Also properly stop and reset the timer to avoid races.
Ensure App.Stop is safe when called before Run by adding run state tracking, a mutex and a stopRequested flag; implement Run and markRunningAndStopIfRequested to defer stopping until the event loop starts. Add synchronized test helpers (setTimedSimAppStopper, stopTimedSimAppForTest) and protect simulated apps with mutexes so tests can reliably stop current TUI instances. Increase several simulation timeouts and update runDecryptWorkflowTUIForTest to use a cancellable context and trigger a timed shutdown path. Also add a test verifying pre-run Stop terminates RunWithContext as expected.
Replace ad-hoc timed simulation plumbing with a reusable timedSimHarness for decrypt TUI e2e tests. Introduce timedSimHarness, timedSimAppState, and richer timedSimKey fields (RequireNewApp, SettleAfterMatch) plus new timing constants to make key injection and app lifecycle more deterministic and to improve diagnostics on timeouts. Update helper functions and tests to return/use the harness and tighten shutdown logic (StopAll, better timeout/error messages).
Also change RunTask startup/shutdown in workflow_ui_tui_decrypt.go to trigger the background task after the first draw (SetAfterDrawFunc), add start synchronization (started, startOnce) and queueProgressUpdate to safely schedule UI updates and ensure proper waiting for task completion on exit. Minor import adjustments (sync) included.
Pin GitHub Actions steps to specific SHAs for reproducible runs. Add a nil-context guard in checkForUpdates to avoid panics when ctx is nil. Include PBS user config collection by running newPBSUserConfigRecipe(). Change lookupAbsolutePath to return an error if exec.LookPath returns a non-absolute path rather than attempting to make it absolute. Enhance TUI e2e test helpers by adding a Wait field to timedSimKey and using it to control timeouts/sleeps. Update test FakeFS to simulate ownership changes via an Ownership map and a FakeOwnership struct in Lchown instead of calling os.Lchown.
Multiple fixes and feature additions across the codebase: update README PayPal link; improve finishMainRun panic handling and ensure runDone/logging are called; allow --install to choose CLI vs TUI; make isNewerVersion treat stable > prerelease and add tests; add timeout to hostname resolution. Backup collector: honor CustomBackupPaths, add pbsRepositoryWithDatastore helper and use it when building PBS_REPOSITORY, plus new tests. Email notifier: centralize mailq lookup (findMailqPath) and use it for queue checks. Orchestrator/restore: introduce localNodeName and helpers (pveshGuestExists, pveshCreateGuestArgs, pveshArgValue, isPveshNotFoundError), ensure missing guests are created before applying configs, stop parsing at section headers, and add tests for these behaviors. TUI tests: add runCompleted signaling to timedSimHarness and ensure RunDecryptWorkflowTUI signals completion. Misc: small test and brick-call refactors.
This PR introduces comprehensive dual-role PVE+PBS host support and refactors the proxsave command architecture into modular components. It redesigns the backup collection system using a "bricks/recipes" pattern, replaces direct command execution with safe wrappers, adds Pushover webhook notifications, and restructures the CLI entry point across multiple focused files. The backup and restore orchestration layers are significantly expanded with new phases, metadata handling, and safety checks.
New data types: appRuntime, appRunState, modeResult, appDeps, UpdateInfo, and CommandSpec; identity detection with detector hooks; FS interface extended with Lchown and UtimesNano for ownership/timestamp control.
Throughout codebase, replaced direct os/exec invocations with safeexec.CommandContext or safeexec.TrustedCommandContext; added error handling for command construction; new command execution helpers (commandForMailTool, lookupAbsolutePath, findMailqPath).
Major refactor: collector logic decomposed into composable "bricks" (individual collection steps) grouped into "recipes" (sequential workflows); separate recipes for PVE, PBS, System, Common, and Dual branches; each brick is idempotent, cancellation-aware, and independently testable; architectural documentation added.
New backup execution flow with context, workspace, and artifacts management; phases for initialization, data collection, archive creation, verification, bundling, and dispatch; specialized error handling via BackupError types; metrics collection and persistence; pre-backup checks integration; exit code derivation from error states.
New restore archive handling: extractTarEntry with type-based dispatch; path traversal prevention; /etc/pve hard guard; ownership/timestamp preservation via Lchown/UtimesNano; symlink/hardlink safe handling; decryption workflow enhancements; E2E TUI test harness with state snapshots and synchronized key injection.
Server identity detection with optional override hook; Telegram status verification with centralized/personal modes; dual-role host auto-detection; category filtering based on host type (pve, pbs, dual, unknown); restore workflow updated to handle cross-role compatibility; compatibility validation with partial-compatibility warnings.
Notification System Enhancements internal/notify/webhook.go, internal/notify/webhook_payloads.go, internal/notify/webhook_test.go, internal/config/config.go (WebhookEndpoint.Priority), internal/config/templates/backup.env, docs/EXAMPLES.md
Pushover webhook format added alongside Discord/Slack/Teams/Generic; per-endpoint priority field for webhooks; endpoint validation with pushover-specific requirements (token, user, POST method); buildPushoverPayload with rune-safe truncation; priority range enforcement (-2 to 1).
Test parallelism removed from guard-related tests for sequential execution; mount guard tests expanded with mock runners and comprehensive scenarios; NIC mapping archive handling refactored with named return values and centralized cleanup via closeDecompressionReader.
CollectorConfig validation expanded: exclude pattern validation, collection option enforcement, concurrency normalization, timeout normalization; safeCopyFile refactored to use statCopySource and copyRegularFileContents; CommandSpec-based command representation throughout collector; new tests for standalone options and cloud settings validation.
New command execution classification system: runAndClassifyCommand executes with context, applies timeouts, classifies outcomes (success, skip, non-critical/critical failure, downgrade); PBS-authenticated commands via safeCmdOutputWithPBSAuth and safeCmdOutputWithPBSAuthForDatastore with per-datastore repository handling; system/PVE/PBS collectors refactored to use commandSpec throughout; comprehensive test updates with CommandSpec wrappers and error path validation.
New docs/README.md as documentation index; DEVELOPER_GUIDE expanded with Key Modules table and Collector Architecture section; Example 7 updated for Discord+Pushover; Example 10 added for dual PVE+PBS hosts; RESTORE_DIAGRAMS extended with DUAL and Unknown paths; CI instructions added (.github/instructions/codacy.instructions.md).
tcell/v2 upgraded from v2.13.8 to v2.13.9; Codecov action upgraded from v5 to v6; Dependabot auto-merge workflow updated with Node.js 24 setup and pinned fetch-metadata commit; README updated with Donate badge and analytics image URL.
Sequence Diagram(s)
sequenceDiagram
participant CLI as CLI Entry
participant Lifecycle as Lifecycle
participant Identity as Identity
participant Config as Configuration
participant Collector as Collector
participant Orchestrator as Orchestrator
participant Storage as Storage
participant Notifier as Notifier
CLI->>Lifecycle: startMainRun()
Lifecycle->>Lifecycle: setupRunContext()
Lifecycle->>Identity: initializeServerIdentity()
Identity-->>Lifecycle: ServerID, MAC
Lifecycle->>Config: loadRunConfig()
Config-->>Lifecycle: Config
Lifecycle->>Collector: build recipes (PVE/PBS/Dual)
Collector->>Collector: execute bricks sequentially
Collector-->>Orchestrator: collection metadata
Orchestrator->>Storage: initializeBackupStorage()
Storage-->>Orchestrator: primary/secondary/cloud backends
Orchestrator->>Notifier: initializeNotifications()
Notifier-->>Orchestrator: Email/Telegram/Gotify/Webhook
Orchestrator->>Orchestrator: createBackupArchive()
Orchestrator->>Orchestrator: verifyAndWriteBackupArtifacts()
Orchestrator-->>CLI: BackupStats, exit code
CLI->>Lifecycle: finishMainRun()
Loading
sequenceDiagram
participant User as Operator
participant Restore as Restore Workflow
participant Detect as System Detection
participant Compat as Compatibility Check
participant Extract as Extraction Engine
participant Verify as Security Guards
participant Apply as Apply & Verify
User->>Restore: initiate restore
Restore->>Detect: detectCurrentSystem()
Detect-->>Restore: host type (pve/pbs/dual/unknown)
Restore->>Detect: detectBackupType()
Detect-->>Restore: backup type (pve/pbs/dual)
Restore->>Compat: analyzeBackupCategories()
Compat-->>Restore: compatible categories (full/partial/incompatible)
alt Partial Compatibility
Restore->>User: warn & filter categories
end
Restore->>User: showRestorePlan()
User->>Restore: confirmRestorePlan()
Restore->>Extract: extractPlainArchive() or extractSelectiveArchive()
Extract->>Verify: isSecurePath() for each entry
Verify-->>Extract: safe or rejected
Extract->>Verify: enforceEtcPveGuard()
Verify-->>Extract: proceed or skip
Extract->>Apply: createDirectory/extractRegularFile/extractSymlink()
Apply->>Apply: setOwnership(Lchown), setTimestamps(UtimesNano)
Apply-->>Restore: entry applied
Restore->>Restore: optionally merge fstab
Restore-->>User: restore complete
Loading
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
This is a large-scale architectural refactoring affecting the core command structure, backup/restore orchestration, collector subsystem, and command execution infrastructure. It involves complex logic density across multiple layers (bricks/recipes pattern, new orchestration phases, dual-role compatibility checks), significant file proliferation (30+ new files), heterogeneous changes requiring separate reasoning for each domain (CLI modularization, collector redesign, orchestrator phases, notification integration), and substantial integration points between previously monolithic code. The scope spans from low-level safe execution wrappers to high-level orchestration flows, demanding careful review of each architectural layer and their interactions.
Possibly related PRs
tis24dev/proxsave#171: Overlapping changes to backup/restore orchestrator and collector subsystems, including command-spec wiring, safeexec integration, and path-key handling logic.
tis24dev/proxsave#198: Implements the same dual PVE+PBS support architecture, collector bricks/recipes model, and matching changes across collector, orchestrator, and detection code.
tis24dev/proxsave#185: Related via overlapping code paths in install.go, identity handling, bundle normalization, webhook/email relay, and various CLI/context guards.
🐰 A grand refactor hops along,
Bricks and recipes, where code belongs!
Dual hosts dance, safe commands run,
Pushover bells ring—backup begun! 🔔
Restore with grace, guards stand tall. ✨
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.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR performs an extensive refactoring of core backup/restore components into modular domains while introducing a 'safeexec' layer to mitigate command injection risks. However, the Codacy analysis indicates the PR is 'not up to standards' due to 23 new issues, primarily related to extreme cyclomatic complexity (CCN reaching 79 in safeexec logic) and security best practices in CI.
Major concerns include the complete absence of a PR description for such a high-impact change and the massive scope which combines core security hardening, new feature implementation (Pushover), and architectural refactoring. These should be addressed to ensure maintainability and proper auditing of the security changes.
About this PR
No description was provided for this PR. Given the massive structural refactoring and high-impact security changes (like the safeexec layer), a detailed summary of changes and rationale is required for a safe review.
The PR scope is extremely large, combining core structural changes, security hardening, test infrastructure expansion, and new feature additions (Pushover) in one set of changes. This violates the principle of atomic PRs and makes verification difficult.
Test suggestions
CLI mode compatibility and conflict validation (e.g., --cleanup-guards with --support)
Pushover notification payload construction, truncation, and priority passthrough
TUI application lifecycle hardening (handling Stop requests before the event loop starts)
Path traversal protection for regular files and hardlinks during archive extraction
Context cancellation propagation in PVE collection bricks (ensuring abort doesn't leave data in unstable states)
Server identity detection ensuring configured IDs override detected ones
ZFS pool status detection and manual import instruction generation after restore
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
This line is flagged for OS command injection. Since you have implemented validation logic and a #nosec tag for Gosec, you should also add a nosemgrep comment to suppress this finding in the logic block.
Try running the following prompt in your IDE agent:
Add a 'nosemgrep' comment to line 203 of internal/safeexec/safeexec.go to suppress the 'go.lang.security.audit.dangerous-exec-command' finding, alongside the existing Gosec annotation.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The initialization of the webhook notifier is becoming complex. Consider extracting the per-endpoint validation logic (specifically the Pushover-specific block) into a separate validation method.
Try running the following prompt in your IDE agent:
Extract the endpoint validation loop and Pushover-specific checks from NewWebhookNotifier in internal/notify/webhook.go into a private validateEndpoint method.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
This method is handling too many responsibilities. Extracting the privilege-sensitive failure matching logic and the systemd/systemctl status checks into private helper methods would significantly improve readability and reduce complexity.
Try running the following prompt in your IDE agent:
Refactor runAndClassifyCommand in internal/backup/collector.go to extract the error classification and privilege-sensitive downgrade logic into dedicated private methods.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The complexity of this function (79) is exceptionally high due to the exhaustive allowlist switch. Consider splitting this into smaller categorized functions (e.g., isCoreSystemCommand, isProxmoxCommand) to reduce the complexity per block while maintaining the use of literals for security analysis tools.
Try running the following prompt in your IDE agent:
Refactor the CommandContext switch-case block in internal/safeexec/safeexec.go into multiple smaller private helper functions grouped by command category.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Nitpick: Only the first capture error is returned. While collection continues for other nodes, consider if an error aggregator would provide better visibility into multi-node failures.
Extract pushover-specific validation from NewWebhookNotifier into a new WebhookNotifier.validateEndpoint method and instantiate the notifier earlier. NewWebhookNotifier now assigns the HTTP client to the notifier and returns it, improving readability and isolating endpoint validation. Also move the nosemgrep annotation to the end of the return statement in TrustedCommandContext to satisfy linters.
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}
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores