Conversation
Verifies and fixes the waitForRetry hook handling in CloudStorage. Adds a defensive wrapper that calls waitForRetry only when it is set, otherwise returning nil, and replaces the direct calls in the remote check and upload retry paths. Also adds regression tests to confirm CloudStorage does not panic when the struct is constructed without that hook.
…decar paths Normalize bundle inputs before generating canonical bundle paths and legacy sidecar filenames, so bundle-based inputs do not produce sidecars like *.bundle.tar.sha256. Adds regression tests for repeated bundle suffixes and bundle-derived candidate paths.
Create a fresh HA test environment inside each t.Run subtest so overrides of package-level globals such as restoreFS, haApplyGeteuid, haIsMounted, and haArmRollback do not leak across sibling subtests. This keeps HA restore tests independent and prevents order-dependent failures.
…l interfaces Add a shared isNilInterface helper in decrypt_workflow_ui.go and use it for UI nil checks so both true nil and typed-nil interface values are rejected safely. Also adds regression tests covering typed-nil UI inputs across the decrypt workflow helpers.
…d of timer-driven Replace timer-based key injection in withTimedSimAppSequence with synchronization based on rendered SimulationScreen state, so timed keys are sent only after the expected TUI view is visible and the screen has advanced. This removes flaky wall-clock waits from the decrypt TUI e2e helper and keeps the decrypt TUI tests stable.
…s and delivered first response Fix the webhook retry cancellation test to use an atomic attempt counter, fully deliver the first HTTP error response before canceling the context, and use a non-zero retry delay so the retry path is actually exercised. This makes the test deterministic and avoids races around early cancellation and unsynchronized attempt counting.
Encode SECONDARY_PATH and SECONDARY_LOG_PATH safely before writing them into the env template, so paths containing #, spaces, or quotes do not break .env parsing. Also adds a round-trip test to verify quoted secondary paths are written and parsed back correctly.
Update env_mutation_test.go to parse the mutated template into key/value pairs and assert exact final values with single occurrences, instead of relying on strings.Contains. This prevents tests from passing when duplicate or stale env entries remain in the rendered template.
Update the setBaseDirEnv test helper so passing an empty string truly unsets BASE_DIR instead of setting it to an empty value. This keeps tests aligned with code paths that rely on os.LookupEnv to distinguish an unset variable from a present-but-empty one.
rim trailing slashes from preserved entry names before appending the display suffix in formatNewInstallPreservedEntries, so inputs like env/ or build// render as env/ and build/ instead of env//. Also adds a regression test covering slash-terminated preserved entries.
Remove the obsolete clearImmutableAttributes wrapper now that the install flow uses clearImmutableAttributesWithContext directly. This keeps the install codepath on the context-aware implementation and removes an unused legacy symbol.
…nfig mode Check ctx.Err() before the missing-file fast path and again before returning the final existing-config decision in promptExistingConfigModeCLI. This ensures a canceled context stops the install flow before it proceeds into config generation, and includes regression coverage for the missing-file cancellation case.
…lper Remove the extra t.Cleanup from captureCLIStdout and keep the existing deferred restoration logic as the single stdout cleanup path. This simplifies the helper and avoids duplicate no-op cleanup.
Replace the hand-duplicated invalidPermissionsTemplate in migration_test.go with a fixture derived from baseInstallTemplate, overriding only the fields needed for the invalid-permissions case. This keeps the migration test fixture aligned with the canonical install template and avoids future drift when template keys change.
…ests Update the restore TUI simulation tests to verify that confirmRestoreTUI passes the original context through to promptYesNoTUIFunc, instead of only checking the returned boolean result. This makes the tests fail if context propagation is broken in the confirmation flow.
Send validateDistinctNewPathInput validation errors to stderr in the CLI workflow instead of stdout, keeping user-facing error output consistent with other CLI error paths. Also updates the CLI workflow test helper and assertions to verify the message is emitted on stderr.
…ng timeout state Wait for the restore TUI countdown goroutine to exit before reading timedOut and cancelled in promptYesNoTUIWithCountdown, using a done channel tied to the ticker worker. This avoids races during TUI shutdown and prevents countdown updates from being queued after the app has already stopped.
Update writeIdentityFileWithContext to check ctx.Err() before writing and always restore the immutable attribute in deferred cleanup using a non-cancelable context. This prevents the identity file from being left mutable when the write path exits early due to cancellation or write failures, and adds regression tests for both cases.
The HTTP 200 handler in sendViaCloudRelay() returned success without
parsing the response body, ignoring the "success" field. If the worker
responded with {"success": false, "error": "..."}, the client silently
treated it as a successful delivery, preventing the PMF fallback from
triggering and causing emails to be lost.
Parse the response body on HTTP 200 and check the success field. When
success=false, return an error so the existing fallback chain activates.
Empty or non-JSON bodies on 200 are still treated as success for backward
compatibility.
Add diagnostic logging of the raw response body for all HTTP status codes
to aid future relay troubleshooting.
Differentiate relay 401/403 failure causes end-to-end by adding explicit worker error codes and client-side classification fallback for legacy responses. Add worker-side trusted IP bypass for the per-MAC daily quota and document relay acceptance semantics, common auth/HMAC errors, and MAC_LIMIT_IP_WHITELIST usage.
📝 WalkthroughWalkthroughThis PR spans multiple system areas with context-aware error handling improvements, enhanced environment variable encoding, typed-nil interface validation, email relay error classification, improved test infrastructure, and path normalization logic. Changes include wrapper function removal, cancellation checks at decision points, immutable attribute re-locking with defer recovery, and stderr output redirection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can get early access to new features in CodeRabbit.Enable the |
![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
🧹 Nitpick comments (5)
internal/orchestrator/restore_ha_additional_test.go (1)
387-395: Optional: extract duplicatednewEnvhelper to a shared test helper.Both top-level tests carry nearly the same
newEnvclosure. A shared helper would reduce drift risk if staging setup changes again.Also applies to: 666-673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_ha_additional_test.go` around lines 387 - 395, Extract the duplicated newEnv closure into a shared test helper function (e.g., newHATestEnv or setupStagedHATestEnv) that returns *haTestEnv and calls setupHATestEnv, adds the staged HA config file at env.stageRoot + "/etc/pve/ha/resources.cfg" and fails the test on error; replace both local newEnv closures in restore_ha_additional_test.go (the one around lines 387–395 and the one around 666–673) with calls to this new helper and update their callers to use the shared helper to avoid duplication and drift.cmd/proxsave/install_existing_config.go (1)
63-80: Consider consolidating repetitive context checks.Each switch case (lines 63-65, 68-70, 73-75, 78-80) performs the same
ctx.Err()check. SincepromptOptionalalready handles context cancellation at line 55-58, these additional checks only catch the narrow window betweenpromptOptionalreturning and the switch executing.If this redundancy is intentional for consistency and defensive coding, it's acceptable. Otherwise, you could consolidate:
♻️ Optional: Single context check after the switch
for { choice, err := promptOptional(ctx, reader, "Choice [3]: ") if err != nil { return existingConfigCancel, err } + if err := ctx.Err(); err != nil { + return existingConfigCancel, err + } switch strings.TrimSpace(choice) { case "": fallthrough case "3": - if err := ctx.Err(); err != nil { - return existingConfigCancel, err - } return existingConfigKeepContinue, nil case "1": - if err := ctx.Err(); err != nil { - return existingConfigCancel, err - } return existingConfigOverwrite, nil case "2": - if err := ctx.Err(); err != nil { - return existingConfigCancel, err - } return existingConfigEdit, nil case "0": - if err := ctx.Err(); err != nil { - return existingConfigCancel, err - } return existingConfigCancel, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/proxsave/install_existing_config.go` around lines 63 - 80, The repeated ctx.Err() checks inside each switch case (around returns for existingConfigKeepContinue, existingConfigOverwrite, existingConfigEdit, existingConfigCancel) are redundant because promptOptional already handles cancellation; remove the per-case ctx.Err() calls and perform a single context cancellation check once either immediately before the switch or immediately after the switch falls through to determine cancellation, keeping the same return values for the cases (existingConfigKeepContinue, existingConfigOverwrite, existingConfigEdit, existingConfigCancel) and preserving behavior if ctx.Err() is non-nil.internal/notify/webhook_test.go (1)
330-381: Consider using atomic counter here for consistency.The
TestWebhookNotifier_Send_Retrytest uses a non-atomicattemptscounter (line 332), while the updatedTestWebhookNotifier_SendToEndpoint_StopsRetryingWhenContextCancelednow usesatomic.Int32. Although the sequential nature of HTTP requests in this test makes the race unlikely to cause incorrect behavior, using atomic operations would maintain consistency and satisfy the Go race detector.♻️ Optional: Use atomic counter for consistency
func TestWebhookNotifier_Send_Retry(t *testing.T) { logger := logging.New(types.LogLevelDebug, false) - attempts := 0 + var attempts atomic.Int32 // Create mock server that fails first 2 times, then succeeds server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - attempts++ - if attempts < 3 { + if attempts.Add(1) < 3 { w.WriteHeader(http.StatusInternalServerError) w.Write([]byte(`{"error":"temporary failure"}`)) return } w.WriteHeader(http.StatusOK) w.Write([]byte(`{"status":"ok"}`)) })) defer server.Close() // ... rest of test ... - if attempts != 3 { - t.Errorf("Expected 3 attempts, got %d", attempts) + if got := attempts.Load(); got != 3 { + t.Errorf("Expected 3 attempts, got %d", got) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notify/webhook_test.go` around lines 330 - 381, The test TestWebhookNotifier_Send_Retry uses a plain int attempts counter; change it to an atomic counter (e.g., var attempts atomic.Int32 or var attempts int32 and use atomic.AddInt32) and update the httptest.NewServer handler to increment via atomic.AddInt32(&attempts, 1) and read via atomic.LoadInt32 when asserting the expected number of attempts; also add the necessary import for sync/atomic (or use the atomic.Int32 type with import "sync/atomic") so the Go race detector is satisfied and the test is consistent with TestWebhookNotifier_SendToEndpoint_StopsRetryingWhenContextCanceled.internal/orchestrator/restore_tui.go (1)
987-1020: Consider unifying goroutine shutdown pattern withpromptYesNoTUIWithCountdown.This function uses inline
close(stopCh); ticker.Stop(); <-donewhereaspromptYesNoTUIWithCountdownnow uses a cleaner scopedstopTickerpattern. Consider refactoring for consistency if you revisit this code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/orchestrator/restore_tui.go` around lines 987 - 1020, The countdown goroutine in this function uses an ad-hoc shutdown pattern (stopCh/done/ticker.Stop() and waiting for <-done) which is inconsistent with the scoped stopTicker helper used by promptYesNoTUIWithCountdown; refactor this block to use the same stopTicker pattern: create the stopTicker function (or reuse the existing one) to encapsulate closing the stop channel, stopping the ticker and waiting for the goroutine to finish, then replace the inline close(stopCh); ticker.Stop(); <-done calls before and after app.RunWithContext(ctx) with a single stopTicker() call and remove duplicated shutdown logic in the anonymous goroutine (refer to symbols stopCh, done, ticker, app.RunWithContext and promptYesNoTUIWithCountdown to locate and update the code).internal/config/env_mutation_test.go (1)
88-111: Good round-trip test, but missing coverage forstrconv.Quotefallback path.This test validates paths containing
#and spaces (single-quote path), but doesn't exercise thestrconv.Quotefallback inencodeEnvValue(triggered when a value contains both'and"). Given the round-trip issue flagged inenv_mutation.go, consider adding a test case that would fail until the encoding/parsing mismatch is fixed.📝 Suggested additional test case
func TestApplySecondaryStorageSettingsQuotesBothQuoteTypes(t *testing.T) { template := "SECONDARY_ENABLED=false\nSECONDARY_PATH=\nSECONDARY_LOG_PATH=\n" // Value containing both single and double quotes triggers strconv.Quote got := ApplySecondaryStorageSettings(template, true, `/mnt/it's "special"`, "") configPath := filepath.Join(t.TempDir(), "backup.env") if err := os.WriteFile(configPath, []byte(got), 0o600); err != nil { t.Fatalf("write config: %v", err) } raw, err := parseEnvFile(configPath) if err != nil { t.Fatalf("parseEnvFile() error = %v", err) } // This will fail until the strconv.Quote/Unquote round-trip is fixed if gotPath := raw["SECONDARY_PATH"]; gotPath != `/mnt/it's "special"` { t.Fatalf("SECONDARY_PATH = %q; want %q", gotPath, `/mnt/it's "special"`) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env_mutation_test.go` around lines 88 - 111, Add a new unit test that exercises the strconv.Quote fallback path by calling ApplySecondaryStorageSettings with a path containing both single and double quotes (e.g. `/mnt/it's "special"`), writing the returned env to disk and asserting that parseEnvFile returns the identical raw value for "SECONDARY_PATH"; this will reference TestApplySecondaryStorageSettingsQuotesBothQuoteTypes (new), ApplySecondaryStorageSettings, encodeEnvValue (which triggers strconv.Quote), and parseEnvFile so the round-trip failure is reproducible and can guide fixing the encode/unquote behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CONFIGURATION.md`:
- Line 849: The log sentence overstates guarantee; update the wording to reflect
that "accepted" refers to the relay returning an HTTP 200-style response rather
than guaranteed delivery—change the line to something like "When logs say the
relay 'accepted request' or 'returned HTTP 200', it means sendViaCloudRelay (and
the upstream email API) returned a success response; it does not guarantee final
inbox delivery (the message may still bounce, be deferred, or land in spam
later)." Ensure the change references sendViaCloudRelay to align wording with
its current behavior when it treats empty or unparsable 200 responses as
success.
In `@internal/config/env_mutation.go`:
- Around line 19-25: encodeEnvValue currently falls back to strconv.Quote when
the value contains both single and double quotes, but TrimQuotes (used by
parseEnvFile) only strips outer quotes and doesn't unescape Go-style quoted
strings, breaking round-trip correctness; either (A) update TrimQuotes in
pkg/utils/strings.go to detect double-quoted values and call strconv.Unquote to
properly unescape Go-style escapes (ensuring errors are handled), or (B) change
encodeEnvValue to never use strconv.Quote by emitting a single-quoted shell-safe
form that escapes internal single quotes using the shell convention ('\''), so
parseEnvFile/TrimQuotes can continue to just strip outer single quotes — pick
one approach and implement it consistently across encodeEnvValue, TrimQuotes,
and parseEnvFile to preserve round-trip integrity.
In `@internal/notify/email_relay.go`:
- Around line 53-56: Change EmailRelayResponse.Success from a plain bool to a
pointer (*bool) so you can detect a missing field (use
EmailRelayResponse.Success *bool). Update the response-parsing/handling logic
that currently checks Success (the code around where EmailRelayResponse is
decoded and the branch at the current 148-163) to treat Success==nil as a
non-failure (backwards-compatible accepted case) and only treat explicit false
as failure; ensure any boolean checks dereference the pointer safely (e.g., if
resp.Success != nil && !*resp.Success then failure).
---
Nitpick comments:
In `@cmd/proxsave/install_existing_config.go`:
- Around line 63-80: The repeated ctx.Err() checks inside each switch case
(around returns for existingConfigKeepContinue, existingConfigOverwrite,
existingConfigEdit, existingConfigCancel) are redundant because promptOptional
already handles cancellation; remove the per-case ctx.Err() calls and perform a
single context cancellation check once either immediately before the switch or
immediately after the switch falls through to determine cancellation, keeping
the same return values for the cases (existingConfigKeepContinue,
existingConfigOverwrite, existingConfigEdit, existingConfigCancel) and
preserving behavior if ctx.Err() is non-nil.
In `@internal/config/env_mutation_test.go`:
- Around line 88-111: Add a new unit test that exercises the strconv.Quote
fallback path by calling ApplySecondaryStorageSettings with a path containing
both single and double quotes (e.g. `/mnt/it's "special"`), writing the returned
env to disk and asserting that parseEnvFile returns the identical raw value for
"SECONDARY_PATH"; this will reference
TestApplySecondaryStorageSettingsQuotesBothQuoteTypes (new),
ApplySecondaryStorageSettings, encodeEnvValue (which triggers strconv.Quote),
and parseEnvFile so the round-trip failure is reproducible and can guide fixing
the encode/unquote behavior.
In `@internal/notify/webhook_test.go`:
- Around line 330-381: The test TestWebhookNotifier_Send_Retry uses a plain int
attempts counter; change it to an atomic counter (e.g., var attempts
atomic.Int32 or var attempts int32 and use atomic.AddInt32) and update the
httptest.NewServer handler to increment via atomic.AddInt32(&attempts, 1) and
read via atomic.LoadInt32 when asserting the expected number of attempts; also
add the necessary import for sync/atomic (or use the atomic.Int32 type with
import "sync/atomic") so the Go race detector is satisfied and the test is
consistent with
TestWebhookNotifier_SendToEndpoint_StopsRetryingWhenContextCanceled.
In `@internal/orchestrator/restore_ha_additional_test.go`:
- Around line 387-395: Extract the duplicated newEnv closure into a shared test
helper function (e.g., newHATestEnv or setupStagedHATestEnv) that returns
*haTestEnv and calls setupHATestEnv, adds the staged HA config file at
env.stageRoot + "/etc/pve/ha/resources.cfg" and fails the test on error; replace
both local newEnv closures in restore_ha_additional_test.go (the one around
lines 387–395 and the one around 666–673) with calls to this new helper and
update their callers to use the shared helper to avoid duplication and drift.
In `@internal/orchestrator/restore_tui.go`:
- Around line 987-1020: The countdown goroutine in this function uses an ad-hoc
shutdown pattern (stopCh/done/ticker.Stop() and waiting for <-done) which is
inconsistent with the scoped stopTicker helper used by
promptYesNoTUIWithCountdown; refactor this block to use the same stopTicker
pattern: create the stopTicker function (or reuse the existing one) to
encapsulate closing the stop channel, stopping the ticker and waiting for the
goroutine to finish, then replace the inline close(stopCh); ticker.Stop();
<-done calls before and after app.RunWithContext(ctx) with a single stopTicker()
call and remove duplicated shutdown logic in the anonymous goroutine (refer to
symbols stopCh, done, ticker, app.RunWithContext and promptYesNoTUIWithCountdown
to locate and update the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff310e78-7376-4c9e-8148-d9bbefa41822
📒 Files selected for processing (28)
cmd/proxsave/install.gocmd/proxsave/install_existing_config.gocmd/proxsave/install_existing_config_test.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.godocs/CONFIGURATION.mddocs/TROUBLESHOOTING.mdinternal/config/config_test.gointernal/config/env_mutation.gointernal/config/env_mutation_test.gointernal/config/migration_test.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/notify/email_relay.gointernal/notify/email_relay_test.gointernal/notify/webhook_test.gointernal/orchestrator/decrypt_tui_e2e_helpers_test.gointernal/orchestrator/decrypt_workflow_ui.gointernal/orchestrator/decrypt_workflow_ui_test.gointernal/orchestrator/restore_ha_additional_test.gointernal/orchestrator/restore_tui.gointernal/orchestrator/restore_tui_simulation_test.gointernal/orchestrator/workflow_ui_cli.gointernal/orchestrator/workflow_ui_cli_test.gointernal/storage/backup_files.gointernal/storage/backup_files_test.gointernal/storage/cloud.gointernal/storage/cloud_test.go
💤 Files with no reviewable changes (1)
- cmd/proxsave/install.go
| - Allowed values for `EMAIL_DELIVERY_METHOD` are: `relay`, `sendmail`, `pmf` (invalid values will skip Email with a warning). | ||
| - `EMAIL_FALLBACK_SENDMAIL` is a historical name (kept for compatibility). When `EMAIL_DELIVERY_METHOD=relay`, it enables fallback to **pmf** (it will not fall back to `/usr/sbin/sendmail`). | ||
| - `relay` requires a real mailbox recipient and blocks `root@…` recipients; set `EMAIL_RECIPIENT` to a non-root mailbox if needed. | ||
| - When logs say the relay "accepted request", it means the worker and upstream email API accepted the submission. It does **not** guarantee final inbox delivery (the message may still bounce, be deferred, or land in spam later). |
There was a problem hiding this comment.
Soften the guarantee in this sentence.
Line 849 is a bit stronger than the current implementation. sendViaCloudRelay still treats some HTTP 200 responses with empty or unparsable bodies as success, so this log line does not always prove the upstream provider accepted the message. Wording this as “the relay accepted the request / returned HTTP 200” would be safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CONFIGURATION.md` at line 849, The log sentence overstates guarantee;
update the wording to reflect that "accepted" refers to the relay returning an
HTTP 200-style response rather than guaranteed delivery—change the line to
something like "When logs say the relay 'accepted request' or 'returned HTTP
200', it means sendViaCloudRelay (and the upstream email API) returned a success
response; it does not guarantee final inbox delivery (the message may still
bounce, be deferred, or land in spam later)." Ensure the change references
sendViaCloudRelay to align wording with its current behavior when it treats
empty or unparsable 200 responses as success.
Comment on lines
+19
to
+25
| if !strings.Contains(value, "'") { | ||
| return "'" + value + "'" | ||
| } | ||
| if !strings.Contains(value, `"`) { | ||
| return `"` + value + `"` | ||
| } | ||
| return strconv.Quote(value) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if TrimQuotes handles strconv.Quote escape sequences
# Find TrimQuotes implementation
rg -n -A10 'func TrimQuotes' pkg/utils/
# Check if there's any strconv.Unquote usage in parsing
rg -n 'strconv\.Unquote' internal/config/ pkg/utils/Repository: tis24dev/proxsave
Length of output: 615
🏁 Script executed:
# Find parseEnvFile and see how it processes values
rg -n -A20 'func parseEnvFile' internal/config/
# Check how encodeEnvValue results are used
rg -n -B5 -A5 'encodeEnvValue' internal/config/
# Look for any unquoting logic after parsing
rg -n 'TrimQuotes' internal/config/ pkg/Repository: tis24dev/proxsave
Length of output: 3319
🏁 Script executed:
# Get the complete parseEnvFile function
sed -n '1329,1400p' internal/config/config.go
# Get SplitKeyValue implementation
rg -n -A15 'func SplitKeyValue' pkg/utils/
# Check the full context of line 116 in strings.go
sed -n '110,125p' pkg/utils/strings.goRepository: tis24dev/proxsave
Length of output: 3276
🏁 Script executed:
# Check where SECONDARY_PATH and SECONDARY_LOG_PATH are used
rg -n 'SECONDARY_PATH|SECONDARY_LOG_PATH' internal/config/
# Check for test cases that might trigger strconv.Quote path
rg -n 'encodeEnvValue' --include='*_test.go'
# Look for any tests with paths containing both quotes
rg -n "both.*quote|quote.*both" pkg/utils/ internal/config/Repository: tis24dev/proxsave
Length of output: 7685
🏁 Script executed:
# Search for tests that might have paths with both quotes
rg -n "both.*quote|'.*\"|\"\.*'" pkg/utils/ internal/config/ --type go
# Check env_mutation_test.go for all test cases
sed -n '1,120p' internal/config/env_mutation_test.goRepository: tis24dev/proxsave
Length of output: 5474
Round-trip correctness issue with strconv.Quote fallback.
When a path contains both single and double quotes, encodeEnvValue uses strconv.Quote (line 25), producing Go-style escape sequences like "path'with\"both". However, TrimQuotes in pkg/utils/strings.go only strips outer quotes without calling strconv.Unquote, so values read back from the file via parseEnvFile will be corrupted (e.g., path'with\"both instead of path'with"both).
The round-trip test (TestApplySecondaryStorageSettingsQuotesUnsafePaths) currently only covers paths with spaces/special characters that use the single-quote path, not the strconv.Quote fallback. A path with both quote types would silently fail.
Consider updating TrimQuotes to use strconv.Unquote for double-quoted values, or avoid the strconv.Quote path by escaping single quotes within single-quoted strings using the shell convention ('\'').
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/env_mutation.go` around lines 19 - 25, encodeEnvValue
currently falls back to strconv.Quote when the value contains both single and
double quotes, but TrimQuotes (used by parseEnvFile) only strips outer quotes
and doesn't unescape Go-style quoted strings, breaking round-trip correctness;
either (A) update TrimQuotes in pkg/utils/strings.go to detect double-quoted
values and call strconv.Unquote to properly unescape Go-style escapes (ensuring
errors are handled), or (B) change encodeEnvValue to never use strconv.Quote by
emitting a single-quoted shell-safe form that escapes internal single quotes
using the shell convention ('\''), so parseEnvFile/TrimQuotes can continue to
just strip outer single quotes — pick one approach and implement it consistently
across encodeEnvValue, TrimQuotes, and parseEnvFile to preserve round-trip
integrity.
Comment on lines
53
to
56
| type EmailRelayResponse struct { | ||
| Success bool `json:"success"` | ||
| Code string `json:"code,omitempty"` | ||
| Message string `json:"message,omitempty"` |
There was a problem hiding this comment.
Differentiate a missing success field from an explicit failure.
Lines 53-56 make success a plain bool, so Lines 148-163 treat any valid JSON 200 body without that field as success=false. That turns responses like {} or {"message":"accepted"} into hard failures, which breaks the backward-compatibility path described in the comment and can reject older/custom relay workers unnecessarily.
Proposed fix
type EmailRelayResponse struct {
- Success bool `json:"success"`
+ Success *bool `json:"success,omitempty"`
Code string `json:"code,omitempty"`
Message string `json:"message,omitempty"`
Error string `json:"error,omitempty"`
}
@@
- if len(body) > 0 {
+ if len(body) > 0 {
var apiResp EmailRelayResponse
- if err := json.Unmarshal(body, &apiResp); err == nil && !apiResp.Success {
+ if err := json.Unmarshal(body, &apiResp); err == nil && apiResp.Success != nil && !*apiResp.Success {
errMsg := apiResp.Error
if errMsg == "" {
errMsg = apiResp.MessageAlso applies to: 148-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/notify/email_relay.go` around lines 53 - 56, Change
EmailRelayResponse.Success from a plain bool to a pointer (*bool) so you can
detect a missing field (use EmailRelayResponse.Success *bool). Update the
response-parsing/handling logic that currently checks Success (the code around
where EmailRelayResponse is decoded and the branch at the current 148-163) to
treat Success==nil as a non-failure (backwards-compatible accepted case) and
only treat explicit false as failure; ensure any boolean checks dereference the
pointer safely (e.g., if resp.Success != nil && !*resp.Success then failure).
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
Release Notes
New Features
Bug Fixes
Documentation