Skip to content

fix: prevents paanic on violations as array#86

Open
gusfcarvalho wants to merge 3 commits into
mainfrom
gc-fix-no-panic-on-violation-array
Open

fix: prevents paanic on violations as array#86
gusfcarvalho wants to merge 3 commits into
mainfrom
gc-fix-no-panic-on-violation-array

Conversation

@gusfcarvalho
Copy link
Copy Markdown
Contributor

@gusfcarvalho gusfcarvalho commented May 27, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Better handling of unexpected policy output shapes: returns clear, contextual errors instead of crashes and no longer emits debug output.
    • Decoding failures now surface as structured errors including policy context.
  • Tests

    • Added test coverage for policies that emit violations as a set to ensure correct decoding and single-violation results.

Review Change Stack

Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e9fd70c-8924-4c17-8961-b21fdbdae2b7

📥 Commits

Reviewing files that changed from the base of the PR and between 6364dae and 4218519.

📒 Files selected for processing (2)
  • policy-manager/policy-manager.go
  • policy-manager/policy-manager_test.go

📝 Walkthrough

Walkthrough

PolicyManager.Execute now validates expression output types, normalizes violation results from object or set shapes into JSON entries, decodes them into Violation objects with contextual errors, and avoids panics. A new test verifies handling of set-defined violations.

Changes

Policy Evaluation Error Handling

Layer / File(s) Summary
Normalize violation entries
policy-manager/policy-manager.go
Adds normalizeViolationEntries to convert violation results from object (map[string]interface{}) or set ([]interface{}) shapes into uniform JSON-encoded entries.
Validate module outputs and unmarshal violations
policy-manager/policy-manager.go
PolicyManager.Execute now requires expression Value to be map[string]interface{} before treating it as module outputs; it uses normalized entries to unmarshal each Violation, returning errors that include policy package and file on failures.
Return structured decode errors
policy-manager/policy-manager.go
Decoding module outputs into EvalOutput no longer panics or prints debug output; it returns errors that wrap mapstructure.Decode failures and include policy package/file context.
Test: supports violations declared as a set
policy-manager/policy-manager_test.go
Adds a subtest defining an inline Rego policy where violation is produced as a set; executes PolicyManager.Execute with violating input and asserts a single parsed Violation with expected fields.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble bytes beneath the log, 🐇
I sort the shapes that once were fog,
No panic now when types misbehave,
Just tidy errors, calm and brave.
Tests hop forward — peace in the code.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title mentions preventing panic on violations as array, which directly matches the main change of refactoring violation decoding to support array form.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@policy-manager/policy-manager_test.go`:
- Line 103: Replace the redundant explicit type declaration for the test
variable `data` with Go's short variable declaration so the type is inferred
from `make(map[string]interface{})`; locate the `data` declaration in
policy-manager_test.go (currently `var data map[string]interface{} =
make(map[string]interface{})`) and change it to use `:=` to satisfy ST1023 and
simplify the code.

In `@policy-manager/policy-manager.go`:
- Around line 183-190: The error message in the violations type-check block
(where you do val.(map[string]interface{}) into violationsMap) is misleading
because it assumes val is a slice; update the error to report the actual runtime
type or a more generic message: include fmt.Sprintf("got %T", val) (or change to
"expected violations to be a map/object") and keep the existing context
(result.Policy.Package, result.Policy.File) so the error reads accurately for
violationsMap, val, result.Policy.Package and result.Policy.File.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: 21ec03db-aa57-485f-83f5-a58c8e44165c

📥 Commits

Reviewing files that changed from the base of the PR and between bf76279 and 2fdade0.

📒 Files selected for processing (2)
  • policy-manager/policy-manager.go
  • policy-manager/policy-manager_test.go

Comment thread policy-manager/policy-manager_test.go Outdated
Comment thread policy-manager/policy-manager.go Outdated
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@policy-manager/policy-manager.go`:
- Around line 187-189: The json.Unmarshal error returned for the violation
decode should be wrapped with context so failures are diagnosable; instead of
returning the raw err from json.Unmarshal([]byte(violation), viol) update the
error return to include identifying context (e.g., policy ID/name and
file/package) — reference the variables json.Unmarshal, violation and viol in
policy-manager.go and wrap the error with fmt.Errorf or errors.Wrapf (e.g.,
"failed to unmarshal violation for policy %s: %w", policyID, err) and add the
required import if missing.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: e0f700ff-2b2f-410f-8af9-d854f0c0de48

📥 Commits

Reviewing files that changed from the base of the PR and between 2fdade0 and 6364dae.

📒 Files selected for processing (2)
  • policy-manager/policy-manager.go
  • policy-manager/policy-manager_test.go

Comment thread policy-manager/policy-manager.go Outdated
fix: normalize violations
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Actionable comments posted: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant