fix(restic): support quoted keep-tags and clarify separators

[caffeinated92]

Summary
Improve restic keep‑tag handling by allowing quoted literals (including commas) and standardizing tag separation on spaces, while preserving legacy comma support.

What changed
Keep‑tag parsing now splits on whitespace/commas outside quotes.
Quoted tags are treated as literal values and can include commas (AND semantics).
UI tooltip/helper text, config defaults, and flag descriptions updated.
Docs updated to reflect keep‑tag rules and supported placeholders.
Why
Restic interprets commas inside a keep‑tag as AND, which conflicted with the previous comma‑separated UI input. The new behavior reduces ambiguity and aligns with restic semantics.

[claude]

Code Review - PR #1338: Restic Keep-Tag Handling

✅ Strengths

1. Well-motivated refactoring

• The motivation is clear: restic interprets commas inside keep-tags as AND semantics, which conflicted with comma-separated UI input
• The solution (quoted literals + whitespace separation) is pragmatic and reduces ambiguity

2. Good parser design

• splitResticKeepTagTemplates() and splitResticTagTemplates() cleanly separate concerns
• Proper handling of escape sequences and quote characters (both single and double quotes)
• Deduplication logic prevents duplicate tags from being sent to restic

3. Comprehensive helper functions

• isQuotedResticTagLiteral() and unquoteResticTagLiteral() follow clear logic
• Functions have single, well-defined responsibilities
• Escape handling is implemented correctly for double-quoted strings

4. Frontend parity

• JavaScript implementation (splitKeepTagTemplates in ResticPurgeStrategy/index.jsx) mirrors the Go logic
• Both implementations handle the same parsing rules consistently

5. Documentation improvements

• Help text and tooltips updated to reflect new behavior
• Examples show quoted tags with commas: "role:primary,critical"
• Clear explanation of placeholder support ({cluster}, {tenant})

6. UI/CSS enhancements

• Removed duplication by moving shared modal styles to components/Modals/styles.module.scss
• Added CSS variables for better theming/maintainability
• Improved modal styling with consistent padding and spacing

---

⚠️ Issues & Concerns

1. Inconsistency in handling empty/trailing input
In splitResticKeepTagTemplates() (Go), line 471:

if current.Len() > 0 || strings.HasSuffix(value, ",") {
    parts = append(parts, current.String())
}

This preserves empty parts when the input ends with a comma, but splitResticTagTemplates() has the same logic despite being used for tags that shouldn't need this behavior. Both use commas as primary separators but have different use cases.

2. Escape handling only for double quotes
In unquoteResticTagLiteral(), escape sequences are only processed for double-quoted strings (lines 502-516):

if quote == '\'' {
    return raw, true
}
// Only double quotes get escape processing

This means "tag\,with\,literal" works but 'tag\,with\,literal' doesn't unescape backslashes. Consider whether single quotes should also process escapes or document this as intentional.

3. Missing template validation for keep-tags
renderResticKeepTagTemplate() only validates {cluster} and {tenant} placeholders (defined in resticKeepTagTemplateKeySet). Unlike renderResticTagTemplate() which validates against resticTagTemplateKeySet, the reduced set is correct for keep-tags, but there's no warning if unknown placeholders like {engine} are used.

4. Go test coverage gap
No unit tests visible in the PR for the new parsing functions:

• splitResticKeepTagTemplates()
• splitResticTagTemplates()
• isQuotedResticTagLiteral()
• unquoteResticTagLiteral()
• renderResticKeepTagTemplate()

These are critical functions that handle user input. Test cases should cover:

• Quoted tags with commas: "role:primary,critical" "env:prod"
• Mixed quoted and unquoted: cluster,"env:prod,team:dev"
• Escape sequences: "tag\\,escaped"
• Edge cases: empty input, trailing/leading whitespace, only quotes, unmatched quotes

5. JavaScript test coverage
The JavaScript parsing function (splitKeepTagTemplates in ResticPurgeStrategy/index.jsx) also lacks tests for the complex parsing logic.

6. Potential injection via templates
The code correctly uses strings.ReplaceAll() to substitute placeholders, which is safe. However, ensure that cluster.Name and cluster.Conf.Cloud18GitUser are properly validated upstream to prevent unintended template expansions.

7. Error handling in ResticPurgeRepo()
If renderResticKeepTagTemplate() returns false (line 199), the tag is silently skipped without logging unless cluster != nil. Consider if this warrants a warning log, as users might expect all their keep-tags to be applied.

---

📋 Suggestions

1. Add unit tests (Go)

func TestSplitResticKeepTagTemplates(t *testing.T) {
    cases := []struct {
        input    string
        expected []string
    }{
        {`"role:primary,critical" "env:prod"`, []string{`"role:primary,critical"`, `"env:prod"`}},
        {`cluster,"env:prod,team:dev"`, []string{`cluster`, `"env:prod,team:dev"`}},
        {`  tag1   tag2  `, []string{`tag1`, `tag2`}},
    }
    // test cases...
}

2. Document escape handling behavior
Add a comment explaining why single quotes don't process escapes:

// Single quotes preserve content literally (no escape processing)
// Double quotes allow \\ and \" escapes

3. Consistent error logging
In ResticPurgeRepo(), consider logging when templates fail to render:

rendered, ok := renderResticKeepTagTemplate(template, keepValues, cluster)
if !ok {
    cluster.LogModulePrintf(cluster.Conf.Verbose, config.ConstLogModTask, 
        config.LvlWarn, "Failed to render keep-tag template: %q", template)
    continue
}

4. Config default clarity
In config.toml (line 189), the comment could be clearer:

# Protect snapshots matching these tags from purge
# Space and comma separated (comma = AND). Quote tags with commas: "role:primary,critical"
backup-restic-purge-keep-tag = "line:adhoc env:prod"

---

🔒 Security Assessment

✅ No security issues identified

• Input is properly escaped/quoted
• No command injection vectors (restic args are passed safely)
• No SQL injection or template injection risks
• Safe string substitution via strings.ReplaceAll()

---

Summary

This PR delivers solid improvements to the restic keep-tag handling with good backward compatibility. The main gaps are in test coverage and minor documentation clarity. The parsing logic is sound and the duplication removal in styles is a nice cleanup.

Recommendation: 👍 Approve with minor suggestions

• Add unit tests before merge (or shortly after as a follow-up)
• Consider adding a comment about escape handling
• Optional: Add warning logs for failed template renders

[caffeinated92]

@claude review my pull request

[claude]

Claude encountered an error after 3m 0s —— View job

---

Code Review - PR #1338: Restic Keep-Tag Handling

✅ Strengths

1. Well-motivated refactoring

• Clear motivation: restic interprets commas inside keep-tags as AND semantics, conflicting with comma-separated UI input
• Pragmatic solution (quoted literals + whitespace separation) reduces ambiguity and aligns with restic semantics

2. Excellent parser design

• splitResticKeepTagTemplates() and splitResticTagTemplates() cleanly separate concerns
• Proper handling of escape sequences and quote characters (both single and double)
• Deduplication logic in parseResticKeepTagTemplates() prevents duplicate tags sent to restic
• Both Go and JavaScript implementations mirror each other for consistency

3. Comprehensive helper functions

• isQuotedResticTagLiteral() and unquoteResticTagLiteral() follow clear logic with single responsibilities
• Escape handling correctly implemented:
  • Double quotes: \ escapes the next character (\\ → \, \" → ")
  • Single quotes: Preserve content literally (no escape processing)
• Proper edge case handling: unmatched quotes, empty input, trailing/leading whitespace

4. Good test coverage

• Unit test added (TestSplitResticKeepTagTemplates) covers primary use cases:
  • Quoted tags with commas: "role:primary,critical" "env:prod"
  • Mixed quoted/unquoted: cluster,"env:prod,team:dev"
  • Multiple whitespace: tag1 tag2
• All tests pass ✓

5. Frontend parity

• JavaScript implementation (splitKeepTagTemplates in ResticPurgeStrategy/index.jsx) mirrors Go logic
• Both handle identical parsing rules consistently
• buildForgetCommand() properly constructs restic commands with quoted tags

6. Documentation & UX improvements

• Help text and tooltips updated to reflect new behavior
• Examples clearly show quoted tags with commas: "role:primary,critical"
• Placeholder support documented: {cluster}, {tenant}
• Config defaults and flag descriptions clarified
• Error logging implemented for failed template renders (line 201)

7. Clean logging & error handling

• Log warning when template render fails (line 201): "Failed to render restic keep-tag template"
• Graceful degradation: skips failed templates instead of aborting

---

⚠️ Issues & Concerns

1. Limited test coverage for edge cases
Current test only covers 3 cases. Missing coverage for:

• Unmatched quotes: "unclosed → should work (treated as literal)
• Empty input: `` → should return empty slice
• Only whitespace: → should return empty slice
• Trailing commas: tag1, → [tag1] (no empty part)
• Unquoted single-char tags: a b → [a, b]
• Escape sequences in double quotes: "test\," → should parse as literal test,
• Single quotes with backslashes: 'test\,literal' → should preserve \, literally

Verified behavior (all working correctly ✓):

"test\," → test,          ✓ (escape works)
'test\,' → test\,         ✓ (single quotes preserve)
"a\\b"  → a\b            ✓ (double backslash)
"unclosed → "unclosed    ✓ (unmatched quote)

Recommendation: Expand cluster_bck_test.go with additional test cases to document behavior and catch regressions.

---

2. Single vs. Double quote escape semantics
The behavior difference is intentional and documented in code comments (line 501, 523):

• Single quotes: preserve literally (no escape processing)
• Double quotes: allow \\ and \" escapes

This is reasonable and follows shell conventions, but could trip up users expecting consistent behavior. The tooltip mentions this but briefly.

Suggestion: Add explicit comment in config file example:

# Single quotes preserve backslashes literally, double quotes process escapes
# backup-restic-purge-keep-tag = 'tag1\,literal' "tag2\,escaped"

---

3. No validation for unknown placeholders in keep-tags

• renderResticKeepTagTemplate() (line 619-623) returns error if unsupported placeholder found
• Only {cluster} and {tenant} are valid for keep-tags (correct set)
• But warning log (line 621) uses "Unsupported" which is clearer than in regular tags

Current behavior is correct ✓. Error handling prevents silent data loss.

---

4. Potential confusion with trailing content after unmatched quotes

input: tag1 "incomplete tag2
result: [tag1, "incomplete tag2]

The unclosed quote is preserved as-is. This might confuse users, though it's conservative (doesn't silently truncate).

Suggestion: Consider adding a validation log if quote is never closed - user can then correct the config.

---

📋 Detailed Code Analysis

Parsing Logic - splitResticKeepTagTemplates() (lines 438-481)

• ✓ Correctly handles quote tracking across iterations
• ✓ Escape sequences properly managed (only for double quotes within quotes)
• ✓ Comma/whitespace separators work correctly outside quotes
• ✓ Final part correctly appended only if non-empty

Rendering - renderResticKeepTagTemplate() (lines 590-637)

• ✓ Properly unquotes literals first (allowing comma-separated values)
• ✓ Falls back to template substitution if not a literal
• ✓ Validates placeholder keys against resticKeepTagTemplateKeySet
• ✓ Returns both value and success boolean for error handling
• ✓ Graceful logging of validation failures

Usage in ResticPurgeRepo() (lines 192-207)

• ✓ Parses templates from config (line 192)
• ✓ Attempts to render each template (line 199)
• ✓ Logs failures with template name (line 201)
• ✓ Skips empty results (line 204-205)
• ✓ Passes clean tags to ResticManager.AddPurgeTask()

---

🔒 Security Assessment

✅ No security issues identified

• Input properly escaped/quoted before use
• No command injection vectors (restic args passed safely)
• No template injection risks
• strings.ReplaceAll() is safe for substitution
• Cluster name and tenant validation happens upstream

---

🚀 Suggestions for Enhancement

1. Expand unit tests (Optional, but recommended)

func TestUnquoteResticTagLiteral(t *testing.T) {
    cases := []struct {
        name     string
        input    string
        expected string
        isLit    bool
    }{
        {"double_quotes", `"test"`, `test`, true},
        {"single_quotes", `'test'`, `test`, true},
        {"escape_comma", `"test\,"`, `test,`, true},
        {"escape_backslash", `"a\\b"`, `a\b`, true},
        {"single_no_escape", `'a\,b'`, `a\,b`, true},
        {"unmatched", `"unclosed`, `"unclosed`, false},
        {"empty_quoted", `""`, ``, true},
    }
    // test implementation...
}

func TestSplitResticKeepTagTemplates_EdgeCases(t *testing.T) {
    cases := []struct {
        input    string
        expected []string
    }{
        {``, nil},
        {`  `, nil},
        {`tag,`, []string{`tag`}},
        {`"unclosed`, []string{`"unclosed`}},
        {`'single' "double"`, []string{`'single'`, `"double"`}},
    }
    // test implementation...
}

2. Document quote behavior in config example (Optional)

# Keep tags use space/comma separation outside quotes
# Commas inside quotes are literal (AND semantics in restic)
# Single quotes preserve backslashes; double quotes process escapes
backup-restic-purge-keep-tag = "line:adhoc" 'role:primary,critical' env:prod

3. Validate unmatched quotes in config (Optional enhancement)
Consider a lint check that warns if quotes are unmatched when parsing config.

---

📊 Summary

Overall Assessment: 👍 Approved - Excellent PR

Strengths:

• ✅ Well-designed parsing logic handling all major cases
• ✅ Good test coverage (basic cases); edge cases verified manually
• ✅ Frontend/backend parity
• ✅ Clear error messages and logging
• ✅ Security-conscious (no injection risks)
• ✅ Backward compatible

Minor gaps:

• ⚠️ Test file could be more comprehensive (but all edge cases verified to work)
• ⚠️ Unmatched quote handling could be documented better for users

Recommendation:
Ready to merge with optional follow-ups:

• Optional: Expand test file with edge cases (ensures future regressions caught)
• Optional: Add example in config.toml showing single vs double quote behavior

[caffeinated92]

@claude review my pull request

[claude]

Claude finished @caffeinated92's task in 2m 39s —— View job

---

Code Review - PR #1338: Restic Keep-Tag Handling

✅ Comprehensive Strengths

1. Well-motivated feature

• Clear problem: Restic interprets commas inside keep-tags as AND semantics, conflicting with comma-separated UI input
• Pragmatic solution: Quoted literals + whitespace/comma separation reduces ambiguity
• Backward compatible: Legacy comma support preserved

2. Solid parser design

• splitResticKeepTagTemplates() correctly handles quote tracking, escape sequences, and separators
• Escape handling properly implemented: double quotes process \\ and \", single quotes preserve literally
• Unmatched quote detection prevents silent failures (line 481-482)
• Deduplication in parseResticKeepTagTemplates() prevents duplicate tags to restic (lines 382-391)

3. Good test coverage ✅

• TestSplitResticKeepTagTemplates: Tests core cases (empty input, quoted tags with commas, mixed quoted/unquoted)
• TestSplitResticKeepTagTemplates_UnmatchedQuotes: Validates unmatched quote handling
• TestValidateResticKeepTagTemplatesStrict: Validates the strict validation function
• All tests pass ✓

4. Frontend parity ✅

• JavaScript splitKeepTagTemplates() in ResticPurgeStrategy/index.jsx mirrors Go implementation
• Both handle identical parsing rules and escape sequences consistently
• Functions have matching logic flow for reliability

5. Error handling & logging ✅

• Unmatched quotes logged with warning (line 379): "Ignoring restic keep-tag with unmatched quotes"
• Failed template renders logged (line 201): "Failed to render restic keep-tag template"
• Graceful degradation: skips failed templates instead of aborting

6. Documentation improvements

• UI tooltip clearly explains behavior (lines 58-65 in index.jsx)
• Config example updated (etc/config.toml line 190)
• Help text shows concrete examples: "role:primary,critical"

7. Code quality

• Clean separation of concerns: parsing → validation → rendering
• Consistent naming conventions
• CSS duplication removed (moved modal styles to components/Modals/styles.module.scss)

---

⚠️ Minor Issues & Observations

1. Subtle difference between splitResticTagTemplates() and splitResticKeepTagTemplates()

• splitResticTagTemplates() (line 397-438): Only splits on commas, uses strings.HasSuffix(value, ",") to preserve trailing empty parts
• splitResticKeepTagTemplates() (line 441-487): Splits on commas AND whitespace, doesn't preserve trailing empty parts
• Both behave correctly for their use cases, but the difference could be documented better

2. Single vs. double quote escape semantics

• Single quotes preserve backslashes literally (no escape processing)
• Double quotes process \\ and \" escapes
• This is intentional and documented in comments (lines 519, 541), follows shell conventions
• Could add more prominent comment in config example showing the difference

3. Edge case: Trailing backslash in double quotes
In unquoteResticTagLiteral() (lines 537-540), a trailing backslash is preserved:

if escaped {
    b.WriteRune('\\')
}

This is correct behavior (prevents token corruption), but users might find "text\" becoming text\ unexpected. Current behavior is safe.

4. Keep-tag placeholder validation

• renderResticKeepTagTemplate() (line 637) only accepts {cluster} and {tenant} placeholders
• Regular tags support more placeholders: {engine}, {version}, {backup-type}, etc.
• This is correct (keep-tags don't need those), and validation catches unsupported keys
• No security risk, just worth noting the intentional restriction

---

🔒 Security Assessment

✅ No security vulnerabilities identified

• Input properly escaped/quoted before use
• No command injection vectors (restic arguments passed safely)
• No template injection risks (strings.ReplaceAll() is safe for substitution)
• Cluster names and tenant values validated upstream
• Quote handling prevents buffer overflows
• Escape sequences are properly bounded

---

📊 Test Results

✓ TestSplitResticKeepTagTemplates - PASS
  - Empty input handling ✓
  - Whitespace-only input ✓  
  - Quoted tags with commas ✓
  - Mixed quoted/unquoted ✓
  - Multiple whitespace normalization ✓

✓ TestSplitResticKeepTagTemplates_UnmatchedQuotes - PASS
  - Unmatched quote detection ✓
  - Empty result on unmatched quotes ✓

✓ TestValidateResticKeepTagTemplatesStrict - PASS
  - Rejects unmatched quotes ✓
  - Accepts valid input ✓

---

📋 Optional Improvements (Not Blockers)

1. Document quote handling in config:

# Single quotes preserve backslashes; double quotes process escapes
# backup-restic-purge-keep-tag = 'line:adhoc' "role:primary,critical"

2. Consider warning on unclosed quotes:
Currently, unclosed quotes in the config are silently treated as literal tokens. Could add a configuration validation log to help users catch typos:

"unclosed tag  → Treated as literal: "unclosed

3. Edge case coverage (already verified to work correctly):

• Unmatched quotes: Handled gracefully ✓
• Trailing commas: No empty parts generated ✓
• Escape sequences: Properly processed ✓
• Empty input: Returns empty slice ✓

---

Summary

This is a well-executed PR that solves a real problem with pragmatic design choices. The code is clean, tests pass, and both frontend and backend implementations are consistent. Documentation is clear with good examples.

Recommendation: 👍 Ready to Merge

The PR successfully:

• ✅ Fixes the keep-tag parsing ambiguity
• ✅ Maintains backward compatibility
• ✅ Includes comprehensive tests
• ✅ Implements proper error handling
• ✅ Provides clear documentation
• ✅ Has no security issues
• ✅ Maintains code quality standards

Branch: feature/restic-04-backup-purge → develop