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: 38aac3a4-bb64-4295-b54f-70d714d04c42

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 — PR #3412

Verdict: Approve

Summary: Clean, well-scoped enhancement that adds enforcement date information to warning messages when a policy failure is demoted due to a future effective_on date. The change correctly updates both filter implementations (LegacyPostEvaluationFilter and UnifiedPostEvaluationFilter), adds a shared helper (formatFutureEnforcementMessage), and includes thorough test coverage across all code paths.

i This PR is a draft. Findings below are provided as early feedback.

Reviewed dimensions

Dimension	Result
Correctness	✅ No issues — logic preserves original semantics, value/reference safety verified, both code paths updated consistently, test integrity confirmed
Security	✅ No issues — %s format string is safe, effective_on values flow through JSON marshaling which handles escaping
Intent & scope	✅ Well-scoped to the stated intent (EC-1901), appropriate authorization via Jira reference
Cross-repo contracts	✅ No contract break — msg is a free-form string with no schema or downstream parsing
Documentation	✅ No staleness — no existing docs describe the warning message format
Style & conventions	🔵 Minor suggestions below

Analysis highlights

Correctness verification:

• Both CategorizeResults implementations are updated identically — the refactoring from || to separate if/else if branches preserves the original semantics while enabling message enhancement only for the !isResultEffective path
• formatFutureEnforcementMessage correctly takes Result by value and only reads (never writes) the shared Metadata map
• determineOriginalType in the Unified filter runs before message modification, so type classification uses the original unmodified message — correct ordering
• Nil Metadata map is safe in Go (map read returns zero value without panic)
• The acceptance test snapshot correctly updates only 1 of 3 occurrences of "Fails in 2099" — the other two are in violations sections (not warnings), where the rule is effective and no demotion occurs

Test coverage:

• Unit tests for formatFutureEnforcementMessage covering: valid effective_on, missing metadata, non-string value, empty string, nil metadata, trailing period handling
• Integration tests for both UnifiedPostEvaluationFilter and LegacyPostEvaluationFilter covering: future date (demoted + enhanced), past date (stays failure), relative time shifting
• Existing TestConftestEvaluatorEvaluateSeverity legitimately updated to match new behavior
• Acceptance test snapshot updated for end-to-end validation

Style suggestions

1. Doc comment convention (filters.go): The comment on formatFutureEnforcementMessage uses a descriptive style ("enhances a result message...") rather than the codebase's godoc convention where the comment starts with the function name and a present-tense verb (e.g., ensureNonNilSlice returns..., extractStringArrayFromRuleData returns...). Suggest: // formatFutureEnforcementMessage returns the result message with an appended enforcement date notice if the effective_on metadata is present. If effective_on is absent or empty, the original message is returned unchanged.

2. Test naming (filters_test.go): TestUnifiedCategorizeResultsFutureEffectiveOn and TestLegacyCategorizeResultsFutureEffectiveOn abbreviate the struct name and omit underscore separators, diverging from the established TestStructName_Qualifier pattern (e.g., TestECPolicyResolver_PipelineIntention_RuleLevel). Consider TestUnifiedPostEvaluationFilter_FutureEffectiveOn and TestLegacyPostEvaluationFilter_FutureEffectiveOn.

3. Code organization (filters.go): The file uses section banners (///// Section Name /////). The new helper is placed at the very end after determineOriginalType without a section banner. Consider moving it into the existing // Helpers // section (near extractStringArrayFromRuleData) or placing it within the post-evaluation filter section near the CategorizeResults methods that call it.

Previous run

Review

Findings

Medium

• [inconsistent code path] internal/evaluator/filters.go:999 — The LegacyPostEvaluationFilter.CategorizeResults method is updated to add message enhancement for future effective_on results, but this code path is never exercised in production. All production callers (conftest_evaluator.go:660, base_evaluator.go:313) use UnifiedPostEvaluationFilter.CategorizeResults. The only caller of LegacyPostEvaluationFilter is in filters_test.go:869, and that test only exercises FilterResults, not CategorizeResults. While the modification is logically correct, there is no test coverage verifying the Legacy path's message enhancement actually works. Consider either adding test coverage for this path or removing the modification to reduce dead code.

Low

• [error-handling-consistency] internal/evaluator/filters.go:1220 — formatFutureEnforcementMessage does not log warnings when the type assertion on metadataEffectiveOn fails. The established codebase helpers getSeverity and isResultEffective in conftest_evaluator.go both call log.Warnf when type assertions fail. However, since formatFutureEnforcementMessage is only called inside the !isResultEffective branch (where effective_on was already parsed successfully), a type assertion failure here would require metadata mutation between calls, making this low risk.

• [semantic-consistency] internal/evaluator/filters.go:999 — The compound condition getSeverity == severityWarning || !isResultEffective is split into separate branches with different behavior: message enhancement only occurs for effective_on-based demotions, not for severity-based demotions. This is semantically correct (severity-demoted warnings have no enforcement date to report), but a brief inline comment would help future readers understand the intentional asymmetry.

• [code-robustness] internal/evaluator/filters.go:1220 — result.Message / filteredResult.Message is mutated in-place during categorization via assignment from formatFutureEnforcementMessage. Since CategorizeResults is only called once per evaluation run in practice, this is not an active bug, but documenting the mutation in the method's docstring would prevent surprises if the calling pattern changes.

Previous run (2)

Review — approve

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

Summary

This PR enhances warning messages for policy violations with future effective_on dates by appending the enforcement start date. The implementation correctly splits the existing compound || condition in both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults to selectively add message enhancement only when a failure is demoted to a warning due to a future effective_on date (not when it's demoted due to severityWarning).

The logic refactor from if A || B to if A { ... } else if B { ... } is semantically equivalent for all cases except the newly added message modification. The formatFutureEnforcementMessage helper is well-guarded against nil metadata maps, non-string values, and empty strings.

Correctness

Logic change is safe. The result.Message mutation operates on Go range-loop copies (value semantics for Result structs), so the original filteredResults slice is never modified. The Metadata map (reference type) is only read, never written, so no aliasing issues arise. The determineOriginalType call in UnifiedPostEvaluationFilter happens before the message mutation, preserving correct message-based matching.

Guard chain is sound. formatFutureEnforcementMessage can only be reached when isResultEffective returns false, which requires a valid, parseable, future effective_on timestamp. The function's own defensive checks (nil map, non-string, empty) are redundant but harmless defense-in-depth.

Snapshot correctness verified. Only 1 of 3 "Fails in 2099" occurrences in the snapshot file is updated, which is correct — the other two correspond to scenarios using --effective-time 2100-... where the 2099 effective_on is already in the past relative to the evaluation time.

Test integrity confirmed. The change to conftest_evaluator_unit_core_test.go is a legitimate test update (expected message updated to match new behavior), not assertion weakening. The assertion structure is unchanged.

Security

No authentication, authorization, secrets handling, or permission manifest changes. The effective_on value interpolated into messages is constrained to valid RFC3339 timestamps by the isResultEffective guard (which calls time.Parse before formatFutureEnforcementMessage is reachable). No injection risk identified.

Findings

#	Severity	Category	File	Description
1	low	test-coverage	internal/evaluator/filters_test.go	No integration-level test verifies message enhancement flows through CategorizeResults. The helper function is well-tested in isolation, and the evaluator-level test (conftest_evaluator_unit_core_test.go) covers the UnifiedPostEvaluationFilter path end-to-end. However, the LegacyPostEvaluationFilter.CategorizeResults path has no test coverage for message enhancement.
2	low	defense-in-depth	internal/evaluator/filters.go	formatFutureEnforcementMessage does not validate the effective_on format itself — it relies on the caller's isResultEffective guard (which calls time.Parse). Currently safe, but if the function is ever called from a context without that guard, unvalidated policy-authored strings would flow into output messages. Consider adding time.Parse validation inside the function as defense-in-depth.

Both findings are low severity and do not block merge. The implementation is clean, well-tested, and architecturally appropriate.

---

Reviewed with six-dimensional analysis: correctness · security · intent & coherence · style & conventions · documentation currency · cross-repo contracts

---

Labels: PR modifies internal evaluator logic for policy result categorization and message formatting

Previous run (3)

Review — comment

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

Summary

This PR enhances warning messages when a policy deny result is demoted to a warning due to a future effective_on date, appending the enforcement date to the message (e.g., ". This will become a failure starting on 2025-01-01T00:00:00Z"). The core implementation in filters.go is correct and well-structured, with thorough unit tests covering edge cases. One correctness issue with acceptance test snapshots needs to be addressed before merging.

Findings

⚠ Incorrect snapshot expectations for violation scenarios

Severity: medium | features/__snapshots__/validate_image.snap

Two of the three snapshot changes incorrectly add the enforcement message suffix to violations (real failures), not just to demoted warnings:

1. "using attestation time as effective time" (line ~1410): Uses attestation time 2100-01-01 as effective time. Since effective_on is 2099-01-01 and 2099 < 2100, isResultEffective() returns true — the result is effective and remains a real failure. The code does not modify the message in the else (failures) branch.

2. "future failure is a deny when using effective-date flag" (line ~2014): Uses --effective-time 2100-01-01T12:00:00Z. Same logic — 2099 < 2100 → real failure → message unchanged.

Only the first snapshot change ("future failure is converted to a warning", line ~360) is correct — there the effective time is the current time (~2025), 2099 > 2025, so the failure is demoted and the message is enhanced.

The two incorrect snapshot changes will cause acceptance test failures. The snapshots appear to have been manually edited rather than regenerated with UPDATE_SNAPS=true make acceptance. The fix is to revert the snapshot changes at lines ~1410 and ~2014, keeping only the change at line ~360.

Code trace confirming the logic

The new code in UnifiedPostEvaluationFilter.CategorizeResults:

case "failure":
    if getSeverity(filteredResult) == severityWarning {
        warnings = append(warnings, filteredResult)
    } else if !isResultEffective(filteredResult, effectiveTime) {
        filteredResult.Message = formatFutureEnforcementMessage(filteredResult)
        warnings = append(warnings, filteredResult)  // message enhanced here
    } else {
        failures = append(failures, filteredResult)   // message NOT touched
    }

isResultEffective (in conftest_evaluator.go:1109):

func isResultEffective(failure Result, now time.Time) bool {
    // returns true when effective_on is before now (i.e., the rule IS effective)
    return effectiveOn.Before(now)
}

For the two failing scenarios: effective_on=2099 is before effectiveTime=2100 → true → real failure → else branch → no message modification.

Positive observations

• Correct logic refactoring: Splitting the compound if into separate branches for severityWarning vs !isResultEffective preserves existing behavior while cleanly adding message enhancement only for the effective_on demotion path.
• Both filter paths updated: The change is applied consistently to both LegacyPostEvaluationFilter.CategorizeResults and UnifiedPostEvaluationFilter.CategorizeResults.
• Thorough unit tests: TestFormatFutureEnforcementMessage covers valid date, missing metadata, non-string type, empty string, nil metadata, and trailing period — good defensive coverage.
• Existing test updated correctly: TestConftestEvaluatorEvaluateSeverity properly updates the expected message for the "not yet effective" case (which has effective_on: "3021-01-01T00:00:00Z", a future date).
• formatFutureEnforcementMessage is safe: The function is only called from the !isResultEffective branch, guaranteeing the effective_on value has already been validated as a parseable RFC3339 date string.

Previous run (4)

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 (5)

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 (6)

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 (7)

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]

🤖 Finished Review · ✅ Success · Started 12:24 PM UTC · Completed 12:32 PM UTC
Commit: 87c4a29 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 8:26 AM UTC · Ended 8:40 AM UTC
Commit: 87c4a29 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/evaluator/filters.go	93.33%	1 Missing ⚠️

Flag	Coverage Δ
acceptance	54.26% <66.66%> (+0.01%)	⬆️
generative	16.78% <0.00%> (-0.02%)	⬇️
integration	27.95% <13.33%> (-0.03%)	⬇️
unit	71.91% <93.33%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/evaluator/filters.go	87.25% <93.33%> (+4.62%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:26 AM UTC · Completed 8:40 AM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:37 AM UTC · Completed 9:51 AM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 11:15 AM UTC · Ended 11:29 AM UTC
Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

[low] style

Doc comment on formatFutureEnforcementMessage uses descriptive style ("enhances a result message...") rather than the codebase godoc convention where the comment starts with the function name and a present-tense verb (e.g., "ensureNonNilSlice returns...", "extractStringArrayFromRuleData returns...").

Suggested fix: Rewrite as: // formatFutureEnforcementMessage returns the result message with an appended enforcement date notice if the effective_on metadata is present. If effective_on is absent or empty, the original message is returned unchanged.

[fullsend-ai-review]

[low] style

The file uses section banners (// Section Name //). The new helper is placed at the very end after determineOriginalType without a section banner, breaking the organizational structure.

Suggested fix: Move formatFutureEnforcementMessage into the existing Helpers section (near extractStringArrayFromRuleData around line 393) or place it within the post-evaluation filter section near the CategorizeResults methods that call it.