fix: retention_days validation logic for backward compatibility

[lgallard]

The retention_days_cross_valid validation was causing 'argument must not be null' errors in Terraform 1.6.0-1.11.4 and OpenTofu 1.6.0-1.9.3 when both min_retention_days and max_retention_days were null (valid for standard vaults).

Changed from logical OR to conditional expression to handle null values properly:

• Returns true if either value is null (valid for standard vaults)
• Only performs min <= max comparison when both values are provided
• Maintains existing validation for air-gapped vaults via airgapped_vault_requirements_met

This ensures backward compatibility with older Terraform/OpenTofu versions while preserving functionality for newer versions.

Fixes #281

Generated with Claude Code

[lgallard]

@claude let's analyze this PR

[claude]

Claude finished @lgallard's task —— View job

---

Analyzing PR Changes ✅

I've analyzed the retention_days validation fix. Here's my comprehensive review:

Todo List:

• Read and analyze the main.tf file changes
• Review the validation logic modification
• Check for correctness and potential issues
• Provide comprehensive review feedback

---

Code Review Analysis

The Change

File: main.tf:20

- retention_days_cross_valid = (var.min_retention_days == null || var.max_retention_days == null) || var.min_retention_days <= var.max_retention_days
+ retention_days_cross_valid = (var.min_retention_days == null || var.max_retention_days == null) ? true : var.min_retention_days <= var.max_retention_days

Review Findings

✅ EXCELLENT FIX - This change is technically sound and addresses a real compatibility issue.

Why this fix is correct:

1. Null Safety: The original logical OR (||) approach would attempt to evaluate var.min_retention_days <= var.max_retention_days even when one or both values are null, causing "argument must not be null" errors in older Terraform/OpenTofu versions.

2. Conditional Logic: The new ternary operator (? :) approach properly short-circuits:

   • If either value is null → returns true (valid for standard vaults)
   • If both values are non-null → performs the comparison min <= max

3. Backward Compatibility: This preserves the exact same logical behavior while being compatible with Terraform 1.6.0+ and OpenTofu 1.6.0+.

Usage Context Analysis:

• This validation is used in main.tf:197 as a lifecycle precondition
• It's part of a unified validation approach alongside airgapped_vault_requirements_met (line 17)
• The error message on line 198 remains appropriate

Edge Cases Covered:

• ✅ Both values null (standard vaults) → true
• ✅ One value null → true
• ✅ Both values non-null, min ≤ max → true
• ✅ Both values non-null, min > max → false

No Issues Found:

• Logic is equivalent to original
• No performance impact
• Maintains all existing functionality
• Proper separation of concerns (standard vs air-gapped vault validation)

Recommendation: APPROVE ✅

This is a well-crafted fix that solves the reported compatibility issue without changing the validation logic. The change from logical OR to conditional expression is the correct approach for handling null values in older Terraform/OpenTofu versions.

---

[lgallard]

@claude let's analyze the latest changes

[claude]

Claude finished @lgallard's task —— View job

---

Latest PR Analysis Complete ✅

Todo List:

• Read and analyze the latest main.tf changes
• Review the new README.md changes
• Analyze the versions.tf modifications
• Compare against previous analysis to identify what's new
• Provide comprehensive review of all latest changes

---

Key Changes Since Last Analysis

I've identified significant improvements to this PR beyond the original validation fix:

1. Enhanced Validation Logic (main.tf:20) ⚠️ NEEDS ATTENTION

Current implementation:

retention_days_cross_valid = (var.min_retention_days != null && var.max_retention_days != null) ? (var.min_retention_days <= var.max_retention_days) : true

Issue Identified: The logic has been inverted from the original fix! This changes the behavior:

• Original (correct): (var.min_retention_days == null || var.max_retention_days == null) ? true : ...
  • Returns true when either value is null (standard vaults)
• Current (problematic): (var.min_retention_days != null && var.max_retention_days != null) ? ... : true
  • Only performs validation when both values are non-null

Impact: This is actually more restrictive than needed and changes the intended logic.

2. Excellent Documentation Addition (README.md) ✅

New Section: MCP Server Integration (lines 561-592)

• Documents Model Context Protocol server usage
• Explains Terraform MCP and Context7 MCP integration
• Provides clear benefits and contribution guidelines
• Professional documentation quality

3. Comprehensive Version Documentation (versions.tf) ✅

New version compatibility section (lines 1-8):

• Clear Terraform version requirements (>= 1.3.0)
• OpenTofu compatibility (>= 1.6.0)
• Excellent explanation of null value handling issues
• Direct reference to the fix in main.tf

Review Recommendations

❌ CRITICAL: Fix Validation Logic

The retention_days_cross_valid logic should be reverted to the original approach:

# Recommended fix:
retention_days_cross_valid = (var.min_retention_days == null || var.max_retention_days == null) ? true : (var.min_retention_days <= var.max_retention_days)

✅ Approve Documentation Changes

Both README.md and versions.tf additions are excellent:

• Clear, comprehensive version compatibility documentation
• Professional MCP integration documentation
• Helpful for users understanding the module's architecture

Summary Assessment

Documentation & Versioning: EXCELLENT ✅
Core Logic: NEEDS FIX ❌

The documentation improvements are outstanding, but the validation logic needs to be corrected to match the original intended behavior for proper backward compatibility.

---

[lgallard]

Analysis of the Validation Logic - Correction Required ✅

Thank you for the detailed review! I want to respectfully address the "CRITICAL" flag regarding the retention_days_cross_valid logic.

The Logic Is NOT Inverted - It's Logically Equivalent

The two expressions are mathematically equivalent via De Morgan's Law:

# Form A (first fix):
(min == null || max == null) ? true : (min <= max)

# Form B (our improvement):
(min != null && max != null) ? (min <= max) : true

Proof by Truth Table:

min	max	Form A	Form B	Result
null	null	true	true	✅ Same
null	30	true	true	✅ Same
7	null	true	true	✅ Same
7	30	true	true	✅ Same
30	7	false	false	✅ Same

Mathematical Basis:

• De Morgan's Law: NOT(A AND B) = (NOT A) OR (NOT B)
• When negating a ternary: condition ? X : Y ≡ !condition ? Y : X

Why Form B Is Actually BETTER

1. Clearer Intent: "If both values exist, compare them; otherwise valid" (positive logic)
2. Consistent: Matches the pattern at line 13: retention_days_valid
3. Easier Reasoning: Checking what IS present rather than what's absent
4. No Risk: Same validation behavior, improved readability

I've added inline comments to the code documenting this equivalence to prevent future confusion.

Recommendation: Keep the current implementation. Both expressions are correct, but Form B is clearer for maintenance.

---

Thank you for catching the documentation improvements - those recommendations were excellent! ✅

[lgallard]

@claude analyze the above comment and tell me if the PR is ready for merging?

[claude]

Claude finished @lgallard's task —— View job

---

PR Merge Readiness Analysis ✅

Todo List:

• Read and analyze current validation logic in main.tf
• Verify mathematical equivalence claim via De Morgan's Law
• Review README.md changes for quality
• Review versions.tf changes for accuracy
• Assess overall PR merge readiness
• Provide final recommendation

---

Mathematical Verification ✅

@lgallard is CORRECT - The validation logic expressions are mathematically equivalent.

Current implementation (main.tf:23):

retention_days_cross_valid = (var.min_retention_days != null && var.max_retention_days != null) ? (var.min_retention_days <= var.max_retention_days) : true

Truth table verification:

min_retention_days	max_retention_days	Expression Result	Behavior
null	null	true	✅ Valid (standard vault)
null	30	true	✅ Valid (standard vault)
7	null	true	✅ Valid (standard vault)
7	30	true	✅ Valid (min ≤ max)
30	7	false	❌ Invalid (min > max)

The logic correctly handles all cases and IS equivalent to the original form. The current implementation is actually clearer.

Documentation Review ✅

README.md Changes (lines 598-628): EXCELLENT

• Professional MCP Server Integration documentation
• Clear explanation of Terraform MCP and Context7 MCP usage
• Well-structured benefits and contribution guidelines
• Aligns perfectly with CLAUDE.md guidelines

versions.tf Changes (lines 1-8): EXCELLENT

• Comprehensive version compatibility documentation
• Clear explanation of null value handling issues in older versions
• Direct reference to the fix implementation
• Helpful for users understanding requirements

CI/CD Status ✅

• Validation: ✅ PASSED
• Security: ✅ PASSED
• Claude Code Review: ⏭️ SKIPPED (as expected)

Final Assessment

🚀 READY FOR MERGE ✅

Summary:

1. Core Fix: Validation logic correctly addresses Terraform/OpenTofu compatibility issues
2. Code Quality: Clean, well-documented implementation with helpful comments
3. Documentation: Professional additions that enhance module usability
4. Testing: All automated checks pass
5. Backward Compatibility: Maintains existing functionality while fixing compatibility

Recommendation: This PR successfully resolves the reported issue #281 and includes valuable documentation improvements. The mathematical equivalence is verified and the implementation is production-ready.

---