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.
📝 WalkthroughWalkthroughAdds a UI-driven AGE encryption setup and orchestrator workflow, Telegram setup bootstrap and CLI, expanded install/new-install flows, stricter secondary-storage path validation, cron scheduling utilities, TUI decrypt prompt refactors, many supporting tests, and documentation updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/orchestrator/tui_simulation_test.go (1)
18-50:⚠️ Potential issue | 🟠 MajorStop using wall-clock sleeps in the shared TUI harness.
These fixed 50ms/10ms delays make every caller of
withSimAppSequencetiming-sensitive again. On slower CI or coverage runs, key injection can still race application startup, which reintroduces flakes in the exact paths this PR is trying to stabilize.#!/bin/bash # Inspect the shared harness and every test that depends on it. # Expected: if this helper is meant to be deterministic, it should not rely on fixed sleeps. rg -n -C2 'withSimAppSequence\(|time\.Sleep\(' internal/orchestrator internal/tui🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/tui_simulation_test.go` around lines 18 - 50, The helper withSimAppSequence must not use fixed time.Sleep delays; instead make newTUIApp synchronize with the app's event loop startup and only inject keys after a readiness signal. Modify newTUIApp (and the goroutine started there) to create a channel or sync.WaitGroup that is closed or done when the tui.App has started processing events (signal from where app.Run begins or immediately after app.SetScreen and any Start/Init call), then wait on that signal before calling screen.InjectKey in the loop; keep using screen.InjectKey and tcell.ModNone as before but remove the 50ms/10ms sleeps and replace them with waiting on that readiness channel so key injection is deterministic across CI and coverage runs.cmd/proxsave/install_tui.go (1)
121-145:⚠️ Potential issue | 🟠 MajorDon't commit
backup.envbefore blocking AGE setup succeeds.Line 121 writes the new config, and Lines 142-144 can still abort the install. If AGE setup is canceled or fails here, the user is left with a partially applied
backup.env(includingENCRYPT_ARCHIVE=true) but no usable recipient material. Please stage the config until setup completes or restore the previous file on error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_tui.go` around lines 121 - 145, The new config (written via writeConfigFile using configPath/tmpConfigPath) is committed before AGE setup runs, so if runInitialEncryptionSetupWithUI (triggered when !skipConfigWizard && wizardData != nil && wizardData.EnableEncryption) fails the repo is left with a partial backup.env; instead, postpone committing the new config/backup.env until the AGE setup completes successfully: write the config to a temporary file (tmpConfigPath) and only atomically replace configPath (and stage/commit backup.env) after setupResult is successful, or on any error restore the original configPath/backups (or delete the temp) and return the error; update the install flow around writeConfigFile, installSupportDocs and runInitialEncryptionSetupWithUI to reflect this transactional behavior.internal/tui/wizard/install.go (1)
727-745:⚠️ Potential issue | 🟠 MajorMake the default action non-destructive.
Overwriteis the first button added here. Intviewmodals, the first button (index 0) receives focus by default and triggering Enter activates it, making the TUI default look destructive while the CLI default isKeep & continue. MoveKeep & continueto the first position or callSetFocus(2)afterAddButtons()to match the safe CLI behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/install.go` around lines 727 - 745, The modal currently places "Overwrite" first so it receives default focus; update the button order in the AddButtons call in the modal builder (the chain using SetText(...).AddButtons(...).SetDoneFunc(...)) so that "Keep & continue" is the first entry (e.g., "Keep & continue", "Overwrite", "Edit existing", "Cancel") and adjust the SetDoneFunc switch cases to match the new labels' order (or alternatively call SetFocus with the index of "Keep & continue" after AddButtons); ensure the action variables (ExistingConfigKeepContinue, ExistingConfigOverwrite, ExistingConfigEdit, ExistingConfigCancel) are still set correctly and app.Stop() remains called at the end.
🧹 Nitpick comments (7)
cmd/proxsave/newkey_test.go (1)
15-36: MakecaptureNewKeyStdoutcleanup failure-safe.If
fn()panics or callst.FailNow,os.Stdoutnever gets restored and the pipe stays open, which can poison later tests in this package. Move the restore/close path into defers before invokingfn().Suggested hardening
-func captureNewKeyStdout(t *testing.T, fn func()) string { +func captureNewKeyStdout(t *testing.T, fn func()) (out string) { t.Helper() orig := os.Stdout r, w, err := os.Pipe() if err != nil { t.Fatalf("pipe: %v", err) } os.Stdout = w + defer func() { os.Stdout = orig }() + defer func() { _ = r.Close() }() var buf bytes.Buffer done := make(chan struct{}) go func() { _, _ = io.Copy(&buf, r) close(done) }() + defer func() { + _ = w.Close() + <-done + out = buf.String() + }() fn() - - _ = w.Close() - os.Stdout = orig - <-done - return buf.String() + return "" }🤖 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 15 - 36, The helper captureNewKeyStdout should make its cleanup fail-safe by moving restoration and pipe-close into defers before calling fn(); specifically, after creating r,w and swapping os.Stdout, register defers to (1) close w (to unblock the io.Copy goroutine) and (2) restore os.Stdout to orig, then start the io.Copy goroutine and invoke fn(); ensure the function still waits for the goroutine (<-done) before returning so the buffer is complete. Update captureNewKeyStdout to use these defers so panics or t.FailNow cannot leak the pipe or leave stdout altered.cmd/proxsave/new_install.go (1)
53-65: Move preserved-entry formatting out ofcmd/.This helper duplicates the trim/append-
//(none)rules now living ininternal/tui/wizard/new_install.go. Keeping two copies makes the CLI and TUI easy to drift apart again even though this PR is trying to unify their reset behavior. A tiny shared internal helper would keep that contract in one place.🤖 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 53 - 65, The formatting logic in formatNewInstallPreservedEntries duplicates behavior already implemented in internal/tui/wizard/new_install.go; extract this helper into a single shared internal helper (e.g., new function PreserveEntriesToDisplay or keep the name formatNewInstallPreservedEntries) under the internal package used by both cmd and tui, preserve the exact trim/skip-empty/append-"/"/"(none)"/join behavior, replace the local implementation in cmd/proxsave/new_install.go with an import and call to the new internal helper, and update the TUI code to use the same shared function so both consumers use one canonical implementation.internal/orchestrator/decrypt_tui_e2e_helpers_test.go (1)
222-243: Redundant timeout handling.The function uses both a context timeout (passed from caller, typically 18s) and an internal
time.After(20 * time.Second). Since the test already passes a context with timeout, thetime.Aftercase is redundant and may cause confusion about which timeout applies.♻️ Consider simplifying timeout handling
func runDecryptWorkflowTUIForTest(t *testing.T, ctx context.Context, cfg *config.Config, configPath string) error { t.Helper() logger := logging.New(types.LogLevelError, false) logger.SetOutput(io.Discard) errCh := make(chan error, 1) go func() { errCh <- RunDecryptWorkflowTUI(ctx, cfg, logger, "1.0.0", configPath, "test-build") }() select { case err := <-errCh: return err case <-ctx.Done(): t.Fatalf("RunDecryptWorkflowTUI context expired: %v", ctx.Err()) return nil - case <-time.After(20 * time.Second): - t.Fatalf("RunDecryptWorkflowTUI did not complete within 20s") - return nil } }🤖 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 222 - 243, The test helper runDecryptWorkflowTUIForTest currently mixes the caller-supplied context timeout with an extra time.After(20 * time.Second) branch; remove the redundant time.After case and its t.Fatalf so the select only waits for errCh or ctx.Done(), relying on the provided context timeout; keep the goroutine that calls RunDecryptWorkflowTUI and the errCh logic unchanged so failures and context cancellations are still handled correctly.internal/orchestrator/age_setup_ui.go (1)
13-18: Consider adding a method to zero sensitive fields.The
AgeRecipientDraftstruct holds sensitive data (Passphrase,PrivateKey). Callers should zero these fields after use to minimize exposure in memory. Consider adding aClear()method to help consumers follow this practice.♻️ Proposed Clear method
type AgeRecipientDraft struct { Kind AgeRecipientInputKind PublicKey string Passphrase string PrivateKey string } + +// Clear zeros sensitive fields to minimize memory exposure. +func (d *AgeRecipientDraft) Clear() { + if d == nil { + return + } + for i := range d.Passphrase { + d.Passphrase = d.Passphrase[:i] + "\x00" + d.Passphrase[i+1:] + } + for i := range d.PrivateKey { + d.PrivateKey = d.PrivateKey[:i] + "\x00" + d.PrivateKey[i+1:] + } + d.Passphrase = "" + d.PrivateKey = "" +}Note: Go strings are immutable, so true zeroing requires working with byte slices. The existing
resetStringhelper inencryption.gomay be a better pattern to follow for consistent handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/age_setup_ui.go` around lines 13 - 18, Add a Clear method to AgeRecipientDraft that zeroes sensitive fields (Passphrase and PrivateKey) after use; implement AgeRecipientDraft.Clear() to call the existing resetString helper from encryption.go (or follow its pattern) on both Passphrase and PrivateKey so callers can invoke draft.Clear() to reduce in-memory exposure while leaving other fields untouched.internal/orchestrator/workflow_ui_cli_test.go (1)
13-41: Good test helper with minor redundancy.The
captureCLIStdouthelper correctly captures stdout usingos.Pipe(). However, Line 39 (os.Stdout = oldStdout) is redundant since the cleanup function on Line 23 already restores stdout. This doesn't affect correctness since the cleanup runs after the function returns, but removing Line 39 would avoid the duplicate assignment.♻️ Optional: Remove redundant stdout restoration
_ = w.Close() <-done _ = r.Close() - os.Stdout = oldStdout return buf.String() }🤖 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 - 41, The helper captureCLIStdout restores os.Stdout twice: once via the t.Cleanup closure that resets os.Stdout = oldStdout and again explicitly at the end of the function; remove the redundant explicit assignment (os.Stdout = oldStdout) near the end of captureCLIStdout so the t.Cleanup closure is the single place that restores stdout, leaving the rest of the logic (os.Pipe, goroutine copying into buf, closing w/r and returning buf.String()) unchanged.cmd/proxsave/main.go (1)
1569-1570: Simplification looks correct, butcontinueis redundant.The direct receive from
ticker.Cis cleaner than the previousselectwith a single case. However, thecontinuestatement at the end of theforloop body is redundant since the loop would continue anyway.🔧 Optional: Remove redundant continue
<-ticker.C - continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/main.go` around lines 1569 - 1570, In the for loop in cmd/proxsave/main.go that directly receives from ticker.C (the line "<-ticker.C"), remove the redundant "continue" statement at the end of the loop body (the continue after the ticker receive) because the loop will naturally iterate without it; update any surrounding comments if present to reflect the simplified flow and ensure no other case fall-through relies on the continue.internal/orchestrator/age_setup_workflow.go (1)
53-79: Minor:resultgets overwritten, losing initial state.At line 53-56,
result.ReusedExistingRecipientsis set totruewhen existing recipients are found. However, at line 75,resultis completely overwritten withsetupResultfrom the wizard, which hasReusedExistingRecipients: false. This could misrepresent the state if the wizard adds recipients on top of existing ones (though this path seems unlikely given thelen(recipients) == 0check).This appears to be benign since the wizard only runs when
len(recipients) == 0, but consider whether the early assignment at lines 54-56 is actually needed.🧹 Simplify result initialization
- result := &AgeRecipientSetupResult{} - if len(recipients) > 0 && !o.forceNewAgeRecipient { - result.ReusedExistingRecipients = true - } - if len(recipients) == 0 { + result := &AgeRecipientSetupResult{} if ui == nil {Or move the conditional assignment after the wizard block to reflect the final state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/age_setup_workflow.go` around lines 53 - 79, The code sets result.ReusedExistingRecipients = true early (in the result variable created by AgeRecipientSetupResult) but then unconditionally overwrites result with setupResult returned from runAgeSetupWorkflow when len(recipients) == 0; to fix, either remove the early assignment entirely or move the ReusedExistingRecipients assignment to after the wizard branch so the final result reflects the real state (merge the boolean from the existing-recipients check with setupResult.ReusedExistingRecipients if you want to preserve both); update uses of result, recipients, runAgeSetupWorkflow, setupResult and o.cfg.AgeRecipientFile accordingly so the returned AgeRecipientSetupResult accurately represents whether existing recipients were reused.
🤖 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/schedule_helpers.go`:
- Around line 14-24: Validate and normalize CRON_SCHEDULE, CRON_HOUR and
CRON_MINUTE before returning and fall back to
cronutil.TimeToSchedule(cronutil.DefaultTime) on any invalid input: trim and
reject empty or multiline values (no '\n'), for CRON_SCHEDULE run it through a
cron parser/validator to ensure it is a valid 5-field schedule then return it
only if valid, and for CRON_HOUR/CRON_MINUTE parse them as integers, ensure hour
is 0–23 and minute is 0–59, then build and return fmt.Sprintf("%s %s * * *")
only when both are valid; otherwise return
cronutil.TimeToSchedule(cronutil.DefaultTime).
In `@internal/orchestrator/age_setup_workflow.go`:
- Around line 183-200: The current use of
resetString(&passphrase)/resetString(&privateKey) is ineffective for clearing
secrets; replace the string-based handling with byte-slice zeroing and explicit
clearing of the draft fields: for AgeRecipientInputPassphrase, build a trimmed
[]byte from draft.Passphrase (e.g. b :=
[]byte(strings.TrimSpace(draft.Passphrase))), defer zeroBytes(b), validate via
validatePassphraseStrength(b) (or convert as needed), call
deriveDeterministicRecipientFromPassphrase(string(b)) and then immediately call
zeroBytes(b) and set draft.Passphrase = "" before returning; do the same pattern
for AgeRecipientInputPrivateKey (use a []byte for draft.PrivateKey, defer
zeroBytes, call ParseAgePrivateKeyRecipient(string(b)), then zeroBytes and set
draft.PrivateKey = ""); remove/reset resetString usages and ensure you reference
zeroBytes, deriveDeterministicRecipientFromPassphrase,
ParseAgePrivateKeyRecipient, and the draft.Passphrase/draft.PrivateKey fields
when making these changes.
In `@internal/tui/wizard/age_ui_adapter.go`:
- Around line 23-25: Add a context-cancellation guard at the start of
ageSetupUIAdapter's ConfirmOverwriteExistingRecipient and
ConfirmAddAnotherRecipient: check if ctx.Err() != nil (or select on ctx.Done())
and if canceled return immediately (e.g., false, ctx.Err()) instead of calling
the blocking UI functions ConfirmRecipientOverwrite or
ConfirmAddAnotherRecipient; this ensures the methods on type ageSetupUIAdapter
respect the provided ctx before invoking blocking UI dialogs.
In `@internal/tui/wizard/install.go`:
- Around line 689-690: The current check uses os.Stat(configPath) and treats any
existing path as a valid config file; update the branch so after calling
os.Stat(configPath) you inspect the returned FileInfo (e.g., fi := <result>) and
reject non-regular files (use fi.Mode().IsRegular() or detect fi.IsDir()) before
presenting the "existing-config" dialog. If the path exists but is not a regular
file, handle it the same as "does not exist" or return an error to avoid
allowing directories or special files into the Edit existing / Keep & continue
flow; update the condition around the os.Stat(configPath) call and any code that
shows the existing-config dialog to enforce this guard.
---
Outside diff comments:
In `@cmd/proxsave/install_tui.go`:
- Around line 121-145: The new config (written via writeConfigFile using
configPath/tmpConfigPath) is committed before AGE setup runs, so if
runInitialEncryptionSetupWithUI (triggered when !skipConfigWizard && wizardData
!= nil && wizardData.EnableEncryption) fails the repo is left with a partial
backup.env; instead, postpone committing the new config/backup.env until the AGE
setup completes successfully: write the config to a temporary file
(tmpConfigPath) and only atomically replace configPath (and stage/commit
backup.env) after setupResult is successful, or on any error restore the
original configPath/backups (or delete the temp) and return the error; update
the install flow around writeConfigFile, installSupportDocs and
runInitialEncryptionSetupWithUI to reflect this transactional behavior.
In `@internal/orchestrator/tui_simulation_test.go`:
- Around line 18-50: The helper withSimAppSequence must not use fixed time.Sleep
delays; instead make newTUIApp synchronize with the app's event loop startup and
only inject keys after a readiness signal. Modify newTUIApp (and the goroutine
started there) to create a channel or sync.WaitGroup that is closed or done when
the tui.App has started processing events (signal from where app.Run begins or
immediately after app.SetScreen and any Start/Init call), then wait on that
signal before calling screen.InjectKey in the loop; keep using screen.InjectKey
and tcell.ModNone as before but remove the 50ms/10ms sleeps and replace them
with waiting on that readiness channel so key injection is deterministic across
CI and coverage runs.
In `@internal/tui/wizard/install.go`:
- Around line 727-745: The modal currently places "Overwrite" first so it
receives default focus; update the button order in the AddButtons call in the
modal builder (the chain using SetText(...).AddButtons(...).SetDoneFunc(...)) so
that "Keep & continue" is the first entry (e.g., "Keep & continue", "Overwrite",
"Edit existing", "Cancel") and adjust the SetDoneFunc switch cases to match the
new labels' order (or alternatively call SetFocus with the index of "Keep &
continue" after AddButtons); ensure the action variables
(ExistingConfigKeepContinue, ExistingConfigOverwrite, ExistingConfigEdit,
ExistingConfigCancel) are still set correctly and app.Stop() remains called at
the end.
---
Nitpick comments:
In `@cmd/proxsave/main.go`:
- Around line 1569-1570: In the for loop in cmd/proxsave/main.go that directly
receives from ticker.C (the line "<-ticker.C"), remove the redundant "continue"
statement at the end of the loop body (the continue after the ticker receive)
because the loop will naturally iterate without it; update any surrounding
comments if present to reflect the simplified flow and ensure no other case
fall-through relies on the continue.
In `@cmd/proxsave/new_install.go`:
- Around line 53-65: The formatting logic in formatNewInstallPreservedEntries
duplicates behavior already implemented in internal/tui/wizard/new_install.go;
extract this helper into a single shared internal helper (e.g., new function
PreserveEntriesToDisplay or keep the name formatNewInstallPreservedEntries)
under the internal package used by both cmd and tui, preserve the exact
trim/skip-empty/append-"/"/"(none)"/join behavior, replace the local
implementation in cmd/proxsave/new_install.go with an import and call to the new
internal helper, and update the TUI code to use the same shared function so both
consumers use one canonical implementation.
In `@cmd/proxsave/newkey_test.go`:
- Around line 15-36: The helper captureNewKeyStdout should make its cleanup
fail-safe by moving restoration and pipe-close into defers before calling fn();
specifically, after creating r,w and swapping os.Stdout, register defers to (1)
close w (to unblock the io.Copy goroutine) and (2) restore os.Stdout to orig,
then start the io.Copy goroutine and invoke fn(); ensure the function still
waits for the goroutine (<-done) before returning so the buffer is complete.
Update captureNewKeyStdout to use these defers so panics or t.FailNow cannot
leak the pipe or leave stdout altered.
In `@internal/orchestrator/age_setup_ui.go`:
- Around line 13-18: Add a Clear method to AgeRecipientDraft that zeroes
sensitive fields (Passphrase and PrivateKey) after use; implement
AgeRecipientDraft.Clear() to call the existing resetString helper from
encryption.go (or follow its pattern) on both Passphrase and PrivateKey so
callers can invoke draft.Clear() to reduce in-memory exposure while leaving
other fields untouched.
In `@internal/orchestrator/age_setup_workflow.go`:
- Around line 53-79: The code sets result.ReusedExistingRecipients = true early
(in the result variable created by AgeRecipientSetupResult) but then
unconditionally overwrites result with setupResult returned from
runAgeSetupWorkflow when len(recipients) == 0; to fix, either remove the early
assignment entirely or move the ReusedExistingRecipients assignment to after the
wizard branch so the final result reflects the real state (merge the boolean
from the existing-recipients check with setupResult.ReusedExistingRecipients if
you want to preserve both); update uses of result, recipients,
runAgeSetupWorkflow, setupResult and o.cfg.AgeRecipientFile accordingly so the
returned AgeRecipientSetupResult accurately represents whether existing
recipients were reused.
In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go`:
- Around line 222-243: The test helper runDecryptWorkflowTUIForTest currently
mixes the caller-supplied context timeout with an extra time.After(20 *
time.Second) branch; remove the redundant time.After case and its t.Fatalf so
the select only waits for errCh or ctx.Done(), relying on the provided context
timeout; keep the goroutine that calls RunDecryptWorkflowTUI and the errCh logic
unchanged so failures and context cancellations are still handled correctly.
In `@internal/orchestrator/workflow_ui_cli_test.go`:
- Around line 13-41: The helper captureCLIStdout restores os.Stdout twice: once
via the t.Cleanup closure that resets os.Stdout = oldStdout and again explicitly
at the end of the function; remove the redundant explicit assignment (os.Stdout
= oldStdout) near the end of captureCLIStdout so the t.Cleanup closure is the
single place that restores stdout, leaving the rest of the logic (os.Pipe,
goroutine copying into buf, closing w/r and returning buf.String()) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1274d3bd-6226-4581-921c-5687ae1b6a40
📒 Files selected for processing (68)
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/schedule_helpers.gocmd/proxsave/schedule_helpers_test.gocmd/proxsave/telegram_setup_cli.gocmd/proxsave/telegram_setup_cli_test.gocmd/proxsave/upgrade.godocs/CLI_REFERENCE.mddocs/CLOUD_STORAGE.mddocs/CONFIGURATION.mddocs/ENCRYPTION.mddocs/INSTALL.mdinternal/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/orchestrator/age_setup_ui.gointernal/orchestrator/age_setup_ui_cli.gointernal/orchestrator/age_setup_workflow.gointernal/orchestrator/age_setup_workflow_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/encryption.gointernal/orchestrator/encryption_exported_test.gointernal/orchestrator/telegram_setup_bootstrap.gointernal/orchestrator/telegram_setup_bootstrap_test.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_shared.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/telegram_setup_tui.gointernal/tui/wizard/telegram_setup_tui_test.go
💤 Files with no reviewable changes (2)
- internal/orchestrator/decrypt_tui_test.go
- internal/orchestrator/decrypt_tui.go
Comment on lines
14
to
+24
| if s := strings.TrimSpace(os.Getenv("CRON_SCHEDULE")); s != "" { | ||
| return s | ||
| } | ||
|
|
||
| hour := strings.TrimSpace(os.Getenv("CRON_HOUR")) | ||
| min := strings.TrimSpace(os.Getenv("CRON_MINUTE")) | ||
| if hour != "" && min != "" { | ||
| return fmt.Sprintf("%s %s * * *", min, hour) | ||
| } | ||
|
|
||
| // Default: 02:00 | ||
| return "0 2 * * *" | ||
| return cronutil.TimeToSchedule(cronutil.DefaultTime) |
There was a problem hiding this comment.
Validate env-derived cron values before returning them.
CRON_SCHEDULE, CRON_HOUR, and CRON_MINUTE are accepted without syntax/range checks. Invalid or multiline values can flow into cron migration and break scheduling behavior. Please normalize/validate and fall back to default when invalid.
🔧 Proposed hardening
import (
"fmt"
"os"
+ "strconv"
"strings"
cronutil "github.com/tis24dev/proxsave/internal/cron"
)
@@
func resolveCronScheduleFromEnv() string {
if s := strings.TrimSpace(os.Getenv("CRON_SCHEDULE")); s != "" {
- return s
+ parts := strings.Fields(s)
+ if len(parts) == 5 && !strings.ContainsAny(s, "\r\n") {
+ return strings.Join(parts, " ")
+ }
}
@@
hour := strings.TrimSpace(os.Getenv("CRON_HOUR"))
min := strings.TrimSpace(os.Getenv("CRON_MINUTE"))
if hour != "" && min != "" {
- return fmt.Sprintf("%s %s * * *", min, hour)
+ h, hErr := strconv.Atoi(hour)
+ m, mErr := strconv.Atoi(min)
+ if hErr == nil && mErr == nil && h >= 0 && h <= 23 && m >= 0 && m <= 59 {
+ return fmt.Sprintf("%02d %02d * * *", m, h)
+ }
}
return cronutil.TimeToSchedule(cronutil.DefaultTime)
}📝 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 s := strings.TrimSpace(os.Getenv("CRON_SCHEDULE")); s != "" { | |
| return s | |
| } | |
| hour := strings.TrimSpace(os.Getenv("CRON_HOUR")) | |
| min := strings.TrimSpace(os.Getenv("CRON_MINUTE")) | |
| if hour != "" && min != "" { | |
| return fmt.Sprintf("%s %s * * *", min, hour) | |
| } | |
| // Default: 02:00 | |
| return "0 2 * * *" | |
| return cronutil.TimeToSchedule(cronutil.DefaultTime) | |
| import ( | |
| "fmt" | |
| "os" | |
| "strconv" | |
| "strings" | |
| cronutil "github.com/tis24dev/proxsave/internal/cron" | |
| ) | |
| func resolveCronScheduleFromEnv() string { | |
| if s := strings.TrimSpace(os.Getenv("CRON_SCHEDULE")); s != "" { | |
| parts := strings.Fields(s) | |
| if len(parts) == 5 && !strings.ContainsAny(s, "\r\n") { | |
| return strings.Join(parts, " ") | |
| } | |
| } | |
| hour := strings.TrimSpace(os.Getenv("CRON_HOUR")) | |
| min := strings.TrimSpace(os.Getenv("CRON_MINUTE")) | |
| if hour != "" && min != "" { | |
| h, hErr := strconv.Atoi(hour) | |
| m, mErr := strconv.Atoi(min) | |
| if hErr == nil && mErr == nil && h >= 0 && h <= 23 && m >= 0 && m <= 59 { | |
| return fmt.Sprintf("%02d %02d * * *", m, h) | |
| } | |
| } | |
| return cronutil.TimeToSchedule(cronutil.DefaultTime) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/proxsave/schedule_helpers.go` around lines 14 - 24, Validate and
normalize CRON_SCHEDULE, CRON_HOUR and CRON_MINUTE before returning and fall
back to cronutil.TimeToSchedule(cronutil.DefaultTime) on any invalid input: trim
and reject empty or multiline values (no '\n'), for CRON_SCHEDULE run it through
a cron parser/validator to ensure it is a valid 5-field schedule then return it
only if valid, and for CRON_HOUR/CRON_MINUTE parse them as integers, ensure hour
is 0–23 and minute is 0–59, then build and return fmt.Sprintf("%s %s * * *")
only when both are valid; otherwise return
cronutil.TimeToSchedule(cronutil.DefaultTime).
Comment on lines
+183
to
+200
| case AgeRecipientInputPassphrase: | ||
| passphrase := strings.TrimSpace(draft.Passphrase) | ||
| defer resetString(&passphrase) | ||
| if passphrase == "" { | ||
| return "", fmt.Errorf("passphrase cannot be empty") | ||
| } | ||
| if err := validatePassphraseStrength([]byte(passphrase)); err != nil { | ||
| return "", err | ||
| } | ||
| recipient, err := deriveDeterministicRecipientFromPassphrase(passphrase) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return recipient, nil | ||
| case AgeRecipientInputPrivateKey: | ||
| privateKey := strings.TrimSpace(draft.PrivateKey) | ||
| defer resetString(&privateKey) | ||
| return ParseAgePrivateKeyRecipient(privateKey) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find resetString implementation to understand if it properly zeroes memory
ast-grep --pattern $'func resetString($_) {
$$$
}'Repository: tis24dev/proxsave
Length of output: 356
🏁 Script executed:
# Check the complete securemem.go file
cat -n internal/orchestrator/securemem.goRepository: tis24dev/proxsave
Length of output: 360
🏁 Script executed:
# Search for proper memory zeroing (crypto/subtle or unsafe)
rg -n "crypto/subtle|unsafe\.Memset|securezero|memset|bzero" --type goRepository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Look at resolveAgeRecipientDraft function signature and callers
ast-grep --pattern $'func resolveAgeRecipientDraft($_) {
$$$
}'Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Check where resolveAgeRecipientDraft is called
rg -n "resolveAgeRecipientDraft" --type goRepository: tis24dev/proxsave
Length of output: 278
🏁 Script executed:
# Find AgeRecipientDraft struct definition
rg -n "type AgeRecipientDraft" --type go -A 10Repository: tis24dev/proxsave
Length of output: 932
🏁 Script executed:
# Check the context where resolveAgeRecipientDraft is called (line 134)
sed -n '125,150p' internal/orchestrator/age_setup_workflow.goRepository: tis24dev/proxsave
Length of output: 625
🏁 Script executed:
# Check the full resolveAgeRecipientDraft function to see all cases
sed -n '171,210p' internal/orchestrator/age_setup_workflow.goRepository: tis24dev/proxsave
Length of output: 1233
Sensitive data clearing is ineffective and incomplete.
The current approach has a critical flaw: resetString() simply assigns an empty string (*s = "") rather than actually zeroing the memory. In Go, string assignment only changes the string header, leaving the original bytes intact in memory.
This means:
- The local
passphraseandprivateKeyvariables are cleared ineffectively - The
draft.Passphraseanddraft.PrivateKeyfields retain sensitive data after this function returns, with no mechanism to clear them - The suggested fix of calling
defer resetString(&draft.Passphrase)will not solve the issue sinceresetStringdoesn't perform true memory zeroing
For proper secure handling of sensitive strings, consider using zeroBytes() with []byte types instead of strings, or implement actual memory zeroing (e.g., using a loop to explicitly set each byte to 0, similar to the existing zeroBytes() function but adapted for strings via unsafe operations or by refactoring to use byte slices).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/orchestrator/age_setup_workflow.go` around lines 183 - 200, The
current use of resetString(&passphrase)/resetString(&privateKey) is ineffective
for clearing secrets; replace the string-based handling with byte-slice zeroing
and explicit clearing of the draft fields: for AgeRecipientInputPassphrase,
build a trimmed []byte from draft.Passphrase (e.g. b :=
[]byte(strings.TrimSpace(draft.Passphrase))), defer zeroBytes(b), validate via
validatePassphraseStrength(b) (or convert as needed), call
deriveDeterministicRecipientFromPassphrase(string(b)) and then immediately call
zeroBytes(b) and set draft.Passphrase = "" before returning; do the same pattern
for AgeRecipientInputPrivateKey (use a []byte for draft.PrivateKey, defer
zeroBytes, call ParseAgePrivateKeyRecipient(string(b)), then zeroBytes and set
draft.PrivateKey = ""); remove/reset resetString usages and ensure you reference
zeroBytes, deriveDeterministicRecipientFromPassphrase,
ParseAgePrivateKeyRecipient, and the draft.Passphrase/draft.PrivateKey fields
when making these changes.
Comment on lines
+23
to
+25
| func (a *ageSetupUIAdapter) ConfirmOverwriteExistingRecipient(ctx context.Context, recipientPath string) (bool, error) { | ||
| return ConfirmRecipientOverwrite(recipientPath, a.configPath, a.buildSig) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/tui/wizard/age_ui_adapter.go | head -100Repository: tis24dev/proxsave
Length of output: 2233
🏁 Script executed:
rg -n "func ConfirmRecipientOverwrite|func ConfirmAddRecipient" internal/tui/wizard/Repository: tis24dev/proxsave
Length of output: 293
🏁 Script executed:
sed -n '80,130p' internal/tui/wizard/age.goRepository: tis24dev/proxsave
Length of output: 1868
🏁 Script executed:
sed -n '130,160p' internal/tui/wizard/age.goRepository: tis24dev/proxsave
Length of output: 974
🏁 Script executed:
sed -n '159,210p' internal/tui/wizard/age.goRepository: tis24dev/proxsave
Length of output: 1810
🏁 Script executed:
sed -n '210,235p' internal/tui/wizard/age.goRepository: tis24dev/proxsave
Length of output: 770
🏁 Script executed:
rg -n "func ageWizardRunner" internal/tui/wizard/Repository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
rg -n "ageWizardRunner" internal/tui/wizard/Repository: tis24dev/proxsave
Length of output: 2945
🏁 Script executed:
sed -n '30,45p' internal/tui/wizard/age.goRepository: tis24dev/proxsave
Length of output: 515
Check context cancellation before opening blocking UI dialogs.
The ConfirmOverwriteExistingRecipient and ConfirmAddAnotherRecipient methods accept a ctx parameter but ignore it before calling UI functions that block waiting for user interaction. If the parent context is already canceled, the installation can still block waiting for user input instead of returning immediately.
Add a context cancellation check at the start of these methods:
Context cancellation guard
func (a *ageSetupUIAdapter) ConfirmOverwriteExistingRecipient(ctx context.Context, recipientPath string) (bool, error) {
+ if err := ctx.Err(); err != nil {
+ return false, orchestrator.ErrAgeRecipientSetupAborted
+ }
return ConfirmRecipientOverwrite(recipientPath, a.configPath, a.buildSig)
}
func (a *ageSetupUIAdapter) ConfirmAddAnotherRecipient(ctx context.Context, currentCount int) (bool, error) {
+ if err := ctx.Err(); err != nil {
+ return false, orchestrator.ErrAgeRecipientSetupAborted
+ }
return ConfirmAddRecipient(a.configPath, a.buildSig, currentCount)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tui/wizard/age_ui_adapter.go` around lines 23 - 25, Add a
context-cancellation guard at the start of ageSetupUIAdapter's
ConfirmOverwriteExistingRecipient and ConfirmAddAnotherRecipient: check if
ctx.Err() != nil (or select on ctx.Done()) and if canceled return immediately
(e.g., false, ctx.Err()) instead of calling the blocking UI functions
ConfirmRecipientOverwrite or ConfirmAddAnotherRecipient; this ensures the
methods on type ageSetupUIAdapter respect the provided ctx before invoking
blocking UI dialogs.
Comment on lines
689
to
690
| if _, err := os.Stat(configPath); err == nil { | ||
| // File exists, ask how to proceed |
There was a problem hiding this comment.
Reject non-regular config paths before showing the existing-config dialog.
This branch treats any existing path as a valid config file. Directories and special files can reach Edit existing or Keep & continue and then fail later in less predictable places, while the CLI path already rejects them up front.
Suggested guard
- if _, err := os.Stat(configPath); err == nil {
+ if info, err := os.Stat(configPath); err == nil {
+ if !info.Mode().IsRegular() {
+ return ExistingConfigCancel, fmt.Errorf("%s is not a regular file", configPath)
+ }
// File exists, ask how to proceed
app := tui.NewApp()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if _, err := os.Stat(configPath); err == nil { | |
| // File exists, ask how to proceed | |
| if info, err := os.Stat(configPath); err == nil { | |
| if !info.Mode().IsRegular() { | |
| return ExistingConfigCancel, fmt.Errorf("%s is not a regular file", configPath) | |
| } | |
| // File exists, ask how to proceed |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tui/wizard/install.go` around lines 689 - 690, The current check
uses os.Stat(configPath) and treats any existing path as a valid config file;
update the branch so after calling os.Stat(configPath) you inspect the returned
FileInfo (e.g., fi := <result>) and reject non-regular files (use
fi.Mode().IsRegular() or detect fi.IsDir()) before presenting the
"existing-config" dialog. If the path exists but is not a regular file, handle
it the same as "does not exist" or return an error to avoid allowing directories
or special files into the Edit existing / Keep & continue flow; update the
condition around the os.Stat(configPath) call and any code that shows the
existing-config dialog to enforce this guard.
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%)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tui/wizard/telegram_setup_tui_test.go (2)
53-91: Consider adding comments explaining layout indices.The hardcoded indices (e.g.,
GetItem(3),GetItem(1)) make this helper fragile and harder to maintain. Brief comments explaining what each index corresponds to would improve clarity.📝 Suggested documentation
func extractTelegramSetupViews(t *testing.T, root tview.Primitive) (*tview.TextView, *tview.TextView, *tview.Form) { t.Helper() layout, ok := root.(*tview.Flex) if !ok { t.Fatalf("expected root *tview.Flex, got %T", root) } if layout.GetItemCount() < 4 { t.Fatalf("unexpected layout item count: %d", layout.GetItemCount()) } + // Layout structure: [0: header, 1: ?, 2: ?, 3: pages (main content)] pages, ok := layout.GetItem(3).(*tview.Pages) if !ok { t.Fatalf("expected pages at layout index 3, got %T", layout.GetItem(3)) } _, bodyPrimitive := pages.GetFrontPage() body, ok := bodyPrimitive.(*tview.Flex) if !ok { t.Fatalf("expected body *tview.Flex, got %T", bodyPrimitive) } if body.GetItemCount() < 4 { t.Fatalf("unexpected body item count: %d", body.GetItemCount()) } + // Body structure: [0: ?, 1: serverIDView, 2: statusView, 3: form] serverIDView, ok := body.GetItem(1).(*tview.TextView)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/telegram_setup_tui_test.go` around lines 53 - 91, The helper extractTelegramSetupViews uses hardcoded layout indices (layout.GetItem(3), body.GetItem(1/2/3), etc.) which is fragile; update the function to add brief inline comments near each index reference explaining what that index represents (e.g., layout index 3 -> Pages container, body index 1 -> server ID TextView, body index 2 -> status TextView, body index 3 -> form) and, where helpful, why those positions are stable (how the layout is constructed) so future readers know the mapping between indices and UI components; leave function logic unchanged, only add concise comments adjacent to the symbols layout, pages, body, serverIDView, statusView, and form.
600-638: Consider increasing timing margins for CI stability.The 20ms sleep before
telegramSetupQueueUpdateDraw(line 617) is quite tight and could lead to flaky behavior on slow CI runners. Consider a slightly longer delay (e.g., 50ms) to improve reliability.⏱️ Suggested timing adjustment
go func() { - time.Sleep(20 * time.Millisecond) + time.Sleep(50 * time.Millisecond) telegramSetupQueueUpdateDraw(app, func() { close(updateDone) app.Stop() }) }() go func() { - time.Sleep(250 * time.Millisecond) + time.Sleep(500 * time.Millisecond) app.Stop() }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/telegram_setup_tui_test.go` around lines 600 - 638, The test TestTelegramSetupDefaultWrappers uses a very short sleep before calling telegramSetupQueueUpdateDraw which can flake on CI; increase the delay from 20ms to a larger margin (e.g., 50ms or 100ms) where the goroutine that closes updateDone waits longer before calling telegramSetupQueueUpdateDraw(app, ...) so the updateDone synchronization is more robust on slow runners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/tui/wizard/telegram_setup_tui_test.go`:
- Around line 53-91: The helper extractTelegramSetupViews uses hardcoded layout
indices (layout.GetItem(3), body.GetItem(1/2/3), etc.) which is fragile; update
the function to add brief inline comments near each index reference explaining
what that index represents (e.g., layout index 3 -> Pages container, body index
1 -> server ID TextView, body index 2 -> status TextView, body index 3 -> form)
and, where helpful, why those positions are stable (how the layout is
constructed) so future readers know the mapping between indices and UI
components; leave function logic unchanged, only add concise comments adjacent
to the symbols layout, pages, body, serverIDView, statusView, and form.
- Around line 600-638: The test TestTelegramSetupDefaultWrappers uses a very
short sleep before calling telegramSetupQueueUpdateDraw which can flake on CI;
increase the delay from 20ms to a larger margin (e.g., 50ms or 100ms) where the
goroutine that closes updateDone waits longer before calling
telegramSetupQueueUpdateDraw(app, ...) so the updateDone synchronization is more
robust on slow runners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c33aee29-ea0a-4620-bdec-fb6df5019a13
📒 Files selected for processing (2)
internal/tui/wizard/new_install_test.gointernal/tui/wizard/telegram_setup_tui_test.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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%)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/wizard/telegram_setup_tui_test.go (1)
646-649: Watchdog goroutine will leak on successful test runs.The watchdog goroutine continues sleeping for up to 500ms after the test completes successfully, since there's no cancellation mechanism. While this doesn't affect correctness, it can slow down test suite execution when many tests run in parallel.
Consider using a cancellable approach:
♻️ Proposed fix using context cancellation
+ ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { - time.Sleep(500 * time.Millisecond) - app.Stop() + select { + case <-time.After(500 * time.Millisecond): + app.Stop() + case <-ctx.Done(): + } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/wizard/telegram_setup_tui_test.go` around lines 646 - 649, The test spawns a watchdog goroutine that unconditionally sleeps for 500ms then calls app.Stop(), which can leak/waste time on successful runs; replace the plain time.Sleep goroutine with a cancellable watchdog using a context.WithCancel (or a time.Timer that you Stop) so the goroutine can exit immediately when the test finishes — create ctx,cancel before starting the goroutine, have the goroutine select on ctx.Done() vs time.After(500*time.Millisecond) and call app.Stop() only on the timeout, and call cancel() (or stop the timer) from the test teardown to prevent the goroutine from lingering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/tui/wizard/telegram_setup_tui_test.go`:
- Around line 646-649: The test spawns a watchdog goroutine that unconditionally
sleeps for 500ms then calls app.Stop(), which can leak/waste time on successful
runs; replace the plain time.Sleep goroutine with a cancellable watchdog using a
context.WithCancel (or a time.Timer that you Stop) so the goroutine can exit
immediately when the test finishes — create ctx,cancel before starting the
goroutine, have the goroutine select on ctx.Done() vs
time.After(500*time.Millisecond) and call app.Stop() only on the timeout, and
call cancel() (or stop the timer) from the test teardown to prevent the
goroutine from lingering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68d2b2dd-a600-46de-8990-f662561f878c
📒 Files selected for processing (1)
internal/tui/wizard/telegram_setup_tui_test.go
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
Improvements
Tests
Documentation