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%)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an internal AGE encryption setup flow with UI/CLI orchestration; refactors install/new-install and cron scheduling; introduces Telegram bootstrap and CLI/TUI flows; centralizes secondary-path validation and env mutation; adds safefs Statfs helpers, GFS retention normalization, cloud backoff, context-aware notify retries, TUI screen helpers, many tests, and defensive guards. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/TUI
participant Orch as Orchestrator
participant UI as AgeSetupUI
participant FS as FileSystem
participant Age as Age Library
CLI->>Orch: runInitialEncryptionSetupWithUI(ctx, configPath, ui)
Orch->>Orch: load config, create orchestrator
Orch->>Orch: prepareAgeRecipientsWithUI(ctx, ui)
alt cached recipients available
Orch-->>CLI: return cached result
else interactive setup required
Orch->>FS: stat recipient file
alt file exists
Orch->>UI: ConfirmOverwriteExistingRecipient(ctx, path)
UI-->>Orch: confirmed?
alt yes
Orch->>FS: backup existing recipient file
else no
Orch-->>CLI: abort (ErrAgeRecipientSetupAborted)
end
end
loop collect drafts
Orch->>UI: CollectRecipientDraft(ctx, path)
UI-->>Orch: draft
Orch->>Age: resolveAgeRecipientDraft(draft) -> recipient string
Orch->>UI: ConfirmAddAnotherRecipient(ctx, count)
UI-->>Orch: addMore?
end
Orch->>FS: write deduplicated recipient file atomically
FS-->>Orch: write success
Orch-->>CLI: encryptionSetupResult
end
sequenceDiagram
participant CLI as Install CLI
participant Config as ExistingConfigDecision
participant Wizard as Config Wizard
participant Post as Post-Install
CLI->>Config: prepareExistingConfigDecisionCLI(ctx, configPath)
Config->>Config: promptExistingConfigModeCLI(ctx, configPath)
Config-->>CLI: existingConfigDecision (mode, baseTemplate, skipWizard, abort)
alt Overwrite or Edit
CLI->>Wizard: runConfigWizardCLI(ctx) -> installConfigResult
Wizard-->>CLI: plan (EnableEncryption, SkipConfigWizard, CronSchedule)
CLI->>Post: runPostInstallSteps(using cronSchedule)
else Keep & Continue
CLI->>Post: runPostInstallSteps(skip config-dependent steps)
else Cancel
CLI-->>CLI: abort install (errInteractiveAborted)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Propagate the caller-selected TUI page builder through shared decrypt prompts so restore flows preserve their restore-specific wrapper instead of falling back to the default decrypt screen. Fix the remaining SelectExportNode bypass, introduce a shared internal/tui screen shell, and migrate the main orchestrator and wizard TUI layouts to it without changing behavior. Strengthen tests for builder propagation, nested prompt handoff, and exact builder parameters, and clean up BuildScreen footer handling by normalizing optional footer values once before rendering. LiveReview Pre-Commit Check: skipped (iter:3, coverage:83%)
Fix FakeFS path projection in tests so upward traversal like "../x" cannot escape the sandbox root, while preserving the expected mapping of absolute paths under the fake filesystem. Add targeted coverage for both absolute-path mapping and traversal blocking. Also propagate caller-selected TUI page builders through shared decrypt prompts, fix the remaining restore SelectExportNode bypass, introduce a shared internal/tui screen shell, migrate the main orchestrator and wizard TUI layouts to it without changing behavior, and strengthen tests around builder propagation and optional footer rendering. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Fix FakeFS test path mapping so "../x" cannot escape the sandbox root, and stabilize decrypt TUI e2e tests by waiting for the actual RunDecryptWorkflowTUI result with a local watchdog timeout instead of racing ctx.Done(). Also preserve restore-specific TUI page builders in shared decrypt prompts, remove the remaining SelectExportNode builder bypass, unify common TUI screen shells, and strengthen related tests. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Update the decrypt TUI abort e2e input sequence to use the same initial startup delays as the stabilized success path, reducing flakiness on slower CI runs without changing the covered abort behavior. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Remove the redundant logger nil check inside BootstrapLogger.Flush and rely on the existing early return that already validates both the bootstrap logger and the target logger before flushing entries. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add a small shared test helper for the preserved entries slice in new_install_test.go and use it across all ConfirmNewInstall tests to centralize the repeated fixture values. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Harden verifyBinaryIntegrity against executable swap races by validating SameFile before applying ownership or permission fixes, then performing any auto-fix operations on the already-opened executable via file descriptor using fd-based chmod/chown. Add targeted test coverage for the new fd-based permission-fix path. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Extract the repeated archive sidecar file-writing setup in bundle_test.go into a shared test helper and reuse it across the createBundle tests to centralize fixture creation without changing test behavior. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add a small test helper to register and restore the global ageWizardRunner via t.Cleanup, and reuse it across age_ui_adapter_test.go to centralize repeated setup and teardown logic. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add shared AGE setup type constants in the wizard package and use them across the AGE setup wizard, UI adapter, and related tests to remove duplicated string literals and keep the setup-type mapping aligned. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add a comment to TestPromptExistingPathDecisionTUI_NewPathContextCanceledWhileRunning explaining that the test waits for two drawCh reads (initial prompt and the secondary "new path" dialog) and cancels the context only after both reads to simulate cancellation while the second dialog is running. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Remove the separate lockDecryptTUIE2E helper and acquire the decryptTUIE2EMu mutex inside withTimedSimAppSequence instead. Tests no longer call the lock helper directly; the helper now locks at start and unlocks in the sequence cleanup goroutine. This centralizes mutex handling for decrypt TUI end-to-end tests and reduces duplicated setup/teardown logic (changes in internal/orchestrator/decrypt_tui_e2e_helpers_test.go and decrypt_tui_e2e_test.go). LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/wizard/telegram_setup_tui.go (1)
103-112:⚠️ Potential issue | 🟡 MinorEscape the interpolated values in
serverIDView.This view has
SetDynamicColors(true), so any persisted identity path containing[or]will be parsed as tview markup instead of literal text. Usetview.Escape()on the interpolated values.🛠️ Suggested fix
- serverIDLine := fmt.Sprintf("[yellow]%s[white]", result.ServerID) + serverIDLine := fmt.Sprintf("[yellow]%s[white]", tview.Escape(result.ServerID)) identityLine := "" if result.IdentityFile != "" { persisted := "not persisted" if result.IdentityPersisted { persisted = "persisted" } - identityLine = fmt.Sprintf("\n[gray]Identity file:[white] %s ([yellow]%s[white])", result.IdentityFile, persisted) + identityLine = fmt.Sprintf("\n[gray]Identity file:[white] %s ([yellow]%s[white])", tview.Escape(result.IdentityFile), persisted) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/telegram_setup_tui.go` around lines 103 - 112, The serverIDView text concatenation uses tview dynamic colors and currently inserts raw values (result.ServerID and result.IdentityFile/persisted) which allows '[' or ']' in those values to be parsed as markup; update the construction of serverIDLine and identityLine to wrap any interpolated user strings with tview.Escape() (e.g., escape result.ServerID, result.IdentityFile and the persisted string) before formatting, then call serverIDView.SetText with the escaped strings so SetDynamicColors(true) no longer misinterprets literal brackets.
🧹 Nitpick comments (3)
internal/tui/wizard/age.go (2)
277-283: Inconsistent indentation in form setup.Lines 277-283 have inconsistent indentation - there appears to be an extra tab before
form.Form.AddFormItem(privateKeyField)and the comment/initialization on lines 279-280 compared to the surrounding code.♻️ Fix indentation
- form.Form.AddFormItem(privateKeyField) + form.Form.AddFormItem(privateKeyField) - // Initialize with "existing" type selected - setupType = ageSetupTypeExisting + // Initialize with "existing" type selected + setupType = ageSetupTypeExisting🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/age.go` around lines 277 - 283, The block initializing the form has inconsistent indentation: align the lines starting with form.Form.AddFormItem(privateKeyField), the comment/initialization of setupType = ageSetupTypeExisting, and the three SetDisabled calls (passphraseField.SetDisabled, passphraseConfirmField.SetDisabled, privateKeyField.SetDisabled) with the surrounding code block indentation (remove the extra tab before form.Form.AddFormItem and ensure the comment and setupType line use the same indent level), so the entire form setup block is consistently indented.
286-314: Inconsistent indentation in submit handler.The submit handler body has inconsistent indentation compared to Go conventions. The switch statement and its cases appear to have extra indentation.
♻️ Fix indentation
form.SetOnSubmit(func(values map[string]string) error { - data.SetupType = setupType + data.SetupType = setupType - switch setupType { - case ageSetupTypeExisting: - publicKey, err := validatePublicKey(publicKeyField.GetText()) - if err != nil { - return err - } - data.PublicKey = publicKey - data.RecipientKey = publicKey + switch setupType { + case ageSetupTypeExisting: + publicKey, err := validatePublicKey(publicKeyField.GetText()) + if err != nil { + return err + } + data.PublicKey = publicKey + data.RecipientKey = publicKey - case ageSetupTypePassphrase: - passphrase, err := validatePassphrase(passphraseField.GetText(), passphraseConfirmField.GetText()) - if err != nil { - return err - } - data.Passphrase = passphrase + case ageSetupTypePassphrase: + passphrase, err := validatePassphrase(passphraseField.GetText(), passphraseConfirmField.GetText()) + if err != nil { + return err + } + data.Passphrase = passphrase - case ageSetupTypePrivateKey: - privateKey, err := validatePrivateKey(privateKeyField.GetText()) - if err != nil { - return err + case ageSetupTypePrivateKey: + privateKey, err := validatePrivateKey(privateKeyField.GetText()) + if err != nil { + return err } data.PrivateKey = privateKey }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/age.go` around lines 286 - 314, The submit handler registered with form.SetOnSubmit has inconsistent indentation around the switch and its cases; reformat the block so the switch setupType and each case (ageSetupTypeExisting, ageSetupTypePassphrase, ageSetupTypePrivateKey) and their error checks are aligned with standard Go indentation, ensuring the data assignments (data.PublicKey, data.RecipientKey, data.Passphrase, data.PrivateKey) and return nil are at the correct indentation level and braces close in the expected positions; update the form.SetOnSubmit anonymous function body that calls validatePublicKey, validatePassphrase, and validatePrivateKey to follow Go formatting conventions.internal/tui/screen_test.go (1)
11-25: Consider extending the helper to cover additional composite primitives.The
primitiveContainsTexthelper currently only handles*tview.TextViewand*tview.Flex. Other container types like*tview.Grid,*tview.Frame, or*tview.Pageswon't be traversed. This is fine for the currentBuildScreentest (which returns aFlexwithTextViews), but may cause false negatives if the implementation changes or the helper is reused elsewhere.♻️ Optional: Extend to handle more composite types
func primitiveContainsText(p tview.Primitive, want string) bool { switch v := p.(type) { case nil: return false case *tview.TextView: return strings.Contains(v.GetText(false), want) case *tview.Flex: for i := 0; i < v.GetItemCount(); i++ { if primitiveContainsText(v.GetItem(i), want) { return true } } + case *tview.Grid: + for i := 0; i < v.GetItemCount(); i++ { + if primitiveContainsText(v.GetItem(i), want) { + return true + } + } + case *tview.Pages: + for _, name := range v.GetPageNames(true) { + if page, _ := v.GetPage(name); page != nil { + if primitiveContainsText(page, want) { + return true + } + } + } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/screen_test.go` around lines 11 - 25, Extend primitiveContainsText to also recurse into other composite tview primitives by adding type switches for *tview.Grid, *tview.Frame, and *tview.Pages (in addition to the existing *tview.Flex and *tview.TextView cases); for each new case, iterate the container’s children using the container’s accessor methods and call primitiveContainsText recursively on each child so nested TextViews are discovered even when wrapped in Grid, Frame, or Pages.
🤖 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/orchestrator/workflow_ui_tui_decrypt_prompts.go`:
- Around line 209-218: The handler passed to form.SetOnSubmit currently trims
the submitted secret and thus mutates valid passphrases and misinterprets inputs
like " 0 " as abort; change the logic in the form.SetOnSubmit function so you
only use strings.TrimSpace on a temporary variable for validation (to check
emptiness and to compare to "0"), but assign the untrimmed original
values[label] to secret when setting it; keep the cancelled = true branch using
the trimmed check and ensure the error for empty input uses the trimmed copy.
In `@internal/tui/screen.go`:
- Around line 59-76: The footer uses dynamic tview color tags but inserts raw
values into the markup for configPath and buildSig, which can break rendering if
they contain '[' or ']'; update the creation of configPathText and buildSigText
to wrap the variable parts with tview.Escape(...) (i.e., use
tview.Escape(configPath) and tview.Escape(buildSig) inside the fmt.Sprintf
before calling SetDynamicColors(true)) so that any brackets are escaped and the
footer renders safely.
In `@internal/tui/wizard/install.go`:
- Around line 65-67: Update the top-level runner to accept and use the provided
context: change the runInstallWizardRunner signature to accept ctx
context.Context (e.g., func(ctx context.Context, app *tui.App, root, focus
tview.Primitive) error) and replace the blocking app.Run() call with a
context-aware run (preferably app.RunWithContext(ctx) if available, otherwise
start app.Run() in a goroutine and cancel/wait on ctx.Done()). Ensure all
callers (including RunInstallWizard and the other runner instance referenced
around the other occurrence) pass the same ctx through to runInstallWizardRunner
so the TUI exits immediately when the workflow context is canceled.
---
Outside diff comments:
In `@internal/tui/wizard/telegram_setup_tui.go`:
- Around line 103-112: The serverIDView text concatenation uses tview dynamic
colors and currently inserts raw values (result.ServerID and
result.IdentityFile/persisted) which allows '[' or ']' in those values to be
parsed as markup; update the construction of serverIDLine and identityLine to
wrap any interpolated user strings with tview.Escape() (e.g., escape
result.ServerID, result.IdentityFile and the persisted string) before
formatting, then call serverIDView.SetText with the escaped strings so
SetDynamicColors(true) no longer misinterprets literal brackets.
---
Nitpick comments:
In `@internal/tui/screen_test.go`:
- Around line 11-25: Extend primitiveContainsText to also recurse into other
composite tview primitives by adding type switches for *tview.Grid,
*tview.Frame, and *tview.Pages (in addition to the existing *tview.Flex and
*tview.TextView cases); for each new case, iterate the container’s children
using the container’s accessor methods and call primitiveContainsText
recursively on each child so nested TextViews are discovered even when wrapped
in Grid, Frame, or Pages.
In `@internal/tui/wizard/age.go`:
- Around line 277-283: The block initializing the form has inconsistent
indentation: align the lines starting with
form.Form.AddFormItem(privateKeyField), the comment/initialization of setupType
= ageSetupTypeExisting, and the three SetDisabled calls
(passphraseField.SetDisabled, passphraseConfirmField.SetDisabled,
privateKeyField.SetDisabled) with the surrounding code block indentation (remove
the extra tab before form.Form.AddFormItem and ensure the comment and setupType
line use the same indent level), so the entire form setup block is consistently
indented.
- Around line 286-314: The submit handler registered with form.SetOnSubmit has
inconsistent indentation around the switch and its cases; reformat the block so
the switch setupType and each case (ageSetupTypeExisting,
ageSetupTypePassphrase, ageSetupTypePrivateKey) and their error checks are
aligned with standard Go indentation, ensuring the data assignments
(data.PublicKey, data.RecipientKey, data.Passphrase, data.PrivateKey) and return
nil are at the correct indentation level and braces close in the expected
positions; update the form.SetOnSubmit anonymous function body that calls
validatePublicKey, validatePassphrase, and validatePrivateKey to follow Go
formatting conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93b0635d-4042-4a06-b575-9485dc09252c
📒 Files selected for processing (28)
internal/logging/bootstrap.gointernal/orchestrator/bundle_test.gointernal/orchestrator/decrypt_tui.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_tui_e2e_test.gointernal/orchestrator/deps_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/tui_screen_env.gointernal/orchestrator/tui_simulation_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/security/security.gointernal/security/security_test.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/new_install.gointernal/tui/wizard/new_install_test.gointernal/tui/wizard/post_install_audit_tui.gointernal/tui/wizard/screen.gointernal/tui/wizard/telegram_setup_tui.go
✅ Files skipped from review due to trivial changes (5)
- internal/orchestrator/workflow_ui_tui_restore.go
- internal/logging/bootstrap.go
- internal/orchestrator/decrypt_tui_e2e_test.go
- internal/orchestrator/workflow_ui_tui_decrypt_test.go
- internal/orchestrator/decrypt_tui_e2e_helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/orchestrator/decrypt_tui.go
- internal/security/security_test.go
- internal/tui/wizard/age_test.go
- internal/tui/wizard/age_ui_adapter_test.go
- internal/tui/wizard/age_ui_adapter.go
Normalize indentation and alignment in RunAgeSetupWizard (internal/tui/wizard/age.go) for improved readability. This is a formatting-only change — no logic or behavior was modified. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Escape user- or runtime-provided strings in TUI views using tview.Escape to avoid accidental markup/formatting and ensure raw/visual text is correct. Changes apply across orchestrator and tui packages: network health and NIC messages, diagnostics paths, decrypt messages and prompts, screen config/build signature, AGE wizard modals and inputs, install/new-install confirmations, post-install audit details, and Telegram setup display. Also adds a test verifying bracketed server/identity values are escaped, and minor formatting cleanups in the AGE setup dropdown handling. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add a test to ensure passphrases with leading/trailing spaces are preserved and update the TUI decrypt prompt to stop trimming the input before returning it. The prompt now uses a trimmed copy for validation and for detecting the "0" cancel token, but returns the raw value so user-intended surrounding spaces are kept. Files changed: added test in decrypt_tui_simulation_test.go and adjusted logic in workflow_ui_tui_decrypt_prompts.go. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Introduce App.RunWithContext to allow running/stopping the TUI based on a context, using an atomic state to differentiate cancellations vs. Run errors. Update internal/tui/app.go imports and logic accordingly. Update install wizard to pass the context into runInstallWizardRunner and call RunWithContext (internal/tui/wizard/install.go), and adjust related tests to validate cancellation behavior and context propagation (internal/tui/abort_context_test.go, internal/tui/wizard/install_test.go). Minor test imports added (errors, tcell, context) and runInstallWizardRunner now sets root/focus before running with context. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/wizard/age.go (1)
167-169:⚠️ Potential issue | 🟡 MinorPass
ctxtoRunWithContextfor context-aware cancellation.The
ctx context.Contextparameter is accepted but never used inRunAgeSetupWizard. TheageWizardRunnershould be updated to accept and pass the context toapp.RunWithContext(ctx)instead ofapp.Run(), matching the pattern used inRunInstallWizardatinternal/tui/wizard/install.go:68. This ensures context cancellation actually stops the TUI wizard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/age.go` around lines 167 - 169, RunAgeSetupWizard currently ignores the ctx parameter; update ageWizardRunner and its invocation so the TUI uses context-aware execution. Modify the ageWizardRunner (and any function literal passed to tui.NewApp or app.Run) to accept a context parameter and call app.RunWithContext(ctx) instead of app.Run(), and then change RunAgeSetupWizard to pass the received ctx into ageWizardRunner so cancellation of the provided context stops the wizard.
🧹 Nitpick comments (2)
internal/tui/screen.go (1)
31-35: Note:HeaderTextis not escaped.The
spec.HeaderTextis rendered withSetDynamicColors(true)but isn't escaped. This is safe as long asHeaderTextis always set to static/trusted strings (as seen in current usage). If user-controlled data is ever passed here, escaping would be needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/screen.go` around lines 31 - 35, The header text is currently passed raw into welcomeText with SetDynamicColors(true) which allows color/control sequences; update the code that sets welcomeText to escape spec.HeaderText before setting it (e.g., use tview.Escape on spec.HeaderText) or, if dynamic colors are required for trusted static headers only, explicitly document and assert the trust boundary; modify the welcomeText construction (the call that uses spec.HeaderText together with SetDynamicColors(true) in the welcomeText variable) to apply escaping or disable dynamic colors to eliminate unescaped user-controlled input.internal/orchestrator/workflow_ui_tui_decrypt_prompts.go (1)
23-62: Consider usingapp.Stop()instead ofQueueEvent(Ctrl+C).The
runTUIAppWithContexthelper sends a Ctrl+C event to cancel the TUI, butApp.RunWithContextinapp.gousesapp.Stop()directly. For consistency and reliability (Ctrl+C handling depends on input handlers), consider aligning with the pattern inApp.RunWithContext.That said, if Ctrl+C is intentionally chosen to trigger specific cleanup behavior, this approach is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/workflow_ui_tui_decrypt_prompts.go` around lines 23 - 62, runTUIAppWithContext currently cancels the TUI by queueing a Ctrl+C key event; replace that with a direct call to app.Stop() for consistency with App.RunWithContext and more reliable shutdown. In the goroutine watching ctx.Done(), call app.Stop() (while keeping the atomic state transitions using the existing atomic.Int32 `state` and CompareAndSwap checks) instead of app.QueueEvent(tcell.NewEventKey(...)); ensure the same state values (0->1 for cancellation, 0->2 for normal stop) and the existing error-return logic in runTUIAppWithContext remain unchanged so behavior and return values still depend on `state` and ctx.Err().
🤖 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/tui/wizard/telegram_setup_tui.go`:
- Around line 22-25: The wizard currently only uses ctx for checkCtx so
cancellation doesn't stop Run(); update the telegram setup flow (where
telegramSetupBuildBootstrap, telegramSetupCheckRegistration,
telegramSetupQueueUpdateDraw and telegramSetupGo are used and where Run() is
invoked) to honor the parent ctx by either (a) wiring the wizard into the
context-aware runner so the screen's Run() is cancelled when ctx.Done() is
closed, or (b) spawn a goroutine that listens for ctx.Done() and calls the TUI
shutdown (stop the app / call app.Stop or equivalent) to unblock Run(); ensure
checkCtx use remains but add the ctx cancellation hook around the app.Run/Run()
path so the install context cancellation propagates upstream.
- Around line 103-115: The status text built from untrusted HTTP response bodies
is not escaped and can be interpreted as tview color markup (since statusView
uses SetDynamicColors), so update the two callers in
internal/notify/telegram_registration.go that build Message (including
string(body)) to escape untrusted parts before passing to setStatus (or before
calling statusView.SetText): wrap the raw response body (and any other untrusted
strings) with tview.Escape(...) and then compose the Message so only trusted
color tags remain; ensure you update the setStatus call sites and any direct
statusView.SetText usage to use the escaped string.
---
Outside diff comments:
In `@internal/tui/wizard/age.go`:
- Around line 167-169: RunAgeSetupWizard currently ignores the ctx parameter;
update ageWizardRunner and its invocation so the TUI uses context-aware
execution. Modify the ageWizardRunner (and any function literal passed to
tui.NewApp or app.Run) to accept a context parameter and call
app.RunWithContext(ctx) instead of app.Run(), and then change RunAgeSetupWizard
to pass the received ctx into ageWizardRunner so cancellation of the provided
context stops the wizard.
---
Nitpick comments:
In `@internal/orchestrator/workflow_ui_tui_decrypt_prompts.go`:
- Around line 23-62: runTUIAppWithContext currently cancels the TUI by queueing
a Ctrl+C key event; replace that with a direct call to app.Stop() for
consistency with App.RunWithContext and more reliable shutdown. In the goroutine
watching ctx.Done(), call app.Stop() (while keeping the atomic state transitions
using the existing atomic.Int32 `state` and CompareAndSwap checks) instead of
app.QueueEvent(tcell.NewEventKey(...)); ensure the same state values (0->1 for
cancellation, 0->2 for normal stop) and the existing error-return logic in
runTUIAppWithContext remain unchanged so behavior and return values still depend
on `state` and ctx.Err().
In `@internal/tui/screen.go`:
- Around line 31-35: The header text is currently passed raw into welcomeText
with SetDynamicColors(true) which allows color/control sequences; update the
code that sets welcomeText to escape spec.HeaderText before setting it (e.g.,
use tview.Escape on spec.HeaderText) or, if dynamic colors are required for
trusted static headers only, explicitly document and assert the trust boundary;
modify the welcomeText construction (the call that uses spec.HeaderText together
with SetDynamicColors(true) in the welcomeText variable) to apply escaping or
disable dynamic colors to eliminate unescaped user-controlled input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d8d0d6a-f36a-41f7-8016-df08c4a2303b
📒 Files selected for processing (14)
internal/orchestrator/decrypt_tui_simulation_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/workflow_ui_tui_decrypt.gointernal/orchestrator/workflow_ui_tui_decrypt_prompts.gointernal/tui/abort_context_test.gointernal/tui/app.gointernal/tui/screen.gointernal/tui/wizard/age.gointernal/tui/wizard/install.gointernal/tui/wizard/install_test.gointernal/tui/wizard/new_install.gointernal/tui/wizard/post_install_audit_tui.gointernal/tui/wizard/telegram_setup_tui.gointernal/tui/wizard/telegram_setup_tui_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/wizard/post_install_audit_tui.go
- internal/orchestrator/decrypt_tui_simulation_test.go
Comment on lines
+103
to
115
| escapedServerID := tview.Escape(result.ServerID) | ||
| serverIDLine := fmt.Sprintf("[yellow]%s[white]", escapedServerID) | ||
| identityLine := "" | ||
| if result.IdentityFile != "" { | ||
| persisted := "not persisted" | ||
| if result.IdentityPersisted { | ||
| persisted = "persisted" | ||
| } | ||
| identityLine = fmt.Sprintf("\n[gray]Identity file:[white] %s ([yellow]%s[white])", result.IdentityFile, persisted) | ||
| escapedIdentityFile := tview.Escape(result.IdentityFile) | ||
| escapedPersisted := tview.Escape(persisted) | ||
| identityLine = fmt.Sprintf("\n[gray]Identity file:[white] %s ([yellow]%s[white])", escapedIdentityFile, escapedPersisted) | ||
| } | ||
| serverIDView.SetText(serverIDLine + identityLine) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its structure
find . -name "telegram_setup_tui.go" -type fRepository: tis24dev/proxsave
Length of output: 104
🏁 Script executed:
# Search for setStatus calls in the file
rg "setStatus\(" internal/tui/wizard/telegram_setup_tui.go -A 2 -B 1Repository: tis24dev/proxsave
Length of output: 597
🏁 Script executed:
# Check for SetDynamicColors usage in the file
rg "SetDynamicColors" internal/tui/wizard/telegram_setup_tui.go -B 3 -A 1Repository: tis24dev/proxsave
Length of output: 424
🏁 Script executed:
# Look at telegram_registration.go to see how Message is constructed
find . -name "telegram_registration.go" -type fRepository: tis24dev/proxsave
Length of output: 103
🏁 Script executed:
# Check the statusView setup and how it's used
rg "statusView" internal/tui/wizard/telegram_setup_tui.go -B 2 -A 2Repository: tis24dev/proxsave
Length of output: 477
🏁 Script executed:
# Now examine telegram_registration.go to see how status.Message is built
cat ./internal/notify/telegram_registration.go | head -100Repository: tis24dev/proxsave
Length of output: 4088
🏁 Script executed:
# Get more of the file to understand the Message construction
wc -l ./internal/notify/telegram_registration.goRepository: tis24dev/proxsave
Length of output: 106
🏁 Script executed:
# Search for where Message is assigned in telegram_registration.go
rg "Message\s*:" ./internal/notify/telegram_registration.go -B 5 -A 2Repository: tis24dev/proxsave
Length of output: 2565
🏁 Script executed:
# Look at the exact context around the two setStatus calls with untrusted data
rg "Linked successfully" ./internal/tui/wizard/telegram_setup_tui.go -B 10 -A 5Repository: tis24dev/proxsave
Length of output: 528
🏁 Script executed:
# Get more context around the truncate call
rg "truncate\(msg" ./internal/tui/wizard/telegram_setup_tui.go -B 15 -A 5Repository: tis24dev/proxsave
Length of output: 666
Escape untrusted status text to prevent markup injection.
The statusView is created with SetDynamicColors(true), so markup characters like [ and ] are interpreted as color tags. In internal/notify/telegram_registration.go, the unexpected-status path constructs Message by including the raw HTTP response body via string(body). If that response contains markup characters, they will corrupt the status display.
Both setStatus() calls that use untrusted data need escaping:
Proposed fix
- setStatus(fmt.Sprintf("[green]✓ Linked successfully.[white]\n\n%s", status.Message))
+ setStatus(fmt.Sprintf("[green]✓ Linked successfully.[white]\n\n%s", tview.Escape(status.Message)))
…
- setStatus(fmt.Sprintf("[yellow]%s[white]%s", truncate(msg, 300), hint))
+ setStatus(fmt.Sprintf("[yellow]%s[white]%s", tview.Escape(truncate(msg, 300)), hint))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tui/wizard/telegram_setup_tui.go` around lines 103 - 115, The status
text built from untrusted HTTP response bodies is not escaped and can be
interpreted as tview color markup (since statusView uses SetDynamicColors), so
update the two callers in internal/notify/telegram_registration.go that build
Message (including string(body)) to escape untrusted parts before passing to
setStatus (or before calling statusView.SetText): wrap the raw response body
(and any other untrusted strings) with tview.Escape(...) and then compose the
Message so only trusted color tags remain; ensure you update the setStatus call
sites and any direct statusView.SetText usage to use the escaped string.
Change ageWizardRunner to accept a context.Context and use App.RunWithContext. Update callers to pass context.Background() where no ctx is available and propagate the provided ctx in RunAgeSetupWizard. Adjust tests to the new runner signature and add TestRunAgeSetupWizard_PassesContextToRunner to verify context propagation. Also update registerAgeWizardRunner and related UI adapter tests to match the new signature. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Use tview.Escape on ScreenSpec.HeaderText before setting the TextView so literal characters like '['/']' aren't interpreted as tview markup. Added TestBuildScreenEscapesHeaderText to assert the header is rendered escaped. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Make TUI wizard runners context-aware: change runner signatures to accept context.Context and use app.RunWithContext(ctx) instead of Run. Update callers to pass the ctx through. Add tests verifying the context is propagated for post-install audit and Telegram setup wizards. Also stop the TUI app via app.Stop() when the orchestration context is done to ensure proper shutdown. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Pass context.Context through TUI prompt/runner APIs and use app.RunWithContext to honor cancellations/timeouts. Updated many orchestrator and wizard functions (prompts, restore flow, decrypt UI, network commit, installers) to accept a ctx parameter and forward it to the underlying TUI runner. Adjusted tests and stub helpers to include context. Removed the custom runTUIAppWithContext helper and an unused import, and replaced direct app.Run() calls with RunWithContext (or taskCtx where appropriate). This enables proper context cancellation handling for interactive UI flows. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Introduce context-aware variants for identity and filesystem operations and wire them through the install/upgrade/TUI flows. Added DetectWithContext, writeIdentityFileWithContext, setImmutableAttributeWithContext, clearImmutableAttributesWithContext and resetInstallBaseDirWithContext (with backward-compatible wrappers). Commands now use exec.CommandContext and operations check ctx.Err() so cancellations/timeouts are respected and propagated as errors. Updated callers in cmd/proxsave to pass context and added tests that validate cancellation behavior (TestResetInstallBaseDirWithContext_CanceledBeforeRemoval, TestSetImmutableAttributeWithContext_CanceledBeforeCommand). LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Prevent tview from interpreting bracketed text in status messages by escaping messages before rendering. Replace raw status.Message and truncated msg usage with tview.Escape in the telegram setup wizard UI. Add TestRunTelegramSetupWizard_EscapesBracketedStatusMessage to verify the view shows the escaped text in the raw view while preserving the literal message in plain text and LastStatusMessage. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/orchestrator/restore_tui.go (1)
364-375: Consider escaping error text to prevent unintended tview markup.The
compatErris interpolated directly into the message at line 365. If the error contains tview markup characters (e.g.,[red]or brackets), it could cause rendering issues. The same applies tocauseat line 378.Given the PR's pattern of escaping user-derived strings in
promptNetworkCommitTUI, consider applyingtview.Escape()here for consistency.💡 Suggested fix
func promptCompatibilityTUI(ctx context.Context, configPath, buildSig string, compatErr error) (bool, error) { - message := fmt.Sprintf("Compatibility check reported:\n\n[red]%v[white]\n\nContinuing may cause system instability.\n\nDo you want to continue anyway?", compatErr) + message := fmt.Sprintf("Compatibility check reported:\n\n[red]%s[white]\n\nContinuing may cause system instability.\n\nDo you want to continue anyway?", tview.Escape(compatErr.Error())) return promptYesNoTUIFunc(func promptContinueWithoutSafetyBackupTUI(ctx context.Context, configPath, buildSig string, cause error) (bool, error) { - message := fmt.Sprintf("Failed to create safety backup:\n\n[red]%v[white]\n\nWithout a safety backup, it will be harder to rollback changes.\n\nContinue without safety backup?", cause) + message := fmt.Sprintf("Failed to create safety backup:\n\n[red]%s[white]\n\nWithout a safety backup, it will be harder to rollback changes.\n\nContinue without safety backup?", tview.Escape(cause.Error())) return promptYesNoTUIFunc(🤖 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 364 - 375, The compatibility error string is interpolated directly into promptCompatibilityTUI's message (and similarly for the `cause` in the nearby code), which can trigger tview markup rendering if it contains brackets; update promptCompatibilityTUI to escape user-derived text before formatting the message by applying tview.Escape to the error (e.g., convert compatErr to a string and pass through tview.Escape) and do the same for any `cause` variable, following the pattern used in promptNetworkCommitTUI; ensure you still use promptYesNoTUIFunc with the escaped message so the UI displays raw error text safely.cmd/proxsave/install.go (1)
492-493: Redundant assignment.
result.SkipConfigWizard = falseis unnecessary since the zero value ofboolis alreadyfalse.♻️ Proposed fix
result.CronSchedule = cronutil.TimeToSchedule(cronTime) - result.SkipConfigWizard = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install.go` around lines 492 - 493, The assignment result.SkipConfigWizard = false is redundant because the bool zero value is false; remove that line from the install.go diff where result.CronSchedule is set (target the result variable and its SkipConfigWizard field) so the field remains at its zero value. Ensure no other code relies on that explicit assignment (run tests or search for places that expect an explicit write) and keep only the cron assignment cronutil.TimeToSchedule(cronTime).
🤖 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/tui/wizard/new_install.go`:
- Line 39: ConfirmNewInstall currently drops caller cancellation by using
context.Background(); change its signature to accept a context.Context (e.g.,
func ConfirmNewInstall(ctx context.Context, baseDir string, buildSig string,
preservedEntries []string) (bool, error)) and pass that ctx into
confirmNewInstallRunner instead of context.Background(); update all callers of
ConfirmNewInstall to forward their context so timeouts/cancellations propagate,
and adjust any tests/mocks to supply a context as well.
- Around line 27-29: The current code unconditionally appends a "/" to each
preservedEntries item (variable trimmed), which incorrectly turns regular files
like "backup.env" into "backup.env/"; update the logic so you only append a
trailing slash when the entry is actually a directory: for each preservedEntries
entry, check the filesystem with os.Stat(trimmed) and call fi.IsDir() (handling
the error by treating unknown entries as non-directories), and only then do
trimmed += "/". Keep the existing strings.HasSuffix(trimmed, "/") guard so you
don't double-append.
---
Nitpick comments:
In `@cmd/proxsave/install.go`:
- Around line 492-493: The assignment result.SkipConfigWizard = false is
redundant because the bool zero value is false; remove that line from the
install.go diff where result.CronSchedule is set (target the result variable and
its SkipConfigWizard field) so the field remains at its zero value. Ensure no
other code relies on that explicit assignment (run tests or search for places
that expect an explicit write) and keep only the cron assignment
cronutil.TimeToSchedule(cronTime).
In `@internal/orchestrator/restore_tui.go`:
- Around line 364-375: The compatibility error string is interpolated directly
into promptCompatibilityTUI's message (and similarly for the `cause` in the
nearby code), which can trigger tview markup rendering if it contains brackets;
update promptCompatibilityTUI to escape user-derived text before formatting the
message by applying tview.Escape to the error (e.g., convert compatErr to a
string and pass through tview.Escape) and do the same for any `cause` variable,
following the pattern used in promptNetworkCommitTUI; ensure you still use
promptYesNoTUIFunc with the escaped message so the UI displays raw error text
safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2efd2a25-56ac-4b3c-b4c6-1ea9d50d4a90
📒 Files selected for processing (28)
cmd/proxsave/install.gocmd/proxsave/install_test.gocmd/proxsave/install_tui.gocmd/proxsave/main.gocmd/proxsave/upgrade.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/orchestrator/network_apply_workflow_ui.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/restore_tui_test.gointernal/orchestrator/tui_simulation_test.gointernal/orchestrator/workflow_ui_tui_decrypt.gointernal/orchestrator/workflow_ui_tui_decrypt_prompts.gointernal/orchestrator/workflow_ui_tui_restore.gointernal/tui/screen.gointernal/tui/screen_test.gointernal/tui/wizard/age.gointernal/tui/wizard/age_test.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/telegram_setup_tui.gointernal/tui/wizard/telegram_setup_tui_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/tui/wizard/age_ui_adapter_test.go
- internal/orchestrator/workflow_ui_tui_restore.go
- internal/orchestrator/workflow_ui_tui_decrypt.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/proxsave/upgrade.go
- internal/tui/wizard/install_test.go
- cmd/proxsave/main.go
- internal/tui/wizard/age_test.go
- internal/tui/wizard/new_install_test.go
- internal/tui/wizard/install.go
- internal/tui/screen.go
Comment on lines
+27
to
+29
| if !strings.HasSuffix(trimmed, "/") { | ||
| trimmed += "/" | ||
| } |
There was a problem hiding this comment.
Don't force every preserved entry to look like a directory.
If preservedEntries includes regular files — e.g. the backup.env preservation mentioned in this PR — Lines 27-29 will render them as backup.env/, which is misleading in a destructive confirmation prompt.
Suggested fix
- if !strings.HasSuffix(trimmed, "/") {
- trimmed += "/"
- }
formatted = append(formatted, trimmed)📝 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 !strings.HasSuffix(trimmed, "/") { | |
| trimmed += "/" | |
| } | |
| formatted = append(formatted, trimmed) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tui/wizard/new_install.go` around lines 27 - 29, The current code
unconditionally appends a "/" to each preservedEntries item (variable trimmed),
which incorrectly turns regular files like "backup.env" into "backup.env/";
update the logic so you only append a trailing slash when the entry is actually
a directory: for each preservedEntries entry, check the filesystem with
os.Stat(trimmed) and call fi.IsDir() (handling the error by treating unknown
entries as non-directories), and only then do trimmed += "/". Keep the existing
strings.HasSuffix(trimmed, "/") guard so you don't double-append.
Escape errors inserted into TUI prompt messages to prevent tview markup interpretation. Use tview.Escape(fmt.Sprint(...)) for compatibility and safety-backup prompts (promptCompatibilityTUI, promptContinueWithoutSafetyBackupTUI). Add tests to verify bracketed text is escaped in both prompts. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Drop the explicit `result.SkipConfigWizard = false` in runConfigWizardCLI. The field's zero value is already false and the assignment was redundant and could overwrite a previously set value, so it's removed to clean up the code. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Add context propagation for the interactive new-install TUI: change ConfirmNewInstall and newInstallConfirmTUI signatures to accept a context.Context and use that context when invoking the TUI runner (instead of context.Background()). Update runNewInstall to pass the incoming ctx to the TUI confirmation. Update tests to match the new signatures and add tests that assert the context is passed through (TestRunNewInstallTUIPassesContextToConfirm, TestConfirmNewInstallPassesContextToRunner). LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Introduce indirection and test hooks to make HA restore and new-install code mockable, and expand unit test coverage. - Expose a newInstallBuildSignature variable so build signature can be overridden in tests and add multiple new_install tests (stdin handling, formatting, plan validation, TUI/CLI flows). - Add ha* function variables (haApplyGeteuid, haIsMounted, haIsRealRestoreFS, haArmRollback, haDisarmRollback, haApplyFromStage) and replace direct calls so HA restore behavior can be injected/mocked in tests. Use nowRestore() for time-sensitive behavior. - Add comprehensive restore_ha_additional_test.go with many tests covering rollback arming/disarming, apply-from-stage edge cases, error paths and commit/rollback outcomes. - Add TUI test helpers and extra tests (newSimulationApp, TestAppRunWithContext variants) and simplify RunWithContext error handling to prefer context errors. - Add small test helpers in wizard/new_install_test.go for overriding confirm runner. These changes primarily enable more robust testing and increase coverage around HA restore and new-install flows without changing core runtime behavior. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Documentation