Conversation
Update go.mod toolchain directive from go1.25.8 to go1.25.9 to pick up the latest Go patch release.
Introduce a bricks/recipes abstraction for backup collection: add internal/backup/collector_bricks.go which defines BrickID, collectionBrick, recipe, collectionState and helper functions, and implements newPVERecipe, newPBSRecipe and newSystemRecipe. Update PVE/PBS/System collectors to run these recipes (use runRecipe) instead of inline procedural logic (collector_pve.go, collector_pbs.go, collector_system.go). Add unit tests for brick execution order and specific bricks (internal/backup/collector_bricks_test.go). Also add an environment test documenting the hybrid PVE+PBS detection bug (internal/environment/detect_hybrid_bug_test.go). The change restructures the collection flow without altering collection behavior, and centralizes per-system steps into reusable bricks.
Perform a large refactor of collection bricks and PBS/system runtime collection. Changes include: renaming and expanding BrickID constants for PVE, PBS, and system collectors; introducing per-component commandsDir fields in collection state (and a systemContext); adding ensure*CommandsDir helpers and ensurePVERuntimeInfo; splitting and reorganizing PBS command collection into many focused collectPBS* functions (core/node/datastore/acme/notifications/access/remote/tape/network/host/s3/storage-stack/etc.) and replacing the old aggregated collectPBSCommands; adding Collector.ensureCommandsDir in collector_paths.go; updating recipes to use the new bricks and runtime collection flows; adjusting tests to expect the new brick ordering and adding requireBrick helper. Various logging and minor control-flow adjustments accompany the refactor.
Restructure and expand the PVE backup collection. Rename and add many BrickIDs and split large collection steps into smaller, focused bricks (VM QEMU/LXC configs, guest inventory, backup job defs/history, schedule pieces, replication defs/status, storage resolve/probe/metadata/analysis/summary, Ceph snapshots, aliases and manifest finalize). Introduce pveStorageScanResult and per-storage scan flow: preparePVEStorageScan, collectPVEStorageMetadataJSONStep, collectPVEStorageMetadataTextStep and helpers. Extend pveContext with fields to track resolved/probed storages, scan results and per-area abort flags, plus ensure/storage result accessors. Factor collector logic into many new helper methods (pveInfoDir, pveJobsDir, pveSchedulesDir, pveDatastoresBaseDir, pveCephDir, pveStorageIOTimout, etc.) and rename/refactor existing collection functions (collectVMConfigs -> collectPVEQEMUConfigs/collectPVELXCConfigs/collectPVEGuestInventory, jobs/schedules/replication split). Update tests to reflect new recipe order and add tests for storage resolve and probe bricks. Logging and skip/reason reporting for storage probes have been cleaned up and centralized.
Split the monolithic PBS recipe into many focused bricks and helper recipes to improve modularity and enable targeted collections. Added a large set of new BrickIDs and reorganized brick construction into functions: newPBSRecipe, newPBSDirectoryRecipe, newPBSCommandsRecipe, newPBSDatastoreInventoryRecipe and helpers that return groups (manifest, runtime, storage-stack, inventory, feature, finalize). Expanded pbsContext with fields for datastores, commandsDir, userIDs, acmeAccountNames, acmePluginIDs, s3EndpointIDs, tape support flags and inventory state; added ensurePBSInventoryState(). Updated PBS runtime and manifest collection flows (more granular runtime/manifests, tape detection, S3/acme/access/token handling, inventory population and writing). Tests updated and new unit tests added for access users, ACME accounts, S3 endpoints, tape detection and inventory initialization. Also minor adjustment in collectPBSDirectories usage. This refactor improves testability and lets callers run directory/commands/inventory-specific recipes independently.
Rework PBS collection bricks into finer-grained units and introduce state for datastore configs and PXAR. Added pbsContext fields (datastoreConfig, pxar) and helper ensurePBSDatastoreConfigState / ensurePBSPXARState. Replaced monolithic manifest/runtime/storage/inventory bricks with many specific bricks (user/acl/domains, tape cfg/jobs/pools/keys, separate notification endpoints, access realm types, storage stack snapshots, inventory file categories, PXAR steps, datastore CLI configs and namespaces, etc.). Removed old directory/manifest helpers and updated Collector methods to use the new manifest entry functions. Tests updated to match new brick ordering and to cover datastore CLI config preparation, PXAR preparation, and individual manifest entries.
Refactor storage-stack and filesystem collection into a shared module: add internal/backup/collector_storage_stack_common.go containing common collectors (fstab, crypttab, iSCSI, multipath, mdadm, LVM, systemd mount units, autofs, referenced files) and helper parsers. Replace PBS-specific brick IDs with common_* IDs and introduce newCommonFilesystemBricks/newCommonStorageStackBricks. Update recipes to use the common bricks (remove newPBSStorageStackBricks from PBS recipe, add common bricks to system recipe and inventory adjustments). Update tests to reflect that inventory-only adapter no longer copies storage-stack files into the backup tree. Also reorganize system recipe bricks and adjust logging/flow for kernel/hardware/critical files and other system collectors.
Remove several thin compatibility wrapper collectors (PBS and PVE) and update tests to exercise the new recipe/brick-based flows directly. Legacy methods removed include PBS: collectPBSCommands, collectDatastoreConfigs, collectPBSPxarMetadata, collectPBSDatastoreInventory, collectUserConfigs; PVE: collectPVEDirectories, collectPVECommands, collectPVEJobs, collectPVESchedules, collectPVEReplication, collectPVEStorageMetadata, collectPVECephInfo, createPVEInfoAliases, and related helpers. Tests were adapted to use runRecipeForTest, runSelectedBricksForTest, and direct recipe runs (newPBSDatastoreConfigRecipe, newPBSCommandsRecipe, newPBSPXARRecipe, newPBSDatastoreInventoryRecipe, newPBSUserConfigRecipe, newPVERecipe, etc.), setting collectionState fields as needed. A few tests were renamed to reflect recipe semantics and assertions were adjusted to check recipe outputs or skipped outputs instead of expecting legacy wrapper behavior. Files changed: internal/backup/collector_pbs.go, collector_pbs_datastore.go, collector_pbs_datastore_inventory.go, collector_pve.go and many PBS/PVE test files to align with the recipe/brick architecture.
Remove redundant wrapper helpers and update tests to call the context-aware implementations directly. Moved recipeBrickIDs into the test file; removed maybeUpgradeIdentityFile, writeIdentityFile and setImmutableAttribute wrappers and updated tests to use the corresponding *WithContext variants. Also removed resolveAndCheckPath and updated tests to call resolvePathWithinRootFS(osFS{}, ...) so the filesystem dependency is explicit. These changes reduce duplication and make context propagation and FS injection explicit in tests.
Remove several file helper functions and their tests. Updated docs to clarify pkg/ is for shared helper packages (not a stable external API). Deleted EnsureDir, ComputeSHA256, GetFileSize, AbsPath, IsAbsPath (and removed related imports) from pkg/utils/files.go and removed their corresponding unit tests from pkg/utils/files_test.go.
Implement automatic extraction of PBS SSL certificate SHA256 fingerprints and only auto-detect for local PBS repositories. Added extractFingerprintFromCert that parses PEM/x509 certificates and returns a colon-separated SHA256 fingerprint. Introduced pbsFingerprintCertPaths and configHostnameFunc (for testing) and updated autoDetectPBSFingerprint to iterate over the configurable cert paths. Added helper logic to determine whether a repository refers to a local PBS host (shouldAutoDetectLocalPBSFingerprint, parsePBSRepositoryHost, isLocalPBSHost, normalizePBSHost) so remote repositories won't use local certs. Added tests that generate temporary certificates to verify fingerprint extraction and the local/remote auto-detection behavior.
Refactor system and PBS inventory collection by splitting large runtime bricks into multiple focused bricks (network addr/rules/routes/links/neighbors/bridges/inventory/bonding/DNS and firewall iptables/ip6tables/nftables/ufw/firewalld). Introduce a systemCommandsBrick helper to reduce duplication when collecting command outputs. Add dedicated PBS inventory host-command collectors for dmsetup, LVM, mdadm, multipath and iSCSI and wire them into the PBS recipe. Update tests to cover the new bricks, their produced files/keys, and command output behavior. This improves separation of concerns, testability and allows per-brick gating/configuration of collections.
Break system runtime collection bricks into finer-grained units (storage mounts, storage block devices, compute memory/CPU, compute bus inventory, packages installed, packages APT policy). Update newSystemRecipe to register the new BrickIDs and wire each to new collector methods. Implement corresponding collector methods (collectSystemStorageMountsRuntime, collectSystemStorageBlockDevicesRuntime, collectSystemComputeMemoryCPURuntime, collectSystemComputeBusInventoryRuntime, collectSystemPackagesInstalledRuntime, collectSystemPackagesAptPolicyRuntime) and adjust previous functions to return/forward appropriately. Add tests verifying per-brick file families and gating behavior for package/apt collection, plus helper listRelativeFiles for test assertions. Changes affect internal/backup/collector_bricks.go, internal/backup/collector_bricks_test.go and internal/backup/collector_system.go.
Detect and handle co-installed Proxmox VE + Proxmox Backup Server (dual) environments. Introduces EnvironmentInfo (Type, Version, PVEVersion, PBSVersion) and a detectEnvironmentInfo flow that returns combined versions; retains a detectProxmox compatibility wrapper. Backups/manifest now include proxmox_targets and separate pve/pbs version fields. Adds dual collection recipe and Collector.CollectDualConfigs plus newDualRecipe and manifest updates. Orchestrator APIs updated to accept EnvironmentInfo (SetEnvironmentInfo, RunGoBackup signature changes, InitializeBackupStats uses env info). Compatibility/SystemType extended to recognize and reason about "dual" targets (targets, Overlaps, validation, categories, storage/mount handling, network checks). Email notifier: improved preflight/delivery PMF fallback logic for relay when recipient detection fails or root recipients are blocked; preserves original errors and records fallback metadata; tests added/updated for these behaviors. Various tests and helpers updated to cover dual support.
Bumps [golang.org/x/term](https://github.com/golang/term) from 0.41.0 to 0.42.0. - [Commits](golang/term@v0.41.0...v0.42.0) --- updated-dependencies: - dependency-name: golang.org/x/term dependency-version: 0.42.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.49.0 to 0.50.0. - [Commits](golang/crypto@v0.49.0...v0.50.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.50.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: tis24dev <71268257+tis24dev@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds dual Proxmox (PVE+PBS) support and per-product versioning; refactors collectors into a recipe/brick orchestration with stateful steps; propagates EnvironmentInfo through orchestrator and manifests; replaces strict type checks with capability predicates; many tests updated to exercise recipes and new flows. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Main as cmd/proxsave/main
participant Env as Environment Detector
participant Orch as Orchestrator
participant Collector as Backup Collector
participant Recipe as Recipe Engine
CLI->>Main: start backup
Main->>Env: detectEnvironmentInfo()
Env-->>Main: EnvironmentInfo{Type,PVEVersion,PBSVersion,Version}
Main->>Orch: SetEnvironmentInfo(envInfo)
Main->>Orch: RunGoBackup(ctx, envInfo, hostname)
Orch->>Collector: CollectAll(ctx)
Collector->>Recipe: choose recipe based on envInfo.Type
Recipe->>Recipe: runRecipe(ctx, recipe, state)
loop bricks
Recipe->>Collector: execute brick runner (PVE/PBS/System)
Collector-->>Recipe: result / error
end
Collector->>Collector: WriteManifest(ProxmoxTargets,PVEVersion,PBSVersion)
Orch-->>CLI: BackupStats + manifest
sequenceDiagram
participant UI as Restore UI
participant Decision as Restore Decision
participant Plan as Restore Plan
participant Restore as Restore Engine
UI->>Decision: analyze archive metadata
Decision->>Decision: parse BACKUP_TARGETS / categories
alt dual targets detected
Decision->>Plan: set SystemTypeDual
else single target
Decision->>Plan: set PVE or PBS
end
Plan->>Restore: apply restore plan
alt SupportsPVE and SupportsPBS
Restore->>Restore: run PVE restore bricks
Restore->>Restore: run PBS restore bricks
else SupportsPVE
Restore->>Restore: run PVE restore bricks
else SupportsPBS
Restore->>Restore: run PBS restore bricks
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/orchestrator/restore_workflow_ui.go (1)
816-849:⚠️ Potential issue | 🟡 MinorZFS pool verification is missing for
SystemTypeDual.After the rewrite, the
zfspost-restore check (lines 841-848) only runs undercase systemType == SystemTypePBS. Dual systems frequently use ZFS on both PVE and PBS sides, and thezfscategory is applicable to dual systems too. With the current structure, a user on aSystemTypeDualhost withzfsinplan.NormalCategorieswill silently skipcheckZFSPoolsAfterRestore.Either move the ZFS block outside the switch (it only depends on
plan.NormalCategories, notsystemType), or duplicate it into the Dual branch.🛠 Proposed fix — hoist ZFS check out of the switch
- switch { - case systemType == SystemTypeDual: + switch { + case systemType == SystemTypeDual: if needsClusterRestore && clusterServicesStopped { logger.Info(" PVE services were stopped/restarted during restore; verify status with: pvecm status") } else { logger.Info(" PVE services: systemctl restart pve-cluster pvedaemon pveproxy") } if pbsServicesStopped { logger.Info(" PBS services were stopped/restarted during restore; verify status with: systemctl status proxmox-backup proxmox-backup-proxy") } else { logger.Info(" PBS services: systemctl restart proxmox-backup-proxy proxmox-backup") } case systemType == SystemTypePVE: if needsClusterRestore && clusterServicesStopped { logger.Info(" PVE services were stopped/restarted during restore; verify status with: pvecm status") } else { logger.Info(" PVE services: systemctl restart pve-cluster pvedaemon pveproxy") } case systemType == SystemTypePBS: if pbsServicesStopped { logger.Info(" PBS services were stopped/restarted during restore; verify status with: systemctl status proxmox-backup proxmox-backup-proxy") } else { logger.Info(" PBS services: systemctl restart proxmox-backup-proxy proxmox-backup") } - - if hasCategoryID(plan.NormalCategories, "zfs") { - logger.Info("") - if err := checkZFSPoolsAfterRestore(logger); err != nil { - logger.Warning("ZFS pool check: %v", err) - } - } else { - logger.Debug("Skipping ZFS pool verification (ZFS category not selected)") - } } + + // ZFS verification is independent of system type; both PVE and PBS (and dual) can use ZFS. + if hasCategoryID(plan.NormalCategories, "zfs") { + logger.Info("") + if err := checkZFSPoolsAfterRestore(logger); err != nil { + logger.Warning("ZFS pool check: %v", err) + } + } else { + logger.Debug("Skipping ZFS pool verification (ZFS category not selected)") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_workflow_ui.go` around lines 816 - 849, The ZFS post-restore check is only executed for SystemTypePBS, so add the same check for SystemTypeDual by hoisting the hasCategoryID(plan.NormalCategories, "zfs") / checkZFSPoolsAfterRestore(logger) block out of the switch: leave the switch handling service messages as-is, then immediately after the switch add the ZFS check that calls hasCategoryID(plan.NormalCategories, "zfs") and on true runs checkZFSPoolsAfterRestore(logger) and logs a Warning on error (matching the existing PBS branch behavior), otherwise log the existing Debug message; this ensures SystemTypeDual and all other relevant types run the ZFS verification.internal/backup/collector_pve_util_test.go (1)
780-956:⚠️ Potential issue | 🟠 MajorThese PVE brick tests no longer assert any outcome.
After the migration to
runSelectedBricksForTest,TestCollectPVEDirectoriesClusteredMode,TestCollectPVEDirectoriesFirewallAsDirectory,TestCollectPVEDirectoriesFirewallAsFile,TestCollectPVEDirectoriesVZDumpConfig,TestCollectPVEDirectoriesVZDumpRelativePath,TestCollectPVEDirectoriesDisabledOptions, andTestCollectPVEDirectoriesWithConfigDBonly verify that the runner does not fail — they no longer check that the expected files (corosync.conf,firewall/cluster.fw,vzdump.conf,config.db, etc.) were actually copied intocollector.tempDir, nor that the disabled-options test produced no copies.A regression in any of these bricks (e.g., the Firewall brick silently skipping directory mode, or the VZDump brick failing to resolve a relative path) would pass this test suite unchanged. Please re-add the assertions that used to exist in the pre-refactor versions.
♻️ Example: minimal assertions to restore for the firewall-as-directory test
runSelectedBricksForTest(t, context.Background(), collector, newPVERecipe(), nil, brickPVEFirewallSnapshot) + + dest := filepath.Join(tmpDir, "etc", "pve", "firewall", "cluster.fw") + if _, err := os.Stat(dest); err != nil { + t.Fatalf("expected firewall/cluster.fw copied, got %v", err) + }The same pattern should be applied to each of the listed tests (or, for
TestCollectPVEDirectoriesDisabledOptions, assert that the destination paths do not exist).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pve_util_test.go` around lines 780 - 956, Each test must re-add assertions that verify expected files were copied into the collector temp directory (use the collector instance returned by NewCollector and inspect collector.tempDir); specifically, in TestCollectPVEDirectoriesClusteredMode assert corosync.conf exists under collector.tempDir (and cluster snapshots when BackupClusterConfig=true), in TestCollectPVEDirectoriesFirewallAsDirectory assert firewall/cluster.fw exists under collector.tempDir, in TestCollectPVEDirectoriesFirewallAsFile assert firewall (single file) exists, in TestCollectPVEDirectoriesVZDumpConfig and TestCollectPVEDirectoriesVZDumpRelativePath assert vzdump.conf was copied to collector.tempDir (resolve relative path for the latter), in TestCollectPVEDirectoriesWithConfigDB assert config.db exists under collector.tempDir, and in TestCollectPVEDirectoriesDisabledOptions assert that none of the above destination paths exist; add these minimal existence/non-existence checks after runSelectedBricksForTest in each named test.internal/backup/collector_pbs_datastore.go (1)
548-591:⚠️ Potential issue | 🟠 MajorPropagate parent context cancellation from the PXAR worker runner.
The current implementation suppresses all
context.Cancelederrors, including parent cancellation. When the parent context is canceled after workers start but before any worker reports a non-cancellation error, the function returnsnilinstead of propagating the cancellation. This breaks the expected error propagation pattern used throughout the codebase and affects standalone PXAR recipes or the last brick in a sequence.The fix should preserve the parent context and check it instead of the derived context at the end:
Suggested fix
func (c *Collector) runPBSPXARStep(ctx context.Context, state *pbsPxarState, fn func(context.Context, pbsDatastore, *pbsPxarState) error) error { if err := ctx.Err(); err != nil { return err } if state == nil || len(state.eligible) == 0 { return nil } dsWorkers := c.config.PxarDatastoreConcurrency if dsWorkers <= 0 { dsWorkers = 1 } - ctx, cancel := context.WithCancel(ctx) + parentCtx := ctx + ctx, cancel := context.WithCancel(parentCtx) defer cancel() ... if firstErr != nil { return firstErr } - if err := ctx.Err(); err != nil && !errors.Is(err, context.Canceled) { + if err := parentCtx.Err(); err != nil { return err } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pbs_datastore.go` around lines 548 - 591, The code is shadowing the parent context and later checks the derived ctx for errors, which causes parent cancellation to be suppressed; save the original parent context (e.g. parentCtx := ctx) before creating the derived ctx (ctx, cancel := context.WithCancel(parentCtx)), and at the end replace the final ctx.Err() check with a check of parentCtx.Err() and return that error when non-nil so parent cancellation is propagated (keep the existing worker error handling and firstErr logic intact).
🧹 Nitpick comments (11)
docs/DEVELOPER_GUIDE.md (1)
119-119: Standardize project name casing in this line for doc consistency.Nice clarification overall. Consider changing
ProxSavetoProxsavehere to match the naming used across this guide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DEVELOPER_GUIDE.md` at line 119, Update the casing of the project name in the docs entry that currently reads "ProxSave" to the standardized form "Proxsave" so it matches the rest of the guide; locate the occurrence of the literal string "ProxSave" (e.g., in the tree line that starts with "├── pkg/") and replace it with "Proxsave".internal/environment/detect_test.go (1)
137-157: Optional: assert PVE/PBS version fields for the dual fixture.The table loop only validates
TypeandVersionare non-empty, so the newPVEVersion/PBSVersionfields on the dual fixture aren't actually exercised. Consider adding a case-specific assertion (e.g. forProxmoxDual, require both sub-versions to be non-empty) so regressions in the dual-version model are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/environment/detect_test.go` around lines 137 - 157, The test loop only checks EnvironmentInfo.Type and Version, so the dual fixture's PVEVersion/PBSVersion aren't validated; update the subtest for the "dual environment" case (or when tt.envInfo.Type == types.ProxmoxDual) to assert that tt.envInfo.PVEVersion and tt.envInfo.PBSVersion are non-empty (use the EnvironmentInfo struct fields PVEVersion and PBSVersion and the ProxmoxDual type constant) so regressions in the dual-version model are caught.internal/orchestrator/categories.go (2)
488-491: Minor: simplify the"dual"branch.The loop just copies every element of
all. You can returnalldirectly (since it's already a fresh slice fromGetAllCategories()).♻️ Suggested tweak
- case "dual": - for _, cat := range all { - categories = append(categories, cat) - } + case "dual": + categories = append(categories, all...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/categories.go` around lines 488 - 491, The "dual" case is copying every element from the slice all into categories with an unnecessary loop; replace the loop by directly using the all slice (e.g., assign categories = all or return all directly) inside the switch branch where the "dual" case is handled (refer to the "dual" branch, variables all and categories, and the GetAllCategories() call) to simplify and avoid the redundant element-by-element append.
591-598: Consider building the"dual"set from the"pve"+"pbs"selections to avoid drift.The hardcoded ID list here duplicates the two branches above; if
"pve"or"pbs"gains/loses an ID (e.g. a new PBS job category), this branch will silently drift. Using a set built from the existing selections keeps the three branches in sync:♻️ Suggested refactor
- case "dual": - for _, cat := range all { - if cat.ID == "pve_cluster" || cat.ID == "storage_pve" || cat.ID == "pve_jobs" || - cat.ID == "pbs_config" || cat.ID == "datastore_pbs" || cat.ID == "maintenance_pbs" || cat.ID == "pbs_jobs" || cat.ID == "pbs_remotes" || - cat.ID == "zfs" || cat.ID == "filesystem" || cat.ID == "storage_stack" { - categories = append(categories, cat) - } - } + case "dual": + seen := make(map[string]struct{}) + for _, cat := range GetStorageModeCategories("pve") { + if _, ok := seen[cat.ID]; ok { + continue + } + seen[cat.ID] = struct{}{} + categories = append(categories, cat) + } + for _, cat := range GetStorageModeCategories("pbs") { + if _, ok := seen[cat.ID]; ok { + continue + } + seen[cat.ID] = struct{}{} + categories = append(categories, cat) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/categories.go` around lines 591 - 598, The "dual" case duplicates the hardcoded ID list from the "pve" and "pbs" branches causing drift; change the "dual" branch in the switch (case "dual") to build a set of allowed IDs by reusing the selections from the "pve" and "pbs" branches (e.g., create a map[string]bool of IDs gathered where those branches append to categories), then iterate over all and append cat to categories when cat.ID exists in that set; reference the variables/function scope names "case \"dual\"", "all", "categories", and "cat.ID" when making the change so the three branches stay in sync.internal/orchestrator/directory_recreation.go (1)
846-864: Consider refactoring theswitchto independentifchecks to remove ordering dependency.The current code requires
SystemTypeDualto be checked beforeSupportsPVE()/SupportsPBS()because both of these methods returntrueforSystemTypeDual(verified:SupportsPVE()returnss == SystemTypePVE || s == SystemTypeDual, andSupportsPBS()returnss == SystemTypePBS || s == SystemTypeDual). Reordering the cases would silently break dual-system support. Replacing theswitchwith two independentifchecks removes this fragile ordering constraint and clarifies that PVE and PBS recreation are independent operations.♻️ Suggested refactor
- switch { - case systemType == SystemTypeDual: - if err := RecreateStorageDirectories(logger); err != nil { - return fmt.Errorf("recreate PVE storage directories: %w", err) - } - if err := RecreateDatastoreDirectories(logger); err != nil { - return fmt.Errorf("recreate PBS datastore directories: %w", err) - } - case systemType.SupportsPVE(): - if err := RecreateStorageDirectories(logger); err != nil { - return fmt.Errorf("recreate PVE storage directories: %w", err) - } - case systemType.SupportsPBS(): - if err := RecreateDatastoreDirectories(logger); err != nil { - return fmt.Errorf("recreate PBS datastore directories: %w", err) - } - default: - logger.Debug("Unknown system type, skipping directory recreation") - } + handled := false + if systemType.SupportsPVE() { + handled = true + if err := RecreateStorageDirectories(logger); err != nil { + return fmt.Errorf("recreate PVE storage directories: %w", err) + } + } + if systemType.SupportsPBS() { + handled = true + if err := RecreateDatastoreDirectories(logger); err != nil { + return fmt.Errorf("recreate PBS datastore directories: %w", err) + } + } + if !handled { + logger.Debug("Unknown system type, skipping directory recreation") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/directory_recreation.go` around lines 846 - 864, Replace the switch on systemType with two independent if checks: if systemType.SupportsPVE() then call RecreateStorageDirectories(logger) and if systemType.SupportsPBS() then call RecreateDatastoreDirectories(logger); ensure both are executed for dual systems (SystemTypeDual) rather than relying on case ordering, and return wrapped errors from RecreateStorageDirectories and RecreateDatastoreDirectories (e.g. return the first error or aggregate both errors into a single fmt.Errorf message) while keeping the logger.Debug fallback for unknown/unsupported types.internal/orchestrator/network_apply.go (1)
516-539: Remove dead code: the legacyswitchis unreachable.
SupportsPVE()andSupportsPBS()returntrueforSystemTypePVE,SystemTypePBS, andSystemTypeDual, so the capability-based block always returns before the switch for any covered type. For unknown types, both predicates arefalse, sochecksremains empty and falls through todefault: return nil. The switch and its guard are redundant.Suggested cleanup
func defaultNetworkPortChecks(systemType SystemType) []tcpPortCheck { checks := make([]tcpPortCheck, 0, 2) if systemType.SupportsPVE() { checks = append(checks, tcpPortCheck{Name: "PVE web UI", Address: "127.0.0.1", Port: 8006}) } if systemType.SupportsPBS() { checks = append(checks, tcpPortCheck{Name: "PBS web UI", Address: "127.0.0.1", Port: 8007}) } - if len(checks) > 0 { - return checks - } - switch systemType { - case SystemTypePVE: - return []tcpPortCheck{ - {Name: "PVE web UI", Address: "127.0.0.1", Port: 8006}, - } - case SystemTypePBS: - return []tcpPortCheck{ - {Name: "PBS web UI", Address: "127.0.0.1", Port: 8007}, - } - default: - return nil - } + return checks }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/network_apply.go` around lines 516 - 539, The switch block in defaultNetworkPortChecks is unreachable and should be removed; rely solely on the capability-based logic using SystemType.SupportsPVE() and SupportsPBS() to build the []tcpPortCheck, and return checks (possibly nil or empty) directly—remove the switch on SystemType and any cases for SystemTypePVE/SystemTypePBS so the function only constructs checks via SupportsPVE/SupportsPBS and returns that slice.internal/orchestrator/restore_tui.go (1)
70-74: Minor: default branch text is misleading for non-PVE/non-PBS systemTypes.The
defaultarm renders "STORAGE/DATASTORE only - Storage or datastore configuration" for any unknownSystemType, whileSystemTypeDualgets its own explicit arm. If an unknown/Unknown system ever reaches this menu, the label suggests functionality that won't actually be present. Consider either an explicitSystemTypeUnknownbranch with a neutral label, or hiding the STORAGE option entirely in that case. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_tui.go` around lines 70 - 74, The default branch of the switch that sets storageText currently prints a misleading label for unknown SystemType values; update the switch in restore_tui.go to handle unknown systems explicitly by adding a new case (e.g., SystemTypeUnknown) with a neutral label like "STORAGE/DATASTORE not available for this system" or, alternatively, detect the unknown value and omit/hide the STORAGE option entirely; adjust the switch handling around SystemTypeDual and default so unknown types use the new neutral behavior instead of the current generic message.internal/orchestrator/restore_workflow_ui.go (1)
816-849: Minor: preferswitch systemTypeoverswitch { case systemType == X }.Since all three cases compare
systemTypeto a constant, the idiomatic form is:switch systemType { case SystemTypeDual: … case SystemTypePVE: … case SystemTypePBS: … }Same behavior, less noise, and it lets the compiler warn on duplicate cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_workflow_ui.go` around lines 816 - 849, Replace the current generic switch with a value switch on systemType (i.e., change "switch { case systemType == SystemTypeDual: ... }" to "switch systemType { case SystemTypeDual: ... case SystemTypePVE: ... case SystemTypePBS: ... }"), preserving all existing conditional logic and logger calls (including checks that reference needsClusterRestore, clusterServicesStopped, pbsServicesStopped, hasCategoryID(plan.NormalCategories, "zfs") and calling checkZFSPoolsAfterRestore), so behavior remains identical while allowing the compiler to detect duplicate cases.internal/config/config_test.go (1)
1203-1241: Optional: share a single test certificate across subtests.
writeTestPBSCertificategenerates a fresh 2048-bit RSA key each call (~100-300 ms). It's invoked from at least four tests. Consider memoizing withsync.Onceor aTestMaininitializer to reduce test runtime. Not blocking — only worth it if test-suite latency matters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 1203 - 1241, The writeTestPBSCertificate helper currently generates a new RSA key and certificate on every call; memoize its result to avoid repeated expensive generation by adding package-level variables (e.g., cachedCertPath, cachedCertSum) and a sync.Once (or initialize in TestMain) and update writeTestPBSCertificate to run the generation only once via that sync.Once and return the cached values on subsequent calls; reference writeTestPBSCertificate, cachedCertPath, cachedCertSum and the sync.Once in the change.internal/config/config.go (1)
1260-1291: Nit: fall back more explicitly when noCERTIFICATEPEM block is present.If the file is, e.g., a key-only PEM or empty after stripping non-CERTIFICATE blocks,
certDERstays set to the full file bytes and is passed tox509.ParseCertificate. Today that just returns an error and you return"", so behavior is safe, but it is slightly misleading — a reader may assume raw DER is intentionally supported. Consider returning""explicitly when no CERTIFICATE block was found instead of falling back to raw-bytes parsing.♻️ Optional clarification
- certDER := data - rest := data - for len(rest) > 0 { + var certDER []byte + rest := data + for len(rest) > 0 { block, remaining := pem.Decode(rest) if block == nil { break } rest = remaining if block.Type == "CERTIFICATE" && len(block.Bytes) > 0 { certDER = block.Bytes break } } + if len(certDER) == 0 { + return "" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 1260 - 1291, In extractFingerprintFromCert, avoid treating the raw file bytes as DER when no CERTIFICATE PEM block is found: add a flag (e.g., foundCert) that is set only when a pem.Decode block with Type == "CERTIFICATE" is located, set certDER from that block, and after the PEM loop return "" if foundCert is false (instead of leaving certDER as the original file bytes) so x509.ParseCertificate is only called with actual certificate DER data.internal/orchestrator/restore_decision.go (1)
290-311:ambiguousTypereturn value is now dead.Every arm of the
switchreturnsfalse, so the second value is alwaysfalse. As a consequence:
- The
case ambiguousType:branch inbuildRestoreDecisionInfo(Line 259–261) is unreachable.RestoreDecisionSourceAmbiguous(Line 23) is no longer assignable from production code.Either simplify the signature to return just
SystemType, or keep the second value and reintroduce a real ambiguous case; the current state is a code smell and makes the intent of Dual vs Unknown harder to audit.♻️ Suggested simplification
-func detectBackupTypeFromCategories(categories []Category) (SystemType, bool) { +func detectBackupTypeFromCategories(categories []Category) SystemType { var hasPVE, hasPBS bool for _, cat := range categories { switch cat.Type { case CategoryTypePVE: hasPVE = true case CategoryTypePBS: hasPBS = true } } switch { case hasPVE && hasPBS: - return SystemTypeDual, false + return SystemTypeDual case hasPVE: - return SystemTypePVE, false + return SystemTypePVE case hasPBS: - return SystemTypePBS, false + return SystemTypePBS default: - return SystemTypeUnknown, false + return SystemTypeUnknown } }Then drop the
case ambiguousType:branch andRestoreDecisionSourceAmbiguousif no other caller uses them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_decision.go` around lines 290 - 311, The helper detectBackupTypeFromCategories always returns the second bool as false, making the ambiguousType branch in buildRestoreDecisionInfo and the RestoreDecisionSourceAmbiguous value dead; fix by simplifying detectBackupTypeFromCategories to return only a SystemType and update all callers (notably buildRestoreDecisionInfo) to drop the unused boolean and the unreachable case handling (remove the case ambiguousType branch and any uses of RestoreDecisionSourceAmbiguous), or alternatively if you want to preserve ambiguity semantics, change detectBackupTypeFromCategories to return true for the ambiguous flag when both CategoryTypePVE and CategoryTypePBS are present and adjust buildRestoreDecisionInfo to handle that true flag—pick one approach and update function signature, call sites, and any related enums accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/backup/checksum.go`:
- Around line 226-234: The current handling of BACKUP_TARGETS/PROXMOX_TARGETS
resets legacy.ProxmoxTargets with [:0] on each match, causing later keys to drop
targets parsed earlier; change the logic in the switch case for "BACKUP_TARGETS"
and "PROXMOX_TARGETS" to avoid unconditional clearing — either only clear when
legacy.ProxmoxTargets is nil/empty or merge new targets into the existing slice
and deduplicate (e.g., track seen strings and append only unseen targets) so
both keys are treated as aliases rather than last-write-wins.
In `@internal/backup/collector_pbs_datastore_inventory.go`:
- Around line 186-194: populatePBSInventoryAutofsFiles currently records only
/etc/auto.master, /etc/autofs.conf and the auto.master.d directory, missing all
/etc/auto.* map files; update the function (populatePBSInventoryAutofsFiles) to
also discover and add any /etc/auto.* files into report.Files using the existing
c.captureInventoryFile helper (e.g., glob /etc/auto.* and skip
directories/auto.master itself), using a clear key (basename or prefixed like
"autofs_map_<name>") so map files are included in the inventory alongside
autofs_master and autofs_conf.
In `@internal/backup/collector_pve.go`:
- Around line 478-484: The node and storage runtime lookups (calls to
captureCommandOutput that write nodes_status.json and the per-node storage file)
should be best-effort — do not return an error that aborts the whole PVE backup.
Replace the current error branch in the captureCommandOutput call around "pvesh
get /nodes --output-format=json" (which currently returns fmt.Errorf("failed to
get node status: %w", err)) with code that logs a warning/error (using the
collector's logger) and continues; only process the nodeData if len(nodeData) >
0. Make the same change for the similar captureCommandOutput call that fetches
"/nodes/<node>/storage" (the block referenced at lines ~580-586): log and
continue on error instead of returning. Ensure filenames nodes_status.json and
the per-node storage file handling remain intact.
In `@internal/backup/collector_storage_stack_common.go`:
- Around line 16-23: Change the log level for missing optional storage component
copies from a warning to a debug so absent-but-expected files (iscsi, multipath,
mdadm, autofs, crypttab, etc.) don’t create noisy warnings; locate the
safeCopyFile call sites (e.g., the call copying "/etc/fstab" in the collector
implementation and the other similar calls referenced in the diff ranges) and
replace c.logger.Warning("Failed to collect ...: %v", err) with
c.logger.Debug("Optional storage file not present or failed to collect ...: %v",
err) (or equivalent debug wording) so real errors remain warnings while
expected/missing components are logged at debug.
- Around line 259-263: The Stat call on base (computed via
c.systemPath("/etc/systemd/system")) currently swallows all errors by returning
nil; change the error handling so only os.IsNotExist(err) is treated as "nothing
to collect" (return nil), and any other error is propagated back to the caller
(return err or wrap it with context). Update the block that calls os.Stat(base)
to check os.IsNotExist(err) and handle non-ENOENT errors by returning them
(include context mentioning base if you wrap the error) so permission/I/O issues
are surfaced instead of silently ignored.
In `@internal/backup/collector_system.go`:
- Around line 978-981: In collectSystemLVMRuntime, the LVM gate currently checks
c.systemPath("/sbin/pvs") which misses pvs when it's on PATH (e.g., /usr/sbin);
replace that hard-coded check with a depLookPath("pvs") lookup (like zpool/zfs
use) and use that result to decide whether to proceed; ensure the same pattern
is used for the vgs and lvs command checks (use depLookPath("vgs") and
depLookPath("lvs") or rely on the presence of the pvs lookup) so the function
calls to invoke those commands match PATH resolution and are consistent with the
rest of the collector.
In `@internal/config/config_test.go`:
- Around line 1150-1192: The three new tests
TestAutoDetectPBSAuthLocalRepositoryAutoDetectsFingerprint,
TestAutoDetectPBSAuthRemoteRepositorySkipsLocalFingerprint, and
TestAutoDetectPBSAuthEmptyRepositoryAutoDetectsFingerprint must clear any
existing PBS-related environment variables before calling cfg.autoDetectPBSAuth;
add calls like t.Setenv("PBS_FINGERPRINT", ""), t.Setenv("PBS_REPOSITORY", ""),
and t.Setenv("PBS_PASSWORD", "") at the start of each test (or a small helper
invoked by each) so the tests run with a clean environment and reliably exercise
Config.autoDetectPBSAuth.
In `@internal/config/config.go`:
- Around line 1333-1361: isLocalPBSHost currently treats pre-dot shortnames as
equal unconditionally, causing false positives (e.g., pbs.example.com vs
pbs.other.com); update the final short-name comparison in isLocalPBSHost so that
short-name equality only counts when at least one of the compared inputs is a
pure short name (contains no dot) or when the full FQDNs match; modify the final
return logic that computes currentShort and hostShort to require (hostShort ==
currentShort) && (strings.Index(host, ".") < 0 || strings.Index(currentHost,
".") < 0) or equivalent, ensuring FQDN equality still returns true and
short-name matches only if one side is truly short.
- Around line 1306-1331: parsePBSRepositoryHost currently returns "" when there
is no "@" (so inputs like "localhost:datastore" are ignored); update it to
handle the case where the username is omitted by treating the final ":datastore"
as the datastore suffix and extracting the host from the part before that final
colon. Concretely, when at < 0 (no "@") set hostPart = repo (after TrimSpace),
if hostPart is bracketed handle IPv6 as today (look for matching "]"), otherwise
split hostPart on the last ":" (not the first) and use the left side as the host
to pass into normalizePBSHost; keep existing behavior for empty/invalid parts
and preserve normalizePBSHost usage; ensure this change is reflected where
parsePBSRepositoryHost is used (e.g., shouldAutoDetectLocalPBSFingerprint).
In `@internal/environment/detect.go`:
- Around line 113-117: The current branch in detectEnvironmentInfo handling
(variables info, err) discards the original detection error when info.Type ==
types.ProxmoxUnknown; preserve and return the original err (or wrap it) instead
of replacing it with a generic string. Update the block in the function that
checks info.Type == types.ProxmoxUnknown to return info and the existing err (or
fmt.Errorf("unable to detect Proxmox environment: %w", err) if you want extra
context) so the detailed diagnostic from detectEnvironmentInfo() is retained.
- Around line 166-186: normalizedDetectedVersion currently preserves the
"unknown" sentinel so combineVersions can produce "pve=unknown,pbs=unknown" and
incorrectly signal success; update normalizedDetectedVersion (and/or
combineVersions) to treat "unknown" (case-insensitive) as a missing value by
returning an empty string for inputs equal to "unknown" after TrimSpace, and
ensure combineVersions treats empty strings as missing when composing the final
string (so both unknowns yield the default "unknown" result instead of
"pve=unknown,pbs=unknown").
In `@internal/notify/email.go`:
- Around line 248-260: The warning logs and the constructed error include raw
recipient addresses; replace all uses of recipient in the two logger.Warning
calls and in the preflightFallbackCause fmt.Errorf with redactEmail(recipient)
so the logs and the error contain the redacted address; update the Debug log
that prints preflightFallbackCause (and any other places in this decision
branch) to ensure they only surface the redacted value (i.e., construct
preflightFallbackCause using redactEmail(recipient) so logger.Debug("%v") is
safe).
- Around line 405-407: The code currently leaves NotificationResult.Error set to
the original failure when a fallback later succeeds (the block using err, cause
and result.Error), creating an inconsistent state; change the logic so that when
err == nil (fallback succeeded) you do NOT assign the prior cause to
result.Error — either remove that assignment or explicitly set result.Error =
nil so NotificationResult reflects success (Success=true and Error=nil). Ensure
you update the block that inspects err and cause (the variables named err, cause
and the struct NotificationResult/result.Error) so successful fallbacks clear
any prior error.
---
Outside diff comments:
In `@internal/backup/collector_pbs_datastore.go`:
- Around line 548-591: The code is shadowing the parent context and later checks
the derived ctx for errors, which causes parent cancellation to be suppressed;
save the original parent context (e.g. parentCtx := ctx) before creating the
derived ctx (ctx, cancel := context.WithCancel(parentCtx)), and at the end
replace the final ctx.Err() check with a check of parentCtx.Err() and return
that error when non-nil so parent cancellation is propagated (keep the existing
worker error handling and firstErr logic intact).
In `@internal/backup/collector_pve_util_test.go`:
- Around line 780-956: Each test must re-add assertions that verify expected
files were copied into the collector temp directory (use the collector instance
returned by NewCollector and inspect collector.tempDir); specifically, in
TestCollectPVEDirectoriesClusteredMode assert corosync.conf exists under
collector.tempDir (and cluster snapshots when BackupClusterConfig=true), in
TestCollectPVEDirectoriesFirewallAsDirectory assert firewall/cluster.fw exists
under collector.tempDir, in TestCollectPVEDirectoriesFirewallAsFile assert
firewall (single file) exists, in TestCollectPVEDirectoriesVZDumpConfig and
TestCollectPVEDirectoriesVZDumpRelativePath assert vzdump.conf was copied to
collector.tempDir (resolve relative path for the latter), in
TestCollectPVEDirectoriesWithConfigDB assert config.db exists under
collector.tempDir, and in TestCollectPVEDirectoriesDisabledOptions assert that
none of the above destination paths exist; add these minimal
existence/non-existence checks after runSelectedBricksForTest in each named
test.
In `@internal/orchestrator/restore_workflow_ui.go`:
- Around line 816-849: The ZFS post-restore check is only executed for
SystemTypePBS, so add the same check for SystemTypeDual by hoisting the
hasCategoryID(plan.NormalCategories, "zfs") / checkZFSPoolsAfterRestore(logger)
block out of the switch: leave the switch handling service messages as-is, then
immediately after the switch add the ZFS check that calls
hasCategoryID(plan.NormalCategories, "zfs") and on true runs
checkZFSPoolsAfterRestore(logger) and logs a Warning on error (matching the
existing PBS branch behavior), otherwise log the existing Debug message; this
ensures SystemTypeDual and all other relevant types run the ZFS verification.
---
Nitpick comments:
In `@docs/DEVELOPER_GUIDE.md`:
- Line 119: Update the casing of the project name in the docs entry that
currently reads "ProxSave" to the standardized form "Proxsave" so it matches the
rest of the guide; locate the occurrence of the literal string "ProxSave" (e.g.,
in the tree line that starts with "├── pkg/") and replace it with "Proxsave".
In `@internal/config/config_test.go`:
- Around line 1203-1241: The writeTestPBSCertificate helper currently generates
a new RSA key and certificate on every call; memoize its result to avoid
repeated expensive generation by adding package-level variables (e.g.,
cachedCertPath, cachedCertSum) and a sync.Once (or initialize in TestMain) and
update writeTestPBSCertificate to run the generation only once via that
sync.Once and return the cached values on subsequent calls; reference
writeTestPBSCertificate, cachedCertPath, cachedCertSum and the sync.Once in the
change.
In `@internal/config/config.go`:
- Around line 1260-1291: In extractFingerprintFromCert, avoid treating the raw
file bytes as DER when no CERTIFICATE PEM block is found: add a flag (e.g.,
foundCert) that is set only when a pem.Decode block with Type == "CERTIFICATE"
is located, set certDER from that block, and after the PEM loop return "" if
foundCert is false (instead of leaving certDER as the original file bytes) so
x509.ParseCertificate is only called with actual certificate DER data.
In `@internal/environment/detect_test.go`:
- Around line 137-157: The test loop only checks EnvironmentInfo.Type and
Version, so the dual fixture's PVEVersion/PBSVersion aren't validated; update
the subtest for the "dual environment" case (or when tt.envInfo.Type ==
types.ProxmoxDual) to assert that tt.envInfo.PVEVersion and
tt.envInfo.PBSVersion are non-empty (use the EnvironmentInfo struct fields
PVEVersion and PBSVersion and the ProxmoxDual type constant) so regressions in
the dual-version model are caught.
In `@internal/orchestrator/categories.go`:
- Around line 488-491: The "dual" case is copying every element from the slice
all into categories with an unnecessary loop; replace the loop by directly using
the all slice (e.g., assign categories = all or return all directly) inside the
switch branch where the "dual" case is handled (refer to the "dual" branch,
variables all and categories, and the GetAllCategories() call) to simplify and
avoid the redundant element-by-element append.
- Around line 591-598: The "dual" case duplicates the hardcoded ID list from the
"pve" and "pbs" branches causing drift; change the "dual" branch in the switch
(case "dual") to build a set of allowed IDs by reusing the selections from the
"pve" and "pbs" branches (e.g., create a map[string]bool of IDs gathered where
those branches append to categories), then iterate over all and append cat to
categories when cat.ID exists in that set; reference the variables/function
scope names "case \"dual\"", "all", "categories", and "cat.ID" when making the
change so the three branches stay in sync.
In `@internal/orchestrator/directory_recreation.go`:
- Around line 846-864: Replace the switch on systemType with two independent if
checks: if systemType.SupportsPVE() then call RecreateStorageDirectories(logger)
and if systemType.SupportsPBS() then call RecreateDatastoreDirectories(logger);
ensure both are executed for dual systems (SystemTypeDual) rather than relying
on case ordering, and return wrapped errors from RecreateStorageDirectories and
RecreateDatastoreDirectories (e.g. return the first error or aggregate both
errors into a single fmt.Errorf message) while keeping the logger.Debug fallback
for unknown/unsupported types.
In `@internal/orchestrator/network_apply.go`:
- Around line 516-539: The switch block in defaultNetworkPortChecks is
unreachable and should be removed; rely solely on the capability-based logic
using SystemType.SupportsPVE() and SupportsPBS() to build the []tcpPortCheck,
and return checks (possibly nil or empty) directly—remove the switch on
SystemType and any cases for SystemTypePVE/SystemTypePBS so the function only
constructs checks via SupportsPVE/SupportsPBS and returns that slice.
In `@internal/orchestrator/restore_decision.go`:
- Around line 290-311: The helper detectBackupTypeFromCategories always returns
the second bool as false, making the ambiguousType branch in
buildRestoreDecisionInfo and the RestoreDecisionSourceAmbiguous value dead; fix
by simplifying detectBackupTypeFromCategories to return only a SystemType and
update all callers (notably buildRestoreDecisionInfo) to drop the unused boolean
and the unreachable case handling (remove the case ambiguousType branch and any
uses of RestoreDecisionSourceAmbiguous), or alternatively if you want to
preserve ambiguity semantics, change detectBackupTypeFromCategories to return
true for the ambiguous flag when both CategoryTypePVE and CategoryTypePBS are
present and adjust buildRestoreDecisionInfo to handle that true flag—pick one
approach and update function signature, call sites, and any related enums
accordingly.
In `@internal/orchestrator/restore_tui.go`:
- Around line 70-74: The default branch of the switch that sets storageText
currently prints a misleading label for unknown SystemType values; update the
switch in restore_tui.go to handle unknown systems explicitly by adding a new
case (e.g., SystemTypeUnknown) with a neutral label like "STORAGE/DATASTORE not
available for this system" or, alternatively, detect the unknown value and
omit/hide the STORAGE option entirely; adjust the switch handling around
SystemTypeDual and default so unknown types use the new neutral behavior instead
of the current generic message.
In `@internal/orchestrator/restore_workflow_ui.go`:
- Around line 816-849: Replace the current generic switch with a value switch on
systemType (i.e., change "switch { case systemType == SystemTypeDual: ... }" to
"switch systemType { case SystemTypeDual: ... case SystemTypePVE: ... case
SystemTypePBS: ... }"), preserving all existing conditional logic and logger
calls (including checks that reference needsClusterRestore,
clusterServicesStopped, pbsServicesStopped, hasCategoryID(plan.NormalCategories,
"zfs") and calling checkZFSPoolsAfterRestore), so behavior remains identical
while allowing the compiler to detect duplicate cases.
🪄 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: c0c93561-6df2-4f4e-a0cb-3118ab9c9ad6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (71)
CHANGELOG.mdcmd/proxsave/main.godocs/DEVELOPER_GUIDE.mdgo.modinternal/backup/checksum.gointernal/backup/collector.gointernal/backup/collector_bricks.gointernal/backup/collector_bricks_test.gointernal/backup/collector_dual.gointernal/backup/collector_manifest.gointernal/backup/collector_paths.gointernal/backup/collector_pbs.gointernal/backup/collector_pbs_commands_coverage_test.gointernal/backup/collector_pbs_datastore.gointernal/backup/collector_pbs_datastore_inventory.gointernal/backup/collector_pbs_datastore_inventory_test.gointernal/backup/collector_pbs_extra_test.gointernal/backup/collector_pbs_small_test.gointernal/backup/collector_pbs_test.gointernal/backup/collector_pve.gointernal/backup/collector_pve_test.gointernal/backup/collector_pve_util_test.gointernal/backup/collector_pxar_datastore_test.gointernal/backup/collector_real_flow_test.gointernal/backup/collector_storage_stack_common.gointernal/backup/collector_system.gointernal/backup/collector_system_test.gointernal/config/config.gointernal/config/config_test.gointernal/environment/detect.gointernal/environment/detect_hybrid_bug_test.gointernal/environment/detect_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/email.gointernal/notify/email_delivery_methods_test.gointernal/notify/email_test.gointernal/orchestrator/additional_helpers_test.gointernal/orchestrator/backup_config.gointernal/orchestrator/backup_config_test.gointernal/orchestrator/backup_safety.gointernal/orchestrator/backup_safety_test.gointernal/orchestrator/categories.gointernal/orchestrator/compatibility.gointernal/orchestrator/directory_recreation.gointernal/orchestrator/extensions.gointernal/orchestrator/mount_guard.gointernal/orchestrator/network_apply.gointernal/orchestrator/network_apply_workflow_ui.gointernal/orchestrator/network_health.gointernal/orchestrator/orchestrator.gointernal/orchestrator/orchestrator_test.gointernal/orchestrator/pbs_staged_apply.gointernal/orchestrator/pve_staged_apply.gointernal/orchestrator/restore.gointernal/orchestrator/restore_access_control.gointernal/orchestrator/restore_access_control_ui.gointernal/orchestrator/restore_decision.gointernal/orchestrator/restore_firewall.gointernal/orchestrator/restore_ha.gointernal/orchestrator/restore_notifications.gointernal/orchestrator/restore_notifications_pbs_api.gointernal/orchestrator/restore_plan.gointernal/orchestrator/restore_sdn.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_workflow_ui.gointernal/orchestrator/selective.gointernal/security/security.gointernal/types/common.gopkg/utils/files.gopkg/utils/files_test.go
💤 Files with no reviewable changes (5)
- CHANGELOG.md
- pkg/utils/files_test.go
- internal/identity/identity.go
- internal/orchestrator/backup_safety.go
- pkg/utils/files.go
Comment on lines
+16
to
+23
| if err := c.safeCopyFile( | ||
| ctx, | ||
| c.systemPath("/etc/fstab"), | ||
| filepath.Join(c.tempDir, "etc/fstab"), | ||
| "filesystem table", | ||
| ); err != nil { | ||
| c.logger.Warning("Failed to collect /etc/fstab: %v", err) | ||
| } |
There was a problem hiding this comment.
Treat missing optional storage components as debug, not warning.
These helpers are now part of the common flow, so hosts without iSCSI, multipath, mdadm, autofs, or crypttab will emit warnings on every run even when nothing is wrong. That will bury real backup problems in noise and make warning counts unreliable.
Also applies to: 33-40, 58-60, 71-87, 96-103, 121-123, 134-147, 165-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/backup/collector_storage_stack_common.go` around lines 16 - 23,
Change the log level for missing optional storage component copies from a
warning to a debug so absent-but-expected files (iscsi, multipath, mdadm,
autofs, crypttab, etc.) don’t create noisy warnings; locate the safeCopyFile
call sites (e.g., the call copying "/etc/fstab" in the collector implementation
and the other similar calls referenced in the diff ranges) and replace
c.logger.Warning("Failed to collect ...: %v", err) with c.logger.Debug("Optional
storage file not present or failed to collect ...: %v", err) (or equivalent
debug wording) so real errors remain warnings while expected/missing components
are logged at debug.
Comment on lines
+1333
to
+1361
| func isLocalPBSHost(host string) bool { | ||
| host = normalizePBSHost(host) | ||
| switch host { | ||
| case "", "localhost", "127.0.0.1", "::1": | ||
| return host != "" | ||
| } | ||
|
|
||
| currentHost, err := configHostnameFunc() | ||
| if err != nil { | ||
| return false | ||
| } | ||
| currentHost = normalizePBSHost(currentHost) | ||
| if currentHost == "" { | ||
| return false | ||
| } | ||
| if host == currentHost { | ||
| return true | ||
| } | ||
|
|
||
| currentShort := currentHost | ||
| if dot := strings.Index(currentShort, "."); dot > 0 { | ||
| currentShort = currentShort[:dot] | ||
| } | ||
| hostShort := host | ||
| if dot := strings.Index(hostShort, "."); dot > 0 { | ||
| hostShort = hostShort[:dot] | ||
| } | ||
| return host == currentShort || hostShort == currentHost || hostShort == currentShort | ||
| } |
There was a problem hiding this comment.
isLocalPBSHost short-name comparison can false-positive across domains.
The final block compares short (pre-dot) hostnames, so pbs.example.com (current host) and pbs.other.com (repo host) both short-resolve to pbs and isLocalPBSHost returns true. That will trigger local certificate fingerprint extraction for a genuinely remote repository, and the inferred fingerprint will not match the remote server — causing PBS client connections to fail with a fingerprint mismatch.
Consider requiring FQDN equality (or at least that one of the two values is a pure short name without any dot) before treating the hosts as the same machine:
♻️ Suggested tightening
- currentShort := currentHost
- if dot := strings.Index(currentShort, "."); dot > 0 {
- currentShort = currentShort[:dot]
- }
- hostShort := host
- if dot := strings.Index(hostShort, "."); dot > 0 {
- hostShort = hostShort[:dot]
- }
- return host == currentShort || hostShort == currentHost || hostShort == currentShort
+ // Only treat as equal when one side is a bare short name matching the other's short form.
+ hostHasDot := strings.Contains(host, ".")
+ currentHasDot := strings.Contains(currentHost, ".")
+ currentShort := currentHost
+ if currentHasDot {
+ currentShort = currentHost[:strings.Index(currentHost, ".")]
+ }
+ hostShort := host
+ if hostHasDot {
+ hostShort = host[:strings.Index(host, ".")]
+ }
+ if !hostHasDot && host == currentShort {
+ return true
+ }
+ if !currentHasDot && hostShort == currentHost {
+ return true
+ }
+ return false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func isLocalPBSHost(host string) bool { | |
| host = normalizePBSHost(host) | |
| switch host { | |
| case "", "localhost", "127.0.0.1", "::1": | |
| return host != "" | |
| } | |
| currentHost, err := configHostnameFunc() | |
| if err != nil { | |
| return false | |
| } | |
| currentHost = normalizePBSHost(currentHost) | |
| if currentHost == "" { | |
| return false | |
| } | |
| if host == currentHost { | |
| return true | |
| } | |
| currentShort := currentHost | |
| if dot := strings.Index(currentShort, "."); dot > 0 { | |
| currentShort = currentShort[:dot] | |
| } | |
| hostShort := host | |
| if dot := strings.Index(hostShort, "."); dot > 0 { | |
| hostShort = hostShort[:dot] | |
| } | |
| return host == currentShort || hostShort == currentHost || hostShort == currentShort | |
| } | |
| func isLocalPBSHost(host string) bool { | |
| host = normalizePBSHost(host) | |
| switch host { | |
| case "", "localhost", "127.0.0.1", "::1": | |
| return host != "" | |
| } | |
| currentHost, err := configHostnameFunc() | |
| if err != nil { | |
| return false | |
| } | |
| currentHost = normalizePBSHost(currentHost) | |
| if currentHost == "" { | |
| return false | |
| } | |
| if host == currentHost { | |
| return true | |
| } | |
| // Only treat as equal when one side is a bare short name matching the other's short form. | |
| hostHasDot := strings.Contains(host, ".") | |
| currentHasDot := strings.Contains(currentHost, ".") | |
| currentShort := currentHost | |
| if currentHasDot { | |
| currentShort = currentHost[:strings.Index(currentHost, ".")] | |
| } | |
| hostShort := host | |
| if hostHasDot { | |
| hostShort = host[:strings.Index(host, ".")] | |
| } | |
| if !hostHasDot && host == currentShort { | |
| return true | |
| } | |
| if !currentHasDot && hostShort == currentHost { | |
| return true | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 1333 - 1361, isLocalPBSHost currently
treats pre-dot shortnames as equal unconditionally, causing false positives
(e.g., pbs.example.com vs pbs.other.com); update the final short-name comparison
in isLocalPBSHost so that short-name equality only counts when at least one of
the compared inputs is a pure short name (contains no dot) or when the full
FQDNs match; modify the final return logic that computes currentShort and
hostShort to require (hostShort == currentShort) && (strings.Index(host, ".") <
0 || strings.Index(currentHost, ".") < 0) or equivalent, ensuring FQDN equality
still returns true and short-name matches only if one side is truly short.
Comment on lines
+405
to
+407
| if err == nil && cause != nil { | ||
| result.Error = cause | ||
| } |
There was a problem hiding this comment.
Don't leave result.Error populated after a successful fallback.
This makes NotificationResult internally inconsistent: Success=true later, but Error still contains the relay/preflight failure. Any caller or serializer that keys off result.Error will report a failed notification even though PMF succeeded.
Suggested fix
if backendPath != "" {
result.Metadata["email_backend_path"] = backendPath
}
- if err == nil && cause != nil {
- result.Error = cause
+ if err == nil && cause != nil {
+ result.Metadata["fallback_cause"] = cause.Error()
}
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err == nil && cause != nil { | |
| result.Error = cause | |
| } | |
| if err == nil && cause != nil { | |
| result.Metadata["fallback_cause"] = cause.Error() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/notify/email.go` around lines 405 - 407, The code currently leaves
NotificationResult.Error set to the original failure when a fallback later
succeeds (the block using err, cause and result.Error), creating an inconsistent
state; change the logic so that when err == nil (fallback succeeded) you do NOT
assign the prior cause to result.Error — either remove that assignment or
explicitly set result.Error = nil so NotificationResult reflects success
(Success=true and Error=nil). Ensure you update the block that inspects err and
cause (the variables named err, cause and the struct
NotificationResult/result.Error) so successful fallbacks clear any prior error.
Multiple fixes and improvements across backup collection, config parsing, environment detection, orchestration and tests: - Deduplicate and sanitize legacy Proxmox targets when parsing metadata. - Fix context cancellation checking in PBS pxar step to examine parent context. - Capture additional autofs map files via globbing and include them in inventory. - Switch some failing collector calls to warnings so collection continues when utilities fail. - Improve systemd mount unit handling to return errors for unexpected stat failures. - Only run LVM commands that are present on the system (pvs/vgs/lvs) instead of assuming binaries. - Harden certificate parsing and PBS repository host parsing/normalization; tighten local-host matching. - Add helper to clear PBS env vars in tests and update tests to assert expected artifacts and dual-mode behavior. - Return underlying errors from Detect() when available and treat "unknown" versions as empty. - Redact root email recipients in logs/errors when relay delivery is blocked. - Simplify category selection logic and storage-mode category building, including dual mode. - Recreate directories for both PVE and PBS when supported, aggregate errors, and add dual-path tests. - Simplify default network port checks to build based on supported systems. - Simplify restore decision detection (no ambiguous flag) and add related tests; adjust restore TUI/UI messages and flow. These changes increase robustness, improve logging/privacy, and expand test coverage for dual and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/orchestrator/restore_decision.go (1)
249-271:⚠️ Potential issue | 🟡 Minor
Sourcenot updated whenBackupTypeis derived fromBACKUP_TARGETS.At lines 251–253
info.BackupTypemay be populated frommetadata.BackupTargets, but the switch at 257–271 keys offmetadata.BackupType(notinfo.BackupType). Whenmetadata.BackupType == SystemTypeUnknownandcategoryType == SystemTypeUnknown, none of the four cases match andinfo.SourceremainsRestoreDecisionSourceUnknowneven thoughinfo.BackupTypewas successfully derived from internal metadata. Consumers that branch onSource(e.g., reporting provenance, trust levels) will treat a targets-derived decision as unknown-source.Consider recording the internal-metadata provenance when the targets path yields a concrete type:
🛠️ Proposed fix
if metadata != nil { info.BackupHostname = strings.TrimSpace(metadata.Hostname) if info.BackupType == SystemTypeUnknown && len(metadata.BackupTargets) > 0 { - info.BackupType = parseSystemTargets(metadata.BackupTargets) + if derived := parseSystemTargets(metadata.BackupTargets); derived != SystemTypeUnknown { + info.BackupType = derived + info.Source = RestoreDecisionSourceInternalMetadata + } } } categoryType := detectBackupTypeFromCategories(categories) switch {Note: the existing switch will still override with
RestoreDecisionSourceCategorieswhencategoryType != SystemTypeUnknown, preserving the archive-wins precedence; this change only fills in the gap when categories are common-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_decision.go` around lines 249 - 271, The code currently derives info.BackupType via parseSystemTargets into info.BackupType while leaving metadata.BackupType unchanged, but the switch only inspects metadata.BackupType so info.Source stays RestoreDecisionSourceUnknown; update the logic in the restore decision block to set the provenance when the type came from internal targets: after you assign info.BackupType = parseSystemTargets(metadata.BackupTargets) (or before the switch) detect if info.BackupType != SystemTypeUnknown and categoryType == SystemTypeUnknown and then set info.Source = RestoreDecisionSourceInternalMetadata (or add a switch case that checks info.BackupType instead of metadata.BackupType); ensure existing precedence where categoryType wins is preserved.internal/backup/collector_pve.go (1)
1697-1735:⚠️ Potential issue | 🟠 MajorThread
ctxthroughcopyIfExiststo enable cancellation.
createPVECoreAliases(ctx)accepts a context parameter but never uses it. The underlyingcopyIfExistsfunction hard-codescontext.Background()when callingsafeCopyFile(line 1785), making file operations non-cancellable. Since this is part of a cancellable recipe,ctxshould be threaded through the call chain.Update
copyIfExiststo accept and propagatectx:Proposed fix
-func (c *Collector) copyIfExists(source, target, description string) error { +func (c *Collector) copyIfExists(ctx context.Context, source, target, description string) error { if _, err := os.Stat(source); err != nil { return err } - return c.safeCopyFile(context.Background(), source, target, description) + return c.safeCopyFile(ctx, source, target, description) }Update call sites at line 1730 (
createPVECoreAliases) and line 1762 (createPVEReplicationAggregate) to passctx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pve.go` around lines 1697 - 1735, createPVECoreAliases accepts a context but never uses it and copyIfExists currently uses context.Background(), making file copies non-cancellable; change copyIfExists to accept a ctx parameter and pass that ctx into safeCopyFile (instead of context.Background()), then update all call sites including createPVECoreAliases (call copyIfExists(ctx, ...)) and createPVEReplicationAggregate to forward the same ctx so cancellation propagates through safeCopyFile and the copy logic.
🧹 Nitpick comments (8)
internal/backup/collector_pbs_datastore_inventory.go (1)
122-147: Redundant fstab/crypttab capture between init and mount-files bricks.
initPBSDatastoreInventoryStatealready capturesfstabandcrypttabat lines 123–124 (needed so line 133 can extractreferencedFilesfrom their content).populatePBSInventoryMountFilesthen re-captures the exact same two files at lines 143–144, duplicating I/O and hashing. Since these bricks are sequenced in the same recipe with no state change in between, the second capture is wasted work and leaves a small footgun if someone later changes one path but forgets the other (e.g., divergence betweenreferencedFilesandreport.Files["fstab"]).Consider either (a) dropping the duplicate captures from
populatePBSInventoryMountFilesand leaving onlyproc_mountsthere, or (b) moving the fstab/crypttab capture out of init and recomputingreferencedFilesin a dedicated brick after MountFiles runs.♻️ Option (a): drop redundant captures
func (c *Collector) populatePBSInventoryMountFiles(ctx context.Context, inventory *pbsInventoryState) error { if err := ctx.Err(); err != nil { return err } report := &inventory.report - report.Files["fstab"] = c.captureInventoryFile(c.systemPath("/etc/fstab"), "/etc/fstab") - report.Files["crypttab"] = c.captureInventoryFile(c.systemPath("/etc/crypttab"), "/etc/crypttab") report.Files["proc_mounts"] = c.captureInventoryFile(c.systemPath("/proc/mounts"), "/proc/mounts") return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pbs_datastore_inventory.go` around lines 122 - 147, initPBSDatastoreInventoryState captures "/etc/fstab" and "/etc/crypttab" and uses their content to compute report.referencedFiles, but populatePBSInventoryMountFiles re-captures the same files redundantly; remove the duplicate captures from populatePBSInventoryMountFiles (leave only the proc_mounts capture) so fstab/crypttab are captured once in initPBSDatastoreInventoryState and report.Files["fstab"] / report.Files["crypttab"] remain the canonical source used by extractFstabReferencedFiles and extractCrypttabKeyFiles.internal/backup/collector_system.go (1)
978-1003: Simplify the LVM gate and makevgs/lvsindependent.Two small issues with
collectSystemLVMRuntime:
- The
if … { … } else { return nil }structure is harder to read than an inverted early return, and it also couples thevgs/lvscollection topvsbeing present: on a system wherepvsis missing butvgs/lvsexist (unusual for LVM2, but possible with minimal/packaged installs), both subsequent collectors are silently skipped. Each command is already independently gated with its owndepLookPathbelow, so the pattern should be consistent withcollectSystemZFSRuntimeabove (lines 950 / 962).- No
ctx.Err()check at entry, unlike most sibling runtime bricks in this file. Since the brick wrapper may be invoked after cancellation, a fast-exit check is preferable.♻️ Proposed refactor
func (c *Collector) collectSystemLVMRuntime(ctx context.Context, commandsDir string) error { - if _, err := c.depLookPath("pvs"); err == nil { - c.safeCmdOutput(ctx, - "pvs", - filepath.Join(commandsDir, "lvm_pvs.txt"), - "LVM physical volumes", - false) - } else { - return nil - } - if _, err := c.depLookPath("vgs"); err == nil { + if err := ctx.Err(); err != nil { + return err + } + if _, err := c.depLookPath("pvs"); err == nil { + c.safeCmdOutput(ctx, + "pvs", + filepath.Join(commandsDir, "lvm_pvs.txt"), + "LVM physical volumes", + false) + } + if _, err := c.depLookPath("vgs"); err == nil { c.safeCmdOutput(ctx, "vgs", filepath.Join(commandsDir, "lvm_vgs.txt"), "LVM volume groups", false) } if _, err := c.depLookPath("lvs"); err == nil { c.safeCmdOutput(ctx, "lvs", filepath.Join(commandsDir, "lvm_lvs.txt"), "LVM logical volumes", false) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_system.go` around lines 978 - 1003, Add an early cancellation check at the top of collectSystemLVMRuntime (e.g. if ctx.Err() != nil { return ctx.Err() }) and remove the else { return nil } branch from the pvs gate so pvs, vgs and lvs are each checked/collected independently (leave the existing depLookPath checks and safeCmdOutput calls for "pvs", "vgs", and "lvs" intact).internal/environment/detect.go (3)
147-154: Deadinfo == nilbranch.
detectEnvironmentInfo()always constructs and returns a non-nil*EnvironmentInfoon every path (lines 129-144). Theif info == nilguard is unreachable and can be removed for clarity.♻️ Proposed simplification
func detectProxmox() (types.ProxmoxType, string, error) { - info, err := detectEnvironmentInfo() - if info == nil { - return types.ProxmoxUnknown, "unknown", err - } - return info.Type, info.Version, err + info, err := detectEnvironmentInfo() + return info.Type, info.Version, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/environment/detect.go` around lines 147 - 154, The nil-check in detectProxmox is unreachable because detectEnvironmentInfo always returns a non-nil *EnvironmentInfo; remove the dead branch that checks if info == nil and simplify the function to directly return info.Type, info.Version, err after calling detectEnvironmentInfo(). Update detectProxmox() (the compatibility wrapper) to eliminate the unreachable guard and return the values from info without the extra "unknown" branch.
88-97: Optional: simplifyProxmoxDualbranch by reusinginfo.Version.
detectEnvironmentInfo()already computesinfo.VersionviacombineVersions(info.PVEVersion, info.PBSVersion)(line 134), so recomputing it here duplicates that work. Also, the guarderr != nil && info.Type == types.ProxmoxUnknownis effectively equivalent toerr != nilsincedetectEnvironmentInfoonly returns a non-nil error whenType == ProxmoxUnknown.♻️ Proposed simplification
case types.ProxmoxDual: - info, err := detectEnvironmentInfo() - if err != nil && info.Type == types.ProxmoxUnknown { - return "", err - } - version := combineVersions(info.PVEVersion, info.PBSVersion) - if version == "" || version == "unknown" { - return "", fmt.Errorf("unable to determine dual Proxmox versions") - } - return version, nil + info, err := detectEnvironmentInfo() + if err != nil { + return "", err + } + if info.Version == "" || info.Version == "unknown" { + return "", fmt.Errorf("unable to determine dual Proxmox versions") + } + return info.Version, nilOne semantic note worth confirming: this branch currently returns a single-side version (e.g. just
pveVersion) if only PVE is detected even though the caller asked forProxmoxDual. If the intent ofGetVersion(ProxmoxDual)is to require both sides, add an explicitinfo.Type == types.ProxmoxDualcheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/environment/detect.go` around lines 88 - 97, Replace the manual recomputation of versions in the types.ProxmoxDual branch by using the already-populated info.Version returned from detectEnvironmentInfo(): call detectEnvironmentInfo(), return error if err != nil, then use info.Version (instead of recomputing via combineVersions) and validate it's not empty/"unknown" before returning; optionally, if GetVersion(ProxmoxDual) should require both sides be present, add an explicit check for info.Type == types.ProxmoxDual before accepting info.Version.
177-189: Minor: redundantTrimSpaceincombineVersions.Callers currently feed values already trimmed/normalized by
normalizedDetectedVersion(e.g. lines 131-134). The extraTrimSpacehere is harmless but redundant. Safe to keep as defensive coding if this helper may be used with raw inputs elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/environment/detect.go` around lines 177 - 189, The helper combineVersions currently trims inputs again even though callers (e.g. normalizedDetectedVersion) already normalize/TrimSpace them; remove the redundant strings.TrimSpace calls on pveVersion and pbsVersion inside combineVersions so it simply uses the provided strings (preserving existing return logic), and update any unit tests or call sites if they relied on the double-trim behavior; reference combineVersions and normalizedDetectedVersion to locate the change.internal/orchestrator/categories.go (1)
573-596: Optional: rename closure parameter to avoid shadowing outersystemType.The
addStorageModeIDsclosure takes a parameter namedsystemType, shadowing the outer function parameter. Functionality is fine, but renaming (e.g.,kind) would make the callers at lines 590/592/594-595 clearer and avoid confusion about which binding is in scope.♻️ Proposed rename
- addStorageModeIDs := func(systemType string) { - switch systemType { + addStorageModeIDs := func(kind string) { + switch kind { case "pve": for _, id := range []string{"pve_cluster", "storage_pve", "pve_jobs", "zfs", "filesystem", "storage_stack"} { allowedIDs[id] = true } case "pbs": for _, id := range []string{"pbs_config", "datastore_pbs", "maintenance_pbs", "pbs_jobs", "pbs_remotes", "zfs", "filesystem", "storage_stack"} { allowedIDs[id] = true } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/categories.go` around lines 573 - 596, The closure addStorageModeIDs currently declares a parameter named systemType which shadows the outer function parameter systemType; rename the closure parameter (e.g., to kind or mode) and update all references inside addStorageModeIDs (the switch on the closure parameter and the for-range loops) to use the new name so callers like addStorageModeIDs("pve"), addStorageModeIDs("pbs") and the dual-case logic remain correct and unambiguous.internal/backup/collector_pve.go (2)
783-810: Consider per-iterationctx.Err()check in the node loop.
collectPVEBackupJobHistoryonly checksctx.Err()once at entry. When iterating over many cluster nodes, a cancellation mid-loop won't be observed until the nextcaptureCommandOutputcall blocks. Same pattern incollectPVEReplicationStatus(lines 955-982).for _, node := range nodes { + if err := ctx.Err(); err != nil { + return err + } node = strings.TrimSpace(node)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pve.go` around lines 783 - 810, collectPVEBackupJobHistory currently only checks ctx.Err() once at the start, so cancellation during the node loop may not be observed; update the node iteration in collectPVEBackupJobHistory to check ctx.Err() at the start of each loop iteration (and return the context error immediately if non-nil) before calling captureCommandOutput, and apply the same per-iteration ctx check to collectPVEReplicationStatus (the second loop that calls captureCommandOutput) to ensure mid-loop cancellation is respected; reference the functions collectPVEBackupJobHistory, collectPVEReplicationStatus and the captureCommandOutput call to locate where to add the checks.
872-877: Typo in method name:pveStorageIOTimout→pveStorageIOTimeout.Missing
ein "Timeout". The method definition has 4 call sites (internal/backup/collector_bricks.golines 680, 707, 730, 753) that would need updating alongside the rename.✏️ Proposed rename
-func (c *Collector) pveStorageIOTimout() time.Duration { +func (c *Collector) pveStorageIOTimeout() time.Duration { if c.config != nil && c.config.FsIoTimeoutSeconds > 0 { return time.Duration(c.config.FsIoTimeoutSeconds) * time.Second } return 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pve.go` around lines 872 - 877, Rename the method pveStorageIOTimout to pveStorageIOTimeout to fix the typo and update all call sites that reference pveStorageIOTimout (there are four places) to use the new name; change the function signature in type Collector and adjust any imports/usages accordingly, run tests/compile to ensure no remaining references to the old name remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/backup/collector_pbs_datastore.go`:
- Around line 358-371: collectPBSDatastoreNamespaces currently writes nil into
state.namespaceResults when collectDatastoreNamespacesSnapshot returns (nil,
nil) for excluded datastores, making "skipped" indistinguishable from "empty";
change the loop in Collector.collectPBSDatastoreNamespaces so that if namespaces
== nil && err == nil you do not assign state.namespaceResults[ds.pathKey()]
(i.e., continue), otherwise assign the returned slice; alternatively, if you
prefer explicit semantics, update collectDatastoreNamespacesSnapshot to return a
sentinel value (or an explicit flag) for the excluded case and set
namespaceResults accordingly—refer to Collector.collectPBSDatastoreNamespaces,
collectDatastoreNamespacesSnapshot, shouldExclude, state.namespaceResults, and
ds.pathKey() when implementing the fix.
In `@internal/backup/collector_pve.go`:
- Around line 1069-1097: The default branch contains an unreachable else because
status is derived by trimming "status=" so status != reason is always true for
"status=*" values; fix by explicitly checking whether reason starts with
"status=" and handle the two cases: if strings.HasPrefix(reason, "status=") then
log the message that includes the parsed status (using c.logger.Warning with
storage.Name, status, storage.Path), otherwise log the generic "storage is not
available" message (the branch that currently lives in the unreachable else).
Update the default case around pveStorageUnavailableReason(storage) to use this
prefix check so both real "status=..." values and any non-"status=" reasons are
logged correctly, and keep the call to c.logPVEStorageSkipDetails(storage,
reason, nil) and return nil, nil.
---
Outside diff comments:
In `@internal/backup/collector_pve.go`:
- Around line 1697-1735: createPVECoreAliases accepts a context but never uses
it and copyIfExists currently uses context.Background(), making file copies
non-cancellable; change copyIfExists to accept a ctx parameter and pass that ctx
into safeCopyFile (instead of context.Background()), then update all call sites
including createPVECoreAliases (call copyIfExists(ctx, ...)) and
createPVEReplicationAggregate to forward the same ctx so cancellation propagates
through safeCopyFile and the copy logic.
In `@internal/orchestrator/restore_decision.go`:
- Around line 249-271: The code currently derives info.BackupType via
parseSystemTargets into info.BackupType while leaving metadata.BackupType
unchanged, but the switch only inspects metadata.BackupType so info.Source stays
RestoreDecisionSourceUnknown; update the logic in the restore decision block to
set the provenance when the type came from internal targets: after you assign
info.BackupType = parseSystemTargets(metadata.BackupTargets) (or before the
switch) detect if info.BackupType != SystemTypeUnknown and categoryType ==
SystemTypeUnknown and then set info.Source =
RestoreDecisionSourceInternalMetadata (or add a switch case that checks
info.BackupType instead of metadata.BackupType); ensure existing precedence
where categoryType wins is preserved.
---
Nitpick comments:
In `@internal/backup/collector_pbs_datastore_inventory.go`:
- Around line 122-147: initPBSDatastoreInventoryState captures "/etc/fstab" and
"/etc/crypttab" and uses their content to compute report.referencedFiles, but
populatePBSInventoryMountFiles re-captures the same files redundantly; remove
the duplicate captures from populatePBSInventoryMountFiles (leave only the
proc_mounts capture) so fstab/crypttab are captured once in
initPBSDatastoreInventoryState and report.Files["fstab"] /
report.Files["crypttab"] remain the canonical source used by
extractFstabReferencedFiles and extractCrypttabKeyFiles.
In `@internal/backup/collector_pve.go`:
- Around line 783-810: collectPVEBackupJobHistory currently only checks
ctx.Err() once at the start, so cancellation during the node loop may not be
observed; update the node iteration in collectPVEBackupJobHistory to check
ctx.Err() at the start of each loop iteration (and return the context error
immediately if non-nil) before calling captureCommandOutput, and apply the same
per-iteration ctx check to collectPVEReplicationStatus (the second loop that
calls captureCommandOutput) to ensure mid-loop cancellation is respected;
reference the functions collectPVEBackupJobHistory, collectPVEReplicationStatus
and the captureCommandOutput call to locate where to add the checks.
- Around line 872-877: Rename the method pveStorageIOTimout to
pveStorageIOTimeout to fix the typo and update all call sites that reference
pveStorageIOTimout (there are four places) to use the new name; change the
function signature in type Collector and adjust any imports/usages accordingly,
run tests/compile to ensure no remaining references to the old name remain.
In `@internal/backup/collector_system.go`:
- Around line 978-1003: Add an early cancellation check at the top of
collectSystemLVMRuntime (e.g. if ctx.Err() != nil { return ctx.Err() }) and
remove the else { return nil } branch from the pvs gate so pvs, vgs and lvs are
each checked/collected independently (leave the existing depLookPath checks and
safeCmdOutput calls for "pvs", "vgs", and "lvs" intact).
In `@internal/environment/detect.go`:
- Around line 147-154: The nil-check in detectProxmox is unreachable because
detectEnvironmentInfo always returns a non-nil *EnvironmentInfo; remove the dead
branch that checks if info == nil and simplify the function to directly return
info.Type, info.Version, err after calling detectEnvironmentInfo(). Update
detectProxmox() (the compatibility wrapper) to eliminate the unreachable guard
and return the values from info without the extra "unknown" branch.
- Around line 88-97: Replace the manual recomputation of versions in the
types.ProxmoxDual branch by using the already-populated info.Version returned
from detectEnvironmentInfo(): call detectEnvironmentInfo(), return error if err
!= nil, then use info.Version (instead of recomputing via combineVersions) and
validate it's not empty/"unknown" before returning; optionally, if
GetVersion(ProxmoxDual) should require both sides be present, add an explicit
check for info.Type == types.ProxmoxDual before accepting info.Version.
- Around line 177-189: The helper combineVersions currently trims inputs again
even though callers (e.g. normalizedDetectedVersion) already normalize/TrimSpace
them; remove the redundant strings.TrimSpace calls on pveVersion and pbsVersion
inside combineVersions so it simply uses the provided strings (preserving
existing return logic), and update any unit tests or call sites if they relied
on the double-trim behavior; reference combineVersions and
normalizedDetectedVersion to locate the change.
In `@internal/orchestrator/categories.go`:
- Around line 573-596: The closure addStorageModeIDs currently declares a
parameter named systemType which shadows the outer function parameter
systemType; rename the closure parameter (e.g., to kind or mode) and update all
references inside addStorageModeIDs (the switch on the closure parameter and the
for-range loops) to use the new name so callers like addStorageModeIDs("pve"),
addStorageModeIDs("pbs") and the dual-case logic remain correct and unambiguous.
🪄 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: bdf07722-bf88-4c3c-83c1-459fb3e0c407
📒 Files selected for processing (22)
internal/backup/checksum.gointernal/backup/collector_pbs_datastore.gointernal/backup/collector_pbs_datastore_inventory.gointernal/backup/collector_pve.gointernal/backup/collector_pve_util_test.gointernal/backup/collector_storage_stack_common.gointernal/backup/collector_system.gointernal/config/config.gointernal/config/config_test.gointernal/environment/detect.gointernal/environment/detect_deterministic_test.gointernal/environment/detect_test.gointernal/notify/email.gointernal/orchestrator/categories.gointernal/orchestrator/directory_recreation.gointernal/orchestrator/directory_recreation_test.gointernal/orchestrator/helpers_test.gointernal/orchestrator/network_apply.gointernal/orchestrator/restore_decision.gointernal/orchestrator/restore_decision_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_workflow_ui.go
✅ Files skipped from review due to trivial changes (1)
- internal/notify/email.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/environment/detect_test.go
- internal/config/config_test.go
- internal/backup/checksum.go
- internal/orchestrator/restore_workflow_ui.go
- internal/config/config.go
- internal/backup/collector_pve_util_test.go
- internal/orchestrator/directory_recreation.go
- internal/backup/collector_storage_stack_common.go
Multiple fixes and refactors across backup, environment detection, orchestrator and tests: - Rename pveStorageIOTimout -> pveStorageIOTimeout and update callers. - Add context cancellation checks in long-running loops (PVE backup history, replication status, LVM runtime collection). - Thread context through copyIfExists and use it with safeCopyFile; update tests accordingly. - Skip nil namespace results when collecting PBS datastore namespaces. - Remove capturing /etc/fstab and /etc/crypttab from PBS inventory mount files. - Improve PVE storage status handling to check reason prefix and adjust logging. - Adjust environment detection API usage: consolidate to Detect()/detectEnvironmentInfo, update GetVersion and wrapper behavior, simplify combineVersions handling, and update related tests to use the new entrypoints. - Tweak category helper param name and restore decision logic to prefer internal metadata source when appropriate. - Add unit tests for restore decision using internal targets and Proxmox type capabilities; expand Proxmox type string test to include dual. These changes fix typos, add robustness around context cancellation, tighten detection behavior for hybrid systems, and expand test coverage.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/backup/collector_pbs_datastore.go (1)
700-752:⚠️ Potential issue | 🟠 MajorRecurse into VM/CT snapshot directories and match
.pxar.*files.Line 709 only scans the immediate
vm/ctdirectory, then Line 731 skips its child directories, so normal nested PBS snapshots are reported as “0 .pxar files”. Line 734 also misses files matched by the metadata step, such as*.pxar.*.🐛 Proposed fix
func (c *Collector) writePxarListReport(ctx context.Context, target string, ds pbsDatastore, subDir string, ioTimeout time.Duration) error { c.logger.Debug("Writing PXAR file list for datastore %s subdir %s", ds.Name, subDir) basePath := filepath.Join(ds.Path, subDir) var builder strings.Builder builder.WriteString(fmt.Sprintf("# List of .pxar files in %s generated on %s\n", basePath, time.Now().Format(time.RFC1123))) builder.WriteString(fmt.Sprintf("# Datastore: %s, Subdirectory: %s\n", ds.Name, subDir)) builder.WriteString("# Format: permissions size date name\n") - entries, err := safefs.ReadDir(ctx, basePath, ioTimeout) - if err != nil { - if errors.Is(err, safefs.ErrTimeout) { - return err - } - builder.WriteString(fmt.Sprintf("# Unable to read directory: %v\n", err)) - if writeErr := c.writeReportFile(target, []byte(builder.String())); writeErr != nil { - return writeErr - } - c.logger.Info("PXAR: datastore %s/%s -> path %s not accessible (%v)", ds.Name, subDir, basePath, err) - return nil - } - type infoEntry struct { mode os.FileMode size int64 time time.Time name string } var files []infoEntry - for _, entry := range entries { - if entry.IsDir() { - continue - } - if !strings.HasSuffix(entry.Name(), ".pxar") { - continue - } + matchesPXAR := func(name string) bool { + for _, pattern := range []string{"*.pxar", "*.pxar.*", "catalog.pxar", "catalog.pxar.*"} { + if matchPattern(name, pattern) { + return true + } + } + return false + } + + type dirItem struct { + path string + } + stack := []dirItem{{path: basePath}} + for len(stack) > 0 { + if err := ctx.Err(); err != nil { + return err + } + item := stack[len(stack)-1] + stack = stack[:len(stack)-1] + + entries, err := safefs.ReadDir(ctx, item.path, ioTimeout) + if err != nil { + if errors.Is(err, safefs.ErrTimeout) { + return err + } + if item.path == basePath { + builder.WriteString(fmt.Sprintf("# Unable to read directory: %v\n", err)) + if writeErr := c.writeReportFile(target, []byte(builder.String())); writeErr != nil { + return writeErr + } + c.logger.Info("PXAR: datastore %s/%s -> path %s not accessible (%v)", ds.Name, subDir, basePath, err) + return nil + } + c.logger.Debug("PXAR: skipping unreadable directory %s: %v", item.path, err) + continue + } - fullPath := filepath.Join(basePath, entry.Name()) - info, err := safefs.Stat(ctx, fullPath, ioTimeout) - if err != nil { - if errors.Is(err, safefs.ErrTimeout) { - return err + for _, entry := range entries { + fullPath := filepath.Join(item.path, entry.Name()) + if entry.IsDir() { + stack = append(stack, dirItem{path: fullPath}) + continue + } + if !matchesPXAR(entry.Name()) { + continue } - continue + + info, err := safefs.Stat(ctx, fullPath, ioTimeout) + if err != nil { + if errors.Is(err, safefs.ErrTimeout) { + return err + } + continue + } + name, relErr := filepath.Rel(basePath, fullPath) + if relErr != nil { + name = fullPath + } + files = append(files, infoEntry{ + mode: info.Mode(), + size: info.Size(), + time: info.ModTime(), + name: filepath.ToSlash(name), + }) } - files = append(files, infoEntry{ - mode: info.Mode(), - size: info.Size(), - time: info.ModTime(), - name: entry.Name(), - }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pbs_datastore.go` around lines 700 - 752, The PXAR report currently only lists .pxar files in the immediate directory (writePxarListReport), skipping child dirs and files like `*.pxar.*`; update the file-collection logic in writePxarListReport to recursively descend into snapshot subdirectories (e.g., vm/ct child dirs) instead of ignoring entries where entry.IsDir(), and treat filenames matching both `*.pxar` and `*.pxar.*` (e.g., with a regex like `\.pxar($|\.)`) as matches; when recursing, continue to use safefs.ReadDir and safefs.Stat with the same ctx and ioTimeout and preserve the existing timeout/error handling (including checking for safefs.ErrTimeout) so the function still returns on timeouts and logs/returns other errors as before.
🧹 Nitpick comments (1)
internal/backup/collector_pbs_datastore_inventory.go (1)
261-360: Consider collapsing the per-tool host-command helpers.Each
populatePBSInventoryHostCommands*helper repeats the same preamble:ctx.Err()check,hostCommandsEnabledguard, and a handful ofcaptureInventoryCommandcalls. This is ~100 lines of near-identical scaffolding that the brick framework splits for ordering/testability, but the per-tool split could equally be expressed as a table of{brickID, []commandSpec}driving a single helper, reducing surface area and keeping future command additions to one place. Non-blocking — leave as is if the granularity is intentional for selective-brick tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backup/collector_pbs_datastore_inventory.go` around lines 261 - 360, The helper methods (populatePBSInventoryHostCommandsCore, populatePBSInventoryHostCommandsDMSetup, populatePBSInventoryHostCommandsLVM, populatePBSInventoryHostCommandsMDADM, populatePBSInventoryHostCommandsMultipath, populatePBSInventoryHostCommandsISCSI, populatePBSInventoryHostCommandsZFS) all duplicate the same ctx.Err() check, hostCommandsEnabled guard, and repeated calls to c.captureInventoryCommand; collapse them into a single table-driven helper: keep the initial ctx.Err() and the inventory.hostCommandsEnabled guard in the new function, define a slice of structs (e.g. {Key string, CmdDesc string, CmdArgs []string}) for each command currently added to inventory.report.Commands, then loop over the slice and set report.Commands[Key] = c.captureInventoryCommand(ctx, CmdDesc, CmdArgs...), preserving the exact keys (like "uname", "pvs_json", "zpool_status", etc.) so tests and bricks keep the same outputs; leave the per-tool functions only if you need to preserve brick-level granularity—otherwise replace them with calls into the new table-driven function and remove the duplicated scaffolding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/backup/collector_bricks.go`:
- Around line 830-857: The PVE aggregate finalizer bricks
(brickPVEAggregateBackupHistory and brickPVEAggregateReplicationStatus) run
unconditionally and create empty artifacts even when features are disabled;
update each brick's Run to first check the corresponding feature flag
(BACKUP_PVE_JOBS for createPVEBackupHistoryAggregate and BACKUP_PVE_REPLICATION
for createPVEReplicationAggregate) and return nil if the feature is disabled,
preserving the existing state.pve.finalizeCollectionAborted guard and error
handling (i.e., only call createPVEBackupHistoryAggregate or
createPVEReplicationAggregate when the matching feature flag is enabled).
In `@internal/backup/collector_pbs_datastore_inventory.go`:
- Around line 245-259: In populatePBSInventoryReferencedFiles, stop overwriting
Snapshot.Reason for non-skipped captures returned by captureInventoryFile;
instead record the fact a file was "referenced by fstab/crypttab" in a separate
place (for example add a Snapshot.Note or Snapshot.Source field or add a
report.References map keyed by referencedFileKey) and populate that new
field/map when snap.Skipped==false so Reason remains reserved for skip/error
semantics; update references to report.Files and any consumers to read the new
field/map rather than inferring state from Reason.
In `@internal/environment/detect_additional_test.go`:
- Around line 470-496: TestDetectEnvironmentInfo currently lets Detect() mutate
process globals; save and restore os.Getenv("PATH") around the test and set
debugBaseDir to t.TempDir() (and restore after) so any files are written into
the temp dir, and ensure extendPath() is called with the original PATH value (or
restore PATH after calling Detect()) to avoid permanently changing PATH; in
short, in TestDetectEnvironmentInfo save originalPATH := os.Getenv("PATH") and
origDebug := debugBaseDir, set debugBaseDir = t.TempDir(), run Detect(), then
restore os.Setenv("PATH", originalPATH) and debugBaseDir = origDebug to fully
isolate side effects from Detect(), extendPath(), and debugBaseDir.
In `@internal/environment/detect.go`:
- Around line 147-150: The function detectProxmox is flagged as unused; either
delete this unused wrapper or explicitly suppress the lint if you intend to keep
it for transitional in-package tests: remove the detectProxmox function entirely
(and any callers) to eliminate the warning, or add a suppression comment such as
"//nolint:unused" directly above the detectProxmox declaration to silence
golangci-lint, keeping references to detectEnvironmentInfo and the wrapper name
detectProxmox so reviewers can find it.
- Around line 88-96: The dual-Proxmox branch currently accepts a single
non-"unknown" version and can return incomplete dual-version data; when handling
types.ProxmoxDual in GetVersion/detect.go (within the case for types.ProxmoxDual
after calling detectEnvironmentInfo), ensure both product versions are present
and valid: split info.Version into its two components (PVE and PBS), verify
there are exactly two parts and neither part is empty or equals "unknown", and
only then return the combined version string; otherwise return an error
indicating unable to determine dual Proxmox versions.
In `@internal/orchestrator/compatibility_test.go`:
- Around line 280-300: The test cases for parseSystemTargets expect recognition
of synonyms and explicit "dual" but the current parseSystemTargets
implementation only matches exact "PVE"/"PBS"; update parseSystemTargets in
internal/orchestrator/compatibility.go to normalize input (trim, lower-case) and
accept synonyms like "proxmox backup server" as PBS, accept "dual" (or mixed
presence of both tokens) to return SystemTypeDual, and return SystemTypeUnknown
for unrecognized tokens; ensure the function returns SystemTypePVE when only
PVE-like tokens are present and SystemTypePBS when only PBS-like tokens are
present so tests (including values ["proxmox backup server"] and ["dual"]) pass.
In `@internal/types/common_test.go`:
- Around line 50-58: Test currently treats nil and empty slice as equivalent for
Targets(), breaking the contract that ProxmoxUnknown should return nil; assign
gotTargets := tt.ptype.Targets() (move this line before checks), then if
tt.targets == nil assert gotTargets == nil explicitly (or use
reflect.DeepEqual(gotTargets, tt.targets)) instead of comparing lengths,
otherwise continue with length and element comparisons; reference the Targets()
method, the test variable gotTargets, and the ProxmoxUnknown case when adding
the nil-check.
---
Outside diff comments:
In `@internal/backup/collector_pbs_datastore.go`:
- Around line 700-752: The PXAR report currently only lists .pxar files in the
immediate directory (writePxarListReport), skipping child dirs and files like
`*.pxar.*`; update the file-collection logic in writePxarListReport to
recursively descend into snapshot subdirectories (e.g., vm/ct child dirs)
instead of ignoring entries where entry.IsDir(), and treat filenames matching
both `*.pxar` and `*.pxar.*` (e.g., with a regex like `\.pxar($|\.)`) as
matches; when recursing, continue to use safefs.ReadDir and safefs.Stat with the
same ctx and ioTimeout and preserve the existing timeout/error handling
(including checking for safefs.ErrTimeout) so the function still returns on
timeouts and logs/returns other errors as before.
---
Nitpick comments:
In `@internal/backup/collector_pbs_datastore_inventory.go`:
- Around line 261-360: The helper methods (populatePBSInventoryHostCommandsCore,
populatePBSInventoryHostCommandsDMSetup, populatePBSInventoryHostCommandsLVM,
populatePBSInventoryHostCommandsMDADM,
populatePBSInventoryHostCommandsMultipath,
populatePBSInventoryHostCommandsISCSI, populatePBSInventoryHostCommandsZFS) all
duplicate the same ctx.Err() check, hostCommandsEnabled guard, and repeated
calls to c.captureInventoryCommand; collapse them into a single table-driven
helper: keep the initial ctx.Err() and the inventory.hostCommandsEnabled guard
in the new function, define a slice of structs (e.g. {Key string, CmdDesc
string, CmdArgs []string}) for each command currently added to
inventory.report.Commands, then loop over the slice and set report.Commands[Key]
= c.captureInventoryCommand(ctx, CmdDesc, CmdArgs...), preserving the exact keys
(like "uname", "pvs_json", "zpool_status", etc.) so tests and bricks keep the
same outputs; leave the per-tool functions only if you need to preserve
brick-level granularity—otherwise replace them with calls into the new
table-driven function and remove the duplicated scaffolding.
🪄 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: f0da5745-2e3f-4288-9839-3a4397ff98db
📒 Files selected for processing (16)
internal/backup/collector_bricks.gointernal/backup/collector_paths_test.gointernal/backup/collector_pbs_datastore.gointernal/backup/collector_pbs_datastore_inventory.gointernal/backup/collector_pve.gointernal/backup/collector_pve_util_test.gointernal/backup/collector_system.gointernal/environment/detect.gointernal/environment/detect_additional_test.gointernal/environment/detect_deterministic_test.gointernal/environment/detect_hybrid_bug_test.gointernal/orchestrator/categories.gointernal/orchestrator/compatibility_test.gointernal/orchestrator/restore_decision.gointernal/orchestrator/restore_decision_test.gointernal/types/common_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/environment/detect_deterministic_test.go
- internal/environment/detect_hybrid_bug_test.go
- internal/orchestrator/restore_decision_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/orchestrator/categories.go
- internal/backup/collector_pve_util_test.go
- internal/orchestrator/restore_decision.go
Comment on lines
+830
to
+857
| ID: brickPVEAggregateBackupHistory, | ||
| Description: "Aggregate backup history aliases", | ||
| Run: func(ctx context.Context, state *collectionState) error { | ||
| c := state.collector | ||
| if state.pve.finalizeCollectionAborted { | ||
| return nil | ||
| } | ||
| if err := c.createPVEBackupHistoryAggregate(ctx); err != nil { | ||
| c.logger.Warning("Failed to aggregate PVE backup history: %v", err) | ||
| state.pve.finalizeCollectionAborted = true | ||
| } | ||
| return nil | ||
| }, | ||
| }, | ||
| { | ||
| ID: brickPVEAggregateReplicationStatus, | ||
| Description: "Aggregate replication status aliases", | ||
| Run: func(ctx context.Context, state *collectionState) error { | ||
| c := state.collector | ||
| if state.pve.finalizeCollectionAborted { | ||
| return nil | ||
| } | ||
| if err := c.createPVEReplicationAggregate(ctx); err != nil { | ||
| c.logger.Warning("Failed to aggregate PVE replication status: %v", err) | ||
| state.pve.finalizeCollectionAborted = true | ||
| } | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
Gate PVE aggregate finalizers with the matching feature flags.
When BACKUP_PVE_JOBS=false or BACKUP_PVE_REPLICATION=false, these bricks still create empty aggregate artifacts under the PVE info tree. Keep finalization consistent with the collection gates.
🛠️ Proposed fix
{
ID: brickPVEAggregateBackupHistory,
Description: "Aggregate backup history aliases",
Run: func(ctx context.Context, state *collectionState) error {
c := state.collector
- if state.pve.finalizeCollectionAborted {
+ if !c.config.BackupPVEJobs || state.pve.finalizeCollectionAborted {
return nil
}
if err := c.createPVEBackupHistoryAggregate(ctx); err != nil {
c.logger.Warning("Failed to aggregate PVE backup history: %v", err)
state.pve.finalizeCollectionAborted = true
@@
{
ID: brickPVEAggregateReplicationStatus,
Description: "Aggregate replication status aliases",
Run: func(ctx context.Context, state *collectionState) error {
c := state.collector
- if state.pve.finalizeCollectionAborted {
+ if !c.config.BackupPVEReplication || state.pve.finalizeCollectionAborted {
return nil
}
if err := c.createPVEReplicationAggregate(ctx); err != nil {
c.logger.Warning("Failed to aggregate PVE replication status: %v", err)
state.pve.finalizeCollectionAborted = 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.
Suggested change
| ID: brickPVEAggregateBackupHistory, | |
| Description: "Aggregate backup history aliases", | |
| Run: func(ctx context.Context, state *collectionState) error { | |
| c := state.collector | |
| if state.pve.finalizeCollectionAborted { | |
| return nil | |
| } | |
| if err := c.createPVEBackupHistoryAggregate(ctx); err != nil { | |
| c.logger.Warning("Failed to aggregate PVE backup history: %v", err) | |
| state.pve.finalizeCollectionAborted = true | |
| } | |
| return nil | |
| }, | |
| }, | |
| { | |
| ID: brickPVEAggregateReplicationStatus, | |
| Description: "Aggregate replication status aliases", | |
| Run: func(ctx context.Context, state *collectionState) error { | |
| c := state.collector | |
| if state.pve.finalizeCollectionAborted { | |
| return nil | |
| } | |
| if err := c.createPVEReplicationAggregate(ctx); err != nil { | |
| c.logger.Warning("Failed to aggregate PVE replication status: %v", err) | |
| state.pve.finalizeCollectionAborted = true | |
| } | |
| return nil | |
| }, | |
| { | |
| ID: brickPVEAggregateBackupHistory, | |
| Description: "Aggregate backup history aliases", | |
| Run: func(ctx context.Context, state *collectionState) error { | |
| c := state.collector | |
| if !c.config.BackupPVEJobs || state.pve.finalizeCollectionAborted { | |
| return nil | |
| } | |
| if err := c.createPVEBackupHistoryAggregate(ctx); err != nil { | |
| c.logger.Warning("Failed to aggregate PVE backup history: %v", err) | |
| state.pve.finalizeCollectionAborted = true | |
| } | |
| return nil | |
| }, | |
| }, | |
| { | |
| ID: brickPVEAggregateReplicationStatus, | |
| Description: "Aggregate replication status aliases", | |
| Run: func(ctx context.Context, state *collectionState) error { | |
| c := state.collector | |
| if !c.config.BackupPVEReplication || state.pve.finalizeCollectionAborted { | |
| return nil | |
| } | |
| if err := c.createPVEReplicationAggregate(ctx); err != nil { | |
| c.logger.Warning("Failed to aggregate PVE replication status: %v", err) | |
| state.pve.finalizeCollectionAborted = true | |
| } | |
| return nil | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/backup/collector_bricks.go` around lines 830 - 857, The PVE
aggregate finalizer bricks (brickPVEAggregateBackupHistory and
brickPVEAggregateReplicationStatus) run unconditionally and create empty
artifacts even when features are disabled; update each brick's Run to first
check the corresponding feature flag (BACKUP_PVE_JOBS for
createPVEBackupHistoryAggregate and BACKUP_PVE_REPLICATION for
createPVEReplicationAggregate) and return nil if the feature is disabled,
preserving the existing state.pve.finalizeCollectionAborted guard and error
handling (i.e., only call createPVEBackupHistoryAggregate or
createPVEReplicationAggregate when the matching feature flag is enabled).
Comment on lines
+245
to
+259
| func (c *Collector) populatePBSInventoryReferencedFiles(ctx context.Context, inventory *pbsInventoryState) error { | ||
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Best-effort: capture and copy referenced key/credential files from crypttab/fstab. | ||
| for _, ref := range uniqueSortedStrings(append( | ||
| extractCrypttabKeyFiles(report.Files["crypttab"].Content), | ||
| extractFstabReferencedFiles(report.Files["fstab"].Content)..., | ||
| )) { | ||
| ref := ref | ||
| report := &inventory.report | ||
| for _, ref := range inventory.referencedFiles { | ||
| key := referencedFileKey(ref) | ||
| snap := c.captureInventoryFile(c.systemPath(ref), ref) | ||
| if !snap.Skipped && snap.Reason == "" { | ||
| snap.Reason = "referenced by fstab/crypttab" | ||
| } | ||
| report.Files[key] = snap | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Minor: Reason is being set on non-skipped snapshots.
In captureInventoryFile, Reason is only populated when Skipped is true (or as an error annotation). Here, for every referenced file that wasn't skipped, you unconditionally set Reason = "referenced by fstab/crypttab", including for files that don't exist (Exists=false, Skipped=false, Reason==""). Any downstream consumer that uses a non-empty Reason as a signal for "skipped/anomalous" will now see these as skipped. Consider moving this annotation to a distinct field (e.g., a Source/Note on the snapshot, or a sibling map on the report) to avoid conflating intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/backup/collector_pbs_datastore_inventory.go` around lines 245 - 259,
In populatePBSInventoryReferencedFiles, stop overwriting Snapshot.Reason for
non-skipped captures returned by captureInventoryFile; instead record the fact a
file was "referenced by fstab/crypttab" in a separate place (for example add a
Snapshot.Note or Snapshot.Source field or add a report.References map keyed by
referencedFileKey) and populate that new field/map when snap.Skipped==false so
Reason remains reserved for skip/error semantics; update references to
report.Files and any consumers to read the new field/map rather than inferring
state from Reason.
Comment on lines
+470
to
496
| func TestDetectEnvironmentInfo(t *testing.T) { | ||
| info, err := Detect() | ||
|
|
||
| if info == nil { | ||
| t.Fatal("Detect() returned nil info") | ||
| } | ||
|
|
||
| // Should always return a valid type | ||
| if pType == "" { | ||
| t.Error("detectProxmox() should return a non-empty type") | ||
| if info.Type == "" { | ||
| t.Error("Detect() should return a non-empty type") | ||
| } | ||
|
|
||
| // On unknown systems, should return error | ||
| if pType == types.ProxmoxUnknown { | ||
| if info.Type == types.ProxmoxUnknown { | ||
| if err == nil { | ||
| t.Error("detectProxmox() should return error for unknown systems") | ||
| t.Error("Detect() should return error for unknown systems") | ||
| } | ||
| if version != "unknown" { | ||
| t.Errorf("Version should be 'unknown' for unknown systems, got %q", version) | ||
| if info.Version != "unknown" { | ||
| t.Errorf("Version should be 'unknown' for unknown systems, got %q", info.Version) | ||
| } | ||
| } | ||
|
|
||
| // On detected systems, version should not be empty | ||
| if pType != types.ProxmoxUnknown && version == "" { | ||
| if info.Type != types.ProxmoxUnknown && info.Version == "" { | ||
| t.Error("Version should not be empty for detected Proxmox systems") | ||
| } | ||
| } |
There was a problem hiding this comment.
Isolate Detect() side effects in this test.
Detect() mutates PATH via extendPath() and writes a debug file under the package-level debugBaseDir on unknown systems. Please restore those globals and redirect debug output to t.TempDir() so this test does not pollute the process environment or /tmp.
🧪 Proposed test isolation fix
func TestDetectEnvironmentInfo(t *testing.T) {
+ t.Setenv("PATH", os.Getenv("PATH"))
+
+ origDebugBaseDir := debugBaseDir
+ debugBaseDir = t.TempDir()
+ t.Cleanup(func() {
+ debugBaseDir = origDebugBaseDir
+ })
+
info, err := Detect()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestDetectEnvironmentInfo(t *testing.T) { | |
| info, err := Detect() | |
| if info == nil { | |
| t.Fatal("Detect() returned nil info") | |
| } | |
| // Should always return a valid type | |
| if pType == "" { | |
| t.Error("detectProxmox() should return a non-empty type") | |
| if info.Type == "" { | |
| t.Error("Detect() should return a non-empty type") | |
| } | |
| // On unknown systems, should return error | |
| if pType == types.ProxmoxUnknown { | |
| if info.Type == types.ProxmoxUnknown { | |
| if err == nil { | |
| t.Error("detectProxmox() should return error for unknown systems") | |
| t.Error("Detect() should return error for unknown systems") | |
| } | |
| if version != "unknown" { | |
| t.Errorf("Version should be 'unknown' for unknown systems, got %q", version) | |
| if info.Version != "unknown" { | |
| t.Errorf("Version should be 'unknown' for unknown systems, got %q", info.Version) | |
| } | |
| } | |
| // On detected systems, version should not be empty | |
| if pType != types.ProxmoxUnknown && version == "" { | |
| if info.Type != types.ProxmoxUnknown && info.Version == "" { | |
| t.Error("Version should not be empty for detected Proxmox systems") | |
| } | |
| } | |
| func TestDetectEnvironmentInfo(t *testing.T) { | |
| t.Setenv("PATH", os.Getenv("PATH")) | |
| origDebugBaseDir := debugBaseDir | |
| debugBaseDir = t.TempDir() | |
| t.Cleanup(func() { | |
| debugBaseDir = origDebugBaseDir | |
| }) | |
| info, err := Detect() | |
| if info == nil { | |
| t.Fatal("Detect() returned nil info") | |
| } | |
| // Should always return a valid type | |
| if info.Type == "" { | |
| t.Error("Detect() should return a non-empty type") | |
| } | |
| // On unknown systems, should return error | |
| if info.Type == types.ProxmoxUnknown { | |
| if err == nil { | |
| t.Error("Detect() should return error for unknown systems") | |
| } | |
| if info.Version != "unknown" { | |
| t.Errorf("Version should be 'unknown' for unknown systems, got %q", info.Version) | |
| } | |
| } | |
| // On detected systems, version should not be empty | |
| if info.Type != types.ProxmoxUnknown && info.Version == "" { | |
| t.Error("Version should not be empty for detected Proxmox systems") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/environment/detect_additional_test.go` around lines 470 - 496,
TestDetectEnvironmentInfo currently lets Detect() mutate process globals; save
and restore os.Getenv("PATH") around the test and set debugBaseDir to
t.TempDir() (and restore after) so any files are written into the temp dir, and
ensure extendPath() is called with the original PATH value (or restore PATH
after calling Detect()) to avoid permanently changing PATH; in short, in
TestDetectEnvironmentInfo save originalPATH := os.Getenv("PATH") and origDebug
:= debugBaseDir, set debugBaseDir = t.TempDir(), run Detect(), then restore
os.Setenv("PATH", originalPATH) and debugBaseDir = origDebug to fully isolate
side effects from Detect(), extendPath(), and debugBaseDir.
Comment on lines
+88
to
+96
| case types.ProxmoxDual: | ||
| info, err := detectEnvironmentInfo() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if info.Type != types.ProxmoxDual || info.Version == "" || info.Version == "unknown" { | ||
| return "", fmt.Errorf("unable to determine dual Proxmox versions") | ||
| } | ||
| return info.Version, nil |
There was a problem hiding this comment.
Require both product versions for dual GetVersion().
If PVE has a version but PBS is only detected as "unknown" through directories/sources, info.Type is dual and info.Version collapses to the single PVE version, so this branch returns success with incomplete dual version data.
🐛 Proposed dual-version validation fix
case types.ProxmoxDual:
info, err := detectEnvironmentInfo()
if err != nil {
return "", err
}
- if info.Type != types.ProxmoxDual || info.Version == "" || info.Version == "unknown" {
+ if info.Type != types.ProxmoxDual || info.PVEVersion == "" || info.PBSVersion == "" {
return "", fmt.Errorf("unable to determine dual Proxmox versions")
}
return info.Version, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/environment/detect.go` around lines 88 - 96, The dual-Proxmox branch
currently accepts a single non-"unknown" version and can return incomplete
dual-version data; when handling types.ProxmoxDual in GetVersion/detect.go
(within the case for types.ProxmoxDual after calling detectEnvironmentInfo),
ensure both product versions are present and valid: split info.Version into its
two components (PVE and PBS), verify there are exactly two parts and neither
part is empty or equals "unknown", and only then return the combined version
string; otherwise return an error indicating unable to determine dual Proxmox
versions.
Comment on lines
+147
to
+150
| func detectProxmox() (types.ProxmoxType, string, error) { | ||
| info, err := detectEnvironmentInfo() | ||
| return info.Type, info.Version, err | ||
| } |
There was a problem hiding this comment.
Resolve the unused compatibility wrapper before lint runs.
golangci-lint reports detectProxmox() as unused. Since it is unexported, either remove it or explicitly suppress the lint if it is intentionally retained for transitional in-package tests.
🧹 Possible suppression if intentionally retained
// detectProxmox is retained as a compatibility wrapper for legacy call sites and tests.
+//nolint:unused // retained temporarily for legacy in-package callers during the detection refactor.
func detectProxmox() (types.ProxmoxType, string, error) {
info, err := detectEnvironmentInfo()
return info.Type, info.Version, err
}🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 147-147: func detectProxmox is unused
(unused)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/environment/detect.go` around lines 147 - 150, The function
detectProxmox is flagged as unused; either delete this unused wrapper or
explicitly suppress the lint if you intend to keep it for transitional
in-package tests: remove the detectProxmox function entirely (and any callers)
to eliminate the warning, or add a suppression comment such as "//nolint:unused"
directly above the detectProxmox declaration to silence golangci-lint, keeping
references to detectEnvironmentInfo and the wrapper name detectProxmox so
reviewers can find it.
Comment on lines
+280
to
+300
| func TestParseSystemTargets(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| values []string | ||
| want SystemType | ||
| }{ | ||
| {name: "pve only", values: []string{"pve"}, want: SystemTypePVE}, | ||
| {name: "pbs only", values: []string{"pbs"}, want: SystemTypePBS}, | ||
| {name: "mixed targets", values: []string{"pve", "proxmox backup server"}, want: SystemTypeDual}, | ||
| {name: "explicit dual", values: []string{"dual"}, want: SystemTypeDual}, | ||
| {name: "unknown", values: []string{"plain-linux"}, want: SystemTypeUnknown}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := parseSystemTargets(tt.values); got != tt.want { | ||
| t.Fatalf("parseSystemTargets(%v) = %v; want %v", tt.values, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update parseSystemTargets() before relying on these cases.
The supplied internal/orchestrator/compatibility.go:120-135 implementation only recognizes exact PVE/PBS tokens, so the new "proxmox backup server" and "dual" cases will currently return SystemTypePVE/SystemTypeUnknown instead of SystemTypeDual.
🐛 Proposed implementation fix in `internal/orchestrator/compatibility.go`
func parseSystemTargets(values []string) SystemType {
- // Normalize to uppercase for comparison
- targets := make(map[string]bool)
+ hasPVE := false
+ hasPBS := false
+
for _, v := range values {
- targets[strings.ToUpper(strings.TrimSpace(v))] = true
+ switch parseSystemTypeString(v) {
+ case SystemTypeDual:
+ hasPVE = true
+ hasPBS = true
+ case SystemTypePVE:
+ hasPVE = true
+ case SystemTypePBS:
+ hasPBS = true
+ }
}
- // Determine system type based on targets
- hasPVE := targets["PVE"]
- hasPBS := targets["PBS"]
-
switch {
case hasPVE && hasPBS:
return SystemTypeDual🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/orchestrator/compatibility_test.go` around lines 280 - 300, The test
cases for parseSystemTargets expect recognition of synonyms and explicit "dual"
but the current parseSystemTargets implementation only matches exact
"PVE"/"PBS"; update parseSystemTargets in internal/orchestrator/compatibility.go
to normalize input (trim, lower-case) and accept synonyms like "proxmox backup
server" as PBS, accept "dual" (or mixed presence of both tokens) to return
SystemTypeDual, and return SystemTypeUnknown for unrecognized tokens; ensure the
function returns SystemTypePVE when only PVE-like tokens are present and
SystemTypePBS when only PBS-like tokens are present so tests (including values
["proxmox backup server"] and ["dual"]) pass.
Comment on lines
+50
to
+58
| gotTargets := tt.ptype.Targets() | ||
| if len(gotTargets) != len(tt.targets) { | ||
| t.Fatalf("Targets() len = %d, want %d (%v)", len(gotTargets), len(tt.targets), gotTargets) | ||
| } | ||
| for i := range gotTargets { | ||
| if gotTargets[i] != tt.targets[i] { | ||
| t.Fatalf("Targets()[%d] = %q, want %q", i, gotTargets[i], tt.targets[i]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve the nil target contract in this assertion.
Line 38 expects nil for ProxmoxUnknown, but the current comparison treats nil and []string{} as equivalent. Use reflect.DeepEqual or an explicit nil check if Targets() should return nil for unknown types.
🧪 Proposed test tightening
+ if (gotTargets == nil) != (tt.targets == nil) {
+ t.Fatalf("Targets() nil = %v, want %v (%v)", gotTargets == nil, tt.targets == nil, gotTargets)
+ }
gotTargets := tt.ptype.Targets()
if len(gotTargets) != len(tt.targets) {
t.Fatalf("Targets() len = %d, want %d (%v)", len(gotTargets), len(tt.targets), gotTargets)
}Note: move gotTargets := tt.ptype.Targets() before the new nil check:
- gotTargets := tt.ptype.Targets()
+ gotTargets := tt.ptype.Targets()
+ if (gotTargets == nil) != (tt.targets == nil) {
+ t.Fatalf("Targets() nil = %v, want %v (%v)", gotTargets == nil, tt.targets == nil, gotTargets)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| gotTargets := tt.ptype.Targets() | |
| if len(gotTargets) != len(tt.targets) { | |
| t.Fatalf("Targets() len = %d, want %d (%v)", len(gotTargets), len(tt.targets), gotTargets) | |
| } | |
| for i := range gotTargets { | |
| if gotTargets[i] != tt.targets[i] { | |
| t.Fatalf("Targets()[%d] = %q, want %q", i, gotTargets[i], tt.targets[i]) | |
| } | |
| } | |
| gotTargets := tt.ptype.Targets() | |
| if (gotTargets == nil) != (tt.targets == nil) { | |
| t.Fatalf("Targets() nil = %v, want %v (%v)", gotTargets == nil, tt.targets == nil, gotTargets) | |
| } | |
| if len(gotTargets) != len(tt.targets) { | |
| t.Fatalf("Targets() len = %d, want %d (%v)", len(gotTargets), len(tt.targets), gotTargets) | |
| } | |
| for i := range gotTargets { | |
| if gotTargets[i] != tt.targets[i] { | |
| t.Fatalf("Targets()[%d] = %q, want %q", i, gotTargets[i], tt.targets[i]) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/types/common_test.go` around lines 50 - 58, Test currently treats
nil and empty slice as equivalent for Targets(), breaking the contract that
ProxmoxUnknown should return nil; assign gotTargets := tt.ptype.Targets() (move
this line before checks), then if tt.targets == nil assert gotTargets == nil
explicitly (or use reflect.DeepEqual(gotTargets, tt.targets)) instead of
comparing lengths, otherwise continue with length and element comparisons;
reference the Targets() method, the test variable gotTargets, and the
ProxmoxUnknown case when adding the nil-check.
This was referenced May 5, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements