Sync dev to main by tis24dev · Pull Request #131 · tis24dev/proxsave

tis24dev · 2026-02-03T21:26:47Z

Improve env config parsing and upgrade logic
Report config upgrade warnings

Refactor config parsing and upgrade flow to better handle comments, casing and multiline values. - setEnvValue: skip commented lines, preserve indentation and inline comments when replacing values. - parseEnvFile/parseEnvValues: ignore comment-only lines inside multi-line block values; return comment text and a caseMap (UPPER->original) for case-insensitive matching; add comment storage to envValue. - splitKeyValueRaw: return parsed inline comment, handle legacy "export " prefixes, and more robustly extract quoted/unquoted values and trailing comments. - computeConfigUpgrade: use case-insensitive lookups via caseMap, track processed user keys, preserve user key casing for extras, count preserved values correctly, and detect no-op upgrades by comparing newContent with originalContent. - renderEnvValue: include preserved inline comments when rendering single-line values. - Added import of utils.IsComment where needed. Also updated internal/orchestrator/.backup.lock (pid/time) as part of the changeset.

Add non-fatal warnings support to the config upgrade flow. Introduce a Warnings []string field on UpgradeResult and propagate warning messages from parseEnvValues through computeConfigUpgrade. Detect and warn about duplicate template keys that differ only by case, case-collision between user keys and template keys, and ignored non-KEY=VALUE lines; preserve behavior for multi-line values. Surface these warnings in the CLI (cmd/proxsave/main.go and cmd/proxsave/upgrade.go) so they are printed during plan/upgrade operations. Update parseEnvValues signature to return case conflict info and warnings, and add tests verifying case-collision and ignored-line warnings.

Copilot

Pull request overview

This PR syncs changes from the dev branch to main, focusing on improving environment configuration parsing and upgrade logic with enhanced warning reporting.

Changes:

Added case-insensitive key matching with collision detection and warnings for duplicate keys that differ only in case
Introduced inline comment preservation during config upgrades and support for legacy "export KEY=VALUE" syntax
Enhanced multi-line block value parsing to skip comment lines, consistent with runtime parsing
Added warning system to report non-fatal parsing issues (ignored lines, case collisions, etc.) to users during config upgrades

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
internal/orchestrator/.backup.lock	Updated runtime lock file with new PID and timestamp (should not be in version control)
internal/config/upgrade_test.go	Added tests for case collision warnings and ignored line warnings
internal/config/upgrade.go	Implemented case-insensitive key matching, comment preservation, export statement handling, and warning generation
internal/config/config.go	Added comment filtering in multi-line block value parsing
cmd/proxsave/upgrade.go	Added warning output display in upgrade footer
cmd/proxsave/main.go	Added warning output for both plan and execute upgrade modes
cmd/proxsave/config_helpers.go	Enhanced setEnvValue to preserve comments and handle key matching without prefix checking

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-03T21:32:05Z

cmd/proxsave/config_helpers.go

+			if idx := strings.Index(line, "#"); idx >= 0 {
+				// Verify # is not part of the key or value?
+				// Assuming standard comment
+				commentPart := line[idx:]
+				// Ensure it's not inside quotes? The original logic didn't check quotes carefully but let's be safe(r).
+				// For setEnvValue we are replacing the value, so we just want to keep the comment at the end.
+				comment = commentPart
+
+				// Find spacing before comment
+				beforeComment := line[:idx]


The comment extraction logic at line 76 doesn't handle the case where the '#' character might be inside quoted values. For example, if value is "some#value", this code will incorrectly treat "#value" as a comment. Consider using similar logic to splitKeyValueRaw (lines 405-423 in upgrade.go) which properly handles quotes when extracting comments.

Suggested change

if idx := strings.Index(line, "#"); idx >= 0 {

// Verify # is not part of the key or value?

// Assuming standard comment

commentPart := line[idx:]

// Ensure it's not inside quotes? The original logic didn't check quotes carefully but let's be safe(r).

// For setEnvValue we are replacing the value, so we just want to keep the comment at the end.

comment = commentPart

// Find spacing before comment

beforeComment := line[:idx]

// Find a comment start '#' that is not inside quotes

commentIdx := -1

inSingle := false

inDouble := false

escaped := false

for i, r := range line {

if escaped {

escaped = false

continue

}

switch r {

case '\\':

escaped = true

case '\'':

if !inDouble {

inSingle = !inSingle

}

case '"':

if !inSingle {

inDouble = !inDouble

}

case '#':

if !inSingle && !inDouble {

commentIdx = i

// First '#' outside quotes is treated as the start of a comment

break

}

}

if commentIdx != -1 {

break

}

}

if commentIdx >= 0 {

commentPart := line[commentIdx:]

// For setEnvValue we are replacing the value, so we just want to keep the comment at the end.

comment = commentPart

// Find spacing before comment

beforeComment := line[:commentIdx]

Copilot · 2026-02-03T21:32:06Z

internal/config/upgrade.go

+	// Handle legacy "export KEY=VALUE" lines
+	if strings.HasPrefix(key, "export ") {
+		key = strings.TrimSpace(strings.TrimPrefix(key, "export "))
+	}
+	// Also handle tab separation just in case "export\tKEY"
+	if strings.HasPrefix(key, "export\t") {
+		key = strings.TrimSpace(strings.TrimPrefix(key, "export\t"))
+	}


The export handling logic only checks for "export " (with space) and "export\t" (with tab) prefixes. However, it doesn't handle the case where there might be multiple spaces or tabs after "export", such as "export KEY=VALUE" or "export\t\tKEY=VALUE". Consider using a more robust approach that handles any whitespace after "export", for example: if strings.HasPrefix(key, "export") { key = strings.TrimSpace(strings.TrimPrefix(key, "export")) }

Suggested change

// Handle legacy "export KEY=VALUE" lines

if strings.HasPrefix(key, "export ") {

key = strings.TrimSpace(strings.TrimPrefix(key, "export "))

}

// Also handle tab separation just in case "export\tKEY"

if strings.HasPrefix(key, "export\t") {

key = strings.TrimSpace(strings.TrimPrefix(key, "export\t"))

}

// Handle legacy "export KEY=VALUE" lines with arbitrary whitespace after "export"

if strings.HasPrefix(key, "export") {

key = strings.TrimSpace(strings.TrimPrefix(key, "export"))

}

Copilot · 2026-02-03T21:32:06Z

internal/config/upgrade.go

+	line := fmt.Sprintf("%s=%s", key, value.rawValue)
+	if value.comment != "" {
+		line += " " + value.comment
+	}
+	return []string{line}


The new comment preservation functionality (storing and re-rendering inline comments) is not covered by tests. Consider adding test cases to verify that inline comments are correctly preserved during config upgrades, both for quoted and unquoted values.

Copilot · 2026-02-03T21:32:06Z

internal/orchestrator/.backup.lock

+pid=99930
 host=pve
-time=2026-02-03T00:07:31+01:00
+time=2026-02-03T19:42:03+01:00


This backup lock file appears to contain PID and timestamp information that is automatically generated. It should not be included in version control as it represents runtime state and will create merge conflicts. Consider adding this file to .gitignore.

Copilot · 2026-02-03T21:32:06Z

internal/config/upgrade.go

+	// Handle legacy "export KEY=VALUE" lines
+	if strings.HasPrefix(key, "export ") {
+		key = strings.TrimSpace(strings.TrimPrefix(key, "export "))
+	}
+	// Also handle tab separation just in case "export\tKEY"
+	if strings.HasPrefix(key, "export\t") {
+		key = strings.TrimSpace(strings.TrimPrefix(key, "export\t"))
+	}


The new export handling functionality (lines 390-397) added to splitKeyValueRaw is not covered by tests. Consider adding test cases to verify that lines with "export KEY=VALUE" are correctly parsed, including edge cases like multiple spaces after export, tabs, and combinations thereof.

internal/config/upgrade.go

Copilot · 2026-02-03T21:32:07Z

internal/config/upgrade.go

+	// BUT checking "nothing changed" is harder now because we might have renamed keys.
+	// If we renamed a key, the content CHANGED.
+	// So we should compare normalized content?
+	// Or just assume if we parsed everything and re-rendered, and it matches original string...


These commented lines look like leftover notes from development explaining why the early return logic was modified. Since this logic has been replaced (lines 312-317), these comments should be removed to keep the code clean.

Suggested change

// BUT checking "nothing changed" is harder now because we might have renamed keys.

// If we renamed a key, the content CHANGED.

// So we should compare normalized content?

// Or just assume if we parsed everything and re-rendered, and it matches original string...

Copilot · 2026-02-03T21:32:07Z

internal/config/upgrade.go

+
+		upperKey := strings.ToUpper(key)
+		if templateKey, ok := templateKeyByUpper[upperKey]; ok && templateKey != key {
+			if caseConflicts == nil || !caseConflicts[upperKey] {


The condition caseConflicts == nil will never be true because caseConflicts is always initialized as a non-nil map in parseEnvValues at line 331. This check should just be !caseConflicts[upperKey] to verify that this particular upper-cased key doesn't have a conflict.

Suggested change

if caseConflicts == nil || !caseConflicts[upperKey] {

if !caseConflicts[upperKey] {

Copilot · 2026-02-03T21:32:07Z

internal/config/upgrade.go

-				return nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1)
+				return nil, nil, nil, nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1)
 			}
 			blockLines = append(blockLines, lines[i+1:blockEnd]...)


The block value parsing here collects all lines as-is (line 363), including comment lines within the block. This is inconsistent with the behavior in config.go (lines 1231-1233) which filters out comments from block values. This inconsistency could lead to comments being preserved during upgrades but ignored during actual config parsing, causing confusion. Consider filtering out comments here as well for consistency.

Suggested change

blockLines = append(blockLines, lines[i+1:blockEnd]...)

for j := i + 1; j < blockEnd; j++ {

blkLine := lines[j]

if utils.IsComment(strings.TrimSpace(blkLine)) {

continue

}

blockLines = append(blockLines, blkLine)

}

Copilot · 2026-02-03T21:32:08Z

internal/config/upgrade.go

+
+		caseMap[upperKey] = key
+
 		if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) {


The check for blockValueKeys uses the user's key casing directly, but blockValueKeys map contains uppercase keys (CUSTOM_BACKUP_PATHS, BACKUP_BLACKLIST). If a user has a lowercase or mixed-case version of these keys, the block value won't be detected and will be parsed as a regular line instead. This should use a case-insensitive check, similar to how the template parsing handles it at line 212.

Suggested change

if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) {

if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) {

Introduce utils string helpers (FindInlineCommentIndex, FindClosingQuoteIndex, SetEnvValue) to centralize handling of inline comments, escaped hashes and quoted values. Replace local setEnvValue implementations to call utils.SetEnvValue and update SplitKeyValue/splitKeyValueRaw to use the new helpers for robust parsing. Fix case-insensitive handling of block value keys in config upgrade logic (use upperKey). Add unit tests across packages to cover escaped hashes, preserved comments, quoted values, and SetEnvValue behavior. Minor reformatting/import adjustments in the install wizard to integrate the new utils usage.

Replace manual HasPrefix checks with a Fields-based parse to handle legacy "export KEY=VALUE" lines that use arbitrary whitespace (spaces or tabs). This prevents accidentally stripping keys like "exporter" while still recognizing true "export" prefixes. Added tests to ensure export-with-whitespace is accepted and that keys named "exporter" are not treated as exports.

Add TestUpgradeConfigPreservesInlineComments which verifies UpgradeConfigFile preserves inline comments when adding missing keys. The test seeds a temporary config with inline and quoted '#' comments, runs UpgradeConfigFile, asserts result.Changed is true, and checks the upgraded file still contains the original inline comments for KEY1 and KEY2.

Extend TestPlanUpgradeConfigHandlesExportWhitespace in internal/config/upgrade_test.go to include an export line with tab characters (export\t\tKEY1=custom-tabbed). This ensures the upgrade parsing handles export lines with tabs as well as spaces.

Remove the redundant `caseConflicts == nil` check when warning about keys that differ only by case. Reading a nil map in Go is safe, so using `!caseConflicts[upperKey]` preserves behavior while simplifying the condition and reducing clutter in internal/config/upgrade.go.

Adds TestUpgradeConfigPreservesBlockComments to internal/config/upgrade_test.go. The new test seeds a user config containing a multi-line variable with inline comment lines, runs UpgradeConfigFile, asserts that an upgrade occurs (result.Changed == true) and verifies the block-style comment lines and content are preserved in the upgraded file.

parseEnvFile: normalize parsed environment variable names to uppercase and support lines beginning with "export". Lookups against blockValueKeys and multiValueKeys now use the uppercased key, and stored map keys are uppercased so legacy/lowercase env files are handled consistently. Added tests verifying lowercase keys are normalized (including multi-line block values and multi-value accumulation) and that lines prefixed with various spacing for "export" are parsed correctly.

codecov · 2026-02-03T22:59:43Z

Codecov Report

❌ Patch coverage is 76.75676% with 43 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/tui/wizard/install.go	3.03%	32 Missing ⚠️
internal/config/upgrade.go	90.90%	4 Missing and 2 partials ⚠️
pkg/utils/strings.go	93.24%	3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

tis24dev added 2 commits February 3, 2026 21:27

Copilot AI review requested due to automatic review settings February 3, 2026 21:26

Copilot started reviewing on behalf of tis24dev February 3, 2026 21:27 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

tis24dev added 7 commits February 3, 2026 23:18

tis24dev closed this Feb 3, 2026

tis24dev reopened this Feb 3, 2026

tis24dev closed this Feb 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Sync dev to main#131

Sync dev to main#131
tis24dev wants to merge 9 commits intomainfrom
dev

tis24dev commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

codecov bot commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-			if idx := strings.Index(line, "#"); idx >= 0 {
-				// Verify # is not part of the key or value?
-				// Assuming standard comment
-				commentPart := line[idx:]
-				// Ensure it's not inside quotes? The original logic didn't check quotes carefully but let's be safe(r).
-				// For setEnvValue we are replacing the value, so we just want to keep the comment at the end.
-				comment = commentPart
-				// Find spacing before comment
-				beforeComment := line[:idx]
+			// Find a comment start '#' that is not inside quotes
+			commentIdx := -1
+			inSingle := false
+			inDouble := false
+			escaped := false
+			for i, r := range line {
+				if escaped {
+					escaped = false
+					continue
+				}
+				switch r {
+				case '\\':
+					escaped = true
+				case '\'':
+					if !inDouble {
+						inSingle = !inSingle
+					}
+				case '"':
+					if !inSingle {
+						inDouble = !inDouble
+					}
+				case '#':
+					if !inSingle && !inDouble {
+						commentIdx = i
+						// First '#' outside quotes is treated as the start of a comment
+						break
+					}
+				}
+				if commentIdx != -1 {
+					break
+				}
+			}
+			if commentIdx >= 0 {
+				commentPart := line[commentIdx:]
+				// For setEnvValue we are replacing the value, so we just want to keep the comment at the end.
+				comment = commentPart
+				// Find spacing before comment
+				beforeComment := line[:commentIdx]

	if caseConflicts == nil \|\| !caseConflicts[upperKey] {
	if !caseConflicts[upperKey] {

-			blockLines = append(blockLines, lines[i+1:blockEnd]...)
+			for j := i + 1; j < blockEnd; j++ {
+				blkLine := lines[j]
+				if utils.IsComment(strings.TrimSpace(blkLine)) {
+					continue
+				}
+				blockLines = append(blockLines, blkLine)
+			}


		caseMap[upperKey] = key

		if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) {

	if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) {
	if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) {

Uh oh!

Comments

Conversation

tis24dev commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 3, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant