Surface volatile-excluded rules as time-bounded exceptions in report output#3419
Surface volatile-excluded rules as time-bounded exceptions in report output#3419arewm wants to merge 2 commits into
Conversation
Exception results were computed but not exposed to the report due to missing exceptions field in Input struct. Preserve effective_until metadata so downstream tooling can consume time-bounded expiry. Assisted-by: Claude Code (Haiku 4.5)
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughConfiguration exclusions now surface as exception results with optional effective-time metadata. Exceptions are aggregated by evaluators and outputs, serialized in reports, and populated by validation and server report-building paths. ChangesConfiguration exclusion exception flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Validator
participant Evaluator
participant UnifiedPostEvaluationFilter
participant Output
participant Report
Validator->>Evaluator: validate input
Evaluator->>UnifiedPostEvaluationFilter: filter policy results
UnifiedPostEvaluationFilter-->>Evaluator: filtered results and config exceptions
Evaluator-->>Output: store outcome exceptions
Output-->>Report: aggregate exceptions
Report-->>Validator: serialize exceptions
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 Review · ❌ Terminated · Started 4:44 PM UTC · Ended 5:01 PM UTC |
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again |
PR Summary by QodoReport volatile-excluded rules as time-bounded exceptions
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/evaluator/criteria.go`:
- Around line 41-46: Change Criteria.meta from value-based keys to full scoped
criterion identities, preserving separate metadata for static versus volatile
entries and for each component. Update the criterion construction and
policy-resolution paths that read or write meta so this identity is carried
through consistently. Add collision tests covering static plus volatile criteria
and the same value across multiple components, ensuring reporting retains each
entry’s correct expiry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f2fb179e-b19f-4d96-80e2-0ae628a8ebf1
📒 Files selected for processing (15)
cmd/validate/input.gointernal/evaluator/DESIGN.mdinternal/evaluator/base_evaluator.gointernal/evaluator/conftest_evaluator.gointernal/evaluator/conftest_evaluator_unit_filtering_test.gointernal/evaluator/criteria.gointernal/evaluator/criteria_test.gointernal/evaluator/filters.gointernal/evaluator/filters_test.gointernal/evaluator/opa_evaluator_integration_test.gointernal/input/report.gointernal/input/report_test.gointernal/output/output.gointernal/output/output_test.gointernal/server/handler.go
| // meta maps criteria value strings to their time metadata (volatile config only). | ||
| type Criteria struct { | ||
| digestItems map[string][]string | ||
| componentItems map[string][]string | ||
| defaultItems []string | ||
| meta map[string]criteriaItemMeta |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve metadata per scoped criterion, not per value.
c.meta[value] = m overwrites metadata when the same value is reused—for example, a permanent Config.Exclude{"pkg.rule"} plus a volatile exclusion, or component-specific entries with different expiries. Downstream reporting may then claim the wrong expiry, including that a permanent exclusion expires.
Key metadata by full criterion identity/scope and carry that identity through policy resolution. Add collision tests for static+volatile and multi-component entries.
As per path instructions, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
Also applies to: 90-95, 229-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/evaluator/criteria.go` around lines 41 - 46, Change Criteria.meta
from value-based keys to full scoped criterion identities, preserving separate
metadata for static versus volatile entries and for each component. Update the
criterion construction and policy-resolution paths that read or write meta so
this identity is carried through consistently. Add collision tests covering
static plus volatile criteria and the same value across multiple components,
ensuring reporting retains each entry’s correct expiry.
Source: Path instructions
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Review — approveSummary: This PR surfaces config-excluded rules (both Strengths
Findings[low] Duplicated metadata stamping block ( [low] Potential exception double-counting edge case ( [low] Inconsistent tab alignment in metadata constants ( Previous runReview — PR #3419Surface volatile-excluded rules as time-bounded exceptions in report output SummaryThis PR adds a significant new capability: rules excluded by The approach is architecturally sound and the test coverage is thorough. However, there is one high-severity finding related to a safety invariant change, and several medium-severity issues that should be addressed before merging. Findings🔴 HIGH:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
enhancement
|
🤖 Review · |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/evaluator/filters_test.go`:
- Line 1922: Run the repository’s configured formatter, targeting
internal/evaluator/filters_test.go and correcting the formatting issue around
line 1922, then include the formatter-generated changes in the commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 55bff216-ad8b-48e1-ac55-2865c6f4153b
📒 Files selected for processing (5)
internal/evaluator/base_evaluator.gointernal/evaluator/conftest_evaluator.gointernal/evaluator/filters.gointernal/evaluator/filters_test.gointernal/input/report.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/input/report.go
- internal/evaluator/filters.go
| assert.NotContains(t, exceptions[0].Metadata, metadataEffectiveUntil) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the formatting failure before merge.
gci reports internal/evaluator/filters_test.go as improperly formatted at Line 1922. Run the repository formatter and commit the resulting change.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 1922-1922: File is not properly formatted
(gci)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/evaluator/filters_test.go` at line 1922, Run the repository’s
configured formatter, targeting internal/evaluator/filters_test.go and
correcting the formatting issue around line 1922, then include the
formatter-generated changes in the commit.
Source: Linters/SAST tools
Expose all rule exceptions (config and Rego sources) with effective_on/effective_until timestamps so downstream tooling can represent time-bounded policy waivers instead of silently dropping volatile exclusions. Consumers now see the complete policy picture. The totalRules invariant is preserved by excluding exceptions from the evaluated count and using a separate config-exclusion counter to detect misconfiguration. Assisted-by: Claude Code (Sonnet 4.6)
6aff090 to
353c0b9
Compare
|
🤖 Review · ❌ Terminated · Started 6:21 PM UTC · Ended 6:28 PM UTC |
Superseded by updated review
| configExceptions = append(configExceptions, result) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] correctness
CategorizeResults unconditionally appends originalResult.Exceptions. Combined with the new configExceptions append in callers, a Rego-level exception whose code also matches a config exclusion would appear twice in result.Exceptions. The overlap scenario is unlikely but possible.
Suggested fix: In follow-up, either skip originalResult.Exceptions entries whose codes already appear in configExceptions, or filter them in CategorizeResults when config exceptions are present.
| metadataSeverity = "severity" | ||
| metadataEffectiveOn = "effective_on" | ||
| metadataSolution = "solution" | ||
| metadataEffectiveOn = "effective_on" |
There was a problem hiding this comment.
[low] style
The new metadataEffectiveUntil and metadataSolution entries use wider tab alignment than metadataTerm and metadataTitle, creating visual inconsistency in the const block.
Suggested fix: Align all entries in the const block to the same tab stop.
fullsend-ai-review
Bot
added
the
ready-for-merge
1 participant
Summary
Exceptions []evaluator.Result \json:"exceptions,omitempty"`to theInputstruct ininternal/input/report.go`, so volatile-excluded rules appear in the JSON report rather than silently disappearingeffective_untilinkeepSomeMetadataSinglealongside the existingeffective_onandtermkeysVolatileConfigasOutcome.Exceptionsentries witheffective_onandeffective_untiltimestamps stamped from theVolatileCriteriaCRD fieldstotalRulesaccounting in bothbase_evaluator.goandconftest_evaluator.goto count exceptions, preventing a spurious "no rules checked" error for all-excluded policiesIncludeExcludePolicyResolver) and conftest evaluator pathsMotivation
Downstream tooling — in particular SVR (Simple Verification Result) attestation generators — needs to represent time-bounded policy exceptions rather than treat excluded rules as if they were never evaluated. Previously, a rule excluded via
VolatileConfigdisappeared entirely from the output: it appeared in neitherviolations,warnings, norsuccesses. Consumers had no way to distinguish "this rule passed" from "this rule was temporarily waived."With this change, excluded rules surface in
exceptionswitheffective_onandeffective_untilmetadata, enabling consumers to apply the SVR time-bounded property pattern to represent waivers that expire automatically.Test plan
go test -tags=unit ./internal/evaluator/... ./internal/output/... ./internal/input/...(348 pass)go test -tags=integration ./internal/evaluator/...(36 pass, including 5 new subtests covering volatile exclusion surfacing, metadata stamping, and component-scoped exclusions witheffective_on/effective_untilassertions)TestConftestEvaluatorIncludeExcludeupdated to reflect the new behavior (excluded results now appear inExceptionsrather than being dropped)🤖 Generated with Claude Code