Conversation
Verifies and fixes the waitForRetry hook handling in CloudStorage. Adds a defensive wrapper that calls waitForRetry only when it is set, otherwise returning nil, and replaces the direct calls in the remote check and upload retry paths. Also adds regression tests to confirm CloudStorage does not panic when the struct is constructed without that hook.
…decar paths Normalize bundle inputs before generating canonical bundle paths and legacy sidecar filenames, so bundle-based inputs do not produce sidecars like *.bundle.tar.sha256. Adds regression tests for repeated bundle suffixes and bundle-derived candidate paths.
Create a fresh HA test environment inside each t.Run subtest so overrides of package-level globals such as restoreFS, haApplyGeteuid, haIsMounted, and haArmRollback do not leak across sibling subtests. This keeps HA restore tests independent and prevents order-dependent failures.
…l interfaces Add a shared isNilInterface helper in decrypt_workflow_ui.go and use it for UI nil checks so both true nil and typed-nil interface values are rejected safely. Also adds regression tests covering typed-nil UI inputs across the decrypt workflow helpers.
…d of timer-driven Replace timer-based key injection in withTimedSimAppSequence with synchronization based on rendered SimulationScreen state, so timed keys are sent only after the expected TUI view is visible and the screen has advanced. This removes flaky wall-clock waits from the decrypt TUI e2e helper and keeps the decrypt TUI tests stable.
…s and delivered first response Fix the webhook retry cancellation test to use an atomic attempt counter, fully deliver the first HTTP error response before canceling the context, and use a non-zero retry delay so the retry path is actually exercised. This makes the test deterministic and avoids races around early cancellation and unsynchronized attempt counting.
Encode SECONDARY_PATH and SECONDARY_LOG_PATH safely before writing them into the env template, so paths containing #, spaces, or quotes do not break .env parsing. Also adds a round-trip test to verify quoted secondary paths are written and parsed back correctly.
Update env_mutation_test.go to parse the mutated template into key/value pairs and assert exact final values with single occurrences, instead of relying on strings.Contains. This prevents tests from passing when duplicate or stale env entries remain in the rendered template.
Update the setBaseDirEnv test helper so passing an empty string truly unsets BASE_DIR instead of setting it to an empty value. This keeps tests aligned with code paths that rely on os.LookupEnv to distinguish an unset variable from a present-but-empty one.
rim trailing slashes from preserved entry names before appending the display suffix in formatNewInstallPreservedEntries, so inputs like env/ or build// render as env/ and build/ instead of env//. Also adds a regression test covering slash-terminated preserved entries.
Remove the obsolete clearImmutableAttributes wrapper now that the install flow uses clearImmutableAttributesWithContext directly. This keeps the install codepath on the context-aware implementation and removes an unused legacy symbol.
…nfig mode Check ctx.Err() before the missing-file fast path and again before returning the final existing-config decision in promptExistingConfigModeCLI. This ensures a canceled context stops the install flow before it proceeds into config generation, and includes regression coverage for the missing-file cancellation case.
…lper Remove the extra t.Cleanup from captureCLIStdout and keep the existing deferred restoration logic as the single stdout cleanup path. This simplifies the helper and avoids duplicate no-op cleanup.
Replace the hand-duplicated invalidPermissionsTemplate in migration_test.go with a fixture derived from baseInstallTemplate, overriding only the fields needed for the invalid-permissions case. This keeps the migration test fixture aligned with the canonical install template and avoids future drift when template keys change.
…ests Update the restore TUI simulation tests to verify that confirmRestoreTUI passes the original context through to promptYesNoTUIFunc, instead of only checking the returned boolean result. This makes the tests fail if context propagation is broken in the confirmation flow.
Send validateDistinctNewPathInput validation errors to stderr in the CLI workflow instead of stdout, keeping user-facing error output consistent with other CLI error paths. Also updates the CLI workflow test helper and assertions to verify the message is emitted on stderr.
…ng timeout state Wait for the restore TUI countdown goroutine to exit before reading timedOut and cancelled in promptYesNoTUIWithCountdown, using a done channel tied to the ticker worker. This avoids races during TUI shutdown and prevents countdown updates from being queued after the app has already stopped.
Update writeIdentityFileWithContext to check ctx.Err() before writing and always restore the immutable attribute in deferred cleanup using a non-cancelable context. This prevents the identity file from being left mutable when the write path exits early due to cancellation or write failures, and adds regression tests for both cases.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughContext-safety, robustness, and test hardening across installers, config rendering, identity persistence, orchestrator UIs/TUI, storage path normalization, retry behavior, and many tests; changes include context cancellation checks, typed-nil detection, env-value quoting, injectable file helpers, iterative bundle-path normalization, and added/updated tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/config/migration_test.go (1)
278-282: Prefer test-time fixture construction over package-init panic.At Line 278, a missing key would panic during package initialization and stop all tests in this package. Consider deriving this fixture during test execution for better failure isolation.
Proposed refactor
-var invalidPermissionsTemplate = overrideTemplateValues(baseInstallTemplate, map[string]string{ - "SET_BACKUP_PERMISSIONS": "true", - "BACKUP_USER": "", - "BACKUP_GROUP": "", -}) +func invalidPermissionsTemplate() string { + return overrideTemplateValues(baseInstallTemplate, map[string]string{ + "SET_BACKUP_PERMISSIONS": "true", + "BACKUP_USER": "", + "BACKUP_GROUP": "", + }) +}And update usage in the test:
withTemplate(t, invalidPermissionsTemplate(), func() { // ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/migration_test.go` around lines 278 - 282, The package-level variable invalidPermissionsTemplate should be converted to a test-time fixture function to avoid panics at package init; replace the var with a function like invalidPermissionsTemplate() that returns overrideTemplateValues(baseInstallTemplate, map[string]string{ "SET_BACKUP_PERMISSIONS":"true", "BACKUP_USER":"", "BACKUP_GROUP":"" }), and update tests that reference invalidPermissionsTemplate to call invalidPermissionsTemplate() (e.g. withTemplate(t, invalidPermissionsTemplate(), func() { ... })); this ensures overrideTemplateValues is evaluated during test execution rather than at package initialization.cmd/proxsave/install_existing_config.go (1)
33-42: Cancellation check afteros.Statis reasonable but non-interruptible.The
ctx.Err()check afteros.IsNotExistcatches cancellation that occurred before or during the stat. However,os.Statitself doesn't respect context cancellation – if the stat blocks (e.g., on a hung NFS mount), it won't be interrupted.Given the
safefs.Stat(ctx, path, timeout)helper exists in the codebase, consider whether a context-aware stat is warranted here for consistency with the broader context-propagation goals of this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 33 - 42, The os.Stat call is not context-aware and can block; replace the blocking os.Stat(configPath) with the context-aware safefs.Stat(ctx, configPath, timeout) helper (pick an appropriate timeout consistent with other callers) so the stat respects cancellation; keep the existing error handling logic (os.IsNotExist branch, ctx.Err() check, and returns existingConfigCancel/existingConfigOverwrite and the wrapped error) but adapt it to use the safefs.Stat return values and error semantics.internal/orchestrator/decrypt_workflow_ui_test.go (1)
305-341: Consider asserting the exact error message for consistency.Unlike the other typed-nil tests, this test only checks that an error is returned but doesn't verify the error message is
"decrypt workflow UI not available". For consistency with the other tests in this file:🔧 Suggested fix
- if _, err := preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil); err == nil { + _, err := preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil) + if err == nil { t.Fatal("expected error for typed-nil UI") } + if got, want := err.Error(), "decrypt workflow UI not available"; got != want { + t.Fatalf("error=%q, want %q", got, want) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_workflow_ui_test.go` around lines 305 - 341, The test TestPreparePlainBundleWithUIRejectsTypedNilUI currently only checks that an error is returned; update it to assert the exact error message returned by preparePlainBundleWithUI equals "decrypt workflow UI not available" to match other typed-nil tests. Modify the final assertion to capture the err value and compare err.Error() (or errors.Is/As if the implementation returns a typed error) against the literal "decrypt workflow UI not available", failing the test if it differs so behavior is consistent and deterministic.internal/orchestrator/restore_ha_additional_test.go (1)
411-419: Minor redundancy in explicit cleanup.The explicit
t.Cleanupat line 414 is redundant sincesetupHATestEnvalready captures and restoreshaIsRealRestoreFSvia its own cleanup. The same applies to line 451 forhaIsMounted. This is harmless but adds unnecessary code.♻️ Optional: Remove redundant cleanup
t.Run("skips on non-system restore fs", func(t *testing.T) { env := newEnv(t) haIsRealRestoreFS = func(fs FS) bool { return false } - t.Cleanup(func() { haIsRealRestoreFS = func(fs FS) bool { return true } }) if err := maybeApplyPVEHAWithUI(context.Background(), &fakeRestoreWorkflowUI{}, env.logger, env.plan, nil, nil, env.stageRoot, false); err != nil {The same simplification can be applied at line 451.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_ha_additional_test.go` around lines 411 - 419, The test adds redundant explicit t.Cleanup calls that restore the globals haIsRealRestoreFS and haIsMounted even though setupHATestEnv already captures and restores those values; remove the explicit t.Cleanup closures in the test(s) that set haIsRealRestoreFS and haIsMounted (the test calling maybeApplyPVEHAWithUI and its counterpart that mounts checks) so the setupHATestEnv cleanup handles restoring the globals and avoid duplicate cleanup logic.internal/config/env_mutation_test.go (1)
88-111: Good coverage for quoting behavior with round-trip validation.The test validates both the raw encoded output (single-quoted values) and the parsed result via
parseEnvFile, ensuring the quoting is correctly decoded back to the original unescaped values. This provides strong confidence in the end-to-end encoding/decoding flow.Consider adding an edge case for values containing both quotes and shell-special characters (e.g.,
path'"$var) to verifystrconv.Quotefallback behavior.📋 Optional: Add edge case test for strconv.Quote fallback
func TestApplySecondaryStorageSettingsQuotesFallback(t *testing.T) { template := "SECONDARY_ENABLED=false\nSECONDARY_PATH=\nSECONDARY_LOG_PATH=\n" // Value with both ' and " triggers strconv.Quote got := ApplySecondaryStorageSettings(template, true, `/mnt/test'"path`, "") values, counts := parseMutatedEnvTemplate(t, got) assertMutatedEnvValue(t, values, counts, "SECONDARY_ENABLED", "true") // strconv.Quote escapes both quote types if !strings.HasPrefix(values["SECONDARY_PATH"], `"`) { t.Fatalf("expected strconv.Quote output, got %q", values["SECONDARY_PATH"]) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation_test.go` around lines 88 - 111, Add an edge-case test that verifies ApplySecondaryStorageSettings falls back to strconv.Quote when a value contains both single and double quotes: create a new test function (e.g., TestApplySecondaryStorageSettingsQuotesFallback) that calls ApplySecondaryStorageSettings with a path like `/mnt/test'"path`, use parseMutatedEnvTemplate and assertMutatedEnvValue for "SECONDARY_ENABLED" and "SECONDARY_PATH", and assert the encoded value in values["SECONDARY_PATH"] starts with `"` (use strings.HasPrefix) to confirm strconv.Quote was used; remember to import "strings" in the test file.
🤖 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/storage/backup_files.go`:
- Line 36: After calling normalizeBundleBasePath(base) ensure the normalized
base is not empty before generating cleanup candidate paths: check the result of
normalizeBundleBasePath in the function that calls it and if it's empty return
early or skip building relative candidates (the code that currently creates
paths like ".sha256" / ".metadata" around lines where candidates are assembled).
Update the logic around normalizeBundleBasePath and the candidate-generation
block so that when normalizeBundleBasePath(base) == "" no relative cleanup
targets are produced.
In `@internal/storage/cloud.go`:
- Around line 1768-1773: The no-op path in CloudStorage.callWaitForRetry
currently returns nil when c.waitForRetry is nil, which hides context
cancellation/deadline; change the fallback to check and return ctx.Err() (i.e.,
if c.waitForRetry == nil then return ctx.Err() so context.Canceled or
context.DeadlineExceeded is propagated), leaving the existing branch that calls
c.waitForRetry(ctx, d) unchanged.
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 33-42: The os.Stat call is not context-aware and can block;
replace the blocking os.Stat(configPath) with the context-aware safefs.Stat(ctx,
configPath, timeout) helper (pick an appropriate timeout consistent with other
callers) so the stat respects cancellation; keep the existing error handling
logic (os.IsNotExist branch, ctx.Err() check, and returns
existingConfigCancel/existingConfigOverwrite and the wrapped error) but adapt it
to use the safefs.Stat return values and error semantics.
In `@internal/config/env_mutation_test.go`:
- Around line 88-111: Add an edge-case test that verifies
ApplySecondaryStorageSettings falls back to strconv.Quote when a value contains
both single and double quotes: create a new test function (e.g.,
TestApplySecondaryStorageSettingsQuotesFallback) that calls
ApplySecondaryStorageSettings with a path like `/mnt/test'"path`, use
parseMutatedEnvTemplate and assertMutatedEnvValue for "SECONDARY_ENABLED" and
"SECONDARY_PATH", and assert the encoded value in values["SECONDARY_PATH"]
starts with `"` (use strings.HasPrefix) to confirm strconv.Quote was used;
remember to import "strings" in the test file.
In `@internal/config/migration_test.go`:
- Around line 278-282: The package-level variable invalidPermissionsTemplate
should be converted to a test-time fixture function to avoid panics at package
init; replace the var with a function like invalidPermissionsTemplate() that
returns overrideTemplateValues(baseInstallTemplate, map[string]string{
"SET_BACKUP_PERMISSIONS":"true", "BACKUP_USER":"", "BACKUP_GROUP":"" }), and
update tests that reference invalidPermissionsTemplate to call
invalidPermissionsTemplate() (e.g. withTemplate(t, invalidPermissionsTemplate(),
func() { ... })); this ensures overrideTemplateValues is evaluated during test
execution rather than at package initialization.
In `@internal/orchestrator/decrypt_workflow_ui_test.go`:
- Around line 305-341: The test TestPreparePlainBundleWithUIRejectsTypedNilUI
currently only checks that an error is returned; update it to assert the exact
error message returned by preparePlainBundleWithUI equals "decrypt workflow UI
not available" to match other typed-nil tests. Modify the final assertion to
capture the err value and compare err.Error() (or errors.Is/As if the
implementation returns a typed error) against the literal "decrypt workflow UI
not available", failing the test if it differs so behavior is consistent and
deterministic.
In `@internal/orchestrator/restore_ha_additional_test.go`:
- Around line 411-419: The test adds redundant explicit t.Cleanup calls that
restore the globals haIsRealRestoreFS and haIsMounted even though setupHATestEnv
already captures and restores those values; remove the explicit t.Cleanup
closures in the test(s) that set haIsRealRestoreFS and haIsMounted (the test
calling maybeApplyPVEHAWithUI and its counterpart that mounts checks) so the
setupHATestEnv cleanup handles restoring the globals and avoid duplicate cleanup
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c077834-4b23-4d3e-9380-111dce26c341
📒 Files selected for processing (24)
cmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/webhook_test.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/storage/backup_files.gointernal/storage/backup_files_test.gointernal/storage/cloud.gointernal/storage/cloud_test.go
💤 Files with no reviewable changes (1)
- cmd/proxsave/install.go
| // When includeBundle is true, both the bundle and the legacy single-file layout | ||
| // are included so retention can clean up either form. | ||
| func buildBackupCandidatePaths(base string, includeBundle bool) []string { | ||
| base = normalizeBundleBasePath(base) |
There was a problem hiding this comment.
Guard against empty normalized base before generating cleanup candidates.
At Line 36, inputs like .bundle.tar normalize to "", and Lines 56-60 then generate relative paths such as .sha256 / .metadata. That can unintentionally target unrelated files during cleanup.
Suggested fix
func buildBackupCandidatePaths(base string, includeBundle bool) []string {
base = normalizeBundleBasePath(base)
+ if base == "" {
+ return []string{}
+ }
seen := make(map[string]struct{})Also applies to: 56-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/backup_files.go` at line 36, After calling
normalizeBundleBasePath(base) ensure the normalized base is not empty before
generating cleanup candidate paths: check the result of normalizeBundleBasePath
in the function that calls it and if it's empty return early or skip building
relative candidates (the code that currently creates paths like ".sha256" /
".metadata" around lines where candidates are assembled). Update the logic
around normalizeBundleBasePath and the candidate-generation block so that when
normalizeBundleBasePath(base) == "" no relative cleanup targets are produced.
Comment on lines
+1768
to
+1773
| func (c *CloudStorage) callWaitForRetry(ctx context.Context, d time.Duration) error { | ||
| if c.waitForRetry != nil { | ||
| return c.waitForRetry(ctx, d) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Preserve context cancellation in the no-op waiter.
At Line 1769, when c.waitForRetry is nil, returning nil unconditionally can hide cancellation/deadline and cause callers to continue/return a stale operational error instead of context.Canceled/context.DeadlineExceeded.
💡 Proposed fix
func (c *CloudStorage) callWaitForRetry(ctx context.Context, d time.Duration) error {
if c.waitForRetry != nil {
return c.waitForRetry(ctx, d)
}
- return nil
+ if ctx == nil {
+ return nil
+ }
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ return 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
| func (c *CloudStorage) callWaitForRetry(ctx context.Context, d time.Duration) error { | |
| if c.waitForRetry != nil { | |
| return c.waitForRetry(ctx, d) | |
| } | |
| return nil | |
| } | |
| func (c *CloudStorage) callWaitForRetry(ctx context.Context, d time.Duration) error { | |
| if c.waitForRetry != nil { | |
| return c.waitForRetry(ctx, d) | |
| } | |
| if ctx == nil { | |
| return nil | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| return nil | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/cloud.go` around lines 1768 - 1773, The no-op path in
CloudStorage.callWaitForRetry currently returns nil when c.waitForRetry is nil,
which hides context cancellation/deadline; change the fallback to check and
return ctx.Err() (i.e., if c.waitForRetry == nil then return ctx.Err() so
context.Canceled or context.DeadlineExceeded is propagated), leaving the
existing branch that calls c.waitForRetry(ctx, d) unchanged.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai review focus on entire repository |
|
User feedback: Review with focus on the entire repository, not just the changed files. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/storage/cloud.go (1)
1768-1773:⚠️ Potential issue | 🟠 MajorPropagate context cancellation/deadline in nil waiter fallback.
At Line 1772, returning
nilunconditionally whenc.waitForRetryis unset can mask cancellation/deadline and surface stale retry errors.💡 Proposed fix
func (c *CloudStorage) callWaitForRetry(ctx context.Context, d time.Duration) error { if c.waitForRetry != nil { return c.waitForRetry(ctx, d) } - return nil + if ctx == nil { + return nil + } + select { + case <-ctx.Done(): + return ctx.Err() + default: + return nil + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/cloud.go` around lines 1768 - 1773, The nil-fallback in CloudStorage.callWaitForRetry masks context cancellations and deadlines by always returning nil; update callWaitForRetry so when c.waitForRetry is nil it checks ctx.Err() and returns that error (if any) instead of unconditionally returning nil, otherwise return nil—this preserves context cancellation/deadline propagation for callers of callWaitForRetry.internal/storage/backup_files.go (1)
36-60:⚠️ Potential issue | 🟠 MajorAvoid emitting relative cleanup targets when normalized base is empty.
At Line 36, inputs like
.bundle.tarnormalize to an empty string. That flows into Line 51 and Line 58-Line 60, producing.bundle.tar/.sha256/.metadata-style relative candidates. This can make cleanup touch unintended files.Proposed fix
func buildBackupCandidatePaths(base string, includeBundle bool) []string { base = normalizeBundleBasePath(base) + if base == "" { + return []string{} + } seen := make(map[string]struct{}) add := func(path string) bool { if path == "" { return false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/backup_files.go` around lines 36 - 60, The normalized base can be empty (normalizeBundleBasePath returning "") which leads to relative cleanup targets; update the code in the function that builds files/candidates (using normalizeBundleBasePath, bundlePathFor, add and the candidates slice) to skip building or appending any entries when base == "" (i.e., return early or no-op the includeBundle block and avoid constructing candidates) so no ".bundle.tar", ".sha256", or ".metadata" style relative names are emitted.
🧹 Nitpick comments (5)
cmd/proxsave/install_existing_config.go (1)
63-80: Optional: deduplicate repeatedctx.Err()checks in switch cases.A tiny helper would keep this easier to maintain if return behavior changes later.
Refactor sketch
+ cancelIfDone := func() error { + if err := ctx.Err(); err != nil { + return err + } + return nil + } + switch strings.TrimSpace(choice) { case "": fallthrough case "3": - if err := ctx.Err(); err != nil { + if err := cancelIfDone(); err != nil { return existingConfigCancel, err } return existingConfigKeepContinue, nil case "1": - if err := ctx.Err(); err != nil { + if err := cancelIfDone(); err != nil { return existingConfigCancel, err } return existingConfigOverwrite, nil case "2": - if err := ctx.Err(); err != nil { + if err := cancelIfDone(); err != nil { return existingConfigCancel, err } return existingConfigEdit, nil case "0": - if err := ctx.Err(); err != nil { + if err := cancelIfDone(); err != nil { return existingConfigCancel, err } return existingConfigCancel, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 63 - 80, The switch contains repeated ctx.Err() checks; move the context error check to a single place to avoid duplication: call ctx.Err() once before entering the switch (or create a small helper like checkCtxErr() that returns the error) and return existingConfigCancel, err if non-nil, then run the switch which returns existingConfigKeepContinue, existingConfigOverwrite, existingConfigEdit, or existingConfigCancel as before; update the branches to return only the chosen constant (no ctx.Err() calls).internal/orchestrator/restore_ha_additional_test.go (1)
677-687: Consider acceptingfsparameter for explicitness (optional).The
makeHandlehelper implicitly relies on the globalrestoreFSbeing set by the precedingnewEnv(t)call. This works correctly in the current usage, but the coupling is subtle. If this pattern expands or someone refactors the test ordering, it could become fragile.An alternative would be to pass the filesystem explicitly:
♻️ Optional: make FS dependency explicit
- makeHandle := func(markerPath string, armedAt time.Time) *haRollbackHandle { + makeHandle := func(fs FS, markerPath string, armedAt time.Time) *haRollbackHandle { handle := &haRollbackHandle{ markerPath: markerPath, scriptPath: markerPath + ".sh", logPath: markerPath + ".log", armedAt: armedAt, timeout: defaultHARollbackTimeout, } - _ = restoreFS.WriteFile(markerPath, []byte("pending\n"), 0o640) + _ = fs.WriteFile(markerPath, []byte("pending\n"), 0o640) return handle }Then update call sites to pass
env.fs:return makeHandle(env.fs, markerPath, nowRestore()), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_ha_additional_test.go` around lines 677 - 687, The helper makeHandle currently closes over the global restoreFS set by newEnv(t); change makeHandle to accept an explicit fs parameter (e.g., fs afero.Fs or the test's filesystem type) and use that fs for WriteFile instead of restoreFS, then update all call sites (where makeHandle is invoked) to pass env.fs (or the appropriate env filesystem) so the dependency is explicit; reference haRollbackHandle, makeHandle, restoreFS and newEnv(t) when making these changes.internal/config/env_mutation_test.go (1)
88-112: Add coverage for thestrconv.Quotefallback branch.Great test addition for spaces and
#. Please add one case where the path contains both'and"so theencodeEnvValuefallback path is regression-protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation_test.go` around lines 88 - 112, Add a test case that triggers the encodeEnvValue fallback (strconv.Quote) by calling ApplySecondaryStorageSettings with a path containing both single and double quotes (e.g., a value with ' and "). Update or add to TestApplySecondaryStorageSettingsQuotesUnsafePaths (or create a new test) to pass that path into ApplySecondaryStorageSettings, assert the mutated env contains a quoted SECONDARY_PATH/SECONDARY_LOG_PATH via assertMutatedEnvValue, write the config to disk and parse it with parseEnvFile, and assert the parsed values exactly match the original unescaped path string to ensure the strconv.Quote branch is covered.internal/orchestrator/decrypt_tui_e2e_helpers_test.go (1)
49-55: Usek.Waitas the step timeout too.Right now the duration only acts as a boolean flag. If the expected text never appears, this loop waits until the outer 30s timeout below, which makes regressions much slower to diagnose.
One way to keep the per-step wait budget
- waitForScreenText := func(expected string) bool { + waitForScreenText := func(expected string, timeout time.Duration) bool { expected = strings.TrimSpace(expected) + timer := time.NewTimer(timeout) + defer timer.Stop() for { current := currentScreenState() if current.signature != "" { if (expected == "" || strings.Contains(current.text, expected)) && (lastInjectedState == "" || current.signature != lastInjectedState) { return true } } select { case <-done: return false + case <-timer.C: + t.Errorf("timed out waiting %s for %q", timeout, expected) + return false case <-screenStateCh: } } } @@ - if !waitForScreenText(k.WaitForText) { + if !waitForScreenText(k.WaitForText, k.Wait) { return }Also applies to: 139-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go` around lines 49 - 55, The timedSimKey struct's Wait field is currently only used as a boolean flag, causing the inner wait loop to fall back to the outer 30s timeout; change the loop that waits for WaitForText to respect k.Wait by creating a per-step deadline (e.g., stepDeadline := time.Now().Add(k.Wait) or default when k.Wait==0) and using that deadline for each iteration's timeout/selection so the wait for k.Wait elapses per step instead of deferring to the outer timeout; update the code that checks for WaitForText (references: timedSimKey, k.Wait, WaitForText) to break with an error when the per-step deadline passes.internal/orchestrator/decrypt_workflow_ui_test.go (1)
338-340: Tighten thepreparePlainBundleWithUItyped-nil assertion.This currently passes on any error, so an unrelated failure would still look green. Matching the exact
"decrypt workflow UI not available"error would make it as strict as the other typed-nil cases here.Suggested tightening
- if _, err := preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil); err == nil { - t.Fatal("expected error for typed-nil UI") - } + if _, err := preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil); err == nil { + t.Fatal("expected error for typed-nil UI") + } else if got, want := err.Error(), "decrypt workflow UI not available"; got != want { + t.Fatalf("error=%q, want %q", got, want) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_workflow_ui_test.go` around lines 338 - 340, The test currently only checks that preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil) returns a non-nil error; tighten it to assert that the error message equals the expected "decrypt workflow UI not available" (or use errors.Is if there is a sentinel error value exported for this case) so unrelated failures won't pass; update the assertion in decrypt_workflow_ui_test.go to compare err.Error() (or use errors.Is with the sentinel) against the exact expected string when calling preparePlainBundleWithUI.
🤖 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/config/env_mutation.go`:
- Around line 16-25: The encoding allows raw "\n" or "\r" into quoted env
output; modify the branch logic around the variable `value` so that if it
contains newline or carriage-return characters (e.g., strings.ContainsAny(value,
"\r\n")), you immediately return strconv.Quote(value) to force escaped multiline
encoding before the subsequent single- vs double-quote checks (the existing
checks using strings.ContainsAny(value, "# \t\r\n\"'") and
strings.Contains(value, "'") / strings.Contains(value, `"`)).
---
Duplicate comments:
In `@internal/storage/backup_files.go`:
- Around line 36-60: The normalized base can be empty (normalizeBundleBasePath
returning "") which leads to relative cleanup targets; update the code in the
function that builds files/candidates (using normalizeBundleBasePath,
bundlePathFor, add and the candidates slice) to skip building or appending any
entries when base == "" (i.e., return early or no-op the includeBundle block and
avoid constructing candidates) so no ".bundle.tar", ".sha256", or ".metadata"
style relative names are emitted.
In `@internal/storage/cloud.go`:
- Around line 1768-1773: The nil-fallback in CloudStorage.callWaitForRetry masks
context cancellations and deadlines by always returning nil; update
callWaitForRetry so when c.waitForRetry is nil it checks ctx.Err() and returns
that error (if any) instead of unconditionally returning nil, otherwise return
nil—this preserves context cancellation/deadline propagation for callers of
callWaitForRetry.
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 63-80: The switch contains repeated ctx.Err() checks; move the
context error check to a single place to avoid duplication: call ctx.Err() once
before entering the switch (or create a small helper like checkCtxErr() that
returns the error) and return existingConfigCancel, err if non-nil, then run the
switch which returns existingConfigKeepContinue, existingConfigOverwrite,
existingConfigEdit, or existingConfigCancel as before; update the branches to
return only the chosen constant (no ctx.Err() calls).
In `@internal/config/env_mutation_test.go`:
- Around line 88-112: Add a test case that triggers the encodeEnvValue fallback
(strconv.Quote) by calling ApplySecondaryStorageSettings with a path containing
both single and double quotes (e.g., a value with ' and "). Update or add to
TestApplySecondaryStorageSettingsQuotesUnsafePaths (or create a new test) to
pass that path into ApplySecondaryStorageSettings, assert the mutated env
contains a quoted SECONDARY_PATH/SECONDARY_LOG_PATH via assertMutatedEnvValue,
write the config to disk and parse it with parseEnvFile, and assert the parsed
values exactly match the original unescaped path string to ensure the
strconv.Quote branch is covered.
In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go`:
- Around line 49-55: The timedSimKey struct's Wait field is currently only used
as a boolean flag, causing the inner wait loop to fall back to the outer 30s
timeout; change the loop that waits for WaitForText to respect k.Wait by
creating a per-step deadline (e.g., stepDeadline := time.Now().Add(k.Wait) or
default when k.Wait==0) and using that deadline for each iteration's
timeout/selection so the wait for k.Wait elapses per step instead of deferring
to the outer timeout; update the code that checks for WaitForText (references:
timedSimKey, k.Wait, WaitForText) to break with an error when the per-step
deadline passes.
In `@internal/orchestrator/decrypt_workflow_ui_test.go`:
- Around line 338-340: The test currently only checks that
preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil)
returns a non-nil error; tighten it to assert that the error message equals the
expected "decrypt workflow UI not available" (or use errors.Is if there is a
sentinel error value exported for this case) so unrelated failures won't pass;
update the assertion in decrypt_workflow_ui_test.go to compare err.Error() (or
use errors.Is with the sentinel) against the exact expected string when calling
preparePlainBundleWithUI.
In `@internal/orchestrator/restore_ha_additional_test.go`:
- Around line 677-687: The helper makeHandle currently closes over the global
restoreFS set by newEnv(t); change makeHandle to accept an explicit fs parameter
(e.g., fs afero.Fs or the test's filesystem type) and use that fs for WriteFile
instead of restoreFS, then update all call sites (where makeHandle is invoked)
to pass env.fs (or the appropriate env filesystem) so the dependency is
explicit; reference haRollbackHandle, makeHandle, restoreFS and newEnv(t) when
making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bea43756-29ed-43f3-949b-333d0d0fbdf0
📒 Files selected for processing (24)
cmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/webhook_test.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/storage/backup_files.gointernal/storage/backup_files_test.gointernal/storage/cloud.gointernal/storage/cloud_test.go
💤 Files with no reviewable changes (1)
- cmd/proxsave/install.go
Comment on lines
+16
to
+25
| if !strings.ContainsAny(value, "# \t\r\n\"'") { | ||
| return value | ||
| } | ||
| if !strings.Contains(value, "'") { | ||
| return "'" + value + "'" | ||
| } | ||
| if !strings.Contains(value, `"`) { | ||
| return `"` + value + `"` | ||
| } | ||
| return strconv.Quote(value) |
There was a problem hiding this comment.
Prevent newline/CR injection in encoded env values.
"\n"/"\r" currently flow into single- or double-quoted output as literal line breaks, which can break the env file and inject unintended entries. Force escaped encoding for multiline input before quote-branching.
💡 Proposed fix
func encodeEnvValue(value string) string {
value = strings.TrimSpace(value)
if value == "" {
return ""
}
+
+ if strings.ContainsAny(value, "\r\n") {
+ return strconv.Quote(value)
+ }
- if !strings.ContainsAny(value, "# \t\r\n\"'") {
+ if !strings.ContainsAny(value, "# \t\"'") {
return value
}
if !strings.Contains(value, "'") {
return "'" + value + "'"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/env_mutation.go` around lines 16 - 25, The encoding allows
raw "\n" or "\r" into quoted env output; modify the branch logic around the
variable `value` so that if it contains newline or carriage-return characters
(e.g., strings.ContainsAny(value, "\r\n")), you immediately return
strconv.Quote(value) to force escaped multiline encoding before the subsequent
single- vs double-quote checks (the existing checks using
strings.ContainsAny(value, "# \t\r\n\"'") and strings.Contains(value, "'") /
strings.Contains(value, `"`)).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/orchestrator/restore_tui.go (1)
821-855:⚠️ Potential issue | 🔴 CriticalThe timeout path races the cancel handler.
Lines 801 and 833-834 create a data race on
cancelled: the form cancel callback writescancelled = truewhile the countdown goroutine independently writes to the same flag. No synchronization guards these concurrent writes—stopTicker()only ensures the goroutine exits, not that writes are serialized. A user cancel near the deadline triggers concurrent writes, violating Go's memory model. Similarly,result(line 797) is written by the form submit callback without protection.Fix this by using
sync/atomicfor the flags, communicating results through a channel, or protecting with a mutex. Rungo test -raceafter fixes.🤖 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 821 - 855, The cancel and timeout goroutines race on the shared variables `cancelled` and `result` (written by the form cancel/submit callbacks and the countdown goroutine); fix by removing unsynchronized shared writes and using a synchronized mechanism: either convert `cancelled` to an atomic flag via `sync/atomic` (e.g., atomic.StoreUint32/LoadUint32 on a uint32) and send the form submit `result` value over a dedicated channel instead of writing the variable directly, or protect both `cancelled` and `result` with a `sync.Mutex`; ensure `stopTicker()` still waits for the goroutine to exit and that the form callbacks send/atomic-set then return, and run `go test -race` to validate no data races remain.
♻️ Duplicate comments (1)
internal/config/env_mutation.go (1)
16-25:⚠️ Potential issue | 🟠 MajorNewline/CR injection vulnerability remains unaddressed.
The previous review correctly identified that
"\n"or"\r"characters will flow into single- or double-quoted output as literal line breaks, breaking the env file format and potentially injecting unintended entries.If
value = "path\nwith\nnewlines"and it contains no single quotes, it becomes'path\nwith\nnewlines'with literal newlines inside the quotes, which corrupts the env file structure.,
🐛 Proposed fix
func encodeEnvValue(value string) string { value = strings.TrimSpace(value) if value == "" { return "" } + if strings.ContainsAny(value, "\r\n") { + return strconv.Quote(value) + } + - if !strings.ContainsAny(value, "# \t\r\n\"'") { + if !strings.ContainsAny(value, "# \t\"'") { return value } if !strings.Contains(value, "'") { return "'" + value + "'" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation.go` around lines 16 - 25, The code in env_mutation.go currently wraps values containing newlines or CRs in single/double quotes which preserves literal line breaks and corrupts .env format; update the quoting logic so that if value contains '\n' or '\r' you always use strconv.Quote(value) (escape sequences) instead of returning "'" + value + "'" or `"` + value + "`; change the branching in the block that checks strings.ContainsAny(value, "...") so presence of '\n' or '\r' forces the strconv.Quote path (affecting the same snippet that currently checks for single-quote and double-quote presence).
🧹 Nitpick comments (4)
internal/storage/backup_files_test.go (1)
16-46: Consider adding an edge case test for empty normalized base.If the empty-base guard is added to
buildBackupCandidatePaths, a test case verifying that inputs like.bundle.tarreturn an empty/nil slice would strengthen coverage.Suggested test case
{ name: "empty after normalization", base: ".bundle.tar", includeBundle: true, want: nil, // or []string{} depending on implementation },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/backup_files_test.go` around lines 16 - 46, Add a unit test case to TestBuildBackupCandidatePathsNormalizesBundleInput to cover the edge where the normalized base is empty (e.g., base ".bundle.tar"); call buildBackupCandidatePaths via the existing test harness by adding a table entry with name "empty after normalization", base ".bundle.tar", includeBundle true (and optionally another with includeBundle false), and assert the returned slice is nil or empty (match the function's intended behavior) so the test verifies buildBackupCandidatePaths handles empty-normalized inputs correctly.cmd/proxsave/install_existing_config.go (1)
63-80: Consider whether repeatedctx.Err()checks are necessary afterpromptOptional.If
promptOptionalalready checksctx.Err()internally and returns an error on cancellation, these additional checks after each switch case may be redundant. However, they act as a safety net if the context is canceled betweenpromptOptionalreturning and processing the result.Given this is defensive and has negligible overhead, it's acceptable to keep. If you prefer consistency and leaner code, you could rely solely on
promptOptional's context handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 63 - 80, The switch branches repeatedly call ctx.Err() after promptOptional returns; if promptOptional already returns an error when the context is canceled, remove the redundant checks to simplify the code: in the switch handling the prompt result (returning existingConfigKeepContinue, existingConfigOverwrite, existingConfigEdit, existingConfigCancel), delete the per-case ctx.Err() checks and directly return the appropriate existingConfig* constant and nil error; keep a single context check only if you want a defensive global guard before the switch. Ensure you update logic around promptOptional, existingConfigCancel, existingConfigKeepContinue, existingConfigOverwrite, and existingConfigEdit consistently.internal/orchestrator/decrypt_workflow_ui_test.go (1)
338-340: Consider asserting the exact error message for consistency.Sibling typed-nil tests validate both non-nil and exact message; matching that pattern here would make failures more diagnostic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_workflow_ui_test.go` around lines 338 - 340, The test currently only checks that preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil) returns an error; update the test to also assert the exact error message for consistency with the sibling typed-nil tests by comparing err.Error() to the same expected string (or shared constant) used in those sibling tests so failures are diagnostic; locate the call to preparePlainBundleWithUI and the typedNil test case and add an assertion that the returned error message equals the expected message.internal/orchestrator/decrypt_tui_e2e_helpers_test.go (1)
158-163: TheWaitduration value is now unused; onlyWait > 0acts as a boolean gate.The condition
k.Wait > 0determines whether to wait for screen text, but the actual duration (e.g.,1 * time.Second,750 * time.Millisecond) is never used as a timeout. ThewaitForScreenTextfunction blocks indefinitely until the text appears ordoneis closed.This is likely intentional for deterministic screen-driven synchronization, but:
- The
Waitvalues in sequences (lines 315-345) suggest timeouts but don't function as such- If expected text never appears, tests hang until the outer context deadline
Consider either:
- Documenting that
Wait > 0is now a "should wait" flag, or- Adding a timeout fallback using
k.Waitto prevent indefinite hangs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go` around lines 158 - 163, The loop over keys currently treats k.Wait as a boolean because waitForScreenText blocks indefinitely; update the logic so k.Wait is used as a timeout: change the branch in the for _, k := range keys loop to call a new or updated waitForScreenTextWithTimeout (or wrap waitForScreenText call in a select with time.After(k.Wait)) and return if the timed wait fails, using the existing k.Wait durations (e.g., 1*time.Second, 750*time.Millisecond) as the timeout; keep the gate semantics when k.Wait == 0 (no wait) and ensure the function respects done/cancellation as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/orchestrator/restore_tui.go`:
- Around line 821-855: The cancel and timeout goroutines race on the shared
variables `cancelled` and `result` (written by the form cancel/submit callbacks
and the countdown goroutine); fix by removing unsynchronized shared writes and
using a synchronized mechanism: either convert `cancelled` to an atomic flag via
`sync/atomic` (e.g., atomic.StoreUint32/LoadUint32 on a uint32) and send the
form submit `result` value over a dedicated channel instead of writing the
variable directly, or protect both `cancelled` and `result` with a `sync.Mutex`;
ensure `stopTicker()` still waits for the goroutine to exit and that the form
callbacks send/atomic-set then return, and run `go test -race` to validate no
data races remain.
---
Duplicate comments:
In `@internal/config/env_mutation.go`:
- Around line 16-25: The code in env_mutation.go currently wraps values
containing newlines or CRs in single/double quotes which preserves literal line
breaks and corrupts .env format; update the quoting logic so that if value
contains '\n' or '\r' you always use strconv.Quote(value) (escape sequences)
instead of returning "'" + value + "'" or `"` + value + "`; change the branching
in the block that checks strings.ContainsAny(value, "...") so presence of '\n'
or '\r' forces the strconv.Quote path (affecting the same snippet that currently
checks for single-quote and double-quote presence).
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 63-80: The switch branches repeatedly call ctx.Err() after
promptOptional returns; if promptOptional already returns an error when the
context is canceled, remove the redundant checks to simplify the code: in the
switch handling the prompt result (returning existingConfigKeepContinue,
existingConfigOverwrite, existingConfigEdit, existingConfigCancel), delete the
per-case ctx.Err() checks and directly return the appropriate existingConfig*
constant and nil error; keep a single context check only if you want a defensive
global guard before the switch. Ensure you update logic around promptOptional,
existingConfigCancel, existingConfigKeepContinue, existingConfigOverwrite, and
existingConfigEdit consistently.
In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go`:
- Around line 158-163: The loop over keys currently treats k.Wait as a boolean
because waitForScreenText blocks indefinitely; update the logic so k.Wait is
used as a timeout: change the branch in the for _, k := range keys loop to call
a new or updated waitForScreenTextWithTimeout (or wrap waitForScreenText call in
a select with time.After(k.Wait)) and return if the timed wait fails, using the
existing k.Wait durations (e.g., 1*time.Second, 750*time.Millisecond) as the
timeout; keep the gate semantics when k.Wait == 0 (no wait) and ensure the
function respects done/cancellation as before.
In `@internal/orchestrator/decrypt_workflow_ui_test.go`:
- Around line 338-340: The test currently only checks that
preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil)
returns an error; update the test to also assert the exact error message for
consistency with the sibling typed-nil tests by comparing err.Error() to the
same expected string (or shared constant) used in those sibling tests so
failures are diagnostic; locate the call to preparePlainBundleWithUI and the
typedNil test case and add an assertion that the returned error message equals
the expected message.
In `@internal/storage/backup_files_test.go`:
- Around line 16-46: Add a unit test case to
TestBuildBackupCandidatePathsNormalizesBundleInput to cover the edge where the
normalized base is empty (e.g., base ".bundle.tar"); call
buildBackupCandidatePaths via the existing test harness by adding a table entry
with name "empty after normalization", base ".bundle.tar", includeBundle true
(and optionally another with includeBundle false), and assert the returned slice
is nil or empty (match the function's intended behavior) so the test verifies
buildBackupCandidatePaths handles empty-normalized inputs correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b888a76c-4b51-4128-a55b-041addc192f2
📒 Files selected for processing (24)
cmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/webhook_test.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/storage/backup_files.gointernal/storage/backup_files_test.gointernal/storage/cloud.gointernal/storage/cloud_test.go
💤 Files with no reviewable changes (1)
- cmd/proxsave/install.go
Comment on lines
+823
to
+835
| if err := writeIdentityFileWithContextSetImmutable(ctx, path, false, logger); err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| lockErr := writeIdentityFileWithContextSetImmutable(context.Background(), path, true, logger) | ||
| if lockErr == nil { | ||
| return | ||
| } | ||
| logDebug(logger, "Identity: writeIdentityFile: failed to restore immutable attribute: %v", lockErr) | ||
| if err == nil { | ||
| err = lockErr | ||
| } | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "internal" -type d 2>/dev/null | head -5Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
find . -path "*/internal/identity/identity.go" -type fRepository: tis24dev/proxsave
Length of output: 92
🏁 Script executed:
cat -n internal/identity/identity.go | sed -n '810,860p'Repository: tis24dev/proxsave
Length of output: 2025
🏁 Script executed:
cat -n internal/identity/identity.go | sed -n '40,80p'Repository: tis24dev/proxsave
Length of output: 2037
🏁 Script executed:
rg -n "writeIdentityFileWithContextSetImmutable\s*=" internal/identity/identity.goRepository: tis24dev/proxsave
Length of output: 140
🏁 Script executed:
cat -n internal/identity/identity.go | sed -n '1,100p'Repository: tis24dev/proxsave
Length of output: 4062
🏁 Script executed:
rg -n "func setImmutableAttributeWithContext" internal/identity/identity.go -A 25Repository: tis24dev/proxsave
Length of output: 988
🏁 Script executed:
rg -n "func setImmutableAttributeWithContext" internal/identity/identity.go -A 50 | head -60Repository: tis24dev/proxsave
Length of output: 1715
Move the relock defer before the unlock attempt and bound it with a timeout.
Line 823 can return before the defer at line 826 is armed, leaving the file unrestored if context is canceled during chattr -i. Additionally, line 827 uses bare context.Background(), which means a wedged relock can block indefinitely after the caller has canceled.
Suggested change
logDebug(logger, "Identity: writeIdentityFile: start path=%s contentBytes=%d", path, len(content))
- // Ensure file is writable even if immutable was previously set
- if err := writeIdentityFileWithContextSetImmutable(ctx, path, false, logger); err != nil {
- return err
- }
defer func() {
- lockErr := writeIdentityFileWithContextSetImmutable(context.Background(), path, true, logger)
+ relockCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ lockErr := writeIdentityFileWithContextSetImmutable(relockCtx, path, true, logger)
if lockErr == nil {
return
}
logDebug(logger, "Identity: writeIdentityFile: failed to restore immutable attribute: %v", lockErr)
if err == nil {
err = lockErr
}
}()
+ // Ensure file is writable even if immutable was previously set.
+ if err := writeIdentityFileWithContextSetImmutable(ctx, path, false, logger); err != nil {
+ 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 := writeIdentityFileWithContextSetImmutable(ctx, path, false, logger); err != nil { | |
| return err | |
| } | |
| defer func() { | |
| lockErr := writeIdentityFileWithContextSetImmutable(context.Background(), path, true, logger) | |
| if lockErr == nil { | |
| return | |
| } | |
| logDebug(logger, "Identity: writeIdentityFile: failed to restore immutable attribute: %v", lockErr) | |
| if err == nil { | |
| err = lockErr | |
| } | |
| }() | |
| defer func() { | |
| relockCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| lockErr := writeIdentityFileWithContextSetImmutable(relockCtx, path, true, logger) | |
| if lockErr == nil { | |
| return | |
| } | |
| logDebug(logger, "Identity: writeIdentityFile: failed to restore immutable attribute: %v", lockErr) | |
| if err == nil { | |
| err = lockErr | |
| } | |
| }() | |
| // Ensure file is writable even if immutable was previously set. | |
| if err := writeIdentityFileWithContextSetImmutable(ctx, path, false, logger); err != nil { | |
| return err | |
| } |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/storage/backup_files.go (1)
36-60:⚠️ Potential issue | 🔴 CriticalPrevent unsafe relative cleanup candidates when normalized base is empty.
At Line 36, inputs like
.bundle.tarnormalize to""; then Lines 56-60 generate.sha256/.metadatapaths, and Line 51 can yield.bundle.tar. That can make cleanup operate on unintended relative files.Proposed fix
func buildBackupCandidatePaths(base string, includeBundle bool) []string { base = normalizeBundleBasePath(base) + if base == "" { + return []string{} + } seen := make(map[string]struct{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/backup_files.go` around lines 36 - 60, The issue is that when the normalized base path is empty, the cleanup candidates generated include relative paths like ".sha256" or ".metadata" which can unintentionally affect files in the current directory. To fix this, modify the function that uses normalizeBundleBasePath by adding a guard clause after normalization to check if the base is empty. If it is empty, return early or skip generating the candidate cleanup paths to prevent adding unsafe relative cleanup targets, specifically in the code block that builds the candidates slice and appends paths to the files slice.internal/storage/cloud.go (1)
1768-1773:⚠️ Potential issue | 🟠 MajorPreserve context cancellation in the no-op retry waiter.
At Line 1772, unconditional
nilcan hidecontext.Canceled/context.DeadlineExceededwhen cancellation happens during retry backoff, causing stale operational errors to be returned instead.Proposed fix
func (c *CloudStorage) callWaitForRetry(ctx context.Context, d time.Duration) error { if c.waitForRetry != nil { return c.waitForRetry(ctx, d) } - return nil + if ctx == nil { + return nil + } + select { + case <-ctx.Done(): + return ctx.Err() + default: + return nil + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/cloud.go` around lines 1768 - 1773, The no-op path in callWaitForRetry currently returns nil unconditionally which can mask context cancellation; update callWaitForRetry (and/or the helper behavior when waitForRetry is nil) to still respect ctx by waiting for either the duration d or ctx.Done and returning ctx.Err() if the context is canceled or its deadline exceeded before the sleep finishes; keep the existing behavior of delegating to c.waitForRetry when non-nil, but when c.waitForRetry is nil implement a cancellable wait (e.g., timer + select on ctx.Done) so context.Canceled / context.DeadlineExceeded are propagated instead of dropped.internal/config/env_mutation.go (1)
16-25:⚠️ Potential issue | 🟠 MajorEscape CR/LF before picking a quote style.
Line 16 still sends values containing
\ror\nthrough the single-/double-quote branches. That writes literal line breaks into the env template, so one value can turn into multiple assignments and corruptbackup.env.💡 Proposed fix
func encodeEnvValue(value string) string { value = strings.TrimSpace(value) if value == "" { return "" } + + if strings.ContainsAny(value, "\r\n") { + return strconv.Quote(value) + } - if !strings.ContainsAny(value, "# \t\r\n\"'") { + if !strings.ContainsAny(value, "# \t\"'") { return value } if !strings.Contains(value, "'") { return "'" + value + "'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation.go` around lines 16 - 25, The current branch selection for quoting uses single/double quotes even when value contains CR/LF which causes literal line breaks in the generated env; update the logic so you first detect any '\r' or '\n' in the value and, if present, return strconv.Quote(value) (or otherwise escape/newline-encode) before checking single-quote or double-quote suitability; reference the same value variable and strconv.Quote used at the end of the snippet so the CR/LF case is handled early and never falls into the "'" or `"` branches.
🧹 Nitpick comments (9)
internal/config/migration_test.go (1)
187-190: Avoid package-init panic for test fixture setup.Because
invalidPermissionsTemplateis built at package scope (Line 278), a missing key panic (Line 189) crashes the entire test package before tests execute. Prefer returning an error and asserting inside the test body.Proposed refactor
-func overrideTemplateValues(template string, overrides map[string]string) string { +func overrideTemplateValues(template string, overrides map[string]string) (string, error) { lines := strings.Split(template, "\n") applied := make(map[string]bool, len(overrides)) @@ for key := range overrides { if !applied[key] { - panic("template key not found: " + key) + return "", fmt.Errorf("template key not found: %s", key) } } - return strings.Join(lines, "\n") + return strings.Join(lines, "\n"), nil } -var invalidPermissionsTemplate = overrideTemplateValues(baseInstallTemplate, map[string]string{ - "SET_BACKUP_PERMISSIONS": "true", - "BACKUP_USER": "", - "BACKUP_GROUP": "", -}) +func invalidPermissionsTemplate(t *testing.T) string { + t.Helper() + got, err := overrideTemplateValues(baseInstallTemplate, map[string]string{ + "SET_BACKUP_PERMISSIONS": "true", + "BACKUP_USER": "", + "BACKUP_GROUP": "", + }) + if err != nil { + t.Fatalf("failed to build invalid permissions template: %v", err) + } + return got +}Also applies to: 278-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/migration_test.go` around lines 187 - 190, The package-init panic comes from validating template keys at package scope (the loop over overrides checking applied and panicking), which crashes the test package; refactor by removing package-scope construction of invalidPermissionsTemplate and instead expose a constructor function (e.g., NewInvalidPermissionsTemplate or BuildInvalidPermissionsTemplate) that performs the same validation but returns (template, error) rather than calling panic; update tests to call that constructor during setup and assert on the returned error (or fatal within the test) so failures are reported as test errors rather than crashing package init, and replace any direct references to the package-scope invalidPermissionsTemplate with calls to the new constructor.internal/storage/backup_files_test.go (1)
23-46: Add a regression case for empty normalized base.Please add a table entry where
baseis.bundle.tar(or repeated bundle suffix only) and assert no candidates are returned. This protects against accidental generation of relative cleanup targets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/backup_files_test.go` around lines 23 - 46, Add a test table entry to cover the regression where the normalized base is empty: add an item with name like "empty normalized base", base set to ".bundle.tar" (or repeated bundle suffixes), includeBundle set as appropriate (true or false) and want set to an empty slice. This entry should be added to the same test cases array (the table-driven test using fields name, base, includeBundle, want) in internal/storage/backup_files_test.go so the test asserts no candidates are returned for a base that normalizes to an empty filename.internal/orchestrator/restore_tui.go (1)
868-1026: Consider applying the same synchronization pattern topromptNetworkCommitTUI.The
promptNetworkCommitTUIfunction (lines 987-1020) uses a similar countdown goroutine but with the older pattern that doesn't wait for goroutine termination synchronously. For consistency and robustness, consider refactoring it to use the samestopTickerclosure pattern with adonechannel.🤖 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 868 - 1026, promptNetworkCommitTUI uses an older countdown goroutine pattern (stopCh, done, ticker) that doesn't synchronously mirror the newer stopTicker closure pattern; update promptNetworkCommitTUI so the countdown goroutine follows the same pattern used elsewhere: replace the ad-hoc stopCh/done handling with a stopTicker closure that closes the stop channel, stops the ticker and waits for the done channel, keep the same semantics for remaining/timedOut/committed/cancelled, and update call sites in this function to invoke stopTicker instead of manually closing stopCh and stopping ticker; key symbols to change are promptNetworkCommitTUI, stopCh, done, ticker and the anonymous goroutine that reads from ticker.C.internal/orchestrator/decrypt_workflow_ui_test.go (1)
338-340: Consider asserting the exact error message for consistency.Other typed-nil tests in this file verify the specific error message (e.g., lines 287-289, 300-302, 367-369). This test only checks that an error is returned without validating the message is
"decrypt workflow UI not available".♻️ Suggested improvement
- if _, err := preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil); err == nil { + _, err := preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil) + if err == nil { t.Fatal("expected error for typed-nil UI") } + if got, want := err.Error(), "decrypt workflow UI not available"; got != want { + t.Fatalf("error=%q, want %q", got, want) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_workflow_ui_test.go` around lines 338 - 340, The test currently only checks that preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil) returns a non-nil error; update the assertion to verify the exact error message is "decrypt workflow UI not available" for consistency with the other typed-nil tests—locate the call to preparePlainBundleWithUI in decrypt_workflow_ui_test.go and replace the generic nil-check with an assertion that err is non-nil and err.Error() (or the error value) equals the expected string.internal/orchestrator/decrypt_tui_e2e_helpers_test.go (1)
158-163: TheWaitduration value is now only used as a boolean flag.The
Waitfield's actual duration is no longer used—only whether it's greater than zero matters. The wait loop runs until the screen state changes, ignoring the duration value. This makes the field name potentially misleading.Consider either:
- Renaming
Waitto something likeWaitForChange bool, or- Using the duration as a timeout for the
waitForScreenTextloop to prevent indefinite hangs if the expected text never appears.♻️ Optional: Add timeout using the Wait duration
- waitForScreenText := func(expected string) bool { + waitForScreenText := func(expected string, timeout time.Duration) bool { expected = strings.TrimSpace(expected) + deadline := time.Now().Add(timeout) for { current := currentScreenState() if current.signature != "" { if (expected == "" || strings.Contains(current.text, expected)) && (lastInjectedState == "" || current.signature != lastInjectedState) { return true } } select { case <-done: return false + case <-time.After(time.Until(deadline)): + return false case <-screenStateCh: } } }Then update the call site:
- if !waitForScreenText(k.WaitForText) { + if !waitForScreenText(k.WaitForText, k.Wait) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go` around lines 158 - 163, The test uses the Keys[*].Wait field only as a boolean (checking if >0) while calling waitForScreenText(k.WaitForText), so either rename Wait to a boolean like WaitForChange on the keys struct and update all call sites to check k.WaitForChange, or make Wait an actual timeout by passing it into waitForScreenText and updating waitForScreenText to accept a timeout (use k.Wait as the maximum wait duration and return false on timeout) so the loop cannot hang indefinitely; update references to keys, Wait, waitForScreenText, and the keys iteration accordingly.internal/orchestrator/restore_ha_additional_test.go (1)
666-673: Consider extracting duplicatednewEnvhelper.This helper is nearly identical to the one at lines 387-395. If additional test functions need the same setup, consider extracting to a package-level helper like
setupHATestEnvWithStagedConfig(t *testing.T) *haTestEnvto reduce duplication. That said, keeping it local to each test function is also a reasonable choice for test code locality.♻️ Optional: Extract shared helper
// At package level, alongside setupHATestEnv func setupHATestEnvWithStagedConfig(t *testing.T) *haTestEnv { t.Helper() env := setupHATestEnv(t) if err := env.fs.AddFile(env.stageRoot+"/etc/pve/ha/resources.cfg", []byte("res\n")); err != nil { t.Fatalf("add staged HA config: %v", err) } return env }Then both test functions can simply call
env := setupHATestEnvWithStagedConfig(t)in each subtest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_ha_additional_test.go` around lines 666 - 673, Duplicate local helper newEnv duplicates setup logic; extract it into a package-level helper function (e.g. setupHATestEnvWithStagedConfig(t *testing.T) *haTestEnv) that calls setupHATestEnv(t), adds the staged config via env.fs.AddFile(env.stageRoot+"/etc/pve/ha/resources.cfg", []byte("res\n")) and t.Fatalf on error, then replace the local newEnv closures in tests with calls to setupHATestEnvWithStagedConfig to remove duplication while keeping haTestEnv and stageRoot usage intact.cmd/proxsave/install_existing_config.go (2)
33-38:os.Staton Line 33 is still not cancellation-aware.You currently observe cancellation only after
os.Statreturns. If you want strict context propagation here too, consider switching this check to a context-aware filesystem helper (for example,internal/safefs.Stat) so cancellation can short-circuit the stat path itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 33 - 38, The code calls os.Stat(configPath) which blocks without observing ctx cancellation; replace this with a context-aware stat helper (e.g., internal/safefs.Stat) so the filesystem operation can be cancelled, and keep the existing cancellation check using ctx.Err() and the existing return path (existingConfigCancel, err) unchanged; update the call site in install_existing_config.go where os.Stat and ctx.Err() are used to call internal/safefs.Stat(ctx, configPath) (or the project’s equivalent) so the stat is cancellation-aware.
63-80: Consolidate repeatedctx.Err()checks to reduce drift risk.The same guard is duplicated in each branch; a small helper keeps behavior consistent and easier to maintain.
♻️ Suggested simplification
+ checkCanceled := func() error { + if err := ctx.Err(); err != nil { + return err + } + return nil + } + for { choice, err := promptOptional(ctx, reader, "Choice [3]: ") if err != nil { return existingConfigCancel, err } switch strings.TrimSpace(choice) { case "": fallthrough case "3": - if err := ctx.Err(); err != nil { + if err := checkCanceled(); err != nil { return existingConfigCancel, err } return existingConfigKeepContinue, nil case "1": - if err := ctx.Err(); err != nil { + if err := checkCanceled(); err != nil { return existingConfigCancel, err } return existingConfigOverwrite, nil case "2": - if err := ctx.Err(); err != nil { + if err := checkCanceled(); err != nil { return existingConfigCancel, err } return existingConfigEdit, nil case "0": - if err := ctx.Err(); err != nil { + if err := checkCanceled(); err != nil { return existingConfigCancel, err } return existingConfigCancel, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 63 - 80, Consolidate the repeated ctx.Err() guard by adding a small helper (e.g., ensureCtxActive(ctx context.Context) error or a local closure checkCtx := func() error { return ctx.Err() }) and replace the repeated blocks inside the switch that currently perform "if err := ctx.Err(); err != nil { return existingConfigCancel, err }" with a single call to that helper; keep the same return behavior using the existingConfigCancel sentinel on error and still return the appropriate branch values (existingConfigKeepContinue, existingConfigOverwrite, existingConfigEdit) when the helper returns nil so you maintain identical control flow.internal/config/env_mutation_test.go (1)
16-30: Keep the test parser aligned withSetEnvValue.
parseMutatedEnvTemplatedoes not understandexport KEY=...or inline comments, whilepkg/utils/strings.go'sSetEnvValuepreserves both. That makes these assertions brittle: a fixture using either format would fail even if the mutation logic is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation_test.go` around lines 16 - 30, The parser parseMutatedEnvTemplate is too strict: it doesn't accept lines starting with "export " and it strips inline comments, while pkg/utils/strings.go's SetEnvValue preserves both. Update parseMutatedEnvTemplate to first trim the line, strip a leading "export " prefix if present, then split using strings.SplitN(trimmed, "=", 2) (not the original untrimmed line) and treat the RHS as the value including any inline comments (do not remove text after '#'); keep incrementing counts[key] and storing values[key] as the full RHS. This ensures parseMutatedEnvTemplate accepts "export KEY=val" and preserves inline comments the same way SetEnvValue does.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/config/env_mutation.go`:
- Around line 16-25: The current branch selection for quoting uses single/double
quotes even when value contains CR/LF which causes literal line breaks in the
generated env; update the logic so you first detect any '\r' or '\n' in the
value and, if present, return strconv.Quote(value) (or otherwise
escape/newline-encode) before checking single-quote or double-quote suitability;
reference the same value variable and strconv.Quote used at the end of the
snippet so the CR/LF case is handled early and never falls into the "'" or `"`
branches.
In `@internal/storage/backup_files.go`:
- Around line 36-60: The issue is that when the normalized base path is empty,
the cleanup candidates generated include relative paths like ".sha256" or
".metadata" which can unintentionally affect files in the current directory. To
fix this, modify the function that uses normalizeBundleBasePath by adding a
guard clause after normalization to check if the base is empty. If it is empty,
return early or skip generating the candidate cleanup paths to prevent adding
unsafe relative cleanup targets, specifically in the code block that builds the
candidates slice and appends paths to the files slice.
In `@internal/storage/cloud.go`:
- Around line 1768-1773: The no-op path in callWaitForRetry currently returns
nil unconditionally which can mask context cancellation; update callWaitForRetry
(and/or the helper behavior when waitForRetry is nil) to still respect ctx by
waiting for either the duration d or ctx.Done and returning ctx.Err() if the
context is canceled or its deadline exceeded before the sleep finishes; keep the
existing behavior of delegating to c.waitForRetry when non-nil, but when
c.waitForRetry is nil implement a cancellable wait (e.g., timer + select on
ctx.Done) so context.Canceled / context.DeadlineExceeded are propagated instead
of dropped.
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 33-38: The code calls os.Stat(configPath) which blocks without
observing ctx cancellation; replace this with a context-aware stat helper (e.g.,
internal/safefs.Stat) so the filesystem operation can be cancelled, and keep the
existing cancellation check using ctx.Err() and the existing return path
(existingConfigCancel, err) unchanged; update the call site in
install_existing_config.go where os.Stat and ctx.Err() are used to call
internal/safefs.Stat(ctx, configPath) (or the project’s equivalent) so the stat
is cancellation-aware.
- Around line 63-80: Consolidate the repeated ctx.Err() guard by adding a small
helper (e.g., ensureCtxActive(ctx context.Context) error or a local closure
checkCtx := func() error { return ctx.Err() }) and replace the repeated blocks
inside the switch that currently perform "if err := ctx.Err(); err != nil {
return existingConfigCancel, err }" with a single call to that helper; keep the
same return behavior using the existingConfigCancel sentinel on error and still
return the appropriate branch values (existingConfigKeepContinue,
existingConfigOverwrite, existingConfigEdit) when the helper returns nil so you
maintain identical control flow.
In `@internal/config/env_mutation_test.go`:
- Around line 16-30: The parser parseMutatedEnvTemplate is too strict: it
doesn't accept lines starting with "export " and it strips inline comments,
while pkg/utils/strings.go's SetEnvValue preserves both. Update
parseMutatedEnvTemplate to first trim the line, strip a leading "export " prefix
if present, then split using strings.SplitN(trimmed, "=", 2) (not the original
untrimmed line) and treat the RHS as the value including any inline comments (do
not remove text after '#'); keep incrementing counts[key] and storing
values[key] as the full RHS. This ensures parseMutatedEnvTemplate accepts
"export KEY=val" and preserves inline comments the same way SetEnvValue does.
In `@internal/config/migration_test.go`:
- Around line 187-190: The package-init panic comes from validating template
keys at package scope (the loop over overrides checking applied and panicking),
which crashes the test package; refactor by removing package-scope construction
of invalidPermissionsTemplate and instead expose a constructor function (e.g.,
NewInvalidPermissionsTemplate or BuildInvalidPermissionsTemplate) that performs
the same validation but returns (template, error) rather than calling panic;
update tests to call that constructor during setup and assert on the returned
error (or fatal within the test) so failures are reported as test errors rather
than crashing package init, and replace any direct references to the
package-scope invalidPermissionsTemplate with calls to the new constructor.
In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go`:
- Around line 158-163: The test uses the Keys[*].Wait field only as a boolean
(checking if >0) while calling waitForScreenText(k.WaitForText), so either
rename Wait to a boolean like WaitForChange on the keys struct and update all
call sites to check k.WaitForChange, or make Wait an actual timeout by passing
it into waitForScreenText and updating waitForScreenText to accept a timeout
(use k.Wait as the maximum wait duration and return false on timeout) so the
loop cannot hang indefinitely; update references to keys, Wait,
waitForScreenText, and the keys iteration accordingly.
In `@internal/orchestrator/decrypt_workflow_ui_test.go`:
- Around line 338-340: The test currently only checks that
preparePlainBundleWithUI(context.Background(), cand, "1.0.0", logger, typedNil)
returns a non-nil error; update the assertion to verify the exact error message
is "decrypt workflow UI not available" for consistency with the other typed-nil
tests—locate the call to preparePlainBundleWithUI in decrypt_workflow_ui_test.go
and replace the generic nil-check with an assertion that err is non-nil and
err.Error() (or the error value) equals the expected string.
In `@internal/orchestrator/restore_ha_additional_test.go`:
- Around line 666-673: Duplicate local helper newEnv duplicates setup logic;
extract it into a package-level helper function (e.g.
setupHATestEnvWithStagedConfig(t *testing.T) *haTestEnv) that calls
setupHATestEnv(t), adds the staged config via
env.fs.AddFile(env.stageRoot+"/etc/pve/ha/resources.cfg", []byte("res\n")) and
t.Fatalf on error, then replace the local newEnv closures in tests with calls to
setupHATestEnvWithStagedConfig to remove duplication while keeping haTestEnv and
stageRoot usage intact.
In `@internal/orchestrator/restore_tui.go`:
- Around line 868-1026: promptNetworkCommitTUI uses an older countdown goroutine
pattern (stopCh, done, ticker) that doesn't synchronously mirror the newer
stopTicker closure pattern; update promptNetworkCommitTUI so the countdown
goroutine follows the same pattern used elsewhere: replace the ad-hoc
stopCh/done handling with a stopTicker closure that closes the stop channel,
stops the ticker and waits for the done channel, keep the same semantics for
remaining/timedOut/committed/cancelled, and update call sites in this function
to invoke stopTicker instead of manually closing stopCh and stopping ticker; key
symbols to change are promptNetworkCommitTUI, stopCh, done, ticker and the
anonymous goroutine that reads from ticker.C.
In `@internal/storage/backup_files_test.go`:
- Around line 23-46: Add a test table entry to cover the regression where the
normalized base is empty: add an item with name like "empty normalized base",
base set to ".bundle.tar" (or repeated bundle suffixes), includeBundle set as
appropriate (true or false) and want set to an empty slice. This entry should be
added to the same test cases array (the table-driven test using fields name,
base, includeBundle, want) in internal/storage/backup_files_test.go so the test
asserts no candidates are returned for a base that normalizes to an empty
filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae73dd6e-5dca-4770-96e7-a5b435c95e4f
📒 Files selected for processing (24)
cmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/webhook_test.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/storage/backup_files.gointernal/storage/backup_files_test.gointernal/storage/cloud.gointernal/storage/cloud_test.go
💤 Files with no reviewable changes (1)
- cmd/proxsave/install.go
Owner
Author
|
@coderabbitai review all code of repo |
|
User feedback: Review all code in the repository, not just the changed files in this PR. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/config/env_mutation.go (1)
16-25:⚠️ Potential issue | 🟠 MajorNewline/CR injection vulnerability in quoted env values.
When a value contains
\ror\nbut no single quotes, the function returns a single-quoted string with literal embedded newlines (e.g.,'foo\nbar'). This breaks the env file format and could inject unintended entries.Values containing newlines should immediately use
strconv.Quoteto ensure proper escaping.🔒 Proposed fix
func encodeEnvValue(value string) string { value = strings.TrimSpace(value) if value == "" { return "" } + if strings.ContainsAny(value, "\r\n") { + return strconv.Quote(value) + } + - if !strings.ContainsAny(value, "# \t\r\n\"'") { + if !strings.ContainsAny(value, "# \t\"'") { return value } if !strings.Contains(value, "'") { return "'" + value + "'" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation.go` around lines 16 - 25, The current logic in the env quoting helper returns a single-quoted string when the value contains newlines/CR but no single quotes, which allows raw newlines into the env file; update the branching in the function that processes the variable "value" (the code using strings.ContainsAny and strconv.Quote) to immediately call strconv.Quote(value) if value contains '\n' or '\r' (i.e., detect newline/CR before falling back to single-quoting) so that values with newlines are always safely escaped.
🧹 Nitpick comments (3)
cmd/proxsave/install_existing_config.go (1)
77-81: Consider: case "0" returns context error instead of nil when both conditions apply.When the user explicitly chooses "0" (cancel installation) and the context is also canceled, this returns the context error rather than
nil. This subtly changes the error semantics—callers may interpret a non-nil error differently than a user-initiated cancellation.If intentional, this is fine. If you want to preserve user-initiated cancellation semantics, the ctx.Err() check could be skipped for case "0" since both paths lead to
existingConfigCancel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 77 - 81, The case "0" branch currently checks ctx.Err() and returns that error when the context is canceled, which makes a user-initiated cancel return a context error; change the case "0" handling in the function (the switch branch labeled case "0") to skip the ctx.Err() check and return existingConfigCancel, nil unconditionally so a user-chosen cancel yields the same nil error semantics as other cancel paths.internal/storage/backup_files_test.go (1)
22-45: Consider adding edge case coverage.The current tests verify normalization of valid paths. Consider adding test cases for edge conditions to increase robustness:
- Input with no
.bundle.tarsuffix (e.g.,"backup.tar.zst")- Input that normalizes to empty string (e.g.,
".bundle.tar")- Empty string input
These would help document expected behavior for boundary conditions and catch potential issues like the empty-base scenario flagged in the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/backup_files_test.go` around lines 22 - 45, Add edge-case table-driven tests to the existing test cases in backup_files_test.go (the table using fields name, base, includeBundle, want) to cover: a base without the ".bundle.tar" suffix like "backup.tar.zst", a base equal to ".bundle.tar" which may normalize to empty, and an empty base string ""; for each case set includeBundle true/false as appropriate and assert the expected slice returned from the function under test (the same test harness used by the existing cases) so the test documents and verifies behavior for these boundary inputs.internal/orchestrator/restore_ha_additional_test.go (1)
665-673: Consider extracting the duplicatednewEnvhelper.This helper is nearly identical to the one in
TestMaybeApplyPVEHAWithUI_BranchCoverage(lines 387-394). You could extract it to a package-level test helper to reduce duplication:func newHATestEnvWithStagedConfig(t *testing.T) *haTestEnv { t.Helper() env := setupHATestEnv(t) if err := env.fs.AddFile(env.stageRoot+"/etc/pve/ha/resources.cfg", []byte("res\n")); err != nil { t.Fatalf("add staged HA config: %v", err) } return env }That said, keeping the helpers local is also reasonable for test readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_ha_additional_test.go` around lines 665 - 673, The test defines a duplicated helper newEnv in TestMaybeApplyPVEHAWithUI_CommitOutcomes that matches the one in TestMaybeApplyPVEHAWithUI_BranchCoverage; extract it to a package-level helper (e.g., newHATestEnvWithStagedConfig) that calls setupHATestEnv, adds the staged HA config via env.fs.AddFile(env.stageRoot+"/etc/pve/ha/resources.cfg", []byte("res\n")), calls t.Helper(), and returns *haTestEnv, then replace the local newEnv closures in both tests with calls to the new helper to remove duplication while keeping behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/config/env_mutation.go`:
- Around line 16-25: The current logic in the env quoting helper returns a
single-quoted string when the value contains newlines/CR but no single quotes,
which allows raw newlines into the env file; update the branching in the
function that processes the variable "value" (the code using strings.ContainsAny
and strconv.Quote) to immediately call strconv.Quote(value) if value contains
'\n' or '\r' (i.e., detect newline/CR before falling back to single-quoting) so
that values with newlines are always safely escaped.
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 77-81: The case "0" branch currently checks ctx.Err() and returns
that error when the context is canceled, which makes a user-initiated cancel
return a context error; change the case "0" handling in the function (the switch
branch labeled case "0") to skip the ctx.Err() check and return
existingConfigCancel, nil unconditionally so a user-chosen cancel yields the
same nil error semantics as other cancel paths.
In `@internal/orchestrator/restore_ha_additional_test.go`:
- Around line 665-673: The test defines a duplicated helper newEnv in
TestMaybeApplyPVEHAWithUI_CommitOutcomes that matches the one in
TestMaybeApplyPVEHAWithUI_BranchCoverage; extract it to a package-level helper
(e.g., newHATestEnvWithStagedConfig) that calls setupHATestEnv, adds the staged
HA config via env.fs.AddFile(env.stageRoot+"/etc/pve/ha/resources.cfg",
[]byte("res\n")), calls t.Helper(), and returns *haTestEnv, then replace the
local newEnv closures in both tests with calls to the new helper to remove
duplication while keeping behavior identical.
In `@internal/storage/backup_files_test.go`:
- Around line 22-45: Add edge-case table-driven tests to the existing test cases
in backup_files_test.go (the table using fields name, base, includeBundle, want)
to cover: a base without the ".bundle.tar" suffix like "backup.tar.zst", a base
equal to ".bundle.tar" which may normalize to empty, and an empty base string
""; for each case set includeBundle true/false as appropriate and assert the
expected slice returned from the function under test (the same test harness used
by the existing cases) so the test documents and verifies behavior for these
boundary inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a9e6c21-3f80-4ff2-80e0-6610624c5bba
📒 Files selected for processing (24)
cmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/webhook_test.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/storage/backup_files.gointernal/storage/backup_files_test.gointernal/storage/cloud.gointernal/storage/cloud_test.go
💤 Files with no reviewable changes (1)
- cmd/proxsave/install.go
The HTTP 200 handler in sendViaCloudRelay() returned success without
parsing the response body, ignoring the "success" field. If the worker
responded with {"success": false, "error": "..."}, the client silently
treated it as a successful delivery, preventing the PMF fallback from
triggering and causing emails to be lost.
Parse the response body on HTTP 200 and check the success field. When
success=false, return an error so the existing fallback chain activates.
Empty or non-JSON bodies on 200 are still treated as success for backward
compatibility.
Add diagnostic logging of the raw response body for all HTTP status codes
to aid future relay troubleshooting.
Differentiate relay 401/403 failure causes end-to-end by adding explicit worker error codes and client-side classification fallback for legacy responses. Add worker-side trusted IP bypass for the per-MAC daily quota and document relay acceptance semantics, common auth/HMAC errors, and MAC_LIMIT_IP_WHITELIST usage.
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
Improvements
Documentation
Tests