Conversation
Fix GO-2026-5018 by updating golang.org/x/crypto and related checksums.
Ensure module checksums are fetched before vulnerability scanning.
Return both JSON and legacy parse errors from LoadManifest and cover the missing-archive metadata path in tests.
Parse legacy targets and version fields in inspectRcloneMetadataManifest to match local manifest handling.
Return scanner errors from parseLegacyMetadata and fail fast when legacy metadata scanning is truncated.
Add broader coverage for safeexec and staged/orchestrated restore flows.
Use accessControlApplyGeteuid and mountGuardGeteuid in maybeApply paths so CI tests can exercise error branches without real root, matching hooks used elsewhere in orchestrator.
Stop runPBSPXARStep from canceling sibling datastores on the first failure. Add isParentContextError and handlePBSPXARStepError so parent cancel/deadline propagates while datastore errors remain best-effort. Stabilizes race CI.
Add branch and integration tests for systemctl, sensors, dmidecode, and related system collection paths in collector_system_test.go.
Dependency ReviewThe following issues were found:
License Issuesgo.mod
OpenSSF Scorecard
Scanned Files
|
Review Summary by QodoFix PXAR parallel collection, enhance error handling, and expand test coverage
WalkthroughsDescription• **Fix PXAR parallel collection error handling**: Isolate per-datastore errors in parallel PXAR backup step to continue processing all datastores instead of stopping at first error, while properly distinguishing parent context cancellations from local failures • **Preserve legacy manifest parse causes**: Improve error messages to include both JSON and legacy parse failures, with scanner errors properly propagated through the parsing chain • **Align remote legacy manifest parsing**: Add support for BACKUP_TARGETS, PROXMOX_TARGETS, and version metadata fields (PROXMOX_VERSION, PVE_VERSION, PBS_VERSION) with target deduplication • **Expand test coverage significantly**: Add comprehensive tests for system collector probes, PVE collector operations, access control restoration, PVE staged apply, safeexec command validation, and legacy metadata parsing with error scenarios • **Improve testability**: Inject geteuid() functions in mount guard and access control modules for better test isolation • **Bump dependencies**: Update golang.org/x/crypto to v0.52.0 and golang.org/x/sys to v0.45.0 • **CI hardening**: Add module download step before govulncheck execution to ensure proper dependency caching Diagramflowchart LR
A["PXAR Parallel<br/>Collection"] -->|"isolate errors"| B["Per-Datastore<br/>Error Handling"]
B -->|"distinguish"| C["Parent Context<br/>vs Local Errors"]
C -->|"propagate"| D["Combined Error<br/>Results"]
E["Legacy Manifest<br/>Parsing"] -->|"preserve causes"| F["Enhanced Error<br/>Messages"]
F -->|"include"| G["JSON + Legacy<br/>Failures"]
H["Remote Manifest<br/>Parsing"] -->|"align with"| I["Additional Metadata<br/>Fields"]
I -->|"deduplicate"| J["Targets & Versions"]
K["Test Coverage"] -->|"expand"| L["System, PVE, ACL,<br/>SafeExec Tests"]
File Changes1. internal/backup/collector_system_test.go
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
📝 WalkthroughWalkthroughRefactors PXAR datastore concurrency and parent-context error handling; surfaces legacy manifest and scanner errors; injects privilege-get hooks for testability; updates CI/action pins and module versions; and adds large suites of tests for PVE, system collectors, PXAR behavior, and access-control staged-apply flows. ChangesInfrastructure and Baseline Dependency Updates
Legacy manifest parsing and error reporting
PXAR Context Propagation and Datastore Concurrency
Orchestrator Testability and Refactoring
Comprehensive Backup Collection Test Coverage
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bumps the actions-updates group with 3 updates: [codecov/codecov-action](https://github.com/codecov/codecov-action), [goreleaser/goreleaser-action](https://github.com/goreleaser/goreleaser-action) and [github/codeql-action](https://github.com/github/codeql-action). Updates `codecov/codecov-action` from 6.0.0 to 6.0.1 - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@57e3a13...e79a696) Updates `goreleaser/goreleaser-action` from 7.2.1 to 7.2.2 - [Release notes](https://github.com/goreleaser/goreleaser-action/releases) - [Commits](goreleaser/goreleaser-action@1a80836...5daf1e9) Updates `github/codeql-action` from 4.35.5 to 4.36.0 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@9e0d7b8...7211b7c) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 6.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-updates - dependency-name: goreleaser/goreleaser-action dependency-version: 7.2.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-updates - dependency-name: github/codeql-action dependency-version: 4.36.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+324
to
+330
| func isParentContextError(ctx context.Context, err error) bool { | ||
| if err == nil || ctx == nil { | ||
| return false | ||
| } | ||
| ctxErr := ctx.Err() | ||
| return ctxErr != nil && errors.Is(err, ctxErr) | ||
| } |
There was a problem hiding this comment.
isContextCancellationError with distinct but subtle semantics
isParentContextError and isContextCancellationError differ in one critical way: the existing function short-circuits on any context.Canceled or context.DeadlineExceeded value regardless of the parent context's state, while the new one only returns true when the parent context is already done and the error matches its current error. This distinction is load-bearing (it's what lets handlePBSPXARStepError treat a locally-cancelled child context as a warning rather than a propagated error), but the names give no hint of it. Consider a comment or a name like isContextDoneError / matchesParentContextCause to make the distinction self-documenting.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Owner
Author
There was a problem hiding this comment.
Good catch — the distinction is intentional.
isContextCancellationError is broad, while this helper is intentionally strict: it returns true only when the error matches the current parent context cause (ctx.Err()).
We use this to avoid swallowing locally returned context.Canceled values in PXAR best-effort paths.
I agree the naming can be clearer; I’ll either rename it to something like matchesParentContextCause or add an explicit comment.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/backup/checksum_legacy_test.go (1)
104-106: ⚡ Quick winAssert both parse causes in this test.
This test only validates the legacy cause. Please also assert the JSON parse cause so regressions in combined-error construction are caught.
Proposed test hardening
if !strings.Contains(err.Error(), "cannot stat archive") { t.Fatalf("error=%v; want legacy stat cause", err) } + if !strings.Contains(err.Error(), "failed to parse manifest as JSON") { + t.Fatalf("error=%v; want json parse cause", err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/backup/checksum_legacy_test.go` around lines 104 - 106, The test currently only asserts the legacy cause by checking strings.Contains(err.Error(), "cannot stat archive"); update the test to also assert the JSON parse cause is present in the combined error (e.g. assert that err.Error() contains a JSON-related substring such as "json" or "parse" or the specific parse message your code produces) so both causes are validated; modify the block around the existing check to require both strings.Contains(err.Error(), "cannot stat archive") and strings.Contains(err.Error(), "<your JSON parse substring>") (use the exact parse message emitted by your code) and fail the test if either is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/backup/checksum.go`:
- Around line 171-175: The LoadManifest function currently shadows the outer err
when calling json.Unmarshal, causing the final fmt.Errorf to report the wrong
JSON parse error; change the json.Unmarshal call to assign its error to a new
variable (e.g., unmarshalErr) or reuse the outer err (do not ":=") so you
capture the real JSON error, then use that variable (unmarshalErr) in the
combined error message alongside legacyErr in the fmt.Errorf call; locate
json.Unmarshal, os.ReadFile, legacyErr and the final fmt.Errorf in LoadManifest
to implement this fix.
In `@internal/orchestrator/pve_staged_apply_additional_test.go`:
- Around line 13-20: Tests currently mutate fixed /mnt/pve paths and the helper
requireWritablePveMountRoot only MkdirAlls and ignores removal errors; change
requireWritablePveMountRoot to actually verify writability by creating a
uniquely named probe file (use a random/test-specific suffix), writing to it,
fsyncing if possible, then removing it and t.Skip on any error; stop using fixed
absolute storage names like "activate-ok"/"guard-ok" and instead generate
per-test unique storage IDs (e.g., proxsave-test-<rand>) and derive staged
configs and cleanup paths from those IDs; replace any os.RemoveAll calls that
target fixed /mnt/pve/* with removals scoped to the generated test-specific
paths and update the tests around requireWritablePveMountRoot plus the areas
referenced (near the other occurrences) to use the new helper and unique IDs.
In `@internal/orchestrator/restore_access_control_additional_test.go`:
- Line 537: The test is passing a nil context to applyPVEAccessControlFromStage
which violates Go conventions; replace the nil with a non-nil context such as
context.Background() when calling applyPVEAccessControlFromStage(nil,
newTestLogger(), "/stage") so update that call to use context.Background()
(ensure you import context if not already) to provide a proper context to the
function.
- Line 129: The test calls applyPBSAccessControlFromStage with a nil context
which violates Go conventions; update the call to pass context.Background()
instead of nil (e.g., replace the first argument in
applyPBSAccessControlFromStage(nil, logger, "/stage") with context.Background())
so the test uses a valid non-nil context while preserving existing behavior.
---
Nitpick comments:
In `@internal/backup/checksum_legacy_test.go`:
- Around line 104-106: The test currently only asserts the legacy cause by
checking strings.Contains(err.Error(), "cannot stat archive"); update the test
to also assert the JSON parse cause is present in the combined error (e.g.
assert that err.Error() contains a JSON-related substring such as "json" or
"parse" or the specific parse message your code produces) so both causes are
validated; modify the block around the existing check to require both
strings.Contains(err.Error(), "cannot stat archive") and
strings.Contains(err.Error(), "<your JSON parse substring>") (use the exact
parse message emitted by your code) and fail the test if either is missing.
🪄 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: cdbaec5b-12ba-49fe-9918-e81b1b2a34fc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.github/workflows/codecov.yml.github/workflows/release.yml.github/workflows/security-ultimate.ymlgo.modinternal/backup/checksum.gointernal/backup/checksum_additional_test.gointernal/backup/checksum_legacy_test.gointernal/backup/collector_bricks.gointernal/backup/collector_bricks_pbs_features.gointernal/backup/collector_pbs_datastore.gointernal/backup/collector_pve_coverage_test.gointernal/backup/collector_pxar_datastore_test.gointernal/backup/collector_system_test.gointernal/orchestrator/decrypt.gointernal/orchestrator/decrypt_test.gointernal/orchestrator/pve_staged_apply.gointernal/orchestrator/pve_staged_apply_additional_test.gointernal/orchestrator/restore_access_control.gointernal/orchestrator/restore_access_control_additional_test.gointernal/safeexec/safeexec_test.go
| t.Fatalf("expected context.Canceled, got %v", err) | ||
| } | ||
|
|
||
| if err := applyPBSAccessControlFromStage(nil, logger, "/stage"); err != nil { |
There was a problem hiding this comment.
Replace nil context with context.Background().
Passing nil as a context violates Go conventions. Use context.Background() for testing scenarios where context behavior is not the focus.
🔧 Proposed fix
- if err := applyPBSAccessControlFromStage(nil, logger, "/stage"); err != nil {
+ if err := applyPBSAccessControlFromStage(context.Background(), logger, "/stage"); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := applyPBSAccessControlFromStage(nil, logger, "/stage"); err != nil { | |
| if err := applyPBSAccessControlFromStage(context.Background(), logger, "/stage"); err != nil { |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 129-129: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/restore_access_control_additional_test.go` at line 129,
The test calls applyPBSAccessControlFromStage with a nil context which violates
Go conventions; update the call to pass context.Background() instead of nil
(e.g., replace the first argument in applyPBSAccessControlFromStage(nil, logger,
"/stage") with context.Background()) so the test uses a valid non-nil context
while preserving existing behavior.
| t.Fatalf("expected context.Canceled, got %v", err) | ||
| } | ||
|
|
||
| if err := applyPVEAccessControlFromStage(nil, newTestLogger(), "/stage"); err != nil { |
There was a problem hiding this comment.
Replace nil context with context.Background().
Passing nil as a context violates Go conventions. Use context.Background() for testing scenarios where context behavior is not the focus.
🔧 Proposed fix
- if err := applyPVEAccessControlFromStage(nil, newTestLogger(), "/stage"); err != nil {
+ if err := applyPVEAccessControlFromStage(context.Background(), newTestLogger(), "/stage"); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := applyPVEAccessControlFromStage(nil, newTestLogger(), "/stage"); err != nil { | |
| if err := applyPVEAccessControlFromStage(context.Background(), newTestLogger(), "/stage"); err != nil { |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 537-537: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/restore_access_control_additional_test.go` at line 537,
The test is passing a nil context to applyPVEAccessControlFromStage which
violates Go conventions; replace the nil with a non-nil context such as
context.Background() when calling applyPVEAccessControlFromStage(nil,
newTestLogger(), "/stage") so update that call to use context.Background()
(ensure you import context if not already) to provide a proper context to the
function.
json.Unmarshal was called with :=, shadowing the outer err variable. The combined error message reported JSON (<nil>) instead of the actual parse failure. Rename to unmarshalErr to capture and propagate it. Harden tests to assert both JSON and legacy causes in combined errors.
… tests Replace fixed /mnt/pve/<name> storage IDs (activate-ok, proxsave-offline, mkdir-fail, etc.) with unique per-run IDs using a proxsave-it-<hash>-<label> prefix. Add safety helpers that refuse RemoveAll/Remove on paths not owned by the test (no proxsave-it- prefix), preventing accidental collision with real PVE storage IDs on privileged hosts. Also add requireWritablePveMountRoot with a real write/remove probe and TestPveMountIntegrationTestPathSafety to guard the path-ownership logic.
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
Bug Fixes
Tests
Chores
Greptile Summary
This PR addresses PXAR parallel collection isolation, improves manifest error reporting, expands test coverage, and hardens the CI pipeline. It touches three functional areas: the backup collection layer, the orchestrator's legacy metadata parsing, and GitHub Actions workflow hardening.
runPBSPXARStepno longer cancels the shared context on the first per-datastore error; it now collects all per-datastore failures witherrors.Joinand the newhandlePBSPXARStepErrorhelper treats datastore errors as best-effort warnings unless the parent context itself is done.LoadManifestnow surfaces both the JSON parse cause and the legacy parse cause in its combined error message;parseLegacyMetadatanow propagatesbufio.Scannererrors instead of silently succeeding.inspectRcloneMetadataManifestindecrypt.gonow handlesBACKUP_TARGETS,PROXMOX_TARGETS,PROXMOX_VERSION,PVE_VERSION, andPBS_VERSIONfields with deduplication logic for targets.Confidence Score: 5/5
Safe to merge — all changed production paths are well-covered by new tests and the behavior changes are intentional and consistent with the PR description.
The core PXAR concurrency fix (collecting all per-datastore errors instead of canceling on first failure) is thoroughly exercised across six new targeted test cases. The manifest error-reporting changes are backwards-compatible (callers only check err != nil). The orchestrator's legacy metadata parsing additions are additive and tested. No existing contract is broken.
No files require special attention; the test coverage for collector_pbs_datastore.go is particularly thorough.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[runPBSPXARStep] -->|pre-check| B{ctx.Err?} B -->|yes| C[return ctxErr] B -->|no| D[launch goroutines per datastore] D --> E{acquire semaphore} E -->|ctx.Done| F[goroutine exits] E -->|acquired| G[fn ctx ds state] G -->|err = nil| H[no-op] G -->|err != nil| I{isParentContextError?} I -->|yes| J[goroutine exits silently] I -->|no| K[Debug log + append to errs] H & J & K --> L[wg.Wait] L --> M{ctx.Err after wait?} M -->|ctxErr + errs| N[errors.Join both] M -->|ctxErr only| O[return ctxErr] M -->|no ctxErr| P[errors.Join errs or nil]Comments Outside Diff (1)
internal/backup/checksum.go, line 116-120 (link)The previous message used
%wto wraperr, so callers could doerrors.As(returnedErr, &*json.SyntaxError{})or similar. The new format uses%vfor botherrandlegacyErr, so neither is reachable viaerrors.Is/errors.As. The existing callers inbackup_sources.goandstorage/local.goonly checkerr != nil, so there is no current breakage, but the semantic change is worth calling out if future callers ever need to distinguish parse failure causes.Reviews (2): Last reviewed commit: "test(orchestrator): use unique per-run s..." | Re-trigger Greptile