Conversation
Centralize validation for SECONDARY_PATH and SECONDARY_LOG_PATH so all entrypoints enforce the same absolute-local-path rules. Reject remote/UNC-style secondary paths during config loading, keep SECONDARY_LOG_PATH optional, and update the CLI installer to retry on invalid secondary path input instead of aborting. Add coverage for config parsing, migration, installer, runtime validation, and TUI flows.
Refactor new-install to use a shared reset plan and a single source of truth for preserved entries (build/env/identity). Route --new-install --cli through CLI confirmation only, keep TUI confirmation as a pure adapter, and propagate TUI runner errors instead of swallowing them. Update related help/log messaging and add tests for new-install planning, CLI confirm behavior, TUI confirm rendering/error handling, and reset/preserve consistency.
Introduce a shared decision flow for pre-existing backup.env with four explicit actions: Overwrite, Edit existing, Keep existing & continue, and Cancel. Update CLI prompts to support all modes (including Edit existing and explicit Cancel), update TUI action mapping to the same semantics, and treat “keep existing” as continue (not abort). Ensure TUI post-config steps are skipped consistently when configuration wizard is skipped (AGE setup, post-install audit, Telegram pairing), while finalization steps still run. Propagate CheckExistingConfig runner errors instead of swallowing them. Add/adjust unit tests for decision resolution, CLI prompts, TUI actions, runner error propagation, and prepareBaseTemplate behavior. Update INSTALL and CLI_REFERENCE docs to match the new aligned behavior.
Tighten AGE setup consistency after the shared CLI/TUI refactor. Reuse a shared private-key validator so the TUI rejects malformed AGE identities before they reach the orchestrator, eliminating silent retry loops. Extend the AGE setup workflow to return explicit outcome details (recipient path, wrote file vs reused existing recipients) and update install TUI messaging to report “saved” only on real writes, while showing reuse clearly when existing recipient configuration is kept. Add regression coverage for private-key validation, reuse-vs-write setup results, and the updated TUI wizard behavior.
Introduce a shared Telegram setup bootstrap so CLI and TUI use the same eligibility rules before showing pairing steps. Stop the TUI from falling back to raw backup.env parsing, skip Telegram setup consistently when config loading fails, personal mode is selected, or no Server ID is available, and centralize skip-reason logging in the command layer. Update the TUI install flow to log shared Telegram bootstrap outcomes, add dedicated tests for bootstrap/CLI/TUI behavior, align user-facing docs, and remove now-unreachable TUI branches left over from the old local decision logic.
Restore consistent decrypt prompt behavior between CLI and TUI by treating "0" as an explicit abort in both flows. Update the TUI decrypt secret prompt to advertise the exit semantics clearly, return ErrDecryptAborted on zero input, and keep Cancel as an equivalent exit path. Adjust TUI simulation coverage so the shared decrypt workflow no longer carries a UI-specific semantic drift on secret entry.
Add deterministic end-to-end smoke tests for RunDecryptWorkflowTUI so the real decrypt TUI production path is covered from entrypoint through source selection, candidate selection, secret prompt, destination prompt, and final bundle creation. Introduce test-only helpers for a real AGE-encrypted raw-backup fixture, serialized TUI simulation across multi-screen workflows, bundle-content inspection, and guarded workflow execution. Verify both the success path (including final *.decrypted.bundle.tar contents, metadata, and checksum) and clean abort at the decrypt secret prompt, without changing production behavior.
Introduce a shared env-template helper for secondary storage state and use it from both installer flows so disabling secondary storage always writes the same canonical config: SECONDARY_ENABLED=false, SECONDARY_PATH=, and SECONDARY_LOG_PATH=. This removes the previous TUI-only drift where editing an existing backup.env could leave stale secondary paths after the user disabled the feature. Add focused unit coverage for the shared helper plus CLI and TUI regression tests covering disabled state and clearing of pre-existing secondary values, and clarify the installer docs to note that disabling secondary storage clears the saved secondary paths.
Introduce shared cron parsing/normalization for install workflows and align CLI with the existing TUI cron capability. Add a neutral internal cron helper package, collect cron time during the CLI install wizard, propagate an explicit CronSchedule through the CLI install result, and make install-time cron finalization honor wizard-selected/default cron values instead of falling back to env overrides after a normal wizard run. Keep skip-config-wizard and upgrade flows on their existing env/default behavior, update the TUI wizard to reuse the same cron validation logic, add regression coverage for shared cron parsing, CLI prompt/result propagation, and install schedule precedence, and update install/CLI docs to reflect cron selection in both modes.
Close the remaining cron-install test gaps after aligning CLI and TUI scheduling behavior. Add a TUI wizard regression test that proves blank cron input resolves to the installer default (02:00) even when CRON_SCHEDULE is set in the environment, and add a CLI wizard regression test that aborting exactly at the cron prompt propagates the interactive abort and leaves backup.env unwritten. Introduce minimal test seams for the install wizard runner and cron prompt boundary to exercise the real command/wizard paths without changing production semantics.
Reduces flakiness in the decrypt TUI end-to-end tests when run with coverage enabled or under package-level load. - increases simulated input delays - extends end-to-end test timeouts and contexts - avoids false negatives without changing production code
Avoid nil-pointer panics in runInstallTUI when the bootstrap logger is not provided. Guard the AGE encryption success-path Info logs and the configuration-saved Debug log with bootstrap nil checks, preserving existing behavior when bootstrap is available.
Prevent nil-pointer panics in the newkey flow by routing final success messages through a shared helper. When a bootstrap logger is available, keep using bootstrap.Info; otherwise fall back to stdout so both CLI and TUI paths remain safe and user-visible. Also add targeted tests for bootstrap and nil-bootstrap cases.
Prevent decrypt path conflict prompts from accepting the same destination path again. Add shared validation that rejects empty or normalized-equivalent paths to the existing target, apply it in both TUI and CLI flows, and update tests to cover valid edits plus normalized-path rejection and retry behavior.
Replace the single-case select in printNetworkRollbackCountdown with a direct receive from ticker.C. The loop behavior remains unchanged: the countdown still updates on each tick and keeps the explicit continue for clarity.
Load the existing config in --newkey so AGE_RECIPIENT_FILE is preserved when configured, instead of always forcing the default recipient path. Also update the success message to report the effective recipient file, add tests for custom/default paths and invalid configs, and align the CLI docs.
Add targeted tests for the TUI wizards to cover the remaining uncovered branches. - cover nil and whitespace-only preserved entries in new install tests - cover Telegram setup bootstrap error propagation - verify persisted identity rendering in the Server ID panel - verify truncation of long registration failure messages - verify ESC behavior after successful verification - exercise default async wrappers used by the Telegram setup wizard LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add missing unit tests for new install and Telegram setup flows, covering edge cases, UI state rendering, error propagation, ESC handling, and async helpers. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Refactor telegram_setup_tui_test to remove hardcoded layout index assumptions when extracting Telegram setup views. The helper now finds the Pages container and the relevant widgets by type and title instead of relying on UI item ordering. The default wrapper test also replaces sleep-based update timing with direct QueueUpdateDraw synchronization, keeping only a watchdog timeout to avoid hangs. LiveReview Pre-Commit Check: skipped (iter:3, coverage:0%)
Fix the used space calculation in the local and secondary storage backends by using Blocks-Bfree instead of Total-Bavail, so reserved blocks are no longer reported as "used". Centralize statfs space conversion in a shared helper and apply the same semantics to the PVE collector. Also propagate UsedSpace through the orchestrator and notifications, removing the recomputation from free/total so displayed usage values and percentages match actual filesystem usage. Update and strengthen tests for Bavail != Bfree cases and for end-to-end propagation of the corrected values. LiveReview Pre-Commit Check: skipped (iter:2, coverage:73%)
…aths Prevent duplicate Close() calls in createBundle() and extractRegularFile() while preserving explicit close error handling. The patch replaces bare deferred closes with guarded defers so resources are only closed if they were not already closed explicitly, and it keeps the explicit Close() calls where finalization order and returned errors still matter. Test coverage was also improved to verify that output files are correctly closed in both success and error paths for bundle creation and regular file extraction. LiveReview Pre-Commit Check: skipped (iter:2, coverage:100%)
Prevent data loss when replacing the AGE recipient file by changing the backup logic to copy the existing file into a .bak-* backup instead of renaming or deleting the active file. The new recipient file is now written atomically through a temporary file and rename, and the workflow creates the backup only at commit time, after the new recipients have been collected. Backup failures are now propagated instead of being logged and ignored, so aborts and write errors leave the current recipient file intact. Regression tests were added to cover abort, backup failure, write failure, and successful overwrite paths. LiveReview Pre-Commit Check: skipped (iter:3, coverage:79%)
extractRegularFile now writes regular-file restores to a sibling temp file and renames it into place only after content copy and close succeed. This prevents truncated archive entries from clobbering an existing target or leaving partial files behind on restore failure. Also improves temp-file handling by reducing name collision risk with an atomic sequence, surfacing deferred close errors, and logging unexpected temp cleanup failures. Tests were added/updated to verify success cleanup, failed-copy cleanup, and preservation of a pre-existing target on copy errors.` LiveReview Pre-Commit Check: skipped (iter:2, coverage:48%)
Align the notification configuration with the shipped template and documentation by making EMAIL_ENABLED default to false when unset. Restore compatibility with legacy *_ENABLE notification flags during runtime loading and legacy env migration, add the missing Gotify environment overrides, and update tests and docs to reflect the corrected behavior. LiveReview Pre-Commit Check: skipped (iter:2, coverage:67%)
Replace the shift-based exponential backoff used by cloud remote checks and upload retries with a bounded retry schedule. This keeps the current retry timings for normal cases (2s, 4s, 8s, 16s) while capping larger attempts at 30s and removing the overflow-prone `1<<attempt` pattern. Also add regression coverage for large attempts and for the actual sleep durations used by cloud retry paths. LiveReview Pre-Commit Check: skipped (iter:2, coverage:83%)
Use os.Lstat in verifyBinaryIntegrity to validate the executable path before opening it, so symlinked executables are rejected correctly. Open the file only when computing the checksum, removing the unused rewind path and making the integrity check flow clearer. Update the related tests to cover the symlink case and the stat failure path. LiveReview Pre-Commit Check: skipped (iter:2, coverage:6%)
Ensure GFS retention consistently normalizes RETENTION_DAILY<=0 to an effective daily minimum of 1 across classification, storage backends, and runtime summaries. This prevents current-period backups from being misclassified or deleted when daily retention is unset/zero, aligns internal comments and tests with the documented contract, and separates pure GFS config normalization from the logging wrapper used in operational paths. LiveReview Pre-Commit Check: skipped (iter:2, coverage:67%)
Propagate context-aware cancellation through notification retry and diagnostic flows. Switch email mail-log inspection helpers to use context and CommandContext, replace blocking sleeps with a context-aware wait helper, and stop relay/webhook retry loops immediately when the context is canceled. Add focused tests covering cancellation behavior. LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
Normalize bundle path handling when BundleAssociatedFiles is enabled so storage backends accept either the raw archive path or an already bundled path without deriving *.bundle.tar.bundle.tar. Reuse a shared helper for canonical bundle naming, add regression tests for double-suffix decoys, and log unexpected bundle stat errors while still ignoring missing optional bundle files. LiveReview Pre-Commit Check: skipped (iter:2, coverage:85%)
Use file-handle metadata operations during restore extraction so FakeFS-backed tests and non-host paths do not fail on chmod/chown. Normalize FakeFS temp-path handling for already-resolved sandbox paths, add focused regression coverage for full and selective archive extraction, and keep the restore workflow tests green for fallback, SAFE cluster export, PBS staged restore, and fstab abort flows. LiveReview Pre-Commit Check: skipped (iter:2, coverage:49%)
Remove the no-op continue after <-ticker.C in cmd/proxsave/main.go. This does not change behavior; it only simplifies the loop control flow. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Extend primitiveContainsText to inspect more tview primitives and add fallback coverage so text assertions work across boxes, forms, modals, and nested containers. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Synchronize RunWithContext tests on actual app startup and assert they stay blocked until Stop is called, preventing false positives from early returns. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Make captureCLIStdout defer pipe shutdown and goroutine cleanup so stdout capture is restored even if the callback panics or aborts the test early. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add t.Parallel() to the secondary path validation subtests so the table-driven cases execute concurrently without changing assertions. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
… flow Drop the redundant filepath.Clean call from the new-path prompt and keep normalization in the caller so the destination path is cleaned exactly once. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Extract the duplicated age setup test UI into a reusable helper under internal/testutil and reuse it in both cmd/proxsave and internal/orchestrator tests. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Delete the dead detectTelegramCode helper now that install flow calls detectTelegramCodeWithContext directly, resolving the staticcheck U1000 failure. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Strip terminal escape sequences and control characters from Telegram registration messages before printing them, with a safe quoted fallback for untrusted server text. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Return upgrade write failures from maybeUpgradeIdentityFileWithContext and surface them from DetectWithContext so context cancellation and timeouts abort identity detection instead of being swallowed. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Replace shared retry attempt counters in email relay tests with sync/atomic operations to remove handler/test goroutine races. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Route mapAgeSetupAbort through mapInputAbortToAgeAbort so context cancellations and input-abort signals from TUI prompts are normalized to ErrAgeRecipientSetupAborted like the CLI path. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Move the decrypt TUI e2e helper cleanup right after locking and saving newTUIApp so the mutex is always unlocked and the app factory is restored even if screen initialization fails early. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Include nanosecond resolution in AGE recipient backup filenames so concurrent operations in the same second do not reuse the same .bak path. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Log inconsistent storage usage stats when used bytes are reported but total capacity is zero, while keeping displayed usage at 0% for unknown totals. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Synchronize the network-apply TUI error path with the countdown goroutine teardown by waiting on the done channel after stopping the ticker and closing stopCh. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Create restore temp files with CreateTemp in the target directory instead of deriving names from the final basename, avoiding ENAMETOOLONG collisions while preserving atomic rename semantics. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Create restored directories with a temporary owner-accessible mode, open them first, and only then apply the archived permissions so restrictive modes do not block non-root restore flows. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Make the TUI simulation injector exit on cleanup or initial-draw timeout instead of waiting indefinitely on readyCh, while preserving the first-draw synchronization used by the tests. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Replace the secondary storage UsedSpace check based on a second Statfs snapshot with stable space-usage invariants so the test no longer depends on concurrent filesystem activity. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Replace context.Background() in AGE overwrite/add-recipient confirmation modals with the parent flow context so dialog lifecycles respect cancellation. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Set the existing configuration modal to focus "Keep & continue" by default, while keeping the existing regular-file validation for config paths. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Pass the parent install flow context into CheckExistingConfig and checkExistingConfigRunner so the existing-config modal can be cancelled with the rest of the TUI install workflow. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Make preserved-entry formatting stat paths relative to the target base directory instead of the process working directory, so directory markers in the confirmation dialog are accurate. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Dependency ReviewThe following issues were found:
License Issuesgo.mod
OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughWalkthroughThis PR introduces context-aware execution throughout the codebase, new interactive workflows for encryption setup and Telegram pairing, unified configuration decision flows, updated storage statistics tracking (including used-space metrics), enhanced secondary storage validation, and comprehensive TUI infrastructure updates with screen-building utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/identity/identity.go (1)
813-835:⚠️ Potential issue | 🟠 MajorDon't let cancellation leave the identity file mutable.
After Line 820 clears the immutable bit, every early return before Line 834 skips the re-lock. With the new context plumbing, a mid-operation cancellation can now return
context.Canceledafteros.WriteFilehas already modified the file, and Line 834 will also skip+ibecause it uses the same canceledctx. Restore immutability in deferred cleanup using a non-cancelable context, and checkctx.Err()before starting the write.Suggested fix
-func writeIdentityFileWithContext(ctx context.Context, path, content string, logger *logging.Logger) error { +func writeIdentityFileWithContext(ctx context.Context, path, content string, logger *logging.Logger) (err error) { if ctx == nil { ctx = context.Background() } logDebug(logger, "Identity: writeIdentityFile: start path=%s contentBytes=%d", path, len(content)) + if err := ctx.Err(); err != nil { + return err + } // Ensure file is writable even if immutable was previously set if err := setImmutableAttributeWithContext(ctx, path, false, logger); err != nil { return err } + restoreImmutable := true + defer func() { + if !restoreImmutable { + return + } + if restoreErr := setImmutableAttributeWithContext(context.Background(), path, true, logger); restoreErr != nil && err == nil { + err = restoreErr + } + }() + + if err := ctx.Err(); err != nil { + return err + } if err := os.WriteFile(path, []byte(content), 0o600); err != nil { logDebug(logger, "Identity: writeIdentityFile: os.WriteFile failed: %v", err) return err } @@ - if err := setImmutableAttributeWithContext(ctx, path, true, logger); err != nil { + if err := setImmutableAttributeWithContext(context.Background(), path, true, logger); err != nil { return err } + restoreImmutable = false logDebug(logger, "Identity: writeIdentityFile: done path=%s", path) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/identity/identity.go` around lines 813 - 835, The function writeIdentityFileWithContext clears the immutable bit then returns early on errors or if ctx is canceled, leaving the file mutable; fix by (1) checking ctx.Err() before performing os.WriteFile to abort early, and (2) installing a deferred cleanup that always re-applies the immutable bit using a non-cancelable context (e.g. context.Background()) that calls setImmutableAttributeWithContext(path, true, logger) so the file is re-locked even if the original ctx is canceled or an early return occurs; reference writeIdentityFileWithContext, setImmutableAttributeWithContext, os.WriteFile and ctx.Err().internal/orchestrator/restore_tui.go (1)
816-843:⚠️ Potential issue | 🟠 MajorWait for the countdown goroutine to exit before reading
timedOut.Line 817 starts a ticker goroutine that writes
timedOut/cancelled, but the function can return from Line 842 without waiting for that goroutine to stop. That leaves a race under-race, and it can still queue one last UI update after the app has already stopped.promptNetworkCommitTUIalready uses the saferstopCh+donepattern.Suggested fix
- stopCh := make(chan struct{}) - defer close(stopCh) + stopCh := make(chan struct{}) + done := make(chan struct{}) if timeout > 0 { go func() { + defer close(done) ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() for { select { case <-stopCh: return case <-ticker.C: left := time.Until(deadline) if left <= 0 { timedOut = true cancelled = true app.Stop() return } app.QueueUpdateDraw(func() { updateCountdown() }) } } }() + } else { + close(done) } app.SetRoot(page, true).SetFocus(form.Form) - if err := app.RunWithContext(ctx); err != nil { + err := app.RunWithContext(ctx) + close(stopCh) + <-done + if err != nil { return false, err }🤖 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 816 - 843, The ticker goroutine writing timedOut/cancelled can race with the function return; change the pattern to use a done channel (e.g., done := make(chan struct{}) ) alongside stopCh and ensure the goroutine closes done before exiting; after app.RunWithContext(ctx) returns, signal the goroutine to stop (close stopCh or send) and then block waiting for <-done so you only read timedOut/cancelled after the goroutine has finished. Update the goroutine body referenced (the anonymous function that creates ticker and calls updateCountdown/app.Stop) to close(done) on all exit paths and ensure it returns promptly when stopCh is closed, preventing any app.QueueUpdateDraw after app has stopped.
🧹 Nitpick comments (4)
internal/orchestrator/workflow_ui_cli.go (1)
136-139: Use stderr for validation feedback in CLI path prompt (Line 138).This is user-error output and should stay consistent with other error paths.
Proposed tweak
- if err != nil { - fmt.Println(err.Error()) + if err != nil { + fmt.Fprintln(os.Stderr, err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/workflow_ui_cli.go` around lines 136 - 139, The CLI validation prints user-error output to stdout; change the call in the block that handles validateDistinctNewPathInput(newPath, current) so errors are written to stderr (e.g., replace fmt.Println(err.Error()) with fmt.Fprintln(os.Stderr, err.Error())) and add an import for os if not already present so validateDistinctNewPathInput's error feedback uses stderr consistently with other CLI error paths.internal/orchestrator/restore_tui_simulation_test.go (1)
66-67: Assert the forwarded context in these stubbed prompt tests.The new
ctxparameter is currently ignored, so these cases only prove the signature changed. IfconfirmRestoreTUIfell back tocontext.Background()internally again, both tests would still pass.🔍 Minimal assertion
- restore := stubPromptYesNo(func(ctx context.Context, title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error) { + testCtx, cancel := context.WithCancel(context.Background()) + defer cancel() + restore := stubPromptYesNo(func(ctx context.Context, title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error) { + if ctx != testCtx { + t.Fatalf("prompt received unexpected context") + } return true, nil }) @@ - ok, err := confirmRestoreTUI(context.Background(), "/tmp/config.env", "sig") + ok, err := confirmRestoreTUI(testCtx, "/tmp/config.env", "sig")Apply the same pattern to the
OverwriteDeclinedReturnsFalsecase.Also applies to: 73-74, 83-84, 90-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_tui_simulation_test.go` around lines 66 - 67, The stubbed prompt functions (stubPromptYesNo) ignore the incoming ctx so tests only verify signature change; update each stub (the one used by confirmRestoreTUI and OverwriteDeclinedReturnsFalse) to assert the received ctx matches the test's context (e.g., compare to the ctx you pass into confirmRestoreTUI) and then return the existing true/false result, and apply this same assertion pattern to the other stubs referenced (lines noted around 73-74, 83-84, 90-91) so the tests will fail if confirmRestoreTUI stops forwarding the original context.internal/config/migration_test.go (1)
158-168: Build the invalid-permissions fixture frombaseInstallTemplate.
baseInstallTemplatenow carries the canonical secondary keys, whileinvalidPermissionsTemplatelater in this file still duplicates the install template by hand. Deriving the invalid fixture from the base template would avoid another drift the next time a canonical env key is added.🤖 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 158 - 168, Replace the hand-duplicated invalidPermissionsTemplate with one derived from baseInstallTemplate: construct invalidPermissionsTemplate by starting from baseInstallTemplate and programmatically overriding the fields that need to represent "invalid permissions" (e.g., change BACKUP_PATH/BACKUP_USER/BACKUP_GROUP and any SECONDARY_* values) so the fixture reuses baseInstallTemplate's canonical keys; update references to invalidPermissionsTemplate in migration_test.go to use this derived value to avoid future drift.internal/orchestrator/workflow_ui_cli_test.go (1)
13-42: Consider simplifying the stdout capture cleanup logic.The cleanup is duplicated between
t.Cleanup(lines 22-24) anddefer(lines 32-38). Thet.Cleanuponly restoresos.Stdout, while thedeferdoes the full cleanup including pipe closure and waiting for the goroutine. Thet.Cleanupbecomes a no-op after thedeferruns.This works correctly but could be simplified by removing the
t.Cleanupregistration since thedeferhandles everything needed before the return.♻️ Optional simplification
os.Stdout = w - t.Cleanup(func() { - os.Stdout = oldStdout - }) var buf bytes.Buffer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/workflow_ui_cli_test.go` around lines 13 - 42, Remove the redundant t.Cleanup registration in captureCLIStdout: the defer block already restores os.Stdout, closes the write end, waits for the read goroutine, closes the read end, and captures the buffer, so delete the t.Cleanup(func() { os.Stdout = oldStdout }) to avoid duplicate/no-op cleanup and keep only the existing defer-based cleanup inside the captureCLIStdout function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 28-35: The function promptExistingConfigModeCLI returns
existingConfigOverwrite when os.Stat reports the file missing without first
checking ctx cancellation, and later returns a decision (around where
existingConfigOverwrite/existingConfigCancel is resolved) without a final
ctx.Err() guard; before taking the fast-path return for a missing file and
before finalizing the decision at the later return, check ctx.Err() and if
non-nil return existingConfigCancel with the ctx error so a canceled context
prevents progressing into config generation (update promptExistingConfigModeCLI
to validate ctx.Err() immediately after os.Stat error handling and just before
final returns).
In `@cmd/proxsave/install.go`:
- Around line 843-845: The function clearImmutableAttributes is unused after the
refactor to clearImmutableAttributesWithContext; remove the obsolete wrapper
function clearImmutableAttributes (or if backward compatibility is required,
replace its body by calling clearImmutableAttributesWithContext with a context
parameter and update any callers to use it). Locate the symbol
clearImmutableAttributes in the diff and either delete that function entirely or
change it to forward to
clearImmutableAttributesWithContext(context.Background(), target, bootstrap) and
ensure no other code relies on the old signature.
In `@cmd/proxsave/new_install.go`:
- Around line 55-63: formatNewInstallPreservedEntries currently appends "/" to
trimmed entries without removing existing trailing slashes, producing double
slashes for inputs like "env/"; update the function to strip any trailing
slashes from each trimmed entry (e.g., via strings.TrimSuffix or
strings.TrimRight for "/") before appending a single "/", skip empty results,
and then append the normalized entry+"/" so outputs are always single-suffixed.
In `@cmd/proxsave/newkey_test.go`:
- Around line 103-129: The test writes a config string with
"ENCRYPT_ARCHIVE=false" but then asserts cfg.EncryptArchive is true; update the
assertion to match the config (expect false) or, if the intent is to test that
loadNewKeyConfig forces encryption on, change the config content to
"ENCRYPT_ARCHIVE=true" instead—locate this in newkey_test.go around the config
content variable and the assertion checking cfg.EncryptArchive after calling
loadNewKeyConfig and make the expectation consistent.
In `@internal/config/config_test.go`:
- Around line 12-15: The helper setBaseDirEnv currently calls
t.Setenv("BASE_DIR", value) which sets the variable to an empty string rather
than unsetting it; update setBaseDirEnv so that when value == "" it calls
os.Unsetenv("BASE_DIR") (and optionally registers a cleanup via t.Cleanup if
needed) otherwise call t.Setenv("BASE_DIR", value) so tests that rely on
os.LookupEnv behave correctly; refer to the setBaseDirEnv helper to locate where
to change behavior.
In `@internal/config/env_mutation_test.go`:
- Around line 13-21: The tests currently just use strings.Contains on the
mutated template variable got which will pass even if old duplicate env entries
remain; instead parse got into key/value pairs (split by newlines and then on
the first '='), create a map of final values, and assert exact values for each
variable (e.g. check map["SECONDARY_ENABLED"] == "true", map["SECONDARY_PATH"]
== "/mnt/secondary", map["SECONDARY_LOG_PATH"] == "/mnt/secondary/log"); update
the assertions in env_mutation_test.go (the loops around got and the checks at
the other ranges mentioned) to use this parsed map so each key has exactly one
final value rather than just presence via strings.Contains.
In `@internal/config/env_mutation.go`:
- Around line 15-16: The current calls to utils.SetEnvValue(template,
"SECONDARY_PATH", strings.TrimSpace(secondaryPath)) and for SECONDARY_LOG_PATH
write raw unquoted paths which can break .env parsing (e.g., paths with #). Fix
by ensuring values are safely quoted or escaped before calling SetEnvValue: wrap
the trimmed secondaryPath and secondaryLogPath with a proper env-safe encoder
(e.g., use strconv.Quote or an escape helper) so SetEnvValue receives a
quoted/escaped string; update the calls in internal/config/env_mutation.go that
set SECONDARY_PATH and SECONDARY_LOG_PATH (and any similar env writes) to use
the encoder to prevent comment/parse issues.
In `@internal/notify/webhook_test.go`:
- Around line 283-323: The test currently cancels the context before the first
response is completed and uses an unsynchronized attempts counter; change
attempts to an atomic counter (e.g., atomic.Int32 or atomic.AddInt32/LoadInt32)
and increment it from inside the test server handler, write the HTTP response
fully (w.WriteHeader and w.Write) and then call cancel() so the first response
is delivered, increase cfg.RetryDelay from 0 to a small non-zero duration to
allow the retry path in NewWebhookNotifier.sendToEndpoint to be exercised, and
update the assertion to read the atomic counter (via atomic.Load...) to verify
the expected number of attempts after cancellation.
In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go`:
- Around line 50-108: The current withTimedSimAppSequence helper drives input by
sleeping (timedSimKey.Wait and timers in the inject goroutine) which causes
flakes; change the inject loop inside newTUIApp to drive each key by observing
screen state or focus transitions instead of wall-clock waits—use the tcell
SimulationScreen's current content/focus or a channel/event emitted by the app
to detect when the UI moved to the expected view before calling screen.InjectKey
for each entry in keys (keep the existing done, injectWG and timing-zero path
intact for immediate keys, but replace timer-based waits with waits for explicit
screen-state checks or a focus-change notification so keys are sent to the
correct view).
In `@internal/orchestrator/decrypt_workflow_ui.go`:
- Around line 22-24: The nil-checks like `if ui == nil { ... }` in
decrypt_workflow_ui.go can miss typed-nil interface values and lead to panics;
add a helper `isNilInterface(v any) bool` (using reflect.ValueOf and rv.IsNil
for kinds Chan, Func, Interface, Map, Pointer, Slice) and replace all three
guards that currently check `ui == nil` (and any other interface parameter
checks in that file) with `if isNilInterface(ui) { return nil,
fmt.Errorf("backup selection UI not available") }` (or the file's existing error
handling) so both true nil and typed-nil interfaces are detected safely.
In `@internal/orchestrator/restore_ha_additional_test.go`:
- Around line 386-391: The test is creating one env and then running multiple
t.Run subtests which mutate package-level globals (restoreFS, haApplyGeteuid,
haIsMounted, haArmRollback, etc.) so later siblings inherit state; fix by
creating a fresh HA test environment inside each subtest (move env :=
setupHATestEnv(t) into each t.Run) or, if you keep a shared parent, ensure every
override of globals is paired with t.Cleanup to restore the original value
(capture originals before assignment and restore them in t.Cleanup for functions
like restoreFS, haApplyGeteuid, haIsMounted, haArmRollback and any env.fs
changes) so each subtest runs with isolated state.
In `@internal/storage/backup_files.go`:
- Around line 16-20: bundlePathFor currently builds the bundle candidate from
the raw path without fully normalizing inputs that already include bundle
suffixes, causing sidecars like .sha256/.metadata to be emitted with the bundled
filename; fix by fully normalizing the input by calling trimBundleSuffix in a
loop (or otherwise strip any trailing bundleSuffix occurrences) to reduce the
path to the true base before appending bundleSuffix in bundlePathFor, and apply
the same normalization logic wherever non-bundle candidates are constructed (see
the same pattern around lines referenced in the review, functions/blocks that
build legacy sidecar names) so legacy sibling names are produced for bundle
inputs.
In `@internal/storage/cloud.go`:
- Around line 380-391: Add a defensive nil-check wrapper for the injectable
waitForRetry hook on CloudStorage similar to the existing execCommand and
lookPath wrappers: implement a method (e.g., callWaitForRetry or
waitForRetrySafe) on CloudStorage that checks if c.waitForRetry != nil and if so
calls it with the same signature, otherwise returns nil; then replace direct
calls to c.waitForRetry(...) (used around the retry handling in wait logic and
the other location) with this new wrapper to avoid panics when the exported
struct is constructed without that hook (see NewCloudStorage, waitForRetry,
execCommand, lookPath for the existing pattern).
---
Outside diff comments:
In `@internal/identity/identity.go`:
- Around line 813-835: The function writeIdentityFileWithContext clears the
immutable bit then returns early on errors or if ctx is canceled, leaving the
file mutable; fix by (1) checking ctx.Err() before performing os.WriteFile to
abort early, and (2) installing a deferred cleanup that always re-applies the
immutable bit using a non-cancelable context (e.g. context.Background()) that
calls setImmutableAttributeWithContext(path, true, logger) so the file is
re-locked even if the original ctx is canceled or an early return occurs;
reference writeIdentityFileWithContext, setImmutableAttributeWithContext,
os.WriteFile and ctx.Err().
In `@internal/orchestrator/restore_tui.go`:
- Around line 816-843: The ticker goroutine writing timedOut/cancelled can race
with the function return; change the pattern to use a done channel (e.g., done
:= make(chan struct{}) ) alongside stopCh and ensure the goroutine closes done
before exiting; after app.RunWithContext(ctx) returns, signal the goroutine to
stop (close stopCh or send) and then block waiting for <-done so you only read
timedOut/cancelled after the goroutine has finished. Update the goroutine body
referenced (the anonymous function that creates ticker and calls
updateCountdown/app.Stop) to close(done) on all exit paths and ensure it returns
promptly when stopCh is closed, preventing any app.QueueUpdateDraw after app has
stopped.
---
Nitpick comments:
In `@internal/config/migration_test.go`:
- Around line 158-168: Replace the hand-duplicated invalidPermissionsTemplate
with one derived from baseInstallTemplate: construct invalidPermissionsTemplate
by starting from baseInstallTemplate and programmatically overriding the fields
that need to represent "invalid permissions" (e.g., change
BACKUP_PATH/BACKUP_USER/BACKUP_GROUP and any SECONDARY_* values) so the fixture
reuses baseInstallTemplate's canonical keys; update references to
invalidPermissionsTemplate in migration_test.go to use this derived value to
avoid future drift.
In `@internal/orchestrator/restore_tui_simulation_test.go`:
- Around line 66-67: The stubbed prompt functions (stubPromptYesNo) ignore the
incoming ctx so tests only verify signature change; update each stub (the one
used by confirmRestoreTUI and OverwriteDeclinedReturnsFalse) to assert the
received ctx matches the test's context (e.g., compare to the ctx you pass into
confirmRestoreTUI) and then return the existing true/false result, and apply
this same assertion pattern to the other stubs referenced (lines noted around
73-74, 83-84, 90-91) so the tests will fail if confirmRestoreTUI stops
forwarding the original context.
In `@internal/orchestrator/workflow_ui_cli_test.go`:
- Around line 13-42: Remove the redundant t.Cleanup registration in
captureCLIStdout: the defer block already restores os.Stdout, closes the write
end, waits for the read goroutine, closes the read end, and captures the buffer,
so delete the t.Cleanup(func() { os.Stdout = oldStdout }) to avoid
duplicate/no-op cleanup and keep only the existing defer-based cleanup inside
the captureCLIStdout function.
In `@internal/orchestrator/workflow_ui_cli.go`:
- Around line 136-139: The CLI validation prints user-error output to stdout;
change the call in the block that handles validateDistinctNewPathInput(newPath,
current) so errors are written to stderr (e.g., replace fmt.Println(err.Error())
with fmt.Fprintln(os.Stderr, err.Error())) and add an import for os if not
already present so validateDistinctNewPathInput's error feedback uses stderr
consistently with other CLI error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3d08981-ee08-4879-bd81-da86a0aa0717
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (139)
cmd/proxsave/encryption_setup.gocmd/proxsave/encryption_setup_test.gocmd/proxsave/helpers_test.gocmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/install_test.gocmd/proxsave/install_tui.gocmd/proxsave/main.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gocmd/proxsave/newkey.gocmd/proxsave/newkey_test.gocmd/proxsave/prompts.gocmd/proxsave/runtime_helpers.gocmd/proxsave/runtime_helpers_more_test.gocmd/proxsave/schedule_helpers.gocmd/proxsave/schedule_helpers_test.gocmd/proxsave/telegram_setup_cli.gocmd/proxsave/telegram_setup_cli_test.gocmd/proxsave/upgrade.godocs/BACKUP_ENV_MAPPING.mddocs/CLI_REFERENCE.mddocs/CLOUD_STORAGE.mddocs/CONFIGURATION.mddocs/ENCRYPTION.mddocs/INSTALL.mddocs/MIGRATION_GUIDE.mdgo.modinternal/backup/collector_pve.gointernal/backup/collector_pve_additional_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration.gointernal/config/migration_test.gointernal/config/templates/backup.envinternal/config/validation_secondary.gointernal/config/validation_secondary_test.gointernal/cron/cron.gointernal/cron/cron_test.gointernal/environment/detect_additional_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/logging/bootstrap.gointernal/logging/bootstrap_test.gointernal/notify/context_helpers.gointernal/notify/email.gointernal/notify/email_mailq_test.gointernal/notify/email_parsing_test.gointernal/notify/email_relay.gointernal/notify/email_relay_test.gointernal/notify/webhook.gointernal/notify/webhook_test.gointernal/orchestrator/additional_helpers_test.gointernal/orchestrator/age_setup_ui.gointernal/orchestrator/age_setup_ui_cli.gointernal/orchestrator/age_setup_workflow.gointernal/orchestrator/age_setup_workflow_test.gointernal/orchestrator/backup_sources.gointernal/orchestrator/backup_sources_test.gointernal/orchestrator/bundle_test.gointernal/orchestrator/decrypt_additional_test.gointernal/orchestrator/decrypt_test.gointernal/orchestrator/decrypt_tui.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_tui_e2e_test.gointernal/orchestrator/decrypt_tui_simulation_test.gointernal/orchestrator/decrypt_tui_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/deps_test.gointernal/orchestrator/encryption.gointernal/orchestrator/encryption_exported_test.gointernal/orchestrator/encryption_more_test.gointernal/orchestrator/encryption_test.gointernal/orchestrator/helpers_test.gointernal/orchestrator/network_apply_workflow_ui.gointernal/orchestrator/notification_adapter.gointernal/orchestrator/notification_adapter_test.gointernal/orchestrator/orchestrator.gointernal/orchestrator/restore.gointernal/orchestrator/restore_access_control_ui_additional_test.gointernal/orchestrator/restore_errors_test.gointernal/orchestrator/restore_firewall_additional_test.gointernal/orchestrator/restore_ha.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/restore_tui_test.gointernal/orchestrator/restore_workflow_integration_test.gointernal/orchestrator/storage_adapter.gointernal/orchestrator/storage_adapter_test.gointernal/orchestrator/telegram_setup_bootstrap.gointernal/orchestrator/telegram_setup_bootstrap_test.gointernal/orchestrator/tui_screen_env.gointernal/orchestrator/tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/orchestrator/workflow_ui_tui_decrypt.gointernal/orchestrator/workflow_ui_tui_decrypt_prompts.gointernal/orchestrator/workflow_ui_tui_decrypt_test.gointernal/orchestrator/workflow_ui_tui_restore.gointernal/orchestrator/workflow_ui_tui_restore_test.gointernal/orchestrator/workflow_ui_tui_shared.gointernal/safefs/safefs.gointernal/safefs/safefs_test.gointernal/security/security.gointernal/security/security_test.gointernal/storage/backup_files.gointernal/storage/cloud.gointernal/storage/cloud_test.gointernal/storage/local.gointernal/storage/retention.gointernal/storage/retention_normalize.gointernal/storage/retention_test.gointernal/storage/secondary.gointernal/storage/secondary_test.gointernal/storage/storage_test.gointernal/testutil/age_setup_ui_stub.gointernal/tui/abort_context_test.gointernal/tui/app.gointernal/tui/screen.gointernal/tui/screen_test.gointernal/tui/wizard/age.gointernal/tui/wizard/age_test.gointernal/tui/wizard/age_ui_adapter.gointernal/tui/wizard/age_ui_adapter_test.gointernal/tui/wizard/install.gointernal/tui/wizard/install_test.gointernal/tui/wizard/new_install.gointernal/tui/wizard/new_install_test.gointernal/tui/wizard/post_install_audit_tui.gointernal/tui/wizard/post_install_audit_tui_test.gointernal/tui/wizard/screen.gointernal/tui/wizard/telegram_setup_tui.gointernal/tui/wizard/telegram_setup_tui_test.go
💤 Files with no reviewable changes (2)
- internal/orchestrator/restore_workflow_integration_test.go
- internal/orchestrator/decrypt_tui_test.go
Comment on lines
+28
to
+35
| func promptExistingConfigModeCLI(ctx context.Context, reader *bufio.Reader, configPath string) (existingConfigMode, error) { | ||
| info, err := os.Stat(configPath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return existingConfigOverwrite, nil | ||
| } | ||
| return existingConfigCancel, fmt.Errorf("failed to access configuration file: %w", err) | ||
| } |
There was a problem hiding this comment.
Honor ctx before taking the overwrite fast path.
Line 31 returns existingConfigOverwrite on a missing file without checking cancellation first, so a canceled install can still advance into config generation on the fresh-install path. Line 109 also resolves the decision without a final ctx.Err() guard.
Suggested fix
func promptExistingConfigModeCLI(ctx context.Context, reader *bufio.Reader, configPath string) (existingConfigMode, error) {
+ if ctx == nil {
+ ctx = context.Background()
+ }
+ if err := ctx.Err(); err != nil {
+ return existingConfigCancel, err
+ }
+
info, err := os.Stat(configPath)
if err != nil {
if os.IsNotExist(err) {
return existingConfigOverwrite, nil
}
@@
func prepareExistingConfigDecisionCLI(ctx context.Context, reader *bufio.Reader, configPath string) (existingConfigDecision, error) {
mode, err := promptExistingConfigModeCLI(ctx, reader, configPath)
if err != nil {
return existingConfigDecision{}, err
}
+ if ctx != nil {
+ if err := ctx.Err(); err != nil {
+ return existingConfigDecision{}, err
+ }
+ }
return resolveExistingConfigDecision(mode, configPath)
}Also applies to: 104-109
🤖 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 28 - 35, The function
promptExistingConfigModeCLI returns existingConfigOverwrite when os.Stat reports
the file missing without first checking ctx cancellation, and later returns a
decision (around where existingConfigOverwrite/existingConfigCancel is resolved)
without a final ctx.Err() guard; before taking the fast-path return for a
missing file and before finalizing the decision at the later return, check
ctx.Err() and if non-nil return existingConfigCancel with the ctx error so a
canceled context prevents progressing into config generation (update
promptExistingConfigModeCLI to validate ctx.Err() immediately after os.Stat
error handling and just before final returns).
Comment on lines
843
to
+845
| func clearImmutableAttributes(target string, bootstrap *logging.BootstrapLogger) { | ||
| _ = clearImmutableAttributesWithContext(context.Background(), target, bootstrap) | ||
| } |
There was a problem hiding this comment.
Remove or use the unused function clearImmutableAttributes.
The pipeline reports this function as unused. After the refactor to context-aware clearImmutableAttributesWithContext, the wrapper is no longer called. Either remove it or update call sites if backward compatibility is needed.
🧹 Proposed fix: remove unused wrapper
-// clearImmutableAttributes attempts to remove immutable flags (chattr -i) so deletion can proceed.
-// It logs warnings on failure but does not return an error, since removal will report issues later.
-func clearImmutableAttributes(target string, bootstrap *logging.BootstrapLogger) {
- _ = clearImmutableAttributesWithContext(context.Background(), target, bootstrap)
-}
-📝 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 clearImmutableAttributes(target string, bootstrap *logging.BootstrapLogger) { | |
| _ = clearImmutableAttributesWithContext(context.Background(), target, bootstrap) | |
| } |
🧰 Tools
🪛 GitHub Actions: Ultimate Security Analysis (Go)
[error] 843-843: staticcheck reported unused function: clearImmutableAttributes is unused (U1000).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/proxsave/install.go` around lines 843 - 845, The function
clearImmutableAttributes is unused after the refactor to
clearImmutableAttributesWithContext; remove the obsolete wrapper function
clearImmutableAttributes (or if backward compatibility is required, replace its
body by calling clearImmutableAttributesWithContext with a context parameter and
update any callers to use it). Locate the symbol clearImmutableAttributes in the
diff and either delete that function entirely or change it to forward to
clearImmutableAttributesWithContext(context.Background(), target, bootstrap) and
ensure no other code relies on the old signature.
Comment on lines
+55
to
+63
| func formatNewInstallPreservedEntries(entries []string) string { | ||
| formatted := make([]string, 0, len(entries)) | ||
| for _, entry := range entries { | ||
| trimmed := strings.TrimSpace(entry) | ||
| if trimmed == "" { | ||
| continue | ||
| } | ||
| formatted = append(formatted, trimmed+"/") | ||
| } |
There was a problem hiding this comment.
Trim trailing slashes before appending /.
formatNewInstallPreservedEntries() currently turns inputs like env/ into env//. If this formatter ever receives already-suffixed or resolved entries, the reset summary regresses back to the double-slash output this flow is trying to avoid.
♻️ Suggested fix
func formatNewInstallPreservedEntries(entries []string) string {
formatted := make([]string, 0, len(entries))
for _, entry := range entries {
trimmed := strings.TrimSpace(entry)
+ trimmed = strings.TrimRight(trimmed, "/")
if trimmed == "" {
continue
}
formatted = append(formatted, trimmed+"/")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/proxsave/new_install.go` around lines 55 - 63,
formatNewInstallPreservedEntries currently appends "/" to trimmed entries
without removing existing trailing slashes, producing double slashes for inputs
like "env/"; update the function to strip any trailing slashes from each trimmed
entry (e.g., via strings.TrimSuffix or strings.TrimRight for "/") before
appending a single "/", skip empty results, and then append the normalized
entry+"/" so outputs are always single-suffixed.
Comment on lines
+103
to
+129
| content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=false\nAGE_RECIPIENT_FILE=" + customPath + "\n" | ||
| if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { | ||
| t.Fatalf("WriteFile(%s): %v", configPath, err) | ||
| } | ||
|
|
||
| cfg, recipientPath, err := loadNewKeyConfig(configPath, baseDir) | ||
| if err != nil { | ||
| t.Fatalf("loadNewKeyConfig error: %v", err) | ||
| } | ||
| if recipientPath != customPath { | ||
| t.Fatalf("recipientPath=%q; want %q", recipientPath, customPath) | ||
| } | ||
| if cfg == nil { | ||
| t.Fatalf("expected config") | ||
| } | ||
| if cfg.BaseDir != baseDir { | ||
| t.Fatalf("BaseDir=%q; want %q", cfg.BaseDir, baseDir) | ||
| } | ||
| if cfg.ConfigPath != configPath { | ||
| t.Fatalf("ConfigPath=%q; want %q", cfg.ConfigPath, configPath) | ||
| } | ||
| if cfg.AgeRecipientFile != customPath { | ||
| t.Fatalf("AgeRecipientFile=%q; want %q", cfg.AgeRecipientFile, customPath) | ||
| } | ||
| if !cfg.EncryptArchive { | ||
| t.Fatalf("EncryptArchive=false; want true") | ||
| } |
There was a problem hiding this comment.
Test assertion contradicts the config file content.
The config file on line 103 sets ENCRYPT_ARCHIVE=false, but the assertion on lines 127-129 expects cfg.EncryptArchive to be true. Either the config content or the assertion needs to be corrected.
🐛 Proposed fix
If the intent is to test that loadNewKeyConfig enforces encryption:
- if !cfg.EncryptArchive {
- t.Fatalf("EncryptArchive=false; want true")
- }
+ if cfg.EncryptArchive {
+ t.Fatalf("EncryptArchive=true; want false (matches config)")
+ }Or if the intent is to test that encryption is enabled:
- content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=false\nAGE_RECIPIENT_FILE=" + customPath + "\n"
+ content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=true\nAGE_RECIPIENT_FILE=" + customPath + "\n"📝 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
| content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=false\nAGE_RECIPIENT_FILE=" + customPath + "\n" | |
| if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { | |
| t.Fatalf("WriteFile(%s): %v", configPath, err) | |
| } | |
| cfg, recipientPath, err := loadNewKeyConfig(configPath, baseDir) | |
| if err != nil { | |
| t.Fatalf("loadNewKeyConfig error: %v", err) | |
| } | |
| if recipientPath != customPath { | |
| t.Fatalf("recipientPath=%q; want %q", recipientPath, customPath) | |
| } | |
| if cfg == nil { | |
| t.Fatalf("expected config") | |
| } | |
| if cfg.BaseDir != baseDir { | |
| t.Fatalf("BaseDir=%q; want %q", cfg.BaseDir, baseDir) | |
| } | |
| if cfg.ConfigPath != configPath { | |
| t.Fatalf("ConfigPath=%q; want %q", cfg.ConfigPath, configPath) | |
| } | |
| if cfg.AgeRecipientFile != customPath { | |
| t.Fatalf("AgeRecipientFile=%q; want %q", cfg.AgeRecipientFile, customPath) | |
| } | |
| if !cfg.EncryptArchive { | |
| t.Fatalf("EncryptArchive=false; want true") | |
| } | |
| content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=false\nAGE_RECIPIENT_FILE=" + customPath + "\n" | |
| if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { | |
| t.Fatalf("WriteFile(%s): %v", configPath, err) | |
| } | |
| cfg, recipientPath, err := loadNewKeyConfig(configPath, baseDir) | |
| if err != nil { | |
| t.Fatalf("loadNewKeyConfig error: %v", err) | |
| } | |
| if recipientPath != customPath { | |
| t.Fatalf("recipientPath=%q; want %q", recipientPath, customPath) | |
| } | |
| if cfg == nil { | |
| t.Fatalf("expected config") | |
| } | |
| if cfg.BaseDir != baseDir { | |
| t.Fatalf("BaseDir=%q; want %q", cfg.BaseDir, baseDir) | |
| } | |
| if cfg.ConfigPath != configPath { | |
| t.Fatalf("ConfigPath=%q; want %q", cfg.ConfigPath, configPath) | |
| } | |
| if cfg.AgeRecipientFile != customPath { | |
| t.Fatalf("AgeRecipientFile=%q; want %q", cfg.AgeRecipientFile, customPath) | |
| } | |
| if cfg.EncryptArchive { | |
| t.Fatalf("EncryptArchive=true; want false (matches config)") | |
| } |
Suggested change
| content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=false\nAGE_RECIPIENT_FILE=" + customPath + "\n" | |
| if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { | |
| t.Fatalf("WriteFile(%s): %v", configPath, err) | |
| } | |
| cfg, recipientPath, err := loadNewKeyConfig(configPath, baseDir) | |
| if err != nil { | |
| t.Fatalf("loadNewKeyConfig error: %v", err) | |
| } | |
| if recipientPath != customPath { | |
| t.Fatalf("recipientPath=%q; want %q", recipientPath, customPath) | |
| } | |
| if cfg == nil { | |
| t.Fatalf("expected config") | |
| } | |
| if cfg.BaseDir != baseDir { | |
| t.Fatalf("BaseDir=%q; want %q", cfg.BaseDir, baseDir) | |
| } | |
| if cfg.ConfigPath != configPath { | |
| t.Fatalf("ConfigPath=%q; want %q", cfg.ConfigPath, configPath) | |
| } | |
| if cfg.AgeRecipientFile != customPath { | |
| t.Fatalf("AgeRecipientFile=%q; want %q", cfg.AgeRecipientFile, customPath) | |
| } | |
| if !cfg.EncryptArchive { | |
| t.Fatalf("EncryptArchive=false; want true") | |
| } | |
| content := "BASE_DIR=" + baseDir + "\nENCRYPT_ARCHIVE=true\nAGE_RECIPIENT_FILE=" + customPath + "\n" | |
| if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { | |
| t.Fatalf("WriteFile(%s): %v", configPath, err) | |
| } | |
| cfg, recipientPath, err := loadNewKeyConfig(configPath, baseDir) | |
| if err != nil { | |
| t.Fatalf("loadNewKeyConfig error: %v", err) | |
| } | |
| if recipientPath != customPath { | |
| t.Fatalf("recipientPath=%q; want %q", recipientPath, customPath) | |
| } | |
| if cfg == nil { | |
| t.Fatalf("expected config") | |
| } | |
| if cfg.BaseDir != baseDir { | |
| t.Fatalf("BaseDir=%q; want %q", cfg.BaseDir, baseDir) | |
| } | |
| if cfg.ConfigPath != configPath { | |
| t.Fatalf("ConfigPath=%q; want %q", cfg.ConfigPath, configPath) | |
| } | |
| if cfg.AgeRecipientFile != customPath { | |
| t.Fatalf("AgeRecipientFile=%q; want %q", cfg.AgeRecipientFile, customPath) | |
| } | |
| if !cfg.EncryptArchive { | |
| t.Fatalf("EncryptArchive=false; want true") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/proxsave/newkey_test.go` around lines 103 - 129, The test writes a config
string with "ENCRYPT_ARCHIVE=false" but then asserts cfg.EncryptArchive is true;
update the assertion to match the config (expect false) or, if the intent is to
test that loadNewKeyConfig forces encryption on, change the config content to
"ENCRYPT_ARCHIVE=true" instead—locate this in newkey_test.go around the config
content variable and the assertion checking cfg.EncryptArchive after calling
loadNewKeyConfig and make the expectation consistent.
Comment on lines
+12
to
15
| func setBaseDirEnv(t *testing.T, value string) { | ||
| t.Helper() | ||
|
|
||
| prev := os.Getenv("BASE_DIR") | ||
| if value == "" { | ||
| _ = os.Unsetenv("BASE_DIR") | ||
| } else { | ||
| if err := os.Setenv("BASE_DIR", value); err != nil { | ||
| t.Fatalf("failed to set BASE_DIR: %v", err) | ||
| } | ||
| } | ||
|
|
||
| return func() { | ||
| if prev == "" { | ||
| _ = os.Unsetenv("BASE_DIR") | ||
| } else { | ||
| _ = os.Setenv("BASE_DIR", prev) | ||
| } | ||
| } | ||
| t.Setenv("BASE_DIR", value) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git ls-files internal/config/ | head -20Repository: tis24dev/proxsave
Length of output: 129
🏁 Script executed:
cat -n internal/config/config_test.go | head -100Repository: tis24dev/proxsave
Length of output: 3040
🏁 Script executed:
cat -n internal/config/config.go | head -100Repository: tis24dev/proxsave
Length of output: 3256
🏁 Script executed:
cat -n internal/config/config_test.go | sed -n '100,300p'Repository: tis24dev/proxsave
Length of output: 7918
🏁 Script executed:
# Look for TestLoadConfigBaseDirFromConfig specifically
grep -n "TestLoadConfigBaseDirFromConfig" internal/config/config_test.go -A 30Repository: tis24dev/proxsave
Length of output: 1026
🏁 Script executed:
# Check how BaseDir is determined in LoadConfig
grep -n "BaseDir" internal/config/config.go -B 2 -A 2Repository: tis24dev/proxsave
Length of output: 1885
🏁 Script executed:
# Look at the getString implementation
grep -n "func.*getString" internal/config/config.go -A 10 | head -50Repository: tis24dev/proxsave
Length of output: 1550
🏁 Script executed:
# Check if getString uses os.Getenv or os.LookupEnv
grep -n "getString\|LookupEnv\|Getenv" internal/config/config.go | head -30Repository: tis24dev/proxsave
Length of output: 2378
🏁 Script executed:
# Check if there are any uses of LookupEnv in config.go
grep -n "LookupEnv" internal/config/config.goRepository: tis24dev/proxsave
Length of output: 112
🏁 Script executed:
# Verify how t.Setenv works in Go - check if setting to empty string is visible via LookupEnv
python3 << 'EOF'
# Let's verify Go's semantics with a simple check of how os.Getenv and os.LookupEnv differ
print("In Go:")
print("- os.Getenv(key) returns '' for both unset and set-to-empty")
print("- os.LookupEnv(key) returns (value, bool) - bool is true if key exists")
print("- t.Setenv(key, '') sets the env var to empty string (exists but empty)")
print("- Unsetting would make the key completely absent")
print("\nThe review comment is correct about the semantic difference.")
EOFRepository: tis24dev/proxsave
Length of output: 381
🏁 Script executed:
# Check how the config file is parsed - does it use c.raw which comes from file parsing?
grep -n "c.raw" internal/config/config.go | head -20Repository: tis24dev/proxsave
Length of output: 870
🏁 Script executed:
# Look at where c.raw is populated
grep -n "loadFromFile\|c.raw\|NewConfig" internal/config/config.go | head -30Repository: tis24dev/proxsave
Length of output: 870
🏁 Script executed:
# Get context around line 1000 to see the LookupEnv usage
sed -n '990,1010p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 449
🏁 Script executed:
# Check the full loadFromFile and initialization logic
sed -n '300,370p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 3363
🏁 Script executed:
# Look for where environment variables are read and take precedence
sed -n '340,380p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 1215
🏁 Script executed:
# Get more context around the config var expansion and how it's used
sed -n '985,1010p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 567
🏁 Script executed:
# Check the test again and see what the actual expectations are
sed -n '565,590p' internal/config/config_test.goRepository: tis24dev/proxsave
Length of output: 759
🏁 Script executed:
# Verify defaultBaseDir implementation
sed -n '918,945p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 828
🏁 Script executed:
# Find all uses of LookupEnv in the codebase
rg "LookupEnv" internal/config/Repository: tis24dev/proxsave
Length of output: 133
🏁 Script executed:
# Check if there are environment variable checks that use LookupEnv for precedence
grep -B5 -A5 "LookupEnv" internal/config/config.goRepository: tis24dev/proxsave
Length of output: 251
🏁 Script executed:
# Look at how loadEnvOverrides interacts with variable expansion
sed -n '340,360p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 737
🏁 Script executed:
# Check the full parseGeneralSettings method to understand the BASE_DIR handling
sed -n '465,475p' internal/config/config.goRepository: tis24dev/proxsave
Length of output: 449
🏁 Script executed:
# Trace through to see if os.Setenv is called after loading the config
grep -n "os.Setenv" internal/config/config.goRepository: tis24dev/proxsave
Length of output: 102
🏁 Script executed:
# Search for tests that might not have BASE_DIR in the config file
grep -n "setBaseDirEnv(t, \"\")" internal/config/config_test.goRepository: tis24dev/proxsave
Length of output: 86
🏁 Script executed:
# Check if there are any other tests using this pattern and see their config content
grep -B20 "setBaseDirEnv(t, \"\")" internal/config/config_test.go | grep -E "content :=|BASE_DIR|BACKUP_PATH"Repository: tis24dev/proxsave
Length of output: 131
t.Setenv("BASE_DIR", "") does not unset BASE_DIR; it sets it to an empty string.
This weakens test coverage for config-file-vs-environment precedence. Although the current test TestLoadConfigBaseDirFromConfig passes because BASE_DIR is defined in the config file, the helper should properly unset the variable to avoid confusion and ensure os.LookupEnv-based logic is tested correctly.
Suggested fix
func setBaseDirEnv(t *testing.T, value string) {
t.Helper()
- t.Setenv("BASE_DIR", value)
+ if value != "" {
+ t.Setenv("BASE_DIR", value)
+ return
+ }
+
+ prev, hadPrev := os.LookupEnv("BASE_DIR")
+ if err := os.Unsetenv("BASE_DIR"); err != nil {
+ t.Fatalf("unset BASE_DIR: %v", err)
+ }
+ t.Cleanup(func() {
+ var err error
+ if hadPrev {
+ err = os.Setenv("BASE_DIR", prev)
+ } else {
+ err = os.Unsetenv("BASE_DIR")
+ }
+ if err != nil {
+ t.Fatalf("restore BASE_DIR: %v", err)
+ }
+ })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config_test.go` around lines 12 - 15, The helper
setBaseDirEnv currently calls t.Setenv("BASE_DIR", value) which sets the
variable to an empty string rather than unsetting it; update setBaseDirEnv so
that when value == "" it calls os.Unsetenv("BASE_DIR") (and optionally registers
a cleanup via t.Cleanup if needed) otherwise call t.Setenv("BASE_DIR", value) so
tests that rely on os.LookupEnv behave correctly; refer to the setBaseDirEnv
helper to locate where to change behavior.
Comment on lines
+50
to
+108
| func withTimedSimAppSequence(t *testing.T, keys []timedSimKey) { | ||
| t.Helper() | ||
|
|
||
| decryptTUIE2EMu.Lock() | ||
| orig := newTUIApp | ||
| done := make(chan struct{}) | ||
| var injectWG sync.WaitGroup | ||
| t.Cleanup(func() { | ||
| close(done) | ||
| injectWG.Wait() | ||
| newTUIApp = orig | ||
| decryptTUIE2EMu.Unlock() | ||
| }) | ||
|
|
||
| screen := tcell.NewSimulationScreen("UTF-8") | ||
| if err := screen.Init(); err != nil { | ||
| t.Fatalf("screen.Init: %v", err) | ||
| } | ||
| screen.SetSize(120, 40) | ||
|
|
||
| var once sync.Once | ||
| newTUIApp = func() *tui.App { | ||
| app := tui.NewApp() | ||
| app.SetScreen(screen) | ||
|
|
||
| once.Do(func() { | ||
| injectWG.Add(1) | ||
| go func() { | ||
| defer injectWG.Done() | ||
|
|
||
| for _, k := range keys { | ||
| if k.Wait > 0 { | ||
| timer := time.NewTimer(k.Wait) | ||
| select { | ||
| case <-done: | ||
| if !timer.Stop() { | ||
| <-timer.C | ||
| } | ||
| return | ||
| case <-timer.C: | ||
| } | ||
| } | ||
| mod := k.Mod | ||
| if mod == 0 { | ||
| mod = tcell.ModNone | ||
| } | ||
| select { | ||
| case <-done: | ||
| return | ||
| default: | ||
| } | ||
| screen.InjectKey(k.Key, k.R, mod) | ||
| } | ||
| }() | ||
| }) | ||
|
|
||
| return app | ||
| } | ||
| } |
There was a problem hiding this comment.
The simulated input harness is time-coupled and already flakes in CI.
This sequence is driven entirely by wall-clock sleeps across successive RunWithContext apps, so any slower screen transition or focus change sends keys to the wrong view and the test hangs until the deadline. The reported TestRunDecryptWorkflowTUI_SuccessLocalEncrypted: context deadline exceeded is consistent with that. Drive each step from an observed screen state/focus transition instead of fixed delays.
Also applies to: 203-235
🤖 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 50 - 108,
The current withTimedSimAppSequence helper drives input by sleeping
(timedSimKey.Wait and timers in the inject goroutine) which causes flakes;
change the inject loop inside newTUIApp to drive each key by observing screen
state or focus transitions instead of wall-clock waits—use the tcell
SimulationScreen's current content/focus or a channel/event emitted by the app
to detect when the UI moved to the expected view before calling screen.InjectKey
for each entry in keys (keep the existing done, injectWG and timing-zero path
intact for immediate keys, but replace timer-based waits with waits for explicit
screen-state checks or a focus-change notification so keys are sent to the
correct view).
Comment on lines
+22
to
+24
| if ui == nil { | ||
| return nil, fmt.Errorf("backup selection UI not available") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, if an interface value holds a typed nil pointer (for example var x *T = nil; var v Interface = x), does v == nil evaluate to true or false?
💡 Result:
v == nil evaluates to false.
Reason: an interface value is only nil when both its dynamic type and dynamic value are nil. Assigning a typed nil pointer (*T(nil)) to an interface sets the dynamic type to *T, so the interface becomes (*T, nil), which is not equal to nil. [1], [2]
Sources:
[1] Dave Cheney, “Typed nils in Go”
[2] Go spec (interface equality rules, via Stack Overflow quoting the spec)
ui == nil will miss typed-nil implementations and allow panics.
In Go, an interface value is only nil when both its dynamic type and dynamic value are nil. If an interface holds a typed nil pointer (e.g., var x *SomeUI = nil; var v Interface = x), the check v == nil returns false, leaving the code vulnerable to nil-method-call panics. All three guards at lines 22–24, 187–189, and 205–207 are affected.
Use a helper that detects both nil interfaces and typed-nil values:
Defensive guard
func isNilInterface(v any) bool {
if v == nil {
return true
}
rv := reflect.ValueOf(v)
switch rv.Kind() {
case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
return rv.IsNil()
default:
return false
}
}Replace all three nil checks with if isNilInterface(ui) { ... } (and similarly for other interface parameters).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/orchestrator/decrypt_workflow_ui.go` around lines 22 - 24, The
nil-checks like `if ui == nil { ... }` in decrypt_workflow_ui.go can miss
typed-nil interface values and lead to panics; add a helper `isNilInterface(v
any) bool` (using reflect.ValueOf and rv.IsNil for kinds Chan, Func, Interface,
Map, Pointer, Slice) and replace all three guards that currently check `ui ==
nil` (and any other interface parameter checks in that file) with `if
isNilInterface(ui) { return nil, fmt.Errorf("backup selection UI not available")
}` (or the file's existing error handling) so both true nil and typed-nil
interfaces are detected safely.
Comment on lines
+386
to
+391
| func TestMaybeApplyPVEHAWithUI_BranchCoverage(t *testing.T) { | ||
| env := setupHATestEnv(t) | ||
| stageWithHA := env.stageRoot + "/etc/pve/ha/resources.cfg" | ||
| if err := env.fs.AddFile(stageWithHA, []byte("res\n")); err != nil { | ||
| t.Fatalf("add staged HA config: %v", err) | ||
| } |
There was a problem hiding this comment.
Set up a fresh HA env per subtest.
These parent-scoped fixtures share mutable package globals across all sibling t.Runs. If one child fails after overriding restoreFS, haApplyGeteuid, haIsMounted, haArmRollback, etc., later siblings inherit that state until the parent cleanup runs. Initializing the env inside each subtest, or wrapping each override in t.Cleanup, will keep failures isolated.
Also applies to: 649-653
🤖 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 386 - 391,
The test is creating one env and then running multiple t.Run subtests which
mutate package-level globals (restoreFS, haApplyGeteuid, haIsMounted,
haArmRollback, etc.) so later siblings inherit state; fix by creating a fresh HA
test environment inside each subtest (move env := setupHATestEnv(t) into each
t.Run) or, if you keep a shared parent, ensure every override of globals is
paired with t.Cleanup to restore the original value (capture originals before
assignment and restore them in t.Cleanup for functions like restoreFS,
haApplyGeteuid, haIsMounted, haArmRollback and any env.fs changes) so each
subtest runs with isolated state.
Comment on lines
+16
to
+20
| // bundlePathFor returns the canonical bundle path for either a raw archive path | ||
| // or a path that already points to a bundle. | ||
| func bundlePathFor(path string) string { | ||
| base, _ := trimBundleSuffix(path) | ||
| return base + bundleSuffix |
There was a problem hiding this comment.
Normalize base before building the non-bundle candidates.
If base is already something like backup.bundle.tar, this still emits backup.bundle.tar.sha256 / .metadata instead of the legacy siblings for backup. That means the new bundle normalization only fixes the bundle file itself; retention/cleanup will still miss the legacy sidecars for bundle inputs.
🛠️ Proposed fix
func buildBackupCandidatePaths(base string, includeBundle bool) []string {
+ base, _ = trimBundleSuffix(base)
+
seen := make(map[string]struct{})
add := func(path string) bool {
if path == "" {
return false
}Also applies to: 26-50
🤖 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 16 - 20, bundlePathFor
currently builds the bundle candidate from the raw path without fully
normalizing inputs that already include bundle suffixes, causing sidecars like
.sha256/.metadata to be emitted with the bundled filename; fix by fully
normalizing the input by calling trimBundleSuffix in a loop (or otherwise strip
any trailing bundleSuffix occurrences) to reduce the path to the true base
before appending bundleSuffix in bundlePathFor, and apply the same normalization
logic wherever non-bundle candidates are constructed (see the same pattern
around lines referenced in the review, functions/blocks that build legacy
sidecar names) so legacy sibling names are produced for bundle inputs.
Comment on lines
+380
to
+391
| if err := c.waitForRetry(timeoutCtx, waitTime); err != nil { | ||
| if parentErr := ctx.Err(); parentErr != nil { | ||
| return parentErr | ||
| } | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| return &remoteCheckError{ | ||
| kind: remoteErrorTimeout, | ||
| msg: fmt.Sprintf("connection timeout (%ds) - remote did not respond in time", timeoutSeconds), | ||
| err: err, | ||
| } | ||
| } | ||
| return err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Direct CloudStorage literals:"
rg -nP -C2 '(&\s*)?(storage\.)?CloudStorage\s*\{' --type=go
echo
echo "Explicit waitForRetry initializations:"
rg -n -C2 --type=go 'waitForRetry\s*:'Repository: tis24dev/proxsave
Length of output: 1340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the CloudStorage struct definition
echo "=== CloudStorage struct definition ==="
rg -n -A 20 'type CloudStorage struct' --type=go
echo
echo "=== All c.waitForRetry calls ==="
rg -n 'c\.waitForRetry' --type=go -A 1 -B 1
echo
echo "=== Check lines 380-391 and 796-802 ==="
sed -n '375,395p' internal/storage/cloud.go
echo "---"
sed -n '791,807p' internal/storage/cloud.goRepository: tis24dev/proxsave
Length of output: 3215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== waitForRetryContext function definition ==="
rg -n -B 2 -A 10 'func waitForRetryContext' --type=go
echo
echo "=== Check for external imports of CloudStorage ==="
rg -n 'CloudStorage' --type=go | grep -E '(import|from|use)' | head -20
echo
echo "=== All references to CloudStorage outside internal/storage ==="
git grep -n 'CloudStorage' -- '*.go' | grep -v 'internal/storage' | head -30
echo
echo "=== Check NewCloudStorage function signature ==="
sed -n '160,200p' internal/storage/cloud.goRepository: tis24dev/proxsave
Length of output: 749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All CloudStorage references across codebase ==="
rg -n 'CloudStorage' --type=go | head -40
echo
echo "=== Check if CloudStorage is used in other packages ==="
rg -l 'CloudStorage' --type=go | grep -v 'internal/storage'
echo
echo "=== Files that import from internal/storage ==="
rg -l 'from.*storage|internal/storage' --type=go | grep -v 'internal/storage'Repository: tis24dev/proxsave
Length of output: 4208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check orchestrator/extensions.go ==="
rg -n 'CloudStorage' internal/orchestrator/extensions.go -B 3 -A 3
echo
echo "=== Check cmd/proxsave files for CloudStorage struct usage ==="
for file in cmd/proxsave/main.go cmd/proxsave/install.go cmd/proxsave/install_test.go; do
if [ -f "$file" ]; then
echo "--- $file ---"
rg -n 'CloudStorage' "$file" -B 2 -A 2
fi
done
echo
echo "=== All direct CloudStorage{} constructions in entire codebase ==="
rg -n '\bCloudStorage\s*\{' --type=goRepository: tis24dev/proxsave
Length of output: 2369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if execCommand and lookPath have nil checks ==="
rg -n 'c\.execCommand|c\.lookPath' --type=go -B 2 -A 2
echo
echo "=== Check NewCloudStorage to see how execCommand/lookPath are initialized ==="
sed -n '175,200p' internal/storage/cloud.go
echo
echo "=== Verify if waitForRetry field is ever checked for nil ==="
rg -n 'waitForRetry.*!=' --type=go
</script>Repository: tis24dev/proxsave
Length of output: 2490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check git log for CloudStorage struct changes ==="
git log -p --follow -S 'waitForRetry' -- internal/storage/cloud.go | head -100
echo
echo "=== Check if there are any tests that construct CloudStorage directly ==="
rg -n 'CloudStorage\s*\{' internal/storage/cloud_test.go -B 3 -A 10Repository: tis24dev/proxsave
Length of output: 579
Add nil check wrapper for waitForRetry to match pattern used for other injectable hooks.
The field lacks the defensive nil check that execCommand and lookPath have (lines 1762-1765 and 295-297 respectively). While all current CloudStorage instances initialize waitForRetry via NewCloudStorage(), the exported struct can technically be constructed directly, which would cause a panic on the first retry. Consistency with how other injectable fields are handled warrants adding a wrapper method.
🛠️ Suggested fix
+func (c *CloudStorage) retryWait(ctx context.Context, d time.Duration) error {
+ if c.waitForRetry != nil {
+ return c.waitForRetry(ctx, d)
+ }
+ return waitForRetryContext(ctx, d)
+}
+
// ...
- if err := c.waitForRetry(timeoutCtx, waitTime); err != nil {
+ if err := c.retryWait(timeoutCtx, waitTime); err != nil {
if parentErr := ctx.Err(); parentErr != nil {
return parentErr
}
if errors.Is(err, context.DeadlineExceeded) {
return &remoteCheckError{
kind: remoteErrorTimeout,
msg: fmt.Sprintf("connection timeout (%ds) - remote did not respond in time", timeoutSeconds),
err: err,
}
}
return err
}
// ...
- if err := c.waitForRetry(ctx, waitTime); err != nil {
+ if err := c.retryWait(ctx, waitTime); err != nil {
return err
}Also applies to: 796-802
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/cloud.go` around lines 380 - 391, Add a defensive nil-check
wrapper for the injectable waitForRetry hook on CloudStorage similar to the
existing execCommand and lookPath wrappers: implement a method (e.g.,
callWaitForRetry or waitForRetrySafe) on CloudStorage that checks if
c.waitForRetry != nil and if so calls it with the same signature, otherwise
returns nil; then replace direct calls to c.waitForRetry(...) (used around the
retry handling in wait logic and the other location) with this new wrapper to
avoid panics when the exported struct is constructed without that hook (see
NewCloudStorage, waitForRetry, execCommand, lookPath for the existing pattern).
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.
@internal/tui/wizard/telegram_setup_tui.goaround lines 281 - 291, The ESC key handler reads result.Verified without holding the mutex that checkHandler uses; acquire the same mutex that guards result.Verified (i.e., the mutex used in checkHandler/telegramSetupQueueUpdateDraw) before reading result.Verified in the app.SetInputCapture callback, then release it immediately (use a brief lock/unlock or defer unlock) and then call doClose(true/false) based on the protected value; ensure you reference the same mutex variable used by checkHandler to avoid introducing a new synchronization primitive.Summary by CodeRabbit
Release Notes
New Features
Improvements