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
📝 WalkthroughWalkthroughAdds a unified AGE encryption setup flow (CLI/TUI), new install/new-install decision and preservation logic, strict secondary-path validation and env mutation helpers, cron utilities, extensive orchestrator refactors for AGE/Telegram/decrypt UI workflows, and many tests wiring these changes across CLI, TUI, and internal packages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Install CLI
participant Config as Config Loader
participant Orch as Orchestrator
participant UI as Age Setup UI
participant FS as FileSystem
User->>CLI: start install
CLI->>Config: load configuration
Config-->>CLI: config / error
alt config exists
CLI->>User: prompt existing-config (overwrite/edit/keep-continue/cancel)
User->>CLI: choose action
CLI->>FS: read/use existing config (skip post-steps if keep-continue)
else new or overwrite
CLI->>Orch: EnsureAgeRecipientsReadyWithUI(ctx, ui)
Orch->>UI: CollectRecipientDraft / ConfirmOverwrite / ConfirmAddAnother
UI->>User: prompt recipient(s)
User->>UI: provide drafts
UI-->>Orch: drafts
Orch->>FS: write recipient file (if needed)
Orch-->>CLI: setup result (path, wrote, reused)
end
CLI->>User: complete installation
sequenceDiagram
participant TUI
participant Bootstrap as Telegram Bootstrap
participant ConfigLoader
participant IdentityDetect
participant TelegramAPI
TUI->>Bootstrap: BuildTelegramSetupBootstrap(configPath, baseDir)
Bootstrap->>ConfigLoader: load config
ConfigLoader-->>Bootstrap: config / error
alt config error
Bootstrap-->>TUI: SkipConfigError
else config loaded
Bootstrap->>IdentityDetect: detect identity
IdentityDetect-->>Bootstrap: identity / error
alt eligible centralized
Bootstrap-->>TUI: EligibleCentralized (ServerID, IdentityFile)
TUI->>User: show pairing UI
User->>TUI: request verification
TUI->>TelegramAPI: poll registration
TelegramAPI-->>TUI: status / code
else skipped (disabled/personal/identity missing)
Bootstrap-->>TUI: SkipReason
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/proxsave/main.go (1)
1567-1572:⚠️ Potential issue | 🟡 MinorFix staticcheck S1000: use simple channel receive instead of
selectwith single case.The pipeline flagged this issue. A
selectwith only one case is equivalent to a direct channel operation.🔧 Proposed fix
fmt.Printf("\r Remaining: %ds ", int(remaining.Seconds())) - select { - case <-ticker.C: - continue - } + <-ticker.C }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/main.go` around lines 1567 - 1572, Replace the unnecessary single-case select by performing a direct receive on the ticker channel: remove the select block and use a plain channel receive from ticker.C (the same ticker used in the loop) to wait for ticks; ensure the surrounding loop logic that currently does "case <-ticker.C: continue" still continues the loop after the receive.cmd/proxsave/newkey.go (1)
116-124:⚠️ Potential issue | 🟠 MajorLoad the persisted config before forcing a new recipient.
Lines 119-124 synthesize a fresh
config.Configand hard-codeAgeRecipientFileto the default path. That drops any recipient-file override already saved inconfigPath, sonewkeycan rotate one file while the installed config still points at another.cmd/proxsave/encryption_setup.goalready avoids this by loading the config first.Patch suggestion
- cfg := &config.Config{ - BaseDir: baseDir, - ConfigPath: configPath, - EncryptArchive: true, - AgeRecipientFile: recipientPath, - } + cfg, loadErr := config.LoadConfig(configPath) + if loadErr != nil { + if !errors.Is(loadErr, os.ErrNotExist) { + return fmt.Errorf("load config: %w", loadErr) + } + cfg = &config.Config{} + } + cfg.BaseDir = baseDir + cfg.ConfigPath = configPath + cfg.EncryptArchive = true + if strings.TrimSpace(cfg.AgeRecipientFile) == "" { + cfg.AgeRecipientFile = recipientPath + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/newkey.go` around lines 116 - 124, In runNewKeySetup, don't overwrite any persisted AgeRecipientFile; first load the existing config from configPath (as encryption_setup.go does) and if it contains a non-empty AgeRecipientFile, use that value instead of hard-coding recipientPath into the new config.Config; otherwise fall back to the default filepath.Join(baseDir, "identity", "age", "recipient.txt"). Ensure you still set BaseDir and ConfigPath on the resulting config.Config and proceed with key rotation using that merged configuration.
🧹 Nitpick comments (11)
internal/orchestrator/tui_simulation_test.go (1)
61-74: Consider renaming the test to match the function under test.The test function
TestPromptOverwriteAction_SelectsOverwritestill references the old function namepromptOverwriteAction, but now testspromptExistingPathDecisionTUI. Consider renaming toTestPromptExistingPathDecisionTUI_SelectsOverwritefor consistency and discoverability.The test logic itself correctly validates the new return signature
(PathDecision, string, error)and appropriately asserts thatnewPathis empty when the decision is to overwrite.🤖 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 61 - 74, Rename the test function TestPromptOverwriteAction_SelectsOverwrite to TestPromptExistingPathDecisionTUI_SelectsOverwrite so the name matches the function under test promptExistingPathDecisionTUI; update the function declaration (the test name) and any references to the old test identifier in the file to ensure consistency and discoverability while keeping the test body (calls to promptExistingPathDecisionTUI and assertions on PathDecision, newPath, err) unchanged.internal/orchestrator/decrypt_tui_e2e_helpers_test.go (1)
57-93: Consider finalizing the simulation screen in cleanup.The simulation screen is initialized at line 62 but not finalized in the cleanup. While this may work for tests, explicitly calling
screen.Fini()in the cleanup would be more robust and prevent potential resource leaks.♻️ Suggested improvement
t.Cleanup(func() { + screen.Fini() newTUIApp = orig })🤖 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 57 - 93, withTimedSimAppSequence currently initializes a tcell SimulationScreen (screen) but never finalizes it; update the t.Cleanup closure inside withTimedSimAppSequence to call screen.Fini() (and restore newTUIApp as existing) so the simulation screen is properly closed after the test, preventing resource leaks; ensure the cleanup runs even if other cleanup logic errors by invoking screen.Fini() before resetting newTUIApp.internal/orchestrator/telegram_setup_bootstrap.go (1)
46-104: Consider simplifying the return type since error is always nil.The function signature is
BuildTelegramSetupBootstrap(configPath, baseDir string) (TelegramSetupBootstrap, error), but the function always returnsnilfor the error. The error information is captured in the struct fields (ConfigError,IdentityDetectError). This is a valid pattern (embedding errors in result structs), but the nil error return could be misleading to callers expecting error conditions to be signaled via the error return.Either:
- Remove the error return:
func BuildTelegramSetupBootstrap(...) TelegramSetupBootstrap- Or document that errors are always embedded in the struct and the error return is reserved for future use
♻️ Option 1: Remove unused error return
-func BuildTelegramSetupBootstrap(configPath, baseDir string) (TelegramSetupBootstrap, error) { +func BuildTelegramSetupBootstrap(configPath, baseDir string) TelegramSetupBootstrap { state := TelegramSetupBootstrap{} cfg, err := telegramSetupBootstrapLoadConfig(configPath) if err != nil { state.Eligibility = TelegramSetupSkipConfigError state.ConfigError = err.Error() - return state, nil + return state }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/telegram_setup_bootstrap.go` around lines 46 - 104, The BuildTelegramSetupBootstrap function always returns a nil error and embeds errors in the TelegramSetupBootstrap fields (ConfigError, IdentityDetectError); change the function signature to remove the unused error return (make it func BuildTelegramSetupBootstrap(configPath, baseDir string) TelegramSetupBootstrap), update all return sites inside the function to return just the state value, and update any callers to no longer expect a second error return (they should inspect state.ConfigError and state.IdentityDetectError instead).internal/cron/cron_test.go (1)
43-61: Consider adding edge case tests for boundary times.The tests cover typical cases well. Consider adding tests for boundary values like
"00:00"(midnight) and"23:59"(end of day) to ensure the schedule conversion handles edge times correctly.♻️ Suggested additional test cases
tests := []struct { name string in string want string }{ {name: "valid", in: "02:05", want: "05 02 * * *"}, {name: "normalized short", in: "2:5", want: "05 02 * * *"}, + {name: "midnight", in: "00:00", want: "00 00 * * *"}, + {name: "end of day", in: "23:59", want: "59 23 * * *"}, {name: "invalid", in: "bad", want: ""}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cron/cron_test.go` around lines 43 - 61, Add tests for boundary times to TestTimeToSchedule to ensure TimeToSchedule handles edge values; specifically add cases with in "00:00" expecting "00 00 * * *" and "23:59" expecting "59 23 * * *" (and keep existing cases). Locate the test table in TestTimeToSchedule and append these two entries (use the same struct fields name/in/want) and run the test suite to verify behavior; if the assertions fail, update TimeToSchedule accordingly to normalize leading zeros and validate 00–23 hours and 00–59 minutes.internal/config/validation_secondary_test.go (1)
24-37: Consider addingt.Parallel()to subtests for faster execution.The parent test calls
t.Parallel(), but subtests don't. For table-driven tests with independent cases, parallelizing subtests improves test execution time.♻️ Proposed fix to parallelize subtests
for _, tt := range tests { + tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { + t.Parallel() err := ValidateRequiredSecondaryPath(tt.path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/validation_secondary_test.go` around lines 24 - 37, Add parallel execution to the subtests by capturing the loop variable and calling t.Parallel() inside each subtest: before calling t.Run, shadow the loop variable (e.g., tt := tt) to avoid closure capture issues, then inside the subtest function immediately call t.Parallel() so each table-driven case that calls ValidateRequiredSecondaryPath runs concurrently and speeds up the test suite.internal/tui/wizard/age_ui_adapter.go (2)
60-62: Context not propagated to ConfirmAddRecipient.Similar to
ConfirmOverwriteExistingRecipient, the context parameter is received but not passed through. This is a minor consistency issue.🤖 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 60 - 62, ConfirmAddAnotherRecipient on ageSetupUIAdapter receives a context but doesn't pass it through; update the call in ageSetupUIAdapter.ConfirmAddAnotherRecipient to forward ctx into ConfirmAddRecipient (matching how ConfirmOverwriteExistingRecipient does), e.g. call ConfirmAddRecipient(ctx, a.configPath, a.buildSig, currentCount) so the context is propagated.
23-25: Context not propagated to underlying wizard function.
ConfirmRecipientOverwritereceivesctxbut doesn't pass it to the underlying function. If the wizard supports context-based cancellation, this could lead to unresponsive UI during cancellation requests.Consider updating the signature if the underlying function supports context
If
ConfirmRecipientOverwriteinage.gocan be updated to accept context:func (a *ageSetupUIAdapter) ConfirmOverwriteExistingRecipient(ctx context.Context, recipientPath string) (bool, error) { - return ConfirmRecipientOverwrite(recipientPath, a.configPath, a.buildSig) + return ConfirmRecipientOverwrite(ctx, recipientPath, a.configPath, a.buildSig) }🤖 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, The method ageSetupUIAdapter.ConfirmOverwriteExistingRecipient is not propagating the received ctx to the underlying ConfirmRecipientOverwrite call; update the call to pass ctx (i.e., ConfirmRecipientOverwrite(ctx, recipientPath, a.configPath, a.buildSig)) and, if ConfirmRecipientOverwrite's signature currently lacks a context parameter, modify its signature to accept context.Context as the first parameter and update all callers accordingly so cancellation propagates through the wizard flow.cmd/proxsave/install_existing_config.go (1)
28-38: Consider using safefs.Stat for timeout protection on network paths.The relevant code snippets show
internal/safefs/safefs.goprovides aStatfunction with timeout protection. Usingos.Statdirectly could hang indefinitely ifconfigPathpoints to an unresponsive network mount.Use safefs.Stat with timeout
+import ( + "time" + "github.com/tis24dev/proxsave/internal/safefs" +) + func promptExistingConfigModeCLI(ctx context.Context, reader *bufio.Reader, configPath string) (existingConfigMode, error) { - info, err := os.Stat(configPath) + info, err := safefs.Stat(ctx, configPath, 5*time.Second) if err != nil { if os.IsNotExist(err) { return existingConfigOverwrite, nil } return existingConfigCancel, fmt.Errorf("failed to access configuration file: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 28 - 38, Replace the direct os.Stat call in promptExistingConfigModeCLI with the safefs.Stat wrapper to get timeout protection for network mounts: call safefs.Stat(ctx, configPath) (and import the safefs package), use the returned info and error exactly as before (preserve the os.IsNotExist check to return existingConfigOverwrite, wrap other errors with "failed to access configuration file: %w"), and keep the subsequent info.Mode().IsRegular() check and its error path unchanged so behavior remains identical except for the added timeout safety.internal/orchestrator/age_setup_ui.go (1)
13-18: Consider adding documentation for the exported struct.The
AgeRecipientDraftstruct contains sensitive fields (Passphrase,PrivateKey). Adding a doc comment clarifying that these should be zeroed after use would help consumers handle them securely.📝 Suggested documentation
+// AgeRecipientDraft holds user-provided input for AGE recipient setup. +// Callers should zero Passphrase and PrivateKey fields after use. type AgeRecipientDraft struct { Kind AgeRecipientInputKind PublicKey string Passphrase string PrivateKey string }🤖 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 doc comment above the exported AgeRecipientDraft struct describing its purpose and explicitly warning that Passphrase and PrivateKey contain sensitive material that must be cleared from memory when no longer needed (e.g., zeroed or overwritten). Reference the struct name AgeRecipientDraft and its sensitive fields Passphrase and PrivateKey in the comment, and include recommended handling guidance such as zeroing/overwriting and limiting lifetime in memory.internal/orchestrator/age_setup_ui_cli.go (1)
47-70: Invalid menu options silently loop without user feedback.When the user enters an option outside the valid range (e.g., "5", "abc", empty string), the switch statement falls through without handling, causing the menu to redisplay without any error message. This could be confusing to users.
♻️ Add default case for invalid options
switch option { case "1": value, err := promptPublicRecipientAge(ctx, u.reader) if err != nil { u.warn(err) continue } return &AgeRecipientDraft{Kind: AgeRecipientInputExisting, PublicKey: value}, nil case "2": passphrase, err := promptAndConfirmPassphraseAge(ctx) if err != nil { u.warn(err) continue } return &AgeRecipientDraft{Kind: AgeRecipientInputPassphrase, Passphrase: passphrase}, nil case "3": privateKey, err := promptPrivateKeyValueAge(ctx) if err != nil { u.warn(err) continue } return &AgeRecipientDraft{Kind: AgeRecipientInputPrivateKey, PrivateKey: privateKey}, nil + default: + fmt.Println("Invalid option. Please select 1-4.") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/age_setup_ui_cli.go` around lines 47 - 70, The menu switch on variable option (the block that returns AgeRecipientDraft via promptPublicRecipientAge, promptAndConfirmPassphraseAge, promptPrivateKeyValueAge) lacks a default branch so invalid inputs silently loop; add a default case to the switch that calls u.warn with a clear message like "Invalid option, please choose 1, 2, or 3" (and continue the loop) so users get feedback when they enter "5", "abc", empty, etc.; keep using the existing u.warn method and the same loop control to re-prompt.internal/orchestrator/decrypt_workflow_ui_test.go (1)
64-80: Global state mutation may cause test flakiness in parallel runs.The test mutates the package-level
restoreFSvariable without usingt.Parallel()protection. While the deferred restore mitigates issues within this test, if other tests in the package run in parallel and also depend onrestoreFS, race conditions could occur.Consider using a test-local approach or ensuring tests that touch
restoreFSare explicitly serialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/decrypt_workflow_ui_test.go` around lines 64 - 80, The test mutates the package-level restoreFS (osFS{}) causing potential races; refactor to avoid global mutation by adding an fs parameter (e.g., fs aferio-like interface) to ensureWritablePathWithUI (or create a helper ensureWritablePathWithUIWithFS(ctx, ui, fs, path, desc)) and update TestEnsureWritablePathWithUI_ReturnsCleanMissingPath to pass a local osFS{} instead of setting restoreFS; remove the restoreFS assignment and deferred restore from the test and update any callers to use the new parameter so tests no longer rely on mutating the shared restoreFS variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/proxsave/install_tui.go`:
- Around line 139-150: The new AGE success-path logging calls use bootstrap
without nil checks and can panic if bootstrap is nil; update the block after
runInitialEncryptionSetupWithUI (where setupResult is handled) to only call
bootstrap.Info(...) when bootstrap != nil, i.e., guard the three bootstrap.Info
invocations (the general "AGE encryption configured successfully", the
"Recipient saved to: %s" branch checking setupResult.WroteRecipientFile &&
setupResult.RecipientPath != "" and the else-if branch checking
setupResult.ReusedExistingRecipients) with a nil-check of the bootstrap logger
so the function works when bootstrap is optional.
In `@cmd/proxsave/newkey.go`:
- Around line 91-92: runNewKey currently calls bootstrap.Info unconditionally at
the end, which will panic if the bootstrap variable is nil; update the success
logging to first check if bootstrap != nil and only call bootstrap.Info(...)
when it's present, otherwise fall back to printing the same messages via
fmt.Println (or log) so TUI-less flows don't panic—locate the bootstrap variable
and the two bootstrap.Info(...) calls near the end of runNewKey and wrap them in
a nil-check.
In `@internal/orchestrator/workflow_ui_tui_decrypt_prompts.go`:
- Around line 75-85: The form currently pre-fills defaultPath and only checks
for emptiness, letting users submit the same conflicting path; update the
AddInputFieldWithValidation callback (the input added via
form.AddInputFieldWithValidation using label "New path") to also compare the
trimmed value against defaultPath and return an error like "path must be
different from existing path" when they match, and likewise guard in
form.SetOnSubmit (which sets newPath from values[label]) to reject or prevent
assigning newPath if it equals defaultPath so the Submit cannot return the same
path; reference the symbols AddInputFieldWithValidation, SetOnSubmit,
defaultPath, newPath, and label when making the change.
---
Outside diff comments:
In `@cmd/proxsave/main.go`:
- Around line 1567-1572: Replace the unnecessary single-case select by
performing a direct receive on the ticker channel: remove the select block and
use a plain channel receive from ticker.C (the same ticker used in the loop) to
wait for ticks; ensure the surrounding loop logic that currently does "case
<-ticker.C: continue" still continues the loop after the receive.
In `@cmd/proxsave/newkey.go`:
- Around line 116-124: In runNewKeySetup, don't overwrite any persisted
AgeRecipientFile; first load the existing config from configPath (as
encryption_setup.go does) and if it contains a non-empty AgeRecipientFile, use
that value instead of hard-coding recipientPath into the new config.Config;
otherwise fall back to the default filepath.Join(baseDir, "identity", "age",
"recipient.txt"). Ensure you still set BaseDir and ConfigPath on the resulting
config.Config and proceed with key rotation using that merged configuration.
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 28-38: Replace the direct os.Stat call in
promptExistingConfigModeCLI with the safefs.Stat wrapper to get timeout
protection for network mounts: call safefs.Stat(ctx, configPath) (and import the
safefs package), use the returned info and error exactly as before (preserve the
os.IsNotExist check to return existingConfigOverwrite, wrap other errors with
"failed to access configuration file: %w"), and keep the subsequent
info.Mode().IsRegular() check and its error path unchanged so behavior remains
identical except for the added timeout safety.
In `@internal/config/validation_secondary_test.go`:
- Around line 24-37: Add parallel execution to the subtests by capturing the
loop variable and calling t.Parallel() inside each subtest: before calling
t.Run, shadow the loop variable (e.g., tt := tt) to avoid closure capture
issues, then inside the subtest function immediately call t.Parallel() so each
table-driven case that calls ValidateRequiredSecondaryPath runs concurrently and
speeds up the test suite.
In `@internal/cron/cron_test.go`:
- Around line 43-61: Add tests for boundary times to TestTimeToSchedule to
ensure TimeToSchedule handles edge values; specifically add cases with in
"00:00" expecting "00 00 * * *" and "23:59" expecting "59 23 * * *" (and keep
existing cases). Locate the test table in TestTimeToSchedule and append these
two entries (use the same struct fields name/in/want) and run the test suite to
verify behavior; if the assertions fail, update TimeToSchedule accordingly to
normalize leading zeros and validate 00–23 hours and 00–59 minutes.
In `@internal/orchestrator/age_setup_ui_cli.go`:
- Around line 47-70: The menu switch on variable option (the block that returns
AgeRecipientDraft via promptPublicRecipientAge, promptAndConfirmPassphraseAge,
promptPrivateKeyValueAge) lacks a default branch so invalid inputs silently
loop; add a default case to the switch that calls u.warn with a clear message
like "Invalid option, please choose 1, 2, or 3" (and continue the loop) so users
get feedback when they enter "5", "abc", empty, etc.; keep using the existing
u.warn method and the same loop control to re-prompt.
In `@internal/orchestrator/age_setup_ui.go`:
- Around line 13-18: Add a doc comment above the exported AgeRecipientDraft
struct describing its purpose and explicitly warning that Passphrase and
PrivateKey contain sensitive material that must be cleared from memory when no
longer needed (e.g., zeroed or overwritten). Reference the struct name
AgeRecipientDraft and its sensitive fields Passphrase and PrivateKey in the
comment, and include recommended handling guidance such as zeroing/overwriting
and limiting lifetime in memory.
In `@internal/orchestrator/decrypt_tui_e2e_helpers_test.go`:
- Around line 57-93: withTimedSimAppSequence currently initializes a tcell
SimulationScreen (screen) but never finalizes it; update the t.Cleanup closure
inside withTimedSimAppSequence to call screen.Fini() (and restore newTUIApp as
existing) so the simulation screen is properly closed after the test, preventing
resource leaks; ensure the cleanup runs even if other cleanup logic errors by
invoking screen.Fini() before resetting newTUIApp.
In `@internal/orchestrator/decrypt_workflow_ui_test.go`:
- Around line 64-80: The test mutates the package-level restoreFS (osFS{})
causing potential races; refactor to avoid global mutation by adding an fs
parameter (e.g., fs aferio-like interface) to ensureWritablePathWithUI (or
create a helper ensureWritablePathWithUIWithFS(ctx, ui, fs, path, desc)) and
update TestEnsureWritablePathWithUI_ReturnsCleanMissingPath to pass a local
osFS{} instead of setting restoreFS; remove the restoreFS assignment and
deferred restore from the test and update any callers to use the new parameter
so tests no longer rely on mutating the shared restoreFS variable.
In `@internal/orchestrator/telegram_setup_bootstrap.go`:
- Around line 46-104: The BuildTelegramSetupBootstrap function always returns a
nil error and embeds errors in the TelegramSetupBootstrap fields (ConfigError,
IdentityDetectError); change the function signature to remove the unused error
return (make it func BuildTelegramSetupBootstrap(configPath, baseDir string)
TelegramSetupBootstrap), update all return sites inside the function to return
just the state value, and update any callers to no longer expect a second error
return (they should inspect state.ConfigError and state.IdentityDetectError
instead).
In `@internal/orchestrator/tui_simulation_test.go`:
- Around line 61-74: Rename the test function
TestPromptOverwriteAction_SelectsOverwrite to
TestPromptExistingPathDecisionTUI_SelectsOverwrite so the name matches the
function under test promptExistingPathDecisionTUI; update the function
declaration (the test name) and any references to the old test identifier in the
file to ensure consistency and discoverability while keeping the test body
(calls to promptExistingPathDecisionTUI and assertions on PathDecision, newPath,
err) unchanged.
In `@internal/tui/wizard/age_ui_adapter.go`:
- Around line 60-62: ConfirmAddAnotherRecipient on ageSetupUIAdapter receives a
context but doesn't pass it through; update the call in
ageSetupUIAdapter.ConfirmAddAnotherRecipient to forward ctx into
ConfirmAddRecipient (matching how ConfirmOverwriteExistingRecipient does), e.g.
call ConfirmAddRecipient(ctx, a.configPath, a.buildSig, currentCount) so the
context is propagated.
- Around line 23-25: The method
ageSetupUIAdapter.ConfirmOverwriteExistingRecipient is not propagating the
received ctx to the underlying ConfirmRecipientOverwrite call; update the call
to pass ctx (i.e., ConfirmRecipientOverwrite(ctx, recipientPath, a.configPath,
a.buildSig)) and, if ConfirmRecipientOverwrite's signature currently lacks a
context parameter, modify its signature to accept context.Context as the first
parameter and update all callers accordingly so cancellation propagates through
the wizard flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5227adc-f049-49d4-a1dc-3049839ce6c9
📒 Files selected for processing (65)
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/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_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
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.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/orchestrator/workflow_ui_cli_test.go (1)
13-40: HardencaptureCLIStdoutcleanup for early exits.If
fn()exits early, pipe closure and reader goroutine shutdown can be skipped. Use deferred cleanup to guarantee restoration and close semantics.♻️ Proposed refactor
func captureCLIStdout(t *testing.T, fn func()) string { t.Helper() oldStdout := os.Stdout r, w, err := os.Pipe() if err != nil { t.Fatalf("os.Pipe: %v", err) } os.Stdout = w - t.Cleanup(func() { - os.Stdout = oldStdout - }) var buf bytes.Buffer done := make(chan struct{}) go func() { _, _ = io.Copy(&buf, r) close(done) }() + defer func() { + _ = w.Close() + <-done + _ = r.Close() + os.Stdout = oldStdout + }() + fn() - - _ = 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 - 40, The captureCLIStdout helper can leak file descriptors and leave the reader goroutine hanging if fn() returns early; update captureCLIStdout to use deferred cleanup to always restore os.Stdout and close the writer and reader and to wait on the goroutine: set os.Stdout = w then immediately register a defer (or use t.Cleanup) that closes w, closes r, restores os.Stdout, and waits for the done channel; ensure the goroutine copying from r to buf is started before fn() and that w.Close() is performed in the defer so the reader goroutine always sees EOF and exits, and keep the returned buf.String() behavior in captureCLIStdout.internal/orchestrator/workflow_ui_tui_decrypt_prompts.go (1)
60-68: Make the validator return a normalized path as the single source of truth.
validateDistinctNewPathInputcurrently validates with cleaned values but returns the uncleaned input, forcing callers to clean again. Returning cleaned output directly will simplify callers and reduce drift risk.♻️ Proposed refactor
func validateDistinctNewPathInput(value, defaultPath string) (string, error) { trimmed := strings.TrimSpace(value) if trimmed == "" { return "", fmt.Errorf("path cannot be empty") } + cleaned := filepath.Clean(trimmed) trimmedDefault := strings.TrimSpace(defaultPath) - if trimmedDefault != "" && filepath.Clean(trimmed) == filepath.Clean(trimmedDefault) { + if trimmedDefault != "" && cleaned == filepath.Clean(trimmedDefault) { return "", fmt.Errorf("path must be different from existing path") } - return trimmed, nil + return cleaned, nil }- return PathDecisionNewPath, filepath.Clean(newPath), nil + return PathDecisionNewPath, newPath, nil- return filepath.Clean(newPath), nil + return newPath, nilAlso applies to: 116-117, 119-131
🤖 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 60 - 68, Update validateDistinctNewPathInput to return the normalized/cleaned path as its single source of truth: change its signature to return (string, error), perform filepath.Clean inside the validator and return the cleaned value instead of the raw input. Then update callers (e.g., promptNewPathInputTUI and the call site that currently does filepath.Clean(newPath) in workflow_ui_tui_decrypt_prompts.go and the other occurrences around the noted ranges) to use the returned cleaned path directly and remove the extra filepath.Clean calls, preserving the same ErrDecryptAborted and error-handling behavior.
🤖 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/orchestrator/workflow_ui_cli_test.go`:
- Around line 13-40: The captureCLIStdout helper can leak file descriptors and
leave the reader goroutine hanging if fn() returns early; update
captureCLIStdout to use deferred cleanup to always restore os.Stdout and close
the writer and reader and to wait on the goroutine: set os.Stdout = w then
immediately register a defer (or use t.Cleanup) that closes w, closes r,
restores os.Stdout, and waits for the done channel; ensure the goroutine copying
from r to buf is started before fn() and that w.Close() is performed in the
defer so the reader goroutine always sees EOF and exits, and keep the returned
buf.String() behavior in captureCLIStdout.
In `@internal/orchestrator/workflow_ui_tui_decrypt_prompts.go`:
- Around line 60-68: Update validateDistinctNewPathInput to return the
normalized/cleaned path as its single source of truth: change its signature to
return (string, error), perform filepath.Clean inside the validator and return
the cleaned value instead of the raw input. Then update callers (e.g.,
promptNewPathInputTUI and the call site that currently does
filepath.Clean(newPath) in workflow_ui_tui_decrypt_prompts.go and the other
occurrences around the noted ranges) to use the returned cleaned path directly
and remove the extra filepath.Clean calls, preserving the same ErrDecryptAborted
and error-handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 927c0c1e-f631-4232-8c60-5e2f5b53573c
📒 Files selected for processing (8)
cmd/proxsave/install_tui.gocmd/proxsave/newkey.gocmd/proxsave/newkey_test.gointernal/orchestrator/tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/orchestrator/workflow_ui_tui_decrypt_prompts.gointernal/orchestrator/workflow_ui_tui_decrypt_test.go
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/newkey_test.go`:
- Around line 15-37: The helper captureNewKeyStdout swaps os.Stdout and starts a
goroutine to read from the pipe but only restores stdout and closes the writer
after calling fn(), so a panic or t.Fatal inside fn() will leak descriptors and
leave stdout swapped; fix by moving the cleanup into a deferred closure
immediately after creating r,w and starting the goroutine: defer a function that
closes w, restores os.Stdout to orig, and waits on the done channel
(closing/ignoring errors as needed) so that regardless of how fn() exits the
writer is closed, the goroutine finishes, and os.Stdout is restored; keep the
rest of captureNewKeyStdout (buf, done, io.Copy goroutine, and calling fn())
intact and reference captureNewKeyStdout, orig, r, w, done, and fn when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95f9f29f-d8ff-4e18-84f9-8c270cedf63f
📒 Files selected for processing (5)
cmd/proxsave/encryption_setup_test.gocmd/proxsave/main.gocmd/proxsave/newkey.gocmd/proxsave/newkey_test.godocs/CLI_REFERENCE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/proxsave/main.go
- cmd/proxsave/encryption_setup_test.go
Comment on lines
+15
to
+37
| func captureNewKeyStdout(t *testing.T, fn func()) string { | ||
| t.Helper() | ||
| orig := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatalf("pipe: %v", err) | ||
| } | ||
| os.Stdout = w | ||
|
|
||
| var buf bytes.Buffer | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| _, _ = io.Copy(&buf, r) | ||
| close(done) | ||
| }() | ||
|
|
||
| fn() | ||
|
|
||
| _ = w.Close() | ||
| os.Stdout = orig | ||
| <-done | ||
| return buf.String() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect capture helper implementation:"
rg -n -A40 -B5 'func captureNewKeyStdout' cmd/proxsave/newkey_test.go
echo
echo "Check for defer-based guaranteed cleanup tokens inside helper:"
python - <<'PY'
import re, pathlib
text = pathlib.Path("cmd/proxsave/newkey_test.go").read_text()
m = re.search(r'func captureNewKeyStdout\(.*?\)\s*string\s*{([\s\S]*?)\n}\n', text)
if not m:
raise SystemExit("helper not found")
body = m.group(1)
tokens = ["defer", "os.Stdout = orig", "w.Close()", "r.Close()"]
for t in tokens:
print(f"{t}: {'present' if t in body else 'missing'}")
PYRepository: tis24dev/proxsave
Length of output: 1555
Ensure stdout/pipe cleanup always runs, even on early exit.
Cleanup code at lines 33–35 executes only after fn() returns. If fn() exits early via panic, FailNow(), or Fatalf(), os.Stdout remains swapped and pipe file descriptors remain open, destabilizing subsequent tests.
Proposed fix
func captureNewKeyStdout(t *testing.T, fn func()) string {
t.Helper()
orig := os.Stdout
r, w, err := os.Pipe()
if err != nil {
t.Fatalf("pipe: %v", err)
}
os.Stdout = w
var buf bytes.Buffer
done := make(chan struct{})
go func() {
_, _ = io.Copy(&buf, r)
close(done)
}()
+
+ defer func() {
+ _ = w.Close()
+ os.Stdout = orig
+ <-done
+ _ = r.Close()
+ }()
fn()
-
- _ = w.Close()
- os.Stdout = orig
- <-done
return buf.String()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func captureNewKeyStdout(t *testing.T, fn func()) string { | |
| t.Helper() | |
| orig := os.Stdout | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatalf("pipe: %v", err) | |
| } | |
| os.Stdout = w | |
| var buf bytes.Buffer | |
| done := make(chan struct{}) | |
| go func() { | |
| _, _ = io.Copy(&buf, r) | |
| close(done) | |
| }() | |
| fn() | |
| _ = w.Close() | |
| os.Stdout = orig | |
| <-done | |
| return buf.String() | |
| } | |
| func captureNewKeyStdout(t *testing.T, fn func()) string { | |
| t.Helper() | |
| orig := os.Stdout | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatalf("pipe: %v", err) | |
| } | |
| os.Stdout = w | |
| var buf bytes.Buffer | |
| done := make(chan struct{}) | |
| go func() { | |
| _, _ = io.Copy(&buf, r) | |
| close(done) | |
| }() | |
| defer func() { | |
| _ = w.Close() | |
| os.Stdout = orig | |
| <-done | |
| _ = r.Close() | |
| }() | |
| fn() | |
| return buf.String() | |
| } |
🤖 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 - 37, The helper
captureNewKeyStdout swaps os.Stdout and starts a goroutine to read from the pipe
but only restores stdout and closes the writer after calling fn(), so a panic or
t.Fatal inside fn() will leak descriptors and leave stdout swapped; fix by
moving the cleanup into a deferred closure immediately after creating r,w and
starting the goroutine: defer a function that closes w, restores os.Stdout to
orig, and waits on the done channel (closing/ignoring errors as needed) so that
regardless of how fn() exits the writer is closed, the goroutine finishes, and
os.Stdout is restored; keep the rest of captureNewKeyStdout (buf, done, io.Copy
goroutine, and calling fn()) intact and reference captureNewKeyStdout, orig, r,
w, done, and fn when making the change.
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