Include enforcement date in warnings for future effective_on violations

[Acepresso]

When the CLI demotes a deny result to a warning due to a future effective_on date, the warning message is now enhanced to indicate when enforcement begins.

Before: "The required 'cpe' label is missing"
After: "The required 'cpe' label is missing. This will become a failure starting on 2025-01-01T00:00:00Z"

This applies to both sources of effective_on: rule annotations and rule_data items. The effective_on date is included as-is (RFC3339 format), matching the pattern used by trusted_task's future_deny_rule warnings.
No duplicate warnings for trusted_task rules. The enhancement only applies to failures being demoted, not to existing warning rules like future_deny_rule.

Test plan

• Unit tests for formatFutureEnforcementMessage covering: valid effective_on, missing metadata, non-string value, empty string, nil metadata
• Existing TestConftestEvaluatorEvaluateSeverity updated and passing
• Full evaluator unit test suite passes with no regressions
• Run ec validate against a policy with a future effective_on date and verify the warning message includes the enforcement date

Ref: https://redhat.atlassian.net/browse/EC-1901
Assisted-by: Claude Opus 4.6

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 447e6572-1916-4615-86d1-1cc3b1135d61

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 3:21 PM UTC · Ended 3:25 PM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:26 PM UTC · Completed 3:31 PM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

Review — ⚠ Changes Requested

PR: #3412 — Include enforcement date in warnings for future effective_on violations
Status: Draft

Overview

The production code changes are well-structured. Splitting the if A || B condition into separate branches is a clean refactoring that correctly preserves the original behavior while enabling message enhancement for the !isResultEffective path. The new formatFutureEnforcementMessage function is clean, handles edge cases well (nil metadata, non-string, empty string, trailing period), and the unit tests provide good coverage.

However, two of the three acceptance test snapshot changes are incorrect and will cause test failures.

---

Findings

🔴 Incorrect snapshot expectations for violation test cases

Severity: high | File: features/__snapshots__/validate_image.snap

The snapshot updates for "using attestation time as effective time" (line ~1410) and "future failure is a deny when using effective-date flag" (line ~2014) are incorrect. Both snapshots are under "violations" (i.e., failures), but the PR changes their "msg" field to include the enhancement suffix ". This will become a failure starting on ${TIMESTAMP}".

Why this is wrong: In both test scenarios, the effective time is set to 2100 (via --effective-time 2100-01-01T12:00:00Z or --effective-time attestation with buildFinishedOn: 2100-01-01). The rule's effective_on is 2099. Since 2099 < 2100, isResultEffective() returns true — the result is effective and stays as a failure/violation. The code only calls formatFutureEnforcementMessage() in the else if !isResultEffective(...) branch, which is not taken when the result is effective. The message must remain "Fails in 2099".

Only the first snapshot change — "future failure is converted to a warning" (line ~360, under "warnings") — is correct, because in that scenario no explicit effective-time flag is set, the effective time defaults to "now" (~2026), and 2099 > 2026 means the result is not effective → demoted to warning → message enhanced.

Code trace:

// In CategorizeResults (both Legacy and Unified):
case "failure":
    if getSeverity(result) == severityWarning {
        // not taken (no severity metadata)
    } else if !isResultEffective(result, effectiveTime) {
        // NOT taken when effectiveTime=2100, effective_on=2099
        // because isResultEffective returns true (2099 < 2100)
        result.Message = formatFutureEnforcementMessage(result)
        warnings = append(warnings, result)
    } else {
        failures = append(failures, result) // ← THIS branch is taken
    }

Remediation: Revert the snapshot changes for the two violation test cases. Only the "future failure is converted to a warning" snapshot (under "warnings") should have the enhanced message. Regenerate snapshots with UPDATE_SNAPS=true make acceptance to verify.

---

🟡 Missing test for severity=warning + future effective_on interaction

Severity: low | File: internal/evaluator/filters_test.go

The refactoring splits if getSeverity == severityWarning || !isResultEffective(...) into separate if/else if branches. When both conditions are true (a failure has severity: "warning" AND a future effective_on), the original code sent it to warnings without message enhancement; the new code does the same because the severity check comes first. However, there's no test explicitly covering this interaction. A test case with both conditions true would guard against future regressions if the branch order is accidentally swapped.

---

What looks good

• Production code logic — the refactoring correctly preserves behavior and the new function is clean
• Unit test coverage — TestFormatFutureEnforcementMessage covers 6 edge cases including nil metadata, non-string values, empty strings, and trailing periods
• Test integrity — the conftest_evaluator_unit_core_test.go change is a legitimate expectation update (tighter assertion), not test weakening
• Security — no injection risk; effectiveOnStr flows only to CLI output text and is pre-validated as a time string in upstream code paths
• No docs staleness — the message format is not documented; no docs updates needed

Previous run

Review of #3412 — Include enforcement date in warnings for future effective_on violations

Verdict: comment · 1 medium finding

---

Summary

This PR enhances the warning messages for policy violations that have future effective_on dates. When a deny result is demoted from failure to warning because its enforcement date hasn't arrived yet, the message now indicates when enforcement begins (e.g., "This will become a failure starting on 2025-01-01T00:00:00Z").

The implementation is clean and well-structured:

• The combined || condition in both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults is correctly split into separate branches, adding message enhancement only for the future-effective demotion path (not for severity-based demotion)
• formatFutureEnforcementMessage handles edge cases defensively — nil metadata, non-string effective_on, empty strings, and messages already ending with a period
• Both filter implementations are updated consistently
• The function is unreachable with nil Metadata in production because isResultEffective returns true when effective_on is absent, preventing the else-if branch from executing — yet the function still handles nil defensively, which is good practice
• Unit tests cover the function thoroughly with six test cases
• The existing TestConftestEvaluatorEvaluateSeverity test is correctly updated

No security concerns — the effective_on value originates from policy metadata (rule annotations or Rego results), not from end-user input, and is included as-is in the warning message, consistent with how the codebase already handles this field.

---

Findings

1. Acceptance test snapshot not updated — future failure is converted to a warning

Severity	medium
Category	test-adequacy
File	features/__snapshots__/validate_image.snap
Line	360

The acceptance test scenario "future failure is converted to a warning" (features/validate_image.feature:310) uses the future_deny.rego example policy, which emits a denial with msg: "Fails in 2099" and effective_on: "2099-01-01T00:00:00Z". Since no --effective-time flag is passed, the current time is used, and the failure is demoted to a warning.

After this PR, the warning message will be enhanced to:

Fails in 2099. This will become a failure starting on 2099-01-01T00:00:00Z

The snapshot framework's timestampRegex replaces RFC 3339 timestamps, so the stored snapshot would become:

"msg": "Fails in 2099. This will become a failure starting on ${TIMESTAMP}"

But the current snapshot still expects:

"msg": "Fails in 2099"

This will cause the acceptance test to fail. The two other scenarios using this policy ("future failure is a deny when using effective-date flag" and "using attestation time as effective time") are unaffected because they set the effective time to 2100, making the rule effective — so the failure is not demoted and the message is not enhanced.

Remediation: Update the snapshot by running UPDATE_SNAPS=true make acceptance (or the relevant subset), or manually update line 360 of features/__snapshots__/validate_image.snap to reflect the new message format.

---

Labels: PR modifies evaluator warning message formatting

Previous run (2)

Review of PR #3412 — Include enforcement date in warnings for future effective_on violations

Verdict: Request changes

Summary

This PR enhances warning messages when the CLI demotes a deny result to a warning due to a future effective_on date. The new formatFutureEnforcementMessage helper appends the enforcement date to the message text (e.g., ". This will become a failure starting on 2025-01-01T00:00:00Z"). Both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults are updated with the same logic. Unit tests for the new function are well-structured and cover edge cases (nil metadata, non-string values, empty strings, trailing periods).

Findings

🔴 High: Acceptance test snapshot not updated

File: features/__snapshots__/validate_image.snap, line 360

The acceptance test scenario "future failure is converted to a warning" (features/validate_image.feature:310) uses the future_deny.rego example policy, which produces a deny result with effective_on: "2099-01-01T00:00:00Z". This result gets demoted from failure to warning by CategorizeResults.

With this PR's changes, the demoted warning message will change from:

"Fails in 2099"

to:

"Fails in 2099. This will become a failure starting on 2099-01-01T00:00:00Z"

After the snapshot framework's timestamp normalization (via timestampRegex in acceptance/snaps/snaps.go), the actual output would be "Fails in 2099. This will become a failure starting on ${TIMESTAMP}", but the snapshot still expects "msg": "Fails in 2099". This will cause the acceptance test to fail.

Remediation: Update the snapshot entry at features/__snapshots__/validate_image.snap line 360 to:

"msg": "Fails in 2099. This will become a failure starting on ${TIMESTAMP}",

Alternatively, run the acceptance tests with snapshot update mode enabled to regenerate the snapshot automatically.

🟡 Low: Shallow copy shares underlying Metadata map

File: internal/evaluator/filters.go, lines 1001–1002 and 1129–1130

The pattern enhancedResult := result creates a struct-level shallow copy. Since Result.Metadata is a map[string]interface{}, both the original and the copy share the same map reference. This is currently safe because only the Message field (a string) is modified on the copy. However, if future changes also modify metadata on the enhanced result, the mutation would silently affect the original. Consider documenting this constraint in a comment, or performing a deep copy of the map if metadata modifications are ever needed.

What looks good

• The formatFutureEnforcementMessage function is clean and handles edge cases well (nil metadata, non-string effective_on, empty string).
• The strings.TrimSuffix(originalMessage, ".") approach avoids double periods when messages already end with ..
• Splitting the if condition (getSeverity == severityWarning || !isResultEffective) into two branches is a clear and readable refactor.
• The unit test in TestConftestEvaluatorEvaluateSeverity was correctly updated to expect the enhanced message.
• The enhancement only applies to failures being demoted (not to existing warning rules), avoiding duplicate annotations.
• Test coverage for formatFutureEnforcementMessage is thorough with 6 test cases.

---

This review was performed against commit b822c50.

Previous run (3)

Review — Approve

PR: #3412 — Include enforcement date in warnings for future effective_on violations
Author: Acepresso · Status: Draft · Files changed: 3 · Diff: +111 / −3

Summary

When the CLI demotes a deny/failure result to a warning because its effective_on date is in the future, the warning message is now enhanced to indicate when enforcement begins (e.g., "The required 'cpe' label is missing. This will become a failure starting on 2025-01-01T00:00:00Z").

The change introduces a helper function formatFutureEnforcementMessage and applies it in both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults. The helper is well-guarded: it returns the original message unchanged when metadata is nil, the effective_on key is missing, or the value is non-string or empty. Implicitly, the isResultEffective gate in the calling code ensures only validly-parseable future dates reach the enhancement path.

Findings

All findings are low severity — none are blocking.

---

1. strings.TrimRight vs strings.TrimSuffix — subtle behavior difference

Severity: low · File: internal/evaluator/filters.go · Line: ~1233

strings.TrimRight(originalMessage, ".") removes all trailing characters in the cutset, not just a single trailing period. If a message ever ends with "..." (e.g., an ellipsis), all three dots are stripped. strings.TrimSuffix(originalMessage, ".") would remove exactly one trailing period, which is the likely intent.

In practice, policy violation messages rarely end with ellipses, so the risk is cosmetic only.

Suggested fix:

return fmt.Sprintf("%s. This will become a failure starting on %s",
    strings.TrimSuffix(originalMessage, "."), effectiveOnStr)

---

2. Duplicated logic across Legacy and Unified filter implementations

Severity: low · File: internal/evaluator/filters.go · Lines: ~1000–1008, ~1125–1133

The same 7-line block (inner if !isResultEffective → create enhancedResult → append) is copy-pasted into both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults. If this logic changes in the future (e.g., message format update, additional guards), both sites must be updated in lockstep.

Consider extracting a small helper like:

func appendWithEnhancement(result Result, effectiveTime time.Time, warnings *[]Result) {
    if !isResultEffective(result, effectiveTime) {
        result.Message = formatFutureEnforcementMessage(result)
    }
    *warnings = append(*warnings, result)
}

This would eliminate the duplication and make future changes single-site.

---

3. Edge case: severity: warning + future effective_on produces misleading message

Severity: low · File: internal/evaluator/filters.go · Lines: ~1000, ~1125

When a deny result carries both severity: warning (demotes it to a warning by annotation) and a future effective_on date, the outer condition getSeverity(result) == severityWarning || !isResultEffective(result, effectiveTime) is satisfied by both disjuncts. The inner if !isResultEffective(...) then fires, appending "This will become a failure starting on ." However, after that date passes, the severity: warning annotation still keeps the result as a warning — it never actually becomes a failure. The message is technically misleading.

This requires contradictory rule configuration (severity: warning plus a future enforcement date is unusual), so real-world impact is negligible. No fix needed now, but worth a comment in the code if the team agrees.

---

Verification

• ✅ The formatFutureEnforcementMessage helper is thoroughly unit-tested (6 cases: valid date, missing metadata, non-string value, empty string, nil metadata, trailing period)
• ✅ The existing TestConftestEvaluatorEvaluateSeverity integration test correctly updated expected output
• ✅ isResultEffective returns true for unparseable dates, preventing the enhancement branch from executing with invalid date strings — good implicit validation
• ✅ No cross-repo contract changes — Result.Message is user-facing text, not an API boundary
• ✅ No security concerns
• ⚠ No test coverage for the UnifiedPostEvaluationFilter.CategorizeResults path (the duplicated logic), though the implementation is identical

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:46 PM UTC · Completed 1:56 PM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:30 PM UTC · Completed 2:39 PM UTC
Commit: 87c4a29 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:55 PM UTC · Completed 3:14 PM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.